-
Notifications
You must be signed in to change notification settings - Fork 81
improve specification of InjectionPoint
#931
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
Conversation
|
I will submit a TCK PR in the next few days, but the spec text should be done IMHO. |
InjectionPoint
|
For the record, I'm not entirely sure if this perhaps doesn't somehow interact with passivation. I looked at the relevant part of the spec and what implementations do and didn't find anything obvious, but I admittedly don't know much about the topic, so I might be missing something. |
828fba5 to
cf48a70
Compare
Azquelt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the comments, this looks good to me.
| * An initial required type of the {@code Instance} is {@code java.lang.Object} and there are | ||
| * initially no required qualifiers. If no qualifier is passed to {@code Instance.select()}, | ||
| * there is one required qualifier: {@code @Default}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * An initial required type of the {@code Instance} is {@code java.lang.Object} and there are | |
| * initially no required qualifiers. If no qualifier is passed to {@code Instance.select()}, | |
| * there is one required qualifier: {@code @Default}. | |
| * The initial required type of the {@code Instance} is {@code java.lang.Object} and there are | |
| * initially no required qualifiers. If no qualifier is passed to {@code Instance.select()}, | |
| * there is one required qualifier: {@code @Default}. |
Also in other places where this text is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
|
||
| `CDI` implements `jakarta.enterprise.inject.Instance` and therefore might be used to perform programmatic lookup as defined in <<dynamic_lookup>>. | ||
| If no qualifier is passed to `CDI.select()` method, the `@Default` qualifier is assumed. | ||
| An initial required type of the `Instance` is `java.lang.Object` and there are initially no required qualifiers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| An initial required type of the `Instance` is `java.lang.Object` and there are initially no required qualifiers. | |
| Initially, the required type is `java.lang.Object` and there are no required qualifiers. |
Just trying to avoid saying "initial" twice in one sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will use this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
The specification of `InjectionPoint` contains a small number of holes that are arguably corner cases, but they should be specified nevertheless. The most important one is dynamically obtained instance (`Instance`) that is not injected. This can happen through several means: - `BeanContainer.createInstance()` - `CDI.current()` - `SeContainer` The specification didn't say what multiple methods should return in this case; it didn't even explicitly say that an `InjectionPoint` should exist and indeed it doesn't on one existing implementation. With this commit, the behavior is clear: an `InjectionPoint` must exist and all methods besides `getType()` and `getQualifiers()` return `null` (or `false`). Further, it is not clear what the `getBean()` method should return in case of a synthetic bean. Such injection points should, in portable extensions, be created using `BeanManager.createInjectionPoint()`, but these methods do not take a `Bean` and cannot figure out on their own which bean is the injection point created for. So on all existing implementations that support portable extensions, this method returns `null`. This is now explicitly allowed. Finally, this commit allows synthetic beans to have injection points that do not correspond to a field or parameter. Currently, such injection points should never exist, because injection points of synthetic beans should be created using `BeanManager.createInjectionPoint()`, but it is not hard to imagine people implementing `InjectionPoint` manually and returning `null` from `getMember()` or `getAnnotated()`. We also plan to introduce specifying injection points for synthetic beans in the build compatible extensions API, where one is supposed to only specify the required type and required qualifiers and it is entirely conceivable that no field or parameter can possibly be determined upfront. So an `InjectionPoint` of a synthetic bean is now explicitly allowed to return `null` from the `getMember()` and `getAnnotated()` methods.
…not injected An `Instance` that is not injected (or, in other words, that is obtained programmatically) has initially a required type of `java.lang.Object` and has initially no requried qualifiers. If no qualifier is passed to `Instance.select()`, there is one required qualifier: `@Default`. This has all been previously specified implicitly or using slightly imprecise wording. This commit is more explicit and more precise.
cf48a70 to
c9a8544
Compare
manovotn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The specification of
InjectionPointcontains a small number of holes that are arguably corner cases, but they should be specified nevertheless.The most important one is dynamically obtained instance (
Instance) that is not injected. This can happen through several means:BeanContainer.createInstance()CDI.current()SeContainerThe specification didn't say what multiple methods should return in this case; it didn't even explicitly say that an
InjectionPointshould exist and indeed it doesn't on one existing implementation. With this commit, the behavior is clear: anInjectionPointmust exist and all methods besidesgetType()andgetQualifiers()returnnull(orfalse).Further, it is not clear what the
getBean()method should return in case of a synthetic bean. Such injection points should, in portable extensions, be created usingBeanManager.createInjectionPoint(), but these methods do not take aBeanand cannot figure out on their own which bean is the injection point created for. So on all existing implementations that support portable extensions, this method returnsnull. This is now explicitly allowed.Finally, this commit allows synthetic beans to have injection points that do not correspond to a field or parameter. Currently, such injection points should never exist, because injection points of synthetic beans should be created using
BeanManager.createInjectionPoint(), but it is not hard to imagine people implementingInjectionPointmanually and returningnullfromgetMember()orgetAnnotated(). We also plan to introduce specifying injection points for synthetic beans in the build compatible extensions API, where one is supposed to only specify the required type and required qualifiers and it is entirely conceivable that no field or parameter can possibly be determined upfront. So anInjectionPointof a synthetic bean is now explicitly allowed to returnnullfrom thegetMember()andgetAnnotated()methods.Fixes #779
Related to #833