Sounds good. Thanks for digging into it!
On Wed, Aug 17, 2022 at 12:14 PM Matej Novotny <manovotn(a)redhat.com> wrote:
FTR, after another Zulip discussion with James, we found out even
more
conflicting expectations from REST towards how it should integrate with CDI.
We agreed that at this point it might be smarter to step back, keep the
state as is and bring this to REST experts to make some decisions and clear
up the expectations.
Matej
On Wed, Aug 17, 2022 at 4:54 PM James Perkins <jperkins(a)redhat.com> wrote:
>
>
> On Tue, Aug 16, 2022 at 6:23 PM Brian Stansberry <
> brian.stansberry(a)redhat.com> wrote:
>
>>
>>
>> On Tue, Aug 16, 2022 at 3:46 PM James Perkins <jperkins(a)redhat.com>
>> wrote:
>>
>>>
>>>
>>> On Tue, Aug 16, 2022 at 7:55 AM Brian Stansberry <
>>> brian.stansberry(a)redhat.com> wrote:
>>>
>>>> James, do we need to get this sorted for WF 27 Beta1?
>>>>
>>>
>>> If we want to do something like this, I'd say we would just so it has
>>> some bake time. It would be change to what we've done in the past, so it
>>> could use some time in the community. Probably a good TCK run too. TBH we
>>> might even fail the standalone REST TCK because of
>>>
https://github.com/jakartaee/rest/issues/1126.
>>>
>>
>> True. Assuming that challenge is regarded as valid we'd need to exclude
>> the test, unless the TCK is updated.
>>
>
> I found a case where this wouldn't work actually and it's a conflict in
> the spec IMO. Section 3.1.2
>
<
https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-...
> [1] is about constructors and how parameters can be injected into
> constructors. I'm not sure how we could handle that in CDI. We failed the
> RESTEasy testsuite and the TCK with this change because of that.
>
> [1]:
>
https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-...
>
>
>>
>>
>>>
>>>>
>>>> On Mon, Jul 11, 2022 at 5:47 AM Matej Novotny
<manovotn(a)redhat.com>
>>>> wrote:
>>>>
>>>>> Thanks for the write-up James.
>>>>> I'll just add a few random thoughts for people to chew on.
>>>>>
>>>>> 1) From CDI perspective, I think the linked PR (turning annotation
to
>>>>> bean defining) makes most sense; at least that's where we landed
after some
>>>>> discussion.
>>>>> There are other ways but they are hacky and prone to errors. For
>>>>> instance explicitly registering all classes as beans via extensions
but
>>>>> then you have to resolve conflicts when jaxrs annotated class also
defines,
>>>>> for example, cdi scope and gets picked up twice.
>>>>>
>>>>> 2) This approach won't solve everything of course. An example of
>>>>> potential problem is when you have an archive with CDI extension and
>>>>> without beans.xml, in such case, RE will fail to discover their
classes and
>>>>> register them as beans because such archive is by definition not a
bean
>>>>> archive (see
>>>>>
https://jakarta.ee/specifications/cdi/2.0/cdi-spec-2.0.html#bean_archive).
>>>>> This, however, is easily fixable by adding beans.xml into the JAR on
user
>>>>> side. But this scenario wouldn't work even today, the jaxrs
classes
>>>>> wouldn't be recognized as beans. And from CDI point of view, this
is
>>>>> actually correct anyway.
>>>>>
>>>>> 3) Jaxrs spec doesn't go too deep WRT CDI integration, for
instance
>>>>> it completely 'forgets' to mention that classes with their
annotations
>>>>> should be subject to CDI proxyability rules.
>>>>> I.e. you can encounter a final class with @Path annotation that the
>>>>> spec expects to be treated as CDI bean, but you cannot proxy final
classes
>>>>> so this is invalid according to CDI spec.
>>>>> This is the problem from WFLY-2859, but IMO it is basically a
>>>>> contradiction in the jaxrs specification.
>>>>> There might be other similar surprises but I am not expert on jaxrs
>>>>> specification :)
>>>>>
>>>>
>>>> Me neither. Without reading beyond this thread, the sentence '*In a
>>>> product that supports CDI, implementations MUST support the use of
>>>> CDI-style Beans as root resource classes, providers and Application
>>>> subclasses' says 'MUST support' *which doesn't rule out
non-spec
>>>> compliant mode that does not have that behavior. So, James, can the
jaxrs
>>>> subsystem provide such an option, defaulting to spec compliant? In terms
of
>>>> impl perhaps WeldCapability could add a method to register bean defining
>>>> annotations, which TBH is a better approach than hard coding them in
weld
>>>> subsystem classes is.
>>>>
>>>
>>> This text has been there for quite a while (at least in 2.1 which is EE
>>> 8) and it hasn't been an issue for WildFly. I just noticed it when doing
>>> the migration work to Jakarta REST 3.1/Jakarta EE 10. IMO the REST
>>> annotations are not correct in that they should be annotated
>>> with @Stereotype and this wouldn't even be an issue.
>>>
>>> By "provide an option" do you mean some way to configure whether or
not
>>> to register the annotations?
>>>
>>
>> Yes.
>>
>>> If so, that seems like something we could do. We'd likely want to add a
>>> subsystem attribute for this as well as allow it to be set by a deployment
>>> descriptor of some sort.
>>>
>>
>>> WRT using the WeldCapability, I'm not sure we can do that. It's
likely
>>> too late in the process of Weld by the time we get there. However, we might
>>> be able to add a new DUP in the which adds the annotations. Another option
>>> might be to have something like a PreProcessWeldCapability where we can
>>> handle things like this.
>>>
>>
>> That would only be an issue for configuration via deployment descriptor,
>> which seems like a nice-to-have. JaxrsSubsystemAdd would call
>> WeldCapability in Stage.RUNTIME.
>>
>>
>>>
>>>>
>>>>
>>>>> 4) CDI has no standardized API for registering additional bean
>>>>> defining annotations as this cannot be done universally across
>>>>> environments. This means the jaxrs spec cannot simply add this is a
>>>>> requirement.
>>>>>
>>>>> Matej
>>>>>
>>>>> On Tue, Jul 5, 2022 at 10:44 PM James Perkins
<jperkins(a)redhat.com>
>>>>> wrote:
>>>>>
>>>>>> Hello All,
>>>>>> In section 11.2.3
>>>>>>
<
https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-...
>>>>>> of the Jakarta REST Specification it states:
>>>>>>
>>>>>> *In a product that supports CDI, implementations MUST support
the
>>>>>> use of CDI-style Beans as root resource classes, providers and
Application
>>>>>> subclasses. Providers and Application subclasses MUST be
singletons or use
>>>>>> application scope.*
>>>>>>
>>>>>> This is something we haven't completely done yet in WildFly.
It
>>>>>> currently works for the most part because users generally add a
beans.xml
>>>>>> to their deployment. In previous versions of CDI the default bean
discovery
>>>>>> mode was all. The default is now annotated. With that it seems
more
>>>>>> critical to enable REST resources, providers and applications CDI
beans.
>>>>>> What currently happens now is beans are processed by the
>>>>>> ResteasyCdiExtension and given scopes. This doesn't quite
work the same if
>>>>>> the bean does not have a bean qualifying annotation and the
discovery mode
>>>>>> is annotated.
>>>>>>
>>>>>> I filed an issue (WFLY-16545
>>>>>> <
https://issues.redhat.com/browse/WFLY-16545> [2]) and a
PR
>>>>>> <
https://github.com/wildfly/wildfly/pull/15706> [3] to
make
>>>>>> the @Path, @Provider and @ApplicationPath annotations be bean
defining
>>>>>> annotations. This does create a different potential issue for
applications.
>>>>>> WFLY-2859 <
https://issues.redhat.com/browse/WFLY-2859> [4]
has one
>>>>>> potential issue which was raised before and seems to be the
reason we do
>>>>>> not treat these types as bean defining annotations.
>>>>>>
>>>>>> I think the REST spec does need some clarification for providers
>>>>>> that they are required to be CDI compliant. However, given the
text from
>>>>>> 11.2.3 of the spec and the fact that the default discovery mode
is now
>>>>>> annotated, it seems we should make these bean defining
annotations.
>>>>>>
>>>>>> @Matej Novotny <manovotn(a)redhat.com> and I have discussed a
bit,
>>>>>> but we'd like to see what others think as well.
>>>>>>
>>>>>> [1]:
>>>>>>
https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-...
>>>>>> [2]:
https://issues.redhat.com/browse/WFLY-16545
>>>>>> [3]:
https://github.com/wildfly/wildfly/pull/15706
>>>>>> [4]:
https://issues.redhat.com/browse/WFLY-2859
>>>>>>
>>>>>> --
>>>>>> James R. Perkins
>>>>>> JBoss by Red Hat
>>>>>>
>>>>> _______________________________________________
>>>>> wildfly-dev mailing list -- wildfly-dev(a)lists.jboss.org
>>>>> To unsubscribe send an email to wildfly-dev-leave(a)lists.jboss.org
>>>>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>>>>
>>>>
>>>>
>>>> --
>>>> Brian Stansberry
>>>> Principal Architect, Red Hat JBoss EAP
>>>> He/Him/His
>>>>
>>>
>>>
>>> --
>>> James R. Perkins
>>> JBoss by Red Hat
>>>
>>
>>
>> --
>> Brian Stansberry
>> Principal Architect, Red Hat JBoss EAP
>> He/Him/His
>>
>
>
> --
> James R. Perkins
> JBoss by Red Hat
>