[grid]: Configs should allow multiple values

SeleniumHQ · Mar 19, 2019 · bc8cf14 · bc8cf14
1 parent ce9100d
commit bc8cf14
Show file tree

Hide file tree

Showing 10 changed files with 153 additions and 49 deletions.
diff --git a/java/server/src/org/openqa/selenium/grid/config/AnnotatedConfig.java b/java/server/src/org/openqa/selenium/grid/config/AnnotatedConfig.java
@@ -17,8 +17,7 @@
 
 package org.openqa.selenium.grid.config;
 
-import com.google.common.collect.ImmutableMap;
-import com.google.common.primitives.Primitives;
+import com.google.common.collect.ImmutableList;
 
 import java.lang.reflect.Field;
 import java.util.ArrayDeque;
@@ -27,6 +26,8 @@
 import java.util.Deque;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -44,18 +45,14 @@
  */
 public class AnnotatedConfig implements Config {
 
-  private final Map<String, Map<String, String>> config;
+  private final Map<String, Map<String, List<String>>> config;
 
   public AnnotatedConfig(Object obj) {
-    Map<String, Map<String, String>> values = new HashMap<>();
+    Map<String, Map<String, List<String>>> values = new HashMap<>();
 
     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);
-      }
-
       if (Map.class.isAssignableFrom(field.getType())) {
         throw new ConfigException("Map fields may not be used for configuration: " + field);
       }
@@ -68,32 +65,53 @@ public AnnotatedConfig(Object obj) {
         throw new ConfigException("Unable to read field: " + field);
       }
 
-      if (value == null) {
-        continue;
+      ConfigValue annotation = field.getAnnotation(ConfigValue.class);
+      Map<String, List<String>> section = values.computeIfAbsent(
+          annotation.section(),
+          str -> new HashMap<>());
+      List<String> all = section.computeIfAbsent(annotation.name(), str -> new LinkedList<>());
+
+      if (value instanceof Collection) {
+        for (Object o : ((Collection<?>) value)) {
+          String singleValue = getSingleValue(o);
+          if (singleValue != null) {
+            all.add(singleValue);
+          }
+        }
+      } else {
+        String singleValue = getSingleValue(value);
+        if (singleValue != null) {
+          all.add(singleValue);
+        }
       }
+    }
 
-      if (boolean.class.isAssignableFrom(field.getType()) && !(boolean) value) {
-        continue;
-      }
+    // Now make the config immutable.
+    this.config = values;
+  }
 
-      if (field.getType().isPrimitive()) {
-        Class<?> wrapped = Primitives.wrap(field.getType());
-        if (Number.class.isAssignableFrom(wrapped) && ((Number) value).floatValue() == 0) {
-          continue;
-        }
-      }
+  private String getSingleValue(Object value) {
+    if (value == null) {
+      return null;
+    }
 
-      if (Number.class.isAssignableFrom(field.getType()) && ((Number) value).floatValue() == 0f) {
+    if (value instanceof Map) {
+      throw new ConfigException("Map fields may not be used for configuration: " + value);
+    }
 
-      }
+    if (value instanceof Collection) {
+      throw new ConfigException("Collection fields may not be used for configuration: " + value);
+    }
 
-      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);
+    if (Boolean.FALSE.equals(value)) {
+      return null;
     }
 
-    config = ImmutableMap.copyOf(values);
+    if (value instanceof Number && ((Number) value).floatValue() == 0f) {
+      return null;
+    }
+
+    return String.valueOf(value);
   }
 
   private Deque<Field> findConfigFields(Class<?> clazz) {
@@ -110,7 +128,7 @@ private Deque<Field> findConfigFields(Class<?> clazz) {
 
       Arrays.stream(clazz.getDeclaredFields())
           .filter(field -> field.getAnnotation(ConfigValue.class) != null)
-          .forEach(toSet::addFirst);
+          .forEach(toSet::addLast);
 
       Class<?> toAdd = clazz.getSuperclass();
       if (!Object.class.equals(toAdd) && !seen.contains(toAdd)) {
@@ -125,15 +143,20 @@ private Deque<Field> findConfigFields(Class<?> clazz) {
   }
 
   @Override
-  public Optional<String> get(String section, String option) {
+  public Optional<List<String>> getAll(String section, String option) {
     Objects.requireNonNull(section, "Section name not set");
     Objects.requireNonNull(option, "Option name not set");
 
-    Map<String, String> sec = config.get(section);
-    if (sec == null) {
+    Map<String, List<String>> sec = config.get(section);
+    if (sec == null || sec.isEmpty()) {
+      return Optional.empty();
+    }
+
+    List<String> values = sec.get(option);
+    if (values == null || values.isEmpty()) {
       return Optional.empty();
     }
 
-    return Optional.ofNullable(sec.get(option));
+    return Optional.of(ImmutableList.copyOf(values));
   }
 }
diff --git a/java/server/src/org/openqa/selenium/grid/config/CompoundConfig.java b/java/server/src/org/openqa/selenium/grid/config/CompoundConfig.java
@@ -17,9 +17,13 @@
 
 package org.openqa.selenium.grid.config;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
 import com.google.common.collect.ImmutableList;
 
+import java.util.Collection;
 import java.util.List;
+import java.util.Objects;
 import java.util.Optional;
 
 public class CompoundConfig implements Config {
@@ -34,11 +38,17 @@ public CompoundConfig(Config mostImportant, Config... othersInDescendingOrderOfI
   }
 
   @Override
-  public Optional<String> get(String section, String option) {
-    return allConfigs.stream()
-        .map(config -> config.get(section, option))
+  public Optional<List<String>> getAll(String section, String option) {
+    Objects.requireNonNull(section, "Section name not set");
+    Objects.requireNonNull(option, "Option name not set");
+
+    List<String> values = allConfigs.stream()
+        .map(config -> config.getAll(section, option))
         .filter(Optional::isPresent)
-        .findFirst()
-        .orElse(Optional.empty());
+        .map(Optional::get)
+        .flatMap(Collection::stream)
+        .collect(toImmutableList());
+
+    return values.isEmpty() ? Optional.empty() : Optional.of(values);
   }
 }
diff --git a/java/server/src/org/openqa/selenium/grid/config/ConcatenatingConfig.java b/java/server/src/org/openqa/selenium/grid/config/ConcatenatingConfig.java
@@ -17,9 +17,11 @@
 
 package org.openqa.selenium.grid.config;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 
 import java.util.AbstractMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -44,12 +46,16 @@ public ConcatenatingConfig(String prefix, char separator, Map<?, ?> values) {
   }
 
   @Override
-  public Optional<String> get(String section, String option) {
+  public Optional<List<String>> getAll(String section, String option) {
+    Objects.requireNonNull(section, "Section name not set");
+    Objects.requireNonNull(option, "Option name not set");
+
     String key = prefix + section + separator + option;
 
     return values.entrySet().stream()
         .filter(entry -> key.equalsIgnoreCase(entry.getKey()))
         .map(Map.Entry::getValue)
-        .findFirst();
+        .findFirst()
+        .map(ImmutableList::of);
   }
 }
diff --git a/java/server/src/org/openqa/selenium/grid/config/Config.java b/java/server/src/org/openqa/selenium/grid/config/Config.java
@@ -17,11 +17,16 @@
 
 package org.openqa.selenium.grid.config;
 
+import java.util.List;
 import java.util.Optional;
 
 public interface Config {
 
-  Optional<String> get(String section, String option);
+  Optional<List<String>> getAll(String section, String option);
+
+  default Optional<String> get(String section, String option) {
+    return getAll(section, option).map(items -> items.isEmpty() ? null : items.get(0));
+  }
 
   default Optional<Integer> getInt(String section, String option) {
     return get(section, option).map(Integer::parseInt);

diff --git a/java/server/src/org/openqa/selenium/grid/config/EnvConfig.java b/java/server/src/org/openqa/selenium/grid/config/EnvConfig.java
@@ -17,12 +17,19 @@
 
 package org.openqa.selenium.grid.config;
 
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+import java.util.Objects;
 import java.util.Optional;
 
 public class EnvConfig implements Config {
 
   @Override
-  public Optional<String> get(String section, String option) {
-    return Optional.ofNullable(System.getenv().get(section + "." + option));
+  public Optional<List<String>> getAll(String section, String option) {
+    Objects.requireNonNull(section, "Section name not set");
+    Objects.requireNonNull(option, "Option name not set");
+
+    return Optional.ofNullable(System.getenv().get(section + "." + option)).map(ImmutableList::of);
   }
 }
diff --git a/java/server/src/org/openqa/selenium/grid/config/MapConfig.java b/java/server/src/org/openqa/selenium/grid/config/MapConfig.java
@@ -17,6 +17,9 @@
 
 package org.openqa.selenium.grid.config;
 
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -30,7 +33,7 @@ public MapConfig(Map<String, Object> raw) {
   }
 
   @Override
-  public Optional<String> get(String section, String option) {
+  public Optional<List<String>> getAll(String section, String option) {
     Objects.requireNonNull(section, "Section name not set");
     Objects.requireNonNull(option, "Option name not set");
 
@@ -40,6 +43,6 @@ public Optional<String> get(String section, String option) {
     }
 
     Object value = ((Map<?, ?>) rawSection).get(option);
-    return value == null ? Optional.empty() : Optional.of(String.valueOf(value));
+    return value == null ? Optional.empty() : Optional.of(ImmutableList.of(String.valueOf(value)));
   }
 }
diff --git a/java/server/test/org/openqa/selenium/grid/config/AnnotatedConfigTest.java b/java/server/test/org/openqa/selenium/grid/config/AnnotatedConfigTest.java
@@ -19,12 +19,14 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 
 import org.junit.Test;
 
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
@@ -59,14 +61,20 @@ class WithTypes {
     assertEquals(Optional.of(42), config.getInt("types", "int"));
   }
 
-  @Test(expected = ConfigException.class)
-  public void shouldNotAllowCollectionTypeFieldsToBeAnnotated() {
+  @Test
+  public void shouldAllowCollectionTypeFieldsToBeAnnotated() {
     class WithBadAnnotation {
-      @ConfigValue(section = "bad", name = "collection")
+      @ConfigValue(section = "the", name = "collection")
       private final Set<String> cheeses = ImmutableSet.of("cheddar", "gouda");
     }
 
-    new AnnotatedConfig(new WithBadAnnotation());
+    AnnotatedConfig config = new AnnotatedConfig(new WithBadAnnotation());
+    List<String> values = config.getAll("the", "collection")
+        .orElseThrow(() -> new AssertionError("No value returned"));
+
+    assertEquals(2, values.size());
+    assertTrue(values.contains("cheddar"));
+    assertTrue(values.contains("gouda"));
   }
 
   @Test(expected = ConfigException.class)
@@ -109,7 +117,6 @@ class Child extends Parent {
     Config config = new AnnotatedConfig(new Child());
 
     assertEquals(Optional.of("gorgonzola"), config.get("cheese", "type"));
-
   }
 
   @Test

diff --git a/java/server/test/org/openqa/selenium/grid/config/BUCK b/java/server/test/org/openqa/selenium/grid/config/BUCK
@@ -6,6 +6,7 @@ java_test(
     ],
     deps = [
         "//java/server/src/org/openqa/selenium/grid/config:config",
+        "//third_party/java/beust:jcommander",
         "//third_party/java/guava:guava",
         "//third_party/java/junit:junit",
     ],

diff --git a/java/server/test/org/openqa/selenium/grid/config/ConfigTest.java b/java/server/test/org/openqa/selenium/grid/config/ConfigTest.java
@@ -20,15 +20,21 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 
+import com.beust.jcommander.JCommander;
+import com.beust.jcommander.Parameter;
+
 import org.junit.Test;
 
+import java.util.List;
+import java.util.Optional;
+
 public class ConfigTest {
 
   @Test
   public void ensureFirstConfigValueIsChosen() {
-
     Config config = new CompoundConfig(
         new MapConfig(ImmutableMap.of("section", ImmutableMap.of("option", "foo"))),
         new MapConfig(ImmutableMap.of("section", ImmutableMap.of("option", "bar"))));
@@ -51,4 +57,39 @@ public void shouldReadSystemProperties() {
 
     assertEquals(System.getProperty("user.home"), config.get("user", "home").get());
   }
+
+  @Test
+  public void shouldReturnAllMatchingOptions() {
+    Config config = new CompoundConfig(
+        new MapConfig(ImmutableMap.of("section", ImmutableMap.of("option", "foo"))),
+        new MapConfig(ImmutableMap.of("section", ImmutableMap.of("cake", "fish"))),
+        new MapConfig(ImmutableMap.of("section", ImmutableMap.of("option", "bar"))));
+
+    assertEquals(Optional.empty(), config.getAll("cheese", "brie"));
+    assertEquals(Optional.of(ImmutableList.of("fish")), config.getAll("section", "cake"));
+    assertEquals(Optional.of(ImmutableList.of("foo", "bar")), config.getAll("section", "option"));
+  }
+
+  @Test
+  public void shouldAllowMultipleValues() {
+    class Settable {
+      @Parameter(
+          names = {"-D"},
+          variableArity = true)
+      @ConfigValue(section = "food", name = "kinds")
+      public List<String> field;
+    }
+
+    Settable settable = new Settable();
+
+    JCommander commander = JCommander.newBuilder()
+        .addObject(settable)
+        .build();
+
+    commander.parse("-D", "peas", "-D", "cheese", "-D", "sausages", "--boo");
+
+    Config config = new AnnotatedConfig(settable);
+
+    assertEquals(Optional.of(settable.field), config.getAll("food", "kinds"));
+  }
 }