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

Improve JAXB Exception handling #39503

Merged
merged 1 commit into from
Apr 22, 2024
Merged

Conversation

ketola
Copy link
Contributor

@ketola ketola commented Mar 16, 2024

I tested the issue with resteasy-reactive-jaxb and confirmed that the response is Internal Server Error (500) when posting an invalid xml body.
I tried to reproduce the issue in the existing jaxb integration test, but that one returns Bad Request (400). I added tests for confirming this.
I had to create a new integration test module resteasy-reactive-jaxb to be able to reproduce the issue with the reactive implementation. That test has been implemented to expect Bad Request (400) so at the moment the test is failing until the exception handling is fixed.

@quarkus-bot

This comment was marked as resolved.

@geoand geoand changed the title #39375 JAXB Exception handling Improve JAXB Exception handling Mar 19, 2024
@geoand geoand requested a review from gsmet March 19, 2024 06:30
@ketola
Copy link
Contributor Author

ketola commented Mar 19, 2024

I'm not sure should I discuss the proposed fix here or in the issue. But here's what I commented in the issue earlier:

Based on my tests in the Draft PR I noticed that the resteasy-reactive-jaxb and resteasy-jaxb behave differently. The reactive implementation returns 500 when an invalid xml is sent and the other one returns 400. My suggestion is that resteasy-reactive-jaxb would be modified to work like resteasy-reactive-jackson works when invalid json is sent -> WebApplicationException resulting in http 400.
I could go on and try to fix it if this sounds like a plan? I would also need to add an integration test module as I have done in the pr to bring up the issue and verify the fix.

@ketola ketola force-pushed the 39375-jaxb-exception branch 2 times, most recently from be40ee8 to 98c138c Compare March 23, 2024 06:41
@ketola ketola marked this pull request as ready for review March 23, 2024 06:43
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 23, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 0a777ca.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@ketola
Copy link
Contributor Author

ketola commented Mar 23, 2024

Well I went ahead and implemented the exception handling in a similar way that it is done in jackson as it was not a big task and I changed the status to ready for review.
But for the future, if I would want some feedback for the plan on how I would do the fix before actually doing it, how should I go about it? Should I just change the status to ready for review even though the fix is not finished or tag the reviewer in a pr comment?

Copy link

😭 Deploy PR Preview failed.

@ketola ketola force-pushed the 39375-jaxb-exception branch 2 times, most recently from c67946c to bf7fa97 Compare March 23, 2024 11:35
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request.

I agree that the changes make sense and thank you for having a look into it and providing a pull request.

One minor gripe though: I would prefer if the test could be moved to https://github.com/quarkusio/quarkus/tree/main/extensions/resteasy-reactive/rest-jaxb/deployment/src/test/java/io/quarkus/resteasy/reactive/jaxb/deployment/test instead.

Adding an IT module means adding several minutes to the native build and I'm not sure it's worth it in this case.
A small test in the deployment module should be easier to add (you will have to adapt your test a bit though, have a look at the other tests that are already in the module).

Thanks!

@ketola ketola force-pushed the 39375-jaxb-exception branch 4 times, most recently from 3703e09 to 0729130 Compare March 27, 2024 13:42
@ketola
Copy link
Contributor Author

ketola commented Mar 27, 2024

@gsmet ok, yeah the deployment/runtime division confused me first and I didn't notice the module had tests also there. But I removed the integration tests and added a test for the invalid xml in the module tests 👍

If UnmarshalException is thrown (which happens for example when the xml
is invalid)  then a WebApplicationException is thrown with status code
400 Bad Request.

This is also how resteasy-reactive-jackson handles invalid JSON.
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks.

Let's get CI to run and merge it once it is green.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 17, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 17, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 744ccfb.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@gsmet gsmet merged commit 54db277 into quarkusio:main Apr 22, 2024
18 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 22, 2024
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone Apr 22, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Apr 22, 2024
@gsmet
Copy link
Member

gsmet commented Apr 22, 2024

Merged! Thanks for your contribution!

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.

Avoid throwing a RuntimeException when parsing an invalid XML body using JAXB
2 participants