Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a callback to jsonpb.Unmarshaler to report unknown fields. #415

Conversation

jmillikin-stripe
Copy link

This is useful when parsing JSON that is expected to exactly match a
protobuf schema, but is permitted to drift slightly. In this case
unknown fields can be reported to higher-level code to decide whether
the parse succeeded.

This is useful when parsing JSON that is expected to exactly match a
protobuf schema, but is _permitted_ to drift slightly. In this case
unknown fields can be reported to higher-level code to decide whether
the parse succeeded.
@dsnet
Copy link
Member

dsnet commented Sep 15, 2017

\cc @cybrcodr

Related to golang/go#15314

@cybrcodr
Copy link
Contributor

Here are my thoughts on this PR. This requires that user sets AllowUnknownField=true for the handler to be called. This means if user sets the handler and didn't set AllowUnknownField to true, nothing happens. I talked to @dsnet regarding this and we have differing opinions on it.

Also, the handler you're proposing is simply given the field name. I'd expect that it would be more useful if it is allowed to do more with the input value, and hence I'd also suggest naming it as ResolveUnknownField or HandleUnknownField, then again perhaps it can be an interface similar to AnyResolver.

Anyways, the package does already allow for custom unmarshaling of message if the message implements jsonpb.UnmarshalJSONPB. This means that the custom unmarshaler can possibly detect for unknown fields. I understand that it may require a bit more work on your end, but it also allows us time to think of how to handle this situation more appropriately.

@dsnet
Copy link
Member

dsnet commented Jan 11, 2018

The JSON<->PB specification explicitly says that handling of unknown fields is failure:

JSON conversion requires type information in both directions (from proto to JSON and from JSON to proto). If there are unknown fields, there is no way to preserve them and the conversion should fail.

Thus, unless the protobuf team changes their opinion about unknown JSON fields, we're not going to deviate in behavior from the other languages.

@dsnet dsnet closed this Jan 11, 2018
@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants