Merge pull request #1550 from jthornec/use-200-for-delete-with-content

dblock · web-flow · commit 76549edbc848 · 2017-01-06T11:00:56.000-05:00
Use 200 as default status for deletes that reply with content
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@
 #### Fixes
 
 * [#1548](https://github.com/ruby-grape/grape/pull/1548): Avoid failing even if given path does not match with prefix - [@thomas-peyric](https://github.com/thomas-peyric), [@namusyaka](https://github.com/namusyaka).
+* [#1550](https://github.com/ruby-grape/grape/pull/1550): Use 200 as default status for deletes that reply with content - [@jthornec](https://github.com/jthornec).
 * Your contribution here.
 
 ### 0.19.0 (12/18/2016)
diff --git a/README.md b/README.md
@@ -1764,7 +1764,7 @@ cookies.delete :status_count, path: '/'
 
 ## HTTP Status Code
 
-By default Grape returns a 201 for `POST`-Requests, 204 for `DELETE`-Requests and 200 status code for all other Requests.
+By default Grape returns a 201 for `POST`-Requests, 204 for `DELETE`-Requests that don't return any content, and 200 status code for all other Requests.
 You can use `status` to query and set the actual HTTP Status Code
 
 ```ruby
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -1,6 +1,38 @@
 Upgrading Grape
 ===============
 
+### Upgrading to >= 0.19.1 (next)
+
+#### DELETE now defaults to status code 200 for responses with a body, or 204 otherwise
+
+Prior to this version, DELETE requests defaulted to a status code of 204 No Content, even when the response included content. This behavior confused some clients and prevented the formatter middleware from running properly. As of this version, DELETE requests will only default to a 204 No Content status code if no response body is provided, and will default to 200 OK otherwise.
+
+Specifically, DELETE behaviour has changed as follows:
+
+- In versions < 0.19.0, all DELETE requests defaulted to a 200 OK status code.
+- In version 0.19.0, all DELETE requests defaulted to a 204 No Content status code, even when content was included in the response.
+- As of version 0.19.1, DELETE requests default to a 204 No Content status code, unless content is supplied, in which case they default to a 200 OK status code.
+
+To achieve the old behavior, one can specify the status code explicitly:
+
+```ruby
+delete :id do
+  status 204 # or 200, for < 0.19.0 behavior
+  'foo successfully deleted'
+end
+```
+
+One can also use the new `return_no_content` helper to explicitly return a 204 status code and an empty body for any request type:
+
+```ruby
+delete :id do
+  return_no_content
+  'this will not be returned'
+end
+```
+
+See [#1550](https://github.com/ruby-grape/grape/pull/1550) for more information.
+
 ### Upgrading to >= 0.18.1
 
 #### Changes in priority of :any routes
diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb
@@ -130,7 +130,11 @@ def status(status = nil)
           when Grape::Http::Headers::POST
             201
           when Grape::Http::Headers::DELETE
-            204
+            if @body.present?
+              200
+            else
+              204
+            end
           else
             200
           end
@@ -181,6 +185,20 @@ def body(value = nil)
         end
       end
 
+      # Allows you to explicitly return no content.
+      #
+      # @example
+      #   delete :id do
+      #     return_no_content
+      #     "not returned"
+      #   end
+      #
+      #   DELETE /12 # => 204 No Content, ""
+      def return_no_content
+        status 204
+        body false
+      end
+
       # Allows you to define the response as a file-like object.
       #
       # @example
diff --git a/spec/grape/dsl/inside_route_spec.rb b/spec/grape/dsl/inside_route_spec.rb
@@ -111,6 +111,13 @@ def initialize
       expect(subject.status).to eq 204
     end
 
+    it 'defaults to 200 on DELETE with a body present' do
+      request = Grape::Request.new(Rack::MockRequest.env_for('/', method: 'DELETE'))
+      subject.body 'content here'
+      expect(subject).to receive(:request).and_return(request)
+      expect(subject.status).to eq 200
+    end
+
     it 'returns status set' do
       subject.status 501
       expect(subject.status).to eq 501
@@ -136,6 +143,14 @@ def initialize
     end
   end
 
+  describe '#return_no_content' do
+    it 'sets the status code and body' do
+      subject.return_no_content
+      expect(subject.status).to eq 204
+      expect(subject.body).to eq ''
+    end
+  end
+
   describe '#content_type' do
     describe 'set' do
       before do
