Support Proto2 extensions in JSON coder/decoder #1002
Merged
Conversation
This file contains 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
This is motivated purely by the existence of a new JSON conformance test. These changes are sufficient to pass that new conformance test.
thomasvl
reviewed
May 15, 2020
|
I'm still filling in tests and tracking down missing bits. I'll look into Any JSON support next. |
thomasvl
reviewed
May 18, 2020
This extends the various Any methods to accept an extensions map. So you can now unpack an Any object holding JSON that contains extensions; you "just" have to provide the appropriate extension map. Note that implicit transcoding from JSON to binary (which occurs when you ask for `.serializedData()` from an Any that was decoded from JSON without explicitly unpacking the JSON contents will always throw if the stored JSON has extensions. I experimented with several alternatives, but this seemed the best. * The Any could remember the extension map that was in effect when it was decoded, then that would be available for later transcoding. This doesn't work for binary -> JSON transcoding (because there is currently no hook that would allow the Any to capture the extensions in effect during the outer binary decode). In addition, this would add another field to Any, increasing memory requirements for a combination of features (Any + JSON + extensions) that I expect will be very rarely used (if ever). * Transcoding JSON -> Binary could silently return empty data, but I don't like silently losing data. * Transcoding JSON -> Binary could ignore extensions, but again I wasn't silently losing data, even in this obscure case.
|
I've filled in support and tests for Any + JSON + extensions, including figuring out a reasonable answer for implicit transcoding (TL;DR: extensions now cause JSON->binary transcoding to throw because none of the other options seemed better). I've filled in additional tests for message-valued extension fields, arrays, and a few other cases. |
thomasvl
reviewed
Jun 3, 2020
… are immutable for the duration of the decode and are more heavily used in the scanner
… extensions argument be optional and default to nil
|
This feels like it's in pretty good shape now. Any concerns before I merge it? |
thomasvl
approved these changes
Jun 4, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is sufficient to pass the conformance check that was recently added, but still needs some additional testing and polish.
Addresses #993