avaje / avaje-http

Controller generation for Javalin, Helidon SE.
https://avaje.io/http/
Apache License 2.0
67 stars 12 forks source link

Can we put the Controller method object into the Context in javalin-generator? #465

Closed ponyjoy closed 2 months ago

ponyjoy commented 2 months ago

In Javalin, both before and after handlers can filter user requests. However, if we are using Controller methods, if we could write the Controller method object into the Context, we would be able to retrieve the Controller method object in the after and before code blocks via the Context. This would allow us to obtain annotations added before the method or other useful information.

I'm wondering if it's possible to add the following similar code in the route method of the generated XXXController$Route class:

ctx.attribute("_MVC_METHOD_", controller.getClass().getMethod("CONTROLLER METHOD NAME"));

This way, we could retrieve the instance of the generated Controller MVC method from the Context’s attribute method in before/after handlers, and then we could obtain annotations added before the method or do other interesting things.

A clumsy solution would be to use the following similar code in the write method of ControllerMethodWriter to generate the relevant code:

writer.append("ctx.attribute(\"_MVC_METHOD_\", controller.getClass().getMethod(\"");
writer.append(method.simpleName()).append("\"));");

I suspect this should work, but I haven’t thought of a more elegant solution.

SentryMan commented 2 months ago

What prevents you from doing

@Get("/path/{pathVar}")
void before(Context ctx, String pathVar) {
ctx.attribute("_MVC_METHOD_", controller.getClass().getMethod("CONTROLLER METHOD NAME"));
}

In any case this seems like something to ask the javalin folks directly

rob-bygrave commented 2 months ago

... and then we could obtain annotations added before the method or do other interesting things.

For myself, it isn't clear what you are trying to achieve here. When I hear ... "obtain annotations ..." I wonder if there is some AOP method interception or Javalin Before/After handler logic that you are looking for. Can you outline an example of what you are looking to achieve?

ponyjoy commented 2 months ago

In Javalin, before/after handlers must specify a URL pattern to intercept. Imagine a scenario where I use before to implement user login status checking (or other logic), and this logic needs to take effect on multiple different URLs (such as /foo and /bar). I must use the following code:

app.before(ctx->{
    var requestPath = ctx.path();
    if("/foo".equals(requestPath) || "/bar".equals(requestPath)) {
            // some code
    }   
});
This approach requires manually determining the request path, and you could also put the URL list into a configuration file or read from a database.

If we are using avaje-http-generator and the Controller API approach, the above code would become like this:
```java
@Controller
public class DemoController {
    @Get("/foo")
    public String foo(){
        return "foo";
    }

    @Get("/bar")
    public String bar(){
        return "bar";
    }
}

In this case, if we define an annotation, let’s temporarily call it @RequireLogin, and place it in front of the foo() and bar() methods in DemoController. If we can write the foo and bar method objects into the Context attribute in the route() method of the generated code DemoController$Route, we can use the following in the generator ControllerMethodWriter:

    writer.append("ctx.attribute(\"_MVC_METHOD_\", controller.getClass().getMethod(\"");
    writer.append(method.simpleName()).append("\"));");

Then, our above before code can be modified to:

app.before(ctx->{
    Method mvcMethod = (Method)ctx.attribute("_MVC_METHOD_");
    if(mvcMethod.isAnnotationPresent(RequireLogin.class)){
            //check code...
    }   
});

If we add other actions in DemoController, we simply put @RequireLogin in front of the method, and it will be intercepted. I’m not sure if my description is clear.

ponyjoy commented 2 months ago

What prevents you from doing

@Get("/path/{pathVar}")
void before(Context ctx, String pathVar) {
ctx.attribute("_MVC_METHOD_", controller.getClass().getMethod("CONTROLLER METHOD NAME"));
}

In any case this seems like something to ask the javalin folks directly

You're right, this is not a common requirement. I forgot that the Context object can be referenced within the Controller method.

ponyjoy commented 2 months ago

My assumption was wrong. “before” is executed prior to the methods in the Controller, and even if the method object is placed in the attribute of Context, it cannot be obtained because at this point, the Context is not the Context of the action method in the Controller. ☹

SentryMan commented 2 months ago

Might need some form of AOP to do what you want then

ponyjoy commented 2 months ago

Might need some form of AOP to do what you want then

I will give it a try.

update:

I succeeded and achieved the intended purpose. However, there is an issue: why does the generated Controller$Proxy in avaje-inject wrap the exceptions thrown by MethodInterceptor in an InvocationException? In this case, the ExceptionHandler needs to pay attention to the InvocationException and then obtain the actual thrown exception through the cause. For now, it works.

SentryMan commented 2 months ago

why does the generated Controller$Proxy in avaje-inject wrap the exceptions thrown by MethodInterceptor in an InvocationException?

Are you using invoke or invokeUnchecked to execute your method? invoke should pass through the exception and only add a suppressed exception modification, invokeUnchecked is the one that wraps your exceptions.

edit: wait do you mean the generated code is wrapping the exception? That shouldn't happen. can you share the catch statements of your proxy class?

ponyjoy commented 2 months ago

why does the generated Controller$Proxy in avaje-inject wrap the exceptions thrown by MethodInterceptor in an InvocationException?

Are you using invokeinvokeUnchecked to execute your method? invoke should pass through the exception and only add a suppressed exception modification, invokeUnchecked is the one that wraps your exceptions.

edit: wait do you mean the generated code is wrapping the exception? That shouldn't happen. can you share the catch statements of your proxy class?

RequireLoginAspect

@Singleton
public class RequireLoginAspect implements AspectProvider<RequireLogin> {
    public static final String LOGIN_SESSION_FIELD = "USERNAME";

    @Override
    public MethodInterceptor interceptor(Method method, RequireLogin aspectAnnotation) {
        return new RequireLoginInterceptor();
    }

    static class RequireLoginInterceptor implements MethodInterceptor {

        @Override
        public void invoke(Invocation invocation) throws Throwable {
            Object[] arguments = invocation.arguments();
            final boolean[] isLogin = {false};
            Arrays.stream(arguments).filter(arg -> arg instanceof Context)
                    .findFirst()
                    .ifPresent(c -> {
                        Context ctx = (Context) c;
                        Object s = ctx.sessionAttribute(LOGIN_SESSION_FIELD);
                        isLogin[0] = s != null;
                    });
            if (!isLogin[0]) {
                throw new NotLoginException("Not login");
            }
            invocation.invoke();

        }
    }
}

MainController$Proxy

@Proxy
@Generated("io.avaje.inject.generator")
public final class MainController$Proxy extends MainController {

  private final Method hello0;
  private final MethodInterceptor hello0RequireLogin;

  public MainController$Proxy(AspectProvider<RequireLogin> requireLogin) {
    super();
    try {
      hello0 = MainController.class.getDeclaredMethod("hello", String.class);
      hello0RequireLogin = requireLogin.interceptor(hello0, hello0.getAnnotation(RequireLogin.class));

    } catch (Exception e) {
      throw new IllegalStateException(e);
    }
  }

  @Override
  public java.lang.String hello(String name) {
    var call = new Invocation.Call<>(() -> super.hello(name))
      .with(this, hello0, name);
    try {
      hello0RequireLogin.invoke(call);
      return call.finalResult();
    } catch (RuntimeException $ex) {
      $ex.addSuppressed(new InvocationException("hello proxy threw exception"));
      throw $ex;
    } catch (Throwable $t) {
      throw new InvocationException("hello proxy threw exception", $t);
    }
  }
}
SentryMan commented 2 months ago
 catch (RuntimeException $ex) {
      $ex.addSuppressed(new InvocationException("hello proxy threw exception"));
      throw $ex;
    }

you aren't throwing any checked exceptions right? It looks here that your runtime exceptions will be thrown unchanged

ponyjoy commented 2 months ago
 catch (RuntimeException $ex) {
      $ex.addSuppressed(new InvocationException("hello proxy threw exception"));
      throw $ex;
    }

you aren't throwing any checked exceptions right? It looks here that your runtime exceptions will be thrown unchanged

I threw NotLoginException in MethodInterceptor

SentryMan commented 2 months ago

I threw NotLoginException in MethodInterceptor

is that a checked exception? Because I've just tested locally and it works as expected for me. Also what version of inject?

ponyjoy commented 2 months ago

I threw NotLoginException in MethodInterceptor

is that a checked exception? Because I've just tested locally and it works as expected for me. Also what version of inject?

public class NotLoginException extends Exception{
    public NotLoginException(String message) {
        super(message);
    }
}

avaje-inject: 9.12

SentryMan commented 2 months ago

Only way around it then is to either change the extends to RuntimeException or add throws NotLoginException to each of the controller methods.

SentryMan commented 2 months ago

Closing since the initial issue has been resolved