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

Not all Jackson exceptions mapper have been overruled #317

Open
chberger opened this issue Oct 30, 2023 · 3 comments
Open

Not all Jackson exceptions mapper have been overruled #317

chberger opened this issue Oct 30, 2023 · 3 comments

Comments

@chberger
Copy link
Contributor

Quarkus: 3.4.3
Resteasy-Problem: 3.0.0

It seems like that not all Jackson exceptions are handled by resteasy-problem.

image

Although there is a mapper for JsonProcessingException, both child exceptions MismatchedInputException and InvalidDefinitionException are handled by Jackson itself.

According to the spec, this seems totally fine:

When choosing an exception mapping provider to map an exception, an implementation MUST use the
provider whose generic type is the nearest superclass of the exception. If two or more exception providers
are applicable, the one with the highest priority MUST be chosen as described in Section 4.1.3.

That is a little annoying cause it looks like you need to overrule each exception mapper individually.

Do you have any strategy in place to detected new mappers automatically?

@lwitkowski
Copy link
Collaborator

Our strategy till now was to simply wait for the community to report missing mappers... I guess your screenshot shows DevUI component?

I can think of 2 improvements:

  • create integration test that will fail if a mapper from ourside of this extension is detected
  • add warn message during Quarkus startup if such 'alien' mapper is detected, to warn developers that not all error responses may comply with problem standard. This can potentially produce false positives if someone creates a mapper for custom exception and maps it into proper problem response.

@chberger
Copy link
Contributor Author

Our strategy till now was to simply wait for the community to report missing mappers... I guess your screenshot shows DevUI component?

Yeah. it's the dev-ui component of resteasy-reactive.

I can think of 2 improvements:

* create integration test that will fail if a mapper from ourside of this extension is detected

This could work if you integrate with Quarkus (main) regularly. AFAIK you already do that, right?

* add warn message during Quarkus startup if such 'alien' mapper is detected, to warn developers that not all error responses may comply with problem standard. This can potentially produce false positives if someone creates a mapper for custom exception and maps it into proper problem response.

From a user perspective, I would prefer the first option. Overall, I guess this is quite important because as a user I rely on a proper RFC7807 error format.

@lwitkowski
Copy link
Collaborator

* create integration test that will fail if a mapper from ourside of this extension is detected

This could work if you integrate with Quarkus (main) regularly. AFAIK you already do that, right?

We bump Quarkus version here regularly, and everytime it happens there's a big regression test run checking if master codebase is still compatible with latest Quarkus. We would see a failing test if new Quarkus version adds new exception mapper, or changes the priority of existing one, so that our gets overriden.

* add warn message during Quarkus startup if such 'alien' mapper is detected, to warn developers that not all error responses may comply with problem standard. This can potentially produce false positives if someone creates a mapper for custom exception and maps it into proper problem response.

From a user perspective, I would prefer the first option. Overall, I guess this is quite important because as a user I rely on a proper RFC7807 error format.

Second option would be also handy in case other quarkus extensions provide exception mappers - today it goes unnoticed. We can't cover all possible extensions in our tests suit and keep track of their releases.

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

No branches or pull requests

2 participants