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

Allow @Enhancement to not restrict the types based on annotations #566

Merged
merged 3 commits into from Nov 29, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Nov 29, 2021

Fixes #564

I intentionally split this into 2 commits:

  1. Make @Enhancement(withAnnotations = {}) mean "no restriction on annotations". That is, types without annotations added through @Discovery may be subject to @Enhancement. The default is still BeanDefiningAnnotations.class.
  2. Chagne the default value to an empty array, to align with Portable Extensions.

That's because I'm pretty sure we'd have a consensus on item 1, but not necessarily on item 2.

In case an unannotated type is added through `@Discovery`, such type
previously couldn't be transformed during `@Enhancement`. With this
commit, it is possible to specify that the set of types considered
for `@Enhancement` is not restricted based on annotations in any way.
To do that, the `@Enhancement#withAnnotations` member should be set
to an empty array.
Previously, the default value of `@Enhancement#withAnnotations` was
the `BeanDefiningAnnotations.class` marker type. To align with
Portable Extensions, which don't have any annotation restriction
by default, this commit changes the default value to an empty array.
@graemerocher
Copy link
Contributor

It should be noted somewhere that if a bean defining annotation is not specified then then enhancement applies to all types discovered via annotations or added via the discovery phase, but not every type in the application

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 29, 2021

I think that directly follows from the @Enhancement javadoc/specification. It's probably not the best text I ever wrote, but it should be very clear in that @Enhancement only ever applies to types discovered during type discovery.

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 29, 2021

Though currently the text says "that were discovered during bean discovery", which is something I discussed with @manovotn and @mkouba at length on Friday. I'll change that to say "during type discovery".

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 29, 2021

Added a commit with clarifications of type discovery / bean discovery.

The terms "type discovery" and "bean discovery" are sometimes used
interchangeably, though they really shouldn't. This commit improves
the usage of these terms in `@Discovery` and `@Enhancement`.
In particular, the `@Enhancement` wording is changed to refer to
"type discovery" instead of "bean discovery", because bean discovery
happens _after_ `@Enhancement`.
@Ladicek Ladicek force-pushed the enhancement-unannotated-types branch from 3cf3043 to 560f832 Compare November 29, 2021 09:57
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @Ladicek!

@manovotn manovotn merged commit 50cde7a into jakartaee:master Nov 29, 2021
@Ladicek Ladicek deleted the enhancement-unannotated-types branch November 29, 2021 15:57
* Marker annotation type that, for the purpose of {@link Enhancement#withAnnotations()},
* represents set of bean defining annotations after the {@link Discovery @Discovery}
* phase is finished. That is, it includes custom normal scope annotations as well as
* custom stereotypes.
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ladicek I have been thinking about this some more and I am not sure if BeanDefiningAnnotations makes much sense with latest change (i.e. if it is justifiable to have that coded into spec).
Setting aside that I am not sure how well can we filter out classes with just custom stereotypes (which I will try to figure tomorrow), in what case would you want to filter for these classes? With annotated mode, this basically translates to "all classes". The only ones which are potentially omitted are those added by build compatible extensions that that don't have any annotation on them. That, to me, seems like a weird filter but maybe I am just not seeing the use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea was to indicate document that in annotated mode only classes with at least a bean definition annotation are processed. Having said that there is no technical reason for needing the annotation itself and maybe it is just documentation.

Copy link
Contributor Author

@Ladicek Ladicek Dec 1, 2021

Choose a reason for hiding this comment

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

I had to think about this for a while, but @manovotn is right. During type discovery, we discover classes with a BDA and classes added via extension in @Discovery. Classes added via extension automatically get @Dependent if they don't have a BDA already. So saying "only process classes that were discovered during type discovery and have a BDA" is basically equivalent to "only process classes that were discovered during type discovery".

I'll submit a PR removing this marker.

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.

@Enhancement(withAnnotations = ...) restricts usability of non-annotated classes added via @Discovery
3 participants