Add validation to the usage service (#54617)

Today the usage service can let in some issues, such as handlers that do
not have a name, where the errors do not manifest until later (calling
the usage API), or conflicting handlers with the same name. This commit
addresses this by adding some validation to the usage service.

server/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java

@@ -89,7 +89,7 @@ public class RestIndexAction extends BaseRestHandler {
 
         @Override
         public String getName() {
-            return "document_create_action";
+            return "document_create_action_auto_id";
         }
 
         @Override

server/src/main/java/org/elasticsearch/usage/UsageService.java

@@ -42,21 +42,21 @@ import org.elasticsearch.action.admin.cluster.node.usage.NodeUsage;
 import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.rest.BaseRestHandler;
 
-import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.List;
+import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 
 /**
  * A service to monitor usage of Elasticsearch features.
  */
 public class UsageService {
 
-    private final List<BaseRestHandler> handlers;
+    private final Map<String, BaseRestHandler> handlers;
     private final long sinceTime;
 
     public UsageService() {
-        this.handlers = new ArrayList<>();
+        this.handlers = new HashMap<>();
         this.sinceTime = System.currentTimeMillis();
     }
 
@@ -67,7 +67,26 @@ public class UsageService {
      *            the {@link BaseRestHandler} to add to the usage service.
      */
     public void addRestHandler(BaseRestHandler handler) {
-        handlers.add(handler);
+        Objects.requireNonNull(handler);
+        if (handler.getName() == null) {
+            throw new IllegalArgumentException("handler of type [" + handler.getClass().getName() + "] does not have a name");
+        }
+        final BaseRestHandler maybeHandler = handlers.put(handler.getName(), handler);
+        /*
+         * Handlers will be registered multiple times, once for each route that the handler handles. This means that we will see handlers
+         * multiple times, so we do not have a conflict if we are seeing the same instance multiple times. So, we only reject if a handler
+         * with the same name was registered before, and it is not the same instance as before.
+         */
+        if (maybeHandler != null && maybeHandler != handler) {
+            final String message = String.format(
+                Locale.ROOT,
+                "handler of type [%s] conflicts with handler of type [%s] as they both have the same name [%s]",
+                handler.getClass().getName(),
+                maybeHandler.getClass().getName(),
+                handler.getName()
+            );
+            throw new IllegalArgumentException(message);
+        }
     }
 
     /**
@@ -85,7 +104,7 @@ public class UsageService {
         Map<String, Long> restUsageMap;
         if (restActions) {
             restUsageMap = new HashMap<>();
-            handlers.forEach(handler -> {
+            handlers.values().forEach(handler -> {
                 long usageCount = handler.getUsageCount();
                 if (usageCount > 0) {
                     restUsageMap.put(handler.getName(), usageCount);

server/src/test/java/org/elasticsearch/action/ActionModuleTests.java

@@ -132,7 +132,14 @@ public class ActionModuleTests extends ESTestCase {
             public List<RestHandler> getRestHandlers(Settings settings, RestController restController, ClusterSettings clusterSettings,
                     IndexScopedSettings indexScopedSettings, SettingsFilter settingsFilter,
                     IndexNameExpressionResolver indexNameExpressionResolver, Supplier<DiscoveryNodes> nodesInCluster) {
-                return singletonList(new RestMainAction());
+                return singletonList(new RestMainAction() {
+
+                    @Override
+                    public String getName() {
+                        return "duplicated_" + super.getName();
+                    }
+
+                });
             }
         };
         SettingsModule settings = new SettingsModule(Settings.EMPTY);

server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java

@@ -32,6 +32,7 @@ import org.elasticsearch.test.rest.FakeRestRequest;
 import java.net.InetAddress;
 import java.util.Collections;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 
 import static org.hamcrest.Matchers.equalTo;
@@ -41,6 +42,58 @@ import static org.hamcrest.Matchers.sameInstance;
 
 public class UsageServiceTests extends ESTestCase {
 
+    /**
+     * Test that we can not add a null reference to a {@link org.elasticsearch.rest.RestHandler} to the {@link UsageService}.
+     */
+    public void testHandlerCanNotBeNull() {
+        final UsageService service = new UsageService();
+        expectThrows(NullPointerException.class, () -> service.addRestHandler(null));
+    }
+
+    /**
+     * Test that we can not add an instance of a {@link org.elasticsearch.rest.RestHandler} with no name to the {@link UsageService}.
+     */
+    public void testAHandlerWithNoName() {
+        final UsageService service = new UsageService();
+        final BaseRestHandler horse = new MockRestHandler(null);
+        final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> service.addRestHandler(horse));
+        assertThat(
+            e.getMessage(),
+            equalTo("handler of type [org.elasticsearch.usage.UsageServiceTests$MockRestHandler] does not have a name"));
+    }
+
+    /**
+     * Test that we can add the same instance of a {@link org.elasticsearch.rest.RestHandler} to the {@link UsageService} multiple times.
+     */
+    public void testHandlerWithConflictingNamesButSameInstance() {
+        final UsageService service = new UsageService();
+        final String name = randomAlphaOfLength(8);
+        final BaseRestHandler first = new MockRestHandler(name);
+        service.addRestHandler(first);
+        // nothing bad ever happens to me
+        service.addRestHandler(first);
+    }
+
+    /**
+     * Test that we can not add different instances of {@link org.elasticsearch.rest.RestHandler} with the same name to the
+     * {@link UsageService}.
+     */
+    public void testHandlersWithConflictingNamesButDifferentInstances() {
+        final UsageService service = new UsageService();
+        final String name = randomAlphaOfLength(8);
+        final BaseRestHandler first = new MockRestHandler(name);
+        final BaseRestHandler second = new MockRestHandler(name);
+        service.addRestHandler(first);
+        final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> service.addRestHandler(second));
+        final String expected = String.format(
+            Locale.ROOT,
+            "handler of type [%s] conflicts with handler of type [%1$s] as they both have the same name [%s]",
+            "org.elasticsearch.usage.UsageServiceTests$MockRestHandler",
+            name
+        );
+        assertThat(e.getMessage(), equalTo(expected));
+    }
+
     public void testRestUsage() throws Exception {
         DiscoveryNode discoveryNode = new DiscoveryNode("foo", new TransportAddress(InetAddress.getByName("localhost"), 12345),
                 Version.CURRENT);