quarkiverse / quarkus-resteasy-problem

Unified error responses for Quarkus REST APIs via Problem Details for HTTP APIs (RFC9457 & RFC7807)
https://docs.quarkiverse.io/quarkus-resteasy-problem/dev
Apache License 2.0
69 stars 12 forks source link

Not all Jackson exceptions mapper have been overruled #317

Open chberger opened 1 year ago

chberger commented 1 year ago

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 commented 1 year ago

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:

chberger commented 1 year ago

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 commented 1 year ago
* 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.