Render a redirect message as the plain text it claims to be - #2845
Render a redirect message as the plain text it claims to be#2845ericproulx wants to merge 1 commit into
Conversation
63cf9a3 to
4e7d9d5
Compare
Danger ReportNo issues found. |
dblock
left a comment
There was a problem hiding this comment.
This is a backwards incompatible change, right? Needs UPGRADING?
dblock
left a comment
There was a problem hiding this comment.
This is a backwards incompatible change, right? Needs UPGRADING?
e2a1c9e to
02da8bb
Compare
|
You're right on both counts — thanks, my "no UPGRADING entry" claim was wrong. The body of a redirect changes on any API whose format isn't Checking the blast radius also turned up something the first version got wrong. Setting redirect '/there', body: { message: 'moved' }
# before: {"message":"moved"}
# then: {message: "moved"} # Hash#to_s — neither JSON nor useful textSo it's now scoped to the message Grape generates, which is the one body known to be plain text: api_format :txt unless bodyThe trade-off is that |
#redirect announces its message as text/plain and has done since it was
introduced in 2015 ("Redirect as plain text with optional message override"),
but it only set the header. The body was still handed to the API's own
formatter, so on a JSON API the sentence came back JSON-encoded:
format :json
get('/r') { redirect '/there' }
Content-Type: text/plain
"This resource has been moved temporarily to /there."
quotes included -- neither valid plain text nor something a client reading the
content type would expect. The existing specs missed it because they run on
the default :txt format, where the formatter is a no-op.
Set api.format alongside the header, the same lever an endpoint already has
via #api_format, so the message is rendered by the txt formatter whatever the
API declares. It is per-request env, so other routes on the same API are
untouched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
02da8bb to
d861d6e
Compare
| ``` | ||
|
|
||
| Nothing changes for the ordinary arrangement — registrations declared before a route, or inherited from an enclosing namespace or a mounting API, still apply exactly as before, including values an enclosing scope gains after the nested scope was created. | ||
| #### `redirect` renders its default message as plain text |
| Nothing changes for the ordinary arrangement — registrations declared before a route, or inherited from an enclosing namespace or a mounting API, still apply exactly as before, including values an enclosing scope gains after the nested scope was created. | ||
| #### `redirect` renders its default message as plain text | ||
|
|
||
| `redirect` has announced its body as `text/plain` since it was introduced in 0.14.0, but it only ever set the header — the message itself was still handed to the API's own formatter. On an API declaring a format other than `:txt`, the sentence came back encoded by that formatter under a `text/plain` content type: |
There was a problem hiding this comment.
I think this can be explained simpler.
The redirect API set the content-type to text/plain, but delegated body rendering to the formatter. With an API defaulting to JSON, a redirect would render the body as JSON with a text/plain content-type header.
Example: ...
Summary
#redirectannounces its message astext/plain— and has since it was introduced in 2015, in a commit literally titled "Redirect as plain text with optional message override" — but it only ever set the header. The body was still handed to the API's own formatter:Quotes included. That is neither valid plain text nor what a client reading the content type would expect, and the header and body contradict each other.
format :jsontext/plain+"…moved temporarily to /there."(quoted)text/plain+…moved temporarily to /there.format :txtformat :xmlThe existing
#redirectspecs missed it because they run on the default:txtformat, where the formatter is a no-op.Approach
Set
api.formatalongside the header for the generated message — the same lever an endpoint already has via#api_format(spec'd atapi_spec.rb:4494) — so the message is rendered by the txt formatter whatever the API declares.:txtis always resolvable:Grape::Formatter.formatter_forfalls back to the built-in registry, which is not narrowed by the API'sformat.It is per-request rack env, so other routes on the same API are unaffected — there is a spec pinning that.
Backward compatibility
UPGRADING entry added — @dblock is right that this is a breaking change, and the original description of it here was wrong.
The body of a redirect changes on any API whose format is not
:txt. Concretely,JSON.parse(response.body)on a redirect succeeded before and now raises. TheLocationheader, the status and theContent-Typeare all unchanged, so a client that follows the redirect is unaffected — but code that reads the body is.Longstanding rather than a regression: present since v0.14.0 (
d1bba79d, 2015) and identical in 3.3.4.Scoped to the generated message
The first version of this PR set
api_format :txtunconditionally, which also caught a body the caller passed. That was worse than the bug on a JSON API:So the format is now only forced for the message Grape generates, which is the one body known to be plain text:
A caller-supplied body keeps the API's format and is untouched by this PR. The trade-off is that
redirect url, body: 'go away'on a JSON API still returns"go away"quoted undertext/plain— the same contradiction, for a body Grape cannot assume anything about. Narrowing it further (say, rendering anyStringbody as text) is a judgement call I left out; happy to widen it if you'd prefer.Test plan
endpoint_spec.rbunder#redirect: plain-text body on a JSON API, a structured and a string caller-supplied body both keeping the API's format, and a guard that other routes keep it too.🤖 Generated with Claude Code