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

Mime Support for "problem detail" #44608

Merged
merged 2 commits into from Mar 18, 2022
Merged

Mime Support for "problem detail" #44608

merged 2 commits into from Mar 18, 2022

Conversation

MGatner
Copy link
Contributor

@MGatner MGatner commented Mar 3, 2022

Summary

RFC7807 defines a "problem detail" as

a way to carry machine-readable details of errors in a HTTP response to avoid the need to define new error response formats for HTTP APIs

The spec is somewhat irrelevant, but due to the extended content type Rails does not recognize application/problem+json as JSON (particularly when parsing test Response bodies). This PR proposes adding it as an additional recognized Mime subtype for application/json

Other Information

The largest counterpoint I can think of is the previous addition and removal of application/vnd.api+json (see #23712). Unlike vnd.api+json, problem_json is an RFC-specific format that is universal - not vendor-specific - and native support would add lots of value to Rails API projects.

@rails-bot rails-bot bot added the actionpack label Mar 3, 2022
@MGatner
Copy link
Contributor Author

MGatner commented Mar 3, 2022

I'm not sure what the "RootLess" test variants denote but that one fails where the other passes so I have removed it - maybe a reviewer can specify if that is important.

@tenderlove
Copy link
Member

I think this PR is totally fine. That said, I read through the PRs that you linked to for context, and I'm not really clear what's going on in those PRs so I don't get why it was reverted 😅

From what I understand, vnd.api+json indicates it's custom for the particular vendor, but it's still in JSON format so I'm not sure why we don't treat it that way.

Anyway, I'm going to merge this but I just wanted to write down what I was thinking as I reviewed the PR.

Thanks for the work!

@tenderlove tenderlove merged commit 97abaa7 into rails:main Mar 18, 2022
@MGatner
Copy link
Contributor Author

MGatner commented Mar 19, 2022

Awesome! Thanks for the review and merge.

@MGatner MGatner deleted the problem-json branch March 19, 2022 11:37
pixeltrix added a commit to pixeltrix/rails that referenced this pull request Mar 20, 2022
In rails#44608 the application/problem+json content type was added as a
default alias for json but as the test only fails intermittently
it wasn't picked up during PR review. It also restores the rootless
test that was reverted due to it failing for the same reason.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants