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 MvcMethod object for setMvcMethod and getMvcMethod. #3532

Closed
agentgt opened this issue Sep 16, 2024 · 9 comments
Closed

Add MvcMethod object for setMvcMethod and getMvcMethod. #3532

agentgt opened this issue Sep 16, 2024 · 9 comments

Comments

@agentgt
Copy link
Contributor

agentgt commented Sep 16, 2024

Currently setMvcMethod and getMvcMethod are a java.lang.reflect.Method.

The original impetus for set/getMvcMethod was for metrics and the actual java.lang.reflect.Method is not even needed but rather method name and signature.

I propose based on this comment: #3529 (comment)

That we replace get/setMvcMethod(io.jooby.MvcMethod method).

MvcMethod type would have enough metadata so that one could get the java.lang.reflect.Method dynamically if desired.

This will avoid reflective lookup during registration regardless of whether nor setMvcMethod is enabled or not.

I can do a PR for this for 3.4.0 depending on timeline of 3.4.0

@agentgt
Copy link
Contributor Author

agentgt commented Sep 16, 2024

The other advantage to this is if one actually did want to call the method they can use the more modern and in theory faster MethodLookup API.

@jknack
Copy link
Member

jknack commented Sep 16, 2024

We can probably add it, but either this way or just enabling the option in jooby-apt will require you to update all your projects, no?

@agentgt
Copy link
Contributor Author

agentgt commented Sep 16, 2024

Correct.

The code change is minor as that is one place but the compiler flag is not because of Maven limitations.

I’m proposing it because it is the right thing to do. We should not kick off reflection on initialization.

@jknack
Copy link
Member

jknack commented Sep 16, 2024

Ok, send a PR so we can release 3.4.0 with the change

@agentgt
Copy link
Contributor Author

agentgt commented Sep 17, 2024

@jknack This probably a silly question but I assume we are still targeting jDK 17 for 3.4.0 correct? I'm fairly sure but just want to check.

@jknack
Copy link
Member

jknack commented Sep 17, 2024

absolutely. jooby 4.0 will be 21

@jknack
Copy link
Member

jknack commented Sep 19, 2024

The apt option will be off by default in next major release: 4.0 regardless of how we model this object

@agentgt
Copy link
Contributor Author

agentgt commented Sep 19, 2024

Sorry @jknack I ran out of time. Can we shoot to add this for 3.5.0?

@agentgt
Copy link
Contributor Author

agentgt commented Sep 19, 2024

Never mind I see you already put it in. Thanks for implementing and I'm sorry I wasn't much help on it. I had some other work and I was trying to make a window for this and #3498 so that I was full jooby focus.

Thanks for putting it in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants