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

Support retrieval of all Multipart parts in RESTEasy Reactive #30307

Merged
merged 4 commits into from
Jan 12, 2023

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jan 11, 2023

This is similar to what RESTEasy Classic provides with org.jboss.resteasy.plugins.providers.multipart.MultipartFormDataInput.

The change is breaking because I moved the package of MultipartFormDataOutput to the API package instead of the internal package where it was residing.

The meat of the PR is the final commit.

@geoand geoand requested a review from Sgitario January 11, 2023 13:27
@geoand geoand changed the title Support retrieval of all Multipart parts Support retrieval of all Multipart parts in RESTEasy Reactive Jan 11, 2023
@geoand
Copy link
Contributor Author

geoand commented Jan 11, 2023

@pedroigor you might be interested in this as well :)

@geoand geoand force-pushed the multipart-improvements branch from f99062a to 2e56114 Compare January 11, 2023 13:34
@@ -11,7 +11,7 @@
import javax.ws.rs.core.MediaType;

import org.jboss.resteasy.reactive.RestResponse;
import org.jboss.resteasy.reactive.server.core.multipart.MultipartFormDataOutput;
import org.jboss.resteasy.reactive.server.multipart.MultipartFormDataOutput;
Copy link
Member

Choose a reason for hiding this comment

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

Would it be hard to keep the old class for a few versions as deprecated and then drop it? (If we can easily extend the new one and have things working.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried initially, but it turned out to be a big hassle so I opted against it (especially considering it's just a package change)

Copy link
Member

Choose a reason for hiding this comment

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

OK, if it's too much of a hassle, I totally agree. Let's make sure we drop a line there: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-2.17 when merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely

@pedroigor
Copy link
Contributor

pedroigor commented Jan 11, 2023

@geoand Thanks. It seems to make the integration easier so I can drop a few code from our side. Are you backporting this change to 2.13? Or is it targeted to v3?

@geoand
Copy link
Contributor Author

geoand commented Jan 11, 2023

It will be tough to backport this one (we would obviously not backport the breaking change commit), but if it's important enough, we can certainly try

@pedroigor
Copy link
Contributor

It is not critical and the current integration seems to work fine.

We started the v3 upgrade and I should be able to merge the RR changes after KC 21 is out.

Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

Really nice refactor!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants