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/03/30 10:07:26 UTC
[GitHub] [pulsar] vroyer opened a new pull request #10085: Fix NPE in KeyValue when the message payload is null
vroyer opened a new pull request #10085:
URL: https://github.com/apache/pulsar/pull/10085
### Motivation
Fix a NullPointerException when decoding a KeyValue with a null message payload.
### Modifications
Return a KeyValue(null, null) when data is null or empty.
### Verifying this change
Add a unit test in org.apache.pulsar.common.schema.KeyValueTest
--
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 commented on a change in pull request #10085: Fix NPE in KeyValue when the message payload is null
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #10085:
URL: https://github.com/apache/pulsar/pull/10085#discussion_r604238949
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/common/schema/KeyValueTest.java
##########
@@ -105,6 +106,18 @@ public void testAllSchemas(Map<Schema, List<Object>> schemas) {
}
}
+ @Test
+ public void testNullPayload() {
+ KeyValue kv = KeyValue.decode(
+ null,
+ (keyBytes, valueBytes) -> new KeyValue(
+ Schema.STRING.decode(keyBytes),
+ Schema.STRING.decode(valueBytes)
+ )
+ );
+ assertNotNull(kv);
Review comment:
Don't you need to assert the key & value of this `kv`?
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/common/schema/KeyValueTest.java
##########
@@ -105,6 +106,18 @@ public void testAllSchemas(Map<Schema, List<Object>> schemas) {
}
}
+ @Test
+ public void testNullPayload() {
+ KeyValue kv = KeyValue.decode(
+ null,
Review comment:
Don't you need to test empty data array?
--
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 #10085: Fix NPE in KeyValue when the message payload is null
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10085:
URL: https://github.com/apache/pulsar/pull/10085#discussion_r603965463
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/common/schema/KeyValue.java
##########
@@ -33,6 +33,8 @@
private final K key;
private final V value;
+ public static final KeyValue NULL = new KeyValue(null, null);
Review comment:
can we make this `private` ?
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/common/schema/KeyValueTest.java
##########
@@ -53,6 +56,7 @@
import org.apache.pulsar.client.impl.schema.StringSchema;
import org.apache.pulsar.client.impl.schema.TimeSchema;
import org.apache.pulsar.client.impl.schema.TimestampSchema;
+import org.checkerframework.checker.units.qual.K;
Review comment:
please remove this import
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/common/schema/KeyValue.java
##########
@@ -130,6 +132,10 @@ public String toString() {
* @return the decoded key/value pair
*/
public static <K, V> KeyValue<K, V> decode(byte[] data, KeyValueDecoder<K, V> decoder) {
+ if (data == null || data.length == 0) {
Review comment:
can you please explain how it can happen that we fall into these cases ?
I mean, probably data == null is possible in case of a message will NULL value, but I cannot think about a case in which the array may be of zero length.
probably in case of zero length array we should throw an IllegalArgumentException, because it is not possible that it is a valid value, as we are always writing the length of the key and the length of the value (with -1 as marker for nulls)
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/common/schema/KeyValueTest.java
##########
@@ -29,6 +31,7 @@
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
+import java.util.ArrayList;
Review comment:
this import looks unused
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/common/schema/KeyValueTest.java
##########
@@ -20,7 +20,9 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import com.google.common.collect.ImmutableList;
Review comment:
this import looks unused
--
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] vroyer commented on a change in pull request #10085: Fix NPE in KeyValue when the message payload is null
Posted by GitBox <gi...@apache.org>.
vroyer commented on a change in pull request #10085:
URL: https://github.com/apache/pulsar/pull/10085#discussion_r604003269
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/common/schema/KeyValue.java
##########
@@ -130,6 +132,10 @@ public String toString() {
* @return the decoded key/value pair
*/
public static <K, V> KeyValue<K, V> decode(byte[] data, KeyValueDecoder<K, V> decoder) {
+ if (data == null || data.length == 0) {
Review comment:
It appends when key is SEPARATED and value is null, here is the KeyValueSchema.encode():
```
public byte[] encode(KeyValue<K, V> message) {
if (keyValueEncodingType != null && keyValueEncodingType == KeyValueEncodingType.INLINE) {
return KeyValue.encode(
message.getKey(),
keySchema,
message.getValue(),
valueSchema
);
} else {
if (message.getValue() == null) {
return null;
}
return valueSchema.encode(message.getValue());
}
}
```
--
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 pull request #10085: Fix NPE in KeyValue when the message payload is null
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10085:
URL: https://github.com/apache/pulsar/pull/10085#issuecomment-844185265
this patch is obsolete, we can close this PR
--
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] vroyer commented on a change in pull request #10085: Fix NPE in KeyValue when the message payload is null
Posted by GitBox <gi...@apache.org>.
vroyer commented on a change in pull request #10085:
URL: https://github.com/apache/pulsar/pull/10085#discussion_r604109226
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/common/schema/KeyValue.java
##########
@@ -130,6 +132,10 @@ public String toString() {
* @return the decoded key/value pair
*/
public static <K, V> KeyValue<K, V> decode(byte[] data, KeyValueDecoder<K, V> decoder) {
+ if (data == null || data.length == 0) {
Review comment:
Yes, https://github.com/apache/pulsar/pull/10089 is possibly the root cause. The KeyValue with a null value was decoded as INLINED, so this protection https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/KeyValueSchema.java#L146 was not working as expected.
--
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 pull request #10085: Fix NPE in KeyValue when the message payload is null
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10085:
URL: https://github.com/apache/pulsar/pull/10085#issuecomment-810315372
it looks like we can close this PR in favour of #10089
correct ?
--
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 #10085: Fix NPE in KeyValue when the message payload is null
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10085:
URL: https://github.com/apache/pulsar/pull/10085#discussion_r604014510
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/common/schema/KeyValue.java
##########
@@ -130,6 +132,10 @@ public String toString() {
* @return the decoded key/value pair
*/
public static <K, V> KeyValue<K, V> decode(byte[] data, KeyValueDecoder<K, V> decoder) {
+ if (data == null || data.length == 0) {
Review comment:
@vroyer I still do not see how we can have a zero byte array from your example.
if we have `keyValueEncodingType == KeyValueEncodingType.SEPARATED` then we are not using the KeyValue encoding, but the value contains only the serialized version of the value (`valueSchema.encode(message.getValue())` ).
this "decode" code should be used only when you read an KeyValueEncodingType.INLINE message, that contains both the key and the value inside the payload of the message:
- keylen
- key bytes
- valuelen
- value bytes
when you serialize a null KeyValue you can a null value
when you serialize a null key you see -1 as "keylen"
when you serialize a null value you see -1 as "valuelen"
there is no way to see a zero byte array
--
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 closed pull request #10085: Fix NPE in KeyValue when the message payload is null
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #10085:
URL: https://github.com/apache/pulsar/pull/10085
--
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