search

vaadin / cdi

CDI Integration for Vaadin
Apache License 2.0
41 stars 56 forks source link

@RouteScoped beans are not discovered when bean-discovery-mode="annotated" #303

Closed roeltje25 closed 2 years ago

roeltje25 commented 4 years ago

When using bean-discovery-mode="annotated" all beans with Pseudo-scopes are not discovered or considered as beans. In order to mitigate this RouteScoped would have to be a Stereotype for example. This is e.g. discussed in https://issues.jboss.org/browse/WELD-1733?_sscc=t

@Stereotype
@Scope
@Inherited
@Target({ ElementType.ANNOTATION_TYPE, ElementType.TYPE, ElementType.FIELD,
        ElementType.METHOD, ElementType.CONSTRUCTOR })
@Retention(RetentionPolicy.RUNTIME)
public @interface RouteScoped {
}
mehdi-vaadin commented 4 years ago

@roeltje25 thank you for reporting this. It seems that the problem is not solved by just adding @Stereotype. It needs investigation.

roeltje25 commented 4 years ago

Are you sure? For me that actually solves it. I am running on wildlfy 17

mehdi-vaadin commented 4 years ago

Maybe I made a mistake. What exactly did you do? Only adding the @Stereotype to RouteScoped?

roeltje25 commented 4 years ago

Yep, that's all. Also for UISCoped by te way.

roeltje25 commented 4 years ago

bump

roeltje25 commented 4 years ago

still bump

roeltje25 commented 3 years ago

I have @RouteScoped and @UISCoped overridden in my projects already from 2019 to add the @StereoType annotation to make vaadin work with CDI and bean-discovery-mode="annotated".

Is there any reason to not fix this? Can I provide any assistance in getting this done? I would be glad to help.

roeltje25 commented 3 years ago

Again, I bump on this issue still.

I would like to refer to the CDI 1.2 spec that unless a pseudoscope is marked by @Stereotype it is not a bean-defining annotation. As such, this clearly is a bug. Please refer to the spec at: https://docs.jboss.org/cdi/spec/1.2/cdi-spec.html#bean_defining_annotations

Is there anyone keeping an eye on this?

roeltje25 commented 3 years ago

sorry for being adamant and persevering... but: bump

roeltje25 commented 2 years ago

Again a Bump here.... I still have to fix this in my project by overriding the Routescoped and UISCoped annotations.

Really nobody cares about this? It is such a minor change just add @Stereotype and it's all fine. And it's not a quirk, it's according to the spec. Refer to the documentation mentioned in the post above.

Please refer to the spec at: https://docs.jboss.org/cdi/spec/1.2/cdi-spec.html#bean_defining_annotations

taefi commented 2 years ago

@roeltje25 thanks for reporting this issue and being patient at the same time. I tried to do the same as you mentioned locally, which is:

Would you be able to demonstrate what you can achieve by adding the @Stereotype in a minimal reproducible example project?

roeltje25 commented 2 years ago

Would you be able to demonstrate what you can achieve by adding the @Stereotype in a minimal reproducible example project?

Will craft an example today.

roeltje25 commented 2 years ago

I have forked the cdi starter and made the MainView RouteScoped and for demo purposes also the GreetService UIScoped. That's a better demo because the @VaadinSessionScoped annotation is a NormalScope and discoverable anyway: https://github.com/roeltje25/vaadin-cdi-issue-303-skeleton-starter-demo

Note that I am not certain how to use the adjusted cdi as a dependency in maven. Therefore I have manually added the two annotations in the project and made them stereotypes. (project sources override library sources). You will notice that everything works. If you remove the two overridden Scope annotations, it does not work anymore.

Note Interestingly, for me wildfly 26.1.1 is used (which seems to make more sense than 17.0.1). Why you got 17.0.1 is a bit puzzling to me.

taefi commented 2 years ago

@roeltje25 You're right, the wildfly version is 26.1.1.Final.

taefi commented 2 years ago

@roeltje25 thanks for taking the time to create the demo application. Before we accept and merge #392 I had some questions which possibly can be discussed.

As mentioned in the https://docs.jboss.org/cdi/spec/1.2/cdi-spec.html#bean_defining_annotations marking a pseudo-scope annotation such as @Routescoped or @UIScoped would make them bean defining annotation, and as the result, any available view and component class in the application would be scanned as well while bean-discovery-mode="annotated", and this kinda reminds me a behavior very similar to what we can achieve with bean-discovery-mode="all". Do you have specific considerations regarding the use of bean-discovery-mode="annotated" in your application? This can shed some light on the decision we should make about marking those annotations as Stereotypes.

roeltje25 commented 2 years ago

Hi @taefi

Most certainly. There are actually many classes in our code base that I absolutely do not wat to be considered as beans. Many utility classes for example. Some of those classes will actually give trouble during deployment of our app. Moreover, the deployment process will take a lot more time when bean-discovery-mode="all" is used. bean-discovery-mode="annotated" gives as precise control over which classes are beans AND gives us higher deployment speed. Especially when deploying many times in environments using docker containers deployment speed is key.

Apart from that this discussion is a bit of a red herring isn't it? The spec of CDI allows for the discovery mode "annotated" -which is at least in java ee 7 and jakarta 8 the default - and Vaadin CDI does not support this -> this should be something we should fix in any case, right?

thanks for the followup!

taefi commented 2 years ago

@roeltje25 Thanks for providing the context. Of course, this should be fixed anyway, but maybe/maybe not adding @Stereotype is the answer.

roeltje25 commented 2 years ago

@taefi Well according to the standard in annotated mode bean defining annotations are

  1. Annotations with @NormalScope
  2. Annotations with @Stereotype
  3. Annotations @interceptor and @Decorator
  4. @Dependent annotation

Option 1 is not an option because components cannot be @normalScope as mentioned in javadoc of e.g. @NormalRouteScoped here : https://github.com/vaadin/cdi/blob/master/vaadin-cdi/src/main/java/com/vaadin/cdi/annotation/NormalRouteScoped.java Option 3 is not applicable as we are looking at normal beans not an interceptor Option 4 is not applicable as the beans in question are not dependent. We would get multiple instances e.g. when injecting in several other beans. It seems we should consider option 2 somehow.

One way is as proposed, adjust the vaadin pseudo scopes. Another way would be to define, or force developers to define their own stereotype which contains the @RouteScoped annotation. The latter solution sounds a bit verbose and 'feels' a bit like: we shove of the issue back to the devs...

Another solution still would be to somehow make the current annotation work as is by adjusting the vaadin cdi extension. However, as the annotations are not bean defining, I think finding the available beans would involve custom logic scanning the classes. This sounds to me overkill, and it's not very elegant to work around the standard this way.

My educated advice would really be to make the RouteScoped annotation a @Stereotype. Or if for some reason this absolutely is unacceptable at least provide a prewritten stereotype that already includes @RouteScoped. But again, while considering that, you already see thats a bit strange. You get 2 annotations that work the same, the only difference is that one of them won't work in discovery mode "annotated".

I rest my case...

taefi commented 2 years ago

@roeltje25 again, thanks, for helping with this issue, it is really appreciated.

Actually, the reason that we are still looking into it slowly and closely is that we are concerned about those applications that for some reasons (such as startup performance, etc.) might not want to get all of their @RouteScoped and @UIScoped beans discovered at startup while they set bean-discovery-mode="annotated". Probably, if from the beginning they were defined as @Stereotypes it was a different story. On one hand, adding @StereoType to @RouteScoped and @UIScoped would change this behavior, and on the other, adding one more set of annotations just for this purpose might be overkill IMO. In the end, we might consider adding @StereoType to those annotations as you suggested.

roeltje25 commented 2 years ago

I understand. Indeed if there are applications that work based on the non-bean discovered operation of these annotations I would agree. But isn't that just the whole point? under bean discovery mode "annotated" there actually is no use of those annotations at all if they are not stereo types. Well, unless: 1) these beans are interceptors or decorators... but I can not easily see what would be the use case of a @Routescoped or @UIScoped interceptor. This may be a valid boundary case though. 2) these beans have another @NormalScope as annotation. but why would you annotate a bean with a normal scope AND with @RouteScoped or @UIScoped? This is clearly an error on the dev's part if it exists in some codebase 3) these beans have a separate stereotype associated that makes them beans. But in that case it doesn't matter if the @RouteScoped and @UIScoped annotations become stereo types as well. 4) these beans are explicitly used as a set of beans that are enabled in discovery mode "all" and not in mode "annotated, an switching between these modes is used as a switch to enable these beans.... but this really is a big no no on the dev's part. One should not overload a switch like the discovery mode to implement some business or deployment logic.

Taking that into account I think that when you think about it, this simply becomes a no-brainer....

Roel

P.S. This is not to sound pushy, but I have thought about this particular issue already a long time...

Roel Meeuws Email: @.*** Mob. phone: +31 (0)6 10 82 44 01

[image: width=] http://www.avg.com/email-signature?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail Virus-free.www.avg.com http://www.avg.com/email-signature?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

Op ma 5 sep. 2022 om 09:45 schreef Soroosh Taefi @.***>:

@roeltje25 https://github.com/roeltje25 again, thanks, for helping with this issue, it is really appreciated.

Actually, the reason that we are still looking into it slowly and closely is that we are concerned about those applications that for some reasons (such as startup performance, etc.) might not want to get all of their @RouteScoped and @UIScoped beans discovered at startup while they set bean-discovery-mode="annotated". Probably, if from the beginning they were defined as @Stereotypes it was a different story. On one hand, adding @StereoType to @RouteScoped and @UIScoped would change this behavior, and on the other, adding one more set of annotations just for this purpose might be overkill IMO. In the end, we might consider adding @StereoType to those annotations as you suggested.

— Reply to this email directly, view it on GitHub https://github.com/vaadin/cdi/issues/303#issuecomment-1236651728, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQLW4RU7PKTVF4TJLTCJOTV4WQIXANCNFSM4JQAASPQ . You are receiving this because you were mentioned.Message ID: @.***>

vaadin-bot commented 1 year ago

This ticket/PR has been released with Vaadin 23.3.0.alpha3 and is also targeting the upcoming stable 23.3.0 version.