Support Objects in octet-stream codec #1099

[derwehr]

See #1099

As a first step, I added ex:bitLength and ex:bitOffset to all methods of the codec and extended the tests accordingly.

I'll add support for the object schema type next. But because I already changed a lot of code, there might be some material for discussion.

[codecov]

Codecov Report

Attention: 38 lines in your changes are missing coverage. Please review.

Comparison is base (6c88392) 76.33% compared to head (28b135c) 77.36%.
Report is 14 commits behind head on master.

Files	Patch %	Lines
packages/core/src/codecs/octetstream-codec.ts	89.70%	32 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1125      +/-   ##
==========================================
+ Coverage   76.33%   77.36%   +1.02%     
==========================================
  Files          80       80              
  Lines       16829    17108     +279     
  Branches     1615     1723     +108     
==========================================
+ Hits        12847    13236     +389     
+ Misses       3952     3837     -115     
- Partials       30       35       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[derwehr]

I add a few more test cases to increase coverage
Would you like me to move the new features to another codec instead of extending octet-steam, or is there anything else I can change to possibly get this merged?

[danielpeintner]

I think it is definitely an improvement since we now support objects (once a schema is provided for it)

Note: I only did a quick review but it looks good to me and the additional tests are useful also!

Let's wait till @relu91 or @JKRhb have time to take a look.

BTW, Sorry for not reacting for such a long time

[derwehr]

Thanks for the review.
By the way, one notable change I made is that I moved byteSeq, signed and length from the URI parameters into the data schema.
I'm unsure if you'd want that, but it seemed more natural. On second thought, that's a deprecation of the parameters, and maybe I should re-add them along with a check making sure they don't differ from the parameters provided by the data schema.
Please let me know which way you'd prefer.

[danielpeintner]

By the way, one notable change I made is that I moved byteSeq, signed and length from the URI parameters into the data schema.
I'm unsure if you'd want that, but it seemed more natural. On second thought, that's a deprecation of the parameters, and maybe I should re-add them along with a check making sure they don't differ from the parameters provided by the data schema.

@relu91 @egekorkan @JKRhb do you have an opinion on that aspect?

I think the advantage of having these parameters as URI means one can tweak them at runtime while in the schema case they are fixed. I would even argue that they can be at both places and the URI parameters takes precedence if available.. but I guess this makes it more complex.

[egekorkan]

@relu91 @egekorkan @JKRhb do you have an opinion on that aspect?

Sadly none of the direction seems to be direction that the standards are developing towards. We will probably have protocol specific keywords for that but I can see why you would want to have these protocol independent since data encoding/coding happens in codec in node-wot, which is not aware of the protocol. I would definitely wait for @relu91 before merging.

[relu91]

Hi @derwehr! 👋
First of all, thank you very much for your efforts!! I like the idea of improving the octet-stream codec with the ability to encode/decode objects. Sorry for all the friction, but this is a delicate topic because it is a moving target. The problem is that the Thing Description and Protocol binding task forces (part of the WoT WG) are discussing, these days, how to improve the mapping between IoT protocol payloads to application data. In practice, in the short future, we might need to change how payloads are processed in node-wot.

Having said that we discussed it internally and decided that for now, it is good to merge this PR and leave the refactoring for later. Before merging though I might ask you one final job. Let me clarify one point. byteSeq, signed, and length were not URI parameters but rather content-type parameters. As the RFC specifies, content-types might have parameters, and third argument of the bytesToValue and valueToBytes functions is the content-type parameters parsed by the upper layer. See here. Therefore, I would ask you if it is possible to revert the change you made and keep those in the content-type parameters.

Now, I'll do a review of the rest of your implementation and let you know if there is something else to change before merging.

[relu91]

I skim through the code but it is hard to visually remove the changes that you did for the parameters. I left a couple of comments, but I think I review the code once again when you revert back to the previous handling of content-type parameters. Thanks!

[relu91]

Thank you very, much for your efforts! We can merge it and try to play with it. As a follow-up work, we should provide a short explanation about the new terms introduced.

[danielpeintner]

Thank you @derwehr for your contribution 👍

I created #1194 as follow-up.