Support transport action names when registering NamedRoutes (opensear…

…ch-project#7957) * Adds ability to register legacy action names along with route Signed-off-by: Darshit Chanpura <[email protected]> Signed-off-by: Darshit Chanpura <[email protected]>
baba-devv · Jul 29, 2023 · 22a1e4e · 22a1e4e
1 parent 49bfd2b
commit 22a1e4e
Show file tree

Hide file tree

Showing 11 changed files with 410 additions and 185 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -99,6 +99,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)

diff --git 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<RestHandler.Route, RestSendToExtensionAction> routeRegistry = new ConcurrentHashMap<>();
+        private final Map<NamedRoute, RestSendToExtensionAction> routeRegistry = new ConcurrentHashMap<>();
 
         private final Set<String> registeredActionNames = new ConcurrentSkipListSet<>();
 
@@ -1112,41 +1112,52 @@ 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<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");
-                }
+
+            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"
+                );
+            }
+
             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);
         }
 
         /**
          * Remove a dynamic route from the registry.
          *
          * @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
@@ -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<Route> 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
@@ -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<Route> restActionsAsRoutes = new ArrayList<>();
         for (String restAction : restActionsRequest.getRestActions()) {
-            Optional<String> name = Optional.empty();
+
+            // TODO Find a better way to parse these to avoid code-smells
+
+            String name;
+            Set<String> 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<DeprecatedRoute> restActionsAsDeprecatedRoutes = new ArrayList<>();
         // Iterate in pairs of route / deprecation message
         List<String> 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