diff --git a/server/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java b/server/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java index 4a86ca118b176..350490c370860 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java +++ b/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 diff --git a/server/src/main/java/org/elasticsearch/usage/UsageService.java b/server/src/main/java/org/elasticsearch/usage/UsageService.java index 9e1c2e0373452..df1f53bcbb7c8 100644 --- a/server/src/main/java/org/elasticsearch/usage/UsageService.java +++ b/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 handlers; + private final Map 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 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); diff --git a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java index 114abe0b2dcb7..aa0e592e01a79 100644 --- a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java @@ -132,7 +132,14 @@ public void testPluginCantOverwriteBuiltinRestHandler() throws IOException { public List getRestHandlers(Settings settings, RestController restController, ClusterSettings clusterSettings, IndexScopedSettings indexScopedSettings, SettingsFilter settingsFilter, IndexNameExpressionResolver indexNameExpressionResolver, Supplier nodesInCluster) { - return singletonList(new RestMainAction()); + return singletonList(new RestMainAction() { + + @Override + public String getName() { + return "duplicated_" + super.getName(); + } + + }); } }; SettingsModule settings = new SettingsModule(Settings.EMPTY); diff --git a/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java b/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java index cfba6390bfb07..7c9ac5568aa1c 100644 --- a/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java +++ b/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);