From 9ca3c90cea6f80f335b8d96a3c88efbf89c33c2b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 18 May 2023 09:37:17 -0400 Subject: [PATCH] Add RouteRegistry to DynamicActionModule Signed-off-by: Craig Perkins --- .../org/opensearch/action/ActionModule.java | 50 ++++--------------- .../java/org/opensearch/rest/RestHandler.java | 10 +++- .../extensions/RestSendToExtensionAction.java | 4 +- .../extensions/ExtensionsManagerTests.java | 4 +- .../rest/RestSendToExtensionActionTests.java | 25 ++++++++++ 5 files changed, 48 insertions(+), 45 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/ActionModule.java b/server/src/main/java/org/opensearch/action/ActionModule.java index c39ddfc5ce1cf..199e23b928258 100644 --- a/server/src/main/java/org/opensearch/action/ActionModule.java +++ b/server/src/main/java/org/opensearch/action/ActionModule.java @@ -498,8 +498,6 @@ public class ActionModule extends AbstractModule { // associated with remote action execution on extensions, possibly in // a different JVM and possibly on a different server. private final DynamicActionRegistry dynamicActionRegistry; - // A dynamic route registry associated with registered extension routes - private final DynamicRouteRegistry dynamicRouteRegistry; private final ActionFilters actionFilters; private final AutoCreateIndex autoCreateIndex; private final DestructiveOperations destructiveOperations; @@ -532,7 +530,6 @@ public ActionModule( actions = setupActions(actionPlugins); actionFilters = setupActionFilters(actionPlugins); dynamicActionRegistry = new DynamicActionRegistry(); - dynamicRouteRegistry = new DynamicRouteRegistry(dynamicActionRegistry); autoCreateIndex = new AutoCreateIndex(settings, clusterSettings, indexNameExpressionResolver, systemIndices); destructiveOperations = new DestructiveOperations(settings, clusterSettings); Set headers = Stream.concat( @@ -1010,10 +1007,6 @@ public DynamicActionRegistry getDynamicActionRegistry() { return dynamicActionRegistry; } - public DynamicRouteRegistry getDynamicRouteRegistry() { - return dynamicRouteRegistry; - } - public RestController getRestController() { return restController; } @@ -1034,6 +1027,10 @@ public static class DynamicActionRegistry { // at times other than node bootstrap. private final Map, TransportAction> registry = new ConcurrentHashMap<>(); + // A dynamic registry to add or remove Route / RestSendToExtensionAction pairs + // at times other than node bootstrap. + private final Map routeRegistry = new ConcurrentHashMap<>(); + private final Set registeredActionNames = new ConcurrentSkipListSet<>(); /** @@ -1098,31 +1095,6 @@ public boolean isActionRegistered(String actionName) { } return registry.get(action); } - } - - /** - * The DynamicRouteRegistry maintains a registry mapping {@link org.opensearch.rest.RestHandler.Route} instances to {@link org.opensearch.rest.extensions.RestSendToExtensionAction} instances. - *

- * This class is modeled after {@link NamedRegistry} but provides both register and unregister capabilities. - * - * @opensearch.internal - */ - public class DynamicRouteRegistry { - // This is an instance of a DynamicActionRegistry containing all registered transport actions - private DynamicActionRegistry dynamicActionRegistry; - // A dynamic registry to add or remove Route / RestSendToExtensionAction pairs - // at times other than node bootstrap. - private final Map registry = new ConcurrentHashMap<>(); - - private final Set registeredRestActionNames = new ConcurrentSkipListSet<>(); - - /** - * - * @param dynamicActionRegistry A registry of all transport actions - native, plugins and dynamic - */ - public DynamicRouteRegistry(DynamicActionRegistry dynamicActionRegistry) { - this.dynamicActionRegistry = dynamicActionRegistry; - } /** * Add a dynamic action to the registry. @@ -1136,15 +1108,15 @@ public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAct Optional routeName = Optional.empty(); if (route instanceof NamedRoute) { routeName = Optional.of(((NamedRoute) route).name()); - if (dynamicActionRegistry.isActionRegistered(routeName.get()) || registeredRestActionNames.contains(routeName.get())) { + if (isActionRegistered(routeName.get()) || registeredActionNames.contains(routeName.get())) { throw new IllegalArgumentException("route [" + route + "] already registered"); } } - if (registry.containsKey(route)) { + if (routeRegistry.containsKey(route)) { throw new IllegalArgumentException("route [" + route + "] already registered"); } - registry.put(route, action); - routeName.ifPresent(registeredRestActionNames::add); + routeRegistry.put(route, action); + routeName.ifPresent(registeredActionNames::add); } /** @@ -1154,11 +1126,11 @@ public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAct */ public void unregisterDynamicRoute(RestHandler.Route route) { requireNonNull(route, "route is required"); - if (registry.remove(route) == null) { + if (routeRegistry.remove(route) == null) { throw new IllegalArgumentException("action [" + route + "] was not registered"); } if (route instanceof NamedRoute) { - registeredRestActionNames.remove(((NamedRoute) route).name()); + registeredActionNames.remove(((NamedRoute) route).name()); } } @@ -1170,7 +1142,7 @@ public void unregisterDynamicRoute(RestHandler.Route route) { */ @SuppressWarnings("unchecked") public RestSendToExtensionAction get(RestHandler.Route route) { - return registry.get(route); + return routeRegistry.get(route); } } } diff --git a/server/src/main/java/org/opensearch/rest/RestHandler.java b/server/src/main/java/org/opensearch/rest/RestHandler.java index e0f6d4e73c81b..7832649e8ad32 100644 --- a/server/src/main/java/org/opensearch/rest/RestHandler.java +++ b/server/src/main/java/org/opensearch/rest/RestHandler.java @@ -196,13 +196,18 @@ public String getPath() { return path; } + public String getPathWithPathParamsReplaced() { + return path.replaceAll("(?<=\\{).*?(?=\\})", "path_param"); + } + public Method getMethod() { return method; } @Override public int hashCode() { - return toString().hashCode(); + String routeStr = "Route [method=" + method + ", path=" + getPathWithPathParamsReplaced() + "]"; + return routeStr.hashCode(); } @Override @@ -219,7 +224,8 @@ public boolean equals(Object o) { return false; } Route that = (Route) o; - return Objects.equals(method, that.method) && Objects.equals(path, that.path); + return Objects.equals(method, that.method) + && Objects.equals(getPathWithPathParamsReplaced(), that.getPathWithPathParamsReplaced()); } } diff --git a/server/src/main/java/org/opensearch/rest/extensions/RestSendToExtensionAction.java b/server/src/main/java/org/opensearch/rest/extensions/RestSendToExtensionAction.java index f0cb148268a1d..e59791124767c 100644 --- a/server/src/main/java/org/opensearch/rest/extensions/RestSendToExtensionAction.java +++ b/server/src/main/java/org/opensearch/rest/extensions/RestSendToExtensionAction.java @@ -111,11 +111,11 @@ public RestSendToExtensionAction( if (name.isPresent()) { NamedRoute nr = new NamedRoute(method, path, name.get()); restActionsAsRoutes.add(nr); - actionModule.getDynamicRouteRegistry().registerDynamicRoute(nr, this); + actionModule.getDynamicActionRegistry().registerDynamicRoute(nr, this); } else { Route r = new Route(method, path); restActionsAsRoutes.add(r); - actionModule.getDynamicRouteRegistry().registerDynamicRoute(r, this); + actionModule.getDynamicActionRegistry().registerDynamicRoute(r, this); } } this.routes = unmodifiableList(restActionsAsRoutes); diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 3fb1e1dc5c8ca..64ea5688b01e0 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -41,7 +41,7 @@ import org.junit.Before; import org.opensearch.Version; import org.opensearch.action.ActionModule; -import org.opensearch.action.ActionModule.DynamicRouteRegistry; +import org.opensearch.action.ActionModule.DynamicActionRegistry; import org.opensearch.action.admin.cluster.state.ClusterStateResponse; import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.ClusterSettingsResponse; @@ -169,7 +169,7 @@ public void setup() throws Exception { new UsageService(), new IdentityService(Settings.EMPTY, List.of()) ); - when(actionModule.getDynamicRouteRegistry()).thenReturn(mock(DynamicRouteRegistry.class)); + when(actionModule.getDynamicActionRegistry()).thenReturn(mock(DynamicActionRegistry.class)); when(actionModule.getRestController()).thenReturn(restController); settingsModule = new SettingsModule(Settings.EMPTY, emptyList(), emptyList(), emptySet()); clusterService = createClusterService(threadPool); diff --git a/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java b/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java index 5d11113b37220..e2346a8ea37b5 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java @@ -220,6 +220,31 @@ public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPath() throws ); } + public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithDifferentPathParams() throws Exception { + RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( + "uniqueid1", + List.of("GET /foo/{path_param1}", "GET /foo/{path_param2}"), + List.of() + ); + expectThrows( + IllegalArgumentException.class, + () -> new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, actionModule) + ); + } + + public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() throws Exception { + RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( + "uniqueid1", + List.of("GET /foo/{path_param}", "GET /foo/{path_param}/list"), + List.of() + ); + try { + new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, actionModule); + } catch (IllegalArgumentException e) { + fail("IllegalArgumentException should not be thrown for different paths"); + } + } + public void testRestSendToExtensionWithNamedRouteCollidingWithDynamicTransportAction() throws Exception { DynamicActionRegistry dynamicActionRegistry = actionModule.getDynamicActionRegistry(); ActionFilters emptyFilters = new ActionFilters(Collections.emptySet());