Route should handle failure to match or no response gracefully

java/client/src/org/openqa/selenium/remote/http/BUCK

@@ -3,10 +3,11 @@ java_library(
     srcs = glob(["*.java"]),
     deps = [
         "//java/client/src/org/openqa/selenium:selenium",
+        "//java/client/src/org/openqa/selenium/json:json",
         "//third_party/java/guava:guava",
     ],
     visibility = [
         "//java/client/src/org/openqa/selenium/devtools:",
         "//java/client/src/org/openqa/selenium/remote/...",
     ],
-)
+)

java/client/src/org/openqa/selenium/remote/http/BUILD.bazel

@@ -17,6 +17,7 @@ java_library(
     ],
     deps = [
         "//java/client/src/org/openqa/selenium:core",
+        "//java/client/src/org/openqa/selenium/json",
         "//third_party/java/guava",
     ],
 )

java/client/src/org/openqa/selenium/remote/http/Route.java

@@ -20,6 +20,7 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import org.openqa.selenium.json.Json;
 
 import java.util.List;
 import java.util.Map;
@@ -40,6 +41,8 @@
 
 public abstract class Route implements HttpHandler, Routable {
 
+  private static final Json JSON = new Json();
+
   public HttpHandler fallbackTo(Supplier<HttpHandler> handler) {
     Objects.requireNonNull(handler, "Handler to use must be set.");
     return req -> {
@@ -50,6 +53,35 @@ public HttpHandler fallbackTo(Supplier<HttpHandler> handler) {
     };
   }
 
+  @Override
+  public final HttpResponse execute(HttpRequest req) {
+    if (!matches(req)) {
+      return new HttpResponse()
+        .setStatus(HTTP_NOT_FOUND)
+        .setContent(utf8String(JSON.toJson(ImmutableMap.of(
+          "value", ImmutableMap.of(
+            "error", "unknown command",
+            "message", "Unable to find handler for " + req,
+            "stacktrace", "")))));
+    }
+
+    HttpResponse res = handle(req);
+
+    if (res != null) {
+      return res;
+    }
+
+    return new HttpResponse()
+      .setStatus(HTTP_INTERNAL_ERROR)
+      .setContent(utf8String(JSON.toJson(ImmutableMap.of(
+        "value", ImmutableMap.of(
+          "error", "unsupported operation",
+          "message", String.format("Found handler for %s, but nothing was returned", req),
+          "stacktrace", "")))));
+  }
+
+  protected abstract HttpResponse handle(HttpRequest req);
+
   public static PredicatedConfig matching(Predicate<HttpRequest> predicate) {
     Objects.requireNonNull(predicate, "Predicate to use must be set.");
     return new PredicatedConfig(predicate);
@@ -141,18 +173,18 @@ public boolean matches(HttpRequest request) {
     }
 
     @Override
-    public HttpResponse execute(HttpRequest request) {
-      UrlTemplate.Match match = template.match(request.getUri());
+    protected HttpResponse handle(HttpRequest req) {
+      UrlTemplate.Match match = template.match(req.getUri());
       HttpHandler handler = handlerFunction.apply(
           match == null ? ImmutableMap.of() : match.getParameters());
 
       if (handler == null) {
         return new HttpResponse()
             .setStatus(HTTP_INTERNAL_ERROR)
-            .setContent(utf8String("Unable to find handler for " + request));
+            .setContent(utf8String("Unable to find handler for " + req));
       }
 
-      return handler.execute(request);
+      return handler.execute(req);
     }
   }
 
@@ -214,8 +246,8 @@ public boolean matches(HttpRequest request) {
     }
 
     @Override
-    public HttpResponse execute(HttpRequest request) {
-      return route.execute(transform(request));
+    protected HttpResponse handle(HttpRequest req) {
+      return route.execute(transform(req));
     }
 
     private HttpRequest transform(HttpRequest request) {
@@ -256,15 +288,15 @@ public boolean matches(HttpRequest request) {
     }
 
     @Override
-    public HttpResponse execute(HttpRequest request) {
+    protected HttpResponse handle(HttpRequest req) {
       return allRoutes.stream()
-          .filter(route -> route.matches(request))
+          .filter(route -> route.matches(req))
           .findFirst()
           .map(route -> (HttpHandler) route)
-          .orElse(req -> new HttpResponse()
+          .orElse(request -> new HttpResponse()
               .setStatus(HTTP_NOT_FOUND)
-              .setContent(utf8String("No handler found for " + req)))
-          .execute(request);
+              .setContent(utf8String("No handler found for " + request)))
+          .execute(req);
     }
   }
 
@@ -297,7 +329,7 @@ public boolean matches(HttpRequest httpRequest) {
     }
 
     @Override
-    public HttpResponse execute(HttpRequest req) {
+    protected HttpResponse handle(HttpRequest req) {
       HttpHandler handler = supplier.get();
       if (handler == null) {
         throw new IllegalStateException("No handler available.");

java/client/test/org/openqa/selenium/remote/http/RouteTest.java

@@ -17,6 +17,7 @@
 
 package org.openqa.selenium.remote.http;
 
+import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR;
 import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
 import static java.net.HttpURLConnection.HTTP_OK;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -29,6 +30,8 @@
 import org.assertj.core.api.Assertions;
 import org.junit.Test;
 
+import java.net.HttpURLConnection;
+
 public class RouteTest {
 
   @Test
@@ -143,4 +146,22 @@ public void shouldUseFallbackIfAnyDeclared() {
     res = handler.execute(new HttpRequest(GET, "/joy"));
     assertThat(res.getStatus()).isEqualTo(HTTP_NOT_FOUND);
   }
+
+  @Test
+  public void shouldReturnA404IfNoRouteMatches() {
+    Route route = Route.get("/hello").to(() -> req -> new HttpResponse());
+
+    HttpResponse response = route.execute(new HttpRequest(GET, "/greeting"));
+
+    assertThat(response.getStatus()).isEqualTo(HTTP_NOT_FOUND);
+  }
+
+  @Test
+  public void shouldReturnA500IfNoResponseIsReturned() {
+    Route route = Route.get("/hello").to(() -> req -> null);
+
+    HttpResponse response = route.execute(new HttpRequest(GET, "/hello"));
+
+    assertThat(response.getStatus()).isEqualTo(HTTP_INTERNAL_ERROR);
+  }
 }