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 API for programmatic access to assignability rules for observer methods and typesafe resolution. #700

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

manovotn
Copy link
Contributor

Fixes #498

There was a request to have access to CDI assignability rules programmatically so this is the first thing coming to my mind.
I am not 100% convinced that this feature is very useful (in that it can find some audience) so I would appreciate some feedback on whether we want to add it or not.

Few notes:

  • Delegate assignability is deliberately left out as I cannot imagine anyone extending that.
  • The varargs argument for qualifiers might instead be a collection because there isn't really a case where this could be entirely left out due to presence of @Default - at least not the way it's worded right now as users are expected to list all required qualifiers.
  • We could use the TypesAndQualifiers to also represent the event payload/qualifiers and injection point type/qualifiers but I somehow found it cleaner this way as the required type would always be a singleton set.

@manovotn manovotn requested a review from Ladicek November 13, 2023 19:18
@Ladicek
Copy link
Contributor

Ladicek commented Nov 15, 2023

I think the current state is decent. I tried to improve it a little here: https://github.com/Ladicek/cdi-spec/commits/programmatic-assignability-access It's mainly about consistency and using terms defined by the CDI specification.

@manovotn
Copy link
Contributor Author

I think the current state is decent. I tried to improve it a little here: https://github.com/Ladicek/cdi-spec/commits/programmatic-assignability-access It's mainly about consistency and using terms defined by the CDI specification.

I've added them here and, as the commit suggested, squashed them into my commit.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 15, 2023

Thanks!

The only remaining question from me is: should we really expect users to correctly add implied qualifiers (@Default and @Any)? It feels like CDI implementations should take care of that, like everywhere else.

If I understand correctly, the rules for that are fairly simple. To all sets of qualifiers, the CDI implementation would:

  • add the @Any qualifier, always
  • add the @Default qualifier if and only if the set of qualifiers contains no other qualifiers but @Any, @Default and @Named(...)

That should be it I think?

@manovotn
Copy link
Contributor Author

The only remaining question from me is: should we really expect users to correctly add implied qualifiers (@default and @Any)? It feels like CDI implementations should take care of that, like everywhere else.

Right, that crossed my mind as well.
The reason I did it like this is to preserve the option to input literally any combination and see how CDI works in that case.
Also, I am not sure if it would then allow to cover every case as you might be able to get a bean (or an injection point) with some highly imaginative (read botched) qualifier combinations via extensions - such as deliberately defining such IP or stripping off its qualifiers etc.
Also note that this is slightly different for events as am OM with @Default will only get notified for events that have @Default or no qualifiers at all (as opposed to having the subset that the OM defines).
That being said, we can surely add that capability.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 15, 2023

Ah yeah, I always forget that the @Default qualifiers work differently for observers. Well, I guess we should put together a list of uncommon situations that could no longer occur when we automatically add implied qualifiers...?

Looking at it more thoroughly, here are some additional thoughts.

@Any:

  • typesafe resolution:
    • beans: all beans have @Any (=> we can safely add it to the set of bean qualifiers)
    • injection points: no rules (=> we shouldn't change the set of required qualifiers)
  • observer resolution:
    • events: all events have @Any (=> we can safely add it to the set of event qualifiers)
    • observer methods: no rules (=> we shouldn't change the set of observed event qualifiers)

@Default:

  • typesafe resolution:
    • beans: @Default is implied when there's no other qualifier other than @Any or @Named(...), but can be declared explicitly with other qualifiers too (=> we can safely add it to the set of bean qualifiers under the same rules)
    • injection points: @Default is implied if the set of required qualifiers is empty (=> we can safely add it to the set of required qualifiers if it's empty)
  • observer resolution:
    • events: no rules (=> we shouldn't change the set of event qualifiers)
    • observer methods: no rules (=> we shouldn't change the set of observed event qualifiers)

@manovotn manovotn force-pushed the issue498 branch 2 times, most recently from a1c1c3f to 083c308 Compare December 12, 2023 11:51
@manovotn manovotn marked this pull request as ready for review December 12, 2023 11:51
@manovotn manovotn requested a review from Ladicek December 12, 2023 11:51
@manovotn
Copy link
Contributor Author

@Ladicek I have reworded the text so that it no longer requires users to specify all qualifiers explicitly.

@Ladicek
Copy link
Contributor

Ladicek commented Dec 12, 2023

And could you please change the commit message? "Draft an API for ..." -> "Add API for ..."

@manovotn manovotn changed the title Draft an API for programmatic access to assignability rules for observer methods and typesafe resolution. Add API for programmatic access to assignability rules for observer methods and typesafe resolution. Dec 12, 2023
@manovotn
Copy link
Contributor Author

And could you please change the commit message? "Draft an API for ..." -> "Add API for ..."

All of the comments should be addressed now.

@Ladicek
Copy link
Contributor

Ladicek commented Dec 12, 2023

Commit message, not [just] PR title :-)

@Ladicek
Copy link
Contributor

Ladicek commented Dec 12, 2023

Ah and we should mention these methods in the spec text, chapter Programmatic access to container (beanmanager_lite.asciidoc).

@Ladicek
Copy link
Contributor

Ladicek commented Dec 12, 2023

I think something like this should be enough:

==== Assignability of beans and observers

The methods `BeanContainer.isMatchingBean()` and `isMatchingObserver()` provide access to assignability rules defined in <<typesafe_resolution>> and <<observer_resolution>>.

[source, java]
----
public boolean isMatchingBean(Set<Type> beanTypes, Set<Annotation> beanQualifiers, Type requiredType, Set<Annotation> requiredQualifiers);

public boolean isMatchingEvent(Type eventType, Set<Annotation> eventQualifiers, Type observedEventType, Set<Annotation> observedEventQualifiers);
----

@manovotn
Copy link
Contributor Author

Commit message, not [just] PR title :-)

Eeh, yea. Apparently it helps to save the changes when you perform them locally 🤦

Ah and we should mention these methods in the spec text, chapter Programmatic access to container (beanmanager_lite.asciidoc).

That's good point. Will do!

@manovotn
Copy link
Contributor Author

@Ladicek I've added the text to beanmanager_lite.asciidoc

Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@manovotn
Copy link
Contributor Author

manovotn commented Dec 12, 2023

Since we agreed to include this feature in during last meeting, I am going to merge it.
There is a tracking issue for TCKs that shouldn't be too hard to do - jakartaee/cdi-tck#518

@manovotn manovotn merged commit e8639b1 into jakartaee:main Dec 12, 2023
5 checks passed
@manovotn manovotn deleted the issue498 branch December 12, 2023 14:24
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.

Programmatic access to Assignability rules
2 participants