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

Ignore unknown JSON fields by default #642

Merged
merged 6 commits into from
Jun 13, 2023

Conversation

timostamm
Copy link
Member

When changing a schema, servers are usually updated first. With the default behavior of the JSON format to reject unknown fields, this causes an issue with in simple cases such as adding a field to a response message. #613 is a good example.

This PR changes the default behavior of both clients and servers to ignore unknown fields. It is equivalent to the following option (for a Transport, or for the ConnectRouter):

jsonOptions: { ignoreUnknownFields: true },

After this PR is merged, clients or servers can opt in to reject unknown fields by providing jsonOptions: { ignoreUnknownFields: false }.

Comment on lines +218 to +219
const o = options ?? {};
o.ignoreUnknownFields ??= true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the change. The rest is tests.

@timostamm timostamm requested a review from smaye81 May 17, 2023 19:53
@smaye81
Copy link
Member

smaye81 commented May 17, 2023

Nice. A few small questions:

  1. Do we consider this a breaking change? I'm guessing no, but I can see the argument where it could be.
  2. Should we mention anywhere that this deviates from the actual JSON docs? Or is it possible to work with the Protobuf team to see if ignoring unknown fields should be the default?

Also, we need to update the docs here: https://github.com/bufbuild/protobuf-es/blob/main/docs/runtime_api.md#json

@timostamm
Copy link
Member Author

Do we consider this a breaking change?

Yes, a breaking change. Only if you rely on clients and server to reject new fields. But still a breaking change.

Should we mention anywhere that this deviates from the actual JSON docs?

Yes, I think this docs page fits best.

@russellhaering
Copy link

Just ran into this in production, easy to work around but just wanted to chime in that this seems like a more sensible and expected default.

@arvid220u
Copy link

Agree that this should definitely be the default. One of the biggest reasons for us to use protobufs was to have backwards compatibility, and we just discovered (the hard way) that our production deployment was not backwards-compatible because of using the default options here...

Would be great to see this PR being merged!

@timostamm timostamm merged commit 134772a into main Jun 13, 2023
2 checks passed
@timostamm timostamm deleted the tstamm/ignore-unknown-json-fields-by-default branch June 13, 2023 00:49
@timostamm
Copy link
Member Author

@russellhaering, @arvid220u, thanks for the feedback. This is going out with the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants