deephaven / deephaven-core

Deephaven Community Core
Other
257 stars 80 forks source link

fix: NPE in ProtobufDescriptorParserImpl for repeated nested repeated fields #6402

Open jcferretti opened 1 day ago

jcferretti commented 1 day ago

Fixes https://github.com/deephaven/deephaven-core/issues/6393

jcferretti commented 1 day ago

Note the current state of the PR is draft and the fix is not in, just the test to demonstrate the failure with a self-contained Core test. The fix would be to change around line 224 of ProtobufDescriptorParserImpl, and remove the parentFieldIsRepeated boolean (effectively make the current code behave as is parentFieldIsRepeated is always false, meaning, always apply BypassOnNull, like so:

                    case MESSAGE: {
                        final ToObjectFunction<Message, Message> fieldAsMessage = mapToObj(MESSAGE_OBJ);
                        final DescriptorContext messageContext = toMessageContext();
                        final ProtobufFunctions subF = messageContext.functions();
                        final Builder builder = ProtobufFunctions.builder();
                        for (ProtobufFunction e : subF.functions()) {
                            final TypedFunction<Message> value = BypassOnNull.of(e.function());
                            builder.addFunctions(
                                    ProtobufFunction.of(prepend(e.path(), fd), fieldAsMessage.map(value)));
                        }
                        return builder.build();
                    }

With the current code as in in the PR now (meaning, without that fix): image

With the fix both new test cases pass, without it both fail (two test cases because originally I thought the issue was related to an optional message in between repeated nesting, but it doesn't seem to matter).