Support absent fields in mutable structs #144
Conversation
| @@ -149,18 +154,25 @@ export class MessageReader<T = unknown> { | |||
| ): unknown { | |||
| let emHeaderSizeBytes; | |||
| if (emHeaderOptions.readMemberHeader) { | |||
| const { id, objectSize: objectSizeBytes, lengthCode } = reader.emHeader(); | |||
| emHeaderSizeBytes = useEmHeaderAsLength(lengthCode) ? objectSizeBytes : undefined; | |||
| if (this.#unusedEmHeader?.readSentinelHeader === true) { | |||
There was a problem hiding this comment.
could use a comment to explain what is going on
| /** Holds the return value of the previously unused emHeader. | ||
| * emHeaders remain unused if their return ID does not match the field ID. | ||
| * They become used if a field is encountered that uses the unusedEmHeader.id or | ||
| * if the unusedEmheader is a sentinel header. | ||
| **/ |
There was a problem hiding this comment.
Is this a behavior described by the spec? If so maybe reference a relevant section/paragraph?
There was a problem hiding this comment.
It is not defined in the spec, just a quirk of our architecture 🤷
| // clear emHeader for aggregated type, since if it was defined, it would've likely been used | ||
| // as the sentinel header | ||
| this.#unusedEmHeader = undefined; |
There was a problem hiding this comment.
not totally sure I understand why this is needed – so in lieu of understanding it I will ask: will one of your tests fail if it is removed?
There was a problem hiding this comment.
Yes. If it is removed an inner struct's sentinel header(representing the end of a struct), if encountered instead of an emHeader due to an absent final field(s) within the inner struct, will not be cleared, causing subsequent fields read in the outer struct to return undefined because the sentinel header is still stored in unusedEmHeader. This would prevent further reading of the message at all essentially.
There was a problem hiding this comment.
I updated the comment to hopefully be more clear
|
Something weird happened with this commit and most of the changes are already on main 🤔 . I'm going to merge this to fix |
Absent fields in structs do not write emHeaders, so the reading of the emHeader for the field will either return a future field's emHeader or a sentinelHeader. To allow for this, our reader stores the emHeader it reads for absent members and keeps them until it reads a field that is present or the struct ends.
Depends on: foxglove/cdr#24
In this PR, we only return
undefinedfor these fields, however the XCDR spec states that non-optional members should return default values for their types specified here:In the spirit of incremental delivery, this will be a followup PR and ticket.
Addresses: FG-6436