diff --git a/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java b/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java index 319c0d1f1dc8..5e4db78ac823 100644 --- a/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java +++ b/core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java @@ -19,8 +19,10 @@ package org.apache.druid.guice; +import com.fasterxml.jackson.databind.AnnotationIntrospector; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.introspect.AnnotationIntrospectorPair; +import com.google.common.annotations.VisibleForTesting; import com.google.inject.Binder; import com.google.inject.Inject; import com.google.inject.Injector; @@ -83,14 +85,28 @@ public ObjectMapper getSmileMapper(Injector injector) return smileMapper; } - private void setupJackson(Injector injector, final ObjectMapper mapper) + @VisibleForTesting + public static void setupJackson(Injector injector, final ObjectMapper mapper) { - final GuiceAnnotationIntrospector guiceIntrospector = new GuiceAnnotationIntrospector(); - mapper.setInjectableValues(new GuiceInjectableValues(injector)); + setupAnnotationIntrospector(mapper, new GuiceAnnotationIntrospector()); + } + + @VisibleForTesting + public static void setupAnnotationIntrospector( + final ObjectMapper mapper, + final AnnotationIntrospector annotationIntrospector + ) + { mapper.setAnnotationIntrospectors( - new AnnotationIntrospectorPair(guiceIntrospector, mapper.getSerializationConfig().getAnnotationIntrospector()), - new AnnotationIntrospectorPair(guiceIntrospector, mapper.getDeserializationConfig().getAnnotationIntrospector()) + new AnnotationIntrospectorPair( + annotationIntrospector, + mapper.getSerializationConfig().getAnnotationIntrospector() + ), + new AnnotationIntrospectorPair( + annotationIntrospector, + mapper.getDeserializationConfig().getAnnotationIntrospector() + ) ); } } diff --git a/core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java b/core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java index 7862c977ee27..29fac1ae3be1 100644 --- a/core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java +++ b/core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java @@ -20,8 +20,12 @@ package org.apache.druid.guice; import com.fasterxml.jackson.annotation.JacksonInject; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.introspect.Annotated; import com.fasterxml.jackson.databind.introspect.AnnotatedMember; import com.fasterxml.jackson.databind.introspect.AnnotatedMethod; +import com.fasterxml.jackson.databind.introspect.AnnotatedParameter; import com.fasterxml.jackson.databind.introspect.NopAnnotationIntrospector; import com.google.inject.BindingAnnotation; import com.google.inject.Key; @@ -56,4 +60,54 @@ public Object findInjectableValueId(AnnotatedMember m) } return Key.get(m.getGenericType(), guiceAnnotation); } + + /** + * This method is used to find what property to ignore in deserialization. Jackson calls this method + * per every class and every constructor parameter. + * + * This implementation returns a {@link JsonIgnoreProperties.Value#empty()} that allows empty names if + * the parameters has the {@link JsonProperty} annotation. Otherwise, it returns + * {@code JsonIgnoreProperties.Value.forIgnoredProperties("")} that does NOT allow empty names. + * This behavior is to work around a bug in Jackson deserializer (see the below comment for details) and + * can be removed in the future after the bug is fixed. + * For example, suppose a constructor like below: + * + *
{@code
+ * @JsonCreator
+ * public ClassWithJacksonInject(
+ * @JsonProperty("val") String val,
+ * @JacksonInject InjectedParameter injected
+ * )
+ * }
+ *
+ * During deserializing a JSON string into this class, this method will be called at least twice,
+ * one for {@code val} and another for {@code injected}. It will return {@code Value.empty()} for {@code val},
+ * while {Value.forIgnoredProperties("")} for {@code injected} because the later does not have {@code JsonProperty}.
+ * As a result, {@code injected} will be ignored during deserialization since it has no name.
+ */
+ @Override
+ public JsonIgnoreProperties.Value findPropertyIgnorals(Annotated ac)
+ {
+ // We should not allow empty names in any case. However, there is a known bug in Jackson deserializer
+ // with ignorals (_arrayDelegateDeserializer is not copied when creating a contextual deserializer.
+ // See https://github.com/FasterXML/jackson-databind/issues/3022 for more details), which makes array
+ // deserialization failed even when the array is a valid field. To work around this bug, we return
+ // an empty ignoral when the given Annotated is a parameter with JsonProperty that needs to be deserialized.
+ // This is valid because every property with JsonProperty annoation should have a non-empty name.
+ // We can simply remove the below check after the Jackson bug is fixed.
+ //
+ // This check should be fine for so-called delegate creators that have only one argument without
+ // JsonProperty annotation, because this method is not even called for the argument of
+ // delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
+ // is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
+ // Jackson perhaps is able to just deserialize it without introspection.
+ if (ac instanceof AnnotatedParameter) {
+ final AnnotatedParameter ap = (AnnotatedParameter) ac;
+ if (ap.hasAnnotation(JsonProperty.class)) {
+ return JsonIgnoreProperties.Value.empty();
+ }
+ }
+
+ return JsonIgnoreProperties.Value.forIgnoredProperties("");
+ }
}
diff --git a/core/src/test/java/org/apache/druid/data/input/impl/DimensionsSpecSerdeTest.java b/core/src/test/java/org/apache/druid/data/input/impl/DimensionsSpecSerdeTest.java
index 45bb9156f508..9b9b90adea61 100644
--- a/core/src/test/java/org/apache/druid/data/input/impl/DimensionsSpecSerdeTest.java
+++ b/core/src/test/java/org/apache/druid/data/input/impl/DimensionsSpecSerdeTest.java
@@ -19,9 +19,12 @@
package org.apache.druid.data.input.impl;
+import com.fasterxml.jackson.databind.AnnotationIntrospector;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import junit.framework.Assert;
+import org.apache.druid.guice.DruidSecondaryModule;
+import org.apache.druid.guice.GuiceAnnotationIntrospector;
import org.junit.Test;
import java.util.Arrays;
@@ -34,6 +37,8 @@ public class DimensionsSpecSerdeTest
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
static {
+ AnnotationIntrospector introspector = new GuiceAnnotationIntrospector();
+ DruidSecondaryModule.setupAnnotationIntrospector(OBJECT_MAPPER, introspector);
OBJECT_MAPPER.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
}
diff --git a/core/src/test/java/org/apache/druid/data/input/impl/StringDimensionSchemaTest.java b/core/src/test/java/org/apache/druid/data/input/impl/StringDimensionSchemaTest.java
new file mode 100644
index 000000000000..ad2909722d2f
--- /dev/null
+++ b/core/src/test/java/org/apache/druid/data/input/impl/StringDimensionSchemaTest.java
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.druid.data.input.impl;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.AnnotationIntrospector;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.druid.data.input.impl.DimensionSchema.MultiValueHandling;
+import org.apache.druid.guice.DruidSecondaryModule;
+import org.apache.druid.guice.GuiceAnnotationIntrospector;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class StringDimensionSchemaTest
+{
+ private final ObjectMapper jsonMapper;
+
+ public StringDimensionSchemaTest()
+ {
+ jsonMapper = new ObjectMapper();
+ AnnotationIntrospector introspector = new GuiceAnnotationIntrospector();
+ DruidSecondaryModule.setupAnnotationIntrospector(jsonMapper, introspector);
+ jsonMapper.registerSubtypes(StringDimensionSchema.class);
+ }
+
+ @Test
+ public void testDeserializeFromSimpleString() throws JsonProcessingException
+ {
+ final String json = "\"dim\"";
+ final StringDimensionSchema schema = (StringDimensionSchema) jsonMapper.readValue(json, DimensionSchema.class);
+ Assert.assertEquals(new StringDimensionSchema("dim"), schema);
+ }
+
+ @Test
+ public void testDeserializeFromJson() throws JsonProcessingException
+ {
+ final String json = "{\n"
+ + " \"type\" : \"StringDimensionSchema\",\n"
+ + " \"name\" : \"dim\",\n"
+ + " \"multiValueHandling\" : \"SORTED_SET\",\n"
+ + " \"createBitmapIndex\" : false\n"
+ + "}";
+ final StringDimensionSchema schema = (StringDimensionSchema) jsonMapper.readValue(json, DimensionSchema.class);
+ Assert.assertEquals(new StringDimensionSchema("dim", MultiValueHandling.SORTED_SET, false), schema);
+ }
+}
diff --git a/core/src/test/java/org/apache/druid/guice/DruidSecondaryModuleTest.java b/core/src/test/java/org/apache/druid/guice/DruidSecondaryModuleTest.java
new file mode 100644
index 000000000000..12ab2ddf0271
--- /dev/null
+++ b/core/src/test/java/org/apache/druid/guice/DruidSecondaryModuleTest.java
@@ -0,0 +1,353 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.druid.guice;
+
+import com.fasterxml.jackson.annotation.JacksonInject;
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonValue;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableList;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import com.google.inject.Module;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+import javax.annotation.Nullable;
+import javax.validation.Validation;
+import javax.validation.Validator;
+import java.util.List;
+import java.util.Properties;
+
+@RunWith(Enclosed.class)
+public class DruidSecondaryModuleTest
+{
+ private static final String PROPERTY_NAME = "druid.injected.val";
+ private static final String PROPERTY_VALUE = "this is the legit val";
+
+ public static class ConstructorWithJacksonInjectTest
+ {
+ @Test
+ public void testInjectWithAnEmptyPropertyNotOverrideInjection() throws JsonProcessingException
+ {
+ final Properties props = new Properties();
+ props.setProperty(PROPERTY_NAME, PROPERTY_VALUE);
+
+ final Injector injector = makeInjectorWithProperties(props);
+ final ObjectMapper mapper = makeObjectMapper(injector);
+ final String json = "{\"test\": \"this is an injection test\", \"\": \"nice try\" }";
+ final ClassWithJacksonInject object = mapper.readValue(json, ClassWithJacksonInject.class);
+ Assert.assertEquals("this is an injection test", object.test);
+ Assert.assertEquals(PROPERTY_VALUE, object.injected.val);
+ }
+
+ @Test
+ public void testInjectNormal() throws JsonProcessingException
+ {
+ final Properties props = new Properties();
+ props.setProperty(PROPERTY_NAME, PROPERTY_VALUE);
+
+ final Injector injector = makeInjectorWithProperties(props);
+ final ObjectMapper mapper = makeObjectMapper(injector);
+ final String json = "{\"test\": \"this is an injection test\" }";
+ final ClassWithJacksonInject object = mapper.readValue(json, ClassWithJacksonInject.class);
+ Assert.assertEquals("this is an injection test", object.test);
+ Assert.assertEquals(PROPERTY_VALUE, object.injected.val);
+ }
+
+ @Test
+ public void testInjectClassWithEmptyProperty() throws JsonProcessingException
+ {
+ final Properties props = new Properties();
+ props.setProperty(PROPERTY_NAME, PROPERTY_VALUE);
+
+ final Injector injector = makeInjectorWithProperties(props);
+ final ObjectMapper mapper = makeObjectMapper(injector);
+ final String json = "{\"test\": \"this is an injection test\", \"\": \"nice try\" }";
+ final ClassWithEmptyProperty object = mapper.readValue(json, ClassWithEmptyProperty.class);
+ Assert.assertEquals("this is an injection test", object.test);
+ Assert.assertEquals(PROPERTY_VALUE, object.injected.val);
+ }
+
+ private static class ClassWithJacksonInject
+ {
+ private final String test;
+
+ private InjectedParameter injected;
+
+ @JsonCreator
+ public ClassWithJacksonInject(
+ @JsonProperty("test") String test,
+ @JacksonInject InjectedParameter injected
+ )
+ {
+ this.test = test;
+ this.injected = injected;
+ }
+
+ @JsonProperty
+ public String getTest()
+ {
+ return test;
+ }
+ }
+
+ private static class ClassWithEmptyProperty
+ {
+ private final String test;
+
+ private InjectedParameter injected;
+
+ @JsonCreator
+ public ClassWithEmptyProperty(
+ @JsonProperty("test") String test,
+ @JacksonInject @JsonProperty("") InjectedParameter injected
+ )
+ {
+ this.test = test;
+ this.injected = injected;
+ }
+
+ @JsonProperty
+ public String getTest()
+ {
+ return test;
+ }
+ }
+ }
+
+ public static class ConstructorWithoutJacksonInjectTest
+ {
+ @Test
+ public void testInjectionWithEmptyPropertyName() throws JsonProcessingException
+ {
+ final Properties props = new Properties();
+
+ final Injector injector = makeInjectorWithProperties(props);
+ final ObjectMapper mapper = makeObjectMapper(injector);
+ final String json = "[\"this is\", \"an injection test\"]";
+ final ClassWithConstructorOfEmptyName object = mapper.readValue(json, ClassWithConstructorOfEmptyName.class);
+ Assert.assertEquals(ImmutableList.of("this is", "an injection test"), object.getTest());
+ }
+
+ @Test
+ public void testInjectEmptyListWithEmptyPropertyName() throws JsonProcessingException
+ {
+ final Properties props = new Properties();
+
+ final Injector injector = makeInjectorWithProperties(props);
+ final ObjectMapper mapper = makeObjectMapper(injector);
+ final String json = "[]";
+ final ClassWithConstructorOfEmptyName object = mapper.readValue(json, ClassWithConstructorOfEmptyName.class);
+ Assert.assertEquals(ImmutableList.of(), object.getTest());
+ }
+
+ @Test
+ public void testInjectClassWithFactoryMethodOfEmptyPropertyName() throws JsonProcessingException
+ {
+ final Properties props = new Properties();
+
+ final Injector injector = makeInjectorWithProperties(props);
+ final ObjectMapper mapper = makeObjectMapper(injector);
+ final String json = "[\"this is\", \"an injection test\"]";
+ final ClassWithFactoryMethodOfEmptyName object = mapper.readValue(json, ClassWithFactoryMethodOfEmptyName.class);
+ Assert.assertEquals(ImmutableList.of("this is", "an injection test"), object.getTest());
+ }
+
+ @Test
+ public void testInjectEmptyListToClassWithFactoryMethodOfEmptyPropertyName() throws JsonProcessingException
+ {
+ final Properties props = new Properties();
+
+ final Injector injector = makeInjectorWithProperties(props);
+ final ObjectMapper mapper = makeObjectMapper(injector);
+ final String json = "[]";
+ final ClassWithFactoryMethodOfEmptyName object = mapper.readValue(json, ClassWithFactoryMethodOfEmptyName.class);
+ Assert.assertEquals(ImmutableList.of(), object.getTest());
+ }
+
+ @Test
+ public void testInjectClassOfEmptyConstructor() throws JsonProcessingException
+ {
+ final Properties props = new Properties();
+ props.setProperty(PROPERTY_NAME, PROPERTY_VALUE);
+
+ final Injector injector = makeInjectorWithProperties(props);
+ final ObjectMapper mapper = makeObjectMapper(injector);
+ final String json = "{}";
+ final ClassOfEmptyConstructor object = mapper.readValue(json, ClassOfEmptyConstructor.class);
+ Assert.assertEquals("empty constructor", object.val);
+ }
+
+ private static class ClassWithConstructorOfEmptyName
+ {
+ private final List