diff --git a/CHANGELOG.md b/CHANGELOG.md index 832878a6d7526..5a049f1d245fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Search pipelines] Add Global Ignore_failure options for Processors ([#8373](https://github.com/opensearch-project/OpenSearch/pull/8373)) - Enable Partial Flat Object ([#7997](https://github.com/opensearch-project/OpenSearch/pull/7997)) - Add partial results support for concurrent segment search ([#8306](https://github.com/opensearch-project/OpenSearch/pull/8306)) -- Support transport action names when registering NamedRoutes ([#7957](https://github.com/opensearch-project/OpenSearch/pull/7957)) - Introduce full support for Search Pipeline ([#8613](https://github.com/opensearch-project/OpenSearch/pull/8613)) ### Dependencies diff --git a/server/src/main/java/org/opensearch/action/ActionModule.java b/server/src/main/java/org/opensearch/action/ActionModule.java index 0f036bb80c4c2..4ffea3e8c8769 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,37 +1112,26 @@ public boolean isActionRegistered(String actionName) { } /** - * Adds a dynamic route to the registry. + * Add a dynamic action to the registry. * * @param route The route instance to add * @param action The corresponding instance of RestSendToExtensionAction to execute */ - public void registerDynamicRoute(NamedRoute route, RestSendToExtensionAction action) { + public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAction action) { requireNonNull(route, "route is required"); requireNonNull(action, "action is required"); - - 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" - ); + 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"); + } } - if (routeRegistry.containsKey(route)) { throw new IllegalArgumentException("route [" + route + "] already registered"); } routeRegistry.put(route, action); - registeredActionNames.add(routeName); - registeredActionNames.addAll(actionNames); + routeName.ifPresent(registeredActionNames::add); } /** @@ -1150,14 +1139,14 @@ public void registerDynamicRoute(NamedRoute route, RestSendToExtensionAction act * * @param route The route to remove */ - public void unregisterDynamicRoute(NamedRoute route) { + public void unregisterDynamicRoute(RestHandler.Route route) { requireNonNull(route, "route is required"); if (routeRegistry.remove(route) == null) { throw new IllegalArgumentException("action [" + route + "] was not registered"); } - - registeredActionNames.remove(route.name()); - registeredActionNames.removeAll(route.actionNames()); + if (route instanceof NamedRoute) { + registeredActionNames.remove(((NamedRoute) route).name()); + } } /** 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 878673b77a4a9..f47f342617732 100644 --- a/server/src/main/java/org/opensearch/extensions/rest/RestInitializeExtensionAction.java +++ b/server/src/main/java/org/opensearch/extensions/rest/RestInitializeExtensionAction.java @@ -22,7 +22,6 @@ 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; @@ -55,7 +54,7 @@ public String getName() { @Override public List routes() { - return List.of(new NamedRoute.Builder().method(POST).path("/_extensions/initialize").uniqueName("extensions:initialize").build()); + return List.of(new Route(POST, "/_extensions/initialize")); } public RestInitializeExtensionAction(ExtensionsManager extensionsManager) { @@ -188,5 +187,6 @@ 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 3dd6056bb36cf..073b3f3f45818 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,43 +90,33 @@ public RestSendToExtensionAction( List restActionsAsRoutes = new ArrayList<>(); for (String restAction : restActionsRequest.getRestActions()) { - - // TODO Find a better way to parse these to avoid code-smells - - String name; - Set actionNames = new HashSet<>(); + Optional name = Optional.empty(); String[] parts = restAction.split(" "); - if (parts.length < 3) { - throw new IllegalArgumentException("REST action must contain at least a REST method, a route and a unique name"); + if (parts.length < 2) { + throw new IllegalArgumentException("REST action must contain at least a REST method and route"); } try { method = RestRequest.Method.valueOf(parts[0].trim()); path = pathPrefix + parts[1].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); - } - } + if (parts.length > 2) { + name = Optional.of(parts[2].trim()); } } catch (IndexOutOfBoundsException | IllegalArgumentException e) { throw new IllegalArgumentException(restAction + " does not begin with a valid REST method"); } - 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); + 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); + } } 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 new file mode 100644 index 0000000000000..189d67c120189 --- /dev/null +++ b/server/src/main/java/org/opensearch/extensions/rest/RouteHandler.java @@ -0,0 +1,69 @@ +/* + * 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 109f688a4924e..f5eaafcd04056 100644 --- a/server/src/main/java/org/opensearch/rest/NamedRoute.java +++ b/server/src/main/java/org/opensearch/rest/NamedRoute.java @@ -9,13 +9,6 @@ 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 @@ -23,123 +16,21 @@ * @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 uniqueName; - private final Set actionNames; - - 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); - } + private final String name; - /** - * 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; + public boolean isValidRouteName(String routeName) { + if (routeName == null || routeName.isBlank() || routeName.length() > MAX_LENGTH_OF_ACTION_NAME) { + return false; } - + return routeName.matches(VALID_ACTION_NAME_PATTERN); } - private NamedRoute(Builder builder) { - super(builder.method, builder.path); - if (!isValidRouteName(builder.uniqueName)) { + public NamedRoute(RestRequest.Method method, String path, String name) { + super(method, path); + if (!isValidRouteName(name)) { 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 " @@ -147,43 +38,18 @@ private NamedRoute(Builder builder) { + " characters" ); } - 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); + this.name = name; } /** * The name of the Route. Must be unique across Route. */ public String 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; + return this.name; } @Override public String toString() { - return "NamedRoute [method=" + method + ", path=" + path + ", name=" + uniqueName + ", actionNames=" + actionNames + "]"; + return "NamedRoute [method=" + method + ", path=" + path + ", name=" + name + "]"; } } diff --git a/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java b/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java index 17e424ee81e7e..963d47df3baff 100644 --- a/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java +++ b/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java @@ -26,7 +26,6 @@ import java.io.IOException; import java.util.Collections; import java.util.Map; -import java.util.Set; import static org.mockito.Mockito.mock; @@ -81,8 +80,8 @@ public void testDynamicActionRegistry() { public void testDynamicActionRegistryWithNamedRoutes() { RestSendToExtensionAction action = mock(RestSendToExtensionAction.class); RestSendToExtensionAction action2 = mock(RestSendToExtensionAction.class); - 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(); + NamedRoute r1 = new NamedRoute(RestRequest.Method.GET, "/foo", "foo"); + NamedRoute r2 = new NamedRoute(RestRequest.Method.GET, "/bar", "bar"); DynamicActionRegistry registry = new DynamicActionRegistry(); registry.registerDynamicRoute(r1, action); @@ -90,38 +89,22 @@ public void testDynamicActionRegistryWithNamedRoutes() { assertTrue(registry.isActionRegistered("foo")); assertTrue(registry.isActionRegistered("bar")); - - registry.unregisterDynamicRoute(r2); - - assertTrue(registry.isActionRegistered("foo")); - assertFalse(registry.isActionRegistered("bar")); } - public void testDynamicActionRegistryWithNamedRoutesAndLegacyActionNames() { + public void testDynamicActionRegistryRegisterAndUnregisterWithNamedRoutes() { RestSendToExtensionAction action = mock(RestSendToExtensionAction.class); RestSendToExtensionAction action2 = mock(RestSendToExtensionAction.class); - 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(); + NamedRoute r1 = new NamedRoute(RestRequest.Method.GET, "/foo", "foo"); + NamedRoute r2 = new NamedRoute(RestRequest.Method.GET, "/bar", "bar"); 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("cluster:admin/opensearch/abc/foo")); - assertFalse(registry.isActionRegistered("cluster:admin/opensearch/xyz/bar")); + assertTrue(registry.isActionRegistered("foo")); + assertFalse(registry.isActionRegistered("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 4817f791fc9c9..32967a9de3c7f 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -445,8 +445,8 @@ public void testHandleRegisterRestActionsRequest() throws Exception { initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; - 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!"); + List actionsList = List.of("GET /foo", "PUT /bar", "POST /baz"); + List deprecatedActionsList = List.of("GET /deprecated/foo", "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 c74a8c9e8d863..02a1407716573 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 foo", "PUT /bar bar", "POST /baz baz"), - List.of("GET /deprecated/foo foo_deprecated", "Its deprecated") + List.of("GET /foo", "PUT /bar", "POST /baz"), + List.of("GET /deprecated/foo", "It's deprecated!") ); RestSendToExtensionAction restSendToExtensionAction = new RestSendToExtensionAction( registerRestActionRequest, @@ -180,70 +180,9 @@ public void testRestSendToExtensionActionWithNamedRoute() throws Exception { 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").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); + 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")); List routes = restSendToExtensionAction.routes(); assertEquals(expected.size(), routes.size()); @@ -253,26 +192,11 @@ public void testRestSendToExtensionActionWithNamedRouteAndLegacyActionName() thr 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 { @@ -287,18 +211,6 @@ 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", @@ -314,7 +226,7 @@ public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPath() throws public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithDifferentPathParams() throws Exception { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", - List.of("GET /foo/{path_param1} fooWithParam", "GET /foo/{path_param2} listFooWithParam"), + List.of("GET /foo/{path_param1}", "GET /foo/{path_param2}"), List.of() ); expectThrows( @@ -323,13 +235,12 @@ public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithDiffer ); } - public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() { + public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() throws Exception { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", - List.of("GET /foo/{path_param} fooWithParam", "GET /foo/{path_param}/list listFooWithParam"), + List.of("GET /foo/{path_param}", "GET /foo/{path_param}/list"), List.of() ); - try { new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, dynamicActionRegistry); } catch (IllegalArgumentException e) { @@ -374,8 +285,8 @@ public void testRestSendToExtensionWithNamedRouteCollidingWithNativeTransportAct public void testRestSendToExtensionActionFilterHeaders() throws Exception { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", - List.of("GET /foo foo", "PUT /bar bar", "POST /baz baz"), - List.of("GET /deprecated/foo foo_deprecated", "It's deprecated!") + List.of("GET /foo", "PUT /bar", "POST /baz"), + List.of("GET /deprecated/foo", "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 new file mode 100644 index 0000000000000..855296b2038f0 --- /dev/null +++ b/server/src/test/java/org/opensearch/extensions/rest/RouteHandlerTests.java @@ -0,0 +1,36 @@ +/* + * 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 cf3e2b5b858bf..d489321ea5dc6 100644 --- a/server/src/test/java/org/opensearch/rest/NamedRouteTests.java +++ b/server/src/test/java/org/opensearch/rest/NamedRouteTests.java @@ -11,17 +11,22 @@ 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.Builder().method(GET).path("foo/bar").uniqueName("").build(); + NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", ""); fail("Expected NamedRoute to throw exception on empty name provided"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -30,7 +35,7 @@ public void testNamedRouteWithEmptyName() { public void testNamedRouteWithNameContainingSpace() { try { - NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo bar").build(); + NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo bar"); fail("Expected NamedRoute to throw exception on name containing space name provided"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -39,7 +44,7 @@ public void testNamedRouteWithNameContainingSpace() { public void testNamedRouteWithNameContainingInvalidCharacters() { try { - NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo@bar!").build(); + NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo@bar!"); fail("Expected NamedRoute to throw exception on name containing invalid characters name provided"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -49,7 +54,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.Builder().method(GET).path("foo/bar").uniqueName(repeated).build(); + NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", repeated); fail("Expected NamedRoute to throw exception on name over maximum length supplied"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -58,7 +63,7 @@ public void testNamedRouteWithNameOverMaximumLength() { public void testNamedRouteWithValidActionName() { try { - NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar").build(); + NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar"); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } @@ -66,7 +71,7 @@ public void testNamedRouteWithValidActionName() { public void testNamedRouteWithValidActionNameWithForwardSlash() { try { - NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar:baz").build(); + NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar/baz"); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } @@ -74,7 +79,7 @@ public void testNamedRouteWithValidActionNameWithForwardSlash() { public void testNamedRouteWithValidActionNameWithWildcard() { try { - NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar/*").build(); + NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar/*"); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } @@ -82,82 +87,9 @@ public void testNamedRouteWithValidActionNameWithWildcard() { public void testNamedRouteWithValidActionNameWithUnderscore() { try { - NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar_baz").build(); + NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar_baz"); } 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()); - } - - } - }