From 608bd18dcf46c6abd417f39ad01d1daef0f83369 Mon Sep 17 00:00:00 2001 From: Stuart Chinery Date: Mon, 16 Oct 2023 09:56:59 +0100 Subject: [PATCH] Set response headers based on Rack version --- .github/workflows/test.yml | 10 +++++ .rubocop_todo.yml | 2 + CHANGELOG.md | 1 + lib/grape/http/headers.rb | 10 ++++- spec/grape/api_spec.rb | 42 +++++++++---------- spec/grape/endpoint_spec.rb | 2 +- .../exceptions/invalid_accept_header_spec.rb | 4 +- .../versioner/accept_version_header_spec.rb | 6 +-- .../grape/middleware/versioner/header_spec.rb | 12 +++--- spec/integration/rack/v2/headers_spec.rb | 6 +++ spec/integration/rack/v3/headers_spec.rb | 6 +++ spec/support/headers_helpers.rb | 15 +++++++ 12 files changed, 81 insertions(+), 35 deletions(-) create mode 100644 spec/integration/rack/v2/headers_spec.rb create mode 100644 spec/integration/rack/v3/headers_spec.rb create mode 100644 spec/support/headers_helpers.rb diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 40bd96b671..150f588407 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -63,6 +63,16 @@ jobs: if: ${{ matrix.gemfile == 'multi_xml' }} run: bundle exec rspec spec/integration/multi_xml + - name: Run tests (spec/integration/rack/v2) + # rack_2_0.gemfile is equals to Gemfile + if: ${{ matrix.gemfile == 'rack_2_0' }} + run: bundle exec rspec spec/integration/rack/v2 + + - name: Run tests (spec/integration/rack/v3) + # rack_2_0.gemfile is equals to Gemfile + if: ${{ matrix.gemfile == 'rack_3_0' }} + run: bundle exec rspec spec/integration/rack/v3 + - name: Coveralls uses: coverallsapp/github-action@master with: diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 51d3219195..fc6b3c553c 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -280,6 +280,8 @@ RSpec/FilePath: - 'spec/integration/eager_load/eager_load_spec.rb' - 'spec/integration/multi_json/json_spec.rb' - 'spec/integration/multi_xml/xml_spec.rb' + - 'spec/integration/rack/v2/headers_spec.rb' + - 'spec/integration/rack/v3/headers_spec.rb' # Offense count: 12 # Configuration parameters: Max. diff --git a/CHANGELOG.md b/CHANGELOG.md index f8dc826bc6..eacba39b7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ #### Features * [#2353](https://github.com/ruby-grape/grape/pull/2353): Added Rails 7.1 support - [@ericproulx](https://github.com/ericproulx). +* [#2355](https://github.com/ruby-grape/grape/pull/2355): Set response headers based on Rack version - [@schinery](https://github.com/schinery). * Your contribution here. #### Fixes diff --git a/lib/grape/http/headers.rb b/lib/grape/http/headers.rb index 564d97ff8e..96a5e4aaf3 100644 --- a/lib/grape/http/headers.rb +++ b/lib/grape/http/headers.rb @@ -10,7 +10,14 @@ module Headers PATH_INFO = 'PATH_INFO' REQUEST_METHOD = 'REQUEST_METHOD' QUERY_STRING = 'QUERY_STRING' - CONTENT_TYPE = 'Content-Type' + + if Gem::Version.new(Rack::RELEASE) < Gem::Version.new('3') + CONTENT_TYPE = 'Content-Type' + X_CASCADE = 'X-Cascade' + else + CONTENT_TYPE = 'content-type' + X_CASCADE = 'x-cascade' + end GET = 'GET' POST = 'POST' @@ -24,7 +31,6 @@ module Headers SUPPORTED_METHODS_WITHOUT_OPTIONS = Grape::Util::LazyObject.new { [GET, POST, PUT, PATCH, DELETE, HEAD].freeze } HTTP_ACCEPT_VERSION = 'HTTP_ACCEPT_VERSION' - X_CASCADE = 'X-Cascade' HTTP_TRANSFER_ENCODING = 'HTTP_TRANSFER_ENCODING' HTTP_ACCEPT = 'HTTP_ACCEPT' diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index c02e0b6e88..36546b86bd 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -689,7 +689,7 @@ class DummyFormatClass 'example' end put '/example' - expect(last_response.headers['Content-Type']).to eql 'text/plain' + expect(last_response.headers[content_type_header]).to eql 'text/plain' end describe 'adds an OPTIONS route that' do @@ -1196,7 +1196,7 @@ class DummyFormatClass it 'sets content type for txt format' do get '/foo' - expect(last_response.headers['Content-Type']).to eq('text/plain') + expect(last_response.headers[content_type_header]).to eq('text/plain') end it 'does not set Cache-Control' do @@ -1206,22 +1206,22 @@ class DummyFormatClass it 'sets content type for xml' do get '/foo.xml' - expect(last_response.headers['Content-Type']).to eq('application/xml') + expect(last_response.headers[content_type_header]).to eq('application/xml') end it 'sets content type for json' do get '/foo.json' - expect(last_response.headers['Content-Type']).to eq('application/json') + expect(last_response.headers[content_type_header]).to eq('application/json') end it 'sets content type for serializable hash format' do get '/foo.serializable_hash' - expect(last_response.headers['Content-Type']).to eq('application/json') + expect(last_response.headers[content_type_header]).to eq('application/json') end it 'sets content type for binary format' do get '/foo.binary' - expect(last_response.headers['Content-Type']).to eq('application/octet-stream') + expect(last_response.headers[content_type_header]).to eq('application/octet-stream') end it 'returns raw data when content type binary' do @@ -1230,7 +1230,7 @@ class DummyFormatClass subject.format :binary subject.get('/binary_file') { File.binread(image_filename) } get '/binary_file' - expect(last_response.headers['Content-Type']).to eq('application/octet-stream') + expect(last_response.headers[content_type_header]).to eq('application/octet-stream') expect(last_response.body).to eq(file) end @@ -1243,7 +1243,7 @@ class DummyFormatClass subject.get('/file') { file test_file } get '/file' expect(last_response.headers['Content-Length']).to eq('25') - expect(last_response.headers['Content-Type']).to eq('text/plain') + expect(last_response.headers[content_type_header]).to eq('text/plain') expect(last_response.body).to eq(file_content) end @@ -1257,7 +1257,7 @@ class DummyFormatClass subject.get('/stream') { stream test_stream } get '/stream', {}, 'HTTP_VERSION' => 'HTTP/1.1', 'SERVER_PROTOCOL' => 'HTTP/1.1' - expect(last_response.headers['Content-Type']).to eq('text/plain') + expect(last_response.headers[content_type_header]).to eq('text/plain') expect(last_response.headers['Content-Length']).to be_nil expect(last_response.headers['Cache-Control']).to eq('no-cache') expect(last_response.headers['Transfer-Encoding']).to eq('chunked') @@ -1268,7 +1268,7 @@ class DummyFormatClass it 'sets content type for error' do subject.get('/error') { error!('error in plain text', 500) } get '/error' - expect(last_response.headers['Content-Type']).to eql 'text/plain' + expect(last_response.headers[content_type_header]).to eql 'text/plain' end it 'sets content type for json error' do @@ -1276,7 +1276,7 @@ class DummyFormatClass subject.get('/error') { error!('error in json', 500) } get '/error.json' expect(last_response.status).to be 500 - expect(last_response.headers['Content-Type']).to eql 'application/json' + expect(last_response.headers[content_type_header]).to eql 'application/json' end it 'sets content type for xml error' do @@ -1284,7 +1284,7 @@ class DummyFormatClass subject.get('/error') { error!('error in xml', 500) } get '/error' expect(last_response.status).to be 500 - expect(last_response.headers['Content-Type']).to eql 'application/xml' + expect(last_response.headers[content_type_header]).to eql 'application/xml' end it 'includes extension in format' do @@ -1314,12 +1314,12 @@ class DummyFormatClass it 'sets content type' do get '/custom.custom' - expect(last_response.headers['Content-Type']).to eql 'application/custom' + expect(last_response.headers[content_type_header]).to eql 'application/custom' end it 'sets content type for error' do get '/error.custom' - expect(last_response.headers['Content-Type']).to eql 'application/custom' + expect(last_response.headers[content_type_header]).to eql 'application/custom' end end @@ -1339,7 +1339,7 @@ class DummyFormatClass image_filename = 'grape.png' post url, file: Rack::Test::UploadedFile.new(image_filename, 'image/png', true) expect(last_response.status).to eq(201) - expect(last_response.headers['Content-Type']).to eq('image/png') + expect(last_response.headers[content_type_header]).to eq('image/png') expect(last_response.headers['Content-Disposition']).to eq("attachment; filename*=UTF-8''grape.png") File.open(image_filename, 'rb') do |io| expect(last_response.body).to eq io.read @@ -1351,7 +1351,7 @@ class DummyFormatClass filename = __FILE__ post '/attachment.rb', file: Rack::Test::UploadedFile.new(filename, 'application/x-ruby', true) expect(last_response.status).to eq(201) - expect(last_response.headers['Content-Type']).to eq('application/x-ruby') + expect(last_response.headers[content_type_header]).to eq('application/x-ruby') expect(last_response.headers['Content-Disposition']).to eq("attachment; filename*=UTF-8''api_spec.rb") File.open(filename, 'rb') do |io| expect(last_response.body).to eq io.read @@ -3311,7 +3311,7 @@ def static it 'is able to cascade' do subject.mount lambda { |env| headers = {} - headers['X-Cascade'] == 'pass' if env['PATH_INFO'].exclude?('boo') + headers[x_cascade_header] == 'pass' if env['PATH_INFO'].exclude?('boo') [200, headers, ['Farfegnugen']] } => '/' @@ -4081,14 +4081,14 @@ def before subject.version 'v1', using: :path, cascade: true get '/v1/hello' expect(last_response.status).to eq(404) - expect(last_response.headers['X-Cascade']).to eq('pass') + expect(last_response.headers[x_cascade_header]).to eq('pass') end it 'does not cascade' do subject.version 'v2', using: :path, cascade: false get '/v2/hello' expect(last_response.status).to eq(404) - expect(last_response.headers.keys).not_to include 'X-Cascade' + expect(last_response.headers.keys).not_to include x_cascade_header end end @@ -4097,14 +4097,14 @@ def before subject.cascade true get '/hello' expect(last_response.status).to eq(404) - expect(last_response.headers['X-Cascade']).to eq('pass') + expect(last_response.headers[x_cascade_header]).to eq('pass') end it 'does not cascade' do subject.cascade false get '/hello' expect(last_response.status).to eq(404) - expect(last_response.headers.keys).not_to include 'X-Cascade' + expect(last_response.headers.keys).not_to include x_cascade_header end end end diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 6eb37227f3..fad8551706 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -497,7 +497,7 @@ def app end it 'responses with given content type in headers' do - expect(last_response.headers['Content-Type']).to eq 'application/json; charset=utf-8' + expect(last_response.headers[content_type_header]).to eq 'application/json; charset=utf-8' end end diff --git a/spec/grape/exceptions/invalid_accept_header_spec.rb b/spec/grape/exceptions/invalid_accept_header_spec.rb index 5017807cbf..12330b42bc 100644 --- a/spec/grape/exceptions/invalid_accept_header_spec.rb +++ b/spec/grape/exceptions/invalid_accept_header_spec.rb @@ -19,7 +19,7 @@ shared_examples_for 'a not-cascaded request' do it 'does not include the X-Cascade=pass header' do - expect(last_response.headers['X-Cascade']).to be_nil + expect(last_response.headers[x_cascade_header]).to be_nil end it 'does not accept the request' do @@ -29,7 +29,7 @@ shared_examples_for 'a rescued request' do it 'does not include the X-Cascade=pass header' do - expect(last_response.headers['X-Cascade']).to be_nil + expect(last_response.headers[x_cascade_header]).to be_nil end it 'does show rescue handler processing' do diff --git a/spec/grape/middleware/versioner/accept_version_header_spec.rb b/spec/grape/middleware/versioner/accept_version_header_spec.rb index a67c26f249..e40e8698db 100644 --- a/spec/grape/middleware/versioner/accept_version_header_spec.rb +++ b/spec/grape/middleware/versioner/accept_version_header_spec.rb @@ -36,7 +36,7 @@ end.to throw_symbol( :error, status: 406, - headers: { 'X-Cascade' => 'pass' }, + headers: { x_cascade_header => 'pass' }, message: 'The requested version is not supported.' ) end @@ -65,7 +65,7 @@ end.to throw_symbol( :error, status: 406, - headers: { 'X-Cascade' => 'pass' }, + headers: { x_cascade_header => 'pass' }, message: 'Accept-Version header must be set.' ) end @@ -76,7 +76,7 @@ end.to throw_symbol( :error, status: 406, - headers: { 'X-Cascade' => 'pass' }, + headers: { x_cascade_header => 'pass' }, message: 'Accept-Version header must be set.' ) end diff --git a/spec/grape/middleware/versioner/header_spec.rb b/spec/grape/middleware/versioner/header_spec.rb index 29fa056ca4..ace5e6b82e 100644 --- a/spec/grape/middleware/versioner/header_spec.rb +++ b/spec/grape/middleware/versioner/header_spec.rb @@ -88,7 +88,7 @@ expect { subject.call('HTTP_ACCEPT' => 'application/vnd.othervendor+json').last } .to raise_exception do |exception| expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader) - expect(exception.headers).to eql('X-Cascade' => 'pass') + expect(exception.headers).to eql(x_cascade_header => 'pass') expect(exception.status).to be 406 expect(exception.message).to include 'API vendor not found' end @@ -115,7 +115,7 @@ expect { subject.call('HTTP_ACCEPT' => 'application/vnd.othervendor-v1+json').last } .to raise_exception do |exception| expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader) - expect(exception.headers).to eql('X-Cascade' => 'pass') + expect(exception.headers).to eql(x_cascade_header => 'pass') expect(exception.status).to be 406 expect(exception.message).to include('API vendor not found') end @@ -143,7 +143,7 @@ it 'fails with 406 Not Acceptable if version is invalid' do expect { subject.call('HTTP_ACCEPT' => 'application/vnd.vendor-v2+json').last }.to raise_exception do |exception| expect(exception).to be_a(Grape::Exceptions::InvalidVersionHeader) - expect(exception.headers).to eql('X-Cascade' => 'pass') + expect(exception.headers).to eql(x_cascade_header => 'pass') expect(exception.status).to be 406 expect(exception.message).to include('API version not found') end @@ -176,7 +176,7 @@ it 'fails with 406 Not Acceptable if header is not set' do expect { subject.call({}).last }.to raise_exception do |exception| expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader) - expect(exception.headers).to eql('X-Cascade' => 'pass') + expect(exception.headers).to eql(x_cascade_header => 'pass') expect(exception.status).to be 406 expect(exception.message).to include('Accept header must be set.') end @@ -185,7 +185,7 @@ it 'fails with 406 Not Acceptable if header is empty' do expect { subject.call('HTTP_ACCEPT' => '').last }.to raise_exception do |exception| expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader) - expect(exception.headers).to eql('X-Cascade' => 'pass') + expect(exception.headers).to eql(x_cascade_header => 'pass') expect(exception.status).to be 406 expect(exception.message).to include('Accept header must be set.') end @@ -262,7 +262,7 @@ it 'fails with another version' do expect { subject.call('HTTP_ACCEPT' => 'application/vnd.vendor-v3+json') }.to raise_exception do |exception| expect(exception).to be_a(Grape::Exceptions::InvalidVersionHeader) - expect(exception.headers).to eql('X-Cascade' => 'pass') + expect(exception.headers).to eql(x_cascade_header => 'pass') expect(exception.status).to be 406 expect(exception.message).to include('API version not found') end diff --git a/spec/integration/rack/v2/headers_spec.rb b/spec/integration/rack/v2/headers_spec.rb new file mode 100644 index 0000000000..abf3e3bba1 --- /dev/null +++ b/spec/integration/rack/v2/headers_spec.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +describe Grape::Http::Headers do + it { expect(described_class::CONTENT_TYPE).to eq('Content-Type') } + it { expect(described_class::X_CASCADE).to eq('X-Cascade') } +end diff --git a/spec/integration/rack/v3/headers_spec.rb b/spec/integration/rack/v3/headers_spec.rb new file mode 100644 index 0000000000..6d0de82c49 --- /dev/null +++ b/spec/integration/rack/v3/headers_spec.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +describe Grape::Http::Headers do + it { expect(described_class::CONTENT_TYPE).to eq('content-type') } + it { expect(described_class::X_CASCADE).to eq('x-cascade') } +end diff --git a/spec/support/headers_helpers.rb b/spec/support/headers_helpers.rb new file mode 100644 index 0000000000..7386fde8c3 --- /dev/null +++ b/spec/support/headers_helpers.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Spec + module Support + module Helpers + def content_type_header + Grape::Http::Headers::CONTENT_TYPE + end + + def x_cascade_header + Grape::Http::Headers::X_CASCADE + end + end + end +end