Fix oneof serialization with proto3 field presence #292
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
= Description
The serialization of a oneof message that contains a message with fields
with explicit presence was buggy.
For example:
```
message A {
oneof kind {
B b = 1;
C c = 2;
}
}
message B {}
message C {
optional bool z = 1;
}
```
Serializing `A(b=B())` would lead to this payload:
```
0A # tag1, length delimited
00 # length: 0
12 # tag2, length delimited
00 # length: 0
```
Which when deserialized, leads to the message `A(c=C())`.
= Explanation
The issue lies in the post_init method. All fields are introspected, and
if different from PLACEHOLDER, the message is marked as having been
"serialized_on_wire".
Then, when serializing `A(b=B())`, we go through each field of the
oneof:
- field 'b': this is the selected field from the group, so it is
serialized
- field 'c': marked as 'serialized_on_wire', so it is added as well.
= Fix
The issue is that support for explicit presence changed the default
value from PLACEHOLDER to None. This breaks the post_init method in that
case, which is relatively easy to fix: if a field is optional, and set
to None, this is considered as the default value (which it is).
This fix however has a side-effect: the group_current for this field (the
oneof trick for explicit presence) is no longer set. This changes the
behavior when serializing the message in JSON: as the value is the
default one (None), and the group is not set (which would force the
serialization of the field), so None fields are no longer serialized in
JSON. This break one test, and will be fixed in the next commit.
This is linked to the fix from the previous commit: after it, scalar None fields were not included in the JSON format, but some were still included. This is all cleaned up: None fields are not added in JSON by default, as they indicate the default value of fields with explicit presence. However, if `include_default_values is set, they are included.
Merged
|
There are already three pull requests that superseed each other, all of them always die out because there is no attention given to them. Can any of the repository maintainers help finishing this feature, which is a serious blocker for some of us? Please? |
|
Hi @marianhlavac I'm currently working with @kalzoo to figure out what the best way to get this and 292 merged. (FYI those 3 are of busy atm, that's why both of us were brought on to work on fixing issues) |
|
@Gobot1234 Great, thanks for the info. Let us know if there is any way for us to help with the merge / missing features / testing. |
|
Good fix, thanks @vthib . Test failure is due to |
kalzoo
approved these changes
Dec 12, 2021
kalzoo
added a commit
that referenced
this pull request
Dec 29, 2021
* Update protobuf pregenerated files * Update grpcio-tools to latest version * Implement proto3 field presence * Fix to_dict with None optional fields. * Add test with optional enum * Properly support optional enums * Add tests for 64-bit ints and floats * Support field presence for int64 types * Fix oneof serialization with proto3 field presence (#292) = Description The serialization of a oneof message that contains a message with fields with explicit presence was buggy. For example: ``` message A { oneof kind { B b = 1; C c = 2; } } message B {} message C { optional bool z = 1; } ``` Serializing `A(b=B())` would lead to this payload: ``` 0A # tag1, length delimited 00 # length: 0 12 # tag2, length delimited 00 # length: 0 ``` Which when deserialized, leads to the message `A(c=C())`. = Explanation The issue lies in the post_init method. All fields are introspected, and if different from PLACEHOLDER, the message is marked as having been "serialized_on_wire". Then, when serializing `A(b=B())`, we go through each field of the oneof: - field 'b': this is the selected field from the group, so it is serialized - field 'c': marked as 'serialized_on_wire', so it is added as well. = Fix The issue is that support for explicit presence changed the default value from PLACEHOLDER to None. This breaks the post_init method in that case, which is relatively easy to fix: if a field is optional, and set to None, this is considered as the default value (which it is). This fix however has a side-effect: the group_current for this field (the oneof trick for explicit presence) is no longer set. This changes the behavior when serializing the message in JSON: as the value is the default one (None), and the group is not set (which would force the serialization of the field), so None fields are no longer serialized in JSON. This break one test, and will be fixed in the next commit. * fix: do not serialize None fields in JSON format This is linked to the fix from the previous commit: after it, scalar None fields were not included in the JSON format, but some were still included. This is all cleaned up: None fields are not added in JSON by default, as they indicate the default value of fields with explicit presence. However, if `include_default_values is set, they are included. * Fix: use builtin annotation prefix * Remove comment Co-authored-by: roblabla <unfiltered@roblab.la> Co-authored-by: Vincent Thiberville <vthib@pm.me>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello, I am using the PR implementing the proto3 field presence: #281
I have come upon an issue with it, when a message containing a
oneofis serialized.A quick description of the issue is that with this proto file:
Serialization of the message
A(b=B())gives the payload0A 00 12 00(basicallyb=B()followed byc=C()), which is deserialized asA(c=C()).This PR implements a fix for this issue, as well as modifications to the serialization of None fields in dict/JSON format.
I have added some tests as well with it.