Skip to content

Commit

Permalink
fix: NPE in ProtobufDescriptorParserImpl for repeated nested repeated…
Browse files Browse the repository at this point in the history 
… fields (#6402)

Fixes #6393

---------

Co-authored-by: Devin Smith <devinsmith@deephaven.io>
  • Loading branch information
@jcferretti @devinrsmith
jcferretti and devinrsmith committed Dec 2, 2024
1 parent 4946cba commit 78a7bf3
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 10 deletions.
25 changes: 15 additions & 10 deletions extensions/protobuf/src/main/java/io/deephaven/protobuf/ProtobufDescriptorParserImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import io.deephaven.function.ToShortFunction;
import io.deephaven.function.TypedFunction;
import io.deephaven.function.TypedFunction.Visitor;
import io.deephaven.util.QueryConstants;

import java.lang.reflect.Array;
import java.util.HashMap;
Expand Down Expand Up @@ -362,40 +361,46 @@ private ProtobufFunctions functions() {
}
}

private <T> ToObjectFunction<Message, T> maybeBypass(ToObjectFunction<Message, T> f) {
// Ideally, we could be very targetted in our application of null checks; in a lot of contexts, our
// implementation could know it will never be called with a null message to produce an array.
return BypassOnNull.of(f);
}

private ToObjectFunction<Message, char[]> mapChars(ToCharFunction<Object> f) {
return ToObjectFunction.of(m -> toChars(m, fd, f), Type.charType().arrayType());
return maybeBypass(ToObjectFunction.of(m -> toChars(m, fd, f), Type.charType().arrayType()));
}

private ToObjectFunction<Message, byte[]> mapBytes(ToByteFunction<Object> f) {
return ToObjectFunction.of(m -> toBytes(m, fd, f), Type.byteType().arrayType());
return maybeBypass(ToObjectFunction.of(m -> toBytes(m, fd, f), Type.byteType().arrayType()));
}

private ToObjectFunction<Message, short[]> mapShorts(ToShortFunction<Object> f) {
return ToObjectFunction.of(m -> toShorts(m, fd, f), Type.shortType().arrayType());
return maybeBypass(ToObjectFunction.of(m -> toShorts(m, fd, f), Type.shortType().arrayType()));
}

private ToObjectFunction<Message, int[]> mapInts(ToIntFunction<Object> f) {
return ToObjectFunction.of(m -> toInts(m, fd, f), Type.intType().arrayType());
return maybeBypass(ToObjectFunction.of(m -> toInts(m, fd, f), Type.intType().arrayType()));
}

private ToObjectFunction<Message, long[]> mapLongs(ToLongFunction<Object> f) {
return ToObjectFunction.of(m -> toLongs(m, fd, f), Type.longType().arrayType());
return maybeBypass(ToObjectFunction.of(m -> toLongs(m, fd, f), Type.longType().arrayType()));
}

private ToObjectFunction<Message, float[]> mapFloats(ToFloatFunction<Object> f) {
return ToObjectFunction.of(m -> toFloats(m, fd, f), Type.floatType().arrayType());
return maybeBypass(ToObjectFunction.of(m -> toFloats(m, fd, f), Type.floatType().arrayType()));
}

private ToObjectFunction<Message, double[]> mapDoubles(ToDoubleFunction<Object> f) {
return ToObjectFunction.of(m -> toDoubles(m, fd, f), Type.doubleType().arrayType());
return maybeBypass(ToObjectFunction.of(m -> toDoubles(m, fd, f), Type.doubleType().arrayType()));
}

private ToObjectFunction<Message, boolean[]> mapBooleans(ToBooleanFunction<Object> f) {
return ToObjectFunction.of(m -> toBooleans(m, fd, f), Type.booleanType().arrayType());
return maybeBypass(ToObjectFunction.of(m -> toBooleans(m, fd, f), Type.booleanType().arrayType()));
}

private <T> ToObjectFunction<Message, T[]> mapGenerics(ToObjectFunction<Object, T> f) {
return ToObjectFunction.of(message -> toArray(message, fd, f), f.returnType().arrayType());
return maybeBypass(ToObjectFunction.of(message -> toArray(message, fd, f), f.returnType().arrayType()));
}

private class ToRepeatedType implements
Expand Down
106 changes: 106 additions & 0 deletions extensions/protobuf/src/test/java/io/deephaven/protobuf/ProtobufDescriptorParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import io.deephaven.protobuf.test.ByteWrapperRepeated;
import io.deephaven.protobuf.test.FieldMaskWrapper;
import io.deephaven.protobuf.test.MultiRepeated;
import io.deephaven.protobuf.test.NestedArrays;
import io.deephaven.protobuf.test.NestedByteWrapper;
import io.deephaven.protobuf.test.NestedRepeatedTimestamps;
import io.deephaven.protobuf.test.NestedRepeatedTimestamps.Timestamps;
Expand Down Expand Up @@ -1470,6 +1471,111 @@ void twoTimestampsOneAsWellKnown() {
assertThat(nf.keySet()).containsExactly(List.of("ts1"), List.of("ts2", "seconds"), List.of("ts2", "nanos"));
}

@Test
void nestedArraysADirect() {
checkKey(
NestedArrays.getDescriptor(),
List.of("a_direct", "b", "c"),
Type.stringType().arrayType(),
new HashMap<>() {
{
put(NestedArrays.getDefaultInstance(), null);

put(NestedArrays.newBuilder()
.setADirect(NestedArrays.A.getDefaultInstance())
.build(), null);

// c is only non-null when b has been explicitly set

put(NestedArrays.newBuilder()
.setADirect(NestedArrays.A.newBuilder()
.setB(NestedArrays.B.getDefaultInstance())
.build())
.build(), new String[0]);

put(NestedArrays.newBuilder()
.setADirect(NestedArrays.A.newBuilder()
.setB(NestedArrays.B.newBuilder()
.addC("Foo")
.addC("Bar")
.build())
.build())
.build(), new String[] {"Foo", "Bar"});
}
});
}

@Test
void nestedArraysARepeated() {
checkKey(
NestedArrays.getDescriptor(),
List.of("a_repeated", "b", "c"),
Type.stringType().arrayType().arrayType(),
new HashMap<>() {
{
put(NestedArrays.getDefaultInstance(), new String[0][]);
put(NestedArrays.newBuilder()
.addARepeated(NestedArrays.A.getDefaultInstance())
.addARepeated(NestedArrays.A.newBuilder()
.setB(NestedArrays.B.getDefaultInstance())
.build())
.addARepeated(NestedArrays.A.newBuilder()
.setB(NestedArrays.B.newBuilder()
.addC("Foo")
.addC("Bar")
.build())
.build())
.build(), new String[][] {null, new String[0], new String[] {"Foo", "Bar"}});
}
});
}

@Test
void nestedArraysBDirect() {
checkKey(
NestedArrays.getDescriptor(),
List.of("b_direct", "c"),
Type.stringType().arrayType(),
new HashMap<>() {
{
put(NestedArrays.getDefaultInstance(), null);

put(NestedArrays.newBuilder()
.setBDirect(NestedArrays.B.getDefaultInstance())
.build(), new String[0]);

put(NestedArrays.newBuilder()
.setBDirect(NestedArrays.B.newBuilder()
.addC("Foo")
.addC("Bar")
.build())
.build(), new String[] {"Foo", "Bar"});
}
});
}

@Test
void nestedArraysBRepeated() {
checkKey(
NestedArrays.getDescriptor(),
List.of("b_repeated", "c"),
Type.stringType().arrayType().arrayType(),
new HashMap<>() {
{
put(NestedArrays.getDefaultInstance(), new String[0][]);

put(NestedArrays.newBuilder()
.addBRepeated(NestedArrays.B.getDefaultInstance())
.addBRepeated(NestedArrays.B.newBuilder()
.addC("Foo")
.addC("Bar")
.build())

.build(), new String[][] {new String[0], new String[] {"Foo", "Bar"}});
}
});
}

private static Map<List<String>, TypedFunction<Message>> nf(Descriptor descriptor) {
return nf(descriptor, ProtobufDescriptorParserOptions.defaults());
}
Expand Down
15 changes: 15 additions & 0 deletions extensions/protobuf/src/test/proto/mytest.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,21 @@ message RepeatedObject {
repeated XYZ xyz = 1;
}

message NestedArrays {
message A {
B b = 1;
}
message B {
repeated string c = 1;
}

A a_direct = 1;
repeated A a_repeated = 2;

B b_direct = 3;
repeated B b_repeated = 4;
}

message MultiRepeated {
repeated RepeatedBasics my_basics = 1;
repeated RepeatedWrappers my_wrappers = 2;
Expand Down

0 comments on commit 78a7bf3

Please sign in to comment.