RESTEasy Reactive: multipart form improvement

[FroMage]

Description

ATM, we only support @RestForm File when they're in a class along with @Consumes(MediaType.MULTIPART_FORM_DATA) to trigger the special handling, forcing me to write code like this:

@Path("albums")
public class Albums extends MyController {
	
    public static class MyForm {
        @RestForm @Length(max = Util.VARCHAR_SIZE) String title;
        @RestForm String photoUrl;
        @RestForm boolean removePhoto;
        @RestForm File photo;
    }

    @POST
    @Consumes(MediaType.MULTIPART_FORM_DATA)
    @Authenticated
	public void edit(@RestPath long id,
	        MyForm myForm, @RestForm Date releaseDate){
...
	}
}

Initially I had this:

@Path("albums")
public class Albums extends MyController {

    @POST
    @Authenticated
	public void edit(@RestPath long id,
        @RestForm @Length(max = Util.VARCHAR_SIZE) String title,
        @RestForm String photoUrl,
        @RestForm boolean removePhoto,
        @RestForm File photo,
        @RestForm Date releaseDate){
...
	}
}

But it did not work. Apparently @FormParam File can only happen in a wrapper class, but while it's fine that there's an option to do this, I'd very much like to be able to do the same on parameters.

Also, the docs at https://quarkus.io/guides/resteasy-reactive#handling-multipart-form-data tell me I can write:

    @POST
    @Produces(MediaType.APPLICATION_JSON)
    @Consumes(MediaType.MULTIPART_FORM_DATA)
    @Path("form")
    public String form(@MultipartForm FormData formData) {
        // return something
    }

And that:

The use of `@MultipartForm` is actually unnecessary as RESTEasy Reactive can infer this information from the use of `@Consumes(MediaType.MULTIPART_FORM_DATA)`

But I don't even see why we require @Consumes(MediaType.MULTIPART_FORM_DATA) to trigger form loading. We don't need such a marker for regular url-encoded forms, do we? IMO we can infer that we're going to have to do form loading at build time by just finding @RestForm or @MultipartForm parameters.

Once we know it's a form, we can check the content-type at runtime to turn on url-encoded or multipart unserialising, no?

BTW @MultipartForm is actually only required because of JAX-RS treating unannotated params as the body, but I think we could go and scan the damn type and if there are @RestForm fields in there not treat it as a body but infer @MultipartForm behaviour.

Implementation ideas

No response

[quarkus-bot]

/cc @geoand, @stuartwdouglas

[FroMage]

Actually, ideally I'd have multipart be transparent entirely, allowing it just as we allow form parameters:

@POST
public void post(@RestHeader String contentType, 
 @RestForm String param, 
 @RestForm File file, 
 @RestForm FileUpload otherFile){}

As well as on @BeanParam which could replace @MultipartForm:

public static class Params {
@RestHeader String contentType;
@RestForm String param;
@RestForm File file;
@RestForm FileUpload otherFile;
}
@POST
public void post(@BeanParam Params all){}

And also, why are the @BeanParam and @MultipartForm generated beans different? From the outside they look like they do exactly the same thing, no?

[geoand]

And also, why are the @BeanParam and @MultipartForm generated beans different? From the outside they look like they do exactly the same thing, no?

Yeah, we should probably merge the code of the two

[FroMage]

So, I vote for the following changes:

• We allow all multipart @RestForm types in method parameters
• We unify the @BeanParam and @MultipartForm code generators (IIRC the only difference should be that the latter uses the serialisers instead of the ParamConverter, but I suspect that's an arbitrary choice not necessarily consistent with the spec or its intent. IIUC the message body reader/writers are meant for entities, not parameters.
• We make @BeanParam and @MultipartForm optional, and auto-detect them by looking at the unannotated parameter and scanning it if it contains any @Rest* or @*Param field/property. This is to differentiate it from an entity body.
• We generate an error if we have any @RestForm parameter which requires multipart (FileUpload, other?) and multipart is disabled by the user (using a conflicting content type).
• We automatically infer @Consumes(MediaType.MULTIPART_FORM_DATA) if we have a multipart-requiring form parameter.

WDYT @geoand ?

[geoand]

We allow all multipart @restform types in method parameters

WDYM by this?

We make @BeanParam and @MultipartForm optional, and auto-detect them by looking at the unannotated parameter and scanning it if it contains any @rest* or @*Param field/property. This is to differentiate it from an entity body

Are you sure this is enough to differentiate them?

The rest sounds reasonable to me

[FroMage]

We allow all multipart @restform types in method parameters

I mean byte[] File and FileUpload unless there are others.

[FroMage]

Did you see https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-3.1.html#consuming_multipart_formdata BTW? There's support for multipart now upstream. But only for EntityPart String or InputStream and no word on whether the element goes via ParameterConverter or MessageBodyReader.

[geoand]

LOL

[FroMage]

So, I'm looking into this. It's really a pity those two things diverged so much. It appears that regular form elements sent via multipart will have a mime type text/plain. We could deserialise them via ParamConverter instead of MessageBodyReader just to avoid users having to write both types of deserialisers for types such as, say Date or what. I've been hit by this before. And for all other mime types, we go via MessageBodyReader if the form is a multipart.

The big differences that I've noted between BeanParam and MultipartForm:

BeanParam:

• Handles @Rest* and @*Param fields/properties
• All supported fields/properties can already be used as endpoint method parameters
• Goes via ParamConverter

MultipartForm

• Handles File, Path, FileUpload and List<File>, List<FileUpload>, List<Path> named params
• Handles List<FileUpload> unnamed param
• Handles arrays and lists specially
• Only @RestForm parameters
• Generates a populator class holding information about Class Type and MediaType for each field.

BTW, I find it a bit confusing that for List<File|Path|FileUpload> the @RestParam("name") form is required and the field name is not used, unlike for every other case. I suppose I would have found @RestParam("*") and an alias @RestParam(RestParam.ALL) less special-casey.

[FroMage]

Maaaan, this code has changed a lot since the last time I looked at it :(

[FroMage]

One thing I can't quite figure out yet is why some fields are annotated with @PartType(MediaType.TEXT_PLAIN) in the tests. They appear to be enums or primitives. I wonder why, because this is the default mime type, surely?

In fact, I don't know what that annotation is for. The javadocs say:

Used on fields of {@link MultipartForm} POJOs to designate the media type the corresponding body part maps to.

The documentation says:

`@PartType` is used to aid in deserialization of the corresponding part of the request into the desired Java type. It is very useful when for example the corresponding body part is JSON and needs to be converted to a POJO.

And I'm left wondering what it's for. Because multipart elements actually come with their mime types set, like Content-Type headers, and it appears we're not using that. I have the feeling they should be used. Perhaps we should even detect when someone sends us a form element of type text/plain and we expect it to be application/json with a @PartType override?

[geoand]

Because multipart elements actually come with their mime types set, like Content-Type headers, and it appears we're not using that

I am pretty sure we are.

[FroMage]

I can't see where.

[FroMage]

I am making good progress allowing multipart parameters already.

[FroMage]

I can see why you didn't add support for parameters, man. It's like fitting a square peg in a round hole!

[geoand]

I honestly don't remember 😂

[geoand]

I can't see where.

Maybe I remember something else...

[FroMage]

Huh, I also suspect that one of the reason you didn't add support for multipart in BeanParam is because it's an ASM generator, while you did it in gizmo ;)

[geoand]

Very likely

[FroMage]

Oh, I can't blame you. This is why I love ASM:

Caused by: java.lang.ArrayIndexOutOfBoundsException: Index -1 out of bounds for length 1
	at org.objectweb.asm.Frame.merge(Frame.java:1268)
	at org.objectweb.asm.Frame.merge(Frame.java:1244)
	at org.objectweb.asm.MethodWriter.computeAllFrames(MethodWriter.java:1611)
	at org.objectweb.asm.MethodWriter.visitMaxs(MethodWriter.java:1547)
	at org.jboss.resteasy.reactive.server.processor.scanning.ClassInjectorTransformer$ClassInjectorVisitor.visitEnd(ClassInjectorTransformer.java:235)
	at org.objectweb.asm.ClassReader.accept(ClassReader.java:748)
	at org.objectweb.asm.ClassReader.accept(ClassReader.java:424)

Translation: you're screwed, man, give up.

[geoand]

Yeah, I stay as far away from it as I can 😅

[FroMage]

First test passed, with the BeanParam generator! Turns out I forgot to DUP before an IFNULL (which pops) :)

[geoand]

🎉

[FroMage]

Oh FFS. This took hours to find:

                injectMethod.visitTypeInsn(Opcodes.CHECKCAST, fieldInfo.type().name().toString('/'));

Apparently when type is String[] the name().toString('/') returns [Ljava.lang.String; which is not using the damn separator I passed :(

[FroMage]

PFEW. OK, the version with message body readers also passes now. Just in time to go on PTO with working code, and later come back and do a PR.

[geoand]

Thanks for your perseverance!

[FroMage]

Pffff, now it looks like I broke the rest client.

[FroMage]

Grrrr, it looks like the types supported by the server and client for multipart don't match :(

[FroMage]

Client appears to support:

• primitives (via custom convertion to String, not using converters)
• byte[] (which the server explicitly forbids and checks against)
• File (same as server)
• Path (same as server)
• Buffer (not supported by server)
• pojo (via message body writer, same as server)
• Multi<Byte> (not supported by server, but that one's just weird)

[FroMage]

Also, it appears there are code paths in the client that invoke the converter for form params, but others not. I don't know why yet.

[FroMage]

It doesn't look like the client really supports generic types wrt. multipart and message body writers, and it doesn't seem to have any default or generated ParamProvider, but 🤷

On the bright side, it appears the client tests pass.

So, done:

• Merged @BeanParam/@MultipartForm code for client and server
• Server support for multipart form method parameters

Todo:

• Client support for multipart form method parameters
• Auto-detection of @BeanParam and @MultipartForm classes (make the annotation optional) on client/server
• Docs

We are getting closer, though.

[FroMage]

Just when I thought I was done, I learned we have a vertx runtime for RESTEasy Reactive, without Quarkus. Huh.

[FroMage]

This is the issue that keeps on giving.

[FroMage]

The motherload of rabbit holes.

[geoand]

☹️

[FroMage]

Finally, it's all there: #27526

[geoand]

Cool! I'll have a look next week.

[FroMage]

Thanks :)

[FroMage]

I believe the UX is much simpler for users now, and we have a single code-base to maintain for containing classes, though sadly it's in ASM rather than gizmo, but we can always later decide to convert it to gizmo as long as we keep a single generator, so there's always hope.