search

jakartaee / rest

Jakarta RESTful Web Services
Other
351 stars 114 forks source link

Deprecate @Context injection and @Suspended for Jakarta Rest 3.2 #1209

Open jim-krueger opened 5 months ago

jim-krueger commented 5 months ago

@Context injection and @Suspended will be deprecated in Jakarta Rest 3.2

jansupol commented 5 months ago

When I voted my +1 for the Jakarta REST 3.2, I did so with an expectation of 3.2 being less work than 4.0, i.e. the 3.1 (proprietary) injection being mandatory, while CDI injection being optional. I hope we agree on this, otherwise it makes more sense to have the release version 4.0.

For this, I would propose not to @Deprecate the @Context, but rather make it deprecated in the Spec document and Javadoc only.

jamezp commented 5 months ago

The @Context annotation is already deprecated in the spec document and the Javadoc. IMO users will not see that and it's going to catch users by surprise that it's gone.

From what I understand there are two issues:

  1. You can't remove things in a minor release, so we can't remove @Context.
  2. If you deprecate something, it needs a replacement.

On point 2 what is not clear to me is how you can deprecate something in documentation and not have a replacement, but if you add the @Deprecated annotation you must provided a replacement.

That said, I really don't see an issue with requiring CDI in a container. I understand that will be an issue on the client side. However, that's still going to be an issue whether we deprecate the annotation or remove it.

mkarg commented 5 months ago

The problem ist that software does exists that relies on the fact that 3.x is able to be run without CDI. It is not a good idea to introduce the enforcement for CDI in the 3.x line. Downstreams expect this enforcement in 4.x only.

jamezp commented 5 months ago

I guess that's what I meant by "requiring CDI in a container". We have this issue in 4.x really as well with the client side and defining whether or not it should be required in SeBootstrap. Maybe I misunderstood, but I thought those two were still an open question.

The current spec, 3.1, already requires CDI if running in a CDI container. I do understand that doesn't address all cases like a servlet container without CDI or SeBootstrap. I guess, and I could be wrong, I don't see it as a far stretch to require it, but I also understand why there would be an objection to it.

That said, I still think it's a really useful and good idea to deprecate @Context via the @Deprecated annotation to make it very clear that it's going away.

jansupol commented 5 months ago

"requiring CDI in a container"

This basically means that in the app server, we need to support the CDI injection in the container, whereas the customer still wants to use @Context. So in the container, we:

jamezp commented 5 months ago

"requiring CDI in a container"

This basically means that in the app server, we need to support the CDI injection in the container, whereas the customer still wants to use @Context. So in the container, we:

  • either need to support @Context by the CDI, which I am not sure is doable by the CDI API and even if it is, it is MORE work than just in 4.0 where there should not have been any @Context
  • or support both injection mechanisms (the 3.1 proprietary and CDI), but both injection mechanisms generate CDI beans and I am not sure how to specify which mechanism is in charge and the other should be skipped for that particular application.

Maybe I'm misunderstanding the spec wrong, but I thought this was already required. Per section 11.2.3:

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.

I don't really understand how this is a stretch from what is already required. Again, maybe I'm misunderstanding though.

jim-krueger commented 4 months ago

or support both injection mechanisms (the 3.1 proprietary and CDI), but both injection mechanisms generate CDI beans and I am not sure how to specify which mechanism is in charge and the other should be skipped for that particular application.

@jansupol do you still have concerns with the feasibility of this? I'm assuming that this is the logical direction to go for 3.2. Writing new code to "support @Context by CDI" solely for 3.2 where @Context is deprecated seems inappropriate.

jansupol commented 4 months ago

Simply deprecating the @Context and @Suspended and providing the alternative is actually not what I am concerned about. The provided replacement can be used in both non-CDI-managed and CDI-managed environments, using the injection mechanism we used in 3.1 as well as by the pure CDI injection.

My concern was mainly about mixing these two types of injection implementations on a single classpath. And a different behavior of each injection framework (possibly on a different issue).

spericas commented 4 months ago

@jim-krueger Do we have now a better understanding as to whether this deprecation (w/alternative) is feasible for 3.2? Still not clear to me that it is.

jim-krueger commented 4 months ago

@spericas Talking with @jamezp , who has had separate discussions concerning this with @jansupol , the feeling I get is that deprecation (w/alternative) is feasible. Of course the challenge is to get a CI (likely RESTEasy) along with the TCK updates completed before the EE11 Wave 5 deadline of March 29th.

Whenever asked we've always indicated that this is an aggressive goal, however the primary issues with deprecation (w/alternative) also exist with the original proposal for Jakarta Rest 4.0, where @Context would just have been removed. So continuing with our work on 3.2 has value, regardless of whether it actually makes it into EE11.

spericas commented 4 months ago

@jim-krueger Understood. I'd like to better understand who we plan to handle resource method invocations in the new and the old (and backward compatible) styles, as well as how to flag (and possibly reject?) any hybrid alternative.