Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional changes for setting Rack versioned headers #2362

Merged
merged 5 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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