Resteasy Problem might adhere to RFC 9457 (Problem Details for HTTP APIs)

chberger commented 10 months ago

Is your feature request related to RFC7807? Please describe.

The positive experience of RFC 7807, whose journey began in 2016, is concluded (deprecation) but also confirmed with a new official proposition: the RFC 9457 (published July 2023).

RFC 7807 is pretty comprehensive and has served already for some time, but RFC 9457 brings some features that represent a positive evolution of best practice. However, the changes between the two RFC versions are very small; helpfully the RFC itself includes a section outlining the changes.

The key items here are around representing multiple problems, providing guidance for using type URIs that cannot be dereferenced and using a shared registry of problem types.

Describe the solution you'd like

Especially the section 3 clarifies how multiple problems should be treated. So they have a strong opinion on how to design an error response returning more than one instance of the same problem type. For instance:

HTTP/1.1 422 Unprocessable Content
Content-Type: application/problem+json
Content-Language: en

{
 "type": "https://example.net/validation-error",
 "title": "Your request is not valid.",
 "errors": [
             {
               "detail": "must be a positive integer",
               "pointer": "#/age"
             },
             {
               "detail": "must be 'green', 'red' or 'blue'",
               "pointer": "#/profile/color"
             }
          ]
}

The problem type here defines the "errors" extension, an array that describes the details of each validation error. Each member is an object containing "detail" to describe the issue and "pointer" to locate the problem within the request's content using a JSON Pointer [JSON-POINTER].

While having https://github.com/TietoEVRY/quarkus-resteasy-problem/pull/313 and https://github.com/TietoEVRY/quarkus-resteasy-problem/pull/314 ready, the implementation of com.tietoevry.quarkus.resteasy.problem.validation.ConstraintViolationExceptionMapper could be easily adapted to comply with RFC9457. Maybe this would be a first good candidate to evolve with the spec changes/enhancements.

lwitkowski commented 10 months ago

Thanks for the link, I wasn't aware of new version of RFC, really cool!

ConstraintViolationExceptionMapper is already very close to this proposal, we have violations instead of errors, message instead of detail, and field (+in) instead of pointer - or am I missing something?

We could make it configurable (opt-in) to keep it backward compatible.

chberger commented 10 months ago

Thanks for the link, I wasn't aware of new version of RFC, really cool!

ConstraintViolationExceptionMapper is already very close to this proposal, we have violations instead of errors, message instead of detail, and field (+in) instead of pointer - or am I missing something?

And the http status code 422 instead of 400. Which isn't a big deal anymore. But yeah, that's it. 👍

We could make it configurable (opt-in) to keep it backward compatible.

Sure, sounds fair enough.

An alternative would be to maintain a new 4.X stream where you can tackle all other discussed topics like:

Getters instead of public members in HttpProblem
Hide public access to register ProblemPostProcessors
ConstraintViolationExceptionMapper comply with RFC 9457
I have some more topics in my mind (which we haven't discussed yet):
- Handling of Throwable
- Considering the priority definition of PostProblemProcessor vs. ExceptionMapper,
- Default problem logging happens before default post processor modifications.

I did not analyze the latter topics in detail yet. So I'm not sure if these are actually valid points. Just my thoughts on how to use the extension ...

lwitkowski commented 10 months ago

Thanks for the link, I wasn't aware of new version of RFC, really cool! ConstraintViolationExceptionMapper is already very close to this proposal, we have violations instead of errors, message instead of detail, and field (+in) instead of pointer - or am I missing something?

And the http status code 422 instead of 400. Which isn't a big deal anymore. But yeah, that's it. 👍

I don't see it in RFC to be frank. 422 response is used as an example of multi-problem response, but I don't see any indication regarding 422 being a replacement for 400 in general.

An alternative would be to maintain a new 4.X stream where you can tackle all other discussed topics like:

The idea is to keep major version aligned with Quarkus versions. It puts a constraint into the type of changes we can or cannot do here, but I think it's worth it, as it makes less confusion (we don't have to explain that quarkus-resteasy-problem 5.X.X is compatible with Quarkus 7.X.X). Migrating into Quarkiverse will not make it easier, as we will also have to keep backward compatibility with Quarkus minor releases.

chberger commented 10 months ago

Thanks for the link, I wasn't aware of new version of RFC, really cool! ConstraintViolationExceptionMapper is already very close to this proposal, we have violations instead of errors, message instead of detail, and field (+in) instead of pointer - or am I missing something?

And the http status code 422 instead of 400. Which isn't a big deal anymore. But yeah, that's it. 👍

I don't see it in RFC to be frank. 422 response is used as an example of multi-problem response, but I don't see any indication regarding 422 being a replacement for 400 in general.

The http response code is a recommendation in the same way as the whole response structure for multi-problem responses. The spec is not saying you have to do it. If you would align the structure, but keep the http status code 400 for violations, then it's your decision. However, IMHO then you don't follow the recommendation.

Edit. You're right. The http response code is not explicitly mentioned in the RFC itself and thus let's ignore it.

An alternative would be to maintain a new 4.X stream where you can tackle all other discussed topics like:

The idea is to keep major version aligned with Quarkus versions. It puts a constraint into the type of changes we can or cannot do here, but I think it's worth it, as it makes less confusion (we don't have to explain that quarkus-resteasy-problem 5.X.X is compatible with Quarkus 7.X.X). Migrating into Quarkiverse will not make it easier, as we will also have to keep backward compatibility with Quarkus minor releases.

IMHO this an artificial limitation which you guys agreed up on. I'm a maintainer of a Quarkiverse extension as well and nobody forced me to align version numbers or support old Quarkus versions.

lwitkowski commented 10 months ago

IMHO this an artificial limitation which you guys agreed up on. I'm a maintainer of a Quarkiverse extension as well and nobody forced me to align version numbers or support old Quarkus versions.

Yes, and no.

Yes, because in Quarkiverse your main/master branch must be compatible only with latest Quarkus, outside Quarkiverse we aim to keep 3.1.0 compatible with all 3.X.X version of Quarkus, which makes it more challenging.

No, because even in Quarkiverse, if you want to follow semver principles, you should not make breaking changes for your users with minor releases. Now the question is - minor releases of what? Extension or Quarkus? I'd say: Quarkus, because extension versions are hidden from end users, they just include quarkiverse bom in their poms, which gives you, as extension developer, a certain freedom, but in the end - extension should not break anything when I update Quarkus from X.Y.Z to X.(Y+1).Z.

On the other hand Quarkus sometimes break things in minor releases, but I choose to believe it's not on purpose ;)

quarkiverse / quarkus-resteasy-problem

Resteasy Problem might adhere to RFC 9457 (Problem Details for HTTP APIs) #315