-
Notifications
You must be signed in to change notification settings - Fork 3k
Spring DI exposing Custom scopes as build item #47565
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
Spring DI exposing Custom scopes as build item #47565
Conversation
.collect(Collectors.toSet())); | ||
} | ||
for (DotName scope : stereotypeScopes.keySet()) { | ||
customScopeBuildProducer.produce(new CustomScopeBuildItem(scope)); |
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.
CustomScopeBuildItem
should only be produced for custom scopes; which isn't the case here, or?
@ozangunalp Why exactly do we need 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.
AFAICT the Spring DI extension is adding CDI scope annotations to classes where these Spring annotations are present.
So when it comes to other extensions adding default scope to classes containing some annotations (RM adding Dependent to classes containing Incoming/Outgoing, grpc adding Singleton to classes containing GrpcService, etc.) they can check whether custom scope is already present on the class.
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.
I see. So the correct solution for multiple extensions trying to add an "auto" scope is to "prioritize", i.e. only one extension can win ;-).
In this particular case, I guess that Spring DI should win because of the org.springframework.stereotype.Service
annotation used.
Therefore, I'd recommend to (1) rewrite the SmallRyeReactiveMessagingProcessor#transformBeanScope()
to produce a transformation with lower priority (Spring DI is using the default priority of 1000
). An idiomatic approach for an annotation transformer that merely adds a scope is to use the AutoAddScopeBuildItem
; it handles most of the tricky parts automatically (such as declared scope annotations).
Something like:
@BuildStep
AutoAddScopeBuildItem autoAddScope() {
return AutoAddScopeBuildItem.builder()
.containsAnnotations(INCOMING, OUTGOING, CHANNEL)
.defaultScope(BuiltinScope.DEPENDENT)
.build();
}
However, the AutoAddScopeBuildItem
transformer has the priority 2000
so we need to (2) modify the priority of the Spring DI's transformer as well. Or use a custom annotation transformer with lower priority instead of AutoAddScopeBuildItem
.
This comment has been minimized.
This comment has been minimized.
I need to adapt the IT I modified so see if it works. Obviously container runtime is not available on windows tests. |
|
||
@Override | ||
public int priority() { | ||
return 500; |
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.
It might make sense to add a comment that the value is 500
because the Spring DI's annotation transformer has the default priority of value 1000
and we want it to take precedence?
Also I would squash the changes, i.e. remove the first commit completely...
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.
Yes, I pushed the change to run the CI. Squasing now.
This comment has been minimized.
This comment has been minimized.
d535548
to
fadfb5a
Compare
…saging and grpc in order to take into account previously added annotations, ex. by the spring-di Resolves quarkusio#47544
fadfb5a
to
3f7a2da
Compare
Status for workflow
|
Resolves #47544
I am not sure of the impact of this change. So letting the CI run and have feedback from @mkouba, @geoand and @aureamunoz