Skip to content

Commit

Permalink
sets 204 as default status for delete
Browse files Browse the repository at this point in the history
  • Loading branch information
LeFnord committed Dec 4, 2016
1 parent 152fa40 commit 44d32da
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 19 deletions.
13 changes: 13 additions & 0 deletions UPGRADING.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,19 @@ Prior to this version the response would be `one is missing`.

See [#1510](https://github.com/ruby-grape/grape/pull/1510) for more information.

#### Change default status code of DELETE to 204

**Breaking change**: Sets the default response status code for a delete request to 204.
Doing it makes the response more explicit, cause a delete request returns in most cases an empty body – the resource was deleted/voided. By setting it to 204, other as 200, makes the handling on consumer side easier.

To achieve the old behaviour, one has to set it explicitly:
```ruby
delete :id do
status 200
'foo successfully deleted'
end
```

### Upgrading to >= 0.17.0

#### Removed official support for Ruby < 2.2.2
Expand Down
2 changes: 2 additions & 0 deletions lib/grape/dsl/inside_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ def status(status = nil)
case request.request_method.to_s.upcase
when Grape::Http::Headers::POST
201
when Grape::Http::Headers::DELETE
204
else
200
end
Expand Down
8 changes: 7 additions & 1 deletion spec/grape/dsl/inside_route_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def initialize
end

describe '#status' do
%w(GET PUT DELETE OPTIONS).each do |method|
%w(GET PUT OPTIONS).each do |method|
it 'defaults to 200 on GET' do
request = Grape::Request.new(Rack::MockRequest.env_for('/', method: method))
expect(subject).to receive(:request).and_return(request)
Expand All @@ -105,6 +105,12 @@ def initialize
expect(subject.status).to eq 201
end

it 'defaults to 204 on DELETE' do
request = Grape::Request.new(Rack::MockRequest.env_for('/', method: 'DELETE'))
expect(subject).to receive(:request).and_return(request)
expect(subject.status).to eq 204
end

it 'returns status set' do
subject.status 501
expect(subject.status).to eq 501
Expand Down
69 changes: 51 additions & 18 deletions spec/grape/endpoint_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1095,32 +1095,65 @@ def memoized
end

context 'anchoring' do
verbs = %w(post get head delete put options patch)
describe 'delete 204' do
it 'allows for the anchoring option with a delete method' do
subject.send(:delete, '/example', anchor: true) {}
send(:delete, '/example/and/some/more')
expect(last_response.status).to eql 404
end

verbs.each do |verb|
it "allows for the anchoring option with a #{verb.upcase} method" do
subject.send(verb, '/example', anchor: true) do
verb
end
send(verb, '/example/and/some/more')
it 'anchors paths by default for the delete method' do
subject.send(:delete, '/example') {}
send(:delete, '/example/and/some/more')
expect(last_response.status).to eql 404
end

it "anchors paths by default for the #{verb.upcase} method" do
subject.send(verb, '/example') do
verb
it 'responds to /example/and/some/more for the non-anchored delete method' do
subject.send(:delete, '/example', anchor: false) {}
send(:delete, '/example/and/some/more')
expect(last_response.status).to eql 204
expect(last_response.body).to be_empty
end
end

describe 'delete 200, with response body' do
it 'responds to /example/and/some/more for the non-anchored delete method' do
subject.send(:delete, '/example', anchor: false) do
status 200
body 'deleted'
end
send(verb, '/example/and/some/more')
expect(last_response.status).to eql 404
send(:delete, '/example/and/some/more')
expect(last_response.status).to eql 200
expect(last_response.body).not_to be_empty
end
end

it "responds to /example/and/some/more for the non-anchored #{verb.upcase} method" do
subject.send(verb, '/example', anchor: false) do
verb
describe 'all other' do
%w(post get head put options patch).each do |verb|
it "allows for the anchoring option with a #{verb.upcase} method" do
subject.send(verb, '/example', anchor: true) do
verb
end
send(verb, '/example/and/some/more')
expect(last_response.status).to eql 404
end

it "anchors paths by default for the #{verb.upcase} method" do
subject.send(verb, '/example') do
verb
end
send(verb, '/example/and/some/more')
expect(last_response.status).to eql 404
end

it "responds to /example/and/some/more for the non-anchored #{verb.upcase} method" do
subject.send(verb, '/example', anchor: false) do
verb
end
send(verb, '/example/and/some/more')
expect(last_response.status).to eql verb == 'post' ? 201 : 200
expect(last_response.body).to eql verb == 'head' ? '' : verb
end
send(verb, '/example/and/some/more')
expect(last_response.status).to eql verb == 'post' ? 201 : 200
expect(last_response.body).to eql verb == 'head' ? '' : verb
end
end
end
Expand Down

0 comments on commit 44d32da

Please sign in to comment.