Skip to content

Commit

Permalink
Additional changes for setting Rack versioned headers (ruby-grape#2362)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
schinery authored Oct 25, 2023
1 parent 2f62365 commit de76b5c
Show file tree
Hide file tree
Showing 14 changed files with 57 additions and 69 deletions.
2 changes: 1 addition & 1 deletion UPGRADING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
4 changes: 2 additions & 2 deletions lib/grape.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions lib/grape/dsl/inside_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand Down
8 changes: 1 addition & 7 deletions lib/grape/http/headers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
8 changes: 4 additions & 4 deletions lib/grape/middleware/error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
6 changes: 3 additions & 3 deletions lib/grape/middleware/formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
2 changes: 1 addition & 1 deletion lib/grape/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions spec/grape/api/custom_validations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
40 changes: 20 additions & 20 deletions spec/grape/api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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

Expand All @@ -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")
Expand All @@ -1268,23 +1268,23 @@ 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
subject.format :json
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
subject.format :xml
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
Expand Down Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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
Expand Down
28 changes: 14 additions & 14 deletions spec/grape/dsl/inside_route_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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

Expand All @@ -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
Expand All @@ -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

Expand Down
Loading

0 comments on commit de76b5c

Please sign in to comment.