-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[CLIENT] fixed NPE in GenericJsonRecord #10482
Conversation
JSONSchema<PC> schema2 = JSONSchema.of(PC.class); | ||
org.apache.avro.Schema avroSchema2 = (Schema) schema2.getNativeSchema().get(); | ||
org.apache.avro.Schema avroSchema2 = | ||
(Schema) schema2.getNativeSchema().orElseThrow(IllegalAccessException::new); |
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.
Is this change necessary?
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 was unsafe to call get on optional without checking it. hence I added this exception there. intellij showed a warning there
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.
and I fixed it as it was just a test class so should not cause any unexpected behaviour
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 a test case, I am not sure it is worth to complicate it.
the test would fail anyway, we are only making the line longer IMHO
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 think that the possible "unsafe call to get" should be fine in this context (in test code).
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.
overall looks good
I left a little comment, non blocker
+1
JSONSchema<PC> schema2 = JSONSchema.of(PC.class); | ||
org.apache.avro.Schema avroSchema2 = (Schema) schema2.getNativeSchema().get(); | ||
org.apache.avro.Schema avroSchema2 = | ||
(Schema) schema2.getNativeSchema().orElseThrow(IllegalAccessException::new); |
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 a test case, I am not sure it is worth to complicate it.
the test would fail anyway, we are only making the line longer IMHO
Thanks, @eolivelli . Yeah, it's just a test and made it a little longer 😅 but the idea warning irritates me so I changed it along with other changes in this file. |
/pulsarbot run-failure-checks |
@lhotari @eolivelli I have reverted additional changes :) |
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.
LGTM
/pulsarbot run-failure-checks |
Fixes apache#10472 ### Motivation GenericJsonRecord should not error out if the non-existent field is queried. ### Modifications Added a null check and returning null if the field does not exist. ### Verifying this change Extended one of the tests for GenericJsonRecord.
Fixes apache#10472 GenericJsonRecord should not error out if the non-existent field is queried. Added a null check and returning null if the field does not exist. Extended one of the tests for GenericJsonRecord. (cherry picked from commit ff076e3)
Fixes #10472
Motivation
GenericJsonRecord should not error out if the non-existent field is queried.
Modifications
Added a null check and returning null if the field does not exist.
Verifying this change
Extended one of the tests for GenericJsonRecord.