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

Revisit the non-standard behavior of ArcContainer#instanceSupplier() #28115

Closed
mkouba opened this issue Sep 21, 2022 · 4 comments · Fixed by #28429
Closed

Revisit the non-standard behavior of ArcContainer#instanceSupplier() #28115

mkouba opened this issue Sep 21, 2022 · 4 comments · Fixed by #28429
Assignees
Labels
area/arc Issue related to ARC (dependency injection) area/housekeeping Issue type for generalized tasks not related to bugs or enhancements
Milestone

Comments

@mkouba
Copy link
Contributor

mkouba commented Sep 21, 2022

Description

Namely this part: https://github.com/quarkusio/quarkus/blob/2.13/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/ArcContainerImpl.java#L266-L276

Note that a change would break the compatibility because some extensions rely on this behavior (e.g. undertow).

@mkouba mkouba added area/arc Issue related to ARC (dependency injection) area/housekeeping Issue type for generalized tasks not related to bugs or enhancements labels Sep 21, 2022
@mkouba mkouba changed the title Revisit the non-standard behavior of ArcContainer#instanceSupplier Revisit the non-standard behavior of ArcContainer#instanceSupplier() Sep 21, 2022
@manovotn
Copy link
Contributor

manovotn commented Oct 3, 2022

@mkouba in what way did you mean to revisit this?
The method doesn't use any reference to standard CDI API and has proper documentation it terms of what it does.
We can surely make it behave more CDI-like in terms of throwing ambig dependency (or even unsatisfied) but at that point the only added value over using standard API is returning a supplier wrapper, right?

@mkouba
Copy link
Contributor Author

mkouba commented Oct 4, 2022

The method doesn't use any reference to standard CDI API and has proper documentation it terms of what it does.

Well, it does not behave as other instance() and lookup methods in general. That "filtering" got added later for one particular use case and I believe that an idiomatic solution would be to use the restricted bean types, i.e. @Typed.

but at that point the only added value over using standard API is returning a supplier wrapper, right?

Yes, and that was the only original added value leveraged in io.quarkus.arc.runtime.BeanContainer.instanceFactory() and recorders.

@manovotn
Copy link
Contributor

manovotn commented Oct 4, 2022

Well, it does not behave as other instance() and lookup methods in general. That "filtering" got added later for one particular use case and I believe that an idiomatic solution would be to use the restricted bean types, i.e. @typed.

I see, originally I thought that the type "filtering" was the initial reason for this method.
Ok, I can take a look at what would it take to transfer this to standard behavior.

@manovotn manovotn self-assigned this Oct 4, 2022
@mkouba
Copy link
Contributor Author

mkouba commented Oct 4, 2022

Ok, I can take a look at what would it take to transfer this to standard behavior.

It should be fairly straightforward to modify the undertow extension but this change could break existing extensions from quarkiverse etc. Also we can't deprecate the method (because we want to keep the original functionality) which makes it more difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/housekeeping Issue type for generalized tasks not related to bugs or enhancements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants