[grid]: Propagate errors correctly, but only as w3c encoded/decoded

This means that the Grid is now demoable, but there are still a number of areas that need improvement. Notably, we should be able to encode errors for local ends that are only speaking the JSON Wire Protocol, rather than the w3c dialect.
SeleniumHQ · Feb 18, 2019 · fe4451e · fe4451e
1 parent a0214fd
commit fe4451e
Show file tree

Hide file tree

Showing 13 changed files with 334 additions and 55 deletions.
diff --git a/java/client/src/org/openqa/selenium/json/StaticInitializerCoercer.java b/java/client/src/org/openqa/selenium/json/StaticInitializerCoercer.java
@@ -43,7 +43,14 @@ public BiFunction<JsonInput, PropertySetting, Object> apply(Type type) {
     fromJson.setAccessible(true);
 
     return (jsonInput, setting) -> {
-      Object obj = jsonInput.read(fromJson.getGenericParameterTypes()[0]);
+      Type argType = fromJson.getGenericParameterTypes()[0];
+      Object obj;
+      if (JsonInput.class.equals(argType)) {
+        obj = jsonInput;
+      } else {
+        obj = jsonInput.read(argType);
+      }
+
       if (obj == null) {
         throw new JsonException("Unable to read value to convert for " + type);
       }

diff --git a/java/client/src/org/openqa/selenium/remote/ProtocolHandshake.java b/java/client/src/org/openqa/selenium/remote/ProtocolHandshake.java
@@ -121,8 +121,8 @@ private Optional<Result> createSession(HttpClient client, InputStream newSession
         blob);
 
     return Stream.of(
-        new JsonWireProtocolResponse().getResponseFunction(),
-        new W3CHandshakeResponse().getResponseFunction())
+        new W3CHandshakeResponse().getResponseFunction(),
+        new JsonWireProtocolResponse().getResponseFunction())
         .map(func -> func.apply(initialResponse))
         .filter(Objects::nonNull)
         .findFirst();

diff --git a/java/client/test/org/openqa/selenium/json/JsonInputTest.java b/java/client/test/org/openqa/selenium/json/JsonInputTest.java
@@ -31,6 +31,8 @@
 import static org.openqa.selenium.json.JsonType.STRING;
 import static org.openqa.selenium.json.PropertySetting.BY_NAME;
 
+import com.google.common.collect.ImmutableMap;
+
 import org.junit.Test;
 
 import java.io.StringReader;
@@ -204,8 +206,41 @@ public void shouldDecodeUnicodeEscapesProperly() {
     }
   }
 
+  @Test
+  public void shouldCallFromJsonWithJsonInputParameter() {
+    String raw = "{\"message\": \"Cheese!\"}";
+
+    try (JsonInput in = new JsonInput(new StringReader(raw), new JsonTypeCoercer(), BY_NAME)) {
+      HasFromJsonWithJsonInputParameter obj = in.read(HasFromJsonWithJsonInputParameter.class);
+
+      assertThat(obj.getMessage()).isEqualTo("Cheese!");
+    }
+  }
+
   private JsonInput newInput(String raw) {
     StringReader reader = new StringReader(raw);
     return new JsonInput(reader, new JsonTypeCoercer(), BY_NAME);
   }
+
+  public static class HasFromJsonWithJsonInputParameter {
+
+    private final String message;
+
+    public HasFromJsonWithJsonInputParameter(String message) {
+      this.message = message;
+    }
+
+    public String getMessage() {
+      return message;
+    }
+
+    private static HasFromJsonWithJsonInputParameter fromJson(JsonInput input) {
+      input.beginObject();
+      input.nextName();
+      String message = input.nextString();
+      input.endObject();
+
+      return new HasFromJsonWithJsonInputParameter(message);
+    }
+  }
 }
diff --git a/java/server/src/org/openqa/selenium/grid/data/DistributorStatus.java b/java/server/src/org/openqa/selenium/grid/data/DistributorStatus.java
@@ -17,6 +17,8 @@
 
 package org.openqa.selenium.grid.data;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.reflect.TypeToken;
@@ -35,7 +37,8 @@
 
 public class DistributorStatus {
 
-  private static final Type SUMMARIES_TYPES = new TypeToken<Set<NodeSummary>>(){}.getType();
+  private static final Type SUMMARIES_TYPES = new TypeToken<Set<NodeSummary>>() {
+  }.getType();
 
   private final Set<NodeSummary> allNodes;
 
@@ -79,8 +82,6 @@ private static DistributorStatus fromJson(JsonInput input) {
   }
 
   public static class NodeSummary {
-    private static final Type STEREOTYPES_TYPE =
-        new TypeToken<Map<Capabilities, Integer>>() {}.getType();
 
     private final UUID nodeId;
     private final URI uri;
@@ -141,6 +142,26 @@ public boolean hasCapacity() {
           .orElse(false);
     }
 
+    private Map<String, Object> toJson() {
+      ImmutableMap.Builder<String, Object> builder = ImmutableMap.builder();
+      builder.put("nodeId", getNodeId());
+      builder.put("uri", getUri());
+      builder.put("up", isUp());
+      builder.put("maxSessionCount", getMaxSessionCount());
+      builder.put("stereotypes", getStereotypes().entrySet().stream()
+          .map(entry -> ImmutableMap.of(
+              "capabilities", entry.getKey(),
+              "count", entry.getValue()))
+          .collect(toImmutableList()));
+      builder.put("usedStereotypes", getUsedStereotypes().entrySet().stream()
+          .map(entry -> ImmutableMap.of(
+              "capabilities", entry.getKey(),
+              "count", entry.getValue()))
+          .collect(toImmutableList()));
+
+      return builder.build();
+    }
+
     private static NodeSummary fromJson(JsonInput input) {
       UUID nodeId = null;
       URI uri = null;
@@ -161,7 +182,7 @@ private static NodeSummary fromJson(JsonInput input) {
             break;
 
           case "stereotypes":
-            stereotypes = input.read(STEREOTYPES_TYPE);
+            stereotypes = readCapabilityCounts(input);
             break;
 
           case "up":
@@ -173,17 +194,50 @@ private static NodeSummary fromJson(JsonInput input) {
             break;
 
           case "usedStereotypes":
-            used = input.read(STEREOTYPES_TYPE);
+            used = readCapabilityCounts(input);
             break;
 
           default:
             input.skipValue();
+            break;
         }
       }
 
       input.endObject();
 
       return new NodeSummary(nodeId, uri, up, maxSessionCount, stereotypes, used);
     }
+
+    private static Map<Capabilities, Integer> readCapabilityCounts(JsonInput input) {
+      Map<Capabilities, Integer> toReturn = new HashMap<>();
+
+      input.beginArray();
+      while (input.hasNext()) {
+        Capabilities caps = null;
+        int count = 0;
+        input.beginObject();
+        while (input.hasNext()) {
+          switch (input.nextName()) {
+            case "capabilities":
+              caps = input.read(Capabilities.class);
+              break;
+
+            case "count":
+              count = input.nextNumber().intValue();
+              break;
+
+            default:
+              input.skipValue();
+              break;
+          }
+        }
+        input.endObject();
+
+        toReturn.put(caps, count);
+      }
+      input.endArray();
+
+      return toReturn;
+    }
   }
 }
diff --git a/java/server/src/org/openqa/selenium/grid/distributor/httpd/DistributorServer.java b/java/server/src/org/openqa/selenium/grid/distributor/httpd/DistributorServer.java
@@ -40,7 +40,9 @@
 import org.openqa.selenium.grid.server.HelpFlags;
 import org.openqa.selenium.grid.server.Server;
 import org.openqa.selenium.grid.server.W3CCommandHandler;
+import org.openqa.selenium.grid.sessionmap.SessionMap;
 import org.openqa.selenium.grid.sessionmap.config.SessionMapFlags;
+import org.openqa.selenium.grid.sessionmap.config.SessionMapOptions;
 import org.openqa.selenium.grid.web.Routes;
 import org.openqa.selenium.remote.http.HttpClient;
 import org.openqa.selenium.remote.tracing.DistributedTracer;
@@ -107,11 +109,15 @@ public Executable configure(String... args) {
       EventBusConfig events = new EventBusConfig(config);
       EventBus bus = events.getEventBus();
 
+      HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
+
+      SessionMap sessions = new SessionMapOptions(config).getSessionMap(clientFactory);
+
       Distributor distributor = new LocalDistributor(
           tracer,
           bus,
-          HttpClient.Factory.createDefault(),
-          null);
+          clientFactory,
+          sessions);
 
       BaseServerOptions serverOptions = new BaseServerOptions(config);
 

diff --git a/java/server/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/server/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java
@@ -18,6 +18,7 @@
 package org.openqa.selenium.grid.distributor.local;
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static java.util.stream.Collectors.toList;
 import static org.openqa.selenium.grid.distributor.local.Host.Status.UP;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -57,6 +58,7 @@
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.function.Supplier;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
 public class LocalDistributor extends Distributor {
 
@@ -112,7 +114,8 @@ public Session newSession(NewSessionPayload payload) throws SessionNotCreatedExc
 
     Session session = selected
         .orElseThrow(
-            () -> new SessionNotCreatedException("Unable to find provider for session: " + allCaps))
+            () -> new SessionNotCreatedException(
+                "Unable to find provider for session: " + payload.stream().collect(toList())))
         .get();
 
     sessions.add(session);

diff --git a/java/server/src/org/openqa/selenium/grid/router/GridStatusHandler.java b/java/server/src/org/openqa/selenium/grid/router/GridStatusHandler.java
@@ -80,7 +80,9 @@ public GridStatusHandler(Json json, HttpClient.Factory clientFactory, Distributo
 
   @Override
   public void execute(HttpRequest req, HttpResponse resp) throws IOException {
-    DistributorStatus status = null;
+    long start = System.currentTimeMillis();
+
+    DistributorStatus status;
     try {
       status = EXECUTOR_SERVICE.submit(distributor::getStatus).get(2, SECONDS);
     } catch (ExecutionException | InterruptedException | TimeoutException e) {
@@ -91,7 +93,6 @@ public void execute(HttpRequest req, HttpResponse resp) throws IOException {
       return;
     }
 
-    long start = System.currentTimeMillis();
     boolean ready = status.hasCapacity();
     String message = ready ? "Selenium Grid ready." : "Selenium Grid not ready";
 

diff --git a/java/server/src/org/openqa/selenium/grid/server/W3CCommandHandler.java b/java/server/src/org/openqa/selenium/grid/server/W3CCommandHandler.java
@@ -20,11 +20,9 @@
 import static com.google.common.net.MediaType.JSON_UTF_8;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
-import com.google.common.collect.ImmutableMap;
-
 import org.openqa.selenium.grid.web.CommandHandler;
+import org.openqa.selenium.grid.web.ErrorCodec;
 import org.openqa.selenium.json.Json;
-import org.openqa.selenium.remote.ErrorCodes;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
 
@@ -34,7 +32,7 @@
 public class W3CCommandHandler implements CommandHandler {
 
   public static final Json JSON = new Json();
-  private final ErrorCodes errors = new ErrorCodes();
+  private final ErrorCodec errors = ErrorCodec.createDefault();
   private final CommandHandler delegate;
 
   public W3CCommandHandler(CommandHandler delegate) {
@@ -50,16 +48,12 @@ public void execute(HttpRequest req, HttpResponse resp) throws IOException {
     try {
       delegate.execute(req, resp);
     } catch (Throwable cause) {
-      // Fair enough. Attempt to convert the exception to something useful.
       resp.setStatus(errors.getHttpStatusCode(cause));
 
       resp.setHeader("Content-Type", JSON_UTF_8.toString());
       resp.setHeader("Cache-Control", "none");
 
-      resp.setContent(JSON.toJson(
-          ImmutableMap.of(
-              "status", errors.toStatusCode(cause),
-              "value", cause)).getBytes(UTF_8));
+      resp.setContent(JSON.toJson(errors.encode(cause)).getBytes(UTF_8));
     }
   }
 }