jakartaee / rest

Jakarta RESTful Web Services
Other
365 stars 121 forks source link

Drop support for @Context injection and related artifacts #951

Open spericas opened 3 years ago

spericas commented 3 years ago

Note that this issue is not about deprecation, but about dropping support, and it is targeted for the next major release. We should review other artifacts related to @Context that need removal.

jamezp commented 1 year ago

What is the intention of CDI for client side? What I mean, is what is the plan to define the expectations of a client outside of a container. For example a JSON writer like:


@Provider
@Produces({ "application/json", "application/*+json", "text/json" })
@Consumes({ "application/json", "application/*+json", "text/json" })
public class JsonBindingProvider implements MessageBodyWriterObject> {

    @Context
    private Providers providers;

    @Override
    public boolean isWriteable(Class<?> type, Type genericType,
            Annotation[] annotations, MediaType mediaType) {
        return MediaType.APPLICATION_JSON_TYPE.isCompatible(mediaType);
    }

    @Override
    public long getSize(Object t, Class<?> type, Type genericType, Annotation[] annotations,
            MediaType mediaType) {
        return -1L;
    }

    @Override
    public void writeTo(Object t, Class<?> type, Type genericType, Annotation[] annotations,
            MediaType mediaType,
            MultivaluedMap<String, Object> httpHeaders,
            OutputStream entityStream)
            throws java.io.IOException, jakarta.ws.rs.WebApplicationException {
        try (Jsonb jsonb = getJsonb(type)) {
            entityStream = new DelegatingOutputStream(entityStream) {
                @Override
                public void flush() throws IOException {
                    // don't flush as this is a performance hit on Undertow.
                    // and causes chunked encoding to happen.
                }
            };
            entityStream.write(jsonb.toJson(t).getBytes(getCharset(mediaType)));
            entityStream.flush();
        } catch (Throwable e) {
            throw new ProcessingException(Messages.MESSAGES.jsonBSerializationError(e.toString()), e);
        }
    }

    private Jsonb getJsonb(Class<?> type) {
        ContextResolver<Jsonb> contextResolver = providers.getContextResolver(Jsonb.class, MediaType.APPLICATION_JSON_TYPE);
        Jsonb delegate = null;
        if (contextResolver != null) {
            delegate = contextResolver.getContext(type);
        } else {
            delegate = JsonbBuilder.create();
        }
        return delegate;
    }
}

What is expected of the client? Using @Inject would require booting a CDI container which is pretty heavy for a client outside of a container.

rmannibucau commented 1 year ago

Nothing prevents the jaxrs container to handle inject annot as context one if cdi is not there. That said jaxrs never defined well the standalone case - not enough to be portable - so can stay an impl thing.

For the record: your sample code misses jsonb.close when built from there, so ensure to not use it like that in real.

jamezp commented 1 year ago

Nothing prevents the jaxrs container to handle inject annot as context one if cdi is not there. That said jaxrs never defined well the standalone case - not enough to be portable - so can stay an impl thing.

Yes, implementations can definitely handle it. It would just be nice to have some clarification if that is expected to work or not.

For the record: your sample code misses jsonb.close when built from there, so ensure to not use it like that in real.

It's closed with in the try-with-resources :)

rmannibucau commented 1 year ago

It's closed with in the try-with-resources :)

Not really, instance should stay for the app lifecycle otherwise you loose most of the optims, but it is another story ;).

OndroMih commented 1 year ago

What is the intention of CDI for client side? What I mean, is what is the plan to define the expectations of a client outside of a container.

Jakarta Batch, for example, defines a simple injection if not in a CDI container, which mandates that Batch components are injected with @Inject by the Batch container in that case: https://jakarta.ee/specifications/batch/2.1/jakarta-batch-spec-2.1.html#field-injection-in-batch-managed-artifact-instances

Jakarta REST could define similar behavior. So anything that can be injected now with @Context would be injected with @Inject.

jamezp commented 1 year ago

What is the intention of CDI for client side? What I mean, is what is the plan to define the expectations of a client outside of a container.

Jakarta Batch, for example, defines a simple injection if not in a CDI container, which mandates that Batch components are injected with @Inject by the Batch container in that case: https://jakarta.ee/specifications/batch/2.1/jakarta-batch-spec-2.1.html#field-injection-in-batch-managed-artifact-instances

Jakarta REST could define similar behavior. So anything that can be injected now with @context would be injected with @Inject.

The problem is @Context works with method parameters as well as instance fields and constructor parameters. The latter two are okay for CDI, but method parameters present a problem. You can see some details described here https://www.eclipse.org/lists/rest-dev/msg00035.html.

I think we need a solid definition of what is expected in an EE container vs in an SE container (SeBootstrap) vs a standalone client. Like what happens with interceptors? In a container CDI handles that, in a standalone client I'm not sure what could happen.

rmannibucau commented 1 year ago

Well the point about CDI not supporting method parameter injections is not really true (and to be honest the PR related to that looks as the CDI Lite part, quite off topic). Arquillian did support that since some years (to not say 7+ years), trick is to get the bean manager and resolve the parameter as an injection, works well and is similar to what JAXRS already does for some other parts - like resources themselves in some cases for ex. The good thing is that NOT letting CDI handling that - so not using the PR at all - is that JAXRS will be able to handle itself some injections like the http request - which is NOT the one from CDI but the JAX-RS one - it is quite rare both match.

The no-arg constructor is required mainly for intercepted or normal scoped beans and there is no real way to bypass this without entering in a risky zone (for all impl, build or run) but this is a more general constraint than just CDI so guess this one is ok.

Then for JAX-RS, the SE vs EE is a thing but SE has a ton of limitations like not supporting most injections - except JAX-RS built-in ones so using the JBatch approach which is to limit the field and parameters support sounds already more than ok, in particular when a CDI impl is ~1M - likely less than JAXRS itself - and can be used with SeBootstrap (combined with SeContainer) so at the end the se case can still rely on CDI anyway.

Hope it helps.

jamezp commented 1 year ago

Well the point about CDI not supporting method parameter injections is not really true (and to be honest the PR related to that looks as the CDI Lite part, quite off topic). Arquillian did support that since some years (to not say 7+ years), trick is to get the bean manager and resolve the parameter as an injection, works well and is similar to what JAXRS already does for some other parts - like resources themselves in some cases for ex. The good thing is that NOT letting CDI handling that - so not using the PR at all - is that JAXRS will be able to handle itself some injections like the http request - which is NOT the one from CDI but the JAX-RS one - it is quite rare both match.

How is it not true? I don't see any such requirement in the CDI spec to support method injection short of what is stated in that email.

WRT "The good thing is that NOT letting CDI handling that", that's fine, but is this true for standalone clients? Yes, seems fine, but IMO it needs to be documented these are NOT true CDI beans. In other words things like scopes, decorators, interceptors and the lifecycle in general will not happen. Effectively it would be like a new Foo() would be injected.

The no-arg constructor is required mainly for intercepted or normal scoped beans and there is no real way to bypass this without entering in a risky zone (for all impl, build or run) but this is a more general constraint than just CDI so guess this one is ok.

Then for JAX-RS, the SE vs EE is a thing but SE has a ton of limitations like not supporting most injections - except JAX-RS built-in ones so using the JBatch approach which is to limit the field and parameters support sounds already more than ok, in particular when a CDI impl is ~1M - likely less than JAXRS itself - and can be used with SeBootstrap (combined with SeContainer) so at the end the se case can still rely on CDI anyway.

Hope it helps.

This seems reasonable, but my point is we need to ensure we document that. It's fine to say things like Providers or HttpHeaders are injectable in a standalone client, but I think we need to be clear it's NOT a true CDI bean.

OndroMih commented 1 year ago

Hi @jamezp

How is it not true? I don't see any such requirement in the CDI spec to support method injection short of what is stated in that email.

I'll explain. Before some time, I also thought method injection is a real problem and CDI will need to add support for method injection. However, this isn't true. There's an important difference between Field/Constructor/Setter injection and method injection - the first 3 (Field/Constructor/Setter) are managed by CDI, which creates a CDI bean and resolves the injection points. But methods are executed by the JAXRS container, which maps an HTTP request to a method call, not by the CDI container.

The only 2 cases where CDI calls methods is: @PostConstruct, which AFAIK doesn't support injection, and @Observes, which does support injection. Observer methods are triggered by the CDI container after a CDI event is triggered. But JAXRS methods are triggered by the JAXRS container on a CDI bean, which is very different. The JAXRS container needs to be responsible for injecting method parameters because CDI container has no control about it. The JAXRS container can read annotations on method parameters, call the CDI container to retrieve the objects to inject, and then call the method with CDI beans retrieved from the CDI container. Or JAXRS can provide its own objects, like the HttpServletRequest object and doesn't need to retrieve anything from the CDI container.

For example, here is a pseudo implementation of what the JAXRS can do to call the following method:

Method:

@POST
public String postMethod(@Context HttpServletRequest request, @Context MyBean bean, String body) {
}

How JAXRS could call the method on restResourceBean JAXRS resource CDI bean:

Method method = restResourceBean.getMethod("postMethod", ...);
List<?> injectables = getInjectableParameters(method);
Class<?> bodyType = getBodyType(method);
var body = mapPostBody(bodyType);

List<?> injectableParameters = injectables.stream().map( injectable -> {
  if (canInjectJaxRsObject(injectable {
    return jaxRsObjectFor(injectable);
  } else {
    return CDI.current().select( injectable.getType(), injectable.getQualifiers() );
  }
}.toList();

List<?> arguments = new ArrayList(injectableParameters);
arguments.add(body);

method.invoke( restResourceBean, toArray(arguments));

Note: I used the @Context annotation to mark which arguments should be injected from the context or CDI container, to differentiate them from the argument injected with mapped HTTP body. It's still important to use @Context here because @Inject isn't allowed in method arguments and cannot be used to mark which arguments should be injected besides those that JAXRS already automatically injects. Alternatively, the argument for HTTP body could be marked with a new annotation like @Body or @RequestBody and all others would be injected with usual injection, but this would be backwards incompatible.

rmannibucau commented 1 year ago

Only small note: don't use CDI.current() which would leak if done this way but something like https://github.com/mojavelinux/arquillian/blob/master/testenrichers/cdi/src/main/java/org/jboss/arquillian/testenricher/cdi/CDIInjectionEnricher.java#L59 (beanmanager should be known of jaxrs at that stage) + track creation contexts to release them after method call when not normal scoped, and be it :). CDI offers all the needed API since v1.0.0 for 3rd parties :).

But Ondro is right, the key is the responsability of the call, CDI must only be resp of its own calls not others calls.

jamezp commented 1 year ago

Hi @jamezp

How is it not true? I don't see any such requirement in the CDI spec to support method injection short of what is stated in that email.

I'll explain. Before some time, I also thought method injection is a real problem and CDI will need to add support for method injection. However, this isn't true. There's an important difference between Field/Constructor/Setter injection and method injection - the first 3 (Field/Constructor/Setter) are managed by CDI, which creates a CDI bean and resolves the injection points. But methods are executed by the JAXRS container, which maps an HTTP request to a method call, not by the CDI container.

The only 2 cases where CDI calls methods is: @PostConstruct, which AFAIK doesn't support injection, and @observes, which does support injection. Observer methods are triggered by the CDI container after a CDI event is triggered. But JAXRS methods are triggered by the JAXRS container on a CDI bean, which is very different. The JAXRS container needs to be responsible for injecting method parameters because CDI container has no control about it. The JAXRS container can read annotations on method parameters, call the CDI container to retrieve the objects to inject, and then call the method with CDI beans retrieved from the CDI container. Or JAXRS can provide its own objects, like the HttpServletRequest object and doesn't need to retrieve anything from the CDI container.

Sorry, I do see what you mean and it makes me realize I'm not being very clear :) I do understand that the Jakarta REST implementation is responsible for invoking the method with the correct parameters. It will also be responsible for providing CDI producers so that instance variables and initializer methods.

My only concern here is that it's clearly state what types are always injectable in things like a client. Implementations can always go further than what the specification dictates, but it should show a bias one way or the other on things that could be injected.

A good example is a ContextResolver. I would assume this needs to be replaced by a CDI producer, but that won't really work without a CDI container.

For example, here is a pseudo implementation of what the JAXRS can do to call the following method:

Method:

@POST
public String postMethod(@Context HttpServletRequest request, @Context MyBean bean, String body) {
}

How JAXRS could call the method on restResourceBean JAXRS resource CDI bean:

Method method = restResourceBean.getMethod("postMethod", ...);
List<?> injectables = getInjectableParameters(method);
Class<?> bodyType = getBodyType(method);
var body = mapPostBody(bodyType);

List<?> injectableParameters = injectables.stream().map( injectable -> {
  if (canInjectJaxRsObject(injectable {
    return jaxRsObjectFor(injectable);
  } else {
    return CDI.current().select( injectable.getType(), injectable.getQualifiers() );
  }
}.toList();

List<?> arguments = new ArrayList(injectableParameters);
arguments.add(body);

method.invoke( restResourceBean, toArray(arguments));

Note: I used the @context annotation to mark which arguments should be injected from the context or CDI container, to differentiate them from the argument injected with mapped HTTP body. It's still important to use @context here because @Inject isn't allowed in method arguments and cannot be used to mark which arguments should be injected besides those that JAXRS already automatically injects. Alternatively, the argument for HTTP body could be marked with a new annotation like @Body or @requestbody and all others would be injected with usual injection, but this would be backwards incompatible.

Yes, this makes sense. Like I said above, I'm more concerned with being explicit on what can be injected on the client side where there may not be a CDI container. Like a MessageBodyWriter to send some custom format that uses a ContextResolver to inject some object serializer. What are is expected to happen there if the ContextResolver interface goes away?

spericas commented 1 year ago

Nothing prevents the jaxrs container to handle inject annot as context one if cdi is not there. That said jaxrs never defined well the standalone case - not enough to be portable - so can stay an impl thing.

Yes, implementations can definitely handle it. It would just be nice to have some clarification if that is expected to work or not.

For the record: your sample code misses jsonb.close when built from there, so ensure to not use it like that in real.

It's closed with in the try-with-resources :)

I don't think it was ever the intention to support injection on the "standalone" client side. Yes, some implementation may support this, but it is not portable. It should be the same going forward with CDI.

jamezp commented 1 year ago

I don't think it was ever the intention to support injection on the "standalone" client side. Yes, some implementation may support this, but it is not portable. It should be the same going forward with CDI.

Oh, so there isn't a requirement to use context injection for providers on the client side? I wasn't aware of that, my apologies.

Azquelt commented 11 months ago

I had a question about the injection of AsyncResponse and SseEventSink.

Currently SseEventSink is injected with @Context, but it can only be injected as a resource method parameter. AsyncResponse is injected as a resource method parameter with @Suspended. I'm grouping them together because they both represent a response to be provided asynchronously.

I note the following things:

It might be possible to make these @Dependent scoped beans, but it would be weird. @Dependent scoped beans should inherit the lifecycle of whatever they're injected into, but I think that's not usually going to be the case for SseEventSink or AsyncResponse. Either they outlast the thing they're injected into if it's RequestScoped or they have a shorter lifecycle if it's ApplicationScoped.

I can see two other possibilities:

rmannibucau commented 11 months ago

@Azquelt guess you hit the "i want everything to have a scope" issue, here you don't need to enter in this game and just let the JAX-RS runtime providing its own proxy which will handle the instance lookup accordingly what "context" means for it - it is an implicit scope if you want but compared to a scope which is an user facing thing, here the framework is responsible to make it good for the user implicitly. Now the critical part for this kind of instance - which is unrelated to CDI BTW: how to propagate the context properly since there is no real spec for that today - concurrency one or MP propagation ones do not fill that gap properly. There are multiple options for that but the sanest is probably to enforce to "touch" the instance in the initial context to let the framework capture a snapshot of that and then provide a "message" friendly instance instead of the injected value:

@Inject
private SseEventSink sink;

 @GET
 @Path("events")
 @Produces(SERVER_SENT_EVENTS)
 public void sse() {
     final var propagableSink = sink.start();
     myPool.execute(() -> { /*send()*/ propagableSink.close(); });
 }

You can complain it is not the less verbose but it is the only portable solution, relying on any implicit propagation will require a layer to do that so will prevent any integration with 3rd parties using their own threading (including JRE HttpClient which relies on commonPool even when you set a custom executor for ex), so not sure there is a real choice.

If the API is not "liked" it can that SseEventSink doesnt become a CDI bean and is replaced by SseEventSinkBuilder in CDI land which would semantically be more acceptable (like AsyncContext in Servlet for ex).

Hope it helps.

Azquelt commented 11 months ago

Thanks @rmannibucau, yes I agree that I'd like to avoid any implicit propagation, it always causes a headache. Your suggestion looks like another reasonable solution, it avoids accessing the injected object from the async code altogether, so the question of propagating context doesn't arise.