Proto null fix #11723
Proto null fix #11723
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11723 +/- ##
=============================================
- Coverage 63.05% 14.44% -48.61%
+ Complexity 1118 201 -917
=============================================
Files 2342 2342
Lines 125787 125802 +15
Branches 19335 19336 +1
=============================================
- Hits 79314 18176 -61138
- Misses 40813 106093 +65280
+ Partials 5660 1533 -4127
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1510 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Thanks ! Your doc that explains the behavior makes sense
https://docs.google.com/document/d/1xfYVL8V600lPV7U6ZO4NpGIV3gOVp8cTlKn2Zj0bCOI/edit
| @@ -62,7 +62,7 @@ private Object getFieldValue(Descriptors.FieldDescriptor fieldDescriptor, Messag | |||
| // Note w.r.t proto3 - If a field is not declared with optional keyword, there's no way to distinguish | |||
| // if its explicitly set to a proto default or not been set at all i.e hasField() returns false | |||
| // and we would use null. | |||
| if (fieldDescriptor.isRepeated() || !fieldDescriptor.isOptional() || message.hasField(fieldDescriptor)) { | |||
| if (fieldDescriptor.isRepeated() || !fieldDescriptor.hasPresence() || message.hasField(fieldDescriptor)) { | |||
There was a problem hiding this comment.
nit: should we change the note above? ("not declared with optional keyword" --> "is set as hasPresence flag")
There was a problem hiding this comment.
Yes, you are right. I'm opening a new PR to change that
This PR fixes the way nulls are read in Protobuf and add some test that verify the correct behavior.
See #11712 to see the same tests failing in current master branch.
PS: See https://docs.google.com/document/d/1xfYVL8V600lPV7U6ZO4NpGIV3gOVp8cTlKn2Zj0bCOI