From 9d37470dec62c045df0272a7340eec970ce4809d Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 1 Apr 2020 18:19:07 -0400 Subject: [PATCH 1/4] Add validation to the usage service 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. --- .../rest/action/document/RestIndexAction.java | 2 +- .../org/elasticsearch/usage/UsageService.java | 31 +++++++++++--- .../usage/UsageServiceTests.java | 40 +++++++++++++++++++ 3 files changed, 66 insertions(+), 7 deletions(-) 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..e1dae74a82bd2 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 reach 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/usage/UsageServiceTests.java b/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java index cfba6390bfb07..285483a480213 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,45 @@ public class UsageServiceTests extends ESTestCase { + public void testNullHandler() { + final UsageService service = new UsageService(); + expectThrows(NullPointerException.class, () -> service.addRestHandler(null)); + } + + 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")); + } + + public void testHandlerWithConflictingNamesButSameInstance() { + final UsageService service = new UsageService(); + final String name = randomAlphaOfLength(8); + final BaseRestHandler first = new MockRestHandler(name); + service.addRestHandler(first); + // nothing bad should happen + service.addRestHandler(first); + } + + 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); From 0dca4551d47d416db5ed55025bc872500fd68346 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 1 Apr 2020 20:17:10 -0400 Subject: [PATCH 2/4] Fix test --- .../java/org/elasticsearch/action/ActionModuleTests.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) 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); From 62a0b79f7023c2ff900d190668146e2b83a72a96 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 1 Apr 2020 21:43:02 -0400 Subject: [PATCH 3/4] Fix typo in comment --- server/src/main/java/org/elasticsearch/usage/UsageService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/usage/UsageService.java b/server/src/main/java/org/elasticsearch/usage/UsageService.java index e1dae74a82bd2..df1f53bcbb7c8 100644 --- a/server/src/main/java/org/elasticsearch/usage/UsageService.java +++ b/server/src/main/java/org/elasticsearch/usage/UsageService.java @@ -73,7 +73,7 @@ public void addRestHandler(BaseRestHandler handler) { } final BaseRestHandler maybeHandler = handlers.put(handler.getName(), handler); /* - * Handlers will be registered multiple times, once for reach route that the handler handles. This means that we will see handlers + * 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. */ From bcbf0015482023731a8d0b53a9c48962432767f9 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Thu, 2 Apr 2020 08:07:15 -0400 Subject: [PATCH 4/4] Add Javadocs --- .../elasticsearch/usage/UsageServiceTests.java | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java b/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java index 285483a480213..7c9ac5568aa1c 100644 --- a/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java +++ b/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java @@ -42,11 +42,17 @@ public class UsageServiceTests extends ESTestCase { - public void testNullHandler() { + /** + * 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); @@ -56,15 +62,22 @@ public void testAHandlerWithNoName() { 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 should happen + // 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);