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

Kind of wish LookupIfProperty and LookupUnlessProperty were a little more powerful #40717

Closed
sixcorners opened this issue May 19, 2024 · 6 comments
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request

Comments

@sixcorners
Copy link

Description

When using Alternative/Priority, currently ArcContainerImpl#resolve will make sure that only the beans with the highest priority number are put into the Instance. It would be nice if the lesser priority beans that are not suppressed are able to be injected.

I also kind of wish that Instance could transitively evaluate these things. Like if you have an Instance<A> and A depends on B and B has a LookupIfProperty that Instance<A>.isResolvable() could check B's LookupIfProperty. If B gets excluded though I'm not sure if the expectation would be for a lower priority B alternative to be found or to just come back and say it's not resolvable. The former kind of gets you to full blown dynamic bean resolution as long as it's on the other side of an Instance which I know you all are trying to avoid.

Implementation ideas

If you implemented something for the first suggestion it occurs to me that you could still filter out lower priority beans that come after a bean that doesn't have a Lookup* annotation.

@sixcorners sixcorners added the kind/enhancement New feature or request label May 19, 2024
@geoand geoand added area/arc Issue related to ARC (dependency injection) and removed triage/needs-triage labels May 20, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented May 20, 2024

/cc @Ladicek (arc), @manovotn (arc), @mkouba (arc)

@mkouba
Copy link
Contributor

mkouba commented May 20, 2024

When using Alternative/Priority, currently ArcContainerImpl#resolve will make sure that only the beans with the highest priority number are put into the Instance. It would be nice if the lesser priority beans that are not suppressed are able to be injected.

Well, that's the purpose of CDI alternatives - to help the container to resolve the ambiguity. And the spec is pretty clear that Instance is resolving ambiguities. If you really need to access all beans that match the given type and qualifiers you can use BeanManager.getBeans(Type, Annotation...) and ArcContainer#instance(InjectableBean<T>). However, keep in mind that such beans might be considered unused and removed from the container.

I also kind of wish that Instance could transitively evaluate these things. Like if you have an Instance and A depends on B and B has a LookupIfProperty that Instance.isResolvable() could check B's LookupIfProperty. If B gets excluded though I'm not sure if the expectation would be for a lower priority B alternative to be found or to just come back and say it's not resolvable. The former kind of gets you to full blown dynamic bean resolution as long as it's on the other side of an Instance which I know you all are trying to avoid.

I'm not quite sure I fully understand this use case but if A depends directly on B than @LookupIfProperty has no effect because it's only applied to programmatic lookup and if A declares Instance<B> b then @LookupIfProperty is taken into account and so you can just test b.isResolvable() inside the business logic of A. We can't really filter out A based on the fact that it declares Instance<B> as we don't know how it is actually used...

@manovotn
Copy link
Contributor

I don't think this suggestion makes sense because you making two assumptions that don't work in CDI.

Firstly, Instance will resolve ambiguities meaning you are left with one bean with the highest priority. This is fully intentional and the typesafe resolution is built with this in mind so that CDI can tell you if it can deterministically pick a bean for any and all injection points.
Beans with lesser priority can still be resolved if you use their specific types, or qualifier combinations (if applicable of course)but they cannot work as a kind of a "fallback".

Secondly, the transitive chain of @LookupIfProperty you are suggesting isn't that straightforward to establish because when you have have an injection point with Instance<B>, you can still use that to resolve any bean that meets the <? extends B> criteria. Plus you can keep adding qualifiers via Instance.select(). And suitable bean implementation can have different @LookupIfProperty (or have none) meaning Arc cannot know which applies up until you actually perform the resolution with Instance#get().

@sixcorners
Copy link
Author

I think I wrote that when I was tired because it's missing some details heh.. Let me try again:

For the first suggestion currently if you have two alternative beans with a priority of 100 and 50, and the first is suppressed with a LookupIfProperty, the Instance<> will say it's unresolved because the 50 one was filtered out before it got to the LookupIfProperty logic. I think it would be useful if the bean at priority 50 could be resolved since it doesn't have a LookupIfProperty.

For the second suggestion. I'm only talking about the situation where B is a direct dependency of A. I know that Instance can always be injected. It seems to me if A depends on B directly and you are asking for programmatic lookup on A you could add B's programmatic lookup logic to the Instance and say it's unresolvable since you couldn't supply it with a programmatically LookupIfProperty suppressed B. On the other hand this is kind of a giant can of worms wrt what people might expect to also work if Instance<> was changed to do some logic with transitive dependencies so this one isn't a strong suggestion from me.

@mkouba
Copy link
Contributor

mkouba commented May 21, 2024

For the first suggestion currently if you have two alternative beans with a priority of 100 and 50, and the first is suppressed with a LookupIfProperty, the Instance<> will say it's unresolved because the 50 one was filtered out before it got to the LookupIfProperty logic. I think it would be useful if the bean at priority 50 could be resolved since it doesn't have a LookupIfProperty.

I see but it works exactly as defined 🤷. LookupIfProperty merely filters the resulting set of available beans for a particular Instance. Whereas an alternative bean B with priority 50 is removed from the set of available beans for a particular Instance (following the type-safe resolution rules). In fact, the bean B can be even removed as it could be considered unused.

@sixcorners
Copy link
Author

ok thanks for the info

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) kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants