Ensure handlers are treated LIFO

We want to be able to override how particular URLs
are handled while configuring the server (especially
the case where we want to handle `/status`) Make sure
that if the map containing the http request
predicates has stable ordering of keys, they are used
in reverse order. Since ImmutableMap and LinkedHashMap
both keep track of keys by insertion order, we're in
a great place. :)

java/server/src/org/openqa/selenium/grid/server/BaseServer.java

@@ -145,21 +145,25 @@ public BaseServer(BaseServerOptions options) {
     http.setIdleTimeout(500000);
 
     server.setConnectors(new Connector[]{http});
-
-    CommandHandler delegate = new CompoundHandler(injector, handlers);
-    W3CCommandHandler handler = new W3CCommandHandler(delegate);
-    addServlet(new CommandHandlerServlet(handler), "/*");
   }
 
   @Override
   public void addServlet(Class<? extends Servlet> servlet, String pathSpec) {
+    if (server.isRunning()) {
+      throw new IllegalStateException("You may not add a servlet to a running server");
+    }
+
     servletContextHandler.addServlet(
         Objects.requireNonNull(servlet),
         Objects.requireNonNull(pathSpec));
   }
 
   @Override
   public void addServlet(Servlet servlet, String pathSpec) {
+    if (server.isRunning()) {
+      throw new IllegalStateException("You may not add a servlet to a running server");
+    }
+
     servletContextHandler.addServlet(
         new ServletHolder(Objects.requireNonNull(servlet)),
         Objects.requireNonNull(pathSpec));
@@ -170,7 +174,7 @@ public void addHandler(
       Predicate<HttpRequest> selector,
       BiFunction<Injector, HttpRequest, CommandHandler> handler) {
     if (server.isRunning()) {
-      throw new RuntimeException("You may not add a handler to a running server");
+      throw new IllegalStateException("You may not add a handler to a running server");
     }
     handlers.put(Objects.requireNonNull(selector), Objects.requireNonNull(handler));
   }
@@ -182,6 +186,10 @@ public boolean isStarted() {
   @Override
   public T start() {
     try {
+      CommandHandler delegate = new CompoundHandler(injector, handlers);
+      W3CCommandHandler handler = new W3CCommandHandler(delegate);
+      addServlet(new CommandHandlerServlet(handler), "/*");
+
       server.start();
 
       PortProber.waitForPortUp(getUrl().getPort(), 10, SECONDS);

java/server/src/org/openqa/selenium/grid/web/CompoundHandler.java

@@ -17,17 +17,34 @@
 
 package org.openqa.selenium.grid.web;
 
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Ordering;
+import com.google.common.collect.Sets;
+
 import org.openqa.selenium.UnsupportedCommandException;
 import org.openqa.selenium.injector.Injector;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
 
 import java.io.IOException;
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Deque;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Objects;
 import java.util.function.BiFunction;
 import java.util.function.Predicate;
 
+/**
+ * Presents a collection of {@link CommandHandler}s as a single unit. Should there be more than
+ * handler that responds to a given {@link HttpRequest}, then the last one returned from the
+ * underlying {@link Map}'s key set will be returned (that is, if the map preserves insertion order
+ * the last inserted handler will be returned). This means that handlers added later take precedence
+ * over handlers added first, allowing them to be overridden.
+ */
 public class CompoundHandler implements Predicate<HttpRequest>, CommandHandler {
 
   private final Injector injector;
@@ -38,8 +55,20 @@ public CompoundHandler(
       Injector injector,
       Map<Predicate<HttpRequest>, BiFunction<Injector, HttpRequest, CommandHandler>> handlers) {
 
-    this.injector = Objects.requireNonNull(injector);
-    this.handlers = Objects.requireNonNull(handlers);
+    this.injector = Objects.requireNonNull(injector, "Injector must be set");
+
+    Objects.requireNonNull(handlers, "Handlers to use must be set");
+
+    // First reverse the key set. In maps without a defined order in the key set, this doesn't mean
+    // much.
+    Deque<Predicate<HttpRequest>> deque = new ArrayDeque<>();
+    handlers.keySet().forEach(deque::addFirst);
+
+    // Now bbuild up the map we want to actually use.
+    ImmutableMap.Builder<Predicate<HttpRequest>, BiFunction<Injector, HttpRequest, CommandHandler>>
+        built = ImmutableMap.builder();
+    deque.forEach(key -> built.put(key, handlers.get(key)));
+    this.handlers = built.build();
   }
 
   @Override

java/server/test/org/openqa/selenium/grid/server/BaseServerTest.java

@@ -18,12 +18,15 @@
 package org.openqa.selenium.grid.server;
 
 import static java.net.HttpURLConnection.HTTP_OK;
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.assertEquals;
+import static org.openqa.selenium.grid.server.Server.get;
 import static org.openqa.selenium.remote.http.HttpMethod.GET;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.net.MediaType;
 
+import org.assertj.core.api.Assertions;
 import org.junit.Test;
 import org.openqa.selenium.grid.config.MapConfig;
 import org.openqa.selenium.remote.http.HttpClient;
@@ -35,14 +38,14 @@
 
 public class BaseServerTest {
 
+  private BaseServerOptions emptyOptions = new BaseServerOptions(new MapConfig(ImmutableMap.of()));
+
   @Test
   public void baseServerStartsAndDoesNothing() throws IOException {
-    Server server = new BaseServer(new BaseServerOptions(new MapConfig(ImmutableMap.of()))).start();
+    Server<?> server = new BaseServer<>(emptyOptions).start();
 
     URL url = server.getUrl();
-
     HttpClient client = HttpClient.Factory.createDefault().createClient(url);
-
     HttpResponse response = client.execute(new HttpRequest(GET, "/status"));
 
     // Although we don't expect the server to be ready, we do expect the request to succeed.
@@ -52,4 +55,47 @@ public void baseServerStartsAndDoesNothing() throws IOException {
     assertEquals(MediaType.JSON_UTF_8, MediaType.parse(response.getHeader("Content-Type")));
   }
 
+  @Test
+  public void shouldAllowAHandlerToBeRegistered() throws IOException {
+    Server<?> server = new BaseServer<>(emptyOptions);
+    server.addHandler(
+        get("/cheese"),
+        (inj, ignored) -> (req, res) -> res.setContent("cheddar".getBytes(UTF_8)));
+
+    server.start();
+    URL url = server.getUrl();
+    HttpClient client = HttpClient.Factory.createDefault().createClient(url);
+    HttpResponse response = client.execute(new HttpRequest(GET, "/cheese"));
+
+    assertEquals("cheddar", response.getContentString());
+  }
+
+  @Test
+  public void ifTwoHandlersRespondToTheSameRequestTheLastOneAddedWillBeUsed() throws IOException {
+    Server<?> server = new BaseServer<>(emptyOptions);
+    server.addHandler(
+        get("/status"),
+        (inj, ignored) -> (req, res) -> res.setContent("one".getBytes(UTF_8)));
+    server.addHandler(
+        get("/status"),
+        (inj, ignored) -> (req, res) -> res.setContent("two".getBytes(UTF_8)));
+
+    server.start();
+    URL url = server.getUrl();
+    HttpClient client = HttpClient.Factory.createDefault().createClient(url);
+    HttpResponse response = client.execute(new HttpRequest(GET, "/status"));
+
+    assertEquals("two", response.getContentString());
+
+  }
+
+  @Test
+  public void addHandlersOnceServerIsStartedIsAnError() {
+    Server<BaseServer> server = new BaseServer<>(emptyOptions);
+    server.start();
+
+    Assertions.assertThatExceptionOfType(IllegalStateException.class).isThrownBy(
+        () -> server.addHandler(get("/foo"), (inj, ignored) -> (req, res) -> {}));
+  }
+
 }

java/server/test/org/openqa/selenium/grid/web/BUCK

@@ -5,7 +5,9 @@ java_test(
     deps = [
         ":utils",
         "//java/client/src/org/openqa/selenium/remote:remote",
+        "//java/server/src/org/openqa/selenium/grid/server:server",
         "//java/server/src/org/openqa/selenium/grid/web:web",
+        "//third_party/java/assertj:assertj",
         "//third_party/java/guava:guava",
         "//third_party/java/junit:junit",
     ],

java/server/test/org/openqa/selenium/grid/web/CompoundHandlerTest.java

@@ -0,0 +1,52 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.openqa.selenium.grid.web;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.openqa.selenium.grid.server.Server.get;
+import static org.openqa.selenium.remote.http.HttpMethod.GET;
+
+import com.google.common.collect.ImmutableMap;
+
+import org.junit.Test;
+import org.openqa.selenium.injector.Injector;
+import org.openqa.selenium.remote.http.HttpRequest;
+import org.openqa.selenium.remote.http.HttpResponse;
+
+import java.io.IOException;
+
+public class CompoundHandlerTest {
+
+  @Test
+  public void ifTwoHandlersRespondToTheSameRequestTheLastOneAddedWillBeUsed() throws IOException {
+    CommandHandler handler = new CompoundHandler(
+        Injector.builder().build(),
+        // Insertion order is preserved with an ImmutableMap
+        ImmutableMap.of(
+            get("/status"), (inj, ignored) -> (req, res) -> res.setContent("one".getBytes(UTF_8)),
+            get("/status"), (inj, ignored) -> (req, res) -> res.setContent("two".getBytes(UTF_8))));
+
+    HttpRequest request = new HttpRequest(GET, "/status");
+    HttpResponse response = new HttpResponse();
+
+    handler.execute(request, response);
+
+    assertThat(response.getContentString()).isEqualTo("two");
+  }
+}