Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

benjamin-confino
Copy link
Contributor

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
Copy link
Contributor

Can one of the admins verify this patch?

@Azquelt
Copy link
Member

Azquelt commented May 30, 2024

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.

Comment on lines 62 to 71
@APIResponse(responseCode = "200", description = "Review deleted",
content = @Content(encoding = @Encoding(name = "enigma", style = "pipeDelimited", explode = true,
allowReserved = true, contentType = "multipart/form-data")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name needs to be the name of a property included in the schema, see https://spec.openapis.org/oas/v3.1.0.html#media-type-object

Note that @Content corresponds to a Media Type Object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, from the Encoding Object part of the spec:

style: ... This property SHALL be ignored if the request body media type is not application/x-www-form-urlencoded or multipart/form-data.
(similar text for explode and allowReserved)

I think this means these properties only make sense when applied to the content of a request body where its media type is multipart/form-data. (Which makes sense I think, multipart/form-data is a type sent by web browsers when the user submits a form).

Note that this is not the contentType of the @Content annotation, it's the value of @Consumes on the method.

Tbh, I'm not that familiar with receiving form data using Jakarta REST, I would probably start by looking up an example and then trying to add the OpenAPI documentation.

@benjamin-confino benjamin-confino force-pushed the 587-encoding branch 3 times, most recently from 65f8abf to 4577950 Compare June 4, 2024 16:46
@Azquelt
Copy link
Member

Azquelt commented Jun 10, 2024

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.

@benjamin-confino benjamin-confino force-pushed the 587-encoding branch 3 times, most recently from bdc389d to 34fd07d Compare June 10, 2024 16:17
Copy link
Member

@Azquelt Azquelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think this looks good now. Ideally we would squash all the commits.

@Azquelt
Copy link
Member

Azquelt commented Jun 11, 2024

Squashed commits and removed merges.

@Azquelt
Copy link
Member

Azquelt commented Jun 11, 2024

@eclipse-microprofile-bot test this please

@Azquelt Azquelt merged commit eb1b324 into microprofile:main Jun 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OAS 3.1.0] More Encoding options can be set for multipart/form-data
4 participants