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

Add methods to RestController to test Rest Paths and unregister them #6395

Open
dbwiddis opened this issue Feb 21, 2023 · 1 comment
Open
Labels
enhancement Enhancement or improvement to existing feature or request extensions

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Feb 21, 2023

Is your feature request related to a problem? Please describe.

One of the goals of extensibility is the ability to add/update/remove an extension without restarting the OpenSearch node.

From #2447:

What would the customer like to see/use:
c. Install/Update/Remove an extension without restarting OpenSearch.

Extensions provide their own REST API for methods/paths that they handle. Presently, the RestController only allows registering paths. There is no mechanism to:

  • Test whether a method+path exists. The method for checking paths (handleNoHandlerFound()) is private and the lower-level getAllHandlers() method it uses is package private.
    • This impacts the ability to unit test code which adds these methods as it can only be tested via IT of the resulting API
  • Unregister a path. One can overwrite an implementation with a new one, but not remove it.
    • This gets more complicated with different results when the path is handled but the method is not.

Describe the solution you'd like

  1. Provide a wrapper method that internally calls handleNoHandlerFound() and exposes information via a Java method rather than simply failing a rest request.
  2. Provide a means to remove a registered/handled rest path.

TLDR:

  • registerHandler(…) exists
  • I want isHandlerRegistered(…) and unregisterHandler(…) methods

Describe alternatives you've considered

  1. Test existence:

    • Create a method to query the path via the REST API (dispatchRequest() is the first public method that accesses this result) and parse the RestResponse. This would work but seems very inefficient compared to calling an existing (private) method.
    • Change the visibility of handleNoHandlerFound() to public, or at least package private with the calling method in the same package. This method isn't named very well for a public API method, though.
    • Create a new class in the same package to call the lower level getAllHandlers() method. This is essentially duplicating the code of handleNoHandlerFound().
  2. Unregister a path:

    • Create an action which emulates the response of the "no handler found" code and "overwrite" the rest path to use this handler. Not unlike a custom 404 page saying "This page used to exist but it doesn't any more." This could provide misleading results to OPTIONS queries that indicated a method exists for a path, when it doesn't really.

Additional context

Part of SDK #360 / Meta issue SDK #356

@dblock
Copy link
Member

dblock commented Mar 6, 2024

Does #11876 begin solving your problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or improvement to existing feature or request extensions
Projects
None yet
Development

No branches or pull requests

4 participants