From 8718155f8ffa8c9d34877cbdb7f8eb86df383e29 Mon Sep 17 00:00:00 2001 From: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Date: Thu, 11 Feb 2021 00:49:57 +0530 Subject: [PATCH] Allow for empty keys in hash map (#10869) * allow for empty keys in hash map * fix serde test --- .../guice/GuiceAnnotationIntrospector.java | 19 ++- .../druid/guice/DruidSecondaryModuleTest.java | 157 +++++++++++++++++- .../MapLookupExtractionFnSerDeTest.java | 13 +- 3 files changed, 180 insertions(+), 9 deletions(-) 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 29fac1ae3be..ce49253e0ff 100644 --- a/core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java +++ b/core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java @@ -23,6 +23,7 @@ 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.AnnotatedClass; import com.fasterxml.jackson.databind.introspect.AnnotatedMember; import com.fasterxml.jackson.databind.introspect.AnnotatedMethod; import com.fasterxml.jackson.databind.introspect.AnnotatedParameter; @@ -32,6 +33,7 @@ import com.google.inject.Key; import org.apache.druid.java.util.common.IAE; import java.lang.annotation.Annotation; +import java.util.Map; /** */ @@ -101,6 +103,7 @@ public class GuiceAnnotationIntrospector extends NopAnnotationIntrospector // 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)) { @@ -108,6 +111,20 @@ public class GuiceAnnotationIntrospector extends NopAnnotationIntrospector } } - return JsonIgnoreProperties.Value.forIgnoredProperties(""); + // A map can have empty string keys e.g. https://github.com/apache/druid/issues/10859. By returning empty ignored + // list for map, we can allow for empty string keys in a map. A nested class within map + // can still be annotated with JacksonInject and still be non-deserializable from user input + // Refer to {@link com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createMapDeserializer} for details + // on how the ignored list is passed to map deserializer + if (ac instanceof AnnotatedClass) { + final AnnotatedClass aClass = (AnnotatedClass) ac; + if (Map.class.isAssignableFrom(aClass.getAnnotated())) { + return JsonIgnoreProperties.Value.empty(); + } + } + + // We will allow serialization on empty properties. Properties marked with {@code @JacksonInject} are still + // not serialized if there is no getter marked with {@code @JsonProperty} + return JsonIgnoreProperties.Value.forIgnoredProperties("").withAllowGetters(); } } diff --git a/core/src/test/java/org/apache/druid/guice/DruidSecondaryModuleTest.java b/core/src/test/java/org/apache/druid/guice/DruidSecondaryModuleTest.java index 12ab2ddf027..ddda5a5de82 100644 --- a/core/src/test/java/org/apache/druid/guice/DruidSecondaryModuleTest.java +++ b/core/src/test/java/org/apache/druid/guice/DruidSecondaryModuleTest.java @@ -26,6 +26,7 @@ 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.common.collect.ImmutableMap; import com.google.inject.Guice; import com.google.inject.Injector; import com.google.inject.Module; @@ -38,6 +39,8 @@ import javax.annotation.Nullable; import javax.validation.Validation; import javax.validation.Validator; import java.util.List; +import java.util.Map; +import java.util.Objects; import java.util.Properties; @RunWith(Enclosed.class) @@ -90,6 +93,89 @@ public class DruidSecondaryModuleTest Assert.assertEquals(PROPERTY_VALUE, object.injected.val); } + @Test + public void testInjectNormalWithEmptyKeysInMap() 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 = "{\n" + + " \"map1\" : {\n" + + " \"foo\" : \"bar\",\n" + + " \"\" : \"empty\"\n" + + " },\n" + + " \"map2\" : {\n" + + " \"foo\" : {\n" + + " \"test\" : \"value1\"\n" + + " },\n" + + " \"\" : {\n" + + " \"test\" : \"value2\"\n" + + " }\n" + + " }\n" + + "}"; + final ClassWithMapAndJacksonInject object = new ClassWithMapAndJacksonInject( + ImmutableMap.of("foo", "bar", "", "empty"), + ImmutableMap.of("foo", new ClassWithJacksonInject("value1", injector.getInstance(InjectedParameter.class)), + "", new ClassWithJacksonInject("value2", injector.getInstance(InjectedParameter.class))) + ); + final String jsonWritten = mapper.writerWithDefaultPrettyPrinter().writeValueAsString(object); + Assert.assertEquals(json, jsonWritten); + final ClassWithMapAndJacksonInject objectRead = mapper.readValue(json, ClassWithMapAndJacksonInject.class); + Assert.assertEquals(object, objectRead); + Assert.assertEquals("empty", objectRead.getStringStringMap().get("")); + } + + @Test + public void testInjectNormalWithEmptyKeysAndInjectClass() 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 = "{\n" + + " \"map1\" : {\n" + + " \"foo\" : \"bar\",\n" + + " \"\" : \"empty\"\n" + + " },\n" + + " \"map2\" : {\n" + + " \"foo\" : {\n" + + " \"test\" : \"value1\",\n" + + " \"\" : \"nice try\"\n" + + " },\n" + + " \"\" : {\n" + + " \"test\" : \"value2\",\n" + + " \"\" : \"nice try\"\n" + + " }\n" + + " }\n" + + "}"; + final String expectedSerializedJson + = "{\n" + + " \"map1\" : {\n" + + " \"foo\" : \"bar\",\n" + + " \"\" : \"empty\"\n" + + " },\n" + + " \"map2\" : {\n" + + " \"foo\" : {\n" + + " \"test\" : \"value1\"\n" + + " },\n" + + " \"\" : {\n" + + " \"test\" : \"value2\"\n" + + " }\n" + + " }\n" + + "}"; + final ClassWithMapAndJacksonInject object = new ClassWithMapAndJacksonInject( + ImmutableMap.of("foo", "bar", "", "empty"), + ImmutableMap.of("foo", new ClassWithJacksonInject("value1", injector.getInstance(InjectedParameter.class)), + "", new ClassWithJacksonInject("value2", injector.getInstance(InjectedParameter.class))) + ); + final String jsonWritten = mapper.writerWithDefaultPrettyPrinter().writeValueAsString(object); + Assert.assertEquals(expectedSerializedJson, jsonWritten); + final ClassWithMapAndJacksonInject objectRead = mapper.readValue(json, ClassWithMapAndJacksonInject.class); + Assert.assertEquals(object, objectRead); + Assert.assertEquals("empty", objectRead.getStringStringMap().get("")); + } + private static class ClassWithJacksonInject { private final String test; @@ -106,11 +192,30 @@ public class DruidSecondaryModuleTest this.injected = injected; } - @JsonProperty + @JsonProperty("test") public String getTest() { return test; } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + ClassWithJacksonInject that = (ClassWithJacksonInject) o; + return Objects.equals(test, that.test) && Objects.equals(injected.val, that.injected.val); + } + + @Override + public int hashCode() + { + return Objects.hash(test, injected.val); + } } private static class ClassWithEmptyProperty @@ -135,6 +240,56 @@ public class DruidSecondaryModuleTest return test; } } + + private static class ClassWithMapAndJacksonInject + { + private final Map stringStringMap; + private final Map stringJacksonInjectMap; + + @JsonCreator + public ClassWithMapAndJacksonInject( + @JsonProperty("map1") Map stringStringMap, + @JsonProperty("map2") Map stringJacksonInjectMap + ) + { + this.stringStringMap = stringStringMap; + this.stringJacksonInjectMap = stringJacksonInjectMap; + } + + @JsonProperty("map1") + public Map getStringStringMap() + { + return stringStringMap; + } + + @JsonProperty("map2") + public Map getStringJacksonInjectMap() + { + return stringJacksonInjectMap; + } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + ClassWithMapAndJacksonInject that = (ClassWithMapAndJacksonInject) o; + return Objects.equals(stringStringMap, that.stringStringMap) && Objects.equals( + stringJacksonInjectMap, + that.stringJacksonInjectMap + ); + } + + @Override + public int hashCode() + { + return Objects.hash(stringStringMap, stringJacksonInjectMap); + } + } } public static class ConstructorWithoutJacksonInjectTest diff --git a/processing/src/test/java/org/apache/druid/query/extraction/MapLookupExtractionFnSerDeTest.java b/processing/src/test/java/org/apache/druid/query/extraction/MapLookupExtractionFnSerDeTest.java index 92e5ef77889..39144ff066b 100644 --- a/processing/src/test/java/org/apache/druid/query/extraction/MapLookupExtractionFnSerDeTest.java +++ b/processing/src/test/java/org/apache/druid/query/extraction/MapLookupExtractionFnSerDeTest.java @@ -21,11 +21,9 @@ package org.apache.druid.query.extraction; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableMap; -import com.google.inject.Injector; -import com.google.inject.Key; -import org.apache.druid.guice.GuiceInjectors; -import org.apache.druid.guice.annotations.Json; +import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.segment.TestHelper; import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; @@ -42,19 +40,20 @@ public class MapLookupExtractionFnSerDeTest private static ObjectMapper mapper; private static final Map RENAMES = ImmutableMap.of( "foo", "bar", - "bar", "baz" + "bar", "baz", + "", "empty" ); @BeforeClass public static void setup() { - Injector defaultInjector = GuiceInjectors.makeStartupInjector(); - mapper = defaultInjector.getInstance(Key.get(ObjectMapper.class, Json.class)); + mapper = TestHelper.makeJsonMapper(); } @Test public void testDeserialization() throws IOException { + NullHandling.initializeForTests(); final DimExtractionFn fn = mapper.readerFor(DimExtractionFn.class).readValue( StringUtils.format( "{\"type\":\"lookup\",\"lookup\":{\"type\":\"map\", \"map\":%s}}",