fix: http server reporting circular structure #513

danielpeintner · 2021-09-17T07:48:24Z

Note: this PR is not meant to be merged as is.

I just run the counter sample and with the current code I get

[binding-http] HttpServer on port 8080 got internal error on read '/counter/properties/count': Converting circular structure to JSON
    --> starting at object with constructor 'ReadableStream'
    |     property '_readableStreamController' -> object with constructor 'ReadableStreamDefaultController'
    --- property '_controlledReadableStream' closes the circle

I tried to fix it but I am not sure whether this is the right way...

danielpeintner · 2021-09-17T07:54:43Z

relates to
https://github.com/eclipse/thingweb.node-wot/blob/59fe3264bb4cfa19a3c8cddb0f1c68b4685a4a47/packages/binding-http/test/http-server-test.ts#L73-L74

relu91

I think we should find a more general approach to solve this issue. One starter is to define an internal content type (e.g., "application/internal" or similar) for values that are passed from the old consumed thing methods in ExposedThing. Later on, we should evaluate a refactoring where the ExposedThing class does not have ConsumedThing methods anymore.

packages/binding-http/src/http-server.ts

packages/core/src/interaction-output.ts

danielpeintner · 2021-09-21T06:55:08Z

packages/binding-http/test/http-server-test.ts

@@ -111,16 +110,16 @@ class HttpServerTest {
        console.log("Testing", uri);

        body = await (await fetch(uri + "properties/test")).text();
-        expect(body).to.equal('"off"');
+        expect(body).to.equal("off");


I am a bit unsure why there was '"off"' before?

add test also

danielpeintner · 2021-09-21T08:49:56Z

packages/binding-http/src/http-server.ts

+                                    .then(async (propMap: WoT.PropertyReadMap) => {
+                                        // Note we create one object to return, TODO piping response
+                                        let recordReponse: Record<string, any> = {};
+                                        for (let key of propMap.keys()) {
+                                            let value: WoT.InteractionOutput = propMap.get(key);
+                                            value.form = { f: "all" }; // to avoid missing form error
+                                            let content = ContentSerdes.get().valueToContent(
+                                                value.data && !value.dataUsed ? value.data : await value.value(),
+                                                undefined,
+                                                "application/json"
+                                            );
+                                            // TODO ProtocolHelpers.readStreamFully() does not work for counter example for SVG countAsImage
+                                            const data = await ProtocolHelpers.readStreamFully(content.body);
+                                            recordReponse[key] = data.toString(); // contentType handling?
+                                        }
+                                        res.setHeader("Content-Type", "application/json"); // contentType handling?


read all properties is very messy... piping does not work for me.

Moreover, ProtocolHelpers.readStreamFully() does not worj for all properties, e.g. SVG image in counter

I agree it is messy, but I think this is something from the spec itself. In all the aggregated operations we are failing to describe how the payload will look like... I remembered that we decided to go with the fact that each property value should be transmitted inside a JSON object similar to what you're doing. However, don't you think is better to just call the value function on each key in propMap and If it fails read the stream? Why are you using valueToContent ?

danielpeintner · 2021-09-21T14:34:59Z

Note: I would like to merge this PR soon so that we continue the work (e.g., #522) and do some more fixes later.

Hence I propose to post-pone:

improve readAllProperties() with piping in HTTP
fixing readAllProperties with contentType (e.g., XML SVG in counter)
content body required? see https://github.com/eclipse/thingweb.node-wot/pull/513/files/0a97293faf602bde03877dab53230529de19b31d#diff-0da1ff0559e462995232565a71f71095469341f25b9b935a590c5803ec357f25

relu91

PR looks good! there are some minor comments to be fixed. I have also another concern but we can fix it in a follow-up PR: I think that the protocol bindings server should not care about content types as much as possible (maybe write/readall is an exception). But in principle, they should just receive the InteractionOuput (already encoded) from the upper level and pipe it to the output stream

relu91 · 2021-09-22T07:06:50Z

packages/binding-http/src/http-server.ts

+                                    .then(async (propMap: WoT.PropertyReadMap) => {
+                                        // Note we create one object to return, TODO piping response
+                                        let recordReponse: Record<string, any> = {};
+                                        for (let key of propMap.keys()) {
+                                            let value: WoT.InteractionOutput = propMap.get(key);
+                                            value.form = { f: "all" }; // to avoid missing form error
+                                            let content = ContentSerdes.get().valueToContent(
+                                                value.data && !value.dataUsed ? value.data : await value.value(),
+                                                undefined,
+                                                "application/json"
+                                            );
+                                            // TODO ProtocolHelpers.readStreamFully() does not work for counter example for SVG countAsImage
+                                            const data = await ProtocolHelpers.readStreamFully(content.body);
+                                            recordReponse[key] = data.toString(); // contentType handling?
+                                        }
+                                        res.setHeader("Content-Type", "application/json"); // contentType handling?


I agree it is messy, but I think this is something from the spec itself. In all the aggregated operations we are failing to describe how the payload will look like... I remembered that we decided to go with the fact that each property value should be transmitted inside a JSON object similar to what you're doing. However, don't you think is better to just call the value function on each key in propMap and If it fails read the stream? Why are you using valueToContent ?

packages/binding-http/src/http-server.ts

packages/core/src/interaction-output.ts

fix: http server reporting circular structure

0d03ef8

danielpeintner marked this pull request as draft September 17, 2021 11:26

relu91 requested changes Sep 20, 2021

View reviewed changes

packages/binding-http/src/http-server.ts Outdated Show resolved Hide resolved

packages/core/src/interaction-output.ts Outdated Show resolved Hide resolved

danielpeintner added 2 commits September 20, 2021 17:00

style: run prettier

f0e0440

fix: circular errors

ee5ee4b

danielpeintner marked this pull request as ready for review September 21, 2021 06:54

danielpeintner commented Sep 21, 2021

View reviewed changes

fix: readAll properties

772f66d

add test also

danielpeintner commented Sep 21, 2021

View reviewed changes

fix: handle action with no input/output

0a97293

danielpeintner mentioned this pull request Sep 21, 2021

CoAP Client not fully working #522

Closed

danielpeintner mentioned this pull request Sep 21, 2021

Commented out test in coap-client-test.ts #523

Closed

relu91 requested changes Sep 22, 2021

View reviewed changes

refactor: move sending into try

1944d7c

danielpeintner mentioned this pull request Sep 22, 2021

Follow-up HTTP fixes/improvements #525

Open

3 tasks

relu91 approved these changes Sep 22, 2021

View reviewed changes

danielpeintner merged commit 1cd63ab into eclipse-thingweb:master Sep 22, 2021

This was referenced Mar 31, 2022

fix(deps): update dependency @node-wot/binding-http to ^0.8.0 nicolasfara/smart-cm#126

Merged

fix(deps): update dependency @node-wot/core to ^0.8.0 nicolasfara/smart-cm#127

Merged

danielpeintner deleted the fix-http-server branch May 24, 2022 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: http server reporting circular structure #513

fix: http server reporting circular structure #513

danielpeintner commented Sep 17, 2021

danielpeintner commented Sep 17, 2021

relu91 left a comment

danielpeintner Sep 21, 2021 •

edited

Loading

danielpeintner Sep 21, 2021

relu91 Sep 22, 2021

danielpeintner commented Sep 21, 2021 •

edited

Loading

relu91 left a comment

relu91 Sep 22, 2021

fix: http server reporting circular structure #513

fix: http server reporting circular structure #513

Conversation

danielpeintner commented Sep 17, 2021

danielpeintner commented Sep 17, 2021

relu91 left a comment

Choose a reason for hiding this comment

danielpeintner Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

danielpeintner Sep 21, 2021

Choose a reason for hiding this comment

relu91 Sep 22, 2021

Choose a reason for hiding this comment

danielpeintner commented Sep 21, 2021 • edited Loading

relu91 left a comment

Choose a reason for hiding this comment

relu91 Sep 22, 2021

Choose a reason for hiding this comment

danielpeintner Sep 21, 2021 •

edited

Loading

danielpeintner commented Sep 21, 2021 •

edited

Loading