From a8e4616427292d9829cdbc9e66cf9fa3d8643d44 Mon Sep 17 00:00:00 2001 From: Choko Date: Fri, 9 Feb 2024 11:53:35 +0900 Subject: [PATCH] Do not assume interning for non-ParserBase parsers (#28) --- .../common/protobuf/json/ParseSupport.java | 16 ++++++++- .../protobuf/json/JacksonInteropTest.java | 35 +++++++++++++++++++ .../protobuf/json/ObjectMapperTest.java | 34 +++++++++++------- 3 files changed, 71 insertions(+), 14 deletions(-) create mode 100644 src/test/java/org/curioswitch/common/protobuf/json/JacksonInteropTest.java diff --git a/src/main/java/org/curioswitch/common/protobuf/json/ParseSupport.java b/src/main/java/org/curioswitch/common/protobuf/json/ParseSupport.java index 8512303..14061e4 100644 --- a/src/main/java/org/curioswitch/common/protobuf/json/ParseSupport.java +++ b/src/main/java/org/curioswitch/common/protobuf/json/ParseSupport.java @@ -8,6 +8,7 @@ import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonToken; +import com.fasterxml.jackson.core.base.ParserBase; import com.fasterxml.jackson.core.exc.InputCoercionException; import com.fasterxml.jackson.core.io.NumberInput; import com.google.protobuf.ByteString; @@ -446,7 +447,20 @@ public static void checkRecursionLimit(int currentDepth) throws InvalidProtocolB */ @SuppressWarnings("ReferenceEquality") public static boolean fieldNamesEqual(JsonParser parser, String name1, String name2) { - return name1 == name2; + if (parser instanceof ParserBase) { + // The standard Jackson parsers all inherit from ParserBase and default to interning field + // names. + // It is possible to disable interning which will cause us to fail, so far we have not had a + // user + // run into this problem for ParserBase implementations but will see if there is a good way to + // make this more robust. + + // TODO(chokoswitch): Check if parser has interning enabled and avoid reference equality if + // so. + return name1 == name2; + } + + return name1.equals(name2); } /** Parses a long out of the input, using the optimized path when the value is not quoted. */ diff --git a/src/test/java/org/curioswitch/common/protobuf/json/JacksonInteropTest.java b/src/test/java/org/curioswitch/common/protobuf/json/JacksonInteropTest.java new file mode 100644 index 0000000..b2daf43 --- /dev/null +++ b/src/test/java/org/curioswitch/common/protobuf/json/JacksonInteropTest.java @@ -0,0 +1,35 @@ +/* + * Copyright (c) Choko (choko@curioswitch.org) + * SPDX-License-Identifier: MIT + */ + +package org.curioswitch.common.protobuf.json; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.fasterxml.jackson.core.JsonFactory; +import com.fasterxml.jackson.core.JsonFactoryBuilder; +import com.fasterxml.jackson.core.JsonParser; +import com.google.protobuf.util.JsonFormat; +import com.google.protobuf.util.JsonTestProto.TestAllTypes; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +class JacksonInteropTest { + @Test + @Disabled // Currently we do not support non-interned field names. + void interningDisabled() throws Exception { + JsonFactory factory = + new JsonFactoryBuilder().configure(JsonFactory.Feature.INTERN_FIELD_NAMES, false).build(); + + String json = JsonFormat.printer().print(JsonTestUtil.testAllTypesAllFields()); + + MessageMarshaller marshaller = MessageMarshaller.builder().register(TestAllTypes.class).build(); + + TestAllTypes.Builder builder = TestAllTypes.newBuilder(); + + JsonParser parser = factory.createParser(json); + marshaller.mergeValue(parser, builder); + assertThat(builder.build()).isEqualTo(JsonTestUtil.testAllTypesAllFields()); + } +} diff --git a/src/testDatabind/java/org/curioswitch/common/protobuf/json/ObjectMapperTest.java b/src/testDatabind/java/org/curioswitch/common/protobuf/json/ObjectMapperTest.java index ce15a85..24051bf 100644 --- a/src/testDatabind/java/org/curioswitch/common/protobuf/json/ObjectMapperTest.java +++ b/src/testDatabind/java/org/curioswitch/common/protobuf/json/ObjectMapperTest.java @@ -7,19 +7,19 @@ import static org.assertj.core.api.Assertions.assertThat; -import com.fasterxml.jackson.core.JsonFactory; import com.fasterxml.jackson.core.JsonGenerator; import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.databind.DeserializationContext; -import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.JsonSerializer; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.SerializerProvider; import com.fasterxml.jackson.databind.deser.std.StdDeserializer; import com.fasterxml.jackson.databind.module.SimpleModule; import com.fasterxml.jackson.databind.ser.std.StdSerializer; -import com.google.protobuf.util.JsonFormat; +import com.fasterxml.jackson.databind.util.TokenBuffer; +import com.google.protobuf.Struct; +import com.google.protobuf.Value; import com.google.protobuf.util.JsonTestProto; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -120,26 +120,34 @@ void doesNotCloseJsonParser() throws Exception { } @Test - @SuppressWarnings("deprecation") // Test old API. - void treeAsTokens() throws Exception { - JsonTestProto.TestAllTypes message = - JsonTestProto.TestAllTypes.newBuilder().setOptionalInt32(10).build(); - + void tokenBuffer() throws Exception { MessageMarshaller marshaller = MessageMarshaller.builder() .register(JsonTestProto.TestAllTypes.getDefaultInstance()) .build(); + Value original = + Value.newBuilder() + .setStructValue( + Struct.newBuilder() + // Allocate field name with new String(), meaning the key is not interned + // automatically + .putFields( + new String("optional_float"), + Value.newBuilder().setNumberValue(3.2).build()) + .build()) + .build(); + ObjectMapper mapper = new ObjectMapper(); - mapper.getFactory().configure(JsonFactory.Feature.INTERN_FIELD_NAMES, false); mapper.registerModule(MessageMarshallerModule.of(marshaller)); - String json = JsonFormat.printer().print(message); - JsonNode tree = mapper.readTree(json); - JsonParser parser = mapper.treeAsTokens(tree); + // Use a jackson TokenBuffer to convert the message to a JsonNode + TokenBuffer buffer = new TokenBuffer(mapper.getFactory().getCodec(), false); + mapper.writeValue(buffer, original); + JsonParser parser = buffer.asParser(); JsonTestProto.TestAllTypes.Builder builder = JsonTestProto.TestAllTypes.newBuilder(); marshaller.mergeValue(parser, builder); - assertThat(builder.build().getOptionalInt32()).isEqualTo(10); + assertThat(builder.build().getOptionalFloat()).isEqualTo(3.2f); } }