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

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

Closed
osiegmar opened this issue Nov 27, 2022 · 15 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@osiegmar
Copy link

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: #28189 (comment)

@ralf-ueberfuhr-ars
Copy link

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

image

@rstoyanchev
Copy link
Contributor

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

@rstoyanchev rstoyanchev self-assigned this Jan 31, 2023
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 31, 2023
@rstoyanchev rstoyanchev added this to the 6.0.5 milestone Jan 31, 2023
@rstoyanchev rstoyanchev changed the title Content negotiation broken for Problem Details response Prefer problem detail media type independent of ordering in Accept header Jan 31, 2023
@ralf-ueberfuhr-ars
Copy link

There're multiple problems with the current behavior:

  • It does not match API guidelines that require the problem-specific content-type (e.g. https://adidas.gitbook.io/api-guidelines/rest-api-guidelines/message/error-reporting)
  • There're further issues, e.g. with a given Accept request header of application/problem+json,application/json, the response will return a Content-Type header of application/problem+json in the case of the successful request handling.
  • I have written further tests, see the details in the image. I'll try to make a PR for that.

image

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Feb 2, 2023

@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
Copy link
Contributor

rstoyanchev commented Feb 2, 2023

@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
Copy link
Author

osiegmar commented Feb 2, 2023

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.

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

  2. 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
Copy link

ueberfuhr commented Feb 2, 2023

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:

  • 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?
  • To get the response content type, we only compare the Accept header with the converter's provided formats. We completely ignore the controller's RequestMapping(produces=...) (which seems okay for me), but would it be a good idea maybe to get this feature for the exception handling, e.g. @ExceptionHandler(produces=..)? The background is, that in OpenAPI, we explicitly define the content types for the response, so one could provide different formats for different response codes.

An example for the last bullet:

  • Request with Accept: application/json
  • Controller handles the request and an error occurs
  • OpenAPI tells that all error responses are of type application/problem+xml (for whatever reason - network proxies...)
  • We then should render the ProblemDetails as application/problem+xml and break with the Accept header. This might not be the recommended way, but it is the scope of the API designer and the implementing developer that has to specify this behaviour explicitely and declarative using @ExceptionHandler(produces=..).

(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
Copy link
Author

osiegmar commented Feb 2, 2023

  • 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
Copy link

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
Copy link

@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
Copy link
Author

osiegmar commented Feb 3, 2023

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

  1. This issue was/is about:
  • Response body has to match the Content-Type. If the response body contains a Problem Details document the Content-Type has to be application/problem+json for JSON documents or application/problem+xml for XML documents – exclusively. This is what the RFC says and why I called the current Spring implementation "broken".
  • 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.
  • We should not expect the client to specify an Accept header of application/problem+json or application/problem+xml in order to send a Problem Details report. In practice this almost never happens. We already explicitly enabled Problem Details use via spring.mvc.problemdetails.enabled.
  • In order do decide between JSON/XML Problem Details we should default on JSON and only send application/problem+xml if the client requested a XML document (Accept header value of */*xml*).
  1. The Problem Details XML format is invalid per RFC
    I created Support ProblemDetail serialization to XML with Jackson #29927 for that.

  2. @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
Copy link
Contributor

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
Copy link
Contributor

rstoyanchev commented Feb 7, 2023

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
Copy link
Author

osiegmar commented Feb 8, 2023

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 spring-projects/spring-boot#33716 to be fixed to solve the others. ;-)

@rstoyanchev
Copy link
Contributor

Thanks for confirming, much appreciated.

rstoyanchev added a commit that referenced this issue Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants