-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
WebSockets Next: activate CDI session context only if needed #44323
WebSockets Next: activate CDI session context only if needed #44323
Conversation
mkouba
commented
Nov 5, 2024
- follo-up to WebSockets Next: activate CDI request context only if needed #43915
- related to WebSockets Next: performance improvements #39148
- follo-up to quarkusio#43915 - related to quarkusio#39148
Status for workflow
|
&& bean.isClassBean() | ||
&& bean.hasAroundInvokeInterceptors() | ||
&& SecurityTransformerUtils.hasSecurityAnnotation(bean.getTarget().get().asClass())) { | ||
// The given scope is RequestScoped, the bean is class-based, has an aroundInvoke interceptor associated and is annotated with a security annotation |
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.
CI is green, so it is probably unnecessary question, but this bean info comes from the ValidationPhaseBuildItem
and I think this item already reflects AnnotationsTransformerBuildItem
, so I am surprised that endpoints would have associated CDI interceptors. I thought they are removed. I suppose that class info does not reflect changes and you would need to use annotation store, but how is it that hasAroundInvokeInterceptors
is true?
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.
That's actually a very good question. I completely forgot that we remove the security annotations from endpoints. However, this PR does not change the logic, i.e. this condition was introduced in #43915: https://github.com/quarkusio/quarkus/pull/43915/files#diff-ceb74c4c03f53a1843b0c3b46d77068d483c1694677307b1415a86d6d91a8a34R487-R495.
Hm, so the annotations are removed and security interceptors should not be associated. But I guess that it's fine because we don't use the injected CurrentIdentityAssociation
(which is @RequestScoped
) for endpoint validation.
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.
Right, that is true, we don't need CurrentIdentityAssociation
for endpoint security checks. I didn't realize that. Cool, in that case it is fine.
🙈 The PR is closed and the preview is expired. |
Status for workflow
|
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