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

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

Closed

Conversation

antongrbin
Copy link
Contributor

@antongrbin antongrbin commented Feb 20, 2024

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:

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 antongrbin marked this pull request as ready for review February 20, 2024 11:46
@antongrbin antongrbin requested review from a team as code owners February 20, 2024 11:46
@antongrbin antongrbin requested review from zhangskz and fowles and removed request for a team February 20, 2024 11:46
@zhangskz zhangskz requested review from acozzette and thomasvl and removed request for a team, zhangskz and fowles February 21, 2024 16:51
@thomasvl thomasvl removed their request for review February 21, 2024 17:08
@acozzette acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 21, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 21, 2024

def testParseErrorForUnknownEnumValue_ScalarWithoutIgnore_Proto2(self):
message = json_format_pb2.TestNumbers()
self.assertRaisesRegexp(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

message TestEnumMap {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by renaming this message.

@antongrbin
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@acozzette acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 29, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 29, 2024
@antongrbin
Copy link
Contributor Author

@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
Copy link
Member

@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.

@antongrbin
Copy link
Contributor Author

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

@acozzette
Copy link
Member

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.

@antongrbin
Copy link
Contributor Author

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

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

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
Copy link
Contributor Author

@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 :)

copybara-service bot pushed a commit that referenced this pull request Mar 21, 2024
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
copybara-service bot pushed a commit that referenced this pull request Mar 21, 2024
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
copybara-service bot pushed a commit that referenced this pull request Mar 21, 2024
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
copybara-service bot pushed a commit that referenced this pull request Mar 21, 2024
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
copybara-service bot pushed a commit that referenced this pull request Mar 21, 2024
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
copybara-service bot pushed a commit that referenced this pull request Mar 21, 2024
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
copybara-service bot pushed a commit that referenced this pull request Mar 21, 2024
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
Copy link
Member

@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
Copy link
Contributor Author

@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!!

@acozzette acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 25, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 25, 2024
elvisdukaj pushed a commit to elvisdukaj/protobuf that referenced this pull request Apr 3, 2024
…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
deannagarcia pushed a commit to deannagarcia/protobuf that referenced this pull request Jun 20, 2024
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
deannagarcia pushed a commit to deannagarcia/protobuf that referenced this pull request Jun 20, 2024
…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
Copy link

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
Copy link
Member

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

@antongrbin antongrbin deleted the anton/7392/fix-python-test branch July 15, 2024 15:01
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

3 participants