[octetsream-codec] fix serialization and deserialization of nested objects #1262
Conversation
derwehr
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1262 +/- ##
==========================================
+ Coverage 77.81% 77.91% +0.10%
==========================================
Files 84 84
Lines 17523 17602 +79
Branches 1781 1810 +29
==========================================
+ Hits 13635 13715 +80
- Misses 3853 3854 +1
+ Partials 35 33 -2 ☔ View full report in Codecov by Sentry. |
| @@ -542,6 +543,7 @@ export default class OctetstreamCodec implements ContentCodec { | |||
| throw new Error("Missing 'length' parameter necessary for write"); | |||
| } | |||
|
|
|||
| const offset = schema["ex:bitOffset"] !== undefined ? parseInt(schema["ex:bitOffset"]) : 0; | |||
There was a problem hiding this comment.
Does it make sense to to a more sophisticated check.. not only for undefined but also NaN and/or invalid parameters?
There was a problem hiding this comment.
I added a check in the last commit.
The bytesToValue and valueTobytes functions could probably check most of the DataSchema values and the length parameter more thoroughly as well.
There was a problem hiding this comment.
If you want to introduce the checks it would be fine by me.. 👍
There was a problem hiding this comment.
I added some more checks (and tests making sure the checks work) in my last commit
There was a problem hiding this comment.
A "simpler" approach could have been a helper method that does the check (since I think the are almost all the same). Anyhow, fine like this as well 👍
After using the octetstream codec's object serialization and deserialization for a while now, I discovered a bug when handling nested objects:
When serializing values, the object's bit-offset would not be passed on to the nested serialization call.
For example (added to the tests in this PR, too):
Serializing an object like
{ "flags1": { "flag1": false, "flag2": true }, "flags2": { "flag1": true, "flag2": false }, }with the following DataSchema
{ "type": "object", "properties": { "flags1": { "type": "object", "properties": { "flag1": { "type": "boolean", "ex:bitOffset": 3, "ex:bitLength": 1 }, "flag2": { "type": "boolean", "ex:bitOffset": 4, "ex:bitLength": 1 }, }, "ex:bitLength": 8, }, "flags2": { "type": "object", "properties": { "flag1": { "type": "boolean", "ex:bitOffset": 3, "ex:bitLength": 1 }, "flag2": { "type": "boolean", "ex:bitOffset": 4, "ex:bitLength": 1 }, }, "ex:bitOffset": 8, "ex:bitLength": 8, }, }, "ex:bitLength": 16, }, "application/octet-stream;length=2;"I would expect:
0000 1000 0001 0000, i.e.Buffer[8, 16], but the current implementation returnsBuffer[24, 0]because it does not passex:bitOffsetofflags2and therefore sets the bits offlags2in the same byte asflags1writing0001 1000 0000 0000.The issue when deserializing nested objects was that
bytesToValuewould only pass the bytes fromoffsettooffset+lengthtoobjectToValuewithout adjusting thelengthparameter. Therefore, deserializing nested objects would throw an error:I added a test case and a fix for this, too.