[MIN-2432] Implement Python Enum schema evolution #4

antongrbin · 2022-06-20T15:29:07Z

Motivation

Tech doc:
https://docs.google.com/document/d/1p5LUSTWrVBcT9F2wXBgHBDT3OJyOm5BpXAyTpwwDVts/edit#

Changes

In this PR we actually fix the Python protobuf library not to break when unknown enum value is encountered if ignoreUnknownFields is true.

The design principles for the change were:

make the change as small as possible, so it's easy to keep up to date with the upstream
if one element is unknown in an array of elements, other elements should still parse (see unit tests)

Changes:

Change _ConvertScalarFieldValue so it throws a special exception when unknown enum string value is encountered.
Wrap every invocation of _ConvertScalarFieldValue to optionally ignore the new exception thrown.
Add unit tests

To write this PR, I reached out for input on #python-club on slack

Tested

Executed the test plan from here: #1 to test this within the protobuf repo.

Also tested as part of noom-contracts repo: https://github.com/noom/noom-contracts/pull/293

…/fix-python

ifabijanovic · 2022-06-21T11:44:12Z

python/google/protobuf/internal/json_format_test.py

+
+  def testParseUnknownEnumStringValueRepeatedProto3(self):
+    message = json_format_proto3_pb2.TestMessage()
+    text = '{"repeatedEnumValue": ["UNKNOWN_STRING_VALUE", "FOO", "BAR"]}'


Isn't an array of enums banned?

Good question :)
We banned the array of enums in noom-contracts to avoid people getting confused with this behavior. The array length is 2 although there were 3 fields (one of them unknown).

In this repository (protobuf) I am still testing that the behavior that we have with repeated enums is consistent with the behavior in the binary serialization (regardless of the fact that the behavior is confusing).

I'll add the comment in the code.

ifabijanovic · 2022-06-21T11:44:36Z

python/google/protobuf/internal/json_format_test.py

+
+  def testParseUnknownEnumStringValueProto2(self):
+    message = json_format_pb2.TestNumbers()
+    text = '{"a": "UNKNOWN_STRING_VALUE"}'


curious why the key is a here instead of enumValue like in other tests?

Good catch, I verified that this is OK and added comment.

# Note that in src/google/protobuf/util/json_format.proto::TestNumbers # has a field 'optional MyType a = 1', which is different # from the test below that are using # src/google/protobuf/util/json_format_proto3.proto::TestMessage.

ifabijanovic · 2022-06-21T11:48:07Z

python/google/protobuf/json_format.py

  """Thrown in case of parsing error."""


+class _UnknownEnumStringValueParseError(ParseError):


Does this class inherit from ParseError (it looks like it does)? So we don't break code that tries to catch ParseError

Correct. Added a comment about that.

Although I think the implementation will never leak _UnknownEnumStringValueParseError, it still gets renamed to ParseError in _ConvertFieldValuePair.. but I wouldn't rely on that, the code is very hard to follow :) It's safer if I just inherit and then I know I didn't change any public interface

ifabijanovic · 2022-06-21T11:49:39Z

python/google/protobuf/internal/json_format_test.py

        'for enum type protobuf_unittest.TestAllTypes.NestedEnum.',
        json_format.Parse, '{"optionalNestedEnum": 12345}', message)

+  def testParseUnknownEnumStringValueProto3(self):


These tests only test deserialisation. Since we are modifying code that can also set values, should we add tests for serialisation as well?

I am unaware that I am changing the code that sets values.

The code changes are in the following functions:

_ConvertScalarFieldValue _ConvertStructMessage _ConvertWrapperMessage _ConvertMapFieldValue

These functions are called from the following functions transitively:
_ConvertFieldValuePair
Parser::ConvertMessage
_ConvertAnyMessage
ParseDict

This is only parsing of JSON, I never change serializing to JSON or setting the values.
Did I miss something?

antongrbin

Thank you for the review!

antongrbin · 2022-06-21T13:41:30Z

python/google/protobuf/internal/json_format_test.py

+
+  def testParseUnknownEnumStringValueProto2(self):
+    message = json_format_pb2.TestNumbers()
+    text = '{"a": "UNKNOWN_STRING_VALUE"}'


Good catch, I verified that this is OK and added comment.

# Note that in src/google/protobuf/util/json_format.proto::TestNumbers # has a field 'optional MyType a = 1', which is different # from the test below that are using # src/google/protobuf/util/json_format_proto3.proto::TestMessage.

antongrbin · 2022-06-21T13:43:38Z

python/google/protobuf/internal/json_format_test.py

+
+  def testParseUnknownEnumStringValueRepeatedProto3(self):
+    message = json_format_proto3_pb2.TestMessage()
+    text = '{"repeatedEnumValue": ["UNKNOWN_STRING_VALUE", "FOO", "BAR"]}'


Good question :)
We banned the array of enums in noom-contracts to avoid people getting confused with this behavior. The array length is 2 although there were 3 fields (one of them unknown).

In this repository (protobuf) I am still testing that the behavior that we have with repeated enums is consistent with the behavior in the binary serialization (regardless of the fact that the behavior is confusing).

I'll add the comment in the code.

antongrbin · 2022-06-21T13:51:57Z

python/google/protobuf/internal/json_format_test.py

        'for enum type protobuf_unittest.TestAllTypes.NestedEnum.',
        json_format.Parse, '{"optionalNestedEnum": 12345}', message)

+  def testParseUnknownEnumStringValueProto3(self):


I am unaware that I am changing the code that sets values.

The code changes are in the following functions:

_ConvertScalarFieldValue _ConvertStructMessage _ConvertWrapperMessage _ConvertMapFieldValue

These functions are called from the following functions transitively:
_ConvertFieldValuePair
Parser::ConvertMessage
_ConvertAnyMessage
ParseDict

This is only parsing of JSON, I never change serializing to JSON or setting the values.
Did I miss something?

antongrbin · 2022-06-21T13:59:09Z

python/google/protobuf/json_format.py

  """Thrown in case of parsing error."""


+class _UnknownEnumStringValueParseError(ParseError):


Correct. Added a comment about that.

Although I think the implementation will never leak _UnknownEnumStringValueParseError, it still gets renamed to ParseError in _ConvertFieldValuePair.. but I wouldn't rely on that, the code is very hard to follow :) It's safer if I just inherit and then I know I didn't change any public interface

We would like for upb_Map_Delete() to optionally return the deleted value. Unfortunately this will require several steps since we are crossing repos. Step #4: advance the upb version used by protobuf and update PHP/Ruby accordingly. PiperOrigin-RevId: 498426185

PiperOrigin-RevId: 574544964

antongrbin added 5 commits April 22, 2022 16:53

implement ignoring unknown enum values

cc973ed

self review

e72f8a9

Merge branch 'min-2432/make-the-json-format-standalone' into min-2432…

b12491c

…/fix-python

self review

b0f3ef2

Merge branch 'json_unknown_enum_fork' into min-2432/fix-python

70efd1d

antongrbin mentioned this pull request Jun 20, 2022

[MIN-2432] Implement ignoring of unknown enum string value in Python #3

Closed

antongrbin requested a review from ifabijanovic June 20, 2022 15:48

antongrbin marked this pull request as ready for review June 20, 2022 15:48

antongrbin added 4 commits June 20, 2022 17:53

fix comment

0d8dabb

apply advice from Clements

f54c8b4

just noting the typo above, should be Clemens

b34fb41

add comment

793ad12

ifabijanovic reviewed Jun 21, 2022

View reviewed changes

address review comments

e2bfadb

antongrbin commented Jun 21, 2022

View reviewed changes

ifabijanovic approved these changes Jun 21, 2022

View reviewed changes

antongrbin merged commit 12610cd into json_unknown_enum_fork Jun 21, 2022

antongrbin pushed a commit that referenced this pull request Jan 10, 2024

Use GetArena() instead of GetOwningArena() #4.

eccbf14

PiperOrigin-RevId: 574544964

		"""Thrown in case of parsing error."""


		class _UnknownEnumStringValueParseError(ParseError):

[MIN-2432] Implement Python Enum schema evolution #4

[MIN-2432] Implement Python Enum schema evolution #4

Uh oh!

Conversation

antongrbin commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Tested

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antongrbin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

antongrbin commented Jun 20, 2022 •

edited

Loading