Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] [Extensions] Not able to register Routes with similar semantics from RestActions #654

Closed
vibrantvarun opened this issue Apr 6, 2023 · 12 comments
Assignees
Labels
wontfix This will not be worked on

Comments

@vibrantvarun
Copy link
Member

vibrantvarun commented Apr 6, 2023

What is the bug?

The get detector has the following route.

/detectors/{detectorId}

return ImmutableList
            .of(
                // GET
                new RouteHandler(
                    RestRequest.Method.GET,
                    String.format(Locale.ROOT, "%s/{%s}", AnomalyDetectorExtension.AD_BASE_DETECTORS_URI, DETECTOR_ID),
                    handleRequest
                )
            );

The detector stats API requires following route to be registered.

/detectors/{nodeId}/stats

new RouteHandler(
                    RestRequest.Method.GET,
                    String.format(Locale.ROOT, "%s/{%s}/%s", AnomalyDetectorExtension.AD_BASE_DETECTORS_URI, NODE_ID, "stats"),
                    handleRequest
                ),

However, the routes for stats API cannot be registered due to the error mentioned below

Exception in thread "main" java.lang.IllegalArgumentException: Trying to use conflicting wildcard names for same path: detectorID and nodeId
        at org.opensearch.common.path.PathTrie$TrieNode.updateKeyWithNamedWildcard(PathTrie.java:131)
        at org.opensearch.common.path.PathTrie$TrieNode.insert(PathTrie.java:159)
        at org.opensearch.common.path.PathTrie$TrieNode.insert(PathTrie.java:176)
        at org.opensearch.common.path.PathTrie$TrieNode.insert(PathTrie.java:176)
        at org.opensearch.common.path.PathTrie.insert(PathTrie.java:329)
        at org.opensearch.sdk.ExtensionRestPathRegistry.registerHandler(ExtensionRestPathRegistry.java:38)
        at org.opensearch.sdk.ExtensionsRunner.<init>(ExtensionsRunner.java:247)
        at org.opensearch.sdk.ExtensionsRunner.run(ExtensionsRunner.java:744)
        at org.opensearch.ad.AnomalyDetectorExtension.main(AnomalyDetectorExtension.java:588)

How can one reproduce the bug?

We can reproduce the bug by trying to register any path which has

/detectors/{anyValue}

What is the expected behavior?

The extensions framework should allow registering the impacted routes

Do you have any additional context?

I think the problem is with using Trie data structure in registering paths. Boiling down it is not able to differentiate between the key of two paths.

@vibrantvarun vibrantvarun added bug Something isn't working untriaged labels Apr 6, 2023
@vibrantvarun vibrantvarun changed the title [BUG] Not able to register Routes with similar semantics [BUG] [Extensions] Not able to register Routes with similar semantics Apr 6, 2023
@vibrantvarun vibrantvarun changed the title [BUG] [Extensions] Not able to register Routes with similar semantics [BUG] [Extensions] Not able to register Routes with similar semantics in RestActions Apr 6, 2023
@vibrantvarun vibrantvarun changed the title [BUG] [Extensions] Not able to register Routes with similar semantics in RestActions [BUG] [Extensions] Not able to register Routes with similar semantics from RestActions Apr 6, 2023
@dbwiddis dbwiddis removed the untriaged label Apr 6, 2023
@dbwiddis
Copy link
Member

dbwiddis commented Apr 6, 2023

Root cause of the bug is the use of insert() rather than insertOrUpdate() in the PathTrie, which required tracking/updating a MethodHandlers object. By using the method (string) as part of the path name, this effectively worked around the need to update the methods... but breaks with different "same method different wildcard" names as shown.

Solution: create an SDK variant of MethodHandlers.

(Update: Nope, this doesn't fix anything. testRegisterSecondMethodWithDifferentNamedWildcard() in the RestController indicates this is an expected failure condition.)

@owaiskazi19
Copy link
Member

Facing the same issue for Search Detectors:

Exception in thread "main" java.lang.IllegalArgumentException: Path [POST /detectors/_search] already has a value [org.opensearch.ad.rest.RestSearchAnomalyDetectorAction@5f8f1712]
        at org.opensearch.common.path.PathTrie$TrieNode.insert(PathTrie.java:168)
        at org.opensearch.common.path.PathTrie$TrieNode.insert(PathTrie.java:176)
        at org.opensearch.common.path.PathTrie$TrieNode.insert(PathTrie.java:176)
        at org.opensearch.common.path.PathTrie.insert(PathTrie.java:329)
        at org.opensearch.sdk.ExtensionRestPathRegistry.registerHandler(ExtensionRestPathRegistry.java:38)
        at org.opensearch.sdk.ExtensionsRunner.<init>(ExtensionsRunner.java:247)
        at org.opensearch.sdk.ExtensionsRunner.run(ExtensionsRunner.java:744)
        at org.opensearch.ad.AnomalyDetectorExtension.main(AnomalyDetectorExtension.java:612)

@dbwiddis
Copy link
Member

dbwiddis commented Apr 6, 2023

Working on a fix and replicated the same way RestController does this but still running into this issue.

Temporary workaround: change your rest paths to detectors1 and detectors2 etc. while I sort this out. I don't see how this isn't already failing for plugins with those paths....

@owaiskazi19
Copy link
Member

I don't see how this isn't already failing for plugins with those paths....

Is it because plugin is using ReplacedRoutes for such cases? Do you think as a part of this fix we should add the same for extensions? It will also help for BWC testing.

@dbwiddis
Copy link
Member

dbwiddis commented Apr 6, 2023

Possibly. I just found testRegisterSecondMethodWithDifferentNamedWildcard() in RestControllerTests which fails on the exact case here so clearly whatever is being registered is not what I think it is.

@dbwiddis
Copy link
Member

dbwiddis commented Apr 6, 2023

Nope, the plugins are registering handlers via this code in RestController:

    public void registerHandler(final RestHandler restHandler) {
        restHandler.routes().forEach(route -> registerHandler(route.getMethod(), route.getPath(), restHandler));
        restHandler.deprecatedRoutes()
            .forEach(route -> registerAsDeprecatedHandler(route.getMethod(), route.getPath(), restHandler, route.getDeprecationMessage()));
        restHandler.replacedRoutes()
            .forEach(
                route -> registerWithDeprecatedHandler(
                    route.getMethod(),
                    route.getPath(),
                    restHandler,
                    route.getDeprecatedMethod(),
                    route.getDeprecatedPath()
                )
            );
    }

That first line restHandler.routes().forEach(route -> registerHandler(route.getMethod(), route.getPath(), restHandler)); is the portion we're implementing. It calls:

    protected void registerHandler(RestRequest.Method method, String path, RestHandler handler) {
        if (handler instanceof BaseRestHandler) {
            usageService.addRestHandler((BaseRestHandler) handler);
        }
        registerHandlerNoWrap(method, path, handlerWrapper.apply(handler));
    }

Which then calls:

    private void registerHandlerNoWrap(RestRequest.Method method, String path, RestHandler maybeWrappedHandler) {
        handlers.insertOrUpdate(
            path,
            new MethodHandlers(path, maybeWrappedHandler, method),
            (mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, method)
        );
    }

I have exactly replicated this last method and it fails, and looks like it is expected to fail.

@owaiskazi19
Copy link
Member

owaiskazi19 commented Apr 6, 2023

I see. For Search, there are 3 RestActions extending the same AbstractSearchAction and registering the routes like this and this.
The urlPath and deprecatedPaths are defined here and here which are passed in super.
Since AbstractSearchAction has route and ReplacedRoute this is not an issue with Plugin. In extensions for now we are supporting just routes because of which we can't register the APIs with same path.

@dbwiddis
Copy link
Member

dbwiddis commented Apr 6, 2023

The issue you're facing in Search (already has a value) is different than the wildcard issue (conflicting wildcards).

@dbwiddis
Copy link
Member

dbwiddis commented Apr 7, 2023

OK, so this does have to do with ReplacedRoutes.

The path indicated for Get Detector in the initial post here doesn't match the plugin.

/detectors/{detectorId}

return ImmutableList
            .of(
                // GET
                new RouteHandler(
                    RestRequest.Method.GET,
                    String.format(Locale.ROOT, "%s/{%s}", AnomalyDetectorExtension.AD_BASE_DETECTORS_URI, DETECTOR_ID),
                    handleRequest
                )
            );

But plugin has:

    @Override
    public List<Route> routes() {
        return ImmutableList
            .of(
                // Opensearch-only API. Considering users may provide entity in the search body, support POST as well.
                new Route(
                    RestRequest.Method.POST,
                    String.format(Locale.ROOT, "%s/{%s}/%s", AnomalyDetectorPlugin.AD_BASE_DETECTORS_URI, DETECTOR_ID, PROFILE)
                ),
                new Route(
                    RestRequest.Method.POST,
                    String.format(Locale.ROOT, "%s/{%s}/%s/{%s}", AnomalyDetectorPlugin.AD_BASE_DETECTORS_URI, DETECTOR_ID, PROFILE, TYPE)
                )
            );
    }

The shorter URL is the replaced routes "alias" for it.

@dbwiddis
Copy link
Member

dbwiddis commented Apr 7, 2023

So, wrapping this up:

  1. I don't think there's a bug in how we register routes. I still might just update the code I already wrote to conform exactly to how the RestController does it anyway, just to be sure, but it won't fix this issue.
  2. We should change the routes we have for AD to match the actual routes() return value. That will near-term solve this bug, even if we have to use longer paths during our testing/validation in the near term.
  3. We should implement ReplacedRoutes extension point. I think there's an issue for that already. If not, we'll add it.

@dbwiddis
Copy link
Member

dbwiddis commented Apr 9, 2023

So deprecated and replaced routes just register additional routes and don't fix this (perceived) problem. It's good to add them but they aren't a bug fix. Because there's no bug. :)

The detector stats API requires following route to be registered.

/detectors/{nodeId}/stats

new RouteHandler(
                    RestRequest.Method.GET,
                    String.format(Locale.ROOT, "%s/{%s}/%s", AnomalyDetectorExtension.AD_BASE_DETECTORS_URI, NODE_ID, "stats"),
                    handleRequest
                ),

Stats does not use AD_BASE_DETECTORS_URI, it uses AD_BASE_URI.

The route is not /detectors/{nodeId}/stats, it is /{nodeId}/stats.

@dbwiddis
Copy link
Member

dbwiddis commented Apr 9, 2023

Facing the same issue for Search Detectors:

Exception in thread "main" java.lang.IllegalArgumentException: Path [POST /detectors/_search] already has a value [org.opensearch.ad.rest.RestSearchAnomalyDetectorAction@5f8f1712]

Hardcoded paths (_search) take precedence over wildcards, and don't conflict, so this is not a problem with the registry. It may be that you have the path registered in a superclass and are inheriting it in multiple subclasses?

@dbwiddis dbwiddis added wontfix This will not be worked on and removed bug Something isn't working labels Apr 9, 2023
@dbwiddis dbwiddis closed this as completed May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants