rest icon indicating copy to clipboard operation
rest copied to clipboard

Handle @*Param annotations with CDI injection

Open jim-krueger opened this issue 1 year ago • 12 comments

The purpose of this issue is to discuss/document how @*Param annotations should be handled in a Jakarta Rest 3.2 version where both the deprecated @Context injection and straight CDI alignment are supported.

This pertains to the following annotations: @HeaderParam, @CookieParam, @MatrixParam, @QueryParam, and @PathParam

The each of following examples must be handled:

field injection @Inject @HeaderParam("who") private String who;

constructor injection @Inject public SampleResource(GreetBean bean, @QueryParam("lang") String lang) { this.bean = bean; this.lang = lang;

resource method injection @POST @Path("async") @Consumes(MediaType.TEXT_PLAIN) public putMessageAsync( @QueryParam("override") boolean override, @Body String message, AsyncResponse ar) { Executors.newSingleThreadExecutor().submit(() -> { // ... ar.resume("Done"); });

jim-krueger avatar Jan 24 '24 17:01 jim-krueger

I've been making some notes and thinking about this. IMO these annotations need to be CDI qualifiers. It will make it much easier for injection. The one catch is for instance fields they'd need the extra @Inject annotation. Maybe there is a way around that I'm not aware of though.

jamezp avatar Jan 24 '24 17:01 jamezp

On page 4 of https://github.com/jakartaee/rest/blob/release-4.0/JakartaRest40.pdf Santiago stated that the @Inject annotation would need to be used for @*Param annotated fields, but (on page 7) not for parameters. This document also agrees with you James that @*Param's need to now be CDI qualifiers.

jim-krueger avatar Jan 24 '24 17:01 jim-krueger

A couple other notes on this. We also need to note that any of the qualifiers with required attributes, the required attribute needs to be annotated with @Nonbinding.

Another thing we need to consider, for implementations, is how a CDI producer would look like for these qualifiers. It's fairly easy if the return type is a String, primitive or some known type. It becomes tricky, if not impossible, if a ParamConverter is required.

We also need to require the producers for these are @Dependent scoped.

jamezp avatar Jan 25 '24 15:01 jamezp

After thinking about this a bit more, it might actually work with a dynamic producer registered in a CDI extension. We'd need to get the generic type from every ParamConverterProvider, which isn't ideal, but we could likely do that.

jamezp avatar Jan 25 '24 16:01 jamezp

Along these lines, I almost wonder if the ParamConverterProvider should just be deprecated for removal. I don't really know what we gain from it. We could just make the ParamConverter be a CDI bean, e.g. annotate with @Provider, and cut out the nuance of the ParamConverterProvider.

I guess the one caveat would be the client side.

jamezp avatar Jan 25 '24 19:01 jamezp

+1 for 4.0+ -1 for pre-4.0

mkarg avatar Jan 26 '24 19:01 mkarg

+1 for 4.0+ -1 for pre-4.0

Just to be clear Markus, are you voting -1 on annotating ParamConverterProvider with @Deprecated(forRemoval = true) for the proposed release 3.2 (pre-4.0) release, or is there more to that vote? Thanks

jim-krueger avatar Jan 27 '24 17:01 jim-krueger

I am +1 for deprecating it in 3.x and removing it in 4.0. I am -1 for removing it in 3.x already, as that would break backwards compatibility.

mkarg avatar Jan 27 '24 18:01 mkarg

I am +1 for deprecating it in 3.x and removing it in 4.0. I am -1 for removing it in 3.x already, as that would break backwards compatibility.

Thank you for the clarification. This makes a lot of sense to me as well.

jamezp avatar Jan 27 '24 18:01 jamezp

I note that with the way ParamConverterProvider is defined, it's not possible to enumerate all of the types that a converter has been registered for.

However, I think converters registered with ParamConverterProvider could still be supported by having a CDI extension which collects all of the different types used by injection points which have @*Param, and then creates a synthetic bean for each of them.

We used a similar approach to allow injection of config values with arbitrary types in MP Config.

Azquelt avatar Jan 29 '24 17:01 Azquelt

If I recall, that would work for beanDiscoveryMode="all", or for CDI beans, i.e. @Path would be needed to be a cdi bean defining annotation. Is that planned for 3.2?

jansupol avatar Feb 12 '24 15:02 jansupol

@jansupol That would be my personal plan, yes. I'm going to propose https://github.com/jamezp/jaxrs-api/compare/cleanup...jamezp:jaxrs-api:cdi-annotations once https://github.com/jakartaee/rest/pull/1211 is merged. We just need one more approval for that if you'd like to give it a review :)

jamezp avatar Feb 12 '24 15:02 jamezp