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

Register Resource methods for reflection if ResourceInfo#getMethod is used in filters #28378

Merged
merged 1 commit into from Oct 6, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Oct 4, 2022

ResourceInfo#getMethod is used in order to retrieve the Resource method that is handles the HTTP request, and it obviously uses reflection. Resource methods are not by default registered for reflection, but this change introduces a way for Quarkus to read the bytecode of all filter methods and checks whether any of them uses the ResourceInfo#getMethod API and if so, forces the registration for reflection of all Resource methods.

Fixes: #28182

@geoand
Copy link
Contributor Author

geoand commented Oct 4, 2022

Note that this should not be backported (unless we really really need it) because this depends on #28371

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor Author

geoand commented Oct 5, 2022

MacOS job failure is unrelated

@Sgitario
Copy link
Contributor

Sgitario commented Oct 5, 2022

The changes in this pull request are really really remarkable and illustrative. Maybe I miss some code coverage, even being for Native only. Apart from that, I want to deeply review this pull request to understand all the tools and components (planning to do that this afternoon or tomorrow morning). If you are in a hurry, feel free to merge it, and I will review it later.

@geoand
Copy link
Contributor Author

geoand commented Oct 5, 2022

No hurry at all.

Indeed this change is needed only for native compatibility. That said, we could have made things A LOT simpler by just registering everything for reflection, but this being Quarkus, we really try to make things are performant as possible :).

The general idea (if it's not evident) is that at build time, we check the bytecode of the method that will actually be called to implement the filter and if we find the ResourceInfo#getMethod is being used, we ensure that all Resource methods are registered for reflection.
How all this is done of course has a lot technicalities :)

Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already had a chance to review these changes.
Awesome job!!
If I would have to comment something:

  • I would refactor the class FilterClassIntrospector to (1) move the logic that reads the method source and deals with the visitMethodInsn semantics into a general purpose class, so we can use it in the future to inspect other sources in the future, and (2) have a UsesGetResourceMethodInstropector (since just reading the class name, you can know what is about).
  • Even being for native, I would add tests to cover this functionality as it involves several components and can be easily broken in the future.

@geoand
Copy link
Contributor Author

geoand commented Oct 5, 2022

Awesome job!!

Thanks!

I would refactor the class FilterClassIntrospector to (1) move the logic that reads the method source and deals with the visitMethodInsn semantics into a general purpose class, so we can use it in the future to inspect other sources in the future, and (2) have a UsesGetResourceMethodInstropector (since just reading the class name, you can know what is about).

To be honest, I don't think it's that general to be applicable to other places.

Even being for native, I would add tests to cover this functionality as it involves several components and can be easily broken in the future.

I agree, however here is the problem: We would need a separate application for each of these cases in order to thoroughly test the functionality and as you know native tests don't come cheap, so I think we need to compromise.

@geoand
Copy link
Contributor Author

geoand commented Oct 6, 2022

I just realized I had not committed the test I had added! PR updated

@quarkus-bot

This comment has been minimized.

… used in filters

ResourceInfo#getMethod is used in order to retrieve the Resource method
that is handles the HTTP request, and it obviously uses reflection.
Resource methods are not by default registered for reflection, but this
change introduces a way for Quarkus to read the bytecode of all
filter methods and checks whether any of them uses
the ResourceInfo#getMethod API and if so, forces the registration
for reflection of all Resource methods.

Fixes: quarkusio#28182
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 6, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 6, 2022

Failing Jobs - Building 9e988b6

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

@geoand geoand merged commit 1993595 into quarkusio:main Oct 6, 2022
@geoand geoand deleted the #28182 branch October 6, 2022 09:34
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Oct 6, 2022
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants