jooby-apt: turn off Route.setReturnType by default and mark as deprecated

[jknack]

Given:

public class Controller1292 {
  @GET
  public String response() {
    return "mvc";
  }
}

Generates:

      /** See {@link Controller1292#response() */
      app.get("/", this::response)
        .setReturnType(String.class)
        .setMvcMethod(Controller1292.class.getMethod("response"));

These two option are now controlled by processor options: jooby.mvcMethod and jooby.returnType starting on next release these two options will be off by default (false). So output will be just:

      /** See {@link Controller1292#response() */
      app.get("/", this::response);

/cc @agentgt @kliushnichenko

[agentgt]

Why not always do it?

Performance? (I can’t see how but maybe)

[jknack]

Output looks clean/readable but also came from #3525 which makes javac to hang/fail not sure why bc jooby-apt generates valid code (probably method size but not sure). Finally you requested this change I doubt anyone else uses it :D

[agentgt]

Finally you requested this change I doubt anyone else uses it :D

IIRC there was someone else but perhaps I'm misremembering. Given the uptake of Open Telemetry and other metrics what do you recommend for applying metrics on a per request endpoint? Perhaps the normal one off Jooby projects don't have this need but we have like 20 different Jooby services. Our original use case was for metrics and logging. Without a ton of boilerplate would be introduced or some sort of heuristic to figure it out based on the route which is precisely what the route logic does.

I'm not going to lie this will be a very painful change for us even if it is configurable. We were not using Maven's annotation processor path and that is the only option if you use compiler flags. It is one of the reasons why I made almost all of JStachio config annotation driven: https://jstach.io/doc/jstachio/current/apidocs/#configuration (pick up the option via a module-info.java / package-info.java annotation).

The problem is that Maven annotation processor path setting has lots of issues. You have to put the version of the "apt" module every time (it ignores dependencyManagement) and properties for version is not ideal since Maven BOM importing does not import that. So you have to use a Maven enforcer to make sure the two are consistent.

As for the attributes that seems independent of this.

If I am the only one using this feature (and I swore I have seen others ask about it) than we should remove it. I will then I guess have to rely on byte code weaving or something aka use the OTEL javaagent which is not desirable. I don't like Java agents but if that is the only way to get class and method name metrics I suppose that is what I will have to do.

[agentgt]

The other option BTW is just to actually set the Method. As in route.setMethod(java.lang.reflect.Method) which avoids all the problems of attributes. If the method lookup is static there is very little cost in this.

EDIT: What I mean is that it is Graal VM native friendly if you statically get access to the reflection Method.

That is generate code that does:

// Graal does not mind reflection if you use literals to get access.
route.setReflectMethod(ExampleController.class.getDeclaredMethod("index", String.class));

Then to get annotations one can just do route.getReflectionMethod().getAnnotation(...) etc.

[jknack]

Route attributes is a bridge between the two models lambda vs mvc. Route attributes don't rely on reflection and all the effort on the new jooby-apt processor (including this one) is to reduce/remove reflection. Nothing on jooby (today) rely on method or return type

The feature is still there, just need to be enabled.

[agentgt]

The feature is still there, just need to be enabled.

Yes but I'm concerned. Every time you make an option it adds complexity (e.g. an if increases complexity). If I'm the only one using it and it is for a specific use case perhaps going after the specific use case is better than a general solution.

With the change you are breaking things (which I get for the better of the project you have to), you have to add more work for documenting the option, and you have to add specific code to check if the flag is there.

I just fail to see why someone would want to disable this. Honestly I seriously doubt the Java compiler is hanging because of generating code for that (and even for attributes). There is no way it is method size. I know this because I have templating library that generates the entire rendering logic in a single method. It takes a lot to exceed the limit.

So we should figure that out first.

Route attributes don't rely on reflection and all the effort on the new jooby-apt processor (including this one) is to reduce/remove reflection. Nothing on jooby (today) rely on method or return type

Yes ideally I would prefer the static stuff you do with attributes as you are right about that. It was unclear if that was getting removed or flagged as well because of to many attributes.

EDIT I don't need the actual reflective method. I just need its name and signature. It can be a String of some format. If we put that in it will work for my metric needs. I can't do this with the normal annotation attributes otherwise I have to copy the method name into some annotation.

[agentgt]

@jknack what I'm getting at is maybe we just remove the setMvcMethod altogether given I'm the only one using it and try to do something better.

To be honest I forgot that setMvcMethod actually takes the reflect.Method and instead maybe we do what Micronaut does and provide our own Method like object.

MvcMethod.of(Class<?>, "methodName", Param1.class, Param2.class....)

This would avoid the lookup on registration.

Then maybe MvcMethod has a helper method to get the actual reflect.Method.