From de76b5cbaf19723c93fa007368f65ca773d14779 Mon Sep 17 00:00:00 2001 From: Stuart Chinery <163900+schinery@users.noreply.github.com> Date: Wed, 25 Oct 2023 13:38:46 +0100 Subject: [PATCH] Additional changes for setting Rack versioned headers (#2362) * Updated UPGRADING notes about Rack 3 * Changed .rack3? to .lowercase_headers? * Use Rack constants where available * Fixed integration specs * Re-added TRANSFER_ENCODING as not in Rack 1 --- UPGRADING.md | 2 +- lib/grape.rb | 4 +-- lib/grape/dsl/inside_route.rb | 8 ++--- lib/grape/http/headers.rb | 8 +---- lib/grape/middleware/error.rb | 8 ++--- lib/grape/middleware/formatter.rb | 6 ++-- lib/grape/request.rb | 2 +- spec/grape/api/custom_validations_spec.rb | 4 +-- spec/grape/api_spec.rb | 40 +++++++++++------------ spec/grape/dsl/inside_route_spec.rb | 28 ++++++++-------- spec/grape/endpoint_spec.rb | 6 ++-- spec/grape/request_spec.rb | 4 +-- spec/integration/rack/v2/headers_spec.rb | 3 -- spec/integration/rack/v3/headers_spec.rb | 3 -- 14 files changed, 57 insertions(+), 69 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index c2f856c8f9..b799dc4c05 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -5,7 +5,7 @@ Upgrading Grape #### Headers -As per [rack/rack#1592](https://github.com/rack/rack/issues/1592) Rack 3.0 is enforcing the HTTP/2 semantics, and thus treats all headers as lowercase. Starting with Grape 1.9.0, headers will be cased based on what version of Rack you are using. +As per [rack/rack#1592](https://github.com/rack/rack/issues/1592) Rack 3 is following the HTTP/2+ semantics which require header names to be lower case. To avoid compatibility issues, starting with Grape 1.9.0, headers will be cased based on what version of Rack you are using. Given this request: diff --git a/lib/grape.rb b/lib/grape.rb index f4ec5701fa..cb36ebd1df 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -42,8 +42,8 @@ def self.deprecator @deprecator ||= ActiveSupport::Deprecation.new('2.0', 'Grape') end - def self.rack3? - Gem::Version.new(::Rack.release) >= Gem::Version.new('3') + def self.lowercase_headers? + Rack::CONTENT_TYPE == 'content-type' end eager_autoload do diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index e2bc9169ad..bcfd3e84c8 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -224,9 +224,9 @@ def status(status = nil) # Set response content-type def content_type(val = nil) if val - header(Grape::Http::Headers::CONTENT_TYPE, val) + header(Rack::CONTENT_TYPE, val) else - header[Grape::Http::Headers::CONTENT_TYPE] + header[Rack::CONTENT_TYPE] end end @@ -328,9 +328,9 @@ def sendfile(value = nil) def stream(value = nil) return if value.nil? && @stream.nil? - header Grape::Http::Headers::CONTENT_LENGTH, nil + header Rack::CONTENT_LENGTH, nil header Grape::Http::Headers::TRANSFER_ENCODING, nil - header Grape::Http::Headers::CACHE_CONTROL, 'no-cache' # Skips ETag generation (reading the response up front) + header Rack::CACHE_CONTROL, 'no-cache' # Skips ETag generation (reading the response up front) if value.is_a?(String) file_body = Grape::ServeStream::FileBody.new(value) @stream = Grape::ServeStream::StreamResponse.new(file_body) diff --git a/lib/grape/http/headers.rb b/lib/grape/http/headers.rb index 442f6b65cc..a7e5984f75 100644 --- a/lib/grape/http/headers.rb +++ b/lib/grape/http/headers.rb @@ -11,19 +11,13 @@ module Headers REQUEST_METHOD = 'REQUEST_METHOD' QUERY_STRING = 'QUERY_STRING' - if Grape.rack3? + if Grape.lowercase_headers? ALLOW = 'allow' - CACHE_CONTROL = 'cache-control' - CONTENT_LENGTH = 'content-length' - CONTENT_TYPE = 'content-type' LOCATION = 'location' TRANSFER_ENCODING = 'transfer-encoding' X_CASCADE = 'x-cascade' else ALLOW = 'Allow' - CACHE_CONTROL = 'Cache-Control' - CONTENT_LENGTH = 'Content-Length' - CONTENT_TYPE = 'Content-Type' LOCATION = 'Location' TRANSFER_ENCODING = 'Transfer-Encoding' X_CASCADE = 'X-Cascade' diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index 259d610eb4..05fc0312f2 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -51,7 +51,7 @@ def call!(env) end def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil) - headers = headers.reverse_merge(Grape::Http::Headers::CONTENT_TYPE => content_type) + headers = headers.reverse_merge(Rack::CONTENT_TYPE => content_type) rack_response(format_message(message, backtrace, original_exception), status, headers) end @@ -63,15 +63,15 @@ def default_rescue_handler(e) def error_response(error = {}) status = error[:status] || options[:default_status] message = error[:message] || options[:default_message] - headers = { Grape::Http::Headers::CONTENT_TYPE => content_type } + headers = { Rack::CONTENT_TYPE => content_type } headers.merge!(error[:headers]) if error[:headers].is_a?(Hash) backtrace = error[:backtrace] || error[:original_exception]&.backtrace || [] original_exception = error.is_a?(Exception) ? error : error[:original_exception] || nil rack_response(format_message(message, backtrace, original_exception), status, headers) end - def rack_response(message, status = options[:default_status], headers = { Grape::Http::Headers::CONTENT_TYPE => content_type }) - message = ERB::Util.html_escape(message) if headers[Grape::Http::Headers::CONTENT_TYPE] == TEXT_HTML + def rack_response(message, status = options[:default_status], headers = { Rack::CONTENT_TYPE => content_type }) + message = ERB::Util.html_escape(message) if headers[Rack::CONTENT_TYPE] == TEXT_HTML Rack::Response.new([message], Rack::Utils.status_code(status), headers) end diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index 06d9a4931d..0f8e7cdb3c 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -54,7 +54,7 @@ def build_formatted_response(status, headers, bodies) end def fetch_formatter(headers, options) - api_format = mime_types[headers[Grape::Http::Headers::CONTENT_TYPE]] || env[Grape::Env::API_FORMAT] + api_format = mime_types[headers[Rack::CONTENT_TYPE]] || env[Grape::Env::API_FORMAT] Grape::Formatter.formatter_for(api_format, **options) end @@ -63,10 +63,10 @@ def fetch_formatter(headers, options) # @param headers [Hash] # @return [Hash] def ensure_content_type(headers) - if headers[Grape::Http::Headers::CONTENT_TYPE] + if headers[Rack::CONTENT_TYPE] headers else - headers.merge(Grape::Http::Headers::CONTENT_TYPE => content_type_for(env[Grape::Env::API_FORMAT])) + headers.merge(Rack::CONTENT_TYPE => content_type_for(env[Grape::Env::API_FORMAT])) end end diff --git a/lib/grape/request.rb b/lib/grape/request.rb index f522ea923b..c907f0b4ab 100644 --- a/lib/grape/request.rb +++ b/lib/grape/request.rb @@ -46,7 +46,7 @@ def build_headers end end - if Grape.rack3? + if Grape.lowercase_headers? def transform_header(header) -header[5..].tr('_', '-').downcase end diff --git a/spec/grape/api/custom_validations_spec.rb b/spec/grape/api/custom_validations_spec.rb index 121e44f918..a6b7a629c1 100644 --- a/spec/grape/api/custom_validations_spec.rb +++ b/spec/grape/api/custom_validations_spec.rb @@ -169,12 +169,12 @@ def validate(request) end def access_header - Grape.rack3? ? 'x-access-token' : 'X-Access-Token' + Grape.lowercase_headers? ? 'x-access-token' : 'X-Access-Token' end end end let(:app) { Rack::Builder.new(subject) } - let(:x_access_token_header) { Grape.rack3? ? 'x-access-token' : 'X-Access-Token' } + let(:x_access_token_header) { Grape.lowercase_headers? ? 'x-access-token' : 'X-Access-Token' } before do described_class.register_validator('admin', admin_validator) diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 5e31f51cfc..161b561cbb 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[Grape::Http::Headers::CONTENT_TYPE]).to eql 'text/plain' + expect(last_response.headers[Rack::CONTENT_TYPE]).to eql 'text/plain' end describe 'adds an OPTIONS route that' do @@ -1196,32 +1196,32 @@ class DummyFormatClass it 'sets content type for txt format' do get '/foo' - expect(last_response.headers[Grape::Http::Headers::CONTENT_TYPE]).to eq('text/plain') + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq('text/plain') end it 'does not set Cache-Control' do get '/foo' - expect(last_response.headers[Grape::Http::Headers::CACHE_CONTROL]).to be_nil + expect(last_response.headers[Rack::CACHE_CONTROL]).to be_nil end it 'sets content type for xml' do get '/foo.xml' - expect(last_response.headers[Grape::Http::Headers::CONTENT_TYPE]).to eq('application/xml') + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq('application/xml') end it 'sets content type for json' do get '/foo.json' - expect(last_response.headers[Grape::Http::Headers::CONTENT_TYPE]).to eq('application/json') + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq('application/json') end it 'sets content type for serializable hash format' do get '/foo.serializable_hash' - expect(last_response.headers[Grape::Http::Headers::CONTENT_TYPE]).to eq('application/json') + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq('application/json') end it 'sets content type for binary format' do get '/foo.binary' - expect(last_response.headers[Grape::Http::Headers::CONTENT_TYPE]).to eq('application/octet-stream') + expect(last_response.headers[Rack::CONTENT_TYPE]).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[Grape::Http::Headers::CONTENT_TYPE]).to eq('application/octet-stream') + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq('application/octet-stream') expect(last_response.body).to eq(file) end @@ -1242,8 +1242,8 @@ class DummyFormatClass subject.get('/file') { file test_file } get '/file' - expect(last_response.headers[Grape::Http::Headers::CONTENT_LENGTH]).to eq('25') - expect(last_response.headers[Grape::Http::Headers::CONTENT_TYPE]).to eq('text/plain') + expect(last_response.headers[Rack::CONTENT_LENGTH]).to eq('25') + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq('text/plain') expect(last_response.body).to eq(file_content) end @@ -1257,9 +1257,9 @@ class DummyFormatClass subject.get('/stream') { stream test_stream } get '/stream', {}, 'HTTP_VERSION' => 'HTTP/1.1', 'SERVER_PROTOCOL' => 'HTTP/1.1' - expect(last_response.headers[Grape::Http::Headers::CONTENT_TYPE]).to eq('text/plain') - expect(last_response.headers[Grape::Http::Headers::CONTENT_LENGTH]).to be_nil - expect(last_response.headers[Grape::Http::Headers::CACHE_CONTROL]).to eq('no-cache') + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq('text/plain') + expect(last_response.headers[Rack::CONTENT_LENGTH]).to be_nil + expect(last_response.headers[Rack::CACHE_CONTROL]).to eq('no-cache') expect(last_response.headers[Grape::Http::Headers::TRANSFER_ENCODING]).to eq('chunked') expect(last_response.body).to eq("c\r\nThis is some\r\nd\r\n file content\r\n0\r\n\r\n") @@ -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[Grape::Http::Headers::CONTENT_TYPE]).to eql 'text/plain' + expect(last_response.headers[Rack::CONTENT_TYPE]).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[Grape::Http::Headers::CONTENT_TYPE]).to eql 'application/json' + expect(last_response.headers[Rack::CONTENT_TYPE]).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[Grape::Http::Headers::CONTENT_TYPE]).to eql 'application/xml' + expect(last_response.headers[Rack::CONTENT_TYPE]).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[Grape::Http::Headers::CONTENT_TYPE]).to eql 'application/custom' + expect(last_response.headers[Rack::CONTENT_TYPE]).to eql 'application/custom' end it 'sets content type for error' do get '/error.custom' - expect(last_response.headers[Grape::Http::Headers::CONTENT_TYPE]).to eql 'application/custom' + expect(last_response.headers[Rack::CONTENT_TYPE]).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[Grape::Http::Headers::CONTENT_TYPE]).to eq('image/png') + expect(last_response.headers[Rack::CONTENT_TYPE]).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[Grape::Http::Headers::CONTENT_TYPE]).to eq('application/x-ruby') + expect(last_response.headers[Rack::CONTENT_TYPE]).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 diff --git a/spec/grape/dsl/inside_route_spec.rb b/spec/grape/dsl/inside_route_spec.rb index 6dff2d15c8..931382e48c 100644 --- a/spec/grape/dsl/inside_route_spec.rb +++ b/spec/grape/dsl/inside_route_spec.rb @@ -263,8 +263,8 @@ def initialize end before do - subject.header Grape::Http::Headers::CACHE_CONTROL, 'cache' - subject.header Grape::Http::Headers::CONTENT_LENGTH, 123 + subject.header Rack::CACHE_CONTROL, 'cache' + subject.header Rack::CONTENT_LENGTH, 123 subject.header Grape::Http::Headers::TRANSFER_ENCODING, 'base64' end @@ -283,13 +283,13 @@ def initialize it 'does not change the Cache-Control header' do subject.sendfile file_path - expect(subject.header[Grape::Http::Headers::CACHE_CONTROL]).to eq 'cache' + expect(subject.header[Rack::CACHE_CONTROL]).to eq 'cache' end it 'does not change the Content-Length header' do subject.sendfile file_path - expect(subject.header[Grape::Http::Headers::CONTENT_LENGTH]).to eq 123 + expect(subject.header[Rack::CONTENT_LENGTH]).to eq 123 end it 'does not change the Transfer-Encoding header' do @@ -324,8 +324,8 @@ def initialize end before do - subject.header Grape::Http::Headers::CACHE_CONTROL, 'cache' - subject.header Grape::Http::Headers::CONTENT_LENGTH, 123 + subject.header Rack::CACHE_CONTROL, 'cache' + subject.header Rack::CONTENT_LENGTH, 123 subject.header Grape::Http::Headers::TRANSFER_ENCODING, 'base64' end @@ -344,19 +344,19 @@ def initialize it 'sets Cache-Control header to no-cache' do subject.stream file_path - expect(subject.header[Grape::Http::Headers::CACHE_CONTROL]).to eq 'no-cache' + expect(subject.header[Rack::CACHE_CONTROL]).to eq 'no-cache' end it 'does not change Cache-Control header' do subject.stream - expect(subject.header[Grape::Http::Headers::CACHE_CONTROL]).to eq 'cache' + expect(subject.header[Rack::CACHE_CONTROL]).to eq 'cache' end it 'sets Content-Length header to nil' do subject.stream file_path - expect(subject.header[Grape::Http::Headers::CONTENT_LENGTH]).to be_nil + expect(subject.header[Rack::CONTENT_LENGTH]).to be_nil end it 'sets Transfer-Encoding header to nil' do @@ -374,8 +374,8 @@ def initialize end before do - subject.header Grape::Http::Headers::CACHE_CONTROL, 'cache' - subject.header Grape::Http::Headers::CONTENT_LENGTH, 123 + subject.header Rack::CACHE_CONTROL, 'cache' + subject.header Rack::CONTENT_LENGTH, 123 subject.header Grape::Http::Headers::TRANSFER_ENCODING, 'base64' end @@ -394,13 +394,13 @@ def initialize it 'sets Cache-Control header to no-cache' do subject.stream stream_object - expect(subject.header[Grape::Http::Headers::CACHE_CONTROL]).to eq 'no-cache' + expect(subject.header[Rack::CACHE_CONTROL]).to eq 'no-cache' end it 'sets Content-Length header to nil' do subject.stream stream_object - expect(subject.header[Grape::Http::Headers::CONTENT_LENGTH]).to be_nil + expect(subject.header[Rack::CONTENT_LENGTH]).to be_nil end it 'sets Transfer-Encoding header to nil' do @@ -421,7 +421,7 @@ def initialize it 'returns default' do expect(subject.stream).to be_nil - expect(subject.header[Grape::Http::Headers::CACHE_CONTROL]).to be_nil + expect(subject.header[Rack::CACHE_CONTROL]).to be_nil end end diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index e81fbc806d..400b45c404 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -146,7 +146,7 @@ def app it 'includes additional request headers' do get '/headers', nil, 'HTTP_X_GRAPE_CLIENT' => '1' - x_grape_client_header = Grape.rack3? ? 'x-grape-client' : 'X-Grape-Client' + x_grape_client_header = Grape.lowercase_headers? ? 'x-grape-client' : 'X-Grape-Client' expect(JSON.parse(last_response.body)[x_grape_client_header]).to eq('1') end @@ -154,7 +154,7 @@ def app env = Rack::MockRequest.env_for('/headers') env[:HTTP_SYMBOL_HEADER] = 'Goliath passes symbols' body = read_chunks(subject.call(env)[2]).join - symbol_header = Grape.rack3? ? 'symbol-header' : 'Symbol-Header' + symbol_header = Grape.lowercase_headers? ? 'symbol-header' : 'Symbol-Header' expect(JSON.parse(body)[symbol_header]).to eq('Goliath passes symbols') end end @@ -499,7 +499,7 @@ def app end it 'responses with given content type in headers' do - expect(last_response.headers[Grape::Http::Headers::CONTENT_TYPE]).to eq 'application/json; charset=utf-8' + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq 'application/json; charset=utf-8' end end diff --git a/spec/grape/request_spec.rb b/spec/grape/request_spec.rb index 3c0c9e2de3..b84b6dffdc 100644 --- a/spec/grape/request_spec.rb +++ b/spec/grape/request_spec.rb @@ -90,7 +90,7 @@ module Grape } end let(:x_grape_is_cool_header) do - Grape.rack3? ? 'x-grape-is-cool' : 'X-Grape-Is-Cool' + Grape.lowercase_headers? ? 'x-grape-is-cool' : 'X-Grape-Is-Cool' end it 'cuts HTTP_ prefix and capitalizes header name words' do @@ -120,7 +120,7 @@ module Grape default_env.merge(request_headers) end let(:grape_likes_symbolic_header) do - Grape.rack3? ? 'grape-likes-symbolic' : 'Grape-Likes-Symbolic' + Grape.lowercase_headers? ? 'grape-likes-symbolic' : 'Grape-Likes-Symbolic' end it 'converts them to string' do diff --git a/spec/integration/rack/v2/headers_spec.rb b/spec/integration/rack/v2/headers_spec.rb index 63c8e45727..4819f21dc1 100644 --- a/spec/integration/rack/v2/headers_spec.rb +++ b/spec/integration/rack/v2/headers_spec.rb @@ -2,9 +2,6 @@ describe Grape::Http::Headers do it { expect(described_class::ALLOW).to eq('Allow') } - it { expect(described_class::CACHE_CONTROL).to eq('Cache-Control') } - it { expect(described_class::CONTENT_LENGTH).to eq('Content-Length') } - it { expect(described_class::CONTENT_TYPE).to eq('Content-Type') } it { expect(described_class::LOCATION).to eq('Location') } it { expect(described_class::TRANSFER_ENCODING).to eq('Transfer-Encoding') } it { expect(described_class::X_CASCADE).to eq('X-Cascade') } diff --git a/spec/integration/rack/v3/headers_spec.rb b/spec/integration/rack/v3/headers_spec.rb index 3f5375e0c7..3be2c1e28b 100644 --- a/spec/integration/rack/v3/headers_spec.rb +++ b/spec/integration/rack/v3/headers_spec.rb @@ -2,9 +2,6 @@ describe Grape::Http::Headers do it { expect(described_class::ALLOW).to eq('allow') } - it { expect(described_class::CACHE_CONTROL).to eq('cache-control') } - it { expect(described_class::CONTENT_LENGTH).to eq('content-length') } - it { expect(described_class::CONTENT_TYPE).to eq('content-type') } it { expect(described_class::LOCATION).to eq('location') } it { expect(described_class::TRANSFER_ENCODING).to eq('transfer-encoding') } it { expect(described_class::X_CASCADE).to eq('x-cascade') }