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

Set response headers based on Rack version #2355

Merged
merged 12 commits into from
Oct 24, 2023
10 changes: 10 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 2 additions & 0 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,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.
Expand Down
5 changes: 3 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
### 1.8.1 (Next)
### 1.9.0 (Next)

#### 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).
* [#2360](https://github.com/ruby-grape/grape/pull/2360): Reduce gem size by removing specs - [@ericproulx](https://github.com/ericproulx).
* Your contribution here.

#### Fixes

* Your contribution here.
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ content negotiation, versioning and much more.

## Stable Release

You're reading the documentation for the next release of Grape, which should be **1.8.1**.
You're reading the documentation for the next release of Grape, which should be **1.9.0**.
Please read [UPGRADING](UPGRADING.md) when upgrading from a previous version.
The current stable release is [1.8.0](https://github.com/ruby-grape/grape/blob/v1.8.0/README.md).

Expand Down
26 changes: 26 additions & 0 deletions UPGRADING.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,32 @@
Upgrading Grape
===============

### Upgrading to >= 1.9.0

#### Response 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, the following headers are now lowercase:

* `allow`
* `cache-control`
* `content-length`
* `content-type`
* `location`
* `transfer-encoding`
* `x-cascade`

For Rack < 3 the following response headers are returned using HTTP/1 semantics, like so:

* `Allow`
* `Cache-Control`
* `Content-Length`
* `Content-Type`
* `Location`
* `Transfer-Encoding`
* `X-Cascade`

dblock marked this conversation as resolved.
Show resolved Hide resolved
See [#2355](https://github.com/ruby-grape/grape/pull/2355) for more information.

### Upgrading to >= 1.7.0

#### Exceptions renaming
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 @@ -185,7 +185,7 @@ def redirect(url, permanent: false, body: nil, **_options)
status 302
body_message ||= "This resource has been moved temporarily to #{url}."
end
header 'Location', url
header Grape::Http::Headers::LOCATION, url
content_type 'text/plain'
body body_message
end
Expand Down Expand Up @@ -328,9 +328,9 @@ def sendfile(value = nil)
def stream(value = nil)
return if value.nil? && @stream.nil?

header 'Content-Length', nil
header 'Transfer-Encoding', nil
header 'Cache-Control', 'no-cache' # Skips ETag generation (reading the response up front)
header Grape::Http::Headers::CONTENT_LENGTH, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Rack::CONTENT_LENGTH if that suits your use case.

header Grape::Http::Headers::TRANSFER_ENCODING, nil
header Grape::Http::Headers::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
2 changes: 1 addition & 1 deletion lib/grape/endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ def run
if (allowed_methods = env[Grape::Env::GRAPE_ALLOWED_METHODS])
raise Grape::Exceptions::MethodNotAllowed.new(header.merge('Allow' => allowed_methods)) unless options?

header 'Allow', allowed_methods
header Grape::Http::Headers::ALLOW, allowed_methods
response_object = ''
status 204
else
Expand Down
20 changes: 18 additions & 2 deletions lib/grape/http/headers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,24 @@ 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')
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'
end

GET = 'GET'
POST = 'POST'
Expand All @@ -24,7 +41,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'

Expand Down
6 changes: 5 additions & 1 deletion lib/grape/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ def build_headers
end

def transform_header(header)
-header[5..].split('_').each(&:capitalize!).join('-')
if Gem::Version.new(Rack.release) < Gem::Version.new('3')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This executes in a tight loop, which is bad for performance. You want this check outside. We also have it in a few places. It should look like this:

if Grape::Rack.rack3?
  def transform_header(header)
      -header[5..].split('_').map(&:capitalize).join('-')
  end
else
  def transform_header(header)
      -header[5..].tr('_', '-').downcase
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added .rack3? method and done this change, although I've added it to Grape rather than Grape::Rack otherwise there was a load of Rack errors raised because we would need to change all the Rack references to ::Rack.

Can do this though if really needs to be Grape::Rack

-header[5..].split('_').map(&:capitalize).join('-')
else
-header[5..].tr('_', '-').downcase
end
end
end
end
2 changes: 1 addition & 1 deletion lib/grape/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@

module Grape
# The current version of Grape.
VERSION = '1.8.1'
VERSION = '1.9.0'
end
15 changes: 12 additions & 3 deletions spec/grape/api/custom_validations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,18 @@ def validate(request)
return unless request.params.key? @attrs.first
# check if admin flag is set to true
return unless @option

# check if user is admin or not
# as an example get a token from request and check if it's admin or not
raise Grape::Exceptions::Validation.new(params: @attrs, message: 'Can not set Admin only field.') unless request.headers['X-Access-Token'] == 'admin'
raise Grape::Exceptions::Validation.new(params: @attrs, message: 'Can not set Admin only field.') unless request.headers[access_header] == 'admin'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use x_access_token_header here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, tried that, but got...

     NameError:
       undefined local variable or method `x_access_token_header' for #<#<Class:0x0000ffff8f8f9b18>:0x0000ffff928d97e8 @attrs=[:admin_field], @option=true, @required=false, @scope=#<Grape::Validations::ParamsScope:0x0000ffff8f8f96b8 @element=nil, @element_renamed=nil, @parent=nil, @api=#<Class:0x0000ffff8f8f9898>, @optional=false, @type=Hash, @group=nil, @dependent_on=nil, @declared_params=nil, @index=nil>, @fail_fast=false, @allow_blank=nil>

which was annoying, as don't like the defining it twice either.

end

def access_header
if Gem::Version.new(Rack.release) < Gem::Version.new('3')
'X-Access-Token'
else
'x-access-token'
end
end
end
end
Expand Down Expand Up @@ -197,14 +206,14 @@ def validate(request)
end

it 'does not fail when we send admin fields and we are admin' do
header 'X-Access-Token', 'admin'
header rack_versioned_headers[:x_access_token], 'admin'
get '/', admin_field: 'tester', non_admin_field: 'toaster', admin_false_field: 'test'
expect(last_response.status).to eq 200
expect(last_response.body).to eq 'bacon'
end

it 'fails when we send admin fields and we are not admin' do
header 'X-Access-Token', 'user'
header rack_versioned_headers[:x_access_token], 'user'
get '/', admin_field: 'tester', non_admin_field: 'toaster', admin_false_field: 'test'
expect(last_response.status).to eq 400
expect(last_response.body).to include 'Can not set Admin only field.'
Expand Down
52 changes: 26 additions & 26 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['Content-Type']).to eql 'text/plain'
expect(last_response.headers[rack_versioned_headers[:content_type]]).to eql 'text/plain'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get rid of rack_versioned_headers and instead do Grape::HTTP::Headers::CONTENT_TYPE?

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['Content-Type']).to eq('text/plain')
expect(last_response.headers[rack_versioned_headers[:content_type]]).to eq('text/plain')
end

it 'does not set Cache-Control' do
get '/foo'
expect(last_response.headers['Cache-Control']).to be_nil
expect(last_response.headers[rack_versioned_headers[:cache_control]]).to be_nil
end

it 'sets content type for xml' do
get '/foo.xml'
expect(last_response.headers['Content-Type']).to eq('application/xml')
expect(last_response.headers[rack_versioned_headers[:content_type]]).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[rack_versioned_headers[:content_type]]).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[rack_versioned_headers[:content_type]]).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[rack_versioned_headers[: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['Content-Type']).to eq('application/octet-stream')
expect(last_response.headers[rack_versioned_headers[: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['Content-Length']).to eq('25')
expect(last_response.headers['Content-Type']).to eq('text/plain')
expect(last_response.headers[rack_versioned_headers[:content_length]]).to eq('25')
expect(last_response.headers[rack_versioned_headers[:content_type]]).to eq('text/plain')
expect(last_response.body).to eq(file_content)
end

Expand All @@ -1257,34 +1257,34 @@ 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-Length']).to be_nil
expect(last_response.headers['Cache-Control']).to eq('no-cache')
expect(last_response.headers['Transfer-Encoding']).to eq('chunked')
expect(last_response.headers[rack_versioned_headers[:content_type]]).to eq('text/plain')
expect(last_response.headers[rack_versioned_headers[:content_length]]).to be_nil
expect(last_response.headers[rack_versioned_headers[:cache_control]]).to eq('no-cache')
expect(last_response.headers[rack_versioned_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")
end

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[rack_versioned_headers[: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['Content-Type']).to eql 'application/json'
expect(last_response.headers[rack_versioned_headers[: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['Content-Type']).to eql 'application/xml'
expect(last_response.headers[rack_versioned_headers[: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['Content-Type']).to eql 'application/custom'
expect(last_response.headers[rack_versioned_headers[:content_type]]).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[rack_versioned_headers[: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['Content-Type']).to eq('image/png')
expect(last_response.headers[rack_versioned_headers[: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['Content-Type']).to eq('application/x-ruby')
expect(last_response.headers[rack_versioned_headers[: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 Expand Up @@ -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[rack_versioned_headers[:x_cascade]] == 'pass' if env['PATH_INFO'].exclude?('boo')
[200, headers, ['Farfegnugen']]
} => '/'

Expand Down Expand Up @@ -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[rack_versioned_headers[:x_cascade]]).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 rack_versioned_headers[:x_cascade]
end
end

Expand All @@ -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[rack_versioned_headers[:x_cascade]]).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 rack_versioned_headers[:x_cascade]
end
end
end
Expand Down
Loading