From 50d8006d5ffb72566b893ef23748d48b26c3a65e Mon Sep 17 00:00:00 2001 From: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> Date: Wed, 5 Jul 2023 17:55:53 -0400 Subject: [PATCH] Support transport action names when registering NamedRoutes (#7957) * Adds ability to register legacy action names along with route Signed-off-by: Darshit Chanpura Signed-off-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> --- CHANGELOG.md | 3 +- .../org/opensearch/action/ActionModule.java | 41 +++-- .../rest/RestInitializeExtensionAction.java | 4 +- .../rest/RestSendToExtensionAction.java | 42 +++-- .../extensions/rest/RouteHandler.java | 69 -------- .../java/org/opensearch/rest/NamedRoute.java | 156 ++++++++++++++++-- .../action/DynamicActionRegistryTests.java | 31 +++- .../extensions/ExtensionsManagerTests.java | 4 +- .../rest/RestSendToExtensionActionTests.java | 109 ++++++++++-- .../extensions/rest/RouteHandlerTests.java | 36 ---- .../org/opensearch/rest/NamedRouteTests.java | 102 ++++++++++-- 11 files changed, 411 insertions(+), 186 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/extensions/rest/RouteHandler.java delete mode 100644 server/src/test/java/org/opensearch/extensions/rest/RouteHandlerTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 99faefda9ad87..aee081167d68f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Enable Point based optimization for custom comparators ([#8168](https://github.com/opensearch-project/OpenSearch/pull/8168)) - [Extensions] Support extension additional settings with extension REST initialization ([#8414](https://github.com/opensearch-project/OpenSearch/pull/8414)) - Adds mock implementation for TelemetryPlugin ([#7545](https://github.com/opensearch-project/OpenSearch/issues/7545)) +- Support transport action names when registering NamedRoutes ([#7957](https://github.com/opensearch-project/OpenSearch/pull/7957)) ### Dependencies - Bump `com.azure:azure-storage-common` from 12.21.0 to 12.21.1 (#7566, #7814) @@ -169,4 +170,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Security [Unreleased 3.0]: https://github.com/opensearch-project/OpenSearch/compare/2.x...HEAD -[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.8...2.x \ No newline at end of file +[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.8...2.x diff --git a/server/src/main/java/org/opensearch/action/ActionModule.java b/server/src/main/java/org/opensearch/action/ActionModule.java index 902ae7cc54e3f..56d1758161ced 100644 --- a/server/src/main/java/org/opensearch/action/ActionModule.java +++ b/server/src/main/java/org/opensearch/action/ActionModule.java @@ -462,9 +462,9 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentSkipListSet; @@ -1044,7 +1044,7 @@ public static class DynamicActionRegistry { // A dynamic registry to add or remove Route / RestSendToExtensionAction pairs // at times other than node bootstrap. - private final Map routeRegistry = new ConcurrentHashMap<>(); + private final Map routeRegistry = new ConcurrentHashMap<>(); private final Set registeredActionNames = new ConcurrentSkipListSet<>(); @@ -1112,26 +1112,37 @@ public boolean isActionRegistered(String actionName) { } /** - * Add a dynamic action to the registry. + * Adds a dynamic route to the registry. * * @param route The route instance to add * @param action The corresponding instance of RestSendToExtensionAction to execute */ - public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAction action) { + public void registerDynamicRoute(NamedRoute route, RestSendToExtensionAction action) { requireNonNull(route, "route is required"); requireNonNull(action, "action is required"); - Optional routeName = Optional.empty(); - if (route instanceof NamedRoute) { - routeName = Optional.of(((NamedRoute) route).name()); - if (isActionRegistered(routeName.get()) || registeredActionNames.contains(routeName.get())) { - throw new IllegalArgumentException("route [" + route + "] already registered"); - } + + String routeName = route.name(); + requireNonNull(routeName, "route name is required"); + if (isActionRegistered(routeName)) { + throw new IllegalArgumentException("route [" + route + "] already registered"); } + + Set actionNames = route.actionNames(); + if (!Collections.disjoint(actionNames, registeredActionNames)) { + Set alreadyRegistered = new HashSet<>(registeredActionNames); + alreadyRegistered.retainAll(actionNames); + String acts = String.join(", ", alreadyRegistered); + throw new IllegalArgumentException( + "action" + (alreadyRegistered.size() > 1 ? "s [" : " [") + acts + "] already registered" + ); + } + if (routeRegistry.containsKey(route)) { throw new IllegalArgumentException("route [" + route + "] already registered"); } routeRegistry.put(route, action); - routeName.ifPresent(registeredActionNames::add); + registeredActionNames.add(routeName); + registeredActionNames.addAll(actionNames); } /** @@ -1139,14 +1150,14 @@ public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAct * * @param route The route to remove */ - public void unregisterDynamicRoute(RestHandler.Route route) { + public void unregisterDynamicRoute(NamedRoute route) { requireNonNull(route, "route is required"); if (routeRegistry.remove(route) == null) { throw new IllegalArgumentException("action [" + route + "] was not registered"); } - if (route instanceof NamedRoute) { - registeredActionNames.remove(((NamedRoute) route).name()); - } + + registeredActionNames.remove(route.name()); + registeredActionNames.removeAll(route.actionNames()); } /** diff --git a/server/src/main/java/org/opensearch/extensions/rest/RestInitializeExtensionAction.java b/server/src/main/java/org/opensearch/extensions/rest/RestInitializeExtensionAction.java index f47f342617732..878673b77a4a9 100644 --- a/server/src/main/java/org/opensearch/extensions/rest/RestInitializeExtensionAction.java +++ b/server/src/main/java/org/opensearch/extensions/rest/RestInitializeExtensionAction.java @@ -22,6 +22,7 @@ import org.opensearch.extensions.ExtensionsSettings.Extension; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; +import org.opensearch.rest.NamedRoute; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestStatus; import org.opensearch.transport.ConnectTransportException; @@ -54,7 +55,7 @@ public String getName() { @Override public List routes() { - return List.of(new Route(POST, "/_extensions/initialize")); + return List.of(new NamedRoute.Builder().method(POST).path("/_extensions/initialize").uniqueName("extensions:initialize").build()); } public RestInitializeExtensionAction(ExtensionsManager extensionsManager) { @@ -187,6 +188,5 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client channel.sendResponse(new BytesRestResponse(RestStatus.ACCEPTED, builder)); } }; - } } diff --git a/server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java b/server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java index 073b3f3f45818..3dd6056bb36cf 100644 --- a/server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java +++ b/server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java @@ -33,9 +33,9 @@ import java.nio.charset.StandardCharsets; import java.security.Principal; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import java.util.concurrent.CompletableFuture; @@ -90,33 +90,43 @@ public RestSendToExtensionAction( List restActionsAsRoutes = new ArrayList<>(); for (String restAction : restActionsRequest.getRestActions()) { - Optional name = Optional.empty(); + + // TODO Find a better way to parse these to avoid code-smells + + String name; + Set actionNames = new HashSet<>(); String[] parts = restAction.split(" "); - if (parts.length < 2) { - throw new IllegalArgumentException("REST action must contain at least a REST method and route"); + if (parts.length < 3) { + throw new IllegalArgumentException("REST action must contain at least a REST method, a route and a unique name"); } try { method = RestRequest.Method.valueOf(parts[0].trim()); path = pathPrefix + parts[1].trim(); - if (parts.length > 2) { - name = Optional.of(parts[2].trim()); + name = parts[2].trim(); + + // comma-separated action names + if (parts.length > 3) { + String[] actions = parts[3].split(","); + for (String action : actions) { + String trimmed = action.trim(); + if (!trimmed.isEmpty()) { + actionNames.add(trimmed); + } + } } } catch (IndexOutOfBoundsException | IllegalArgumentException e) { throw new IllegalArgumentException(restAction + " does not begin with a valid REST method"); } - logger.info("Registering: " + method + " " + path); - if (name.isPresent()) { - NamedRoute nr = new NamedRoute(method, path, name.get()); - restActionsAsRoutes.add(nr); - dynamicActionRegistry.registerDynamicRoute(nr, this); - } else { - Route r = new Route(method, path); - restActionsAsRoutes.add(r); - dynamicActionRegistry.registerDynamicRoute(r, this); - } + logger.info("Registering: " + method + " " + path + " " + name); + + // All extension routes being registered must have a unique name associated with them + NamedRoute nr = new NamedRoute.Builder().method(method).path(path).uniqueName(name).legacyActionNames(actionNames).build(); + restActionsAsRoutes.add(nr); + dynamicActionRegistry.registerDynamicRoute(nr, this); } this.routes = unmodifiableList(restActionsAsRoutes); + // TODO: Modify {@link NamedRoute} to support deprecated route registration List restActionsAsDeprecatedRoutes = new ArrayList<>(); // Iterate in pairs of route / deprecation message List deprecatedActions = restActionsRequest.getDeprecatedRestActions(); diff --git a/server/src/main/java/org/opensearch/extensions/rest/RouteHandler.java b/server/src/main/java/org/opensearch/extensions/rest/RouteHandler.java deleted file mode 100644 index 189d67c120189..0000000000000 --- a/server/src/main/java/org/opensearch/extensions/rest/RouteHandler.java +++ /dev/null @@ -1,69 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.extensions.rest; - -import java.util.function.Function; - -import org.opensearch.rest.RestHandler.Route; -import org.opensearch.rest.RestRequest; -import org.opensearch.rest.RestRequest.Method; - -/** - * A subclass of {@link Route} that includes a handler method for that route. - */ -public class RouteHandler extends Route { - - private final String name; - - private final Function responseHandler; - - /** - * Handle the method and path with the specified handler. - * - * @param method The {@link Method} to handle. - * @param path The path to handle. - * @param handler The method which handles the method and path. - */ - public RouteHandler(Method method, String path, Function handler) { - super(method, path); - this.responseHandler = handler; - this.name = null; - } - - /** - * Handle the method and path with the specified handler. - * - * @param name The name of the handler. - * @param method The {@link Method} to handle. - * @param path The path to handle. - * @param handler The method which handles the method and path. - */ - public RouteHandler(String name, Method method, String path, Function handler) { - super(method, path); - this.responseHandler = handler; - this.name = name; - } - - /** - * Executes the handler for this route. - * - * @param request The request to handle - * @return the {@link ExtensionRestResponse} result from the handler for this route. - */ - public ExtensionRestResponse handleRequest(RestRequest request) { - return responseHandler.apply(request); - } - - /** - * The name of the RouteHandler. Must be unique across route handlers. - */ - public String name() { - return this.name; - } -} diff --git a/server/src/main/java/org/opensearch/rest/NamedRoute.java b/server/src/main/java/org/opensearch/rest/NamedRoute.java index f5eaafcd04056..109f688a4924e 100644 --- a/server/src/main/java/org/opensearch/rest/NamedRoute.java +++ b/server/src/main/java/org/opensearch/rest/NamedRoute.java @@ -9,6 +9,13 @@ package org.opensearch.rest; import org.opensearch.OpenSearchException; +import org.opensearch.transport.TransportService; + +import java.util.HashSet; +import java.util.Set; +import java.util.function.Function; + +import static java.util.Objects.requireNonNull; /** * A named Route @@ -16,21 +23,123 @@ * @opensearch.internal */ public class NamedRoute extends RestHandler.Route { + private static final String VALID_ACTION_NAME_PATTERN = "^[a-zA-Z0-9:/*_]*$"; static final int MAX_LENGTH_OF_ACTION_NAME = 250; - private final String name; + private final String uniqueName; + private final Set actionNames; - public boolean isValidRouteName(String routeName) { - if (routeName == null || routeName.isBlank() || routeName.length() > MAX_LENGTH_OF_ACTION_NAME) { - return false; + private Function handler; + + /** + * Builder class for constructing instances of {@link NamedRoute}. + */ + public static class Builder { + private RestRequest.Method method; + private String path; + private String uniqueName; + private final Set legacyActionNames = new HashSet<>(); + private Function handler; + + /** + * Sets the REST method for the route. + * + * @param method the REST method for the route + * @return the builder instance + */ + public Builder method(RestRequest.Method method) { + requireNonNull(method, "REST method must not be null."); + this.method = method; + return this; + } + + /** + * Sets the URL path for the route. + * + * @param path the URL path for the route + * @return the builder instance + */ + public Builder path(String path) { + requireNonNull(path, "REST path must not be null."); + this.path = path; + return this; + } + + /** + * Sets the name for the route. + * + * @param name the name for the route + * @return the builder instance + */ + public Builder uniqueName(String name) { + requireNonNull(name, "REST route name must not be null."); + this.uniqueName = name; + return this; + } + + /** + * Sets the legacy action names for the route. + * + * @param legacyActionNames the legacy action names for the route + * @return the builder instance + */ + public Builder legacyActionNames(Set legacyActionNames) { + this.legacyActionNames.addAll(validateLegacyActionNames(legacyActionNames)); + return this; + } + + /** + * Sets the handler for this route + * + * @param handler the handler for this route + * @return the builder instance + */ + public Builder handler(Function handler) { + requireNonNull(handler, "Route handler must not be null."); + this.handler = handler; + return this; + } + + /** + * Builds a new instance of {@link NamedRoute} based on the provided parameters. + * + * @return a new instance of {@link NamedRoute} + * @throws OpenSearchException if the route name is invalid + */ + public NamedRoute build() { + checkIfFieldsAreSet(); + return new NamedRoute(this); + } + + /** + * Checks if all builder fields are set before creating a new NamedRoute object + */ + private void checkIfFieldsAreSet() { + if (method == null || path == null || uniqueName == null) { + throw new IllegalStateException("REST method, path and uniqueName are required."); + } + } + + private Set validateLegacyActionNames(Set legacyActionNames) { + if (legacyActionNames == null) { + return new HashSet<>(); + } + for (String actionName : legacyActionNames) { + if (!TransportService.isValidActionName(actionName)) { + throw new OpenSearchException( + "Invalid action name [" + actionName + "]. It must start with one of: " + TransportService.VALID_ACTION_PREFIXES + ); + } + } + return legacyActionNames; } - return routeName.matches(VALID_ACTION_NAME_PATTERN); + } - public NamedRoute(RestRequest.Method method, String path, String name) { - super(method, path); - if (!isValidRouteName(name)) { + private NamedRoute(Builder builder) { + super(builder.method, builder.path); + if (!isValidRouteName(builder.uniqueName)) { throw new OpenSearchException( "Invalid route name specified. The route name may include the following characters" + " 'a-z', 'A-Z', '0-9', ':', '/', '*', '_' and be less than " @@ -38,18 +147,43 @@ public NamedRoute(RestRequest.Method method, String path, String name) { + " characters" ); } - this.name = name; + this.uniqueName = builder.uniqueName; + this.actionNames = Set.copyOf(builder.legacyActionNames); + this.handler = builder.handler; + } + + public boolean isValidRouteName(String routeName) { + return routeName != null + && !routeName.isBlank() + && routeName.length() <= MAX_LENGTH_OF_ACTION_NAME + && routeName.matches(VALID_ACTION_NAME_PATTERN); } /** * The name of the Route. Must be unique across Route. */ public String name() { - return this.name; + return this.uniqueName; + } + + /** + * The legacy transport Action name to match against this route to support authorization in REST layer. + * MUST be unique across all Routes + */ + public Set actionNames() { + return this.actionNames; + } + + /** + * The handler associated with this route + * @return the handler associated with this route + */ + public Function handler() { + return handler; } @Override public String toString() { - return "NamedRoute [method=" + method + ", path=" + path + ", name=" + name + "]"; + return "NamedRoute [method=" + method + ", path=" + path + ", name=" + uniqueName + ", actionNames=" + actionNames + "]"; } } diff --git a/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java b/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java index 963d47df3baff..17e424ee81e7e 100644 --- a/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java +++ b/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java @@ -26,6 +26,7 @@ import java.io.IOException; import java.util.Collections; import java.util.Map; +import java.util.Set; import static org.mockito.Mockito.mock; @@ -80,8 +81,8 @@ public void testDynamicActionRegistry() { public void testDynamicActionRegistryWithNamedRoutes() { RestSendToExtensionAction action = mock(RestSendToExtensionAction.class); RestSendToExtensionAction action2 = mock(RestSendToExtensionAction.class); - NamedRoute r1 = new NamedRoute(RestRequest.Method.GET, "/foo", "foo"); - NamedRoute r2 = new NamedRoute(RestRequest.Method.GET, "/bar", "bar"); + NamedRoute r1 = new NamedRoute.Builder().method(RestRequest.Method.GET).path("/foo").uniqueName("foo").build(); + NamedRoute r2 = new NamedRoute.Builder().method(RestRequest.Method.PUT).path("/bar").uniqueName("bar").build(); DynamicActionRegistry registry = new DynamicActionRegistry(); registry.registerDynamicRoute(r1, action); @@ -89,22 +90,38 @@ public void testDynamicActionRegistryWithNamedRoutes() { assertTrue(registry.isActionRegistered("foo")); assertTrue(registry.isActionRegistered("bar")); + + registry.unregisterDynamicRoute(r2); + + assertTrue(registry.isActionRegistered("foo")); + assertFalse(registry.isActionRegistered("bar")); } - public void testDynamicActionRegistryRegisterAndUnregisterWithNamedRoutes() { + public void testDynamicActionRegistryWithNamedRoutesAndLegacyActionNames() { RestSendToExtensionAction action = mock(RestSendToExtensionAction.class); RestSendToExtensionAction action2 = mock(RestSendToExtensionAction.class); - NamedRoute r1 = new NamedRoute(RestRequest.Method.GET, "/foo", "foo"); - NamedRoute r2 = new NamedRoute(RestRequest.Method.GET, "/bar", "bar"); + NamedRoute r1 = new NamedRoute.Builder().method(RestRequest.Method.GET) + .path("/foo") + .uniqueName("foo") + .legacyActionNames(Set.of("cluster:admin/opensearch/abc/foo")) + .build(); + NamedRoute r2 = new NamedRoute.Builder().method(RestRequest.Method.PUT) + .path("/bar") + .uniqueName("bar") + .legacyActionNames(Set.of("cluster:admin/opensearch/xyz/bar")) + .build(); DynamicActionRegistry registry = new DynamicActionRegistry(); registry.registerDynamicRoute(r1, action); registry.registerDynamicRoute(r2, action2); + assertTrue(registry.isActionRegistered("cluster:admin/opensearch/abc/foo")); + assertTrue(registry.isActionRegistered("cluster:admin/opensearch/xyz/bar")); + registry.unregisterDynamicRoute(r2); - assertTrue(registry.isActionRegistered("foo")); - assertFalse(registry.isActionRegistered("bar")); + assertTrue(registry.isActionRegistered("cluster:admin/opensearch/abc/foo")); + assertFalse(registry.isActionRegistered("cluster:admin/opensearch/xyz/bar")); } private static final class TestAction extends ActionType { diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 713a70c6a7d3e..3f61d01166fb9 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -443,8 +443,8 @@ public void testHandleRegisterRestActionsRequest() throws Exception { initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; - List actionsList = List.of("GET /foo", "PUT /bar", "POST /baz"); - List deprecatedActionsList = List.of("GET /deprecated/foo", "It's deprecated!"); + List actionsList = List.of("GET /foo foo", "PUT /bar bar", "POST /baz baz"); + List deprecatedActionsList = List.of("GET /deprecated/foo foo_deprecated", "It's deprecated!"); RegisterRestActionsRequest registerActionsRequest = new RegisterRestActionsRequest(uniqueIdStr, actionsList, deprecatedActionsList); TransportResponse response = extensionsManager.getRestActionsRequestHandler() .handleRegisterRestActionsRequest(registerActionsRequest, actionModule.getDynamicActionRegistry()); 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 fe8792b36f048..23a9169b91e21 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java @@ -135,8 +135,8 @@ public void tearDown() throws Exception { public void testRestSendToExtensionAction() throws Exception { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", - List.of("GET /foo", "PUT /bar", "POST /baz"), - List.of("GET /deprecated/foo", "It's deprecated!") + List.of("GET /foo foo", "PUT /bar bar", "POST /baz baz"), + List.of("GET /deprecated/foo foo_deprecated", "Its deprecated") ); RestSendToExtensionAction restSendToExtensionAction = new RestSendToExtensionAction( registerRestActionRequest, @@ -180,9 +180,70 @@ public void testRestSendToExtensionActionWithNamedRoute() throws Exception { assertEquals("send_to_extension_action", restSendToExtensionAction.getName()); List expected = new ArrayList<>(); String uriPrefix = "/_extensions/_uniqueid1"; - expected.add(new NamedRoute(Method.GET, uriPrefix + "/foo", "foo")); - expected.add(new NamedRoute(Method.PUT, uriPrefix + "/bar", "bar")); - expected.add(new NamedRoute(Method.POST, uriPrefix + "/baz", "baz")); + NamedRoute nr1 = new NamedRoute.Builder().method(Method.GET).path(uriPrefix + "/foo").uniqueName("foo").build(); + + NamedRoute nr2 = new NamedRoute.Builder().method(Method.PUT).path(uriPrefix + "/bar").uniqueName("bar").build(); + + NamedRoute nr3 = new NamedRoute.Builder().method(Method.POST).path(uriPrefix + "/baz").uniqueName("baz").build(); + + expected.add(nr1); + expected.add(nr2); + expected.add(nr3); + + List routes = restSendToExtensionAction.routes(); + assertEquals(expected.size(), routes.size()); + List expectedPaths = expected.stream().map(Route::getPath).collect(Collectors.toList()); + List paths = routes.stream().map(Route::getPath).collect(Collectors.toList()); + List expectedMethods = expected.stream().map(Route::getMethod).collect(Collectors.toList()); + List methods = routes.stream().map(Route::getMethod).collect(Collectors.toList()); + List expectedNames = expected.stream().map(NamedRoute::name).collect(Collectors.toList()); + List names = routes.stream().map(r -> ((NamedRoute) r).name()).collect(Collectors.toList()); + assertTrue(paths.containsAll(expectedPaths)); + assertTrue(expectedPaths.containsAll(paths)); + assertTrue(methods.containsAll(expectedMethods)); + assertTrue(expectedMethods.containsAll(methods)); + assertTrue(expectedNames.containsAll(names)); + } + + public void testRestSendToExtensionActionWithNamedRouteAndLegacyActionName() throws Exception { + RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( + "uniqueid1", + List.of( + "GET /foo foo cluster:admin/opensearch/abc/foo", + "PUT /bar bar cluster:admin/opensearch/jkl/bar,cluster:admin/opendistro/mno/bar*", + "POST /baz baz cluster:admin/opensearch/xyz/baz" + ), + List.of("GET /deprecated/foo foo_deprecated cluster:admin/opensearch/abc/foo_deprecated", "It's deprecated!") + ); + RestSendToExtensionAction restSendToExtensionAction = new RestSendToExtensionAction( + registerRestActionRequest, + discoveryExtensionNode, + transportService, + dynamicActionRegistry + ); + + assertEquals("send_to_extension_action", restSendToExtensionAction.getName()); + List expected = new ArrayList<>(); + String uriPrefix = "/_extensions/_uniqueid1"; + NamedRoute nr1 = new NamedRoute.Builder().method(Method.GET) + .path(uriPrefix + "/foo") + .uniqueName("foo") + .legacyActionNames(Set.of("cluster:admin/opensearch/abc/foo")) + .build(); + NamedRoute nr2 = new NamedRoute.Builder().method(Method.PUT) + .path(uriPrefix + "/bar") + .uniqueName("bar") + .legacyActionNames(Set.of("cluster:admin/opensearch/jkl/bar", "cluster:admin/opendistro/mno/bar*")) + .build(); + NamedRoute nr3 = new NamedRoute.Builder().method(Method.POST) + .path(uriPrefix + "/baz") + .uniqueName("baz") + .legacyActionNames(Set.of("cluster:admin/opensearch/xyz/baz")) + .build(); + + expected.add(nr1); + expected.add(nr2); + expected.add(nr3); List routes = restSendToExtensionAction.routes(); assertEquals(expected.size(), routes.size()); @@ -192,11 +253,26 @@ public void testRestSendToExtensionActionWithNamedRoute() throws Exception { List methods = routes.stream().map(Route::getMethod).collect(Collectors.toList()); List expectedNames = expected.stream().map(NamedRoute::name).collect(Collectors.toList()); List names = routes.stream().map(r -> ((NamedRoute) r).name()).collect(Collectors.toList()); + Set expectedActionNames = expected.stream().flatMap(nr -> nr.actionNames().stream()).collect(Collectors.toSet()); + Set actionNames = routes.stream().flatMap(nr -> ((NamedRoute) nr).actionNames().stream()).collect(Collectors.toSet()); assertTrue(paths.containsAll(expectedPaths)); assertTrue(expectedPaths.containsAll(paths)); assertTrue(methods.containsAll(expectedMethods)); assertTrue(expectedMethods.containsAll(methods)); assertTrue(expectedNames.containsAll(names)); + assertTrue(expectedActionNames.containsAll(actionNames)); + } + + public void testRestSendToExtensionActionWithoutUniqueNameShouldFail() { + RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( + "uniqueid1", + List.of("GET /foo", "PUT /bar"), + List.of() + ); + expectThrows( + IllegalArgumentException.class, + () -> new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, dynamicActionRegistry) + ); } public void testRestSendToExtensionMultipleNamedRoutesWithSameName() throws Exception { @@ -211,6 +287,18 @@ public void testRestSendToExtensionMultipleNamedRoutesWithSameName() throws Exce ); } + public void testRestSendToExtensionMultipleNamedRoutesWithSameLegacyActionName() throws Exception { + RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( + "uniqueid1", + List.of("GET /foo foo cluster:admin/opensearch/abc/foo", "PUT /bar bar cluster:admin/opensearch/abc/foo"), + List.of() + ); + expectThrows( + IllegalArgumentException.class, + () -> new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, dynamicActionRegistry) + ); + } + public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPath() throws Exception { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", @@ -226,7 +314,7 @@ 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("GET /foo/{path_param1} fooWithParam", "GET /foo/{path_param2} listFooWithParam"), List.of() ); expectThrows( @@ -235,12 +323,13 @@ public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithDiffer ); } - public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() throws Exception { + public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", - List.of("GET /foo/{path_param}", "GET /foo/{path_param}/list"), + List.of("GET /foo/{path_param} fooWithParam", "GET /foo/{path_param}/list listFooWithParam"), List.of() ); + try { new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, dynamicActionRegistry); } catch (IllegalArgumentException e) { @@ -285,8 +374,8 @@ public void testRestSendToExtensionWithNamedRouteCollidingWithNativeTransportAct public void testRestSendToExtensionActionFilterHeaders() throws Exception { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", - List.of("GET /foo", "PUT /bar", "POST /baz"), - List.of("GET /deprecated/foo", "It's deprecated!") + List.of("GET /foo foo", "PUT /bar bar", "POST /baz baz"), + List.of("GET /deprecated/foo foo_deprecated", "It's deprecated!") ); RestSendToExtensionAction restSendToExtensionAction = new RestSendToExtensionAction( registerRestActionRequest, diff --git a/server/src/test/java/org/opensearch/extensions/rest/RouteHandlerTests.java b/server/src/test/java/org/opensearch/extensions/rest/RouteHandlerTests.java deleted file mode 100644 index 855296b2038f0..0000000000000 --- a/server/src/test/java/org/opensearch/extensions/rest/RouteHandlerTests.java +++ /dev/null @@ -1,36 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.extensions.rest; - -import org.opensearch.rest.RestRequest; -import org.opensearch.rest.RestStatus; -import org.opensearch.test.OpenSearchTestCase; - -public class RouteHandlerTests extends OpenSearchTestCase { - public void testUnnamedRouteHandler() { - RouteHandler rh = new RouteHandler( - RestRequest.Method.GET, - "/foo/bar", - req -> new ExtensionRestResponse(req, RestStatus.OK, "content") - ); - - assertEquals(null, rh.name()); - } - - public void testNamedRouteHandler() { - RouteHandler rh = new RouteHandler( - "foo", - RestRequest.Method.GET, - "/foo/bar", - req -> new ExtensionRestResponse(req, RestStatus.OK, "content") - ); - - assertEquals("foo", rh.name()); - } -} diff --git a/server/src/test/java/org/opensearch/rest/NamedRouteTests.java b/server/src/test/java/org/opensearch/rest/NamedRouteTests.java index d489321ea5dc6..cf3e2b5b858bf 100644 --- a/server/src/test/java/org/opensearch/rest/NamedRouteTests.java +++ b/server/src/test/java/org/opensearch/rest/NamedRouteTests.java @@ -11,22 +11,17 @@ import org.opensearch.OpenSearchException; import org.opensearch.test.OpenSearchTestCase; +import java.util.Set; +import java.util.function.Function; + import static org.opensearch.rest.NamedRoute.MAX_LENGTH_OF_ACTION_NAME; +import static org.opensearch.rest.RestRequest.Method.GET; public class NamedRouteTests extends OpenSearchTestCase { - public void testNamedRouteWithNullName() { - try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", null); - fail("Expected NamedRoute to throw exception on null name provided"); - } catch (OpenSearchException e) { - assertTrue(e.getMessage().contains("Invalid route name specified")); - } - } - public void testNamedRouteWithEmptyName() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", ""); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("").build(); fail("Expected NamedRoute to throw exception on empty name provided"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -35,7 +30,7 @@ public void testNamedRouteWithEmptyName() { public void testNamedRouteWithNameContainingSpace() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo bar"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo bar").build(); fail("Expected NamedRoute to throw exception on name containing space name provided"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -44,7 +39,7 @@ public void testNamedRouteWithNameContainingSpace() { public void testNamedRouteWithNameContainingInvalidCharacters() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo@bar!"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo@bar!").build(); fail("Expected NamedRoute to throw exception on name containing invalid characters name provided"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -54,7 +49,7 @@ public void testNamedRouteWithNameContainingInvalidCharacters() { public void testNamedRouteWithNameOverMaximumLength() { try { String repeated = new String(new char[MAX_LENGTH_OF_ACTION_NAME + 1]).replace("\0", "x"); - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", repeated); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName(repeated).build(); fail("Expected NamedRoute to throw exception on name over maximum length supplied"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -63,7 +58,7 @@ public void testNamedRouteWithNameOverMaximumLength() { public void testNamedRouteWithValidActionName() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar").build(); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } @@ -71,7 +66,7 @@ public void testNamedRouteWithValidActionName() { public void testNamedRouteWithValidActionNameWithForwardSlash() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar/baz"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar:baz").build(); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } @@ -79,7 +74,7 @@ public void testNamedRouteWithValidActionNameWithForwardSlash() { public void testNamedRouteWithValidActionNameWithWildcard() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar/*"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar/*").build(); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } @@ -87,9 +82,82 @@ public void testNamedRouteWithValidActionNameWithWildcard() { public void testNamedRouteWithValidActionNameWithUnderscore() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar_baz"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar_baz").build(); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } } + + public void testNamedRouteWithNullLegacyActionNames() { + try { + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar").legacyActionNames(null).build(); + assertTrue(r.actionNames().isEmpty()); + } catch (OpenSearchException e) { + fail("Did not expect NamedRoute to pass with an invalid legacy action name"); + } + } + + public void testNamedRouteWithInvalidLegacyActionNames() { + try { + NamedRoute r = new NamedRoute.Builder().method(GET) + .path("foo/bar") + .uniqueName("foo:bar") + .legacyActionNames(Set.of("foo:bar-legacy")) + .build(); + fail("Did not expect NamedRoute to pass with an invalid legacy action name"); + } catch (OpenSearchException e) { + assertTrue(e.getMessage().contains("Invalid action name [foo:bar-legacy]. It must start with one of:")); + } + } + + public void testNamedRouteWithHandler() { + Function fooHandler = restRequest -> null; + try { + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar_baz").handler(fooHandler).build(); + assertEquals(r.handler(), fooHandler); + } catch (OpenSearchException e) { + fail("Did not expect NamedRoute to throw exception"); + } + } + + public void testNamedRouteNullChecks() { + try { + NamedRoute r = new NamedRoute.Builder().method(null).path("foo/bar").uniqueName("foo:bar_baz").build(); + fail("Expected NamedRoute to throw exception as method should not be null"); + } catch (NullPointerException e) { + assertEquals("REST method must not be null.", e.getMessage()); + } + + try { + NamedRoute r = new NamedRoute.Builder().method(GET).path(null).uniqueName("foo:bar_baz").build(); + fail("Expected NamedRoute to throw exception as path should not be null"); + } catch (NullPointerException e) { + assertEquals("REST path must not be null.", e.getMessage()); + } + + try { + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName(null).build(); + fail("Expected NamedRoute to throw exception as route name should not be null"); + } catch (NullPointerException e) { + assertEquals("REST route name must not be null.", e.getMessage()); + } + + try { + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar_baz").handler(null).build(); + fail("Expected NamedRoute to throw exception as handler should not be null"); + } catch (NullPointerException e) { + assertEquals("Route handler must not be null.", e.getMessage()); + } + } + + public void testNamedRouteEmptyBuild() { + try { + NamedRoute r = new NamedRoute.Builder().build(); + fail("Expected NamedRoute to throw exception as fields should not be null"); + } catch (IllegalStateException e) { + assertEquals("REST method, path and uniqueName are required.", e.getMessage()); + } + + } + }