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

Drop support for @Context injection and related artifacts #951

Open
spericas opened this issue Apr 12, 2021 · 66 comments
Open

Drop support for @Context injection and related artifacts #951

spericas opened this issue Apr 12, 2021 · 66 comments
Milestone

Comments

@spericas
Copy link
Contributor

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.

@spericas spericas added this to the 4.0 milestone Apr 12, 2021
@jamezp
Copy link
Contributor

jamezp commented Jan 19, 2022

What is the intention for something like:

@GET
@Path("eventStream")
@Produces(MediaType.SERVER_SENT_EVENTS)
public void eventStream(@Context SseEventSink eventSink,
                        @Context Sse sse) {
    executor.execute(() -> {
        try (SseEventSink sink = eventSink) {
            eventSink.send(sse.newEvent("event1"));
            eventSink.send(sse.newEvent("event2"));
            eventSink.send(sse.newEvent("event3"));
        }
    });
}

Injecting the SseEventSink as a parameter is used in all the TCK tests and examples. I've personally only ever seen SseEventSink injected as a method parameter.

It would likely require large refactoring for users to remove parameter injection of @Context. Not that I'm opposed to using CDI and deprecating @Context and ContextResolver. I just worry about the removal for the migration issue.

P.S. I wasn't sure if I should have commented this here or on #569, but I opted for here only because it's newer :)

@spericas
Copy link
Contributor Author

@jamezp It would be injectable as a normal CDI bean without the need to use @Context. Any CDI bean shall be injectable in param position on a resource method, in fact.

@jamezp
Copy link
Contributor

jamezp commented Jan 19, 2022

@spericas Kind of what I assumed, but just wanted to verify thank you. I'm working on this for RESTEasy which seems like for full support is going to take some refactoring. However, that is not a spec issue :)

@rmannibucau
Copy link

Can you confirm you intend to break existing applications at two levels:

  1. Force users to move from context to inject
  2. Prevent users to distinguish between CDI and JAXRS beans without a custom qualifier (since currently context is considered as a classifier in several/most applications so you can have @Inject Foo foo; and @Inject @Context Foo foo - with implicit or not @Inject)
  3. Not be able to distinguish anymore what is a context from a payload in POST/PUT/... methods - guess you intend to fail somehow?

On my side I don't understand the gain to break something which works so I would request to maybe not do it to avoid people to have to rewrite their apps.

@arjantijms
Copy link
Contributor

On my side I don't understand the gain

The gain is to go to a single component model, and not have 6 different ones, which all have their quirks in interaction and all have to be maintained separately. Faces moved to CDI in this release, and there's a long standing wish toi phase out EJB beans in favour of CDI compatible alternatives.

@rmannibucau
Copy link

@arjantijms I get this point but can't you get it without any breaking change defining @Context as a qualifier? Will keep existing code working and unify the model anyway. So proposal is to:

  1. deprecate @Context injection wtihout @Inject for fields/methods,
  2. define @Context as being a qualifier when ran in a CDI container (SE or EE)
  3. keep parameter injections as it is

This makes everyone happy (no code to rewrite, CDI fully embraced - actually it is what most JAXRS containers already do in CDI integrations).

@arjantijms
Copy link
Contributor

Redefining Context as qualifier might be an option to consider.

We did do this once with jakarta.faces.convert.FacesConverter, which was made retroactively a qualifier. It wasn't without its own issues though, although the mistake we made there was perhaps not switching over completely (it supports converters being CDI and old-style at the same time).

@rmannibucau
Copy link

A side note is that CDI embracement is also about not keeping SeBootstrap unreleased class since defining CDI integration in JAXRS (scanning etc) solves wat SeBootstrap tries to do in a clumsy way since it requires users to either be fully explicit or reimplement the scanning + it does not promote what all implementations had been doing (cause it fits user requests) plus it has some other pitfalls like not being well integrated with the container since it is not linked to servlet container (only http layer in EE) to control the bindings. SE API already failed in other specs (thinking strongly to JAXWS Endpoint) so not sure current API SNAPSHOT is great so I think this issue should be part of an epic "CDI integration" which would mean:

  1. Ensure it is integrated with CDI at API level - previous proposal,
  2. Ensure it is integrated with CDI at scanning level (mainly define the CDI extension behavior to find providers, endpoints, features, application, ...) and start the server. Here it should work well and transparently with CDI SE API as well as CDI EE API when in an EE container (which would just get more scanning rules to stay backward compat but the minimum should work in both env making code way more reusable).
  3. Ensure beans can be CDI instances and not just injectable instances (it is not well defined currently IIRC), including the scope delegation (whatever scope should work, not only requetscoped or singleton/appscoped)
  4. (optional) define that an event is fired when the server starts/stops (to know endpoints are available or not)

Guess it would be the main integrations which would enable to run JAXRS in all env without learning a new programming model. Not sure how epics are managed for this project but feel free to promote as such that if it makes sense for you.

@spericas
Copy link
Contributor Author

spericas commented May 12, 2022

Redefining @Context as a qualifier isn't going to make the new Jakarta REST compatible. There are other significant differences (resource method parameters, context resolvers, entity params, etc.) that have been proposed and discussed. Moreover, making something that looks syntactically compatible but is semantically different is even worse (e.g, @Context can not be used in parameter position).

I understand that dropping backward compatibility has some cost, but Jakarta REST has been compatible for a decade and some of the initial decisions would have been different had CDI been matured at the time. Implementations that support the existing API will remain available for a long time for those that don't want to migrate.

@spericas
Copy link
Contributor Author

@rmannibucau On the topic of scanning, absolutely, CDI scanning should be part of the SE proposal.

@rmannibucau
Copy link

@spericas well, if you want to enforce all JAXRS component to be CDI beans then you break 100% of JAXRS applications (most of features, body readers/writers etc are standalone instances and not even managed by CDI - which is good in general in terms of design for the application but also for libraries writers).
To make it CDI compatible you don't have to break anything, just keep the integration with 3rd party libraries smooth and easy so probably don't drop @Context support without @Inject for not managed beans - this is the part to spec properly since it is undefined. Supporting @Inject @Context is a good move forward for CDI beans but this one requires @Context to be a qualifier.
So overall the logic would be:

  1. if the instance is a cdi bean, only @Inject @Context are respected (which implies CDI bean instances are explicitly supported, yeah :))
  2. if the instance is a standalone bean only @Context are respected

This is exactly what jbatch does with quite some success. Moving to a full CDI model had been evaluated but was not judged realistic for end users and 3rd parties and I tend to think it would be the same for JAXRS.

@arjantijms
Copy link
Contributor

if the instance is a standalone bean only @context are respected

It's best not to have that confusing difference.

People also thought in 2003 not everything could be a spring bean in Spring, but they hold on to that concept and it worked quite well for them.

For Faces, people also thought it could not use CDI exclusively, and there we are; using CDI exclusively. And for Jakarta Security, the same was thought; invent a semi component model that abstracts from CDI, since who knows, some person, somewhere out there, might want to use Jakarta Security with Python, and doesn't want to use CDI. But it too uses CDI exclusively making everything a lot simpler.

Don't forget that Jakarta EE doesn't have even close to the resources it had before. Maintaining large abstraction framework over things that are essentially abstract themselves, who is going to maintain that?

@rmannibucau
Copy link

Not sure it is true, most jsf components are still not cdi beans and making it beans slows down things and makes the app consuming way more mem.
Same for spring.
Whatever you want, the way jaxrs is defined it has its own ioc so handling standalone is cheap and easy to understand for the consumers i mentionned.
If you want to align it on cdi you should drop jaxrs filters, interceptors etc for the same argument/point so jaxrs becomes cdi centric and way lighter by dripping redundant concept.
While I can see benefit to that it would be a huge breaking change but not doing it does not solve the duplication and ambiguities IMHO.

@arjantijms
Copy link
Contributor

Not sure it is true, most jsf components are still not cdi beans

Faces UIComponents are not CDI beans, but I was referring to the managed beans (which are mostly used for backing beans). The own native managed bean / IOC facility of Faces has just been pruned and it's gone.

Internally several things are managed by CDI already, and there's more on the agenda. See e.g. https://arjan-tijms.omnifaces.org/p/jsf-23.html#cdi

and making it beans slows down things and makes the app consuming way more mem.

What is the slow down exactly? And how much more memory does it consume? Do you have any benchmarks related to this?

Whatever you want, the way jaxrs is defined it has its own ioc so handling standalone is cheap and easy to understand for the consumers i mentionned.

Is that really the case? Now Jersey for instance uses HK2 internally to handle the bean model and injection. Going forward that could be e.g. Weld. Why is Jersey using HK2 internally easier to understand than using Weld Internally?

@mkarg
Copy link
Contributor

mkarg commented May 14, 2022

I'd like to propose to separate discussions into several issues, hence discuss replacing the annotation @context by the annotation @Inject separately from enforcing the use of CDI, separately from how to deal with SeBootstrap shortcomings. There might be interrelations, but I assume it makes finding a consensus much easier.

@rmannibucau
Copy link

Agree but it also needs an "epic" to coordinate all of them with consistency otherwise we would end up with local solutions and no global one IMHO.

@mkarg
Copy link
Contributor

mkarg commented May 14, 2022

Thank you for this idea, but actually I think the JAX-RS Committers have proven since years that they are pretty able to decide on their own how they organize their project.

@rmannibucau
Copy link

@mkarg exactly, this is why I asked how governance would be handled to avoid errors on this topic but it is really up to you to make it specified (my only request is no breaking change nor new useless api as an user).

@spericas
Copy link
Contributor Author

Remaining backward compatible is definitely not a goal for 4.0. In fact, we have been trying to better align with CDI for a long time. Keeping support for @Context and all that implies would be for a new 3.X release, should the need for that arise.

@rmannibucau
Copy link

@spericas well since alignment is not done it can be added but the questions are "does it need to break" on one side and on ther other side "is the standalone feature used and needed". All applications I used were answering "no" and "yes" and with some good background so I think it can just be a clarification of CDI integration and not a restatement of the basis which would better fit a new spec where half of JAXRS concepts are dropped cause overlapping with CDI.
To be super clear: you can break context support but if you really base JAXRS on CDI you drop all the JAXRS chains and half of the components/models/API so I humbly request a quick and dirty integration is not done if the goal is a deep integration but a full restart in a new spec to avoid ambiguity OR keep it compatible and aligned on the mainstream usage (way cheaper and likely sufficient for 99% of apps IMHO).

side note: microprofile played this "backward compatible is not a goal" but they lost a lot of users after very few years due to that and one of the biggest quality of JavaEE was to be backward compatible long enough regarding project life time so I hope it stays a fundation of JakartaEE too.

@spericas
Copy link
Contributor Author

@rmannibucau I'm sorry but I cannot disagree more with your assessment of MP. In my view, Jakarta EE is the one at risk here if it continues to rely on old specs and does not bring better integration and innovations into the platform soon. As I stated before, if backward compatibility is essential, 3.X will continue to be supported by vendors for a long time.

@rmannibucau
Copy link

@spericas you assume moving forward means breaking. It implies that you take microprofile path, ie break each year applications enforcing coding for just upgrades (including security upgrade). This is a very bad experience for end user IMHO and technically not justifiable. Here, the topic is to specify the CDI integration in JAXRS spec, it is a relatively easy task - since all vendors did it almost the same way - and does not require any breaking change. If you want to break, you can indeed, but as explained, you will either take the MP path and make your user paying for no feature gain OR you will create a new spec so incrementing the version is irrelevant. The issue of EE is not relying on old specs (@Inject will likely stay for way more years without any issue) but to keep embracing new features properly - here I agree with you EE should keep up, for example defining what happens on a record, a better integration of Flow etc...nothing related to any breaking change, debt in JAXRS is not yet high enough to justify to break from the ground and I suspect it would go through another spec as explained.

@OndroMih
Copy link

OndroMih commented May 16, 2022

Hi, my view on this is sort of mixed but practical. I've always had problems with @Context injection, which works completely on its own and doesn't even rely on plain @Inject like Batch injection does. On the other hand, I don't see much value in dropping @Context if it only means more work on the spec to remove it from the API, docs and tests. The only benefit for removing I see is that new potential implementations wouldn't need to implement it. For that sake, we can just make it optional and deprecated.

I would rather first focus on the replacement - to fully specify how CDI works in JAX-RS, how method arguments are injected (AFAIK this isn't supported by @Inject and CDI), and everything that would replace @Context.

Once we're done with that, we can drop @Context. Batch did it this way, improved CDI integration without dropping anything and left all removals for later. Faces defined CDI integration first with @Named and CDI scopes and only later dropped managed beans. But Faces now suffer a bit because a lot of tests will need to be removed or modified in the Jakarta TCK to align it.

@bmarwell
Copy link

Last comment sounds like a good idea.
Maybe stretch dropping over a few releases. Yes, this means YEARS.
Maybe like this?

  • the next major version should issue a warning
  • the major version after that should rework context to be a qualifier
  • only then in the again next major version remove it completely.

Just my 2¢ because I agree with all of you: why break something for users which has been working like this forever? But then, don't hold on to old standards?

From a user's perspective (I use OpenLiberty a lot), jaxrs-x.0 will pull in servlet-y.0. Maybe some day servlet 3.0 will not be available anymore. Or I want to update step by step because (you got it:) big companies. And at some point you have to update.

So, if you need to break it, either give it a new name and artifact, or do it over multiple releases/years as gracefully as possible.

Thanks. 😉

Oh, and thanks everyone for the good work and being a part of this. Even comments and opinions in the discussion matter a lot! 🙏🏻

@arjantijms
Copy link
Contributor

how method arguments are injected (AFAIK this isn't suppoeted by @Inject and CDI),

Well... actually it is. It's just that the parameters don't need to have the @Inject annotation applied to it. Producers and constructors use this all the time.

For instance:

@ApplicationScoped
public class ApplicationInit {
    
    private static final Logger logger = Logger.getLogger(ApplicationInit.class.getName());
    
    @Produces
    public HttpAuthenticationMechanism produce(InterceptionFactory<HttpAuthenticationMechanismWrapper> interceptionFactory, BeanManager beanManager) {
        .. 
    }
}

Here, the interceptionFactory and beanManager parameters are not part of any method signature and can be declared in any order, and any amount of parameters can be specified here. CDI will inject them as required, exactly as Jakarta REST does.

That said, we do have an issue open in CDI to make this a bit more convenient to use for arbitrary methods:

jakartaee/cdi#460

@arjantijms
Copy link
Contributor

arjantijms commented May 16, 2022

Maybe stretch dropping over a few releases. Yes, this means YEARS.

In a way dropping @Context has already been in progress for give or take 13 to 14 years. Yes, that process already started before JAX-RS was released in 2009.

Somewhere around 2008 @gavinking noticed that the JAX-RS team was duplicating the efforts of the CDI team and went out to talk to them. It has been lost in history what exactly transpired in that talk, but shortly before the release of EE 6 a couple of amendments were made to provide some level of integration between the JAX-RS component model and the CDI one.

@rmannibucau
Copy link

@arjantijms your example is great to complete the reaons why @Context is needed. You example shows that @Inject can be omitted when implicit - your example is a CDI managed method where all parameters are CDI beans. JAX-RS is not that since parameters could come from CDI but they can also come from JAX-RS so it is not comparable, in particular with subresources or context instances which can be different from CDI instance (think to JSON-B instance for a trivial case but security and other transversal cases will get more complex usages of such a thing - multiple instances of the same bean "typed" in DI only through a qualifier).

The reason JAX-RS does not use CDI is because making it easy to run with another IoC always had been a goal which also led to creating the JAX-RS chains (filters, interceptors, etc...). All that is useless if you are based on CDI except @Context to remove the injection ambiguity as explained earlier so really think context is not the right fight for any coming release since best case it would be replaced by @Qualifier @interface Context {} which would be 1-1 for end users in all cases but the field/setter injections where @Inject would become required until CDI supports implicit inject for qualifiers - AFAIK it is more or less pushed by quarkus to become standard but some implementations already support it - quarkus/openwebbeans AFAIK.

@jamezp
Copy link
Contributor

jamezp commented Jun 15, 2022

I'm not too sure where the best place for this comment is, but given the spec would like to align more with CDI injection it seems we should add @Inherited to the annotations. Per the CDI spec:

Inheritance of type-level metadata by beans from their superclasses is controlled via use of the Java @Inherited meta-annotation. Type-level metadata is never inherited from interfaces implemented by a bean.

This seems related to #539 and possibly to #968.

@rmannibucau
Copy link

@jamezp JAXRS-CDI integration already handles that AFAIK (only some implementations doing reflection on proxy without unwrapping have bugs but it is out of spec), @Inherited meaning seems unrelated to the linked issues and means something else so not sure it would help.

@jamezp
Copy link
Contributor

jamezp commented Jun 15, 2022

@jamezp JAXRS-CDI integration already handles that AFAIK (only some implementations doing reflection on proxy without unwrapping have bugs but it is out of spec), @Inherited meaning seems unrelated to the linked issues and means something else so not sure it would help.

@rmannibucau I guess I was thinking more of the user, but maybe that doesn't matter. For example:

@Inject
MyApplication application;

public String getContextPath() {
    final ApplicationPath ap = application.getAnnotation(ApplicationPath.class);
    return ap == null ? "/" : ap.value();
}

Essentially anything the user injects. Granted, I could see a change like this as a backwards incompatible issue. However, if we're trying to move towards CDI then it seems like a reasonable expectation.

I came across this implementing CDI support for the SeBootstrap instance in RESTEasy and was just a bit surprised to see the annotations were not inherited.

@rmannibucau
Copy link

@jamezp it only partially solves the issue since you can get the exact same with most frameworks, even CDI depending how you setup/get the proxy. A CDI way to get this information is an extension or using the annotated type of the related class (MyApplication). Generally speaking, in CDI you should never do java.lang.reflect (application.getClass() in your example I assume) but always use CDI metamodel, this is why I think it is another issue.

@jamezp
Copy link
Contributor

jamezp commented Jun 15, 2022

Okay, I apologize for the noise. This sounds like a user (me :)) error then.

@rmannibucau
Copy link

@jamezp it is a common one, can makes sense to emphasis it in CDI spec/doc ;)

@ArneLimburg
Copy link

ArneLimburg commented Jun 27, 2022

What about this proposal:
Deprecate the usage of JAX-RS specific Classes/Interfaces (I mean all that can be injected via @context) as HTTP body.

Once this is done, we can simply recognize the Request body from every List of method paramters without using @context).

After that we could finally remove @context without really breaking any app. Field injection then would be via @Inject and parameter injection by type (to distinguish from request body). Apps, that have @context in their code then can add an api dependency to compile it, at runtime it will simply be ignored.

@rmannibucau
Copy link

Context being custom injections too, not only readers/writers and others, it means spec must support cdi param injections so leads back to some param ambiguity no?

@OndroMih
Copy link

Reading the history of this issue, it seems that many different things were discussed here:

  • whether JAX-RS should keep support for running outside of CDI container and thus keep support for injecting non-CDI context beans with @Context
  • whether we should drop @Context or keep API backwards compatibility, replacing the current @Context injection semantics with CDI semantics
  • if we decide to drop @Context, whether to delay it a bit, first releasing a REST version that allows using plain CDI and @Inject to replace @Context, and drop @Context in a later release

When we discuss all these in this issue, we should all understand that these are different issues and we can decided to do one or more, it's not all or nothing.

@OndroMih
Copy link

Ad. JAX-RS should keep support for running outside of CDI container

I think it was already decided in the platform level that there should be better integration with CDI and CDI should replace all other injection mechanisms if possible, even if it means that it won't be possible to use EE components outside of EE without loss of functionality. This vision has been adopted by e.g. Faces and Batch already. They both can be used outside of CDI. But CDI is either required for injection (in Faces), or a simple injection is supported without CDI (Batch). It could would be possible to use REST in Spring or plain Java SE even without injection, just context injection wouldn't work. We can provide a different mechanism to retrieve the context in Java SE, which I envisioned here for client API but it would work equally well for server: #953 (comment)

Ad. whether we should drop @Context or keep API backwards compatibility

I believe we don't have to break existing applications by dropping the @Context completely. We can just define that it's supported only in a CDI container and how it works in the CDI container. All contexts that it can inject could be provided via CDI producers or extension as CDI beans. We can turn @Context into a qualifier to allow @Inject @Context Configuration config. Via a CDI extension, I think it's even possible to inject context without @Inject, in keeping backwards compatibility with the existing code. The ProcessAnnotatedType event should allow adding a virtual @Inject annotation everywhere next to the @Context annotation if it's not already present.

Ad. Delay dropping of @Context

There were many breaking changes in Jakarta EE 9 and 10 already. Dropping @Context would have significant impact on existing code, because I believe most of the JAX-RS apps use it at least a few times, and there might be libraries that also rely on it, which developers can't just change and would have to wait for their maintainers to update. So I'd like to avoid this in Jakarta EE 11, otherwise users would face significant breaking changes basically in every Jakarta EE release, making them tired of upgrading to Jakarta EE versions. We first implement CDI alternatives for injecting context in all places where @Context is supported, then create a release that allows using both CDI and @Context to inject the context, and later drop @Context. This would allow users to upgrade without breaking their applications, try out the new CDI way, but still be able to revert back to @Context if needed, until the next REST/Jakarta EE release that drops it.

With the previous point which allows complete transition to CDI and droping custom DI in REST, I think there's no desperate need to drop @Context and we could keep it for a long time, at least until we really need to introduce another breaking change which cannot be avoided. @Context would continue to be just an alias to @Inject.

@spericas
Copy link
Contributor Author

If someone comes up with a detailed proposal in which @Context can be kept and makes sense, we should consider it. I'm not sure that proposal exists, and as usual the devil is in details. E.g., @Context can be used in parameter position and that is a real problem to align with CDI and the new proposal for executable methods.

With tighter CDI integration, keeping @Context around will likely create even more confusion, which is the very thing we are trying to eliminate as part of this effort.

@rmannibucau
Copy link

E.g., @context can be used in parameter position and that is a real problem to align with CDI and the new proposal for executable methods.

Can you detail how it is a problem? JAXRS method parameters are handled by JAXRS runtime and CDI supports method parameters lookup - it is used by some libs and CDI itself.

Only real point is backward compat IMHO.

@jamezp
Copy link
Contributor

jamezp commented Jun 30, 2023

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

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

jamezp commented Jun 30, 2023

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

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

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

jamezp commented Jul 3, 2023

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

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

jamezp commented Jul 3, 2023

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

OndroMih commented Jul 3, 2023

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

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

jamezp commented Jul 3, 2023

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

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

jamezp commented Jul 14, 2023

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

Azquelt commented Nov 28, 2023

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:

  • They're tied to the request, they're not valid when there isn't a current request and there's only one per request
  • If you're injecting it, you need to know what the current request is. If you're within or are preparing to call the resource method, that's obvious, but if you've spun off a task to run asynchronously then it may not be.
  • They're meant to be used on other threads, so they can't be RequestScoped, the request scope is only active on the original request thread while the resource method is being called

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:

  • Define your own scope for these beans.
    • You then need to always be able to find the current AsyncResponse or SseEventSink, even when you're running some time later in an asynchronous task. This probably means specifying that some information is propagated to any async task started from a request thread to keep track of what request that task is associated with. This is a little tricky since RestfulWS doesn't depend on Jakarta concurrency and there are a lot of other asynchronous mechanisms used by different runtimes.
  • Retain the existing restriction that these objects can only be injected as a resource method parameter and have that injection be handled by RestfulWS directly, rather than delegated to CDI as for other objects previously injected with @Context.

@rmannibucau
Copy link

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

Azquelt commented Nov 29, 2023

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.

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

No branches or pull requests

10 participants