Merge pull request #1190 from tylerdooling/1162

Bypass formatting for statuses with no entity-body

Author: dblock

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).

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

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

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.
       #

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

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|