Conformance test: JSON parsing of unknown enum string values

[antongrbin]

Motivation

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

1. java, cpp will ignore the unknown enum string value when ignoreUnknown is set
2. 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: https://github.com/protocolbuffers/protobuf/issues/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 https://github.com/protocolbuffers/protobuf/issues/7392
  • Asks the protobuf team for the specification in this situation.
  • dupe https://github.com/protocolbuffers/protobuf/issues/8244
• ISSUE https://github.com/protocolbuffers/protobuf/issues/3012
  • original Java issue, fixed now
  • PR https://github.com/protocolbuffers/protobuf/pull/4825
    • original fix for Java
• PR https://github.com/protocolbuffers/protobuf/pull/7974
  • c# fix attempt
• PR https://github.com/protocolbuffers/protobuf/pull/7768
  • c# fix attempt

Related PRs in other external protobuf implementations:

• PR https://github.com/timostamm/protobuf-ts/pull/170
  • fix for the typescript library
• PR https://github.com/streem/pbandk/pull/100
  • fix for Kotlin implementation
• ISSUE https://github.com/apple/swift-protobuf/issues/972
  • swift issue
  • PR https://github.com/apple/swift-protobuf/pull/896
    • attempt to fix for Swift-> lesson learned: map is a thing

[antongrbin]

As explained in https://github.com/protocolbuffers/protobuf/issues/8358, I don't have permissions to add a label. I would like to add label conformance tests.

[elharo]

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

[antongrbin]

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

@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 https://github.com/apple/swift-protobuf/issues/972#issuecomment-1293696511.

Thanks!

[antongrbin]

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]

Anton,

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

[antongrbin]

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]

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

[antongrbin]

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

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

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]

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]

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