Do not assume interning for non-ParserBase parsers (#28)

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. */

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());
+  }
+}

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);
   }
 }