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

[FEATURE] Provide a higher level implementation for REST handlers #128

Closed
dblock opened this issue Sep 7, 2022 · 3 comments · Fixed by #246
Closed

[FEATURE] Provide a higher level implementation for REST handlers #128

dblock opened this issue Sep 7, 2022 · 3 comments · Fixed by #246
Assignees
Labels
enhancement New feature or request

Comments

@dblock
Copy link
Member

dblock commented Sep 7, 2022

Is your feature request related to a problem?

In https:/opensearch-project/opensearch-sdk-java/blob/main/src/main/java/org/opensearch/sdk/sample/rest/RestHelloAction.java#L37 the developer has to implement a handleRequest that acts as a router that checks for methods, e.g.

if (Method.GET.equals(method) && "/hello".equals(uri)) {
            return new BytesRestResponse(OK, GREETING);
        }

This is unnecessarily verbose.

What solution would you like?

Provide a higher level implementation that implements ExtensionRestHandler . As a developer I would like to write something shorter:

public class RestHelloAction extends GenericExtensionRestHandler {

    @Override
    public List<Route> routes() {
        return singletonList(
            new RouteHandler(GET, "/hello", this.handleHello),
            new RouteHandler(GET, "/world", this.handleWorld)
        );
    }

    public RestResponse handleHello(...) {
    }

    public RestResponse handleWorld(...) {
    }
}

Alternatives?

There might be an HTTP handler library that provides routing out of the box that we could optionally plug in, e.g. https:/undertow-io/undertow like so.

@dblock dblock added enhancement New feature or request untriaged labels Sep 7, 2022
@dbwiddis
Copy link
Member

dbwiddis commented Sep 7, 2022

new RouteHandler(GET, "/hello", this.handleHello)

I can see some of the benefits here, where we would store a functional interface in the registry rather than a class. However, I'm not sure there's much added benefit vs. just splitting out the two routes into two classes (RestHelloAction and RestWorldAction) each with their own handleRequest() implementation.

The "unnecessarily verbose" check is really just (possibly unnecessary?) defensive programming against a "this should never happen" condition.

I definitely think we should at least keep the existing implementation as it is a direct corollary to the prepareRequest() method on plugins and should ease plugin-to-extension migration.

The big difference in your proposal vs. just splitting out classes is in mapping methods to routes, and I think a class like your proposed GenericExtensionRestHandler could enable that optional behavior.

So I agree this is a "nice to have" functionality but don't think I should change the existing interfaces.

@dbwiddis
Copy link
Member

dbwiddis commented Sep 8, 2022

Speaking of libraries, might be interesting to try to integrate with a JAX-RS (JSR-311) library to use annotations. I wonder if these also help in generating OpenAPI specs?

@Path("/hello")  
public class HelloService{  
    @GET  
    @Path("/{param}")  
    public Response getMsg(@PathParam("param") String msg) {  
        String output = "Jersey say : " + msg;  
        return Response.status(200).entity(output).build();  
    }  
}  

@dbwiddis
Copy link
Member

dbwiddis commented Nov 13, 2022

Two ways to go about this syntax:

new RouteHandler(GET, "/hello", this.handleHello)

Function<ExtensionRestRequest, ExtensionRestResponse> handleHello = (request) -> { ... };

Or, alternately

new RouteHandler(GET, "/hello", r -> this.handleHello(r))

ExtensionRestResponse handleHello(ExtensionRestRequest request) { ... }

Or one could combine them like so, but I think it negates the point of brevity. :-)

new RouteHandler(GET, "/hello", this.handleHello)

Function<ExtensionRestRequest, ExtensionRestResponse> handleHello = (r) -> { return handleHello(r); };

ExtensionRestResponse handleHello(ExtensionRestRequest request) { ... }

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

Successfully merging a pull request may close this issue.

2 participants