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

Do multipart parsing inside RESTEasyReactive #17687

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

stuartwdouglas
Copy link
Member

This gives us a lot more flexibility, and allows it to work
with multiple backends.

Fixes #17430

@stuartwdouglas stuartwdouglas requested review from FroMage and geoand June 4, 2021 05:59
@quarkus-bot quarkus-bot bot added the area/rest label Jun 4, 2021
@geoand
Copy link
Contributor

geoand commented Jun 4, 2021

Looking at the PR, I assume this is based on your Undertow code?

@stuartwdouglas
Copy link
Member Author

Yea, the multipart parser is adapted from the Undertow parser. I guess I should copy the tests as well.

@geoand
Copy link
Contributor

geoand commented Jun 4, 2021

Assuming the tests pass (and given the fact that this code has already been battle tested in Undertow), this looks great!

@stuartwdouglas
Copy link
Member Author

Actually the Undertow tests don't really add much more than is already covered by our existing multipart tests.

@geoand
Copy link
Contributor

geoand commented Jun 4, 2021

Should we mark this for backporting to 2.0?

@stuartwdouglas
Copy link
Member Author

Looks like there might be some issues with the TCK, investigating now.

@geoand geoand changed the title Do multipart parsing inside RR Do multipart parsing inside RESTEasyReactive Jun 4, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 4, 2021

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

🚫 This workflow run has been cancelled.

Failing Jobs - Building d9f853d

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

Status Name Step Test failures Logs Raw logs
Gradle Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
Gradle Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 16 Build ⚠️ Check → Logs Raw logs
MicroProfile TCKs Tests Verify ⚠️ Check → Logs Raw logs
Native Tests - Cache Download Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Data1 Build ⚠️ Check → Logs Raw logs
Native Tests - Data2 Build ⚠️ Check → Logs Raw logs
Native Tests - Data3 Build ⚠️ Check → Logs Raw logs
Native Tests - Data4 Build ⚠️ Check → Logs Raw logs
Native Tests - HTTP ⚠️ Check → Logs Raw logs
Native Tests - Messaging1 Build ⚠️ Check → Logs Raw logs
Native Tests - Misc1 ⚠️ Check → Logs Raw logs
Native Tests - Misc2 ⚠️ Check → Logs Raw logs
Native Tests - Misc3 ⚠️ Check → Logs Raw logs
Native Tests - Misc4 ⚠️ Check → Logs Raw logs
Native Tests - Security2 Build ⚠️ Check → Logs Raw logs
Native Tests - Security3 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Spring ⚠️ Check → Logs Raw logs
Native Tests - Windows - hibernate-validator ⚠️ Check → Logs Raw logs
Native Tests - gRPC ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 4, 2021

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

Failing Jobs - Building 843944a

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

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

io.quarkus.resteasy.reactive.server.test.multipart.LargerThanDefaultFormAttributeMultipartFormInputTest.test line 59 - More details - Source on GitHub


⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

io.quarkus.resteasy.reactive.server.test.multipart.LargerThanDefaultFormAttributeMultipartFormInputTest.test line 59 - More details - Source on GitHub


⚙️ JVM Tests - JDK 16 #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

io.quarkus.resteasy.reactive.server.test.multipart.LargerThanDefaultFormAttributeMultipartFormInputTest.test line 59 - More details - Source on GitHub

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

I think the multipart structures exposed via the API changed, no? If that's true we need to update the docs too.

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

Sorry I was thinking of the move QuarkusFileUpload -> DefaultFileUpload but it turns out they implement an interface which hasn't moved, so we're good :)

@geoand
Copy link
Contributor

geoand commented Jun 4, 2021

The LargerThanDefaultFormAttributeMultipartFormInputTest test which is relevant seems to be failing

This gives us a lot more flexibility, and allows it to work
with multiple backends.

Fixes quarkusio#17430
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.

RESTEasy Reactive: read ContainerRequestContext getEntityStream in filter resource method payload becomes null
3 participants