Handle @*Param annotations with CDI injection

[jim-krueger]

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");
});

[jamezp]

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.

[jim-krueger]

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.

[jamezp]

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]

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]

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.

[mkarg]

+1 for 4.0+
-1 for pre-4.0

[jim-krueger]

+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

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

[jamezp]

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.

[Azquelt]

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.

[jansupol]

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?

[jamezp]

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