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.

Sign up for GitHub

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

Jump to bottom

Rest Client Reactive - multipart support #17173

Merged
merged 1 commit into from
May 27, 2021
Merged

Rest Client Reactive - multipart support #17173

merged 1 commit into from
May 27, 2021

Conversation

michalszynkiewicz
Copy link
Member

No description provided.

@michalszynkiewicz michalszynkiewicz changed the title Rest Client Reactive - multipart support - needs more tests Rest Client Reactive - multipart support May 14, 2021
@michalszynkiewicz michalszynkiewicz marked this pull request as ready for review May 14, 2021 14:06
@quarkus-bot
Copy link

quarkus-bot bot commented May 14, 2021 •
edited

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building b63447c

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Cache Maven Repository ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented May 14, 2021 •
edited

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building b63447c

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Attach pull request number ⚠️ Check → Logs Raw logs
⌛ CI Sanity Check ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented May 14, 2021 •
edited

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building fd1aa99

Status Name Step Test failures Logs Raw logs
✔️ JVM Tests - JDK 11
✖ JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 16

@michalszynkiewicz
Copy link
Member Author

The failure is as follows and is not related to the change

Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M5:test (default-test) on project quarkus-integration-test-mongodb-client: There was a timeout in the fork -> [Help 1]

@michalszynkiewicz
Copy link
Member Author

I'm aware that this should get some documentation. But if this PR is otherwise okay, I'd rather add the documentation in a separate PR as I need to do some other stuff in the following days...

@geoand
Copy link
Contributor

geoand commented May 15, 2021

I'll have a look on Monday.

We will absolutely need documentation for this before the final release

geoand
geoand reviewed May 17, 2021
View reviewed changes
.../src/main/java/io/quarkus/jaxrs/client/reactive/deployment/JaxrsClientReactiveProcessor.java
@@ -871,6 +910,212 @@ A more full example of generated client (with sub-resource) can is at the bottom

}

private ResultHandle createMultipartForm(MethodCreator methodCreator, ResultHandle methodParam, Type formClassType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get some comments on this? It's pretty large and I have no idea what it's supposed to do :)

Copy link
Member Author

Choose a reason for hiding this comment

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

added some comments :)

geoand
geoand reviewed May 17, 2021
View reviewed changes
...rest-client-multipart/src/main/java/io/quarkus/it/rest/client/multipart/MultipartClient.java

@FormParam("file")
@PartType(MediaType.APPLICATION_OCTET_STREAM)
public byte[] file;
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll have to be very careful on how we document this and other blocking variants

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the PR and, among others, added some basic documentation. Could you elaborate on ^? What do you think is worth mentioning?

@geoand
Copy link
Contributor

geoand commented May 17, 2021

With the current implementation, is there any way a user could send a multipart stream without reading all bytes in memory?

@michalszynkiewicz
Copy link
Member Author

With the current implementation, is there any way a user could send a multipart stream without reading all bytes in memory?

One could use files, see https://github.com/quarkusio/quarkus/pull/17173/files#diff-00c5b96175fbd95dec22f03f0df7e05d4369109d19a6547cf60bffffdc7625bbR82

@geoand
Copy link
Contributor

geoand commented May 18, 2021

With the current implementation, is there any way a user could send a multipart stream without reading all bytes in memory?

One could use files, see https://github.com/quarkusio/quarkus/pull/17173/files#diff-00c5b96175fbd95dec22f03f0df7e05d4369109d19a6547cf60bffffdc7625bbR82

Nice!
Is Path also supported? Furthermore, where exactly in the code is that implemented? Just want to make sure it's actually non-blocking :)

@michalszynkiewicz
Copy link
Member Author

With the current implementation, is there any way a user could send a multipart stream without reading all bytes in memory?

One could use files, see https://github.com/quarkusio/quarkus/pull/17173/files#diff-00c5b96175fbd95dec22f03f0df7e05d4369109d19a6547cf60bffffdc7625bbR82

Nice!
Is Path also supported? Furthermore, where exactly in the code is that implemented? Just want to make sure it's actually non-blocking :)

at the moment only File is supported.

This creates a io.vertx.ext.web.multipart.MultipartForm with the fields from the multipart "body object": https://github.com/michalszynkiewicz/quarkus/blob/fd1aa99842331b2413ffcd6fe171d17bf863d1e6/extensions/resteasy-reactive/jaxrs-client-reactive/deployment/src/main/java/io/quarkus/jaxrs/client/reactive/deployment/JaxrsClientReactiveProcessor.java#L913

Then the code here does the actual sending

@geoand
Copy link
Contributor

geoand commented May 18, 2021

at the moment only File is supported.

Let's support Path as well since we do on the server side as well.

This creates a io.vertx.ext.web.multipart.MultipartForm with the fields from the multipart "body object": https://github.com/michalszynkiewicz/quarkus/blob/fd1aa99842331b2413ffcd6fe171d17bf863d1e6/extensions/resteasy-reactive/jaxrs-client-reactive/deployment/src/main/java/io/quarkus/jaxrs/client/reactive/deployment/JaxrsClientReactiveProcessor.java#L913

Then the code here does the actual sending

So IIUC, this code is lifted from the vertx-web client? If so, great!

@quarkus-bot
Copy link

quarkus-bot bot commented May 27, 2021 •
edited

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building de3d256

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@michalszynkiewicz
Copy link
Member Author

Yes, the code is heavily based on (and in some places simply copied from) vertx-web, as such, should be non-blocking.

I added support for Path, added documentation.

@michalszynkiewicz michalszynkiewicz merged commit 4a66d0e into quarkusio:main May 27, 2021
@quarkus-bot quarkus-bot bot added this to the 2.1 - main milestone May 27, 2021
@gsmet gsmet modified the milestones: 2.1 - main, 2.0.0.Final May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

Assignees
No one assigned
Labels
area/documentation area/resteasy-reactive
Projects
None yet
Milestone
2.0.0.CR3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@michalszynkiewicz @geoand @gsmet