-
Notifications
You must be signed in to change notification settings - Fork 88
chore(binding-opcua) fix application/octet-stream encoding/decoding #… #1449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1449 +/- ##
==========================================
- Coverage 77.58% 77.41% -0.17%
==========================================
Files 79 84 +5
Lines 15331 15989 +658
Branches 1445 1519 +74
==========================================
+ Hits 11894 12378 +484
- Misses 3414 3585 +171
- Partials 23 26 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const variantInJson = opcuaJsonEncodeVariant(dataValue.value, false); | ||
| const content = contentSerDes.valueToContent(variantInJson, schemaDataValue, contentType); | ||
| return content; | ||
| } else if (contentType === "application/octet-stream") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the general problem with the current code is that it right away decodes the data on the wire. It should first get the data raw and afterwards the ContentSerdes decodes the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at CoAP for example where the readResource call returns the raw data
node-wot/packages/binding-coap/src/coap-client.ts
Lines 59 to 75 in 8dc6ed8
| public async readResource(form: CoapForm): Promise<Content> { | |
| const req = await this.generateRequest(form, "GET"); | |
| debug(`CoapClient sending ${req.statusCode} to ${form.href}`); | |
| return new Promise<Content>((resolve, reject) => { | |
| req.on("response", (res: ObserveReadStream) => { | |
| debug(`CoapClient received ${res.code} from ${form.href}`); | |
| debug(`CoapClient received Content-Format: ${res.headers["Content-Format"]}`); | |
| // FIXME does not work with blockwise because of node-coap | |
| const contentType = (res.headers["Content-Format"] as string) ?? form.contentType; | |
| resolve(new Content(contentType, Readable.from(res.payload))); | |
| }); | |
| req.on("error", (err: Error) => reject(err)); | |
| req.end(); | |
| }); | |
| } |
Afterwards the configured ContentSerdes deals with the actual content and whether the "byte stream" is JSON, CBOR or anything else..
The same should happen with OPC UA....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current code is that it right away decodes the data
In fact this code is where the encoding takes palce;( not decoding)
|
I tried to test the PR with a simple example and I think it does not work as expected. Maybe I am just not understanding it properly. As mentioned in #1400 OPC-UA seems to work fine until I add Note: FYI, not adding any contentType at all would mean JSON which is not the case for OPC UA I believe. If I add Also, it seems that the according code hasn't changed in that regard. Try to add |
Could you provide an example of things that reproduce what you're doing. That would be easier to grasp . My understanding should be clearly available in the new unit test added. May be I missed your specific case. |
…1400