RESTEasy Reactive: multipart form improvements #27526

FroMage · 2022-08-26T09:27:32Z

Made @BeanParam and @MultipartForm optional (any container class with fields using the @Rest* annotations or their JAX-RS equivalents will be auto-detected as a bean param
Merged the @MultipartForm special handling into the @BeanParam code, so that you can now have any request field in that container class (@RestCookie and @RestForm for example)
Support multipart parameters in method parameters (no need for containing class anymore)
Auto-detect multipart based on the presence of FileUpload, File, Path (for the server) and byte[], Multi<Byte> (for the client), removing need for explicit @Consumes
Multipart fields default to text/plain for @PartType except for FileUpload, File, Path (for the server) and byte[], Multi<Byte> (for the client) which default to application/octet-stream
Multipart fields that are text/plain go through the ParamConverter route instead of the MessageBodyReader/Writer one.
Multipart fields that are neither text/plain nor FileUpload, File, Path (for the server) and byte[], Multi<Byte> (for the client) go through the MessageBodyReader/Writer route.
Deprecated @MultipartForm which is never useful anymore. Would deprecate @BeanParam too if I could.
Replaced the awkward @RestForm List<FileUpload> uploads pattern which was special-cased to obtain all uploads with the clearer pattern @RestForm(FileUpload.ALL) List<FileUpload> uploads. Note that this is a breaking change.
Updated the docs.

I had to touch some bits that were also touched by #27474 due to parallel work.
I also saw some special support for getters/setters in multipart forms that may have fallen by the side due to bean param probably not dealing with them, but I'm not sure. I'm also not sure it's useful.

geoand · 2022-08-26T09:33:30Z

@Sgitario you might also be interested in reviewing this

Sgitario

Apart from the comments in the review, I'm not a big fan of doing @RestForm(FileUpload.ALL) to load a list of FileUpload List<FileUpload>.

Also, if possible and only if it's easy, we could move the converters into the common module, so it's reused by both the client and the server.

The rest of the changes look good to me.

Sgitario · 2022-08-26T09:45:06Z

docs/src/main/asciidoc/rest-client-reactive.adoc

@@ -744,35 +744,42 @@ REST Client Reactive support multipart messages.
 REST Client Reactive allows sending data as multipart forms. This way you can for example
 send files efficiently.

-To send data as a multipart form, you need to create a class that would encapsulate all the fields
-to be sent, e.g.
+To send data as a multipart form, you can just use the regular `@RestForm` annotations:


We should mention the regular JAX-RS annotation @FormParam here.

I guess… In the server guide we avoid mentioning these annotations to not confuse users.

docs/src/main/asciidoc/rest-client-reactive.adoc

Sgitario · 2022-08-26T09:47:17Z

docs/src/main/asciidoc/rest-client-reactive.adoc

@@ -809,7 +830,7 @@ Then, create an interface method that corresponds to the call and make it return
    @GET
    @Produces(MediaType.MULTIPART_FORM_DATA)
    @Path("/get-file")
-    FormDto data sendMultipart();
+    FormDto data receiveMultipart();


we're sending a multipart object, not receiving, right ?

Well, from the PoV of the client, we're receiving one, we're not sending anything, especially not from a GET request.

docs/src/main/asciidoc/resteasy-reactive.adoc

...ava/io/quarkus/resteasy/reactive/server/test/multipart/ErroneousParamMultipartInputTest.java

...ment/src/test/java/io/quarkus/resteasy/reactive/server/test/multipart/MultipartResource.java

FroMage · 2022-08-26T13:55:05Z

Apart from the comments in the review, I'm not a big fan of doing @RestForm(FileUpload.ALL) to load a list of FileUpload List<FileUpload>.

Well, consider that the semantics of @RestForm List<FileUpload> foo for every other case is to default to the foo name (name of the annotated field/parameter), but only in the case of multipart, that default was dropped in favour of requiring the user to duplicate the parameter name @RestForm("foo") List<FileUpload> foo, but again only for the special List<FileUpload> type. This sort of special-case is super harmful to users, counter-intuitive and really bad practice.

The new way of doing his, by explicitely overriding the name to a meaningful constant FileUpload.ALL (which is the * string, which is not a valid parameter name otherwise) is self-describing as well as consistent with all other usages of @RestForm.

Also, if possible and only if it's easy, we could move the converters into the common module, so it's reused by both the client and the server.

The server doesn't actually need the primitive converters, because it will generate them if they're required. The client should do the same, possibly by sharing the server code which generates the converters, but that's not for this issue.

geoand · 2022-08-29T05:31:06Z

FWIW, the test failures do seem related

FroMage · 2022-08-29T07:55:08Z

Damn, look like I broke quite a few TCK tests :(

FroMage · 2022-08-29T08:06:57Z

MultipartResourceTest runs locally. Do you happen to know how I can run the MultipartResourceIT one?

geoand · 2022-08-29T08:13:03Z

mvn verify -Dstart-containers -Dtest-containers

will build the native binary and run the tests.
If you already have the native binary built, you can save a bunch of time by doing:

mvn failsafe:integration-test -Dstart-containers -Dtest-containers -Dnative -Dit.test=MultipartResourceIT

FroMage · 2022-08-29T08:45:23Z

Thanks.

FroMage · 2022-08-29T13:04:10Z

I was able to fix all the test issues, except the native image one. It's throwing a 500, I guess I need to find out how to make it log the cause.

FroMage · 2022-08-29T13:41:34Z

It's a reflection issue, apparently.

FroMage · 2022-08-30T08:32:15Z

OK, the tests should pass now.

BTW @geoand I couldn't find any way to make RR print the exceptions, no setting, no docs about it, and nothing in the code that leads me to think we even log exceptions before we map them. Is that true? If yes, shouldn't we add a setting or log them on error level or something?

FroMage · 2022-08-30T08:36:52Z

Guys, I'd love a review on this, in particular about the docs and changes, and an opinion on the impact of the breaking change regarding obtaining all FileUpload regardless of their names.

But can we not merge this until I get back from PTO at the end of september? Otherwise I won't be able to help with any bugs.

geoand · 2022-08-30T08:38:01Z

But can we not merge this until I get back from PTO at the end of september? Otherwise I won't be able to help with any bugs

Sure yeah. @Sgitario let's not make any multipart changes until this is in, because it will cause @FroMage a lot of unnecessary headaches.

geoand · 2022-08-30T08:39:56Z

BTW @geoand I couldn't find any way to make RR print the exceptions, no setting, no docs about it, and nothing in the code that leads me to think we even log exceptions before we map them. Is that true? If yes, shouldn't we add a setting or log them on error level or something?

I think you are probably right...

FroMage · 2022-08-30T08:44:23Z

Turning to draft to avoid premature merging

FroMage · 2022-10-13T13:18:56Z

😥

The failure showed up in a few PRs (like quarkusio#28429 and quarkusio#27526)

geoand · 2022-10-17T07:52:18Z

@gsmet are you okay with this one?

We really need to get this in because it's causing frequent conflicts (@FroMage can you address the latest one please?).

FroMage · 2022-10-17T13:15:25Z

Done.

geoand · 2022-10-17T13:16:37Z

TY

FroMage · 2022-10-20T13:37:53Z

This test failure looks unrelated. Can we merge this @gsmet and move on with our lives? :)

- Merged support into @BeanParam handling - Deprecated @MultipartForm - Auto-detect @BeanParam classes, annotation now optional - Use ParamConverter instead of MessageBodyReader for plain text multiparts - Auto-default to Accept: urlencoded or multipart depending on types of @FormParam present - Support multipart form elements as endpoint parameters and fields - Breaking: need @restform(FileUpload.ALL) for getting all uploads

FroMage · 2022-10-21T08:13:37Z

Squashed into a single commit.

issue addressed

quarkus-bot · 2022-10-21T13:44:37Z

Failing Jobs - Building `03833f8`

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
⌛	JVM Tests - JDK 17	`Build`	⚠️ Check →	Logs	Raw logs
✖	JVM Tests - JDK 17 MacOS M1	`Build`	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 MacOS M1 #

- Failing: integration-tests/mongodb-devservices

📦 integration-tests/mongodb-devservices

✖ io.quarkus.it.mongodb.BookResourceTest.health line 45 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:283)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:309)

✖ io.quarkus.it.mongodb.BookResourceTest.testReactiveClients line 39 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:283)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:309)

✖ io.quarkus.it.mongodb.BookResourceTest.testBlockingClient line 34 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:283)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:309)

✖ io.quarkus.it.mongodb.BookResourceWithParameterInjectionTest.testInjectedClient line 31 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:283)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:309)

✖ io.quarkus.it.mongodb.OtherProfileBookResourceTest.testBlockingClient line 14 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:283)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:309)

FroMage · 2022-10-24T08:48:57Z

Updated the migration guide too.

The failure showed up in a few PRs (like quarkusio#28429 and quarkusio#27526)

quarkus-bot bot added area/documentation area/rest labels Aug 26, 2022

FroMage mentioned this pull request Aug 26, 2022

RESTEasy Reactive: multipart form improvement #22205

Closed

This comment has been minimized.

Sign in to view

FroMage mentioned this pull request Aug 26, 2022

Default parameter conversion for REST Client Reactive #27430

Closed

FroMage force-pushed the rr-multipart-beanparam branch from 418845b to c6220ea Compare August 26, 2022 09:42

This comment has been minimized.

Sign in to view

Sgitario reviewed Aug 26, 2022

View reviewed changes

FroMage force-pushed the rr-multipart-beanparam branch from c6220ea to 44db973 Compare August 26, 2022 16:37

This comment has been minimized.

Sign in to view

FroMage marked this pull request as draft August 30, 2022 08:43

This was referenced Aug 30, 2022

RESTEasy Reactive - Server side multipart implementation is not being respected #27083

Closed

RESTEasy Reactive: log exceptions #27585

Closed

This comment has been minimized.

Sign in to view

igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 16, 2022

Fix RESTEasy Reactive testsuite

edba8ec

The failure showed up in a few PRs (like quarkusio#28429 and quarkusio#27526)

igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 16, 2022

Fix RESTEasy Reactive testsuite

b6dc518

The failure showed up in a few PRs (like quarkusio#28429 and quarkusio#27526)

igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 16, 2022

Fix RESTEasy Reactive testsuite

7288257

The failure showed up in a few PRs (like quarkusio#28429 and quarkusio#27526)

igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 17, 2022

Fix RESTEasy Reactive testsuite

9f17d73

The failure showed up in a few PRs (like quarkusio#28429 and quarkusio#27526)

geoand mentioned this pull request Oct 17, 2022

Using raw RestResponse return type has unexpected consequences #28627

Closed

FroMage force-pushed the rr-multipart-beanparam branch from 683db74 to 7510b62 Compare October 17, 2022 13:15

This comment has been minimized.

Sign in to view

FroMage force-pushed the rr-multipart-beanparam branch from 7510b62 to 03833f8 Compare October 21, 2022 08:13

geoand added triage/waiting-for-ci Ready to merge when CI successfully finishes release/noteworthy-feature labels Oct 21, 2022

geoand merged commit e50ae5d into quarkusio:main Oct 21, 2022

quarkus-bot bot added this to the 2.14 - main milestone Oct 21, 2022

quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Oct 21, 2022

michalvavrik mentioned this pull request Oct 24, 2022

RESTEasy Reactive - mapping MULTIPART_FORM_DATA to request body don't work #28782

Closed

tmihalac pushed a commit to tmihalac/quarkus that referenced this pull request Oct 27, 2022

Fix RESTEasy Reactive testsuite

0961691

The failure showed up in a few PRs (like quarkusio#28429 and quarkusio#27526)

Sgitario mentioned this pull request Nov 2, 2022

MultipartForm is receiving null parameters at the server #27215

Closed

Postremus mentioned this pull request Mar 22, 2023

Allow PartFileName as method parameter #32033

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RESTEasy Reactive: multipart form improvements #27526

RESTEasy Reactive: multipart form improvements #27526

FroMage commented Aug 26, 2022

This comment has been minimized.

geoand commented Aug 26, 2022

This comment has been minimized.

This comment has been minimized.

Sgitario left a comment

Sgitario Aug 26, 2022

FroMage Aug 26, 2022

Sgitario Aug 26, 2022

FroMage Aug 26, 2022

FroMage commented Aug 26, 2022

This comment has been minimized.

geoand commented Aug 29, 2022

FroMage commented Aug 29, 2022

FroMage commented Aug 29, 2022

geoand commented Aug 29, 2022

FroMage commented Aug 29, 2022

FroMage commented Aug 29, 2022

FroMage commented Aug 29, 2022

FroMage commented Aug 30, 2022

FroMage commented Aug 30, 2022

geoand commented Aug 30, 2022

geoand commented Aug 30, 2022

FroMage commented Aug 30, 2022

This comment has been minimized.

FroMage commented Oct 13, 2022

geoand commented Oct 17, 2022

FroMage commented Oct 17, 2022

geoand commented Oct 17, 2022

This comment has been minimized.

FroMage commented Oct 20, 2022

FroMage commented Oct 21, 2022

quarkus-bot bot commented Oct 21, 2022

FroMage commented Oct 24, 2022

RESTEasy Reactive: multipart form improvements #27526

RESTEasy Reactive: multipart form improvements #27526

Conversation

FroMage commented Aug 26, 2022

This comment has been minimized.

geoand commented Aug 26, 2022

This comment has been minimized.

This comment has been minimized.

Sgitario left a comment

Choose a reason for hiding this comment

Sgitario Aug 26, 2022

Choose a reason for hiding this comment

FroMage Aug 26, 2022

Choose a reason for hiding this comment

Sgitario Aug 26, 2022

Choose a reason for hiding this comment

FroMage Aug 26, 2022

Choose a reason for hiding this comment

FroMage commented Aug 26, 2022

This comment has been minimized.

geoand commented Aug 29, 2022

FroMage commented Aug 29, 2022

FroMage commented Aug 29, 2022

geoand commented Aug 29, 2022

FroMage commented Aug 29, 2022

FroMage commented Aug 29, 2022

FroMage commented Aug 29, 2022

FroMage commented Aug 30, 2022

FroMage commented Aug 30, 2022

geoand commented Aug 30, 2022

geoand commented Aug 30, 2022

FroMage commented Aug 30, 2022

This comment has been minimized.

FroMage commented Oct 13, 2022

geoand commented Oct 17, 2022

FroMage commented Oct 17, 2022

geoand commented Oct 17, 2022

This comment has been minimized.

FroMage commented Oct 20, 2022

FroMage commented Oct 21, 2022

quarkus-bot bot commented Oct 21, 2022

Failing Jobs - Building 03833f8

Failures

⚙️ JVM Tests - JDK 17 MacOS M1 #

📦 integration-tests/mongodb-devservices

FroMage commented Oct 24, 2022

Failing Jobs - Building `03833f8`