-
Notifications
You must be signed in to change notification settings - Fork 220
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
Serialize default values in oneofs when calling to_dict() or to_json() #110
Changes from 4 commits
265c075
d9eab6d
4fc2f71
68d58b3
72f9686
4252a10
0726f7f
4789636
f904b5c
2e86a44
8d3fa9e
b520064
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
syntax = "proto3"; | ||
|
||
import "google/protobuf/duration.proto"; | ||
import "google/protobuf/timestamp.proto"; | ||
import "google/protobuf/wrappers.proto"; | ||
|
||
message Message{ | ||
int64 value = 1; | ||
} | ||
|
||
message NestedMessage{ | ||
int64 id = 1; | ||
oneof value_type{ | ||
Message wrapped_message_value = 2; | ||
} | ||
} | ||
|
||
message Test{ | ||
oneof value_type { | ||
bool bool_value = 1; | ||
int64 int64_value = 2; | ||
google.protobuf.Timestamp timestamp_value = 3; | ||
google.protobuf.Duration duration_value = 4; | ||
Message wrapped_message_value = 5; | ||
NestedMessage wrapped_nested_message_value = 6; | ||
google.protobuf.BoolValue wrapped_bool_value = 7; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
import pytest | ||
import datetime | ||
|
||
import betterproto | ||
from betterproto.tests.output_betterproto.oneof_default_value_serialization import ( | ||
Test, | ||
Message, | ||
NestedMessage, | ||
) | ||
|
||
|
||
def assert_round_trip_serialization_works(message: Test) -> None: | ||
assert betterproto.which_one_of(message, "value_type") == betterproto.which_one_of( | ||
Test().from_json(message.to_json()), "value_type" | ||
) | ||
|
||
|
||
def test_oneof_default_value_serialization_works_for_all_values(): | ||
""" | ||
Serialization from message with oneof set to default -> JSON -> message should keep | ||
default value field intact. | ||
""" | ||
|
||
test_cases = [ | ||
Test(bool_value=False), | ||
Test(int64_value=0), | ||
Test( | ||
timestamp_value=datetime.datetime( | ||
year=1970, | ||
month=1, | ||
day=1, | ||
hour=0, | ||
minute=0, | ||
tzinfo=datetime.timezone.utc, | ||
) | ||
), | ||
Test(duration_value=datetime.timedelta(0)), | ||
Test(wrapped_message_value=Message(value=0)), | ||
# NOTE: Do NOT use betterproto.BoolValue here, it will cause JSON serialization | ||
# errors. | ||
# TODO: Do we want to allow use of BoolValue directly within a wrapped field or | ||
# should we simply hard fail here? | ||
Test(wrapped_bool_value=False), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! It seems odd that passing a It does raise some questions. We may need to look into this, in a separate issue. Its fine with me to keep the TODO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah if I had to make a choice (with just thinking about this briefly) I would err on the side of failing fast when attempting to assign a non-primitive to a wrapped field. That at least makes the usage of the field unambiguous |
||
] | ||
for message in test_cases: | ||
assert_round_trip_serialization_works(message) | ||
|
||
|
||
def test_oneof_no_default_values_passed(): | ||
message = Test() | ||
assert ( | ||
betterproto.which_one_of(message, "value_type") | ||
== betterproto.which_one_of(Test().from_json(message.to_json()), "value_type") | ||
== ("", None) | ||
) | ||
|
||
|
||
def test_oneof_nested_oneof_messages_are_serialized_with_defaults(): | ||
""" | ||
Nested messages with oneofs should also be handled | ||
""" | ||
message = Test( | ||
wrapped_nested_message_value=NestedMessage( | ||
id=0, wrapped_message_value=Message(value=0) | ||
) | ||
) | ||
assert ( | ||
betterproto.which_one_of(message, "value_type") | ||
== betterproto.which_one_of(Test().from_json(message.to_json()), "value_type") | ||
== ( | ||
"wrapped_nested_message_value", | ||
NestedMessage(id=0, wrapped_message_value=Message(value=0)), | ||
) | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,34 +9,36 @@ | |
from betterproto.tests.util import get_test_case_json_data | ||
|
||
|
||
@pytest.mark.xfail | ||
def test_which_one_of_returns_enum_with_default_value(): | ||
""" | ||
returns first field when it is enum and set with default value | ||
""" | ||
message = Test() | ||
message.from_json(get_test_case_json_data("oneof_enum", "oneof_enum-enum-0.json")) | ||
assert message.move is None | ||
|
||
assert message.move == Move( | ||
x=0, y=0 | ||
) # Proto3 will default this as there is no null | ||
assert message.signal == Signal.PASS | ||
assert betterproto.which_one_of(message, "action") == ("signal", Signal.PASS) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change needed to solve the bug? As I understand the bug is in I think the idea that proto doesn't have null values might actually be somewhat muddled, and incorrect. Because values can be skipped when they are sent over the wire, effectively making them undefined. However, the protobuf libraries will then simply return the default value for those skipped fields. That behavior is indeed part of protobuf specification, but for oneof groups, I consider it not part of the spec that all other fields should have a default value as well (unless its actually wrtten in the spec, sorry I didn't have time to read it 😅 ). At least it doesn't make much sense to me why it would be needed. Unless it is somehow needed for this bugfix, I would suggest to exclude that part from this PR, because its a functional change (and not sure if its an improvement). And that is a bit harder to judge than the bugfix, which we could merge quickly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this is actually default behavior in Google's protobuf implementation as well. What's actually happening here is that the
Compiling with the Google proto impl I then checked the following: In [4]: t = Test()
In [5]: t
Out[5]:
In [6]: t.point
Out[6]:
In [7]: t.point.x
Out[7]: 0
...
In [9]: type(t.point.x)
Out[9]: int
In [10]: type(t.point)
Out[10]: foo.Point
In [11]: type(t.int_value)
Out[11]: int
In [12]: t.bool_value
Out[12]:
In [13]: type(t.bool_value)
Out[13]: google.protobuf.wrappers_pb2.BoolValue
In [14]: from google.protobuf.json_format import MessageToDict
In [15]: MessageToDict(t)
Out[15]: {}
In [16]: t.point.x = 1
In [17]: MessageToDict(t)
Out[17]: {'point': {'x': '1'}}
In [18]: t.int_value = 1
In [19]: MessageToDict(t)
Out[19]: {'intValue': '1'}
In [20]: t.bool_value.value = True
In [21]: t
Out[21]:
bool_value {
value: true
}
In [22]: MessageToDict(t)
Out[22]: {'boolValue': True} I can either add an assert to this test case or a separate test case demonstrating that betterprotos has similar behaviour |
||
|
||
@pytest.mark.xfail | ||
def test_which_one_of_returns_enum_with_non_default_value(): | ||
""" | ||
returns first field when it is enum and set with non default value | ||
""" | ||
message = Test() | ||
message.from_json(get_test_case_json_data("oneof_enum", "oneof_enum-enum-1.json")) | ||
assert message.move is None | ||
assert message.signal == Signal.PASS | ||
assert message.move == Move( | ||
x=0, y=0 | ||
) # Proto3 will default this as there is no null | ||
assert message.signal == Signal.RESIGN | ||
assert betterproto.which_one_of(message, "action") == ("signal", Signal.RESIGN) | ||
|
||
|
||
@pytest.mark.xfail | ||
def test_which_one_of_returns_second_field_when_set(): | ||
message = Test() | ||
message.from_json(get_test_case_json_data("oneof_enum")) | ||
assert message.move == Move(x=2, y=3) | ||
assert message.signal == 0 | ||
assert message.signal == Signal.PASS | ||
bradykieffer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert betterproto.which_one_of(message, "action") == ("move", Move(x=2, y=3)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -263,3 +263,14 @@ class TestParentMessage(betterproto.Message): | |
"someDouble": 1.2, | ||
"someMessage": {"someOtherInt": 0}, | ||
} | ||
|
||
|
||
def test_oneof_default_value_set_causes_writes_wire(): | ||
@dataclass | ||
class Foo(betterproto.Message): | ||
bar: int = betterproto.int32_field(1, group="group1") | ||
baz: str = betterproto.string_field(2, group="group1") | ||
|
||
assert bytes(Foo(bar=0)) != b"" | ||
assert bytes(Foo(baz="")) == b"" # Baz is just an empty string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean its not possible to detect Same with other default values? 😕 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch - this actually works with the other default values but not for a string at the moment, I'll get a fix in 😄 |
||
assert bytes(Foo()) == b"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused how
from_dict
ever worked properly, if message fields were not assigned to their parent before..any idea of why this was needed, or why
from_dict
did actually work?Also, lines
930
-931
assignv
to the field. Perhaps just assigningv=
would suffice?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I realized it!
v
is an existing Message object. The required behavior is that the data from the map is merged into the existing fields. I think no assignment is needed here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah correct, the
v = v.from_dict(value[key])
isn't needed, I'm just used to transforming objects vs mutating them that I wrote it like that! Also, it looks like I can just move lines930
-931
out an indentation layer and achieve the same effect 😄