Fix encoder/decoder bugs when nested types are encoded as a flattened message#54
Open
fumoboy007 wants to merge 3 commits intohirotakan:masterfrom
Open
Fix encoder/decoder bugs when nested types are encoded as a flattened message#54fumoboy007 wants to merge 3 commits intohirotakan:masterfrom
fumoboy007 wants to merge 3 commits intohirotakan:masterfrom
Conversation
Tested two cases of nested types: 1. Nested type that is flattened in its encoded form. 2. Flattened type that is nested in its encoded form. Note that some tests for the first case are skipped via `XCTSkip` because they currently fail due to bugs in the encoder/decoder.
For a nested type that is flattened during encoding, the outer type and the inner type both create a container at the same nesting level. However, `MessagePackEncoder` does not currently support this use case because it does the encoding when a container is deinitialized. This change allows containers at the same nesting level to share the same underlying storage and defers the encoding until the latest point in the flow.
Currently, `MessagePackDecoder` does the decoding when a caller requests a container. Decoding results are not cached. For a flattened message that is decoded into nested types, the outer type and the inner type both create a container at the same nesting level. For an unkeyed message, the current approach does not work because the unkeyed container returned to the outer type and the inner type need to share the same underlying storage so that the current array index stays consistent. This change allows containers at the same nesting level to share the same underlying storage, which includes the current array index for unkeyed containers. This approach also has the performance benefit of caching the result of the container split.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commits
1. Add tests to verify encoding/decoding nested types.
Tested two cases of nested types:
Note that some tests for the first case are skipped via
XCTSkipbecause they currently fail due to bugs in the encoder/decoder.2. Fix encoder bug when flattening nested types.
For a nested type that is flattened during encoding, the outer type and the inner type both create a container at the same nesting level. However,
MessagePackEncoderdoes not currently support this use case because it does the encoding when a container is deinitialized.This change allows containers at the same nesting level to share the same underlying storage and defers the encoding until the latest point in the flow.
3. Fix decoder bug when unflattening unkeyed message into nested types.
Currently,
MessagePackDecoderdoes the decoding when a caller requests a container. Decoding results are not cached.For a flattened message that is decoded into nested types, the outer type and the inner type both create a container at the same nesting level. For an unkeyed message, the current approach does not work because the unkeyed container returned to the outer type and the inner type need to share the same underlying storage so that the current array index stays consistent.
This change allows containers at the same nesting level to share the same underlying storage, which includes the current array index for unkeyed containers. This approach also has the performance benefit of caching the result of the container split.