Support constructor injection for RESTEasy Reactive multipart bodies

[andreas-eberle]

Description

When using Jackson with RestEasy Reactive, normal method bodies can be POJOs that initialize the fields via the constructor. For Kotlin this works with the dependency com.fasterxml.jackson.module:jackson-module-kotlin and the body class can look like this:

data class RequestDto(
    val iapProductId: String,
    val iapPlatformToken: String
)

Unfortunately, for the @MultipartBody objects, this does not work. There, you have to have a default constructor.

Please also implement the constructor creation for the @MultipartBody objects.

Implementation ideas

No response

[quarkus-bot]

/cc @FroMage, @evanchooly, @geoand, @manovotn, @mkouba, @stuartwdouglas

[geoand]

Is this still an issue?

[FWest98]

This is still an issue on 3.7.1

[geoand]

Thanks for checking

[geoand]

This is more of a note to myself for a possible way to approach this.

The problem is that we have ASM code that transforms the @BeanParam target class to create the __quarkus_rest_inject method that then proceeds to fill in all the fields.
Changing that code is hard, and I'd like to avoid it all costs, so I was thinking that we might able to use this code on an auxiliary class that would also have a "create" method which would use the populated fields to call the constructor of the actual class.

[FroMage]

The example above is not a multipart body, is it? It's a JSON body. There are no @FormParam anywhere.

[geoand]

Regardless, the problem of not being able to use Java Record for multipart and for beanparam remains

[FroMage]

Yes, agreed, perhaps. But this doesn't appear what this issue is asking, though.

Does CDI injection work with records?

[geoand]

Yes, agreed, perhaps. But this doesn't appear what this issue is asking, though.

Isn't it? OP explicitly mentions Multipart

[geoand]

Yes, agreed, perhaps

Also, why perhaps? What's more reasonable about making users write less boilerplate via a Java record instead of a typical class with fields?

[FWest98]

So to provide a complete code example:

data class ThisWorks(
    val foo: String
)

data class ThisDoesNotWork(
    @RestPath("foo")
    val foo: String,

    @RestForm(FileUpload.ALL)
    val files: List<FileUpload>
)

@POST
@Consumes(MediaType.APPLICATION_JSON)
fun thisWorks(data: ThisWorks): Uni<RestResponse<...>>

@POST
@Consumes(MediaType.MULTIPART_FORM_DATA)
fun thisDoesNotWork(data: ThisDoesNotWork): Uni<RestResponse<....>>

A workaround for the second data class is to apply the kotlin noarg compiler plugin, register an annotation to cause a class to generate a no-arg constructor, and then write the class like this:

@NoArgConstructor
data class ThisDoesNotWork(
    @field:RestPath("foo")
    val foo: String,

    @field:RestForm(FileUpload.ALL)
    val files: List<FileUpload>
)

[FroMage]

Isn't it? OP explicitly mentions Multipart

Well, it was missing an example showing multipart usage.

Also, why perhaps? What's more reasonable about making users write less boilerplate via a Java record instead of a typical class with fields?

It is reasonable, for sure. But, I'm not entirely sure that it's easier to write:

public record DoesNotWork(
 @RestPath String foo, 
 @RestForm(FileUpload.ALL) List<FileUpload> files) {}

Than:

public class Works {
 @RestPath public String foo; 
 @RestForm(FileUpload.ALL) public List<FileUpload> files;
}

So, sure, we should support it. But it's not like the workaround is horrible or harder to write.

It's a good thing I wrote a bunch of docs on how this is handled: https://github.com/quarkusio/quarkus/wiki/RESTEasy-Reactive-(Server)

[andreas-eberle]

Hello together,

for me, using records or Kotlin data classes with constructor injection is also about clean code by using final fields or val in Kotlin.
Using these, you can ensure the values of a data transfer object are not changed in some random place in the code. Instead, you have a non-mutable object you can pass around.

Similarly, it is not so nice to have public/protected mutable fields in a bean where every user could meddle with its internal workings. Instead, when using constructor injection, all is sealed inside the class and only the interface intended to be used is public.

On top, records/data classes provide additional benefits like toString etc.

[FroMage]

I am so glad I documented this 😂

Now, IIRC the reason why we didn't go with constructor creation and usage was because we wanted to be able to inject our stuff into beans that we got via CDI, so that CDI could create the bean (making all @Inject points work), and we'd fill the remaining fields.

There was no API to call CDI injection if we created the bean via our generated constructor. Perhaps this has changed. If we support records, I'm not sure how CDI injection will work.

I doubt we can create a record via CDI, for the same reason as us, but perhaps I'm wrong.

Now, if we do not care about CDI injection, we can change the pattern of our injected type from:

public class RegularBean implements ResteasyReactiveInjectionTarget {
 	@Override
	public void __quarkus_rest_inject(ResteasyReactiveInjectionContext ctx) {
		// if our supertype is injectable
		//	  super.__quarkus_rest_inject(ctx);

		// a regular field with no converter
		try {
			Object val = ctx.getFormParameter("regular", true, true);
			// if we have a default value
			if(val == null) {
				val = "default";
			}
			if(val != null) {
				regular = (String)val;
			}
		} catch (WebApplicationException x) {
			throw x;
		} catch (Throwable x) {
			throw new BadRequestException();
		}

		// another bean param class
		otherBeanParamClass = new OtherBeanParamClass();
		otherBeanParamClass.__quarkus_rest_inject(ctx);
    }
}

Into something like:

public class Record { // no supertype, we must rely on generated static method
	public static Record __quarkus_rest_inject(ResteasyReactiveInjectionContext ctx) {
		// a regular field with no converter
                String regular = null;
		try {
			Object val = ctx.getFormParameter("regular", true, true);
			// if we have a default value
			if(val == null) {
				val = "default";
			}
			if(val != null) {
				regular = (String)val;
			}
		} catch (WebApplicationException x) {
			throw x;
		} catch (Throwable x) {
			throw new BadRequestException();
		}

		// another bean param class if it's a regular bean
		OtherBeanParamClass otherBeanParamClass = new OtherBeanParamClass();
		otherBeanParamClass.__quarkus_rest_inject(ctx);

		// another bean param class if it's a record
		OtherBeanParamClass otherBeanParamClass = OtherBeanParamClass.__quarkus_rest_inject(ctx);

                // power rangers assemble
                return new Record(regular, otherBeanParamClass);
    }
}

So, for each annotated field, we declare a local variable instead of assigning the field, until we have them all. Then we call the record constructor and return it. It's not a big variation. We can keep the static converters and mediatypes and most of the code, I think.

Records don't have supertypes, so that's also one less thing to worry about.

Writing that, I realised that CDI injection probably only works for the first level, unless we don't really do new OtherBeanParamClass() like in my docs, but delegate to CDI. Not sure.

Unless you have a better idea?

[geoand]

IIRC, the CDI injection thing is only relevant for the Resources themselves, not for the "structs" needed for multipart or beanparam

[FroMage]

IIRC, the CDI injection thing is only relevant for the Resources themselves, not for the "structs" needed for multipart or beanparam

Is that true? Oh, perhaps, right, I forgot people use field injection for rest endpoints. Hopefully nobody asks us to support record rest endpoints 😂

[geoand]

Doubtful 😃

[FroMage]

Turns out I wasn't wrong, we do create all our bean param method parameters as beans, to support injection:

public class InjectParamExtractor implements ParameterExtractor {

    private final BeanFactory<Object> factory;

    public InjectParamExtractor(BeanFactory<Object> factory) {
        this.factory = factory;
    }

    @Override
    public Object extractParameter(ResteasyReactiveRequestContext context) {
        BeanFactory.BeanInstance<Object> instance = factory.createInstance();
        context.registerCompletionCallback(new CompletionCallback() {
            @Override
            public void onComplete(Throwable throwable) {
                instance.close();
            }
        });
        ((ResteasyReactiveInjectionTarget) instance.getInstance()).__quarkus_rest_inject(context);
        return instance.getInstance();
    }
}

So, that's the first thing that needs to change for records.

[geoand]

And that's not a lot of effort?

[FroMage]

Not sure. It's certainly more effort than I anticipated, which reminds us both how good I am at estimating tasks ;)

[FroMage]

So, I need to stop registering those as beans, that's one thing. Then I need to generate an instantiator class as a ParameterExtractor which will call the record's static method we need to generate.

[FroMage]

It'd be so much easier to transform records to stop being records and be regular classes 😅

I wonder if there's any trick we can use to generate a ParameterExtractor that calls the static method creator, without creating an extra class, since I can't make the record class implement this (well, actually I can, I just need to create an instance with null parameters…) or perhaps via lambdas, or method handles…

[FroMage]

So, this turned out to not be as trivial as expected. But I have records support for @*Param types, including primitives.

Missing support for records containing records (or bean params) and context objects (since there's no CDI injection in them).

[geoand]

Thanks for taking this up!

[FroMage]

Man, did I underestimate the problem touching this nest of vipers.

Turns out we register @BeanParam classes as CDI beans, so that injection works in them. Of course, @*Param injection does not work so we also produce our own injection methods, to run after CDI.

But records can't be beans, or not in a way that I know how, so injection can't work in them. Fine, that's a limitation.

But also, we register @BeanParam to be an alias of @Inject, which only works when it's explicit, not when a bean param is implicit, since there's no explicit annotation. I suppose I have to make a virtual one.

Soooo many lose ends…

4 days in, and still no end in sight. And I haven't even had time to refactor the ASM into gizmo :(

[FroMage]

But also, we register @BeanParam to be an alias of @Inject, which only works when it's explicit, not when a bean param is implicit, since there's no explicit annotation. I suppose I have to make a virtual one.

And also, remove the @BeanParam annotation from records, because CDI can't do injection in them.

[FroMage]

Actually, I just discovered this:

    /**
     * We generate a class that contains as many CDI producer methods as there are JAX-RS Resources that use JAX-RS params.
     *
     * If for example there was a single such JAX-RS resource looking like:
     *
     * <code><pre>
     *
     * &#64;Path("/query")
     * public class QueryParamResource {
     *
     * 	 private final String queryParamValue;
     * 	 private final UriInfo uriInfo;
     *
     * 	 public QueryParamResource(@QueryParam("p1") String headerValue, @Context UriInfo uriInfo) {
     * 		this.headerValue = headerValue;
     *   }
     *
     *   &#64;GET
     *   public String get() {
     * 	    // DO something
     *   }
     * }
     *
     *  </pre></code>
     *
     *
     * then the generated producer would look like this:
     *
     * <code><pre>
     *
     *  &#64;Singleton
     * public class ResourcesWithParamProducer {
     *
     *    &#64;Inject
     *    CurrentVertxRequest currentVertxRequest;
     *
     *    &#64;Produces
     *    &#64;RequestScoped
     *    public QueryParamResource producer_QueryParamResource_somehash(UriInfo uriInfo) {
     * 		return new QueryParamResource(getContext().getContext().queryParams().get("p1"), uriInfo);
     *    }
     *
     * 	private QuarkusRestRequestContext getContext() {
     * 		return (QuarkusRestRequestContext) currentVertxRequest.getOtherHttpContextObject();
     *    }
     * }
     *
     *  </pre></code>
     */

And I'm wondering if we should not do the same for bean param records :-/
Also, I wonder if that producer is even correct, it seems to be missing parameter conversion and multipart handling…

[FroMage]

I'm trying to make an annotation transformer that removes the @BeanInfo from injected records, for example:

    public static class OtherBeanParam {
        @RestQuery
        String q;
        @BeanParam // I WANT TO REMOVE THIS
        OtherBeanParamRecord obpr;
        @BeanParam
        OtherBeanParamClass obpc;
    }

    public record OtherBeanParamRecord(@RestQuery String q) {
    }

But apparently, this is not working, perhaps because my annotation transformer is declared before AutoInjectFieldProcessor which adds @Inject for @BeanParam annotations, and I'm guessing that annotation transformers do not compose, so the auto-inject looks at the original annotations, which still contain @BeanParam and then add the @Inject.

Is this correct @manovotn @mkouba @Ladicek ?

If yes, any idea how I can achieve the same and tell CDI to not inject record fields?

[FroMage]

OK, sorry, my bad. Annotation transformers are indeed chained, and I wanted mine to be before auto-inject-field, and my annotation removal code would have worked, if only I'd called .end() on the transformation.

User error, as often. Sorry for the ping, thanks anyway :)

[FroMage]

Alright, so one further surprise is that @BeanParam also applies to the rest client, but here it makes no sense to turn them into beans, damnit.

[FroMage]

OK, so #42642 contains support for record bean params on the server. But this issue is about kotlin data classes, which is something else entirely. Do they compile to records? Should I open another issue for the record support and leave this one open?

[manovotn]

OK, sorry, my bad. Annotation transformers are indeed chained, and I wanted mine to be before auto-inject-field, and my annotation removal code would have worked, if only I'd called .end() on the transformation.

User error, as often. Sorry for the ping, thanks anyway :)

Sorry I had a sick leave yesterday but it seems you have it all figured out now :)

However, I was thinking maybe we should tweak AutoInjectFieldProcessor to avoid handling records altogether?

[geoand]

Do they compile to records

Not by default IIRC

[FroMage]

However, I was thinking maybe we should tweak AutoInjectFieldProcessor to avoid handling records altogether

That is an option indeed.

[FroMage]

I have updated https://github.com/quarkusio/quarkus/wiki/RESTEasy-Reactive-(Server) with everything I learned during this rabbit hole, and everything I changed for records.

[FroMage]

I've added #42646 where I collected all the work remaining to be done that I noticed while working on this.

[manovotn]

However, I was thinking maybe we should tweak AutoInjectFieldProcessor to avoid handling records altogether

That is an option indeed.

Actually, I looked into it and recalled that we cannot do that.
Reason being, records can still become valid CDI beans so long as they don't need to be proxied.
It is not very useful but it is an option nonetheless 🤷

[FroMage]

Oh, so in the case that every record component is a CDI bean?

[manovotn]

Oh, so in the case that every record component is a CDI bean?

No, just like not every class is a CDI bean.
You still need to have some bean defining annotation on the class (@Dependent for instance) for CDI container to pick it up.

[FroMage]

Sure, I mean that for a record to be allowed to be a bean, all of its components must also be beans, otherwise the constructor won't be able to be called. But it only applies when there's a bean defining annotation.

[manovotn]

Yea, that's right 👍
Anyhow, your approach seems like a way forward here.