Python JSON parser: Ignore invalid enum string values if ignore_unknown_fields is set #15887

antongrbin · 2024-02-20T11:45:25Z

Motivation

This PR fixes failing conformance tests for python with name IgnoreUnknownEnumStringValue.

The JSON parsing spec was discussed in #7392.

Recent equivalent changes for other languages:

Swift: Use JSONDecodingOptions.ignoreUnknownFields to ignore unknown enums. apple/swift-protobuf#1345
C#: C#: Ignore invalid enum string values in JSON, when invalid fields are being ignored. #15758

Changes

1st commit is a noop refactoring to make relevant _ConvertScalarFieldValue invocations localized
2nd commit introduces the child exception of ParseError named EnumStringValueParseError which is suppressed if ignore_unknown_fields is set
3rd commit updates the conformance test failure lists

antongrbin · 2024-02-22T07:51:03Z

python/google/protobuf/internal/json_format_test.py

+
+  def testParseErrorForUnknownEnumValue_ScalarWithoutIgnore_Proto2(self):
+    message = json_format_pb2.TestNumbers()
+    self.assertRaisesRegexp(


Failure from CI:
https://github.com/protocolbuffers/protobuf/actions/runs/7993753108/job/21830301947?pr=15887#step:4:1822

Fixed by using assertRaisesRegex (without p at the end) which is consistent with the rest of the file.

antongrbin · 2024-02-22T07:51:48Z

src/google/protobuf/util/json_format.proto

@@ -114,3 +115,11 @@ enum EnumValue {
 message TestDefaultEnumValue {
  optional EnumValue enum_value = 1 [default = DEFAULT];
 }
+
+message TestEnumMap {


Failure from CI:

https://github.com/protocolbuffers/protobuf/actions/runs/7993753108/job/21830279412?pr=15887#step:13:3493

https://github.com/protocolbuffers/protobuf/actions/runs/7993753108/job/21830264580?pr=15887#step:3:1046

Fixed by renaming this message.

antongrbin · 2024-02-23T11:43:48Z

Rebased with upstream due to a conflict with an added conformance test (0fa67a9) that we now fixed as well. I used force-push since I don't believe review started already.

antongrbin · 2024-02-29T16:56:57Z

To over-communicate, this is now ready to review!

antongrbin · 2024-03-07T18:11:30Z

@acozzette thank you for your time!

I don't have permissions to merge the PR -- let me know if there are any other next steps for me here.

acozzette · 2024-03-08T19:56:13Z

@antongrbin Sorry for the delay but would you mind rebasing your PR (or merging main into it)? There was a test run added just a few days ago and for technical reasons our sync process can't proceed until it sees that the test is passing.

…on-test

antongrbin · 2024-03-12T11:08:25Z

@acozzette let me know if there is anything I can do with the CI failure for Windows CMake shared:

LINK : fatal error LNK1189: library limit of 65535 objects exceeded
https://github.com/protocolbuffers/protobuf/actions/runs/8216437017/job/22526672782#step:13:2714

acozzette · 2024-03-12T19:47:45Z

Sorry for the hassle but would you mind merging main into your branch one more time? I'm pretty sure that test failure is unrelated to your change, and it might have been fixed on main already.

…on-test

antongrbin · 2024-03-12T20:40:55Z

@acozzette, done -- merged latest main here. No hassle at all -- I understand this is not a simple repo.

acozzette · 2024-03-12T20:56:48Z

Thanks! Unfortunately the failure is still there: LINK : fatal error LNK1189: library limit of 65535 objects exceeded I think this change must somehow be pushing the build just over some limit in the linker in that test run. I also see a lot of warnings that make it sound like we probably have something set up wrong in that build. I will take a look tomorrow and see if I can fix the problem.

antongrbin · 2024-03-18T10:15:43Z

@acozzette friendly ping :) Not that I am blocked on this (we are running a fork with this fix already), but would like to stop thinking about it sooner than later :)

When building with `-Dprotobuf_BUILD_SHARED_LIBS=ON`, we currently put all test protos into their own shared library. PR #15887 seems to be pushing us over a limit on the number of exported symbols so that this library no longer links successfully, though. This change fixes that problem by always building the test protos into a static library. This should be fine since it's purely for testing and not meant to be installed. The only things that depend on it are executables, so we don't need to worry about ODR violations. PiperOrigin-RevId: 617666678

When building with `-Dprotobuf_BUILD_SHARED_LIBS=ON`, we currently put all test protos into their own shared library. PR #15887 seems to be pushing us over a limit on the number of exported symbols so that this library no longer links successfully, though. This change fixes that problem by always building the test protos into a static library. This should be fine since it's purely for testing and not meant to be installed. The only things that depend on it are executables, so we don't need to worry about ODR violations. PiperOrigin-RevId: 617929396

acozzette · 2024-03-21T22:06:45Z

@antongrbin Sorry again for all the delays on this. I was able to get in a fix for the failing CMake test, so if you wouldn't mind merging main again then hopefully I can finally merge this.

antongrbin · 2024-03-25T12:56:52Z

@antongrbin Sorry again for all the delays on this. I was able to get in a fix for the failing CMake test, so if you wouldn't mind merging main again then hopefully I can finally merge this.

@acozzette no worries! Merged with upstream :) Thank you for fixing the problem!!

…wn_fields is set (protocolbuffers#15887) # Motivation This PR fixes failing conformance tests for python with name `IgnoreUnknownEnumStringValue`. The JSON parsing spec was discussed in protocolbuffers#7392. Recent equivalent changes for other languages: * Swift: apple/swift-protobuf#1345 * C#: protocolbuffers#15758 # Changes - 1st commit is a noop refactoring to make relevant _ConvertScalarFieldValue invocations localized - 2nd commit introduces the child exception of `ParseError` named `EnumStringValueParseError` which is suppressed if `ignore_unknown_fields` is set - 3rd commit updates the conformance test failure lists Closes protocolbuffers#15887 COPYBARA_INTEGRATE_REVIEW=protocolbuffers#15887 from noom:anton/7392/fix-python-test fbcc93a PiperOrigin-RevId: 619288323

When building with `-Dprotobuf_BUILD_SHARED_LIBS=ON`, we currently put all test protos into their own shared library. PR protocolbuffers#15887 seems to be pushing us over a limit on the number of exported symbols so that this library no longer links successfully, though. This change fixes that problem by always building the test protos into a static library. This should be fine since it's purely for testing and not meant to be installed. The only things that depend on it are executables, so we don't need to worry about ODR violations. PiperOrigin-RevId: 617929396

…wn_fields is set (protocolbuffers#15887) # Motivation This PR fixes failing conformance tests for python with name `IgnoreUnknownEnumStringValue`. The JSON parsing spec was discussed in protocolbuffers#7392. Recent equivalent changes for other languages: * Swift: apple/swift-protobuf#1345 * C#: protocolbuffers#15758 # Changes - 1st commit is a noop refactoring to make relevant _ConvertScalarFieldValue invocations localized - 2nd commit introduces the child exception of `ParseError` named `EnumStringValueParseError` which is suppressed if `ignore_unknown_fields` is set - 3rd commit updates the conformance test failure lists Closes protocolbuffers#15887 COPYBARA_INTEGRATE_REVIEW=protocolbuffers#15887 from noom:anton/7392/fix-python-test fbcc93a PiperOrigin-RevId: 619288323

sbchisholm · 2024-07-09T18:27:16Z

Is there a way now to use ignore_unknown_fields to ignore the unknown fields but still validate that the enum strings are valid options?

acozzette · 2024-07-09T18:33:21Z

No, for the purposes of JSON parsing an unknown enum string is considered an unknown field.

antongrbin marked this pull request as ready for review February 20, 2024 11:46

antongrbin requested review from a team as code owners February 20, 2024 11:46

antongrbin requested review from zhangskz and fowles and removed request for a team February 20, 2024 11:46

zhangskz requested review from acozzette and thomasvl and removed request for a team, zhangskz and fowles February 21, 2024 16:51

thomasvl removed their request for review February 21, 2024 17:08

acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 21, 2024

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 21, 2024

antongrbin commented Feb 22, 2024

View reviewed changes

antongrbin added 3 commits February 23, 2024 12:34

noop: refactor calls to _ConvertScalarFieldValue

6a5a630

Ignore unknown enum string value if flag set

3186170

Update failure lists for python confromance \o/

277e212

antongrbin force-pushed the anton/7392/fix-python-test branch from cb25010 to 277e212 Compare February 23, 2024 11:40

acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 29, 2024

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 29, 2024

acozzette approved these changes Mar 6, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into anton/7392/fix-pyth…

171b27e

…on-test

antongrbin force-pushed the anton/7392/fix-python-test branch from 06c8485 to 171b27e Compare March 9, 2024 14:19

Merge remote-tracking branch 'upstream/main' into anton/7392/fix-pyth…

a8c96a0

…on-test

acozzette approved these changes Mar 12, 2024

View reviewed changes

acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 12, 2024

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 12, 2024

copybara-service bot mentioned this pull request Mar 21, 2024

Make CMake always build test protos into a static library #16251

Closed

Merge branch 'main' into anton/7392/fix-python-test

fbcc93a

acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 25, 2024

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 25, 2024

copybara-service bot closed this in 86abf35 Mar 26, 2024

antongrbin deleted the anton/7392/fix-python-test branch July 15, 2024 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python JSON parser: Ignore invalid enum string values if ignore_unknown_fields is set #15887

Python JSON parser: Ignore invalid enum string values if ignore_unknown_fields is set #15887

antongrbin commented Feb 20, 2024 •

edited

Loading

antongrbin Feb 22, 2024

antongrbin Feb 22, 2024

antongrbin Feb 22, 2024

antongrbin Feb 22, 2024

antongrbin commented Feb 23, 2024

antongrbin commented Feb 29, 2024

antongrbin commented Mar 7, 2024

acozzette commented Mar 8, 2024

antongrbin commented Mar 12, 2024

acozzette commented Mar 12, 2024

antongrbin commented Mar 12, 2024

acozzette commented Mar 12, 2024

antongrbin commented Mar 18, 2024

acozzette commented Mar 21, 2024

antongrbin commented Mar 25, 2024

sbchisholm commented Jul 9, 2024

acozzette commented Jul 9, 2024

Python JSON parser: Ignore invalid enum string values if ignore_unknown_fields is set #15887

Python JSON parser: Ignore invalid enum string values if ignore_unknown_fields is set #15887

Conversation

antongrbin commented Feb 20, 2024 • edited Loading

Motivation

Changes

antongrbin Feb 22, 2024

Choose a reason for hiding this comment

antongrbin Feb 22, 2024

Choose a reason for hiding this comment

antongrbin Feb 22, 2024

Choose a reason for hiding this comment

antongrbin Feb 22, 2024

Choose a reason for hiding this comment

antongrbin commented Feb 23, 2024

antongrbin commented Feb 29, 2024

antongrbin commented Mar 7, 2024

acozzette commented Mar 8, 2024

antongrbin commented Mar 12, 2024

acozzette commented Mar 12, 2024

antongrbin commented Mar 12, 2024

acozzette commented Mar 12, 2024

antongrbin commented Mar 18, 2024

acozzette commented Mar 21, 2024

antongrbin commented Mar 25, 2024

sbchisholm commented Jul 9, 2024

acozzette commented Jul 9, 2024

antongrbin commented Feb 20, 2024 •

edited

Loading