Merge pull request #8721 from vespa-engine/gjoranv/correct-configs-pr…

…oduced

Reimplement how to find all configs produced by a ConfigProducer.

config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java

@@ -474,7 +474,7 @@ public AllocatedHosts allocatedHosts() {
     }
 
     private static Set<ConfigKey<?>> configsProduced(ConfigProducer cp) {
-        Set<ConfigKey<?>> ret = ReflectionUtil.configsProducedByInterface(cp.getClass(), cp.getConfigId());
+        Set<ConfigKey<?>> ret = ReflectionUtil.getAllConfigsProduced(cp.getClass(), cp.getConfigId());
         UserConfigRepo userConfigs = cp.getUserConfigs();
         for (ConfigDefinitionKey userKey : userConfigs.configsProduced()) {
             ret.add(new ConfigKey<>(userKey.getName(), cp.getConfigId(), userKey.getNamespace()));

config-model/src/main/java/com/yahoo/vespa/model/utils/internal/ReflectionUtil.java

@@ -1,43 +1,36 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.utils.internal;
 
+import com.google.common.reflect.TypeToken;
 import com.yahoo.config.ChangesRequiringRestart;
 import com.yahoo.config.ConfigInstance;
 import com.yahoo.vespa.config.ConfigKey;
+import com.yahoo.vespa.model.ConfigProducer;
 
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
-import java.util.LinkedHashSet;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 /**
- * Utility class containing static methods for retrievinig information about the config producer tree.
+ * Utility class containing static methods for retrieving information about the config producer tree.
  *
  * @author Ulf Lilleengen
  * @author bjorncs
- * @since 5.1
+ * @author gjoranv
  */
 public final class ReflectionUtil {
 
     private ReflectionUtil() {
     }
 
-    /**
-     * Returns a set of all the configs produced by a given producer.
-     *
-     * @param iface The config producer or interface to check for producers.
-     * @param configId The config id to use when creating keys.
-     * @return A set of config keys.
-     */
-    public static Set<ConfigKey<?>> configsProducedByInterface(Class<?> iface, String configId) {
-        Set<ConfigKey<?>> ret = new LinkedHashSet<>();
-        if (isConcreteProducer(iface)) {
-            ret.add(createConfigKeyFromInstance(iface.getEnclosingClass(), configId));
-        }
-        for (Class<?> parentIface : iface.getInterfaces()) {
-            ret.addAll(configsProducedByInterface(parentIface, configId));
-        }
-        return ret;
+    public static Set<ConfigKey<?>> getAllConfigsProduced(Class<? extends ConfigProducer> producerClass, String configId) {
+        // TypeToken is @Beta in guava, so consider implementing a simple recursive method instead.
+        TypeToken<? extends ConfigProducer>.TypeSet interfaces = TypeToken.of(producerClass).getTypes().interfaces();
+        return interfaces.rawTypes().stream()
+                .filter(ReflectionUtil::isConcreteProducer)
+                .map(i -> createConfigKeyFromInstance(i.getEnclosingClass(), configId))
+                .collect(Collectors.toSet());
     }
 
     /**
@@ -97,18 +90,26 @@ private static ConfigKey<?> createConfigKeyFromInstance(Class<?> configInstClass
         }
     }
 
-    private static boolean classIsConfigInstanceProducer(Class<?> clazz) {
-        return clazz.getName().equals(ConfigInstance.Producer.class.getName());
-    }
-
     private static boolean isConcreteProducer(Class<?> producerInterface) {
+        if (isRootConfigProducerInterface(producerInterface))  return false;
+
         boolean parentIsConfigInstance = false;
         for (Class<?> ifaceParent : producerInterface.getInterfaces()) {
-            if (classIsConfigInstanceProducer(ifaceParent)) {
+            if (isConfigInstanceProducer(ifaceParent)) {
                 parentIsConfigInstance = true;
             }
         }
-        return (ConfigInstance.Producer.class.isAssignableFrom(producerInterface) && parentIsConfigInstance && !classIsConfigInstanceProducer(producerInterface));
+        return (ConfigInstance.Producer.class.isAssignableFrom(producerInterface)
+                && parentIsConfigInstance
+                && !isConfigInstanceProducer(producerInterface));
+    }
+
+    private static boolean isConfigInstanceProducer(Class<?> clazz) {
+        return clazz.getName().equals(ConfigInstance.Producer.class.getName());
+    }
+
+    private static boolean isRootConfigProducerInterface(Class<?> clazz) {
+        return clazz.getCanonicalName().equals(ConfigProducer.class.getCanonicalName());
     }
 
 }

config-model/src/test/java/com/yahoo/vespa/model/utils/internal/ReflectionUtilTest.java

@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.utils.internal;
 
+import com.yahoo.config.model.producer.AbstractConfigProducer;
 import com.yahoo.test.ArraytypesConfig;
 import com.yahoo.config.ChangesRequiringRestart;
 import com.yahoo.config.ConfigInstance;
@@ -10,6 +11,7 @@
 
 import java.util.Set;
 
+import static com.yahoo.vespa.model.utils.internal.ReflectionUtil.getAllConfigsProduced;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -19,26 +21,37 @@
 /**
  * @author Ulf Lilleengen
  * @author bjorncs
+ * @author gjoranv
  * @since 5.1
  */
 public class ReflectionUtilTest {
 
-    private static interface ComplexInterface extends SimpletypesConfig.Producer, ArraytypesConfig.Producer {
-    }
+    private static class SimpleProducer extends AbstractConfigProducer implements SimpletypesConfig.Producer {
+        SimpleProducer(AbstractConfigProducer parent, String subId) { super(parent, subId); }
 
-    private static class SimpleProducer implements SimpletypesConfig.Producer {
         @Override
-        public void getConfig(SimpletypesConfig.Builder builder) {
-        }
+        public void getConfig(SimpletypesConfig.Builder builder) { }
     }
 
-    private static class ComplexProducer implements ComplexInterface {
+    private interface ProducerInterface extends SimpletypesConfig.Producer, ArraytypesConfig.Producer { }
+
+    private static class InterfaceImplementingProducer extends AbstractConfigProducer implements ProducerInterface {
+        InterfaceImplementingProducer(AbstractConfigProducer parent, String subId) { super(parent, subId); }
+
         @Override
-        public void getConfig(ArraytypesConfig.Builder builder) {
-        }
+        public void getConfig(ArraytypesConfig.Builder builder) { }
         @Override
-        public void getConfig(SimpletypesConfig.Builder builder) {
-        }
+        public void getConfig(SimpletypesConfig.Builder builder) { }
+    }
+
+    private static abstract class MyAbstractProducer extends AbstractConfigProducer implements SimpletypesConfig.Producer {
+        MyAbstractProducer(AbstractConfigProducer parent, String subId) { super(parent, subId); }
+        @Override
+        public void getConfig(SimpletypesConfig.Builder builder) { }
+    }
+
+    private static class ConcreteProducer extends MyAbstractProducer {
+        ConcreteProducer(AbstractConfigProducer parent, String subId) { super(parent, subId); }
     }
 
     private static class RestartConfig extends ConfigInstance {
@@ -56,16 +69,23 @@ private ChangesRequiringRestart getChangesRequiringRestart(RestartConfig newConf
     private static class NonRestartConfig extends ConfigInstance {}
 
     @Test
-    public void requireThatConfigsProducedByInterfaceTakesParentIntoAccount() {
-        Set<ConfigKey<?>> configs = ReflectionUtil.configsProducedByInterface(ComplexProducer.class, "foo");
+    public void getAllConfigsProduced_includes_configs_produced_by_super_class() {
+        Set<ConfigKey<?>> configs = getAllConfigsProduced(ConcreteProducer.class, "foo");
+        assertThat(configs.size(), is(1));
+        assertTrue(configs.contains(new ConfigKey<>(SimpletypesConfig.CONFIG_DEF_NAME, "foo", SimpletypesConfig.CONFIG_DEF_NAMESPACE)));
+    }
+
+    @Test
+    public void getAllConfigsProduced_includes_configs_produced_by_implemented_interface() {
+        Set<ConfigKey<?>> configs = getAllConfigsProduced(InterfaceImplementingProducer.class, "foo");
         assertThat(configs.size(), is(2));
         assertTrue(configs.contains(new ConfigKey<>(SimpletypesConfig.CONFIG_DEF_NAME, "foo", SimpletypesConfig.CONFIG_DEF_NAMESPACE)));
         assertTrue(configs.contains(new ConfigKey<>(ArraytypesConfig.CONFIG_DEF_NAME, "foo", ArraytypesConfig.CONFIG_DEF_NAMESPACE)));
     }
 
     @Test
-    public void requireThatConfigsProducedByInterfaceAreFound() {
-        Set<ConfigKey<?>> configs = ReflectionUtil.configsProducedByInterface(SimpleProducer.class, "foo");
+    public void getAllConfigsProduced_includes_configs_directly_implemented_by_producer() {
+        Set<ConfigKey<?>> configs = getAllConfigsProduced(SimpleProducer.class, "foo");
         assertThat(configs.size(), is(1));
         assertTrue(configs.contains(new ConfigKey<>(SimpletypesConfig.CONFIG_DEF_NAME, "foo", SimpletypesConfig.CONFIG_DEF_NAMESPACE)));
     }