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

Allows users to exclude DefaultMismatchedInputException #43126

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Sep 9, 2024

A few users have run into an issue where
they are trying to handle all Jackson exceptions
but the existence of DefaultMismatchedInputException
prevents that for MismatchedInputException
(as per JAX-RS / Jakarta REST rules).

This change allows users to do use:

quarkus.class-loading.removed-resources."io.quarkus\:quarkus-rest-jackson"=io/quarkus/resteasy/reactive/jackson/runtime/mappers/DefaultMismatchedInputException.class

in order to have Quarkus ignore the mapper.

P.S. We could also introduce a specific property for this, but currently I am on the fence about that...

Copy link

github-actions bot commented Sep 9, 2024

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

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.

Hmmm, I have a few comments here.

Should we rename the class to DefaultMismatchedInputExceptionMapper before documenting it somewhere? When I read the description, I thought it was an exception and had a hard time understanding the issue.

Second question: isn't it easier to create a mapper with a higher priority that specifically handles this exception instead of promoting a rather obscure feature?

And finally, I wonder if at some point, these dev modes features should be handled specifically after the exception mappers have been applied (and only be triggered if no ExceptionMapper handled the exception).

@geoand
Copy link
Contributor Author

geoand commented Sep 9, 2024

Should we rename the class to DefaultMismatchedInputExceptionMapper before documenting it somewhere? When I read the description, I thought it was an exception and had a hard time understanding the issue

How about BuiltinMismatchedInputExceptionMapper?

Second question: isn't it easier to create a mapper with a higher priority that specifically handles this exception instead of promoting a rather obscure feature?

That doesn't work (as per JAX-RS / Jakarta REST) when users want to handle a super class of MismatchedInputException.

And finally, I wonder if at some point, these dev modes features should be handled specifically after the exception mappers have been applied (and only be triggered if no ExceptionMapper handled the exception).

I don't really understand this, can you elaborate a little?

@gsmet
Copy link
Member

gsmet commented Sep 10, 2024

How about BuiltinMismatchedInputExceptionMapper?

Looks good.

That doesn't work (as per JAX-RS / Jakarta REST) when users want to handle a super class of MismatchedInputException.

Maybe I wasn't clear but my understanding was that they would have to have a specific mapper for MismatchedInputException. I thought it might be easier to grasp from a user perspective.

But you can start your way and see how to make it easier to grasp later.

I don't really understand this, can you elaborate a little?

I think it's not the first time we have dev mode features in the way of how you would expect a standard JAX-RS implementation to work.
Typically this feature we have is nice in dev mode but I think it shouldn't be in the way when you customize things on your side.

Thus why I was wondering if we should have a way to enable these features only if something from the user didn't catch things earlier.

Basically, we should find a way for our mappers (or not implement them as mappers) to be very last resort if nothing else from the users intervened first.

@geoand
Copy link
Contributor Author

geoand commented Sep 10, 2024

Maybe I wasn't clear but my understanding was that they would have to have a specific mapper for MismatchedInputException. I thought it might be easier to grasp from a user perspective.

That would work today without any configuration. What does not work as users would like, is handling all exceptions that are super classes of this exception.

Basically, we should find a way for our mappers (or not implement them as mappers) to be very last resort if nothing else from the users intervened first.

Yeah, that would be nice, but unfortunately the resolution rules don't really allow this - hence the ability to remove this one.

I think it's not the first time we have dev mode features in the way of how you would expect a standard JAX-RS implementation to work.

Agreed, but it's also surprising when code exists in dev-mode and affects the behavior of the system to not exist in prod mode, so I am really not sure what the best solution is here.

A few users have run into an issue where
they are trying to handle all Jackson exceptions
but the existence of DefaultMismatchedInputException
prevents that for MismatchedInputException
(as per JAX-RS / Jakarta REST rules).

This change allows users to do use:

quarkus.class-loading.removed-resources."io.quarkus\:quarkus-rest-jackson"=io/quarkus/resteasy/reactive/jackson/runtime/mappers/DefaultMismatchedInputException.class

in order to have Quarkus ignore the mapper.
@geoand
Copy link
Contributor Author

geoand commented Sep 10, 2024

PR updated

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

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.

I will handle the comment given I'm already on this subject, no need for you to context switch.

docs/src/main/asciidoc/rest.adoc Outdated Show resolved Hide resolved
@gsmet
Copy link
Member

gsmet commented Sep 10, 2024

Force pushed with the doc fix squashed!

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 10, 2024

Status for workflow Quarkus Documentation CI

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

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 10, 2024

Status for workflow Quarkus CI

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

✅ 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.

You can consult the Develocity build scans.

@geoand
Copy link
Contributor Author

geoand commented Sep 10, 2024

Thanks!

@gsmet gsmet merged commit bea1fbd into quarkusio:main Sep 10, 2024
35 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 10, 2024
@gsmet gsmet modified the milestones: 3.16 - main, 3.14.3 Sep 10, 2024
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.

2 participants