Make PolyBind to fail if property value is not found (fixes #4369) (#4374)

* Make PolyBind to fail if property value is not found

* Fix test

* Add onHeap option in NamespaceExtractionModule

* Add PolyBind.createChoiceWithDefaultNoScope()

* Fix NPE

* Fix

* Configure MetadataStorageProvider option for MySQL, PostgreSQL and SQLServer

* Deprecate PolyBind.createChoiceWithDefault form with unused defaultKey

* Fix NPE

api/src/main/java/io/druid/guice/PolyBind.java

@@ -19,6 +19,7 @@
 
 package io.druid.guice;
 
+import com.google.common.base.Preconditions;
 import com.google.inject.Binder;
 import com.google.inject.Inject;
 import com.google.inject.Injector;
@@ -30,6 +31,7 @@ import com.google.inject.binder.ScopedBindingBuilder;
 import com.google.inject.multibindings.MapBinder;
 import com.google.inject.util.Types;
 
+import javax.annotation.Nullable;
 import java.lang.reflect.ParameterizedType;
 import java.util.Map;
 import java.util.Properties;
@@ -58,23 +60,18 @@ public class PolyBind
       Binder binder,
       String property,
       Key<T> interfaceKey,
-      Key<? extends T> defaultKey
+      @Nullable Key<? extends T> defaultKey
   )
   {
-    return createChoiceWithDefault(binder, property, interfaceKey, defaultKey, null);
+    ConfiggedProvider<T> provider = new ConfiggedProvider<>(interfaceKey, property, defaultKey, null);
+    return binder.bind(interfaceKey).toProvider(provider);
   }
 
   /**
-   * Sets up a "choice" for the injector to resolve at injection time.
+   * @deprecated use {@link #createChoiceWithDefault(com.google.inject.Binder, String, com.google.inject.Key, String)}
-   *
+   * instead. {@code defaultKey} argument is ignored.
-   * @param binder the binder for the injector that is being configured
-   * @param property the property that will be checked to determine the implementation choice
-   * @param interfaceKey the interface that will be injected using this choice
-   * @param defaultKey the default instance to be injected if the property doesn't match a choice.  Can be null
-   * @param defaultPropertyValue the default property value to use if the property is not set.
-   * @param <T> interface type
-   * @return A ScopedBindingBuilder so that scopes can be added to the binding, if required.
    */
+  @Deprecated
   public static <T> ScopedBindingBuilder createChoiceWithDefault(
       Binder binder,
       String property,
@@ -83,7 +80,29 @@ public class PolyBind
       String defaultPropertyValue
   )
   {
-    return binder.bind(interfaceKey).toProvider(new ConfiggedProvider<T>(interfaceKey, property, defaultKey, defaultPropertyValue));
+    return createChoiceWithDefault(binder, property, interfaceKey, defaultPropertyValue);
+  }
+
+  /**
+   * Sets up a "choice" for the injector to resolve at injection time.
+   *
+   * @param binder the binder for the injector that is being configured
+   * @param property the property that will be checked to determine the implementation choice
+   * @param interfaceKey the interface that will be injected using this choice
+   * @param defaultPropertyValue the default property value to use if the property is not set.
+   * @param <T> interface type
+   * @return A ScopedBindingBuilder so that scopes can be added to the binding, if required.
+   */
+  public static <T> ScopedBindingBuilder createChoiceWithDefault(
+      Binder binder,
+      String property,
+      Key<T> interfaceKey,
+      String defaultPropertyValue
+  )
+  {
+    Preconditions.checkNotNull(defaultPropertyValue);
+    ConfiggedProvider<T> provider = new ConfiggedProvider<>(interfaceKey, property, null, defaultPropertyValue);
+    return binder.bind(interfaceKey).toProvider(provider);
   }
 
   /**
@@ -118,7 +137,9 @@ public class PolyBind
   {
     private final Key<T> key;
     private final String property;
+    @Nullable
     private final Key<? extends T> defaultKey;
+    @Nullable
     private final String defaultPropertyValue;
 
     private Injector injector;
@@ -127,8 +148,8 @@ public class PolyBind
     ConfiggedProvider(
         Key<T> key,
         String property,
-        Key<? extends T> defaultKey,
+        @Nullable Key<? extends T> defaultKey,
-        String defaultPropertyValue
+        @Nullable String defaultPropertyValue
     )
     {
       this.key = key;
@@ -165,17 +186,20 @@ public class PolyBind
 
       String implName = props.getProperty(property);
       if (implName == null) {
+        if (defaultPropertyValue == null) {
+          if (defaultKey == null) {
+            throw new ProvisionException(String.format("Some value must be configured for [%s]", key));
+          }
+          return injector.getInstance(defaultKey);
+        }
         implName = defaultPropertyValue;
       }
       final Provider<T> provider = implsMap.get(implName);
 
       if (provider == null) {
-        if (defaultKey == null) {
+        throw new ProvisionException(
-          throw new ProvisionException(
+            String.format("Unknown provider[%s] of %s, known options[%s]", implName, key, implsMap.keySet())
-              String.format("Unknown provider[%s] of %s, known options[%s]", implName, key, implsMap.keySet())
+        );
-          );
-        }
-        return injector.getInstance(defaultKey);
       }
 
       return provider.get();

api/src/test/java/io/druid/guice/PolyBindTest.java

@@ -55,7 +55,7 @@ public class PolyBindTest
                   {
                     binder.bind(Properties.class).toInstance(props);
                     PolyBind.createChoice(binder, "billy", Key.get(Gogo.class), Key.get(GoA.class));
-                    PolyBind.createChoiceWithDefault(binder, "sally", Key.get(GogoSally.class), null, "b");
+                    PolyBind.createChoiceWithDefault(binder, "sally", Key.get(GogoSally.class), "b");
 
                   }
                 }
@@ -107,8 +107,22 @@ public class PolyBindTest
     Assert.assertEquals("B", injector.getInstance(Gogo.class).go());
     Assert.assertEquals("A", injector.getInstance(Key.get(Gogo.class, Names.named("reverse"))).go());
     props.setProperty("billy", "c");
-    Assert.assertEquals("A", injector.getInstance(Gogo.class).go());
+    try {
-    Assert.assertEquals("B", injector.getInstance(Key.get(Gogo.class, Names.named("reverse"))).go());
+      Assert.assertEquals("A", injector.getInstance(Gogo.class).go());
+      Assert.fail(); // should never be reached
+    }
+    catch (Exception e) {
+      Assert.assertTrue(e instanceof ProvisionException);
+      Assert.assertTrue(e.getMessage().contains("Unknown provider[c] of Key[type=io.druid.guice.PolyBindTest$Gogo"));
+    }
+    try {
+      Assert.assertEquals("B", injector.getInstance(Key.get(Gogo.class, Names.named("reverse"))).go());
+      Assert.fail(); // should never be reached
+    }
+    catch (Exception e) {
+      Assert.assertTrue(e instanceof ProvisionException);
+      Assert.assertTrue(e.getMessage().contains("Unknown provider[c] of Key[type=io.druid.guice.PolyBindTest$Gogo"));
+    }
     
     // test default property value
     Assert.assertEquals("B", injector.getInstance(GogoSally.class).go());
@@ -120,7 +134,8 @@ public class PolyBindTest
     try {
       injector.getInstance(GogoSally.class).go();
       Assert.fail(); // should never be reached
-    } catch(Exception e) {
+    }
+    catch (Exception e) {
       Assert.assertTrue(e instanceof ProvisionException);
       Assert.assertTrue(e.getMessage().contains("Unknown provider[c] of Key[type=io.druid.guice.PolyBindTest$GogoSally"));
     }

extensions-contrib/sqlserver-metadata-storage/src/main/java/io/druid/metadata/storage/sqlserver/SQLServerMetadataStorageModule.java

@@ -26,6 +26,8 @@ import io.druid.guice.PolyBind;
 import io.druid.guice.SQLMetadataStorageDruidModule;
 import io.druid.initialization.DruidModule;
 import io.druid.metadata.MetadataStorageConnector;
+import io.druid.metadata.MetadataStorageProvider;
+import io.druid.metadata.NoopMetadataStorageProvider;
 import io.druid.metadata.SQLMetadataConnector;
 
 import java.util.List;
@@ -52,12 +54,20 @@ public class SQLServerMetadataStorageModule extends SQLMetadataStorageDruidModul
   {
     super.configure(binder);
 
-    PolyBind.optionBinder(binder, Key.get(MetadataStorageConnector.class))
+    PolyBind
+        .optionBinder(binder, Key.get(MetadataStorageProvider.class))
+        .addBinding(TYPE)
+        .to(NoopMetadataStorageProvider.class)
+        .in(LazySingleton.class);
+
+    PolyBind
+        .optionBinder(binder, Key.get(MetadataStorageConnector.class))
         .addBinding(TYPE)
         .to(SQLServerConnector.class)
         .in(LazySingleton.class);
 
-    PolyBind.optionBinder(binder, Key.get(SQLMetadataConnector.class))
+    PolyBind
+        .optionBinder(binder, Key.get(SQLMetadataConnector.class))
         .addBinding(TYPE)
         .to(SQLServerConnector.class)
         .in(LazySingleton.class);

extensions-core/lookups-cached-global/src/main/java/io/druid/server/lookup/namespace/NamespaceExtractionModule.java

@@ -77,13 +77,15 @@ public class NamespaceExtractionModule implements DruidModule
   @Override
   public void configure(Binder binder)
   {
-    PolyBind.createChoiceWithDefault(
+    PolyBind
-        binder,
+        .createChoiceWithDefault(binder, TYPE_PREFIX, Key.get(NamespaceExtractionCacheManager.class), "onHeap")
-        TYPE_PREFIX,
+        .in(LazySingleton.class);
-        Key.get(NamespaceExtractionCacheManager.class),
+
-        Key.get(OnHeapNamespaceExtractionCacheManager.class),
+    PolyBind
-        "onHeap"
+        .optionBinder(binder, Key.get(NamespaceExtractionCacheManager.class))
-    ).in(LazySingleton.class);
+        .addBinding("onHeap")
+        .to(OnHeapNamespaceExtractionCacheManager.class)
+        .in(LazySingleton.class);
 
     PolyBind
         .optionBinder(binder, Key.get(NamespaceExtractionCacheManager.class))

extensions-core/mysql-metadata-storage/src/main/java/io/druid/metadata/storage/mysql/MySQLMetadataStorageModule.java

@@ -27,6 +27,8 @@ import io.druid.guice.PolyBind;
 import io.druid.guice.SQLMetadataStorageDruidModule;
 import io.druid.initialization.DruidModule;
 import io.druid.metadata.MetadataStorageConnector;
+import io.druid.metadata.MetadataStorageProvider;
+import io.druid.metadata.NoopMetadataStorageProvider;
 import io.druid.metadata.SQLMetadataConnector;
 
 import java.util.List;
@@ -51,14 +53,22 @@ public class MySQLMetadataStorageModule extends SQLMetadataStorageDruidModule im
   {
     super.configure(binder);
 
-    PolyBind.optionBinder(binder, Key.get(MetadataStorageConnector.class))
+    PolyBind
-            .addBinding(TYPE)
+        .optionBinder(binder, Key.get(MetadataStorageProvider.class))
-            .to(MySQLConnector.class)
+        .addBinding(TYPE)
-            .in(LazySingleton.class);
+        .to(NoopMetadataStorageProvider.class)
+        .in(LazySingleton.class);
 
-    PolyBind.optionBinder(binder, Key.get(SQLMetadataConnector.class))
+    PolyBind
-            .addBinding(TYPE)
+        .optionBinder(binder, Key.get(MetadataStorageConnector.class))
-            .to(MySQLConnector.class)
+        .addBinding(TYPE)
-            .in(LazySingleton.class);
+        .to(MySQLConnector.class)
+        .in(LazySingleton.class);
+
+    PolyBind
+        .optionBinder(binder, Key.get(SQLMetadataConnector.class))
+        .addBinding(TYPE)
+        .to(MySQLConnector.class)
+        .in(LazySingleton.class);
   }
 }

extensions-core/postgresql-metadata-storage/src/main/java/io/druid/metadata/storage/postgresql/PostgreSQLMetadataStorageModule.java

@@ -27,6 +27,8 @@ import io.druid.guice.PolyBind;
 import io.druid.guice.SQLMetadataStorageDruidModule;
 import io.druid.initialization.DruidModule;
 import io.druid.metadata.MetadataStorageConnector;
+import io.druid.metadata.MetadataStorageProvider;
+import io.druid.metadata.NoopMetadataStorageProvider;
 import io.druid.metadata.SQLMetadataConnector;
 
 import java.util.List;
@@ -52,14 +54,22 @@ public class PostgreSQLMetadataStorageModule extends SQLMetadataStorageDruidModu
   {
     super.configure(binder);
 
-    PolyBind.optionBinder(binder, Key.get(MetadataStorageConnector.class))
+    PolyBind
-            .addBinding(TYPE)
+        .optionBinder(binder, Key.get(MetadataStorageProvider.class))
-            .to(PostgreSQLConnector.class)
+        .addBinding(TYPE)
-            .in(LazySingleton.class);
+        .to(NoopMetadataStorageProvider.class)
+        .in(LazySingleton.class);
 
-    PolyBind.optionBinder(binder, Key.get(SQLMetadataConnector.class))
+    PolyBind
-            .addBinding(TYPE)
+        .optionBinder(binder, Key.get(MetadataStorageConnector.class))
-            .to(PostgreSQLConnector.class)
+        .addBinding(TYPE)
-            .in(LazySingleton.class);
+        .to(PostgreSQLConnector.class)
+        .in(LazySingleton.class);
+
+    PolyBind
+        .optionBinder(binder, Key.get(SQLMetadataConnector.class))
+        .addBinding(TYPE)
+        .to(PostgreSQLConnector.class)
+        .in(LazySingleton.class);
   }
 }

server/src/main/java/io/druid/guice/SQLMetadataStorageDruidModule.java

@@ -37,7 +37,6 @@ import io.druid.metadata.MetadataStorageActionHandlerFactory;
 import io.druid.metadata.MetadataStorageConnector;
 import io.druid.metadata.MetadataStorageProvider;
 import io.druid.metadata.MetadataSupervisorManager;
-import io.druid.metadata.NoopMetadataStorageProvider;
 import io.druid.metadata.SQLMetadataConnector;
 import io.druid.metadata.SQLMetadataRuleManager;
 import io.druid.metadata.SQLMetadataRuleManagerProvider;
@@ -65,106 +64,28 @@ public class SQLMetadataStorageDruidModule implements Module
   /**
    * This function only needs to be called by the default SQL metadata storage module
    * Other modules should default to calling super.configure(...) alone
+   *
+   * @param defaultValue default property value
    */
-  public void createBindingChoices(Binder binder, String defaultPropertyValue)
+  public void createBindingChoices(Binder binder, String defaultValue)
   {
-    PolyBind.createChoiceWithDefault(
+    String prop = PROPERTY;
-        binder, PROPERTY, Key.get(MetadataStorageConnector.class), null, defaultPropertyValue
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataStorageConnector.class), defaultValue);
-    );
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataStorageProvider.class), defaultValue);
-    PolyBind.createChoiceWithDefault(
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(SQLMetadataConnector.class), defaultValue);
-        binder,
+
-        PROPERTY,
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataSegmentManager.class), defaultValue);
-        Key.get(MetadataStorageProvider.class),
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataSegmentManagerProvider.class), defaultValue);
-        Key.get(NoopMetadataStorageProvider.class),
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataRuleManager.class), defaultValue);
-        defaultPropertyValue
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataRuleManagerProvider.class), defaultValue);
-    );
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataSegmentPublisher.class), defaultValue);
-    PolyBind.createChoiceWithDefault(
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataSegmentPublisherProvider.class), defaultValue);
-        binder, PROPERTY, Key.get(SQLMetadataConnector.class), null, defaultPropertyValue
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(IndexerMetadataStorageCoordinator.class), defaultValue);
-    );
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataStorageActionHandlerFactory.class), defaultValue);
-    PolyBind.createChoiceWithDefault(
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataStorageUpdaterJobHandler.class), defaultValue);
-        binder,
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(AuditManager.class), defaultValue);
-        PROPERTY,
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(AuditManagerProvider.class), defaultValue);
-        Key.get(MetadataSegmentManager.class),
+    PolyBind.createChoiceWithDefault(binder, prop, Key.get(MetadataSupervisorManager.class), defaultValue);
-        Key.get(SQLMetadataSegmentManager.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(MetadataSegmentManagerProvider.class),
-        Key.get(SQLMetadataSegmentManagerProvider.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(MetadataRuleManager.class),
-        Key.get(SQLMetadataRuleManager.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(MetadataRuleManagerProvider.class),
-        Key.get(SQLMetadataRuleManagerProvider.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(MetadataSegmentPublisher.class),
-        Key.get(SQLMetadataSegmentPublisher.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(MetadataSegmentPublisherProvider.class),
-        Key.get(SQLMetadataSegmentPublisherProvider.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(IndexerMetadataStorageCoordinator.class),
-        Key.get(IndexerSQLMetadataStorageCoordinator.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(MetadataStorageActionHandlerFactory.class),
-        Key.get(SQLMetadataStorageActionHandlerFactory.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(MetadataStorageUpdaterJobHandler.class),
-        Key.get(SQLMetadataStorageUpdaterJobHandler.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(AuditManager.class),
-        Key.get(SQLAuditManager.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(AuditManagerProvider.class),
-        Key.get(SQLAuditManagerProvider.class),
-        defaultPropertyValue
-    );
-    PolyBind.createChoiceWithDefault(
-        binder,
-        PROPERTY,
-        Key.get(MetadataSupervisorManager.class),
-        Key.get(SQLMetadataSupervisorManager.class),
-        defaultPropertyValue
-    );
   }
 
   @Override

services/src/main/java/io/druid/guice/RealtimeModule.java

@@ -58,13 +58,7 @@ public class RealtimeModule implements Module
   @Override
   public void configure(Binder binder)
   {
-    PolyBind.createChoiceWithDefault(
+    PolyBind.createChoiceWithDefault(binder, "druid.publish.type", Key.get(SegmentPublisher.class), "metadata");
-        binder,
-        "druid.publish.type",
-        Key.get(SegmentPublisher.class),
-        null,
-        "metadata"
-    );
     final MapBinder<String, SegmentPublisher> publisherBinder = PolyBind.optionBinder(
         binder,
         Key.get(SegmentPublisher.class)