Add explicit delimiters to node configs list (#12444)

* Add explicit delimiter to node configs list

* Remove sorting of flattened map entries

Author: sbabcoc

java/src/org/openqa/selenium/grid/config/Config.java

@@ -23,6 +23,8 @@
 import java.util.Optional;
 import java.util.Set;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
+
 import org.openqa.selenium.json.Json;
 
 public interface Config {
@@ -59,14 +61,21 @@ default <X> X getClass(String section, String option, Class<X> typeOfClass, Stri
     }
   }
 
+  String DELIM_KEY = "\u001E";
+  String DELIMITER = DELIM_KEY + "=\"record-separator\"";
+
   default List<String> toEntryList(Map<String, Object> mapItem) {
-    return mapItem.entrySet().stream()
-        .map(
-            entry -> {
-              return String.format("%s=%s", entry.getKey(), toJson(entry.getValue()));
-            })
-        .sorted()
-        .collect(ImmutableList.toImmutableList());
+    // transform config settings map into list of key/value strings
+    List<String> entryList = mapItem.entrySet().stream()
+      .map(
+        entry -> {
+          return String.format("%s=%s", entry.getKey(), toJson(entry.getValue()));
+        })
+      .collect(Collectors.toList());
+    // add record separator
+    entryList.add(DELIMITER);
+    // return immutable config settings list
+    return ImmutableList.<String>builder().addAll(entryList).build();
   }
 
   default String toJson(Object value) {

java/src/org/openqa/selenium/grid/node/config/NodeOptions.java

@@ -344,143 +344,124 @@ private SessionFactory createSessionFactory(String clazz, Capabilities stereotyp
   private void addDriverConfigs(
       Function<ImmutableCapabilities, Collection<SessionFactory>> factoryFactory,
       ImmutableMultimap.Builder<Capabilities, SessionFactory> sessionFactories) {
-    Multimap<WebDriverInfo, SessionFactory> driverConfigs = HashMultimap.create();
-    config
-        .getAll(NODE_SECTION, "driver-configuration")
-        .ifPresent(
-            drivers -> {
-              /*
-               The four accepted keys are: display-name, max-sessions, stereotype, webdriver-executable.
-               The mandatory keys are display-name and stereotype. When configs are read, they keys always
-               come alphabetically ordered. This means that we know a new config is present when we find
-               the "display-name" key again.
-              */
-
-              if (drivers.size() == 0) {
-                throw new ConfigException("No driver configs were found!");
-              }
 
-              drivers.stream()
-                  .filter(driver -> !driver.contains("="))
-                  .peek(
-                      driver ->
-                          LOG.warning(
-                              driver
-                                  + " does not have the required 'key=value' "
-                                  + "structure for the configuration"))
-                  .findFirst()
-                  .ifPresent(
-                      ignore -> {
-                        throw new ConfigException(
-                            "One or more driver configs does not have the "
-                                + "required 'key=value' structure");
-                      });
-
-              // Find all indexes where "display-name" is present, as it marks the start of a config
-              int[] configIndexes =
-                  IntStream.range(0, drivers.size())
-                      .filter(index -> drivers.get(index).startsWith("display-name"))
-                      .toArray();
-
-              if (configIndexes.length == 0) {
-                throw new ConfigException(
-                    "No 'display-name' keyword was found in the provided configs!");
-              }
-
-              List<Map<String, String>> driversMap = new ArrayList<>();
-              for (int i = 0; i < configIndexes.length; i++) {
-                int fromIndex = configIndexes[i];
-                int toIndex =
-                    (i + 1) >= configIndexes.length ? drivers.size() : configIndexes[i + 1];
-                Map<String, String> configMap = new HashMap<>();
-                drivers
-                    .subList(fromIndex, toIndex)
-                    .forEach(
-                        keyValue -> {
-                          String[] values = keyValue.split("=", 2);
-                          configMap.put(values[0], unquote(values[1]));
-                        });
-                driversMap.add(configMap);
-              }
+    Multimap<WebDriverInfo, SessionFactory> driverConfigs = HashMultimap.create();
 
-              List<DriverService.Builder<?, ?>> builders = new ArrayList<>();
-              ServiceLoader.load(DriverService.Builder.class).forEach(builders::add);
+    // get all driver configuration settings
+    config.getAll(NODE_SECTION, "driver-configuration")
+      // if settings exist
+      .ifPresent(drivers -> {
+        Map<String, String> configMap = new HashMap<>();
+        List<Map<String, String>> configList = new ArrayList<>();
+
+        // iterate over driver settings
+        for (String setting : drivers) {
+          // split this setting into key/value pair
+          String[] values = setting.split("=", 2);
+          // if format is invalid
+          if (values.length != 2) {
+            throw new ConfigException("Driver setting '" + setting
+              + "' does not adhere to the required 'key=value' format!");
+          }
+          // if this is a record separator
+          if (values[0].equals(Config.DELIM_KEY)) {
+            // if config lacks settings
+            if (configMap.isEmpty()) {
+              throw new ConfigException("Found config delimiter with no preceding settings!");
+            }
+
+            // if config lacks 'display-name' setting
+            if (!configMap.containsKey("display-name")) {
+              throw new ConfigException("Found config with no 'display-name' setting! " + configMap);
+            }
+
+            // if config lacks 'stereotype' setting
+            if (!configMap.containsKey("stereotype")) {
+              throw new ConfigException("Found config with no 'stereotype' setting! " + configMap);
+            }
+
+            // add config to list
+            configList.add(configMap);
+            // prepare for next config
+            configMap = new HashMap<>();
+          } else {
+            // add setting to config
+            configMap.put(values[0], unquote(values[1]));
+          }
+        }
 
-              List<WebDriverInfo> infos = new ArrayList<>();
-              ServiceLoader.load(WebDriverInfo.class).forEach(infos::add);
+        // if no configs were found
+        if (configList.isEmpty()) {
+          throw new ConfigException("No driver configs were found!");
+        }
 
-              driversMap.forEach(
-                  configMap -> {
-                    if (!configMap.containsKey("stereotype")) {
-                      throw new ConfigException(
-                          "Driver config is missing stereotype value. " + configMap);
-                    }
+        List<DriverService.Builder<?, ?>> builderList = new ArrayList<>();
+        ServiceLoader.load(DriverService.Builder.class).forEach(builderList::add);
+
+        List<WebDriverInfo> infoList = new ArrayList<>();
+        ServiceLoader.load(WebDriverInfo.class).forEach(infoList::add);
+
+        // iterate over driver configs
+        configList.forEach(thisConfig -> {
+          // create Capabilities object from stereotype of this config
+          Capabilities confStereotype = JSON.toType(thisConfig.get("stereotype"), Capabilities.class);
+
+          // extract driver executable path from this config
+          String webDriverExecutablePath = thisConfig.get("webdriver-executable");
+          // if executable path is specified
+          if (null != webDriverExecutablePath) {
+            // create File object from executable path string
+            File webDriverExecutable = new File(webDriverExecutablePath);
+            // if specified path isn't a file
+            if (!webDriverExecutable.isFile()) {
+              LOG.warning("Driver executable does not seem to be a file! " + webDriverExecutablePath);
+            }
+
+            // if specified path isn't executable
+            if (!webDriverExecutable.canExecute()) {
+              LOG.warning("Driver file exists but does not seem to be a executable! " + webDriverExecutablePath);
+            }
+
+            // add specified driver executable path to capabilities
+            confStereotype = new PersistentCapabilities(confStereotype)
+              .setCapability("se:webDriverExecutable", webDriverExecutablePath);
+          }
+
+          Capabilities stereotype = enhanceStereotype(confStereotype);
+          String configName = thisConfig.getOrDefault("display-name", "Custom Slot Config");
+
+          WebDriverInfo info = infoList.stream()
+            .filter(webDriverInfo -> webDriverInfo.isSupporting(stereotype))
+            .findFirst()
+            .orElseThrow(() ->
+              new ConfigException("Unable to find matching driver for %s", stereotype));
+
+          int driverMaxSessions = Integer.parseInt(thisConfig.getOrDefault(
+            "max-sessions", String.valueOf(info.getMaximumSimultaneousSessions())));
+          Require.positive("Driver max sessions", driverMaxSessions);
+
+          WebDriverInfo driverInfoConfig = createConfiguredDriverInfo(info, stereotype, configName);
+
+          builderList.stream()
+            .filter(builder -> builder.score(stereotype) > 0)
+            .max(Comparator.comparingInt(builder -> builder.score(stereotype)))
+            .ifPresent(builder -> {
+                ImmutableCapabilities immutable = new ImmutableCapabilities(stereotype);
+                int maxDriverSessions = getDriverMaxSessions(info, driverMaxSessions);
+                for (int i = 0; i < maxDriverSessions; i++) {
+                  driverConfigs.putAll(driverInfoConfig, factoryFactory.apply(immutable));
+                }
+              }
+            );
+          }
+        );
+      }
+    );
 
-                    Capabilities confStereotype =
-                        JSON.toType(configMap.get("stereotype"), Capabilities.class);
-                    if (configMap.containsKey("webdriver-executable")) {
-                      String webDriverExecutablePath =
-                          configMap.getOrDefault("webdriver-executable", "");
-                      File webDriverExecutable = new File(webDriverExecutablePath);
-                      if (!webDriverExecutable.isFile()) {
-                        LOG.warning(
-                            "Driver executable does not seem to be a file! "
-                                + webDriverExecutablePath);
-                      }
-                      if (!webDriverExecutable.canExecute()) {
-                        LOG.warning(
-                            "Driver file exists but does not seem to be a executable! "
-                                + webDriverExecutablePath);
-                      }
-                      confStereotype =
-                          new PersistentCapabilities(confStereotype)
-                              .setCapability("se:webDriverExecutable", webDriverExecutablePath);
-                    }
-                    Capabilities stereotype = enhanceStereotype(confStereotype);
-
-                    String configName =
-                        configMap.getOrDefault("display-name", "Custom Slot Config");
-
-                    WebDriverInfo info =
-                        infos.stream()
-                            .filter(webDriverInfo -> webDriverInfo.isSupporting(stereotype))
-                            .findFirst()
-                            .orElseThrow(
-                                () ->
-                                    new ConfigException(
-                                        "Unable to find matching driver for %s", stereotype));
-
-                    int driverMaxSessions =
-                        Integer.parseInt(
-                            configMap.getOrDefault(
-                                "max-sessions",
-                                String.valueOf(info.getMaximumSimultaneousSessions())));
-                    Require.positive("Driver max sessions", driverMaxSessions);
-
-                    WebDriverInfo driverInfoConfig =
-                        createConfiguredDriverInfo(info, stereotype, configName);
-
-                    builders.stream()
-                        .filter(builder -> builder.score(stereotype) > 0)
-                        .max(Comparator.comparingInt(builder -> builder.score(stereotype)))
-                        .ifPresent(
-                            builder -> {
-                              ImmutableCapabilities immutable =
-                                  new ImmutableCapabilities(stereotype);
-                              int maxDriverSessions = getDriverMaxSessions(info, driverMaxSessions);
-                              for (int i = 0; i < maxDriverSessions; i++) {
-                                driverConfigs.putAll(
-                                    driverInfoConfig, factoryFactory.apply(immutable));
-                              }
-                            });
-                  });
-            });
     driverConfigs.asMap().entrySet().stream()
-        .peek(this::report)
-        .forEach(
-            entry ->
-                sessionFactories.putAll(
-                    entry.getKey().getCanonicalCapabilities(), entry.getValue()));
+      .peek(this::report)
+      .forEach(entry ->
+        sessionFactories.putAll(entry.getKey().getCanonicalCapabilities(), entry.getValue()));
   }
 
   private void addDetectedDrivers(

java/test/org/openqa/selenium/grid/config/JsonConfigTest.java

@@ -83,8 +83,8 @@ void shouldContainConfigFromArrayOfTables() {
 
     List<String> expected =
         Arrays.asList(
-            "default=\"brie\"", "name=\"soft cheese\"",
-            "default=\"Emmental\"", "name=\"Medium-hard cheese\"");
+            "name=\"soft cheese\"", "default=\"brie\"", Config.DELIMITER,
+            "name=\"Medium-hard cheese\"", "default=\"Emmental\"", Config.DELIMITER);
     assertThat(config.getAll("cheeses", "type").orElse(Collections.emptyList()))
         .isEqualTo(expected);
     assertThat(config.getAll("cheeses", "type").orElse(Collections.emptyList()).subList(0, 2))
@@ -116,7 +116,8 @@ void ensureCanReadListOfMaps() {
     List<String> expected =
         Arrays.asList(
             "display-name=\"htmlunit\"",
-            "stereotype={\"browserName\": \"htmlunit\",\"browserVersion\": \"chrome\"}");
+            "stereotype={\"browserName\": \"htmlunit\",\"browserVersion\": \"chrome\"}",
+            Config.DELIMITER);
     Optional<List<String>> content = config.getAll("node", "driver-configuration");
     assertThat(content).isEqualTo(Optional.of(expected));
   }

java/test/org/openqa/selenium/grid/config/MapConfigTest.java

@@ -68,8 +68,8 @@ void shouldContainConfigFromArrayOfTables() {
 
     List<String> expected =
         Arrays.asList(
-            "default=\"brie\"", "name=\"soft cheese\"",
-            "default=\"Emmental\"", "name=\"Medium-hard cheese\"");
+            "name=\"soft cheese\"", "default=\"brie\"", Config.DELIMITER,
+            "name=\"Medium-hard cheese\"", "default=\"Emmental\"", Config.DELIMITER);
     assertThat(config.getAll("cheeses", "type").orElse(Collections.emptyList()))
         .isEqualTo(expected);
     assertThat(config.getAll("cheeses", "type").orElse(Collections.emptyList()).subList(0, 2))
@@ -123,7 +123,8 @@ void ensureCanReadListOfMaps() {
     List<String> expected =
         Arrays.asList(
             "display-name=\"htmlunit\"",
-            "stereotype={\"browserName\": \"htmlunit\",\"browserVersion\": \"chrome\"}");
+            "stereotype={\"browserName\": \"htmlunit\",\"browserVersion\": \"chrome\"}",
+            Config.DELIMITER);
     Optional<List<String>> content = config.getAll("node", "driver-configuration");
     assertThat(content).isEqualTo(Optional.of(expected));
   }

java/test/org/openqa/selenium/grid/config/TomlConfigTest.java

@@ -55,8 +55,8 @@ void shouldContainConfigFromArrayOfTables() {
 
     List<String> expected =
         Arrays.asList(
-            "default=\"brie\"", "name=\"soft cheese\"",
-            "default=\"Emmental\"", "name=\"Medium-hard cheese\"");
+            "default=\"brie\"", "name=\"soft cheese\"", Config.DELIMITER,
+            "default=\"Emmental\"", "name=\"Medium-hard cheese\"", Config.DELIMITER);
     assertThat(config.getAll("cheeses", "type").orElse(Collections.emptyList()))
         .isEqualTo(expected);
     assertThat(config.getAll("cheeses", "type").orElse(Collections.emptyList()).subList(0, 2))
@@ -89,7 +89,8 @@ void ensureCanReadListOfMaps() {
     List<String> expected =
         Arrays.asList(
             "display-name=\"htmlunit\"",
-            "stereotype={\"browserVersion\": \"chrome\",\"browserName\": \"htmlunit\"}");
+            "stereotype={\"browserVersion\": \"chrome\",\"browserName\": \"htmlunit\"}",
+            Config.DELIMITER);
     Optional<List<String>> content = config.getAll("node", "driver-configuration");
     assertThat(content).isEqualTo(Optional.of(expected));
   }