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

JAX-RS ApplicationPath and Provider are bean defining annotations #1924

Merged
merged 1 commit into from
Apr 9, 2019

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Apr 8, 2019

@mkouba mkouba added this to the 0.14.0 milestone Apr 8, 2019
@mkouba mkouba requested review from gsmet and geoand April 8, 2019 16:10
@geoand
Copy link
Contributor

geoand commented Apr 8, 2019

Seems like it needs some import sorting :)

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Approved pending the imports being sorted out

@gastaldi
Copy link
Contributor

gastaldi commented Apr 8, 2019

I can confirm that this PR fixes #1927 too

@gsmet
Copy link
Member

gsmet commented Apr 8, 2019

@mkouba I have doubts about this PR but maybe I don't understand everything.

My doubts are about the @Provider annotations. If all the providers are made unremovable singleton beans, then our logic to determine which providers are indeed useful and should be included in the native image will be totally useless. Or am I missing something?

@kenfinnigan
Copy link
Member

I'm wondering if there's confusion between ArC removal of Beans, and native image DCE?

Is this fix purely to stop ArC from removing Beans, or does it impact native image size too?

@gsmet
Copy link
Member

gsmet commented Apr 8, 2019

@kenfinnigan my understanding is that if all the providers are made unremovable CDI beans, they will be included in the native image. Or am I wrong on this?

@kenfinnigan
Copy link
Member

That's why I was asking, as I'm not sure.

Is there a correlation?

Is it possible that ArC bean removal is separate and distinct from native image DCE?

@mkouba?

@geoand
Copy link
Contributor

geoand commented Apr 8, 2019

Good question! My guess is that they are not the same thing but it's a great opportunity for @mkouba to clarify

@mkouba
Copy link
Contributor Author

mkouba commented Apr 8, 2019

@kenfinnigan So ArC bean removal is a "framework level DCE". Normal DCE does not apply here, because we need hold references of all beans so that we're able to do programmatic lookup (for example CDI.current().select(Foo.class).get()).

my understanding is that if all the providers are made unremovable CDI beans, they will be included in the native image. Or am I wrong on this?

@gsmet Yes, you're right. So in general, any Provider that is part of the index will be included in the native image.

@kenfinnigan
Copy link
Member

Ok, so that's definitely bad then. As we don't want to retain all Provider instances.

Is there a way to register the provider classes once RESTEasy Common has found which ones we need? Instead of adding bean definition for everything with @Provider?

@mkouba
Copy link
Contributor Author

mkouba commented Apr 8, 2019

So is the ResteasyCommonProcessor also doing some kind of framework-level DCE? BTW we only make unremovable beans from indexed providers that doesn't mean all the providers on the class path.

@kenfinnigan
Copy link
Member

Yes, as it doesn't include for reflection to substrate any provider classes that aren't used by @Consumes or @Produces annotations

@mkouba
Copy link
Contributor Author

mkouba commented Apr 8, 2019

In that case, the ResteasyCommonProcessor should also register additional beans for the providers found. I'll try to update the PR tomorrow.

@mkouba mkouba force-pushed the issue-1880-jaxrs-bean-annotations branch 3 times, most recently from 6f1f545 to 7e121d2 Compare April 9, 2019 10:30
@mkouba
Copy link
Contributor Author

mkouba commented Apr 9, 2019

@gsmet I've updated the PR. @gastaldi Could you pls try it again?

@gastaldi
Copy link
Contributor

gastaldi commented Apr 9, 2019

@mkouba yes, this PR (still) fixes #1927 😃

Good job!

- Providers are unremovable beans
- a Provider with an injection point is Singleton
- resolves quarkusio#1880
@mkouba mkouba force-pushed the issue-1880-jaxrs-bean-annotations branch from 7e121d2 to 7b80be3 Compare April 9, 2019 12:33
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Let's wait for CI and merge.

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

Successfully merging this pull request may close these issues.

Make javax.ws.rs.ApplicationPath a bean defining annotation
5 participants