Fix string-to-long coercion when afterburner is enabled

carterkozak · carterkozak · commit 3239a08915bc · 2020-12-02T10:15:51.000-05:00
Reported upstream: FasterXML/jackson-modules-base#120 The CoercionConfig is fantastic, unfortunately it is not yet supported by the AfterBurner module. Separately we may consider moving to the new Blackbird module on java 11 runtimes. For now I've updated our existing long deserializers for 2.12.0 which has the benefit of rejecting coercion from string to SafeLong.
diff --git a/conjure-java-client-verifier/src/test/resources/ignored-test-cases.jersey.yml b/conjure-java-client-verifier/src/test/resources/ignored-test-cases.jersey.yml
@@ -99,8 +99,6 @@ client:
    - 'null'
    receiveMapEnumExampleAlias:
    - 'null'
-   receiveSafeLongExample: #allowed long coercion applies to safelong
-   - '{"value":"12"}'
 
   singleHeaderService: {}
 
diff --git a/conjure-java-client-verifier/src/test/resources/ignored-test-cases.retrofit.yml b/conjure-java-client-verifier/src/test/resources/ignored-test-cases.retrofit.yml
@@ -136,8 +136,6 @@ client:
    - 'null'
    receiveMapEnumExampleAlias:
    - 'null'
-   receiveSafeLongExample: #allowed long coercion applies to safelong
-   - '{"value":"12"}'
 
   singleHeaderService: {}
 
diff --git a/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/LenientLongModule.java b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/LenientLongModule.java
@@ -0,0 +1,94 @@
+/*
+ * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.conjure.java.serialization;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.cfg.CoercionAction;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.exc.InvalidFormatException;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.fasterxml.jackson.datatype.jdk8.OptionalLongDeserializer;
+import com.palantir.logsafe.exceptions.SafeIoException;
+import java.io.IOException;
+import java.util.OptionalLong;
+
+/**
+ * Provides support for the {@link Long} deserialization from JSON string and numeric values regardless of
+ *
+ * <pre>MapperFeature.ALLOW_COERCION_OF_SCALARS</pre>
+ *
+ * configuration.
+ */
+final class LenientLongModule extends SimpleModule {
+
+    LenientLongModule() {
+        super("lenient long");
+        // Register to both Long.TYPE and Long.class
+        this.addDeserializer(long.class, new LongAsStringDeserializer())
+                .addDeserializer(Long.class, new LongAsStringDeserializer())
+                .addDeserializer(OptionalLong.class, new OptionalLongAsStringDeserializer());
+    }
+
+    private static final class LongAsStringDeserializer extends StdDeserializer<Long> {
+
+        private LongAsStringDeserializer() {
+            super(Long.TYPE);
+        }
+
+        @Override
+        public Long deserialize(JsonParser jsonParser, DeserializationContext _ctxt) throws IOException {
+            switch (jsonParser.currentToken()) {
+                case VALUE_NUMBER_INT:
+                    return jsonParser.getLongValue();
+                case VALUE_STRING:
+                    return parseLong(jsonParser);
+                case VALUE_NULL:
+                    return null;
+                default:
+                    throw new SafeIoException("Expected a long value");
+            }
+        }
+
+        @Override
+        public boolean isCachable() {
+            return true;
+        }
+
+        private static Long parseLong(JsonParser jsonParser) throws IOException {
+            String value = jsonParser.getValueAsString();
+            try {
+                return Long.valueOf(value);
+            } catch (NumberFormatException e) {
+                InvalidFormatException failure =
+                        new InvalidFormatException(jsonParser, "not a valid long value", value, long.class);
+                failure.initCause(e);
+                throw failure;
+            }
+        }
+    }
+
+    private static final class OptionalLongAsStringDeserializer extends OptionalLongDeserializer {
+
+        private OptionalLongAsStringDeserializer() {}
+
+        @Override
+        protected CoercionAction _checkFromStringCoercion(DeserializationContext _ctxt, String _value) {
+            return CoercionAction.TryConvert;
+        }
+    }
+}
diff --git a/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java
@@ -20,15 +20,12 @@
 import com.fasterxml.jackson.databind.MapperFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.SerializationFeature;
-import com.fasterxml.jackson.databind.cfg.CoercionAction;
-import com.fasterxml.jackson.databind.cfg.CoercionInputShape;
 import com.fasterxml.jackson.dataformat.cbor.CBORFactory;
 import com.fasterxml.jackson.dataformat.smile.SmileFactory;
 import com.fasterxml.jackson.datatype.guava.GuavaModule;
 import com.fasterxml.jackson.datatype.jdk8.Jdk8Module;
 import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
 import com.fasterxml.jackson.module.afterburner.AfterburnerModule;
-import java.util.OptionalLong;
 
 public final class ObjectMappers {
 
@@ -131,14 +128,12 @@ public static ObjectMapper newSmileServerObjectMapper() {
      * </ul>
      */
     public static ObjectMapper withDefaultModules(ObjectMapper mapper) {
-        allowStringCoercion(mapper, long.class);
-        allowStringCoercion(mapper, Long.class);
-        allowStringCoercion(mapper, OptionalLong.class);
         return mapper.registerModule(new GuavaModule())
                 .registerModule(new ShimJdk7Module())
                 .registerModule(new Jdk8Module().configureAbsentsAsNulls(true))
                 .registerModule(new AfterburnerModule())
                 .registerModule(new JavaTimeModule())
+                .registerModule(new LenientLongModule())
                 .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
                 .disable(SerializationFeature.WRITE_DURATIONS_AS_TIMESTAMPS)
                 .disable(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE)
@@ -148,10 +143,4 @@ public static ObjectMapper withDefaultModules(ObjectMapper mapper) {
                 .disable(MapperFeature.ALLOW_COERCION_OF_SCALARS)
                 .disable(DeserializationFeature.ACCEPT_FLOAT_AS_INT);
     }
-
-    private static void allowStringCoercion(ObjectMapper mapper, Class<?> clazz) {
-        mapper.coercionConfigFor(clazz)
-                .setAcceptBlankAsEmpty(false)
-                .setCoercion(CoercionInputShape.String, CoercionAction.TryConvert);
-    }
 }
diff --git a/conjure-java-jackson-serialization/src/test/java/com/palantir/conjure/java/serialization/ObjectMappersTest.java b/conjure-java-jackson-serialization/src/test/java/com/palantir/conjure/java/serialization/ObjectMappersTest.java
@@ -19,12 +19,14 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.core.JsonParseException;
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.core.exc.InputCoercionException;
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.exc.InvalidFormatException;
+import com.palantir.logsafe.Preconditions;
 import java.io.File;
 import java.io.IOException;
 import java.math.BigInteger;
@@ -38,6 +40,7 @@
 import java.time.ZonedDateTime;
 import java.util.Collections;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.OptionalInt;
 import java.util.OptionalLong;
@@ -128,11 +131,115 @@ public void testLongTypeDeserializationFromString() throws IOException {
         assertThat(MAPPER.readValue("\"1\"", Long.TYPE)).isEqualTo(1L);
     }
 
+    @Test
+    public void testLongBeanTypeDeserializationFromString() throws IOException {
+        assertThat(MAPPER.readValue("{\"value\":\"1\"}", LongBean.class)).isEqualTo(new LongBean(1L));
+    }
+
+    @Test
+    public void testLongBeanTypeDeserializationFromNumber() throws IOException {
+        assertThat(MAPPER.readValue("{\"value\":\"1\"}", LongBean.class)).isEqualTo(new LongBean(1L));
+    }
+
+    static final class LongBean {
+        @JsonProperty
+        private long value;
+
+        LongBean() {}
+
+        LongBean(long value) {
+            setValue(value);
+        }
+
+        public long getValue() {
+            return value;
+        }
+
+        public void setValue(long value) {
+            this.value = value;
+        }
+
+        @Override
+        public boolean equals(Object other) {
+            if (this == other) {
+                return true;
+            }
+            if (other == null || getClass() != other.getClass()) {
+                return false;
+            }
+            LongBean that = (LongBean) other;
+            return value == that.value;
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hashCode(value);
+        }
+
+        @Override
+        public String toString() {
+            return "LongBean{value=" + value + '}';
+        }
+    }
+
     @Test
     public void testOptionalLongTypeDeserializationFromString() throws IOException {
         assertThat(MAPPER.readValue("\"1\"", OptionalLong.class)).hasValue(1L);
     }
 
+    @Test
+    public void testOptionalLongBeanTypeDeserializationFromString() throws IOException {
+        assertThat(MAPPER.readValue("{\"value\":\"1\"}", OptionalLongBean.class))
+                .isEqualTo(new OptionalLongBean(OptionalLong.of(1L)));
+    }
+
+    @Test
+    public void testOptionalLongBeanTypeDeserializationFromNumber() throws IOException {
+        assertThat(MAPPER.readValue("{\"value\":1}", OptionalLongBean.class))
+                .isEqualTo(new OptionalLongBean(OptionalLong.of(1L)));
+    }
+
+    static final class OptionalLongBean {
+        @JsonProperty
+        private OptionalLong value = OptionalLong.empty();
+
+        OptionalLongBean() {}
+
+        OptionalLongBean(OptionalLong value) {
+            setValue(value);
+        }
+
+        public OptionalLong getValue() {
+            return value;
+        }
+
+        public void setValue(OptionalLong value) {
+            this.value = Preconditions.checkNotNull(value, "value");
+        }
+
+        @Override
+        public boolean equals(Object other) {
+            if (this == other) {
+                return true;
+            }
+            if (other == null || getClass() != other.getClass()) {
+                return false;
+            }
+            OptionalLongBean that = (OptionalLongBean) other;
+            return value.equals(that.value);
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hashCode(value);
+        }
+
+        @Override
+        public String toString() {
+            return "OptionalLongBean{value=" + value + '}';
+        }
+    }
+
     @Test
     public void testLongDeserializationFromJsonNumber() throws IOException {
         assertThat(MAPPER.readValue("1", Long.class)).isEqualTo(1L);
