From bc2fcdb5eeb70ec273b56c273e0ee746a18ee67f Mon Sep 17 00:00:00 2001 From: Bas Janssen Date: Wed, 25 Jul 2018 00:36:57 +0200 Subject: [PATCH 1/4] Progress --- lib/grape/dsl/inside_route.rb | 25 +++++++++------- spec/grape/api_spec.rb | 4 +-- spec/grape/dsl/inside_route_spec.rb | 45 ++++++++++++++++++++++++----- spec/grape/endpoint_spec.rb | 10 +++---- 4 files changed, 60 insertions(+), 24 deletions(-) diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index b4672bb97d..c3ebfeb09c 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -178,10 +178,10 @@ def status(status = nil) when Grape::Http::Headers::POST 201 when Grape::Http::Headers::DELETE - if @body.present? - 200 - else + if @body.nil? 204 + else + 200 end else 200 @@ -222,12 +222,17 @@ def cookies # end # # GET /body # => "Body" - def body(value = nil) - if value - @body = value - elsif value == false - @body = '' - status 204 + def body(*value) + raise ArgumentError 'you can only set a body with one argument' if value.length > 1 + + if value.length == 1 + value = value.first + if value.nil? + @body = '' + status 204 + else + @body = value + end else @body end @@ -244,7 +249,7 @@ def body(value = nil) # DELETE /12 # => 204 No Content, "" def return_no_content status 204 - body false + body nil end # Allows you to define the response as a file-like object. diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index c38205f474..a1a134c673 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -3553,10 +3553,10 @@ def before end context 'body' do - context 'false' do + context 'nil' do before do subject.get '/blank' do - body false + body nil end end it 'returns blank body' do diff --git a/spec/grape/dsl/inside_route_spec.rb b/spec/grape/dsl/inside_route_spec.rb index 928510f75e..349e23b0bc 100644 --- a/spec/grape/dsl/inside_route_spec.rb +++ b/spec/grape/dsl/inside_route_spec.rb @@ -111,11 +111,42 @@ 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 + describe 'defaulting to 200 on DELETE' do + 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 'regards an empty string as content' do + request = Grape::Request.new(Rack::MockRequest.env_for('/', method: 'DELETE')) + subject.body '' + expect(subject).to receive(:request).and_return(request) + expect(subject.status).to eq 200 + end + + it 'regards an empty hash as content' do + request = Grape::Request.new(Rack::MockRequest.env_for('/', method: 'DELETE')) + subject.body({}) # Use brackets, because it's a block otherwise + expect(subject).to receive(:request).and_return(request) + expect(subject.status).to eq 200 + end + + it 'regards an empty array as content' do + request = Grape::Request.new(Rack::MockRequest.env_for('/', method: 'DELETE')) + subject.body [] + expect(subject).to receive(:request).and_return(request) + expect(subject.status).to eq 200 + end + + # Still failing for some weird kind of reason. + it 'regards a nil as no content' do + request = Grape::Request.new(Rack::MockRequest.env_for('/', method: 'DELETE')) + subject.body nil + expect(subject).to receive(:request).and_return(request) + expect(subject.status).to eq 204 + end end it 'returns status set' do @@ -184,9 +215,9 @@ def initialize end end - describe 'false' do + describe 'nil' do before do - subject.body false + subject.body nil end it 'sets status to 204' do diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 5614eea76e..c7e3b3f85d 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -1320,18 +1320,18 @@ def memoized end end - describe 'delete 204, with nil has return value (no explicit body)' do + describe 'delete 200, with empty string as return value' do it 'responds to /example delete method' do - subject.delete(:example) { nil } + subject.delete(:example) { '' } delete '/example' - expect(last_response.status).to eql 204 + expect(last_response.status).to eql 200 expect(last_response.body).to be_empty end end - describe 'delete 204, with empty array has return value (no explicit body)' do + describe 'delete 204, with nil as return value (no explicit body)' do it 'responds to /example delete method' do - subject.delete(:example) { '' } + subject.delete(:example) { nil } delete '/example' expect(last_response.status).to eql 204 expect(last_response.body).to be_empty From 44cd8fb8d3fd84552c9f1c9748b683ce2a5da7e2 Mon Sep 17 00:00:00 2001 From: Bas Janssen Date: Wed, 25 Jul 2018 00:53:40 +0200 Subject: [PATCH 2/4] Change formulation of spec --- spec/grape/dsl/inside_route_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/grape/dsl/inside_route_spec.rb b/spec/grape/dsl/inside_route_spec.rb index 349e23b0bc..9a9b7c547f 100644 --- a/spec/grape/dsl/inside_route_spec.rb +++ b/spec/grape/dsl/inside_route_spec.rb @@ -111,8 +111,8 @@ def initialize expect(subject.status).to eq 204 end - describe 'defaulting to 200 on DELETE' do - it 'defaults to 200 on DELETE with a body present' do + describe 'what is regarded as content on DELETE' do + it 'regards a string as content' do request = Grape::Request.new(Rack::MockRequest.env_for('/', method: 'DELETE')) subject.body 'content here' expect(subject).to receive(:request).and_return(request) From 275ebe9868ae4c0fd37432adc6dbfdb40efc9ae7 Mon Sep 17 00:00:00 2001 From: Bas Janssen Date: Thu, 18 Oct 2018 09:30:42 +0200 Subject: [PATCH 3/4] Fix spec --- spec/grape/dsl/inside_route_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/grape/dsl/inside_route_spec.rb b/spec/grape/dsl/inside_route_spec.rb index 9a9b7c547f..90dad08ffe 100644 --- a/spec/grape/dsl/inside_route_spec.rb +++ b/spec/grape/dsl/inside_route_spec.rb @@ -140,11 +140,9 @@ def initialize expect(subject.status).to eq 200 end - # Still failing for some weird kind of reason. it 'regards a nil as no content' do - request = Grape::Request.new(Rack::MockRequest.env_for('/', method: 'DELETE')) + Grape::Request.new(Rack::MockRequest.env_for('/', method: 'DELETE')) subject.body nil - expect(subject).to receive(:request).and_return(request) expect(subject.status).to eq 204 end end From 050089fd1882deb6a75015bb00cc05b4fc27e80e Mon Sep 17 00:00:00 2001 From: Bas Janssen Date: Thu, 18 Oct 2018 10:14:14 +0200 Subject: [PATCH 4/4] Update changelog and upgrading files --- CHANGELOG.md | 11 ++++++++--- UPGRADING.md | 4 ++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc39f22549..666fe1a6b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,11 @@ -### 1.1.0 (Next) +### ? (Next) + +#### Fixes + +* [#1774](https://github.com/ruby-grape/grape/pull/1774): Change HTTP delete status code and response body logic - [@basjanssen](https://github.com/basjanssen). +* Your contribution here. + +### 1.1.0 (8/4/2018) #### Features @@ -6,13 +13,11 @@ #### Fixes - * [#1762](https://github.com/ruby-grape/grape/pull/1763): Fix unsafe HTML rendering on errors - [@ctennis](https://github.com/ctennis). * [#1759](https://github.com/ruby-grape/grape/pull/1759): Update appraisal for rails_edge - [@zvkemp](https://github.com/zvkemp). * [#1758](https://github.com/ruby-grape/grape/pull/1758): Fix expanding load_path in gemspec - [@2maz](https://github.com/2maz). * [#1765](https://github.com/ruby-grape/grape/pull/1765): Use 415 when request body is of an unsupported media type - [@jdmurphy](https://github.com/jdmurphy). * [#1771](https://github.com/ruby-grape/grape/pull/1771): Fix param aliases with 'given' blocks - [@jereynolds](https://github.com/jereynolds). -* Your contribution here. ### 1.0.3 (4/23/2018) diff --git a/UPGRADING.md b/UPGRADING.md index d63fa23e64..0292f2bf91 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,5 +1,9 @@ Upgrading Grape =============== +### Upgrading to >= ? +#### Changes in HTTP Delete Response Body and HTTP status code +Previously, using `body false` or returning an empty Hash/Array would result in a empty response body and HTTP 204 (No content). +Currently, returning an empty Hash or Array will return the corresponding json/xml string and HTTP 200. Calling `body nil` or setting the response body to `nil` or using `return_no_content` will return an empty response body and HTTP 204. ### Upgrading to >= 1.1.0