[2.9] Revert "Support transport action names when registering NamedRoutes (#7957) (#8459)"

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

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<NamedRoute, RestSendToExtensionAction> routeRegistry = new ConcurrentHashMap<>();
+        private final Map<RestHandler.Route, RestSendToExtensionAction> routeRegistry = new ConcurrentHashMap<>();
 
         private final Set<String> registeredActionNames = new ConcurrentSkipListSet<>();
 
@@ -1112,52 +1112,41 @@ 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<String> actionNames = route.actionNames();
-            if (!Collections.disjoint(actionNames, registeredActionNames)) {
-                Set<String> alreadyRegistered = new HashSet<>(registeredActionNames);
-                alreadyRegistered.retainAll(actionNames);
-                String acts = String.join(", ", alreadyRegistered);
-                throw new IllegalArgumentException(
-                    "action" + (alreadyRegistered.size() > 1 ? "s [" : " [") + acts + "] already registered"
-                );
+            Optional<String> 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);
         }
 
         /**
          * Remove a dynamic route from the registry.
          *
          * @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());
+            }
         }
 
         /**

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<Route> 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));
             }
         };
+
     }
 }

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<Route> restActionsAsRoutes = new ArrayList<>();
         for (String restAction : restActionsRequest.getRestActions()) {
-
-            // TODO Find a better way to parse these to avoid code-smells
-
-            String name;
-            Set<String> actionNames = new HashSet<>();
+            Optional<String> 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<DeprecatedRoute> restActionsAsDeprecatedRoutes = new ArrayList<>();
         // Iterate in pairs of route / deprecation message
         List<String> deprecatedActions = restActionsRequest.getDeprecatedRestActions();

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<RestRequest, ExtensionRestResponse> 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<RestRequest, ExtensionRestResponse> 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<RestRequest, ExtensionRestResponse> 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;
+    }
+}