jakartaee / rest

Jakarta RESTful Web Services
Other
363 stars 119 forks source link

Deprecating JAX-RS-specific annotations in favor of CDI #569

Open glassfishrobot opened 7 years ago

glassfishrobot commented 7 years ago

Latest CDI specifications allows to use CDI with Java SE. Some JAX-RS specific annotations duplicate ideas also covered by CDI annotations.

For long term, it makes sense to use CDI instead of proprietary annotations. To start this process, the JAX-RS EG should identify which annotations can be replaced by CDI, and mark them as deprecated.

glassfishrobot commented 6 years ago
andymc12 commented 6 years ago

@mkarg, per our discussion on PR #608, I think it might be good to change this issue's milestone to 2.2. The idea is to deprecate the some of the JAX-RS-specific annotations and replace them with optional CDI annotations. That would allow removal of the JAX-RS-specific annotations in the 3.0 release.

So in the 2.2 release the JAX-RS spec would support both @Context and @Inject - CDI is optional in the sense that users would not need to use CDI at all, but vendors would be required to support CDI if the vendor also makes CDI available. The JAX-RS 2.1 spec does something similar regarding JSON-B providers (vendors are required to supply a JSON-B provider if they already supply a JSON-B implementation). This allows JAX-RS-only vendors (like Jersey, CXF, RestEasy, etc.) to continue operating like 2.1 (no need to recognize CDI annotations), but full Java EE / Jakarta EE vendors (like Liberty, WildFly, etc.) would need to support both injection mechanisms.

mkarg commented 6 years ago

@andymc12 I tend to +1 for this but I am not fully conviced, because @spericas wrote that @Context and @Inject are not fully compatible. We should give it a deeper thought before pulling this into 2.2. Particularly I would be more happy if all vendors would give a short statement if they really want to do this in short term, because I do not want to delay 2.2 just for the sake of some troubles with CDI discovered too late.

arjantijms commented 6 years ago

@andymc12

I think it might be good to change this issue's milestone to 2.2. The idea is to deprecate the some of the JAX-RS-specific annotations and replace them with optional CDI annotations. That would allow removal of the JAX-RS-specific annotations in the 3.0 release.

+1

A CDI first approach for all EE specs (thus including JAX-RS) is favoured by Payara.

chkal commented 6 years ago

I'm also +1 for this. But I agree with @mkarg, that the incompatibilities between @Context and @Inject mentioned by @spericas should be discussed in detail.

ggam commented 6 years ago

+1. This was my goal for JAX-RS 2.1 but unfortunately the concern was raised too late to fit the schedule.

At that time I emailed Antoine Sabot Durand to ask some questions regarding this CDI integration and he seemed to be interested in helping. I think we could count with him now again.

El jue., 31 may. 2018 16:31, Andy McCright notifications@github.com escribió:

@mkarg https://github.com/mkarg, per our discussion on PR #608 https://github.com/eclipse-ee4j/jaxrs-api/pull/608, I think it might be good to change this issue's milestone to 2.2. The idea is to deprecate the some of the JAX-RS-specific annotations and replace them with optional CDI annotations. That would allow removal of the JAX-RS-specific annotations in the 3.0 release.

So in the 2.2 release the JAX-RS spec would support both @Context and @Inject - CDI is optional in the sense that users would not need to use CDI at all, but vendors would be required to support CDI if the vendor also makes CDI available. The JAX-RS 2.1 spec does something similar regarding JSON-B providers (vendors are required to supply a JSON-B provider if they already supply a JSON-B implementation). This allows JAX-RS-only vendors (like Jersey, CXF, RestEasy, etc.) to continue operating like 2.1 (no need to recognize CDI annotations), but full Java EE / Jakarta EE vendors (like Liberty, WildFly, etc.) would need to support both injection mechanisms.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/eclipse-ee4j/jaxrs-api/issues/569#issuecomment-393549787, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAucEMDA039wSPsTxUBwSiZNn88l_FIks5t3_7BgaJpZM4UVJ2E .

arjantijms commented 6 years ago

Hi,

On Thu, May 31, 2018 at 7:47 PM, Christian Kaltepoth < notifications@github.com> wrote:

I'm also +1 for this. But I agree with @mkarg https://github.com/mkarg, that the incompatibilities between @Context and @Inject mentioned by @spericas https://github.com/spericas should be discussed in detail.

It would be good to discuss indeed what @Context can do (if anything) that CDI can't.

In JSF we're busy with a similar switchover. We introduced CDI versions for existing JSF DI/Managed Bean constructs, and right away deprecated these JSF Managed Beans. In JSF 2.3 one can technically still use the native JSF Managed Beans and DI, but users are strongly encouraged to use the CDI based ones. I intend to remove the native managed beans system fully in JSF 2.3 (if, of course, enough of the other committers agree).

So for JAX-RS, let's see how we can practically best approach this.

Kind regards, Arjan Tijms

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/eclipse-ee4j/jaxrs-api/issues/569#issuecomment-393616581, or mute the thread https://github.com/notifications/unsubscribe-auth/AC5XTjfBmF3dM2y8GeM_mWsVu7xO4tYgks5t4Cy4gaJpZM4UVJ2E .

jansupol commented 6 years ago

Jersey made some experiments in making CDI the primary injection framework, but it was not 100% successful. I do not know the details right now, but Jersey allows for registering additional injectables in runtime, and we had an issue to tell CDI while the application already runs: look cdi, here is another class that from now on you start to inject. So this dynamically added injectables are injected via @Context and hk2 and @Inject is handled (more or less) by CDI. Replacing @Context with @Inject seems like large refactoring that we prefer to do in 3.0

arjantijms commented 6 years ago

Hi,

I'm indeed well aware of Jersey using HK2 internally and the step it took to experiment with CDI there (at Payara I also come into contact with that on almost a daily basis). I've even worked directly with the code I think you're referring to:

https://github.com/payara/patched-src-jersey-old/blob/jersey-2.26.payara-maintenance/ext/cdi/jersey-cdi1x/src/main/java/org/glassfish/jersey/ext/cdi1x/internal/ProcessAllAnnotatedTypes.java

and

https://github.com/payara/patched-src-jersey-old/blob/jersey-2.26.payara-maintenance/ext/cdi/jersey-cdi1x/src/main/java/org/glassfish/jersey/ext/cdi1x/internal/ProcessJAXRSAnnotatedTypes.java#L56

Which is related to org.glassfish.jersey.ext.cdi1x.spi.Hk2CustomBoundTypesProvider being present or not.

But these may be orthogonal concerns, in theory at least. At first it's perhaps not so much about replacing the implementation of @Context, but just making sure that all artefacts that currently can be injected using @Context can be injected using @Inject instead(in so far not already possible), and/or there being CDI alternatives.

For instance, in JSF we opted to add a small number of new names for existing native managed bean features, instead of letting CDI take over these in some way.

E.g.

See https://github.com/eclipse-ee4j/mojarra/blob/master/impl/src/main/java/javax/faces/annotation/ManagedProperty.java

which is the CDI replacement for;

https://github.com/eclipse-ee4j/mojarra/blob/master/impl/src/main/java/javax/faces/bean/ManagedProperty.java

Kind regards, Arjan Tijms

On Fri, Jun 1, 2018 at 11:44 AM, jansupol notifications@github.com wrote:

Jersey made some experiments in making CDI the primary injection framework, but it was not 100% successful. I do not know the details right now, but Jersey allows for registering additional injectables in runtime, and we had an issue to tell CDI while the application already runs: look cdi, here is another class that from now on you start to inject. So this dynamically added injectables are injected via @context https://github.com/context and hk2 and @Inject https://github.com/Inject is handled (more or less) by CDI. Replacing @context https://github.com/context with @Inject https://github.com/Inject seems like large refactoring that we prefer to do in 3.0

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/eclipse-ee4j/jaxrs-api/issues/569#issuecomment-393830305, or mute the thread https://github.com/notifications/unsubscribe-auth/AC5XTnKa41uVfHIjvkt5CskG8TdU_iH0ks5t4Q0DgaJpZM4UVJ2E .

spericas commented 6 years ago

As I stated in the PR, @Inject and @Context have different targets. @Inject cannot be used for params and @Context cannot be used in constructors. A very popular use case for @Context is indeed in param position.

The majority of JAX-RS resource methods use @Context in param position, except for the entity. This would need to be converted to an @Inject at method level, but would make things less clear regarding the entity param. There's also the issue of ContextResolver's which are somewhat similar to CDI producers and would need tweaking.

Again, the idea of replacing JAX-RS's DI framework with CDI is very sound, but I still think this is an all-or-nothing exercise that will necessarily break backward compatibility, and as such should be out of scope for 2.2.

chkal commented 6 years ago

@spericas To be honest, I'm surprised to hear that using @Context on parameters is a popular choice. I actually never saw this in any JAX-RS code. Of course @*Param usually is used on parameters. But if you want to get UriInfo and friends, using fields usually makes more sense. Or am I missing something here?

spericas commented 6 years ago

@chkal You're right, in thinking about using @Inject at method level (as it cannot be used at param level), I was thinking about @Context and @Param together. Clearly, @Param is far more popular at that level. Having said that, @Context isn't that uncommon in param position.

If it is just about field injection, then it shouldn't be that different. Differences in annotations would be in constructors (@Context again here), resource methods and perhaps even in properties.

ggam commented 6 years ago

I understand the all-or-nothing approach but we have to define what's that "all". My personal opinion: since CDI doesn't support parameter injection, people using CDI won't really miss it on JAX-RS. Moreover, they could even wonder why they can inject dependencies on JAX-RS and nowhere else.

Developers are currently using @Context at parameter level, and it will still work (for now). But it's possible that supporting it on CDI is not worth the effort, which I don't say it's not, but should be discussed anyway.

Just my 2 cents.

arjantijms commented 6 years ago

Hi,

Providing values for @*Param annotated parameters is a JAX-RS functionality, that would keep working regardless of what the bean model and DI system is.

As an @Context replacement for method parameters there may be a couple of solutions:

  1. For CDI based endpoints, just don't support injection of arbitrary beans
  2. Any method parameter not having a native JAX-RS @*Param annotation is taken to be injected via CDI. Such parameter may optionally be qualified.

@RequestScoped @Produces(TEXT_PLAIN) public class ProtectedResource {

@Inject
private SecurityContext securityContext;

@GET
@Path("sayHi")
public String sayHi(@NotNull @FormParam("name") String name, Foo foo,

@Red Bar bar) { return "saying hi " + "name"; } }

In the example above "name" is provided by the JAX-RS implementation.

---> param0Value = getInternally();

Where for "foo" the JAX-RS implementation would consult CDI:

---> param1Value = CDI.current().select(param1.getClass());

And for "bar" the JAX-RS implementation would consult with CDI as well, but passing in the qualifier:

---> param2Value = CDI.current().select(param2.getClass(), getQualifiers(param2));

Kind regards, Arjan Tijms

On Fri, Jun 1, 2018 at 5:41 PM, Santiago Pericas-Geertsen < notifications@github.com> wrote:

@chkal https://github.com/chkal You're right, in thinking about using @Inject https://github.com/Inject at method level (as it cannot be used at param level), I was thinking about @context https://github.com/context and @Param together. Clearly, @Param is far more popular at that level. Having said that, @context https://github.com/context isn't that uncommon in param position.

If it is just about field injection, then it shouldn't be that different. Differences in annotations would be in constructors (@context https://github.com/context again here), resource methods and perhaps even in properties.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/eclipse-ee4j/jaxrs-api/issues/569#issuecomment-393920254, or mute the thread https://github.com/notifications/unsubscribe-auth/AC5XTrvdmF0SUe8JsPqf8BzgOjjaL6MNks5t4WC0gaJpZM4UVJ2E .

spericas commented 6 years ago

Seems that we may have digressed into 3.0 discussions (which is great, but likely for another issue). This issue is about deprecating annotations in 2.2 and potentially providing a transition path to 3.0, and maybe allowing CDI annotations in certain locations in 2.2. Needless to say, JAX-RS 2.2 is intended to be backward compatible.

I have argued that the differences are large enough that people would need to change their applications quite significantly when moving to 3.0 that I don't see the point of more intermediary steps and new rules for developers to learn during the transition.

I'd much rather start the discussion on 3.0, but let's close the loop on this issue first. So, should we do anything related to DI and CDI in 2.2?

arjantijms commented 6 years ago

Needless to say, JAX-RS 2.2 is intended to be backward compatible.

I hear you, though I think the proposed changes till so far do ensure that.

Deprecating @Context is just that, deprecation.

While public String sayHi(@NotNull @FormParam("name") String name, Foo foo is backwards compatible, unless I'm missing something. As far as I know this syntax wasn't allowed in 2.1, so adding it to 2.2 constitutes a new feature. It's not an intermediate step as this would be in 3.0 as well.

Comparing to JSF again, between between 2.2 and 2.3 we made a couple of larger additions to support CDI than what is currently proposed here, so my gut feeling is this all could be for JAX-RS 2.2.

arjantijms commented 6 years ago

p.s. one question is where the work of saying that public String sayHi(@NotNull @FormParam("name") String name, Foo foo) is supported can be done.

I can do a PR for Jersey to support this, but where do we document it? There is no spec project yet for JAX-RS.

spericas commented 6 years ago

@arjantijms I understand that deprecation does not change semantics. However, it does result in annoying warning messages which would be tolerable if the version after 2.2 would only require replacing @Context by @Inject. If such a version is 3.0, which is likely, that would hardly be the only requirement.

As for the sayHi() method. If it was an @PUT/@POST and you didn't use @Context, how would distinguish the entity among all the injected bean params?

arjantijms commented 6 years ago

I understand that deprecation does not change semantics. However, it does result in annoying warning messages which would be tolerable if the version after 2.2 would only require replacing @context by @Inject.

I'm not sure if it should be the version after 2.2. If there's an actual @Deprecated annotation, there should be a replacement right away.

I might still be missing something, but it may not be that difficult.

It concerns the following artefacts / types that are now injectable via @Context:

  1. SecurityContext
  2. Request
  3. Application,
  4. Configuration
  5. Providers
  6. ResourceContext
  7. ServletConfig
  8. ServletContext
  9. HttpServletRequest
  10. HttpServletResponse
  11. HttpHeaders
  12. UriInfo

HttpServletRequest and ServletContext are already injectable via @Inject (built-in CDI beans). To make the others injectable should only be a matter of shipping a CDI extension that produces these, although JAX-RS should perhaps not define producers for the Servlet types.

As for the sayHi() method. If it was an @PUT/@post and you didn't use @context, how would distinguish the entity among all the injected bean params?

You're right, that was the part I'm missing there. JAX-RS already has a non-annotated parameter for the incoming entity. I typically used @FormParam or @BeanParam, but you're referring to something like this right?

@Consumes(APPLICATION_JSON)
public void method(MyEntity entity) {
    ...
}

With MyEntity

public Class MyEntity {
    int age;
    String name;
}

There's still a couple of possible solutions I can think of it. Perhaps others can think of some additional options.

  1. Qualify the entity with a JAX-RS specific annotation
  2. Qualify the injected bean parameters
  3. Adopt a convention; first non annotated parameter is the JAX-RS entity, next ones are bean parameters.

For example:

1 entity and 1 bean parameter:

@Consumes(APPLICATION_JSON)
public void method(@EntityParam MyEntity entity, Foo foo) {
    ...
}

1 entity with annotation

@Consumes(APPLICATION_JSON)
public void method(@EntityParam MyEntity entity) {
    ...
}

1 entity without annotation (for backwards compatibility)

@Consumes(APPLICATION_JSON)
public void method(MyEntity entity) {
    ...
}
spericas commented 6 years ago

@arjantijms Yes, there are indeed solutions, but those that are backward compatible aren't very elegant IMO (not just because of additional conventions, but also for the use of ContextResolver's as "producers" for non-Context injection points) and the other more elegant ones are for a 3.0 discussion ;)

arjantijms commented 6 years ago

Yes, there are indeed solutions, but those that are backward compatible aren't very elegant IMO

Which ones do you exactly mean? (several where mention above ;))

@Inject ResourceContext resourceContext

That's probably as elegant and as backwards compatible as you can get, I guess. It's something that should be easy to implement, easy to specify, and won't need changing in a 3.0.

The other one:

@Consumes(APPLICATION_JSON)
public void method(@EntityParam MyEntity entity, Foo foo) {
    ...
}

Is that the elegant one for the 3.0 discussion, or the not-very elegantly one?

chkal commented 6 years ago

@arjantijms

HttpServletRequest and ServletContext are already injectable via @Inject (built-in CDI beans). To make the others injectable should only be a matter of shipping a CDI extension that produces these, although JAX-RS should perhaps not define producers for the Servlet types.

I don't think it will be that easy. Actually the HttpServletRequest injectable via CDI has one major issue. It only provides the original HttpServletRequest without any wrappers applied via Servlet filters, which can lead to surprising results (see CDI-709).

And I can imagine that JAX-RS implementations also use wrappers which are then injected via @Context into the corresponding resource. At least CXF is doing this. So maybe JAX-RS implementations must produce the Servlet objects them self?

arjantijms commented 6 years ago

I don't think it will be that easy. Actually the HttpServletRequest injectable via CDI has one major issue. It only provides the original HttpServletRequest without any wrappers applied via Servlet filters, which can lead to surprising results (see CDI-709).

You're right, I mentioned that issue a couple of times before. In theory it's even a bit worse as you could even get a random HttpServletRequest even (the CDI spec doesn't say anything about which version you should get).

And I can imagine that JAX-RS implementations also use wrappers which are then injected via @Context into the corresponding resource. At least CXF is doing this.

Interesting. If that happens after the JAX-RS Servlet is called then the Servlet implementation wouldn't know about them.

The simplest way out for now may be to only provide producers for the JAX-RS owned types, then keep using @Context for the other ones.

Another option may be to use qualifiers, so it's clear code is asking for the JAX-RS version of these Servlet owned types.

It's also possible to create an alternative producer, which can return the originally produced HttpServletRequest when not in a JAX-RS context, and otherwise the version that's potentially wrapped by JAX-RS.

Ultimately the Servlet owned types should be produced by the Servlet spec, but unfortunately Greg from Jetty is always blocking that, but that's just because of not understanding what's being asked. See https://issues.jboss.org/browse/CDI-492

chkal commented 6 years ago

Interesting. If that happens after the JAX-RS Servlet is called then the Servlet implementation wouldn't know about them.

Correct. I don't remember all the details anymore, but if you use @Context to get the HttpServletRequest from CXF, you will get a ThreadLocalHttpServletRequest. And IIRC this one is even wrapping another CXF request wrapper.

The simplest way out for now may be to only provide producers for the JAX-RS owned types, then keep using @context for the other ones.

I don't like this idea. Actually we want to get rid of @Context in the long term, correct?

Another option may be to use qualifiers, so it's clear code is asking for the JAX-RS version of these Servlet owned types.

This was also my first idea. But it may be confusing for developers that there are multiple "versions" of the Servlet objects available for injection.

Ultimately the Servlet owned types should be produced by the Servlet spec, but unfortunately Greg from Jetty is always blocking that

Yeah, unfortunately it looks like there won't be much progress on this issue in the near future.

The only way to work around these issues could be to provide some JAX-RS owned class that provides access to the Servlet objects. Something like:

@Path("foo")
public class MyResource {

  @Inject
  private ServletEnvironment servletEnv;

  @GET
  public String get() {
    HttpServletRequest request = servletEnv.getRequest();
    HttpServletResponse response = servletEnv.getResponse();
    // ...
  }

}

Maybe this approach could also be used to make it more explicit that JAX-RS actually doesn't require an Servlet environment? So there could be something like the ExternalContext in JSF. Just thinking out loud. Not sure I really like this idea.

arjantijms commented 6 years ago

I don't like this idea. Actually we want to get rid of @Context in the long term, correct?

I'm not particularly fond about that idea either, but just wanted to mention it for completeness. We absolutely should get rid of @Context. It's one of the things that's very confusing, specifically to people who are new to Java EE and JAX-RS. Sometimes needing @Context to inject things, and sometimes needing @Inject is hard to explain.

This was also my first idea. But it may be confusing for developers that there are multiple "versions" of the Servlet objects available for injection.

It can be, although it does represent reality a little. At some point we could potentially have @Inject @Original HttpServletRequest if you wanted the original, and @Inject @Current HttpServletRequest for well, the current one. But "current" (last known one by Servlet) vs "jaxrs" might not be intuitive.

Yeah, unfortunately it looks like there won't be much progress on this issue in the near future.

Perhaps not. We'll have to see how things move forward when the Servlet group starts again though. Maybe splitting up the Servlet spec (as proposed on the EE4J list before) is an option.

The only way to work around these issues could be to provide some JAX-RS owned class that provides access to the Servlet objects.

Hmmm, might not be a bad idea indeed. Although not exactly the same, EE Security has something similar:

https://javaee.github.io/javaee-spec/javadocs/javax/security/enterprise/authentication/mechanism/http/HttpMessageContext.html

ggam commented 6 years ago

I'm not particularly fond about that idea either, but just wanted to mention it for completeness. We absolutely should get rid of @context. It's one of the things that's very confusing, specifically to people who are new to Java EE and JAX-RS. Sometimes needing @context to inject things, and sometimes needing @Inject is hard to explain.

Totally agree. And then, having parameter injection support ONLY on JAX-RS and nowhere else isn't a bit confusing too? ;)

It can be, although it does represent reality a little. At some point we could potentially have @Inject @Original HttpServletRequest if you wanted the original, and @Inject @Current HttpServletRequest for well, the current one. But "current" (last known one by Servlet) vs "jaxrs" might not be intuitive.

Is is intended by the Servlet spec that you have a way to access the original request? Using the original request can be problematic as the request might have even been forwarded, which changes some attributes.

For me, the injected request should be the one that the dispatched (JSF, JAX-RS, etc) Servlet got. Getting the original request doesn't feel safe to me and is not something we should encourage. People who need the original request can easily register a filter as the first one on web.xml, store it on a request attribute and read it from a @Producer afterwards.

chkal commented 6 years ago

Is is intended by the Servlet spec that you have a way to access the original request? Using the original request can be problematic as the request might have even been forwarded, which changes some attributes.

For me, the injected request should be the one that the dispatched (JSF, JAX-RS, etc) Servlet got. Getting the original request doesn't feel safe to me and is not something we should encourage. People who need the original request can easily register a filter as the first one on web.xml, store it on a request attribute and read it from a @Producer afterwards.

I absolutely agree on this one. The only problem is what @arjantijms already mentioned: You could also inject the Servlet objects (HttpServletRequest and friends) into a filter or into a bean which is used as part of the filter processing. So at this point the request didn't even reach the Servlet yet. And in each filter the "current" request/response could be a completely different one, because every filter could wrap it again.

ggam commented 6 years ago

I absolutely agree on this one. The only problem is what @arjantijms already mentioned: You could also inject the Servlet objects (HttpServletRequest and friends) into a filter or into a bean which is used as part of the filter processing. So at this point the request didn't even reach the Servlet yet. And in each filter the "current" request/response could be a completely different one, because every filter could wrap it again.

True. But even in that case, there's already a "current request", which incidentally won't be request scoped, but @Dependent. This might seem strange, but is similar to the problem with the authenticated caller and JASPIC, which Arjan also discovered on MicroProfile: the request can be initially unauthenticated, authenticated at some point and unauthenticated again before the request ends. Which is the caller principal? It depends on when you ask for it.

I can think of two solutions for that:

Either way provides users enough power to create their own producers to support more sophisticated configurations.

mkarg commented 6 years ago

@chkal As you more actively contribute in this thread as I currently do, I would like you to co-assign this issue to you. Agreed?

chkal commented 6 years ago

@ggam Thanks for the summary. I would actually prefer the second option. IMO injecting a proxied view should be the default. Not sure if we really need the Supplier<...> version to get an request without wrapper though. However, I guess all this should go into the Servlet or CDI spec. I don't think that JAX-RS should provide these mechanisms. So perhaps creating some JAX-RS owned class which is directly injectable like I proposed above would be a good option for JAX-RS?

@mkarg Sure, I just so-assigned it to me.

ggam commented 6 years ago

Thanks for the summary. I would actually prefer the second option. IMO injecting a proxied view should be the default. Not sure if we really need the Supplier<...> version to get an request without wrapper though.

The supplier would support a way to maintain the same request view across the whole request. I'm also unsure about it. It was more to illustrate the multiple views of the request problem.

However, I guess all this should go into the Servlet or CDI spec. I don't think that JAX-RS should provide these mechanisms. So perhaps creating some JAX-RS owned class which is directly injectable like I proposed above would be a good option for JAX-RS?

Totally agree this doesn't belong to JAX-RS. The discussion was just casual here. But before creating another class for JAX-RS and Servlet integration, I'd prefer to see what the new Servlet project committers think about adopting their CDI producers.

The problem of what request exactly is injected was already there on JAX-RS and @Context as far as I know so I personally see it can wait a bit more to see if Servlet (or a Servlet CDI bridge) finally takes care of it. That way we would get a consistent solution for all Jakarta EE.

chkal commented 6 years ago

The supplier would support a way to maintain the same request view across the whole request. I'm also unsure about it. It was more to illustrate the multiple views of the request problem.

Ok, got it.

Totally agree this doesn't belong to JAX-RS. The discussion was just casual here. But before creating another class for JAX-RS and Servlet integration, I'd prefer to see what the new Servlet project committers think about adopting their CDI producers.

I agree. I would love to see the Servlet API project take care of this. But it looks like there is no activity in the Servlet project yet. So I don't think that this will happen in the near future.

andymc12 commented 6 years ago

Wow! I can't afford to take a day off if I want to keep up with this thread! :) Great conversation!

Regarding @spericas 's concern about deprecation warning messages, I think it is valid but not a show-stopper. The warning messages should occur at compile-time which tells developers that they need to start thinking about the future - and if I'm an app developer, I'd prefer to know earlier rather than later. The message we want to deliver should indicate that CDI injection is replacing @Context and that might mean some changes to the app - if the app was injection @Context object as method parameters, then they need to move to fields. The 2.2 release would still be backward compatible.

Regarding the discussion around HttpServletRequest, I +1 to @chkal 's idea of a ServletEnvironment interface that enables users to get at the initial request - and @Inject HttpServletRequest should inject the JAX-RS-specific/wrapped request.

Thanks!

chkal commented 6 years ago

if the app was injection @Context object as method parameters, then they need to move to fields. The 2.2 release would still be backward compatible.

+1 for this. Currently CDI doesn't allow to inject method parameters, but that shouldn't be a blocker for deprecating @Context. For most types which can currently be injected via @Context, fields make typically more sense. So "dropping" the support for parameter injection for such types with CDI would be fine to me.

Regarding the discussion around HttpServletRequest, I +1 to @chkal 's idea of a ServletEnvironment interface that enables users to get at the initial request - and @Inject HttpServletRequest should inject the JAX-RS-specific/wrapped request.

I think it should be the other way around. We don't control @Inject HttpServletRequest and CDI implementations typically produce the initial (unwrapped) request. If we introduce a JAX-RS owned object which provides access to the Servlet request/response, it should use the fully wrapped instances, which is basically the same as what @Context provides today.

Please note that I'm not sure about the name ServletEnvironment for the new type. It was just a first thought.

ggam commented 6 years ago

Instead of the ServletEnvironment (or whatever it would be named), I'd prefer Arjan's idea of a qualifier, like @Inject @JaxRsRequest HttpServletRequest to get the equivalent of the @Context HttpServletRequest.

I expect the Servlet spec to solve this problem at some point by defining their own qualifiers or a way to get the different stages of the request, so anything we do here would be eventually deprecated. The qualifier idea is more CDI-ish and a bit cleaner IMO.

spericas commented 6 years ago

The reason why I'm not convinced that deprecating @context and forcing a particular way of doing things (like moving injection into fields) is correct is because we haven't decided how we are going to do things in 3.0. Should 3.0 injection targets always use qualifiers? Should we support arbitrary bean injection in params as @arjantijms suggested?

Moving bean injection from params to fields is fine in @requestscoped, but creates implementation difficulties in other scopes like @applicationscoped where fields become thread "demultiplexers", and that creates interesting challenges with @asynchronous. I honestly much prefer the solution suggested by @arjantijms.

But the key point is that this decision has not been made so 2.2 recommendations regarding deprecated warnings may or may not accurate. As a developer, I'd rather change my code only once.

arjantijms commented 6 years ago

The reason why I'm not convinced that deprecating @context and forcing a particular way of doing things (like moving injection into fields) is correct is because we haven't decided how we are going to do things in 3.0.

I see your point, however I do wonder if it really has to be a 3.0 decision. If things remain backward compatible, is there a pressing reason not to do it in 2.2?

If 2.2 specifies a good way to do injection via CDI, then the way I see it there should not be a reason to change it again in 3.0.

Moving bean injection from params to fields is fine in @requestscoped, but creates implementation difficulties in other scopes like @applicationscoped where fields become thread "demultiplexers", and that creates interesting challenges with @asynchronous.

CDI injects proxies in that case, so they'd delegate to the right bean instance. I.e. if a request scoped bean is injected into an application scoped bean, then even with an @asynchronous you'd get the right version. Of course there may be various potential issues, but those are likely shared by asynchronous servlets, asynchronous CDI beans and even asynchronous EJBs.

But the key point is that this decision has not been made so 2.2 recommendations regarding deprecated warnings may or may not accurate. As a developer, I'd rather change my code only once.

+1 we should not officially deprecate anything without having a clear (and lasting) alternative.

andymc12 commented 6 years ago

Moving bean injection from params to fields is fine in @requestscoped, but creates implementation difficulties in other scopes like @applicationscoped where fields become thread "demultiplexers", and that creates interesting challenges with @asynchronous.

CDI injects proxies in that case, so they'd delegate to the right bean instance. I.e. if a request scoped bean is injected into an application scoped bean, then even with an @asynchronous you'd get the right version. Of course there may be various potential issues, but those are likely shared by asynchronous servlets, asynchronous CDI beans and even asynchronous EJBs.

Right, this works today with the @Context on fields in providers (which are singletons by default).

I honestly much prefer the solution suggested by @arjantijms.

Which one? Annotating the entity parameter, but not annotating the context-specific parameters?

What if we just dropped the requirement for @Context on resource methods entirely? Implementors should be able to figure out which parameters are JAX-RS context-specific parameter types without needing the @Context annotation - and should be able to distinguish between those parameters and the entity. I cannot think of many use cases where a user would need to pass things like HttpHeaders or HttpServletRequest, etc. as an entity.

@POST
public Response newEntity(@Context SecurityContext secCtx, @Context UriInfo uriInfo, MyEntity entity) { 

becomes:

@POST
public Response newEntity(SecurityContext secCtx, UriInfo uriInfo, MyEntity entity) { 

But the key point is that this decision has not been made so 2.2 recommendations regarding deprecated warnings may or may not accurate. As a developer, I'd rather change my code only once.

+1 we should not officially deprecate anything without having a clear (and lasting) alternative.

Right, and I think this is the right thread to discuss the future alternative. The disadvantage to putting this discussion off until 3.0 is that then we still have two injection mechanisms until at least version 4.0. I'd rather tackle this sooner rather than later.

Thanks!

spericas commented 6 years ago

I honestly much prefer the solution suggested by @arjantijms.

Which one? Annotating the entity parameter, but not annotating the context-specific parameters?

I meant generally injecting arbitrary beans as params instead of forcing the use of fields. How they are qualified is less important IMO.

What if we just dropped the requirement for @Context on resource methods entirely? Implementors should be able to figure out which parameters are JAX-RS context-specific parameter types without needing the @Context annotation

Yes, that could be a possibility. However, we need to keep in mind that an @Context injection point can also be satisfied by a user-provided ContextResolver. Still possible to do what you suggest, but looking at a method, harder for a developer to spot the param entity.

Right, and I think this is the right thread to discuss the future alternative. The disadvantage to putting this discussion off until 3.0 is that then we still have two injection mechanisms until at least version 4.0. I'd rather tackle this sooner rather than later.

Are you saying that we have a requirement to deprecate before introducing a version that isn't backward compatible? I didn't think that was a requirement (anymore). The goal was for 3.0 to be incompatible right out of the gate and not include support for @Context.

Why don't we shift gears and start the 3.0 discussion on another issue? I suspect that it won't take that long for us to understand the new direction. Just to reiterate, I'm not against deprecation, but I'd be nice to offer the proper recommendation for those that want to start migrating their code.

arjantijms commented 6 years ago

Are you saying that we have a requirement to deprecate before introducing a version that isn't backward compatible?

The question is if there's indeed a backwards compatibility problem or not. From all current proposals I don't see anything that's backwards incompatible, but I might be missing something of course.

From what I see, the new way (using @Inject for artefacts owned by JAX-RS into fields) can happily co-exist with using @Context.

As for method injection, having parameters qualified with CDI qualifiers be injectable by CDI is backwards compatible as well. You'd still have the @Param annotations that are handled by JAX-RS natively, and @Context can still be used as well. There's just an extra option.

If I'm mistaken somewhere, please let me know.

spericas commented 6 years ago

@arjantijms I may be missing your point, but allow me to clarify the context of my question. The incompatible version that I was referring to was 3.0. That version will by definition be incompatible as it will only support CDI. That is, if I have an existing JAX-RS application that uses @Context and no CDI (as CDI is optional today) it will not run in 3.0 without changes. I don't think we want to keep @Context, context resolvers, etc. in 3.0.

ggam commented 6 years ago

@spericas my understanding (and I think the general one) was that we were looking to preserve the JCP style of "deprecation on one release, pruning on the next".

So your proposal is to just remove @Context support on 3.0 without officially deprecating it first? I'd rather prefer lots of IDE warnings about the use of a deprecated annotation than finding my application doesn't work anymore after an upgrade.

spericas commented 6 years ago

So your proposal is to just remove @Context support on 3.0 without officially deprecating it first? I'd rather prefer lots of IDE warnings about the use of a deprecated annotation than finding my application doesn't work anymore after an upgrade.

No, that is certainly not my proposal. I've only stated that I don't think that's a requirement anymore and that before we deprecate we should agree on the guidelines on how code should be migrated.

ggam commented 6 years ago

Thanks @spericas!

arjantijms commented 6 years ago

@spericas

The incompatible version that I was referring to was 3.0. That version will by definition be incompatible as it will only support CDI. That is, if I have an existing JAX-RS application that uses @Context and no CDI (as CDI is optional today) it will not run in 3.0 without changes. I don't think we want to keep @Context, context resolvers, etc. in 3.0.

Okay understood, +1 to that.

I may be missing your point

Well, the point is that we may be able to make a number of changes in 2.2 already. Unless of course someone shows that it's incompatible. Then, indeed, it will have to wait for 3.0. But if in any way possible I hope we can do it for 2.2.

jwcarman commented 4 years ago

I sincerely don't like the idea of requiring CDI for any JAX-RS project. My preferred stack these days is CXF in Spring Boot. It'd be a shame for CXF not to work in Spring Boot anymore since it's not a fully CDI-compliant IOC container.

Perhaps we can just come up with some requirements of how JAX-RS applications are to behave in an IOC container (OSGi, Spring, CDI, etc) and then the provider (CXF in my usual stack) would need to make sure they adhere to those requirements in the most appropriate/idiomatic way for that particular IOC environment.

arjantijms commented 4 years ago

@jwcarman Since you are a Spring user, just wondering if you know. How does Spring MVC (with REST) does this? How does that framework makes sure it can work in OSGi, Spring, CDI, and perhaps Guice and HK2, and maybe even JSF Managed Beans?

jwcarman commented 4 years ago

@jwcarman Since you are a Spring user, just wondering if you know. How does Spring MVC (with REST) does this? How does that framework makes sure it can work in OSGi, Spring, CDI, and perhaps Guice and HK2, and maybe even JSF Managed Beans?

They don't. That's why I don't use SpringMVC and I use JAX-RS.