[WFLY-16545] Register the @Provider, @Path and @ApplicationPath annot… #15706

jamezp · 2022-06-27T21:18:14Z

…ations as bean defining annotations.

https://issues.redhat.com/browse/WFLY-16545

Note the some what of a revert back to an empty beans.xml for these tests as @ApplicationScoped was added to the CDIBean. This was the requirement for the bean-discovery-mode="all". However, by defining the scope on the bean that resolves the bean discovery issue.

jamezp · 2022-06-28T00:31:57Z

These failures are valid. I've not looked into yet as maybe we don't want to end up doing this as it does cause issues with deployments that were previously not processed as beans. This is the reason it's now a draft.

manovotn

Looking at Linux - JDK 11 CI run, there are failures in CoordinatorTestCase. These are because org.wildfly.test.extension.rts.common.WorkRestATResource is marked as final and that will fail if it needs to create any subclass (which is always the case for normal scoped beans such as application scoped, or if you need to intercept/decorate these beans).
And I think all of these resources are going to be @ApplicationScoped, right? That's what's done in org.jboss.resteasy.cdiResteasyCdiExtension IIRC.
Does jaxrs spec say anything about these resources being final?

In the OpenAPIMultiModuleDeploymentIndexTestCase I am not sure what's the problem. It seems like the EJB is not recognized but nothing has changes WRT that.This might actually be the case you described where the deployment wasn't a bean archive previously (this test uses EAR with WAR and EJB jar)?

...m/src/main/java/org/jboss/as/weld/deployment/processors/BeanDefiningAnnotationProcessor.java

jamezp · 2022-06-28T15:26:38Z

@manovotn Interestingly enough, no it doesn't say anything about whether a resource can be final. There is an example of a @Provider which does show a final class. However, that seems wrong especially given a provider is required to be a CDI bean if CDI is available.

The default scope for resources is @RequestScoped, but that's not a huge issue :) And yes the org.jboss.resteasy.cdiResteasyCdiExtension assigns a scope to resources, providers and applications (jakata.ws.rs.core.Application).

manovotn · 2022-06-28T15:43:54Z

However, that seems wrong especially given a provider is required to be a CDI bean if CDI is available.

+1, those classes are not proxyable according to CDI spec
For clarity, client proxy is required for any built-in bean scope apart from @Dependent and @Singleton

jamezp · 2022-06-29T01:19:29Z

@manovotn This has some information on why this was not done before https://issues.redhat.com/browse/WFLY-2859. Well done, but reverted it seems.

manovotn · 2022-06-29T11:49:57Z

@jamezp right, that's exactly what we talked about - that some archives that were not bean archive now will be which can in turn lead to some side effects.
But truth be told, I don't fully understand how that leads to unproxyable exception? That is, unless you absolutely have to allow final class Provider and other things which just doesn't work with CDI by definition.

jamezp · 2022-06-29T14:27:13Z

@manovotn I definitely agree with you. Especially since the REST spec states that if CDI is available that applications, providers and resources MUST be CDI beans. I guess really it's an argument of should we mark the deployment as a CDI deployment if it contains any of those.

manovotn · 2022-06-29T14:46:28Z

Right, this should be really brought up to the spec. If you create an issue, I can pitch in with CDI perspective in case it's needed.

Also note that there is a possible scenario in which you can have an archive with REST bits that should become beans but there can be beans.xml that explicitly states none discovery mode. But I guess you'd have fallback for that already.

bstansberry · 2022-08-16T14:59:20Z

...m/src/main/java/org/jboss/as/weld/deployment/processors/BeanDefiningAnnotationProcessor.java

+        // happen, in which case these can be removed.
+        addAnnotation(deploymentUnit, new AnnotationType(PROVIDER, false));
+        addAnnotation(deploymentUnit, new AnnotationType(APPLICATION_PATH, false));
+        addAnnotation(deploymentUnit, new AnnotationType(PATH, false));


@jamezp I think these names (or perhaps the AnnotationType objects), and any others handled this way in this DUP, that are not required by the CDI spec itself should be passed in by the relevant subsystem using a new method in WeldCapability.

Would that happen early enough though? Weld needs to know the full set of bean defining annotations before it analyses any archive in order to determine if the archive is a bean archive in the first place.

I could add a DUP which executes between PARSE_CDI_BEAN_DEFINING_ANNOTATIONS and PARSE_WELD_DEPLOYMENT. There would be room for 128 different processors between those so that does seem reasonable.

I'll update the PR locally and do some testing.

@bstansberry @manovotn We could do something like this jamezp@eeb1369. It passed the single tests I added. I'll run the full test suite too and see how it does. It needs some cleanup, but that was a quick test. Using a DUP requires subsystems to have a dependency on org.jboss.as.weld which didn't seem right. This keeps the dependency on org.jboss.as.weld.common.

I updated the commit as I missed something in it, but the branch is https://github.com/jamezp/wildfly/tree/WFLY-16545-redo

@manovotn -- a subsystem attribute like James did is what I had in mind. The values get provided to the weld subsystem object before any DUP runs.

@jamezp I don't see the need for a new capability interface; the existing WeldCapability should be fine.

a subsystem attribute like James did is what I had in mind. The values get provided to the weld subsystem object before any DUP runs.

Got it, makes sense.

the existing WeldCapability should be fine.

+1, let's reuse that

Ah, right. It's a capability runtime API so it's not created and added very early. I'll update the PR as I think that makes the most sense.

…ations as bean defining annotations. https://issues.redhat.com/browse/WFLY-16545 Signed-off-by: James R. Perkins <jperkins@redhat.com>

jamezp · 2022-08-17T14:57:11Z

I unfortunately don't think this is going to work. Section 3.1.2 of the specification states:

A public constructor MAY include parameters annotated with one of the following: @Context, @HeaderParam, @CookieParam, @MatrixParam, @QueryParam or @PathParam.

There is no way I'm aware of in CDI to support cases like this.

manovotn · 2022-08-17T16:19:46Z

I unfortunately don't think this is going to work. Section 3.1.2 of the specification states:

A public constructor MAY include parameters annotated with one of the following: @Context, @HeaderParam, @CookieParam, @MatrixParam, @QueryParam or @PathParam.

There is no way I'm aware of in CDI to support cases like this.

Don't you already support that via wrapping of InjectionTarget in ResteasyCdiExtension?
I mean, even now you were providing these classes as beans (in certain scenarios) and these parameters could have been present. Hence I assume this was taken care of through mixing in your custom injection.
That logic could stay in place but the discovery removes the need for manual registration of beans. Or am I missing something?

Alternatively, those parameters would need to be beans and I have no idea if that is doable under current state or how RE handles them ATM.

jamezp · 2022-08-17T17:46:37Z

Given a Zulip conversation with @manovotn about clarification we likely need in the REST spec, I'm going to close this for now. I'll leave the JIRA open for the time being. There might be some workarounds for this, but the constructor parameter injection is a real issue that could break users.

jamezp requested a review from manovotn June 27, 2022 21:18

github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jun 27, 2022

jamezp marked this pull request as draft June 27, 2022 21:40

manovotn reviewed Jun 28, 2022

View reviewed changes

...m/src/main/java/org/jboss/as/weld/deployment/processors/BeanDefiningAnnotationProcessor.java Show resolved Hide resolved

jamezp force-pushed the WFLY-16545 branch 2 times, most recently from 52c21a1 to 08e6fec Compare July 11, 2022 20:33

jamezp marked this pull request as ready for review July 11, 2022 20:33

bstansberry reviewed Aug 16, 2022

View reviewed changes

jamezp force-pushed the WFLY-16545 branch from 08e6fec to db24862 Compare August 16, 2022 20:26

[WFLY-16545] Register the @Provider, @path and @ApplicationPath annot…

ce62041

…ations as bean defining annotations. https://issues.redhat.com/browse/WFLY-16545 Signed-off-by: James R. Perkins <jperkins@redhat.com>

jamezp force-pushed the WFLY-16545 branch from db24862 to ce62041 Compare August 16, 2022 20:34

jamezp closed this Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFLY-16545] Register the @Provider, @Path and @ApplicationPath annot… #15706

[WFLY-16545] Register the @Provider, @Path and @ApplicationPath annot… #15706

jamezp commented Jun 27, 2022

jamezp commented Jun 28, 2022

manovotn left a comment •

edited

jamezp commented Jun 28, 2022

manovotn commented Jun 28, 2022

jamezp commented Jun 29, 2022

manovotn commented Jun 29, 2022

jamezp commented Jun 29, 2022

manovotn commented Jun 29, 2022

bstansberry Aug 16, 2022

manovotn Aug 16, 2022

jamezp Aug 16, 2022

jamezp Aug 16, 2022 •

edited

bstansberry Aug 17, 2022

manovotn Aug 17, 2022

jamezp Aug 17, 2022

jamezp commented Aug 17, 2022

manovotn commented Aug 17, 2022 •

edited

jamezp commented Aug 17, 2022

[WFLY-16545] Register the @Provider, @Path and @ApplicationPath annot… #15706

[WFLY-16545] Register the @Provider, @Path and @ApplicationPath annot… #15706

Conversation

jamezp commented Jun 27, 2022

jamezp commented Jun 28, 2022

manovotn left a comment • edited

Choose a reason for hiding this comment

jamezp commented Jun 28, 2022

manovotn commented Jun 28, 2022

jamezp commented Jun 29, 2022

manovotn commented Jun 29, 2022

jamezp commented Jun 29, 2022

manovotn commented Jun 29, 2022

bstansberry Aug 16, 2022

Choose a reason for hiding this comment

manovotn Aug 16, 2022

Choose a reason for hiding this comment

jamezp Aug 16, 2022

Choose a reason for hiding this comment

jamezp Aug 16, 2022 • edited

Choose a reason for hiding this comment

bstansberry Aug 17, 2022

Choose a reason for hiding this comment

manovotn Aug 17, 2022

Choose a reason for hiding this comment

jamezp Aug 17, 2022

Choose a reason for hiding this comment

jamezp commented Aug 17, 2022

manovotn commented Aug 17, 2022 • edited

jamezp commented Aug 17, 2022

manovotn left a comment •

edited

jamezp Aug 16, 2022 •

edited

manovotn commented Aug 17, 2022 •

edited