You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/05/05 08:45:54 UTC
[GitHub] [pulsar] abhilashmandaliya opened a new pull request #10482: fixed NPE in GenericJsonRecord
abhilashmandaliya opened a new pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on a change in pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on a change in pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#discussion_r626390876
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -380,16 +382,15 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
}
@Test
- public void testGetNativeSchema() throws SchemaValidationException {
+ public void testGetNativeSchema() throws IllegalAccessException {
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);
Review comment:
and I fixed it as it was just a test class so should not cause any unexpected behaviour
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#discussion_r626393684
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -380,16 +382,15 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
}
@Test
- public void testGetNativeSchema() throws SchemaValidationException {
+ public void testGetNativeSchema() throws IllegalAccessException {
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);
Review comment:
I think that the possible "unsafe call to get" should be fine in this context (in test code).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#issuecomment-832562473
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie merged pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#discussion_r626391595
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -380,16 +382,15 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
}
@Test
- public void testGetNativeSchema() throws SchemaValidationException {
+ public void testGetNativeSchema() throws IllegalAccessException {
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);
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on a change in pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on a change in pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#discussion_r626389570
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -380,16 +382,15 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
}
@Test
- public void testGetNativeSchema() throws SchemaValidationException {
+ public void testGetNativeSchema() throws IllegalAccessException {
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);
Review comment:
it was unsafe to call get on optional without checking it. hence I added this exception there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#discussion_r626386839
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -380,16 +382,15 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
}
@Test
- public void testGetNativeSchema() throws SchemaValidationException {
+ public void testGetNativeSchema() throws IllegalAccessException {
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);
Review comment:
Is this change necessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on a change in pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on a change in pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#discussion_r626389570
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -380,16 +382,15 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
}
@Test
- public void testGetNativeSchema() throws SchemaValidationException {
+ public void testGetNativeSchema() throws IllegalAccessException {
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);
Review comment:
it was unsafe to call get on optional without checking it. hence I added this exception there. intellij showed a warning there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#issuecomment-832594748
@lhotari @eolivelli I have reverted additional changes :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#issuecomment-832632749
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on pull request #10482: [CLIENT] fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#issuecomment-832535774
> overall looks good
> I left a little comment, non blocker
>
> +1
Thanks, @eolivelli . Yeah, it's just a test and made it a little longer :sweat_smile: but the idea warning irritates me so I changed it along with other changes in this file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] abhilashmandaliya commented on pull request #10482: fixed NPE in GenericJsonRecord
Posted by GitBox <gi...@apache.org>.
abhilashmandaliya commented on pull request #10482:
URL: https://github.com/apache/pulsar/pull/10482#issuecomment-832522494
@merlimat @codelipenghui @eolivelli @lhotari PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org