diff --git a/CHANGELOG.md b/CHANGELOG.md index 13795e507..866302f7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ * Your contribution here. +* [#1190](https://github.com/ruby-grape/grape/putt/1190): Bypass formatting for statuses with no entity-body - [@tylerdooling](https://github.com/tylerdooling). * [#1188](https://github.com/ruby-grape/grape/putt/1188): Allow parameters with more than one type - [@dslh](https://github.com/dslh). * [#1179](https://github.com/ruby-grape/grape/pull/1179): Allow all RFC6838 valid characters in header vendor - [@suan](https://github.com/suan). * [#1170](https://github.com/ruby-grape/grape/pull/1170): Allow dashes and periods in header vendor - [@suan](https://github.com/suan). diff --git a/README.md b/README.md index ddff3bcbb..c32059ae0 100644 --- a/README.md +++ b/README.md @@ -1976,6 +1976,11 @@ Built-in formatters are the following. * `:serializable_hash`: use object's `serializable_hash` when available, otherwise fallback to `:json` * `:binary`: data will be returned "as is" +Response statuses that indicate no content as defined by [Rack](https://github.com/rack) +[here](https://github.com/rack/rack/blob/master/lib/rack/utils.rb#L567) +will bypass serialization and the body entity - though there should be none - +will not be modified. + ### JSONP Grape supports JSONP via [Rack::JSONP](https://github.com/rack/rack-contrib), part of the diff --git a/UPGRADING.md b/UPGRADING.md index 44088f6f0..dfcc5731f 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -15,6 +15,14 @@ Identical endpoints with different versions now work correctly. A regression int See [#1114](https://github.com/ruby-grape/grape/pull/1114) for more information. +#### Bypasses formatters when status code indicates no content + +To be consistent with rack and it's handling of standard responses +associated with no content, both default and custom formatters will now +be bypassed when processing responses for status codes defined [by rack](https://github.com/rack/rack/blob/master/lib/rack/utils.rb#L567) + +See [#1190](https://github.com/ruby-grape/grape/pull/1190) for more information. + ### Upgrading to >= 0.12.0 #### Changes in middleware diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index 9b0f32739..f8a65ec14 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -21,35 +21,36 @@ def before def after status, headers, bodies = *@app_response - if bodies.is_a?(Grape::Util::FileResponse) - headers = ensure_content_type(headers) - - response = - Rack::Response.new([], status, headers) do |resp| - resp.body = bodies.file - end + if Rack::Utils::STATUS_WITH_NO_ENTITY_BODY.include?(status) + @app_response else - # Allow content-type to be explicitly overwritten - api_format = mime_types[headers[Grape::Http::Headers::CONTENT_TYPE]] || env[Grape::Env::API_FORMAT] - formatter = Grape::Formatter::Base.formatter_for(api_format, options) + build_formatted_response(status, headers, bodies) + end + end - begin - bodymap = bodies.collect do |body| - formatter.call(body, env) - end + private - headers = ensure_content_type(headers) + def build_formatted_response(status, headers, bodies) + headers = ensure_content_type(headers) - response = Rack::Response.new(bodymap, status, headers) - rescue Grape::Exceptions::InvalidFormatter => e - throw :error, status: 500, message: e.message + if bodies.is_a?(Grape::Util::FileResponse) + Rack::Response.new([], status, headers) do |resp| + resp.body = bodies.file end + else + # Allow content-type to be explicitly overwritten + formatter = fetch_formatter(headers, options) + bodymap = bodies.collect { |body| formatter.call(body, env) } + Rack::Response.new(bodymap, status, headers) end - - response + rescue Grape::Exceptions::InvalidFormatter => e + throw :error, status: 500, message: e.message end - private + def fetch_formatter(headers, options) + api_format = mime_types[headers[Grape::Http::Headers::CONTENT_TYPE]] || env[Grape::Env::API_FORMAT] + Grape::Formatter::Base.formatter_for(api_format, options) + end # Set the content type header for the API format if it is not already present. # diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index f6e6a1f9c..87afe9a80 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -537,16 +537,38 @@ def subject.enable_root_route! expect(last_response.headers['Content-Type']).to eql 'text/plain' end - it 'adds an OPTIONS route that returns a 204, an Allow header and a X-Custom-Header' do - subject.before { header 'X-Custom-Header', 'foo' } - subject.get 'example' do - 'example' + describe 'adds an OPTIONS route that' do + before do + subject.before { header 'X-Custom-Header', 'foo' } + subject.get 'example' do + 'example' + end + options '/example' + end + + it 'returns a 204' do + expect(last_response.status).to eql 204 + end + + it 'has an empty body' do + expect(last_response.body).to be_blank + end + + it 'has an Allow header' do + expect(last_response.headers['Allow']).to eql 'OPTIONS, GET, HEAD' + end + + it 'has a X-Custom-Header' do + expect(last_response.headers['X-Custom-Header']).to eql 'foo' + end + + it 'has no Content-Type' do + expect(last_response.content_type).to be_nil + end + + it 'has no Content-Length' do + expect(last_response.content_length).to be_nil end - options '/example' - expect(last_response.status).to eql 204 - expect(last_response.body).to eql '' - expect(last_response.headers['Allow']).to eql 'OPTIONS, GET, HEAD' - expect(last_response.headers['X-Custom-Header']).to eql 'foo' end it 'allows HEAD on a GET request' do diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb index 313c23631..85aaf6bc7 100644 --- a/spec/grape/middleware/formatter_spec.rb +++ b/spec/grape/middleware/formatter_spec.rb @@ -201,6 +201,18 @@ def to_xml end end + context 'no content responses' do + let(:no_content_response) { ->(status) { [status, {}, ['']] } } + + Rack::Utils::STATUS_WITH_NO_ENTITY_BODY.each do |status| + it "does not modify a #{status} response" do + expected_response = no_content_response[status] + allow(app).to receive(:call).and_return(expected_response) + expect(subject.call({})).to eq(expected_response) + end + end + end + context 'input' do %w(POST PATCH PUT DELETE).each do |method| ['application/json', 'application/json; charset=utf-8'].each do |content_type|