Conformance test: JSON parsing of unknown enum string values #9534

antongrbin · 2022-02-23T10:04:51Z

Motivation

Different clients yield different results when JSON parsing unknown enum string values:

java, cpp will ignore the unknown enum string value when ignoreUnknown is set
python, swift, c# will fail although ignoreUnknown is set

I am proposing a new conformance test with the hope to align the implementations. The expected behavior I am proposing is the one followed by cpp and java already. This is also aligned with binary serialization behavior for unknown enum values.

More details in the issue:
#7392

To keep the PR and discussion small, we leave the unknown enum integer values out of scope. For integer values, the difference between proto2 and proto3 comes into discussion, which makes it more complex.

Related PRs and issues

Related issues/PR in the official repo:

ISSUE Need specification for how to parse unrecognized enums in JSON #7392
- Asks the protobuf team for the specification in this situation.
- dupe Invalid enum value in JSON conversion when adding new enum value #8244
ISSUE JsonFormat.Parser throws InvalidProtocolBufferException when parsing unknown enum constants #3012
- original Java issue, fixed now
- PR Ignore unknown enum received in json when ignoreUnknownFields flag is set #4825
  - original fix for Java
PR JsonParser.withIgnoreUnknownFields true will now ignore unknown enums. #7974
- c# fix attempt
PR Optionally ignore unknown enum string values in the JSON parser #7768
- c# fix attempt

Related PRs in other external protobuf implementations:

PR json: Ignore unknown enum values timostamm/protobuf-ts#170
- fix for the typescript library
PR Fix JSON parsing of unknown enum values streem/pbandk#100
- fix for Kotlin implementation
ISSUE Issue parsing unknown enum case apple/swift-protobuf#972
- swift issue
- PR Add an option to ignore unknown enum cases when decoding from JSON (instead of failing) apple/swift-protobuf#896
  - attempt to fix for Swift-> lesson learned: map is a thing

antongrbin · 2022-02-23T10:11:01Z

As explained in #8358, I don't have permissions to add a label.
I would like to add label conformance tests.

elharo · 2022-02-23T13:04:30Z

Thanks. This is useful. I think we need a clear decision on the correct behavior in #7392 before we can proceed though.

mcy

I would agree that this is the most reasonable behavior for "ignore unknown". I will comment on the related issue on this...

I am working on trying to nail down all this stuff in a forthcoming spec, which should hopefully, help clarify things.

mcy · 2022-09-13T15:46:29Z

conformance/binary_json_conformance_suite.cc

+    string input_json;
+  };
+  const std::vector<TestCase> test_cases = {
+    {"InOptionalField", R"({ "optional_nested_enum": "UNKNOWN_ENUM_VALUE" })"},


Can you mark these raw strings as R"json(, and wrap them so that they span multiple lines like e.g.

R"json({ "foo": "bar" })json"

TIL, thank you!

antongrbin · 2022-10-14T12:47:53Z

@mcy thank you for the approval! Due to a typo we need another kokoro:run to see if any failure list needs updating.

I would agree that this is the most reasonable behavior for "ignore unknown". I will comment on the related issue on this...

Did you end up commenting about this? I couldn't find your comment, please link :)

cassianomonteiro · 2022-10-27T15:36:53Z

@antongrbin @mcy Looks like the checks got stuck, would you be able to take a look? We're really looking forward to get this merged so we can move forward with apple/swift-protobuf#972 (comment).

Thanks!

antongrbin · 2022-10-27T16:09:35Z

I don't have permissions to trigger the checks (first time contributor here).

@cassianomonteiro I am also looking forward to get this merged. My team is running a forked version of Python and SWIFT for the past few quarters and I am looking forward to contributing the fixes upstream and removing the fork.

@mcy is there anything blocking us from merging this?

mkruskal-google · 2022-11-03T18:57:34Z

Anton,

Can you please rebase to the tip of main? That will allow us to qualify this against google internal code

antongrbin · 2022-11-04T10:12:00Z

Can you please rebase to the tip of main? That will allow us to qualify this against google internal code

@mkruskal-google thank you for your response!

Rebased!

cassianomonteiro · 2022-11-10T15:23:13Z

@mkruskal-google Hi, would you be able to move this forward? Thanks!

antongrbin · 2022-11-14T11:17:44Z

@mcy @mkruskal-google it's been 10 days since my last rebase so I'm rebasing again. GitHub automatically assigned @acozzette.

Can we get clarity on the next steps here?

cassianomonteiro · 2022-11-14T15:13:52Z

@mcy @mkruskal-google it's been 10 days since my last rebase so I'm rebasing again. GitHub automatically assigned @acozzette.

Can we get clarity on the next steps here?

+1!

Thanks @antongrbin !

mkruskal-google · 2022-11-14T21:18:46Z

Hey all,

Sorry for the delay! Both of us got sick and this fell through the cracks. I'll rerun tests and qualify it internally before submitting

cassianomonteiro · 2022-11-14T21:27:46Z

Hey all,

Sorry for the delay! Both of us got sick and this fell through the cracks. I'll rerun tests and qualify it internally before submitting

Awesome, thanks!

mkruskal-google · 2022-11-15T00:35:23Z

Update: This has been imported and looks good to go. Just waiting on Miguel to confirm the internal changes look ok

-- 064b263 by Anton Grbin <[email protected]>: squash and rebase COPYBARA_INTEGRATE_REVIEW=#9534 from noom:anton/MIN-2347/json_conformance_unknown_fields 064b263 PiperOrigin-RevId: 488667851

mkruskal-google · 2022-11-15T19:10:08Z

conformance/failure_list_cpp.txt

+Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput
+Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput


Why were these added as failures? What's the expected behavior here? From the PR description it sounds like C++ and Java were implemented "correctly", so it's a little strange that two of the added tests are failing for C++

I think this was committed already and is causing presubmits to fail since these indeed shouldn't be in the failure list: e.g. https://github.com/protocolbuffers/upb/actions/runs/3473202608/jobs/5804957297

It was, I'm trying to retroactively understand the goal here. We can roll it back if it's causing issues, but it looks like you solved it by bumping upb's protobuf version

(sorry, i was actually wrong above -- the presubmit failure was for upb and wasn't from the cpp failure list)

Apologies for the breakage!

Initially I was considering unknown enum string value in optional fields. That case works for C++ as expected, the unknown value is ignored.

When the unknown enum string value is encountered in a repeated field or a map, the C++ shows unexpected behavior too. It fails parsing the whole message although ignoreUnknownFields is set. This is inconsistent with Java, csharp, objc, but more importantly it breaks forward compatibility for enum evolution.

@zhangskz, based on your latest message I suspect you already executed the following steps to debug upb failure, but to over-communicate adding it here.

I can reproduce the failure you linked above link locally. The error message is:

These tests were listed in the failure list, but they don't exist: Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput

It looks like the conformance test runner and the failure list fell out of sync:

conformance test runner was was taken from d938afd (defined in bazel/workspace_deps.bzl)

failure list is taken from 064b263 (copied to upb/conformance_upb_failures.txt)

There is an open PR that is supposed to fix this: protocolbuffers/upb#938. That PR is held back due to a python test failure that looks unrelated to the conformance test that we added.

This updates all generated code to match the contents of the latest v22.0 release of Protobuf. This involved a couple of changes to the script that does the sync'ing: 1. The new Protobuf version no longer includes autoconf configuration and instead requires using bazel to build things. 2. The new Protobuf release does not have an artifact named "protobuf-all-${VERSION}.tar.gz", but the one named "protobuf-${VERSION}.tar.gz" has all of the sources and was sufficient for the regenerate.bash script to complete. This change does NOT regenerate the protos related to benchmarks. The Protobuf repo no longer includes benchmarks. The CL removing them says they are superceded by google/fleetbench: protocolbuffers/protobuf@83c499d But that project's proto benchmark files are very different: https://github.com/google/fleetbench/tree/main/fleetbench/proto So I commented out those steps in the generation code since the benchmarks will need some work to reconcile with fleetbench. This code adds known failing tests for conformance. New test cases were added in protocolbuffers/protobuf#9534, but the Go protojson package does not behave according to the new tests. There is an existing issue in GitHub about this: golang/protobuf#1208 Change-Id: Iad796ec7889bc2a74b58db5224facf850cd1a1cd Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/469255 Reviewed-by: Michael Stapelberg <[email protected]> Reviewed-by: Damien Neil <[email protected]>

antongrbin mentioned this pull request Feb 23, 2022

[MIN-2347] (early draft, testing the current state) Implementing Enum schema evolution #9334

Closed

antongrbin marked this pull request as ready for review February 23, 2022 10:07

antongrbin changed the title ~~Conformance test for JSON behaviour when parsing unknown enum string values~~ Conformance test: JSON behavior when parsing unknown enum string values Feb 23, 2022

antongrbin changed the title ~~Conformance test: JSON behavior when parsing unknown enum string values~~ Conformance test: JSON parsing of unknown enum string values Feb 23, 2022

antongrbin mentioned this pull request Feb 23, 2022

Need specification for how to parse unrecognized enums in JSON #7392

Closed

antongrbin force-pushed the anton/MIN-2347/json_conformance_unknown_fields branch from f0e0866 to 675d781 Compare February 23, 2022 10:42

elharo added json syntax specification labels Feb 23, 2022

acozzette added release notes: no labels Feb 23, 2022

protobuf-kokoro removed the kokoro:run label Feb 23, 2022

fowles added the conformance tests label Feb 23, 2022

acozzette added the kokoro:run label Feb 24, 2022

protobuf-kokoro removed the kokoro:run label Feb 24, 2022

esorot added the kokoro:force-run label Jun 29, 2022

protobuf-kokoro removed the kokoro:force-run label Jun 29, 2022

fowles requested a review from mcy August 3, 2022 14:21

mcy reviewed Sep 13, 2022

View reviewed changes

googleberg assigned mcy Oct 13, 2022

mcy approved these changes Oct 13, 2022

View reviewed changes

mcy added the kokoro:run label Oct 13, 2022

protobuf-kokoro removed the kokoro:run label Oct 13, 2022

cassianomonteiro mentioned this pull request Oct 24, 2022

Issue parsing unknown enum case apple/swift-protobuf#972

Closed

antongrbin force-pushed the anton/MIN-2347/json_conformance_unknown_fields branch from 4e0a67a to d925ffa Compare November 4, 2022 10:10

squash and rebase

064b263

antongrbin force-pushed the anton/MIN-2347/json_conformance_unknown_fields branch from d925ffa to 064b263 Compare November 14, 2022 11:06

antongrbin requested a review from a team as a code owner November 14, 2022 11:06

antongrbin requested review from acozzette and removed request for a team November 14, 2022 11:06

mkruskal-google added the kokoro:run label Nov 14, 2022

protobuf-kokoro removed the kokoro:run label Nov 14, 2022

mkruskal-google added copybara-kick and removed copybara-kick labels Nov 14, 2022

mkruskal-google self-requested a review November 14, 2022 23:57

mkruskal-google approved these changes Nov 14, 2022

View reviewed changes

mkruskal-google closed this Nov 15, 2022

mkruskal-google reviewed Nov 15, 2022

View reviewed changes

mkruskal-google reopened this Nov 15, 2022

mkruskal-google closed this Nov 17, 2022

jhump mentioned this pull request Feb 17, 2023

encoding/protojson: DiscardUnknown doesn't work for unknown enum strings golang/protobuf#1208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conformance test: JSON parsing of unknown enum string values #9534

Conformance test: JSON parsing of unknown enum string values #9534

antongrbin commented Feb 23, 2022 •

edited

Loading

antongrbin commented Feb 23, 2022 •

edited

Loading

elharo commented Feb 23, 2022

mcy left a comment •

edited

Loading

mcy Sep 13, 2022

antongrbin Sep 13, 2022

antongrbin commented Oct 14, 2022 •

edited

Loading

cassianomonteiro commented Oct 27, 2022

antongrbin commented Oct 27, 2022

mkruskal-google commented Nov 3, 2022

antongrbin commented Nov 4, 2022

cassianomonteiro commented Nov 10, 2022

antongrbin commented Nov 14, 2022

cassianomonteiro commented Nov 14, 2022

mkruskal-google commented Nov 14, 2022

cassianomonteiro commented Nov 14, 2022

mkruskal-google commented Nov 15, 2022

mkruskal-google Nov 15, 2022

zhangskz Nov 15, 2022

mkruskal-google Nov 15, 2022 •

edited

Loading

zhangskz Nov 15, 2022

antongrbin Nov 16, 2022

antongrbin Nov 16, 2022

		Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput
		Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput

Conformance test: JSON parsing of unknown enum string values #9534

Conformance test: JSON parsing of unknown enum string values #9534

Conversation

antongrbin commented Feb 23, 2022 • edited Loading

Motivation

Related PRs and issues

antongrbin commented Feb 23, 2022 • edited Loading

elharo commented Feb 23, 2022

mcy left a comment • edited Loading

Choose a reason for hiding this comment

mcy Sep 13, 2022

Choose a reason for hiding this comment

antongrbin Sep 13, 2022

Choose a reason for hiding this comment

antongrbin commented Oct 14, 2022 • edited Loading

cassianomonteiro commented Oct 27, 2022

antongrbin commented Oct 27, 2022

mkruskal-google commented Nov 3, 2022

antongrbin commented Nov 4, 2022

cassianomonteiro commented Nov 10, 2022

antongrbin commented Nov 14, 2022

cassianomonteiro commented Nov 14, 2022

mkruskal-google commented Nov 14, 2022

cassianomonteiro commented Nov 14, 2022

mkruskal-google commented Nov 15, 2022

mkruskal-google Nov 15, 2022

Choose a reason for hiding this comment

zhangskz Nov 15, 2022

Choose a reason for hiding this comment

mkruskal-google Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

zhangskz Nov 15, 2022

Choose a reason for hiding this comment

antongrbin Nov 16, 2022

Choose a reason for hiding this comment

antongrbin Nov 16, 2022

Choose a reason for hiding this comment

antongrbin commented Feb 23, 2022 •

edited

Loading

antongrbin commented Feb 23, 2022 •

edited

Loading

mcy left a comment •

edited

Loading

antongrbin commented Oct 14, 2022 •

edited

Loading

mkruskal-google Nov 15, 2022 •

edited

Loading