| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
@kalzoo What's the status on this? My team would benefit alot from having this in place. |
Sorry, something went wrong.
|
I am using this PR and have found some issues with it. I have written a PR to fix those here: #292 |
Sorry, something went wrong.
= 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.
|
@alfredgunnar status is that the tests were caught up in an issue with black, I pushed a fix and @Gobot1234 straightened it out but I've been busy since. Now we're on to a new issue in the suite which I'll debug as time permits. Once this pipeline is green, we should be set to merge and release a new version. If you want to accelerate this, feel free to PR a test pipeline fix. |
Sorry, something went wrong.
|
Hey @kalzoo are the tests only failing on the pipeline? I ran them successfully locally. |
Sorry, something went wrong.
|
This is fixed - CI failure was caused by a mistake in merge from master and resulting regression - see 3a41992. Since I made the final changes, I'd like to ask @Gobot1234 (and others) for review. |
Sorry, something went wrong.
|
Everything here looks good to me! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This finishes the work done on #268 by @roblabla . See that PR for more information