Add validation to the usage service

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

@@ -89,7 +89,7 @@ public AutoIdHandler(ClusterService clusterService) {
 
         @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.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 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 NodeUsage getUsageStats(DiscoveryNode localNode, boolean restActions) {
         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 void testPluginCantOverwriteBuiltinRestHandler() throws IOException {
             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 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 @@
 
 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);