Skip to content
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

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

Open
jim-krueger opened this issue Jan 24, 2024 · 11 comments
Open

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

jim-krueger opened this issue Jan 24, 2024 · 11 comments

Comments

@jim-krueger
Copy link
Contributor

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

@jansupol
Copy link
Contributor

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
Copy link
Contributor

jamezp commented Jan 26, 2024

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
Copy link
Contributor

mkarg commented Jan 26, 2024

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
Copy link
Contributor

jamezp commented Jan 26, 2024

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
Copy link
Contributor

"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.

@jamezp
Copy link
Contributor

jamezp commented Jan 29, 2024

"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
Copy link
Contributor Author

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
Copy link
Contributor

jansupol commented Feb 7, 2024

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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants