-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[DO NOT MERGE] Add a test to prove how to use protobuf nullability #11712
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11712 +/- ##
=============================================
- Coverage 63.05% 0.00% -63.05%
+ Complexity 1118 6 -1112
=============================================
Files 2342 2266 -76
Lines 125787 122066 -3721
Branches 19335 18780 -555
=============================================
- Hits 79314 6 -79308
- Misses 40813 122060 +81247
+ Partials 5660 0 -5660
Flags with carried forward coverage won't be shown. Click here to find out more. see 1990 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -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.hasOptionalKeyword() || message.hasField(fieldDescriptor)) { |
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.
This is exactly what my PR changed - from hasOptionalKeyword to isOptional
API is deprecated
protocolbuffers/protobuf@d6157f7
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.
It is deprecated in latest versions of protobuf, but:
- It is not deprecated in the version we use
- There is no alternative to this method in the latest protobuf version.
Theoretically we should is isPresent
, but it is not honoring its javadoc. You can verify that by changing this to isPresent
and see that it returns false for the test added 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.
It seems isPresent is not bug, but I was unable to correctly configure protobuf model programmatically.
I've changed the code to:
I think
It is easier and more useful to read the DataProviders than the tests. As you can see, both
|
This PR adds a test to verify the correct nullability read on
ProtoBufRecordExtractor
.The PR is not yet ready to merge because for some reason the descriptor is using v2 instead of 3.
cc @swaminathanmanish