Walk class hierarchy when setting annotated config values

java/server/src/org/openqa/selenium/grid/config/AnnotatedConfig.java

@@ -20,17 +20,22 @@
 import com.google.common.collect.ImmutableMap;
 
 import java.lang.reflect.Field;
+import java.util.ArrayDeque;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.Deque;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 
 /**
  * A form of {@link Config} that is generated by looking at fields in the constructor arg that are
- * annotated with {@link ConfigValue}. The class hierarchy is not walked, null values are ignored,
- * and the order in which fields are read is not stable (meaning duplicate config values may give
- * different values each time).
+ * annotated with {@link ConfigValue}. The class hierarchy is walked from closest to Object to the
+ * constructor argument's type, null values are ignored, and the order in which fields are read is
+ * not stable (meaning duplicate config values may give different values each time).
  * <p>
  * The main use of this class is to allow an object configured using (for example) jcommander to be
  * used directly within the app, without requiring intermediate support classes to convert flags to
@@ -43,13 +48,9 @@ public class AnnotatedConfig implements Config {
   public AnnotatedConfig(Object obj) {
     Map<String, Map<String, String>> values = new HashMap<>();
 
-    for (Field field : obj.getClass().getDeclaredFields()) {
-      ConfigValue annotation = field.getAnnotation(ConfigValue.class);
-
-      if (annotation == null) {
-        continue;
-      }
+    Deque<Field> allConfigValues = findConfigFields(obj.getClass());
 
+    for (Field field : allConfigValues) {
       if (Collection.class.isAssignableFrom(field.getType())) {
         throw new ConfigException("Collection fields may not be used for configuration: " + field);
       }
@@ -70,6 +71,7 @@ public AnnotatedConfig(Object obj) {
         continue;
       }
 
+      ConfigValue annotation = field.getAnnotation(ConfigValue.class);
       Map<String, String> section = values.getOrDefault(annotation.section(), new HashMap<>());
       section.put(annotation.name(), String.valueOf(value));
       values.put(annotation.section(), section);
@@ -78,6 +80,34 @@ public AnnotatedConfig(Object obj) {
     config = ImmutableMap.copyOf(values);
   }
 
+  private Deque<Field> findConfigFields(Class<?> clazz) {
+    Deque<Field> toSet = new ArrayDeque<>();
+    Set<Class<?>> toVisit = new HashSet<>();
+    toVisit.add(clazz);
+
+    Set<Class<?>> seen = new HashSet<>();
+
+    while (!toVisit.isEmpty()) {
+      clazz = toVisit.iterator().next();
+      toVisit.remove(clazz);
+      seen.add(clazz);
+
+      Arrays.stream(clazz.getDeclaredFields())
+          .filter(field -> field.getAnnotation(ConfigValue.class) != null)
+          .forEach(toSet::addFirst);
+
+      Class<?> toAdd = clazz.getSuperclass();
+      if (!Object.class.equals(toAdd) && !seen.contains(toAdd)) {
+        toVisit.add(toAdd);
+      }
+      Arrays.stream(clazz.getInterfaces())
+          .filter(face -> !seen.contains(face))
+          .forEach(toVisit::add);
+    }
+
+    return toSet;
+  }
+
   @Override
   public Optional<String> get(String section, String option) {
     Objects.requireNonNull(section, "Section name not set");

java/server/test/org/openqa/selenium/grid/config/AnnotatedConfigTest.java

@@ -78,4 +78,37 @@ class WithBadAnnotation {
     new AnnotatedConfig(new WithBadAnnotation());
   }
 
+  @Test
+  public void shouldWalkInheritanceHierarchy() {
+    class Parent {
+      @ConfigValue(section = "cheese", name = "type")
+      private final String value = "cheddar";
+    }
+
+    class Child extends Parent {
+    }
+
+    Config config = new AnnotatedConfig(new Child());
+
+    assertEquals(Optional.of("cheddar"), config.get("cheese", "type"));
+  }
+
+  @Test
+  public void configValuesFromChildClassesAreMoreImportant() {
+    class Parent {
+      @ConfigValue(section = "cheese", name = "type")
+      private final String value = "cheddar";
+    }
+
+    class Child extends Parent {
+      @ConfigValue(section = "cheese", name = "type")
+      private final String cheese = "gorgonzola";
+    }
+
+    Config config = new AnnotatedConfig(new Child());
+
+    assertEquals(Optional.of("gorgonzola"), config.get("cheese", "type"));
+
+  }
+
 }