spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.63k stars 38.14k forks source link

Prefer problem detail media type independent of ordering in Accept header #29588

Closed osiegmar closed 1 year ago

osiegmar commented 1 year ago

While replacing a custom implementation of Problem Details by the new one that came with Spring 6 I was puzzled by how content negotiation is implemented. I'm currently using Spring Boot 3.0.0 with Spring 6.0.2.

I'm getting a Content-Type application/problem+json as expected when sending no accept header or Accept: */* or Accept: application/problem+json, application/json.

I also expected to receive a Problem Details JSON with Content-Type application/problem+json when sending Accept: application/json. But that might be correct - it doesn't seem to be clearly defined by RFC 7807.

Clearly a bug, seems that I'm still getting Content-Type application/json when sending Accept: application/json, application/problem+json. See also:

Message converters and encoders indicate a preference for application/problem+json when ProblemType is serialized. This ensures application/problem+json is preferred when the client is flexible or has no preference.

from: https://github.com/spring-projects/spring-framework/issues/28189#issuecomment-1121378986

ralf-ueberfuhr-ars commented 1 year ago

There's the problem. If not already existing, I could make a PR with a solution.

image

rstoyanchev commented 1 year ago

I wouldn't call it broken. We're returning JSON after, but we can refine the behavior.

ralf-ueberfuhr-ars commented 1 year ago

There're multiple problems with the current behavior:

image

rstoyanchev commented 1 year ago

@ralf-ueberfuhr-ars, thanks for your time and feedback, and for the attached draft PR. Note that I have local changes in progress too, but before implementation details, we should discuss desired behavior.

For the Adidas API guidelines, I presume you're referring to:

Problem Detail is intended for use with the HTTP status codes 4xx and 5xx. Problem Detail MUST NOT be used with 2xx status code responses.

This is not something we consider explicitly because there should never be a 2xx response with ProblemDetail. Any such response is a programming error, illegal state essentially. At best we can turn it into a 500 error.

rstoyanchev commented 1 year ago

@osiegmar, thinking further about:

I also expected to receive a Problem Details JSON with Content-Type application/problem+json when sending Accept: application/json.

Clearly a bug, seems that I'm still getting Content-Type application/json when sending Accept: application/json, application/problem+json.

We compare Accept header media types to supported media types (i.e. those supported by each HttpMessageConverter), and the outer loop is by Accept header, so the order of acceptable has priority over the order of supported media types.

For ProblemDetail the Jackson converter lists application/problem+json first, followed by application/*+json and application/json. This is why problem+json is favored when the client has no preference. But if the client requests json as the first and/or only media type, then json is used.

I think you are proposing to always use problem+json or problem+xml when ProblemDetails is rendered, and never application/json or application/xml. We can do that by changing the Jackson converter to return problem+json as the only supported media type for ProblemDetail, and hence jsonis never selected. I think it's okay to do that, so if the client understandsproblem+json` it can recognise the response reliably, or otherwise if it doesn't understand it, it can still see that it is kind of JSON.

It means we would mostly ignore the Accept header for ProblemDetail, unless it has problem+json and problem+xml, potentially influencing in what order they are selected.

osiegmar commented 1 year ago

Thanks for all your feedback and analysis @ralf-ueberfuhr-ars and @rstoyanchev!

After thinking about it for a while now and re-reading RFC 7807 my conclusion would be:

1) A JSON based Problem Details response exclusively comes with a Content-Type header of application/problem+json. This is based on section 3 of RFC 7807:

When serialized as a JSON document, that format is identified with the "application/problem+json" media type.

2) A XML based Problem Details response exclusively comes with application/problem+xml according to Appendix A of the RFC.

3) A XML based Problem Details response is returned only if the client explicitly asked for it via Accept header of value */*xml* (like text/xml, application/xml or application/problem+xml). Otherwise the JSON variant is returned. As the Problem Details functionality has to be opted in by spring.mvc.problemdetails.enabled anyway, I don't see this (default to JSON Problem Details) would break backwards compatibility.

Did I miss something? What do you think?

ueberfuhr commented 1 year ago

I agree, and I would add:

  1. If the value to convert is NOT a ProblemDetail, then we should NOT return the application/problem+json (or XML) type.
    1. In case of a 2xx response code: We should ignore the problem content types if they are sent with the Accept header. If there isn't any other matching content type, we should return the 406 status code.
    2. In case of a 4xx/5xx response code: We could use a "normal" content type to render the response (does this have to match the Accept header?) or - if the problem content type is preferred by the Accept header, we could automatically wrap it into a ProblemDetails object (?)

Further points:

An example for the last bullet:

(Excuse my complicated way of thinking. 😅) (And excuse that I currently work with 2 GitHub accounts at the same time - I'll use this account from now on.)

osiegmar commented 1 year ago
  • Is it correct, that ProblemDetails is currently not supported to be rendered as XML? Should we simply add JAXB annotations to ProblemDetails to get support from the corresponding converter?

Good catch! I just tried it and received an invalid Problem Details XML document:

curl -H "Accept: application/problem+xml" -i http://localhost:8080/rest/foo
<ProblemDetail><type>about:blank</type><title>Not Found</title><status>404</status><detail>No endpoint GET /rest/foo.</detail><instance>/rest/foo</instance></ProblemDetail>

The root node has to be <problem xmlns="urn:ietf:rfc:7807"> per Appendix A of the RFC. Also a <?xml version="1.0" encoding="UTF-8"?> header is prepended in the RFC example.

Need to think about the other points 😴 😄

ueberfuhr commented 1 year ago

The question is, what sense does it make for a client to specify an Accept Header of only a problem type? Nobody requests the server especially to expect a problem. 😉 And what would then be the format of the 2xx response? (I guess JSON for objects, and plain text for strings, right?) As already said, specifying a problem type in the Accept header would only make sense, if the client tells the server, which format of problem responses it is able to deal with, or which it prefers.

ueberfuhr commented 1 year ago

@ralf-ueberfuhr-ars, thanks for your time and feedback, and for the attached draft PR. Note that I have local changes in progress too, but before implementation details, we should discuss desired behavior.

For the Adidas API guidelines, I presume you're referring to:

Problem Detail is intended for use with the HTTP status codes 4xx and 5xx. Problem Detail MUST NOT be used with 2xx status code responses.

This is not something we consider explicitly because there should never be a 2xx response with ProblemDetail. Any such response is a programming error, illegal state essentially. At best we can turn it into a 500 error.

No, I meant

The application/problem+json (Problem Detail) MUST be used to communicate details about an error.

Because this would break with the initial problem reported by @osiegmar.

osiegmar commented 1 year ago

This is getting quite complex. I think we should separate issues.

1) This issue was/is about:

2) The Problem Details XML format is invalid per RFC I created https://github.com/spring-projects/spring-framework/issues/29927 for that.

3) @ueberfuhr raised several concerns/ideas/questions regarding Content Negotiation for implicit/explicit error responses. I'd suggest you also create a separate issue for that and link it to this one.

rstoyanchev commented 1 year ago

Thanks again for the thoughts and feedback.

I've made a change to the Jackson message converter to return application/problem+json as the only supported media type when writing ProblemDetail, so it should always be written with that media type irrespective of the presence of application/json in the Accept header.

As @ueberfuhr mentioned the opposite is also true: A non-Problem Details response MUST NOT neither use a Content-Type header value of application/problem+json nor application/problem+xml.

The Jackson message converter returns application/problem+json as a supported media type only for ProblemDetail, so I don't expect it should be possible to render any other object with that media type.

If you could please give it a try. The changes are now in the latest 6.0.5-SNAPSHOT.

rstoyanchev commented 1 year ago

I will respond separately under #29927 for application/problem+xml support.

For selecting JSON or XML problem details, I don't think the presence of application/xml should make a difference. After the current change, we effectively ignore the Accept header unless it has problem detail media types or wildcards, on the assumption that anything else is for a success response, and does not necessarily apply to an error response.

For example, I can imagine a client that calls a REST API using different content types depending on the specific endpoints, but otherwise expecting to handle error responses as application/problem+json. In any case, I expect clients would typically support at least JSON, but possibly JSON and XML problem details. I also can't imagine a very good reason to have a strong preference for XML problem details, in which case one still has the option to add it to the Accept header.

For the time being we will not treat application/xml specially for RFC 7807 responses, and will always write ProblemDetail as application/problem+json first unless application/problem+xml is explicitly requested.

osiegmar commented 1 year ago

Thanks for your feedback and fix @rstoyanchev! I gave 6.0.5-SNAPSHOT a try and now almost all my tests are green.

Now waiting for related https://github.com/spring-projects/spring-boot/issues/33716 to be fixed to solve the others. ;-)

rstoyanchev commented 1 year ago

Thanks for confirming, much appreciated.