From 07a57b6b3a2b724b5cbd43d5e724ab97280d8460 Mon Sep 17 00:00:00 2001 From: Per Lundberg Date: Tue, 27 Mar 2018 20:55:00 +0300 Subject: [PATCH] Always set CONTENT_TYPE for non-GET requests (#223) ...even when no parameters are provided (or the provided parameters are `nil`) The long story: - https://github.com/rack-test/rack-test/pull/132 changed the behavior for HTTP DELETE requests to behave like GET: put the parameters in the query string, and don't set the Content-Type. This change was merged in https://github.com/rack-test/rack-test/commit/d016695d84dd2e9b09eb77e5a89ffcbc1892921a - This broke `rack-test` for certain people, which was highlighted in https://github.com/rack-test/rack-test/issues/200. Arguably, the change incorporated in https://github.com/rack-test/rack-test/commit/d016695d84dd2e9b09eb77e5a89ffcbc1892921a was too brutal. - https://github.com/rack-test/rack-test/pull/212 tried to sort it out, by not setting Content-Type if params are nil, and also reverting the change to put DELETE parameters in the query string. The first part of this (params being nil) caused issues for certain people. So this PR now tries to once and for all sort this out by: - Always setting `env['CONTENT_TYPE']`, even when params are `nil`. - Adding more tests for how `CONTENT_TYPE` and `CONTENT_LENGTH` should behave; some of this does not come from us, but from Rack upstream. - Settles with the discussion in https://github.com/rack-test/rack-test/pull/220: if you are using `DELETE` and must use query params, put them manually in there like this: `delete '/foo?bar=baz'`. Arguably not very clean, but better than changing back and forth. `params` are overloaded in rack 0.x and will be so in 1.0 also. I am thinking that we should go with the excellent suggestion provided by @malacalypse in #200 to use dedicated `body` and `params` parameters in the long run (and probably use keyword parameters instead, but that's definitely a 2.x feature since it breaks all existing usage.) For now, I think we'll live with the ugliness. I encourage everyone to give this a try before we merge, to ensure we don't have to revert once more. --- lib/rack/test.rb | 4 +- spec/rack/test_spec.rb | 111 +++++++++++++++++++++++++++-------------- 2 files changed, 76 insertions(+), 39 deletions(-) diff --git a/lib/rack/test.rb b/lib/rack/test.rb index 446c114c..c4d16485 100644 --- a/lib/rack/test.rb +++ b/lib/rack/test.rb @@ -233,7 +233,7 @@ def env_for(uri, env) uri.query = [uri.query, build_nested_query(params)].compact.reject { |v| v == '' }.join('&') end elsif !env.key?(:input) - env['CONTENT_TYPE'] ||= 'application/x-www-form-urlencoded' unless params.nil? + env['CONTENT_TYPE'] ||= 'application/x-www-form-urlencoded' if params.is_a?(Hash) if data = build_multipart(params) @@ -241,6 +241,8 @@ def env_for(uri, env) env['CONTENT_LENGTH'] ||= data.length.to_s env['CONTENT_TYPE'] = "multipart/form-data; boundary=#{MULTIPART_BOUNDARY}" else + # NB: We do not need to set CONTENT_LENGTH here; + # Rack::ContentLength will determine it automatically. env[:input] = params_to_string(params) end else diff --git a/spec/rack/test_spec.rb b/spec/rack/test_spec.rb index 01b759bf..72706c6c 100644 --- a/spec/rack/test_spec.rb +++ b/spec/rack/test_spec.rb @@ -457,11 +457,11 @@ def close end end - shared_examples_for 'any #verb methods' do + shared_examples_for 'any #verb methods' do |verb| it 'requests the URL using VERB' do public_send(verb, '/') - check expect(last_request.env['REQUEST_METHOD']).to eq(verb.upcase) + check expect(last_request.env['REQUEST_METHOD']).to eq(verb.to_s.upcase) expect(last_response).to be_ok end @@ -470,9 +470,28 @@ def close expect(last_request.env['HTTP_USER_AGENT']).to eq('Rack::Test') end - it 'does not set CONTENT_TYPE if params are explicitly set to nil' do - public_send(verb, '/', nil) - expect(last_request.env['CONTENT_TYPE']).to be_nil + context 'when params are not provided', unless: verb == :get do + it 'sets CONTENT_TYPE to application/x-www-form-urlencoded' do + public_send(verb, '/') + expect(last_request.env['CONTENT_TYPE']).to eq 'application/x-www-form-urlencoded' + end + + it 'sets CONTENT_LENGTH to zero' do + public_send(verb, '/') + expect(last_request.env['CONTENT_LENGTH']).to eq '0' + end + end + + context 'when params are explicitly set to nil', unless: verb == :get do + it 'sets CONTENT_TYPE to application/x-www-form-urlencoded' do + public_send(verb, '/', nil) + expect(last_request.env['CONTENT_TYPE']).to eq 'application/x-www-form-urlencoded' + end + + it 'sets CONTENT_LENGTH to 0' do + public_send(verb, '/') + expect(last_request.env['CONTENT_LENGTH']).to eq '0' + end end it 'yields the response to a given block' do @@ -506,10 +525,43 @@ def close end describe '#get' do - it_should_behave_like 'any #verb methods' + it_should_behave_like 'any #verb methods', :get + + context 'when params are not provided' do + # This is not actually explicitly stated in the relevant RFCs; + # https://tools.ietf.org/html/rfc7231#section-3.1.1.5 + # ...but e.g. curl do not set it for GET requests. + it 'does not set CONTENT_TYPE' do + get '/' + expect(last_request.env.key?('CONTENT_TYPE')).to eq false + end + + # Quoting from https://tools.ietf.org/html/rfc7230#section-3.3.2: + # + # A user agent SHOULD NOT send a Content-Length header field when + # the request message does not contain a payload body and the + # method semantics do not anticipate such a body. + # + # _However_, something causes CONTENT_LENGTH to always be present. + # Even when we don't set it ourselves. It could be + # Rack::ContentLength that is playing tricks with us: + # https://github.com/rack/rack/blob/master/lib/rack/content_length.rb + it 'sets CONTENT_LENGTH to zero' do + get '/' + expect(last_request.env['CONTENT_LENGTH']).to eq '0' + end + end - def verb - 'get' + context 'when params are explicitly set to nil' do + it 'sets CONTENT_TYPE to application/x-www-form-urlencoded' do + get '/', nil + expect(last_request.env.key?('CONTENT_TYPE')).to eq false + end + + it 'sets CONTENT_LENGTH to zero' do + get '/', nil + expect(last_request.env['CONTENT_LENGTH']).to eq '0' + end end it 'uses the provided params hash' do @@ -539,19 +591,11 @@ def verb end describe '#head' do - it_should_behave_like 'any #verb methods' - - def verb - 'head' - end + it_should_behave_like 'any #verb methods', :head end describe '#post' do - it_should_behave_like 'any #verb methods' - - def verb - 'post' - end + it_should_behave_like 'any #verb methods', :post it 'uses the provided params hash' do post '/', foo: 'bar' @@ -568,6 +612,13 @@ def verb expect(last_request.env['CONTENT_TYPE']).to eq('application/x-www-form-urlencoded') end + # NB: This is never set in _our code_, but is added automatically + # (presumably by Rack::ContentLength) + it 'sets the CONTENT_LENGTH' do + post '/', foo: 'bar' + expect(last_request.env['CONTENT_LENGTH']).to eq('7') + end + it 'accepts a body' do post '/', 'Lobsterlicious!' expect(last_request.body.read).to eq('Lobsterlicious!') @@ -582,11 +633,7 @@ def verb end describe '#put' do - it_should_behave_like 'any #verb methods' - - def verb - 'put' - end + it_should_behave_like 'any #verb methods', :put it 'accepts a body' do put '/', 'Lobsterlicious!' @@ -595,11 +642,7 @@ def verb end describe '#patch' do - it_should_behave_like 'any #verb methods' - - def verb - 'patch' - end + it_should_behave_like 'any #verb methods', :patch it 'accepts a body' do patch '/', 'Lobsterlicious!' @@ -608,11 +651,7 @@ def verb end describe '#delete' do - it_should_behave_like 'any #verb methods' - - def verb - 'delete' - end + it_should_behave_like 'any #verb methods', :delete it 'uses the provided params hash' do delete '/', foo: 'bar' @@ -636,11 +675,7 @@ def verb end describe '#options' do - it_should_behave_like 'any #verb methods' - - def verb - 'options' - end + it_should_behave_like 'any #verb methods', :options end describe '#custom_request' do