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 SyntheticBeanBuilder.withInjectionPoint() #833

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jan 13, 2025

No description provided.

@Ladicek Ladicek added this to the CDI 5.0 milestone Jan 13, 2025
@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 13, 2025

This is a draft, because it's very much not ready. The biggest open question is: does SyntheticBeanBuilder.withInjectionPoint() in any way affect the Instance<Object> lookup parameter in SyntheticBeanCreator / SyntheticBeanDisposer?

One way would be to say something like:

Implementations are allowed to only support looking up beans from {@code lookup} that have previously been registered using {@code withInjectionPoint()}.

That is obviously a breaking change, but I don't see a better way that wouldn't be more breaking.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 13, 2025

CC @manovotn @mkouba

@mkouba
Copy link
Contributor

mkouba commented Jan 13, 2025

This is a draft, because it's very much not ready. The biggest open question is: does SyntheticBeanBuilder.withInjectionPoint() in any way affect the Instance<Object> lookup parameter in SyntheticBeanCreator / SyntheticBeanDisposer?

One way would be to say something like:

Implementations are allowed to only support looking up beans from {@code lookup} that have previously been registered using {@code withInjectionPoint()}.

That is obviously a breaking change, but I don't see a better way that wouldn't be more breaking.

👍 Although a breaking change it makes perfect sense IMO.

@Azquelt
Copy link
Member

Azquelt commented Jan 13, 2025

What are you trying to fix by adding this?

I don't see an explanation of the problem or a linked issue.

@mkouba
Copy link
Contributor

mkouba commented Jan 13, 2025

What are you trying to fix by adding this?

I don't see an explanation of the problem or a linked issue.

In a build-time environment, such as Quarkus, injection point metadata has to be recorded for any @Dependent synthetic bean since we don't know if it defines an InjectionPoint dependency or not. In other words, the current API prevents some build-time optimizations and enforces unnecessary bytecode generation and reflection.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 13, 2025

What @mkouba says is just the reason why I'm submitting this PR now.

The ultimate cause is feature parity -- Portable Extensions allow this, while Build Compatible Extensions do not.

@Ladicek Ladicek force-pushed the synthetic-bean-injection-points branch from 03541db to 039589f Compare January 14, 2025 15:41
@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 14, 2025

This is a draft, because it's very much not ready. The biggest open question is: does SyntheticBeanBuilder.withInjectionPoint() in any way affect the Instance<Object> lookup parameter in SyntheticBeanCreator / SyntheticBeanDisposer?

One way would be to say something like:

Implementations are allowed to only support looking up beans from {@code lookup} that have previously been registered using {@code withInjectionPoint()}.

That is obviously a breaking change, but I don't see a better way that wouldn't be more breaking.

I eventually decided that this is something we should keep doing in Quarkus, outside of the specification. The spec should stay silent on this.

@Ladicek Ladicek marked this pull request as ready for review January 14, 2025 15:43
@Azquelt
Copy link
Member

Azquelt commented Jan 14, 2025

Thanks @Ladicek for the explanation on the call. As I understand it now:

With portable extensions, you can create a synthetic bean by implementing Bean<T> which also allows you to implement the getInjectionPoints method. The injection points specified here aren't used in the creation of the bean instance, but are validated by the container and are available for inspection and validation by other extensions observing ProcessBean.

Other extensions can also modify the injection points by observing ProcessInjectionPoint, but any modifications would likely be ignored by the synthetic bean.

As far as I can see, there's no way to link an injection point to an injected dependent bean to allow the dependent bean to inspect where it's been injected.

This PR would add equivalent functionality to build compatible extensions. Injection points can be added to the synthetic bean and will be validated by the container and available for other extensions to inspect with @Registration and BeanInfo.

Additionally, Quarkus want the ability to do some additional out-of-spec optimisations (such as removing any beans which aren't needed), and having this functionality in build compatible extensions would add a way for the user to indicate that certain beans are used by their synthetic bean and can't be optimised away.

@Ladicek Ladicek marked this pull request as draft January 15, 2025 12:33
@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 15, 2025

Actually, let me push this PR back to draft, because there's a bit of a conflict we need to resolve: the Portable Extensions API allows this, but expect a direct implementation of InjectionPoint, which contains a lot more than just type and qualifiers. And at least in Weld, those extra members are used in validation, for example. So we need to unify those two APIs somehow...

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.

3 participants