add support for synthetic bean injection points

[Ladicek]

Injection points are registered on a SyntheticBeanBuilder using withInjectionPoint(). The synthetic bean creation/destruction functions can look up injectable references for registered injection points from SyntheticInjections.

The SyntheticBeanCreator and SyntheticBeanDisposer interfaces used to have one method. That method is now deprecated for removal and a second method is added. The second method no longer has an Instance<Object> parameter for arbitrary lookups; instead, is has a parameter of type SyntheticInjections for pre-registered injections.

Fixes #957

[Ladicek]

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]

CC @manovotn @mkouba

[mkouba]

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]

What are you trying to fix by adding this?

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

[mkouba]

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]

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]

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.

[Azquelt]

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]

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...

[Ladicek]

I found some time and updated the proposal with a somewhat better (I believe) API. The main question remains (and it's why I'm keeping this PR as a draft): how can we bridge this with Portable Extensions, which expect full blown InjectionPoint? Need to discuss with @manovotn, because I'm no expert in that area.

[manovotn]

I found some time and updated the proposal with a somewhat better (I believe) API. The main question remains (and it's why I'm keeping this PR as a draft): how can we bridge this with Portable Extensions, which expect full blown InjectionPoint? Need to discuss with @manovotn, because I'm no expert in that area.

Pardon my ignorance, I am pretty sure we discussed this but I don't see it recorder here - what was the issue with introducing SyntheticInjectionPoint interface (a parent of jakarta.enterprise.inject.spi.InjectionPoint) that would have just getters for type and qualifiers?
I know Weld ATM assumes the other properties of the IP are non-null but that seems mendable.

[Ladicek]

I don't think there's an issue with that. (Besides the name. We need a good name.) If you think that's a reasonable way forward, I'll try to figure something out.

[manovotn]

I don't think there's an issue with that. (Besides the name. We need a good name.) If you think that's a reasonable way forward, I'll try to figure something out.

It does sound reasonable to me, but we can discuss it further in the mtg today (assuming you are coming).

[manovotn]

@Ladicek @Azquelt based on our discussion during the mtg, I checked for ProcessInjectionPoint event and it does not occur when the IP is defined by a synthetic bean.
[In order to check this, I used the TCK test BeanConfiguratorTest which has several synth beans declaring injection points, so you can just add PIP observer and see it right away]

It should therefore be safe to conclude that the synthetic IPs we are discussing here should not trigger the event either.
Which in turn means that adding the BaseInjectionPoint (a parent of current InjectioPoint) should be safe as users cannot really get their hands on an IP that would have null member value. Implementations will of course have to adapt accordingly. And we needn't change the javadoc of current InjectionPoint either.
Agree, disagree? :)

[Ladicek]

For an unexplainable reason, I'm quite fond of the idea of SyntheticInjectionPoint extending InjectionPoint :-)

[manovotn]

For an unexplainable reason, I'm quite fond of the idea of SyntheticInjectionPoint extending InjectionPoint :-)

Sure, works for me.

[Azquelt]

Users can also obtain the injection point via:

• Bean.getInjectionPoints()
• injecting InjectionPoint into a dependent-scoped bean

If we allow synthetic beans to have an InjectionPoint with a null member, then the first case would obviously allow the user to see it. I think they should also see it in the second case if the bean was obtained via BeanManager.getInjectableReference(InjectionPoint, CreationalContext), or the new SyntheticInjections methods.

If we really want to guarantee that the user cannot get an InjectionPoint with a null member, I think we'd need InjectionPoint to extend BasicInjectionPoint which doesn't have getMember or getAnnotated, add new methods to allow users to obtain BasicInjectionPoint rather than an InjectionPoint, and decide what to do when a bean which injects InjectionPoint is injected using a BasicInjectionPoint.

If we don't do that, I can't see how we could avoid altering InjectionPoint to allow getMember and getAnnotated to return null.

[manovotn]

Bean.getInjectionPoints()

Right, you'd only be getting regular IPs here until now, or empty collections for synth beans (excepting some edge cases where a synth. bean would "mimic" actual IP for the sake of validation which has no real use).
So this is indeed breaking one way or the other.

I think they should also see it in the second case if the bean was obtained via BeanManager.getInjectableReference(InjectionPoint, CreationalContext), or the new SyntheticInjections methods

I am not sure I understand how would you need to see the new IP here? Or you mean to use it in place of the current method?

[Ladicek]

I was thinking it would be quite acceptable to do a small breaking change and say that InjectionPoint.getMember() may return null in case the bean declaring the injection point is a synthetic bean. But then, I found this: https://jakarta.ee/specifications/cdi/4.1/jakarta-cdi-spec-4.1.html#inter_module_injection_full Apparently, the spec relies upon getMember() never returning null, even in case of synthetic beans.

[manovotn]

I was thinking it would be quite acceptable to do a small breaking change and say that InjectionPoint.getMember() may return null in case the bean declaring the injection point is a synthetic bean. But then, I found this: https://jakarta.ee/specifications/cdi/4.1/jakarta-cdi-spec-4.1.html#inter_module_injection_full Apparently, the spec relies upon getMember() never returning null, even in case of synthetic beans.

I was looking around and I didn't really find good explanation of what is this particular spec part about.
I do not dispute that the change would break this part of specification, I am just trying to grasp why it is there.
Specifically this part:

InjectionPoint.getMember() and then Member.getDeclaringClass() to determine the class that declares an injection point.

It has nothing to do with checking bean availability, we can do that with just type and qualifier and knowing which beans are enabled where. So it is likely meant to support the last bullet point:

the bean class is required to be accessible to classes in the module, according to the class accessibility requirements of the module architecture.

But how does it help there?
If you are breaking accessibility rules (I am thinking modules in java, or those in WFLY perhaps?) then the injection will not work regardless of whether the container checks the IP#getMember or not. You are essentially running into a problem that's more elementary than what CDI deals with.

[Ladicek]

Just FYI, my force-pushes recently didn't change anything in this PR, I just rebased on latest main.

[Ladicek]

Rebased and force-pushed and fixed 2 small issues.

[Ladicek]

Right now, I'm thinking we actually don't need to solve the bridge from Type/Annotation[] in BCExtensions to InjectionPoint in PExtensions, because that can be solved purely on the implementation level. Implementations are free to implement the extensions in any way they wish. If they implement BCExtensions using a PExtension, they can build an internal implementation of InjectionPoint that is not subject to validation, for example.

Hence, marking as ready.

[Ladicek]

Just for the record, I'm aware that the suggestion in my previous comment may lead to an unspecified situation when someone obtains the InjectionPoints from the Bean. It is a variation on the unspecified situation mentioned in #779. So I'm not suggesting we merge this now -- in fact, I think it would be best to combine this with a solution for #779.

[manovotn]

Just for the record, I'm aware that the suggestion in my previous comment may lead to an unspecified situation when someone obtains the InjectionPoints from the Bean. It is a variation on the unspecified situation mentioned in #779. So I'm not suggesting we merge this now -- in fact, I think it would be best to combine this with a solution for #779.

👍
I agree, the ability to return null from all other methods than type/qualifiers on the IP API would solve this.

[Ladicek]

Technically the only problematic method is IP.getMember(). getBean() is allowed to return null when the IP does not belong to a bean (such as a servlet), so it should be fine to allow null in more scenarios, and getAnnotated() is allowed to return null in CDI Lite, so I think there would be no harm in allowing that in certain situations in CDI Full as well.

I have an idea how we could accomplish IP.getMember() returning null, but I need to run a few more experiments.

[Ladicek]

I added an experiment with custom IPs on custom Beans here: https://github.com/Ladicek/custom-bean-ips

It seems IP.getMember() is not called for anything interesting. Weld only calls it during bean validation and OWB doesn't seem to call it at all. So I think we're safe and just have to rewrite the spec to allow this.

[Ladicek]

Rebased. I believe this is ready for merging, now that #931 is merged.

[manovotn]

I also wanted to ask whether you think we should mention this API in the spec doc itself (probably spi_lite.asciidoc)?
I know we currently don't describe the SyntheticBeanBuilder in any real detail and I don't insist on it, just wanted to know if leaving it out was intentional.

[Ladicek]

Added a short mention of injection points to spi_lite.asciidoc, but nothing major.

[Azquelt]

How do implementations destroy unneeded @Dependent instances? Should SyntheticInjections have a destroy method?

[Ladicek]

Great point. Let me think about that...

[Ladicek]

I pushed a small commit with my proposal. I removed this sentence, because the set of beans provided by SyntheticInjections is bounded (pre-defined by SyntheticBeanBuilder) and so we don't really have to care that much. The SyntheticInjections javadoc now says:

All @Dependent bean instances created by SyntheticInjections are destroyed when the corresponding synthetic bean instance is destroyed.

[Azquelt]

"Contains injectable references" suggests that the injectable references for injection points are always created prior to bean creation, which would in turn suggest that any @Dependent scoped beans are always created, even if they're never retrieved by the user.

Is this actually the case?

If not, I suggest changing the wording to "Provides injectable references".

[Ladicek]

We don't really specify whether the injectable references are created eagerly or lazily, and I think that's on purpose. I agree that "provides" is a better verb than "contains", for sure.

[Ladicek]

Good point made by @Azquelt on the CDI call today: we currently don't have an equivalent of @TransientReference.