Remove Favicon Special Path in RestController (#61460) (#61487)

It's unnecessary (and adds one string comparison to every request) to special
case the favicon so I added it as a normal REST handler to simplify the code.

server/src/main/java/org/elasticsearch/rest/RestController.java

@@ -28,6 +28,7 @@ import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.breaker.CircuitBreaker;
 import org.elasticsearch.common.bytes.BytesArray;
+import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.io.stream.BytesStreamOutput;
 import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.path.PathTrie;
@@ -42,7 +43,6 @@ import org.elasticsearch.usage.UsageService;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -66,6 +66,18 @@ public class RestController implements HttpServerTransport.Dispatcher {
     private static final Logger logger = LogManager.getLogger(RestController.class);
     private static final DeprecationLogger deprecationLogger = new DeprecationLogger(logger);
 
+    private static final BytesReference FAVICON_RESPONSE;
+
+    static {
+        try (InputStream stream = RestController.class.getResourceAsStream("/config/favicon.ico")) {
+            ByteArrayOutputStream out = new ByteArrayOutputStream();
+            Streams.copy(stream, out);
+            FAVICON_RESPONSE = new BytesArray(out.toByteArray());
+        } catch (IOException e) {
+            throw new AssertionError(e);
+        }
+    }
+
     private final PathTrie<MethodHandlers> handlers = new PathTrie<>(RestUtils.REST_DECODER);
 
     private final UnaryOperator<RestHandler> handlerWrapper;
@@ -88,6 +100,8 @@ public class RestController implements HttpServerTransport.Dispatcher {
         this.handlerWrapper = handlerWrapper;
         this.client = client;
         this.circuitBreakerService = circuitBreakerService;
+        registerHandlerNoWrap(RestRequest.Method.GET, "/favicon.ico", (request, channel, clnt) ->
+                channel.sendResponse(new BytesRestResponse(RestStatus.OK, "image/x-icon", FAVICON_RESPONSE)));
     }
 
     /**
@@ -149,7 +163,10 @@ public class RestController implements HttpServerTransport.Dispatcher {
         if (handler instanceof BaseRestHandler) {
             usageService.addRestHandler((BaseRestHandler) handler);
         }
-        final RestHandler maybeWrappedHandler = handlerWrapper.apply(handler);
+        registerHandlerNoWrap(method, path, handlerWrapper.apply(handler));
+    }
+
+    private void registerHandlerNoWrap(RestRequest.Method method, String path, RestHandler maybeWrappedHandler) {
         handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, method),
             (mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, method));
     }
@@ -168,10 +185,6 @@ public class RestController implements HttpServerTransport.Dispatcher {
 
     @Override
     public void dispatchRequest(RestRequest request, RestChannel channel, ThreadContext threadContext) {
-        if (request.rawPath().equals("/favicon.ico")) {
-            handleFavicon(request.method(), request.uri(), channel);
-            return;
-        }
         try {
             tryAllHandlers(request, channel, threadContext);
         } catch (Exception e) {
@@ -429,28 +442,6 @@ public class RestController implements HttpServerTransport.Dispatcher {
         return validMethods;
     }
 
-    private void handleFavicon(RestRequest.Method method, String uri, final RestChannel channel) {
-        try {
-            if (method != RestRequest.Method.GET) {
-                handleUnsupportedHttpMethod(uri, method, channel, Collections.singleton(RestRequest.Method.GET), null);
-            } else {
-                try {
-                    try (InputStream stream = getClass().getResourceAsStream("/config/favicon.ico")) {
-                        ByteArrayOutputStream out = new ByteArrayOutputStream();
-                        Streams.copy(stream, out);
-                        BytesRestResponse restResponse = new BytesRestResponse(RestStatus.OK, "image/x-icon", out.toByteArray());
-                        channel.sendResponse(restResponse);
-                    }
-                } catch (IOException e) {
-                    channel.sendResponse(
-                        new BytesRestResponse(INTERNAL_SERVER_ERROR, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
-                }
-            }
-        } catch (final IllegalArgumentException e) {
-            handleUnsupportedHttpMethod(uri, null, channel, Collections.singleton(RestRequest.Method.GET), e);
-        }
-    }
-
     private static final class ResourceHandlingHttpChannel implements RestChannel {
         private final RestChannel delegate;
         private final CircuitBreakerService circuitBreakerService;

server/src/main/java/org/elasticsearch/rest/RestHandler.java

@@ -29,6 +29,7 @@ import java.util.List;
 /**
  * Handler for REST requests
  */
+@FunctionalInterface
 public interface RestHandler {
 
     /**

server/src/test/java/org/elasticsearch/rest/RestControllerTests.java

@@ -492,8 +492,9 @@ public class RestControllerTests extends ESTestCase {
     }
 
     public void testFaviconWithWrongHttpMethod() {
-        final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
+        final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withMethod(
-            .withMethod(randomValueOtherThan(RestRequest.Method.GET, () -> randomFrom(RestRequest.Method.values())))
+                randomValueOtherThanMany(m -> m == RestRequest.Method.GET || m == RestRequest.Method.OPTIONS,
+                        () -> randomFrom(RestRequest.Method.values())))
             .withPath("/favicon.ico")
             .build();
         final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.METHOD_NOT_ALLOWED);