Update JSON unknown field support #771
Conversation
These were actually unknown field name getting skipped and not real test of setting the field to null.
According to the spec: https://developers.google.com/protocol-buffers/docs/proto3#json_options Unknown field name encountered while parsing JSON should cause errors, but language can provide a option to allow those fields to be skipped instead. We had it backwards, and always skipped them with no way to get an error. - Add an option to ignore unknown fields during JSON decode. - Add a JSON Decoding Error to indicated when an unknown field was encountered. - Wire up the option and new error. - Update the existing tests get the behaviour they arelady required around unknown fields in JSON.
- Ensure the unknown field stop parsing (so any other errors don't show.) - Ensure ensure the unknown field name is reported.
|
Given we had the behavior wrong compared to the spec, should we move to 1.1.x to help call out the behavior change? |
|
The first commit in there is a bug fix in the existing tests. They thought they were doing one thing, but the actually were dependent on ignoring unknown fields. Sadly we have a few too many failure tests that just see if things fail, but don't really check what error they fail with to confirm they are testing what we think they are. |
My vote is to do this just incase someone is/was dependent on it. |
| @@ -55,4 +55,8 @@ public enum JSONDecodingError: Error { | |||
| case conflictingOneOf | |||
| /// Reached the nesting limit for messages within messages while decoding. | |||
| case messageDepthLimit | |||
| /// Encountered a unknown field with the given name. When parsing JSON, you | |||
There was a problem hiding this comment.
s/Encountered a/Encountered an/
| @@ -55,4 +55,8 @@ public enum JSONDecodingError: Error { | |||
| case conflictingOneOf | |||
| /// Reached the nesting limit for messages within messages while decoding. | |||
| case messageDepthLimit | |||
| /// Encountered a unknown field with the given name. When parsing JSON, you | |||
| /// can instead instructe the library to ignore this via | |||
| @@ -21,5 +21,9 @@ public struct JSONDecodingOptions { | |||
| /// while parsing. | |||
| public var messageDepthLimit: Int = 100 | |||
|
|
|||
| /// If unknown fields in the JSON should be ignored. If they aren't | |||
| /// ignored, and error will be raised if one is encountered. | |||
|
Regarding version number, I agree, let's bump it. |
Since we are adding a minor behavior change around unknown field names in JSON parsing, bump the minor version to help call out this subtle change in behavior.
|
Add a commit to pump the version also. |
| @@ -21,5 +21,9 @@ public struct JSONDecodingOptions { | |||
| /// while parsing. | |||
| public var messageDepthLimit: Int = 100 | |||
|
|
|||
| /// If unknown fields in the JSON should be ignored. If they aren't | |||
| /// ignored, an error will be raised if one is encountered. | |||
| public var ignoreUnknownFields: Bool = false | |||
There was a problem hiding this comment.
Just out of curiosity, why is the default value for this option false?
I ask because it seems more natural to set this to true, to me, as someone who uses protobuf to help manage version compatibility across various components.
There was a problem hiding this comment.
I also had the pointer in the second commit on the PR, but it is in the spec –
Spec: https://developers.google.com/protocol-buffers/docs/proto3#json_options
Ignore unknown fields: Proto3 JSON parser should reject unknown fields by default but may provide an option to ignore unknown fields in parsing.
With the binary format, the unknown fields can be parsed and preserved to be reflected back out. But for the JSON format, there is no way to know if something was an int32, int64, uint32, etc., so while it could be reflected back out in JSON, you can't convert it or even expose it via a reflection (if you language has proto reflection).
There was a problem hiding this comment.
Also, since the "ignore" is destructive, it really shouldn't be the default since it means by default we lose things we can't deal with. If we tried to capture the text and reflect it out for JSON, maybe it would make sense, but the fact that a binary conversion would loose things is also bad.
There is a similar issue with enums, if you get a value string you don't know, there is no way to keep it. They also offer up that folks could provide an encoding option to use number, but we haven't added that.
There was a problem hiding this comment.
Thanks, that makes a lot of sense. I am not accustomed to using JSON with proto aside from debugging.
Fixes #769