proto: treat bad wire types as unknown fields #511
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
Previously, an error was returned during unmarshal when a wiretype was encountered that did not match the expected type. In order to match the behavior of the C++ and Python implementations, we no longer return an error and instead store the bad wire fragment as an unknown field (or skip them if unknown field preservation is disabled). The generator still produces code that references ErrInternalBadWireType for unmarshal logic for oneof fields. However, the current proto package does not use the generated unmarshalers for oneofs, so their existence has no bearing on unmarshal semantics. Cleaning up the generator to stop producing these is future work.
dsnet
added a commit
that referenced
this pull request
Mar 20, 2018
A prior changed (see #511) allows Unmarshal to treat fields with unknown wire types with known tags as unknown fields. When do so, we must be careful not to set the required field bit. For example, suppose we have: message Foo { require fixed32 my_field = 1; } Then parsing a message with a field of type varint and tag 1 should not satisfy the required that Foo.my_field be set.
dsnet
added a commit
that referenced
this pull request
Mar 21, 2018
A prior changed (see #511) allows Unmarshal to treat fields with unknown wire types with known tags as unknown fields. When do so, we must be careful not to set the required field bit. For example, suppose we have: message Foo { require fixed32 my_field = 1; } Then parsing a message with a field of type varint and tag 1 should not satisfy the required that Foo.my_field be set.
dsnet
added a commit
that referenced
this pull request
Mar 28, 2018
A prior changed (see #511) allows Unmarshal to treat fields with unknown wire types with known tags as unknown fields. When do so, we must be careful not to set the required field bit. For example, suppose we have: message Foo { require fixed32 my_field = 1; } Then parsing a message with a field of type varint and tag 1 should not satisfy the required that Foo.my_field be set.
dsnet
added a commit
that referenced
this pull request
Mar 28, 2018
A prior changed (see #511) allows Unmarshal to treat fields with unknown wire types with known tags as unknown fields. When doing so, we must be careful not to set the required field bit. For example, suppose we have: message Foo { require fixed32 my_field = 1; } Then parsing a message with a field of type varint and tag 1 should not satisfy the required that Foo.my_field be set.
|
@dsnet just curious. Do you know why the original protobuf implementation treats incompatible wiretypes as unknowns? is it to help preserve compatibility between messages that might change wiretypes? |
|
This is the behavior of the C++ implementation, which has become the de-facto "standard" of "correct" behavior. The rationale for this property is not documented to my knowledge. |
|
thank you for the prompt response! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Previously, an error was returned during unmarshal when a wiretype
was encountered that did not match the expected wiretype.
In order to match the behavior of the C++ and Python implementations,
we no longer return an error and instead store the bad wire fragment
as an unknown field (or skip them if unknown field preservation is disabled).
The generator still produces code that references ErrInternalBadWireType
for the unmarshal logic for oneof fields.
However, the current proto package does not use the generated unmarshalers
for oneofs, so their existence has no bearing on unmarshal semantics.
Cleaning up the generator to stop producing these is future work.