diff --git a/CHANGELOG.md b/CHANGELOG.md index 866302f7e..bded9531f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ #### Fixes +* [#1194](https://github.com/ruby-grape/grape/pull/1194): Redirect as plain text with message - [@tylerdooling](https://github.com/tylerdooling). * [#1185](https://github.com/ruby-grape/grape/pull/1185): Use formatters for custom vendored content types - [@tylerdooling](https://github.com/tylerdooling). * [#1156](https://github.com/ruby-grape/grape/pull/1156): Fixed `no implicit conversion of Symbol into Integer` with nested `values` validation - [@quickpay](https://github.com/quickpay). * [#1153](https://github.com/ruby-grape/grape/pull/1153): Fixes boolean declaration in an external file - [@towanda](https://github.com/towanda). diff --git a/UPGRADING.md b/UPGRADING.md index dfcc5731f..4ba58e41c 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -23,6 +23,15 @@ be bypassed when processing responses for status codes defined [by rack](https:/ See [#1190](https://github.com/ruby-grape/grape/pull/1190) for more information. +#### Redirects respond as plain text with message + +`#redirect` now uses `text/plain` regardless of whether that format has +been enabled. This prevents formatters from attempting to serialize the +message body and allows for a descriptive message body to be provided - and +optionally overridden - that better fulfills the theme of the HTTP spec. + +See [#1194](https://github.com/ruby-grape/grape/pull/1194) for more information. + ### Upgrading to >= 0.12.0 #### Changes in middleware diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index c6aac88d2..e5ed63f45 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -91,19 +91,25 @@ def error!(message, status = nil, headers = nil) # @param url [String] The url to be redirect. # @param options [Hash] The options used when redirect. # :permanent, default false. + # :body, default a short message including the URL. def redirect(url, options = {}) - merged_options = options.reverse_merge(permanent: false) - if merged_options[:permanent] + permanent = options.fetch(:permanent, false) + body_message = options.fetch(:body, nil) + if permanent status 301 + body_message ||= "This resource has been moved permanently to #{url}." else if env[Grape::Http::Headers::HTTP_VERSION] == 'HTTP/1.1' && request.request_method.to_s.upcase != Grape::Http::Headers::GET status 303 + body_message ||= "An alternate resource is located at #{url}." else status 302 + body_message ||= "This resource has been moved temporarily to #{url}." end end header 'Location', url - body '' + content_type 'text/plain' + body body_message end # Set or retrieve the HTTP status code. diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index e894333f5..e7286c390 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -752,7 +752,7 @@ def app get '/hey' expect(last_response.status).to eq 302 expect(last_response.headers['Location']).to eq '/ha' - expect(last_response.body).to eq '' + expect(last_response.body).to eq 'This resource has been moved temporarily to /ha.' end it 'has status code 303 if it is not get request and it is http 1.1' do @@ -762,6 +762,7 @@ def app post '/hey', {}, 'HTTP_VERSION' => 'HTTP/1.1' expect(last_response.status).to eq 303 expect(last_response.headers['Location']).to eq '/ha' + expect(last_response.body).to eq 'An alternate resource is located at /ha.' end it 'support permanent redirect' do @@ -771,7 +772,15 @@ def app get '/hey' expect(last_response.status).to eq 301 expect(last_response.headers['Location']).to eq '/ha' - expect(last_response.body).to eq '' + expect(last_response.body).to eq 'This resource has been moved permanently to /ha.' + end + + it 'allows for an optional redirect body override' do + subject.get('/hey') do + redirect '/ha', body: 'test body' + end + get '/hey' + expect(last_response.body).to eq 'test body' end end