search

eclipse / microprofile-open-api

Microprofile open api
Apache License 2.0
132 stars 82 forks source link

Add tests for encoding with multipart/form-data #624

Closed benjamin-confino closed 3 months ago

benjamin-confino commented 4 months ago

Fixes #587

Depends on #616

For the comments there was mention of "Shall be ignored if not ... www-form-urlencoded or or multipart/form-data" in several places in the spec, but only one mention of "Shall be ignored if not ... www-form-urlencoded" in the current comments. I wasn't sure if I should only update the existing one or if I should add new ones too all. I decided there might be a good reason the other methods don't have that, but if you want them all say so and I'll add them.

eclipse-microprofile-bot commented 4 months ago

Can one of the admins verify this patch?

Azquelt commented 4 months ago

I wasn't sure if I should only update the existing one or if I should add new ones too all.

The javadoc is a little scatter-shot and inconsistent, so I'm also not sure whether there's a good reason that it's only mentioned in one place. I think your current change that just updates the information where we do have it is the right approach.

Azquelt commented 3 months ago

I had a look at this from the point of view of whether it's a valid resource method.

As far as I could find, there's no built-in support required for multipart/form-data in Jakarta REST 3.0 (EE 9). Support was only required in Jakarta REST 3.1 (EE 10).

However, in that case it would be treated as any other media type without built-in support and the Jakarta REST implementation would attempt to look up a MessageBodyReader and if one was not found, it would generate a NotSupportedException which could then be handled by an ExceptionHandler to generate a response.

Given this process, I think this method is a valid resource method, even if a compatible implementation may throw a NotSupportedException if the method was actually called.

Another option might be to use String or byte[] for which a built-in MessageBodyReader is required for all media types.

Azquelt commented 3 months ago

Squashed commits and removed merges.

Azquelt commented 3 months ago

@eclipse-microprofile-bot test this please