Closed FroMage closed 1 year ago
/cc @geoand, @stuartwdouglas
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?
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
So, I vote for the following changes:
@RestForm
types in method parameters@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.@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.@RestForm
parameter which requires multipart (FileUpload
, other?) and multipart is disabled by the user (using a conflicting content type).@Consumes(MediaType.MULTIPART_FORM_DATA)
if we have a multipart-requiring form parameter.WDYT @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
We allow all multipart @RestForm types in method parameters
I mean byte[]
File
and FileUpload
unless there are others.
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
.
LOL
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
:
@Rest*
and @*Param
fields/propertiesParamConverter
MultipartForm
File
, Path
, FileUpload
and List<File>
, List<FileUpload>
, List<Path>
named paramsList<FileUpload>
unnamed param@RestForm
parametersClass
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.
Maaaan, this code has changed a lot since the last time I looked at it :(
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?
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.
I can't see where.
I am making good progress allowing multipart parameters already.
I can see why you didn't add support for parameters, man. It's like fitting a square peg in a round hole!
I honestly don't remember đ
I can't see where.
Maybe I remember something else...
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 ;)
Very likely
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.
Yeah, I stay as far away from it as I can đ
First test passed, with the BeanParam generator! Turns out I forgot to DUP
before an IFNULL
(which pops) :)
đ
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 :(
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.
Thanks for your perseverance!
Pffff, now it looks like I broke the rest client.
Grrrr, it looks like the types supported by the server and client for multipart don't match :(
Client appears to support:
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)Multi<Byte>
(not supported by server, but that one's just weird)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.
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 :shrug:
On the bright side, it appears the client tests pass.
So, done:
@BeanParam
/@MultipartForm
code for client and serverTodo:
@BeanParam
and @MultipartForm
classes (make the annotation optional) on client/serverWe are getting closer, though.
Just when I thought I was done, I learned we have a vertx runtime for RESTEasy Reactive, without Quarkus. Huh.
This is the issue that keeps on giving.
The motherload of rabbit holes.
âšī¸
Finally, it's all there: #27526
Cool! I'll have a look next week.
Thanks :)
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.
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:Initially I had this:
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:
And that:
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