From 042f0ce02f3cdc74705cf2b51a77c5bb454b7d86 Mon Sep 17 00:00:00 2001 From: Nishant Date: Tue, 2 Jan 2018 00:16:59 +0530 Subject: [PATCH 1/2] Fix broken KafkaEmitterConfig parsing This was a regression introduced in https://github.com/druid-io/druid/pull/4722 KafkaEmitterConfig property names have dot(.) in the name of properties and JsonConfigurator behavior was changed to not support that. Added a test and fixed parsing of properties that have dot(.) in property names --- .../java/io/druid/guice/JsonConfigurator.java | 3 +- .../io/druid/guice/JsonConfiguratorTest.java | 34 ++++++++++++++++--- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/api/src/main/java/io/druid/guice/JsonConfigurator.java b/api/src/main/java/io/druid/guice/JsonConfigurator.java index f6d766dd9551..991cf50a84d4 100644 --- a/api/src/main/java/io/druid/guice/JsonConfigurator.java +++ b/api/src/main/java/io/druid/guice/JsonConfigurator.java @@ -93,7 +93,8 @@ public T configurate(Properties props, String propertyPrefix, Class clazz log.info(e, "Unable to parse [%s]=[%s] as a json object, using as is.", prop, propValue); value = propValue; } - + // Put exact property name for extensions that have dot(.) in property names + jsonMap.put(prop.substring(propertyBase.length()), value); hieraricalPutValue(propertyPrefix, prop, prop.substring(propertyBase.length()), value, jsonMap); } } diff --git a/api/src/test/java/io/druid/guice/JsonConfiguratorTest.java b/api/src/test/java/io/druid/guice/JsonConfiguratorTest.java index 0ce4f77a79aa..8bdb4ef5dc08 100644 --- a/api/src/test/java/io/druid/guice/JsonConfiguratorTest.java +++ b/api/src/test/java/io/druid/guice/JsonConfiguratorTest.java @@ -94,10 +94,13 @@ public ExecutableValidator forExecutables() public void testTest() { Assert.assertEquals( - new MappableObject("p1", ImmutableList.of("p2")), - new MappableObject("p1", ImmutableList.of("p2")) + new MappableObject("p1", ImmutableList.of("p2"), "p2"), + new MappableObject("p1", ImmutableList.of("p2"), "p2") + ); + Assert.assertEquals( + new MappableObject("p1", null, null), + new MappableObject("p1", ImmutableList.of(), null) ); - Assert.assertEquals(new MappableObject("p1", null), new MappableObject("p1", ImmutableList.of())); } @Test @@ -140,6 +143,19 @@ public void testQuotedConfig() Assert.assertEquals("testing \"prop1\"", obj.prop1); Assert.assertEquals(ImmutableList.of(), obj.prop1List); } + + @Test + public void testPropertyWithDot() + { + final JsonConfigurator configurator = new JsonConfigurator(mapper, validator); + properties.setProperty(PROP_PREFIX + "prop2.prop", "testing"); + properties.setProperty(PROP_PREFIX + "prop1", "prop1"); + final MappableObject obj = configurator.configurate(properties, PROP_PREFIX, MappableObject.class); + Assert.assertEquals("testing", obj.prop2); + Assert.assertEquals(ImmutableList.of(), obj.prop1List); + Assert.assertEquals("prop1", obj.prop1); + + } } class MappableObject @@ -148,15 +164,19 @@ class MappableObject final String prop1; @JsonProperty("prop1List") final List prop1List; + @JsonProperty("prop2.prop") + final String prop2; @JsonCreator protected MappableObject( @JsonProperty("prop1") final String prop1, - @JsonProperty("prop1List") final List prop1List + @JsonProperty("prop1List") final List prop1List, + @JsonProperty("prop2.prop") final String prop2 ) { this.prop1 = prop1; this.prop1List = prop1List == null ? ImmutableList.of() : prop1List; + this.prop2 = prop2; } @@ -172,6 +192,12 @@ public String getProp1() return prop1; } + @JsonProperty + public String getProp2() + { + return prop2; + } + @Override public boolean equals(Object o) { From d73db21d230fe89be9d283eab3fc96db9d8b15f5 Mon Sep 17 00:00:00 2001 From: Nishant Date: Wed, 3 Jan 2018 21:10:45 +0530 Subject: [PATCH 2/2] Fix test failure --- api/src/main/java/io/druid/guice/JsonConfigurator.java | 7 ++++--- api/src/test/java/io/druid/guice/JsonConfiguratorTest.java | 6 +++--- .../java/io/druid/server/emitter/EmitterModuleTest.java | 2 ++ 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/api/src/main/java/io/druid/guice/JsonConfigurator.java b/api/src/main/java/io/druid/guice/JsonConfigurator.java index 991cf50a84d4..09721a932d59 100644 --- a/api/src/main/java/io/druid/guice/JsonConfigurator.java +++ b/api/src/main/java/io/druid/guice/JsonConfigurator.java @@ -93,8 +93,6 @@ public T configurate(Properties props, String propertyPrefix, Class clazz log.info(e, "Unable to parse [%s]=[%s] as a json object, using as is.", prop, propValue); value = propValue; } - // Put exact property name for extensions that have dot(.) in property names - jsonMap.put(prop.substring(propertyBase.length()), value); hieraricalPutValue(propertyPrefix, prop, prop.substring(propertyBase.length()), value, jsonMap); } } @@ -176,8 +174,11 @@ private static void hieraricalPutValue( ) { int dotIndex = property.indexOf('.'); + // Always put property with name even if it is of form a.b. This will make sure the property is available for classes + // where JsonProperty names are of the form a.b + // Note:- this will cause more than required properties to be present in the jsonMap. + targetMap.put(property, value); if (dotIndex < 0) { - targetMap.put(property, value); return; } if (dotIndex == 0) { diff --git a/api/src/test/java/io/druid/guice/JsonConfiguratorTest.java b/api/src/test/java/io/druid/guice/JsonConfiguratorTest.java index 8bdb4ef5dc08..acfadf57131f 100644 --- a/api/src/test/java/io/druid/guice/JsonConfiguratorTest.java +++ b/api/src/test/java/io/druid/guice/JsonConfiguratorTest.java @@ -148,7 +148,7 @@ public void testQuotedConfig() public void testPropertyWithDot() { final JsonConfigurator configurator = new JsonConfigurator(mapper, validator); - properties.setProperty(PROP_PREFIX + "prop2.prop", "testing"); + properties.setProperty(PROP_PREFIX + "prop2.prop.2", "testing"); properties.setProperty(PROP_PREFIX + "prop1", "prop1"); final MappableObject obj = configurator.configurate(properties, PROP_PREFIX, MappableObject.class); Assert.assertEquals("testing", obj.prop2); @@ -164,14 +164,14 @@ class MappableObject final String prop1; @JsonProperty("prop1List") final List prop1List; - @JsonProperty("prop2.prop") + @JsonProperty("prop2.prop.2") final String prop2; @JsonCreator protected MappableObject( @JsonProperty("prop1") final String prop1, @JsonProperty("prop1List") final List prop1List, - @JsonProperty("prop2.prop") final String prop2 + @JsonProperty("prop2.prop.2") final String prop2 ) { this.prop1 = prop1; diff --git a/server/src/test/java/io/druid/server/emitter/EmitterModuleTest.java b/server/src/test/java/io/druid/server/emitter/EmitterModuleTest.java index 52a6876a5b62..0cb94f6d447f 100644 --- a/server/src/test/java/io/druid/server/emitter/EmitterModuleTest.java +++ b/server/src/test/java/io/druid/server/emitter/EmitterModuleTest.java @@ -31,6 +31,7 @@ import io.druid.guice.LazySingleton; import io.druid.guice.LifecycleModule; import io.druid.guice.ServerModule; +import io.druid.jackson.JacksonModule; import org.junit.Assert; import org.junit.Test; @@ -67,6 +68,7 @@ private Injector makeInjectorWithProperties(final Properties props) new DruidGuiceExtensions(), new LifecycleModule(), new ServerModule(), + new JacksonModule(), new Module() { @Override