Skip to content

Commit

Permalink
Add RouteRegistry to DynamicActionModule
Browse files Browse the repository at this point in the history 
Signed-off-by: Craig Perkins <cwperx@amazon.com>
  • Loading branch information
@cwperks
cwperks committed May 18, 2023
1 parent 5a9c363 commit 9ca3c90
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 45 deletions.
50 changes: 11 additions & 39 deletions server/src/main/java/org/opensearch/action/ActionModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,8 +498,6 @@ public class ActionModule extends AbstractModule {
// associated with remote action execution on extensions, possibly in
// a different JVM and possibly on a different server.
private final DynamicActionRegistry dynamicActionRegistry;
// A dynamic route registry associated with registered extension routes
private final DynamicRouteRegistry dynamicRouteRegistry;
private final ActionFilters actionFilters;
private final AutoCreateIndex autoCreateIndex;
private final DestructiveOperations destructiveOperations;
Expand Down Expand Up @@ -532,7 +530,6 @@ public ActionModule(
actions = setupActions(actionPlugins);
actionFilters = setupActionFilters(actionPlugins);
dynamicActionRegistry = new DynamicActionRegistry();
dynamicRouteRegistry = new DynamicRouteRegistry(dynamicActionRegistry);
autoCreateIndex = new AutoCreateIndex(settings, clusterSettings, indexNameExpressionResolver, systemIndices);
destructiveOperations = new DestructiveOperations(settings, clusterSettings);
Set<RestHeaderDefinition> headers = Stream.concat(
Expand Down Expand Up @@ -1010,10 +1007,6 @@ public DynamicActionRegistry getDynamicActionRegistry() {
return dynamicActionRegistry;
}

public DynamicRouteRegistry getDynamicRouteRegistry() {
return dynamicRouteRegistry;
}

public RestController getRestController() {
return restController;
}
Expand All @@ -1034,6 +1027,10 @@ public static class DynamicActionRegistry {
// at times other than node bootstrap.
private final Map<ActionType<?>, TransportAction<?, ?>> registry = new ConcurrentHashMap<>();

// 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 Set<String> registeredActionNames = new ConcurrentSkipListSet<>();

/**
Expand Down Expand Up @@ -1098,31 +1095,6 @@ public boolean isActionRegistered(String actionName) {
}
return registry.get(action);
}
}

/**
* The DynamicRouteRegistry maintains a registry mapping {@link org.opensearch.rest.RestHandler.Route} instances to {@link org.opensearch.rest.extensions.RestSendToExtensionAction} instances.
* <p>
* This class is modeled after {@link NamedRegistry} but provides both register and unregister capabilities.
*
* @opensearch.internal
*/
public class DynamicRouteRegistry {
// This is an instance of a DynamicActionRegistry containing all registered transport actions
private DynamicActionRegistry dynamicActionRegistry;
// A dynamic registry to add or remove Route / RestSendToExtensionAction pairs
// at times other than node bootstrap.
private final Map<RestHandler.Route, RestSendToExtensionAction> registry = new ConcurrentHashMap<>();

private final Set<String> registeredRestActionNames = new ConcurrentSkipListSet<>();

/**
*
* @param dynamicActionRegistry A registry of all transport actions - native, plugins and dynamic
*/
public DynamicRouteRegistry(DynamicActionRegistry dynamicActionRegistry) {
this.dynamicActionRegistry = dynamicActionRegistry;
}

/**
* Add a dynamic action to the registry.
Expand All @@ -1136,15 +1108,15 @@ public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAct
Optional<String> routeName = Optional.empty();
if (route instanceof NamedRoute) {
routeName = Optional.of(((NamedRoute) route).name());
if (dynamicActionRegistry.isActionRegistered(routeName.get()) || registeredRestActionNames.contains(routeName.get())) {
if (isActionRegistered(routeName.get()) || registeredActionNames.contains(routeName.get())) {
throw new IllegalArgumentException("route [" + route + "] already registered");
}
}
if (registry.containsKey(route)) {
if (routeRegistry.containsKey(route)) {
throw new IllegalArgumentException("route [" + route + "] already registered");
}
registry.put(route, action);
routeName.ifPresent(registeredRestActionNames::add);
routeRegistry.put(route, action);
routeName.ifPresent(registeredActionNames::add);
}

/**
Expand All @@ -1154,11 +1126,11 @@ public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAct
*/
public void unregisterDynamicRoute(RestHandler.Route route) {
requireNonNull(route, "route is required");
if (registry.remove(route) == null) {
if (routeRegistry.remove(route) == null) {
throw new IllegalArgumentException("action [" + route + "] was not registered");
}
if (route instanceof NamedRoute) {
registeredRestActionNames.remove(((NamedRoute) route).name());
registeredActionNames.remove(((NamedRoute) route).name());
}
}

Expand All @@ -1170,7 +1142,7 @@ public void unregisterDynamicRoute(RestHandler.Route route) {
*/
@SuppressWarnings("unchecked")
public RestSendToExtensionAction get(RestHandler.Route route) {
return registry.get(route);
return routeRegistry.get(route);
}
}
}
10 changes: 8 additions & 2 deletions server/src/main/java/org/opensearch/rest/RestHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,18 @@ public String getPath() {
return path;
}

public String getPathWithPathParamsReplaced() {
return path.replaceAll("(?<=\\{).*?(?=\\})", "path_param");
}

public Method getMethod() {
return method;
}

@Override
public int hashCode() {
return toString().hashCode();
String routeStr = "Route [method=" + method + ", path=" + getPathWithPathParamsReplaced() + "]";
return routeStr.hashCode();
}

@Override
Expand All @@ -219,7 +224,8 @@ public boolean equals(Object o) {
return false;
}
Route that = (Route) o;
return Objects.equals(method, that.method) && Objects.equals(path, that.path);
return Objects.equals(method, that.method)
&& Objects.equals(getPathWithPathParamsReplaced(), that.getPathWithPathParamsReplaced());
}
}

Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/opensearch/rest/extensions/RestSendToExtensionAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ public RestSendToExtensionAction(
if (name.isPresent()) {
NamedRoute nr = new NamedRoute(method, path, name.get());
restActionsAsRoutes.add(nr);
actionModule.getDynamicRouteRegistry().registerDynamicRoute(nr, this);
actionModule.getDynamicActionRegistry().registerDynamicRoute(nr, this);
} else {
Route r = new Route(method, path);
restActionsAsRoutes.add(r);
actionModule.getDynamicRouteRegistry().registerDynamicRoute(r, this);
actionModule.getDynamicActionRegistry().registerDynamicRoute(r, this);
}
}
this.routes = unmodifiableList(restActionsAsRoutes);
Expand Down
4 changes: 2 additions & 2 deletions server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
import org.junit.Before;
import org.opensearch.Version;
import org.opensearch.action.ActionModule;
import org.opensearch.action.ActionModule.DynamicRouteRegistry;
import org.opensearch.action.ActionModule.DynamicActionRegistry;
import org.opensearch.action.admin.cluster.state.ClusterStateResponse;
import org.opensearch.client.node.NodeClient;
import org.opensearch.cluster.ClusterSettingsResponse;
Expand Down Expand Up @@ -169,7 +169,7 @@ public void setup() throws Exception {
new UsageService(),
new IdentityService(Settings.EMPTY, List.of())
);
when(actionModule.getDynamicRouteRegistry()).thenReturn(mock(DynamicRouteRegistry.class));
when(actionModule.getDynamicActionRegistry()).thenReturn(mock(DynamicActionRegistry.class));
when(actionModule.getRestController()).thenReturn(restController);
settingsModule = new SettingsModule(Settings.EMPTY, emptyList(), emptyList(), emptySet());
clusterService = createClusterService(threadPool);
Expand Down
25 changes: 25 additions & 0 deletions server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,31 @@ public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPath() throws
);
}

public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithDifferentPathParams() throws Exception {
RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest(
"uniqueid1",
List.of("GET /foo/{path_param1}", "GET /foo/{path_param2}"),
List.of()
);
expectThrows(
IllegalArgumentException.class,
() -> new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, actionModule)
);
}

public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() throws Exception {
RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest(
"uniqueid1",
List.of("GET /foo/{path_param}", "GET /foo/{path_param}/list"),
List.of()
);
try {
new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, actionModule);
} catch (IllegalArgumentException e) {
fail("IllegalArgumentException should not be thrown for different paths");
}
}

public void testRestSendToExtensionWithNamedRouteCollidingWithDynamicTransportAction() throws Exception {
DynamicActionRegistry dynamicActionRegistry = actionModule.getDynamicActionRegistry();
ActionFilters emptyFilters = new ActionFilters(Collections.emptySet());
Expand Down

0 comments on commit 9ca3c90

Please sign in to comment.