diff --git a/CHANGELOG.md b/CHANGELOG.md index 76b0b4700..dbb5bd64b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ #### Fixes +* [#1114](https://github.com/ruby-grape/grape/pull/1114): Fix regression which broke identical endpoints with different versions - [@suan](https://github.com/suan). * [#1109](https://github.com/ruby-grape/grape/pull/1109): Memoize Virtus attribute and fix memory leak - [@marshall-lee](https://github.com/marshall-lee). * [#1101](https://github.com/intridea/grape/pull/1101): Fix: Incorrect media-type `Accept` header now correctly returns 406 with `strict: true` - [@elliotlarson](https://github.com/elliotlarson). * [#1108](https://github.com/ruby-grape/grape/pull/1039): Raise a warning when `desc` is called with options hash and block - [@rngtng](https://github.com/rngtng). diff --git a/README.md b/README.md index a2f729863..f25521d1c 100644 --- a/README.md +++ b/README.md @@ -1608,6 +1608,10 @@ rescue_from RuntimeError, rescue_subclasses: false do |e| end ``` +#### Unrescuable Exceptions + +`Grape::Exceptions::InvalidVersionHeader`, which is raised when the version in the request header doesn't match the currently evaluated version for the endpoint, will _never_ be rescued from a `rescue_from` block (even a `rescue_from :all`) This is because Grape relies on Rack to catch that error and try the next versioned-route for cases where there exist identical Grape endpoints with different versions. + ### Rails 3.x When mounted inside containers, such as Rails 3.x, errors like "404 Not Found" or diff --git a/UPGRADING.md b/UPGRADING.md index 68c461b7d..b8c11b57d 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,6 +1,14 @@ Upgrading Grape =============== +### Upgrading to >= 0.13.1 + +#### Changes to header versioning and invalid header version handling + +Identical endpoints with different versions now work correctly. A regression introduced in Grape 0.11.0 caused all but the first-mounted version for such an endpoint to wrongly throw an `InvalidAcceptHeader`. As a side effect, requests with a correct vendor but invalid version can no longer be rescued from a `rescue_from` block. + +See [#1114](https://github.com/ruby-grape/grape/pull/1114) for more information. + ### Upgrading to >= 0.12.0 #### Changes in middleware diff --git a/lib/grape.rb b/lib/grape.rb index b4abcf217..52b30edec 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -68,6 +68,7 @@ module Exceptions autoload :UnsupportedGroupTypeError, 'grape/exceptions/unsupported_group_type' autoload :InvalidMessageBody autoload :InvalidAcceptHeader + autoload :InvalidVersionHeader end module ErrorFormatter diff --git a/lib/grape/exceptions/invalid_version_header.rb b/lib/grape/exceptions/invalid_version_header.rb new file mode 100644 index 000000000..48f2307e5 --- /dev/null +++ b/lib/grape/exceptions/invalid_version_header.rb @@ -0,0 +1,10 @@ +# encoding: utf-8 +module Grape + module Exceptions + class InvalidVersionHeader < Base + def initialize(message, headers) + super(message: compose_message('invalid_version_header', message: message), status: 406, headers: headers) + end + end + end +end diff --git a/lib/grape/locale/en.yml b/lib/grape/locale/en.yml index 3b5f0ecd3..358846ec0 100644 --- a/lib/grape/locale/en.yml +++ b/lib/grape/locale/en.yml @@ -45,4 +45,7 @@ en: invalid_accept_header: problem: 'Invalid accept header' resolution: '%{message}' + invalid_version_header: + problem: 'Invalid version header' + resolution: '%{message}' diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index 8f3d23027..9716e53d4 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -47,6 +47,7 @@ def find_handler(klass) end def rescuable?(klass) + return false if klass == Grape::Exceptions::InvalidVersionHeader options[:rescue_all] || (options[:rescue_handlers] || []).any? { |error, _handler| klass <= error } || (options[:base_only_rescue_handlers] || []).include?(klass) end diff --git a/lib/grape/middleware/versioner/header.rb b/lib/grape/middleware/versioner/header.rb index 6c780207c..c3ce0d318 100644 --- a/lib/grape/middleware/versioner/header.rb +++ b/lib/grape/middleware/versioner/header.rb @@ -30,8 +30,10 @@ def before if media_type media_type_header_handler - elsif headers_contain_wrong_vendor_or_version? - fail_with_invalid_accept_header!('API vendor or version not found.') + elsif headers_contain_wrong_vendor? + fail_with_invalid_accept_header!('API vendor not found.') + elsif headers_contain_wrong_version? + fail_with_invalid_version_header!('API version not found.') end end @@ -39,7 +41,7 @@ def before def strict_header_checks strict_accept_header_presence_check - strict_verion_vendor_accept_header_presence_check + strict_version_vendor_accept_header_presence_check end def strict_accept_header_presence_check @@ -47,7 +49,7 @@ def strict_accept_header_presence_check fail_with_invalid_accept_header!('Accept header must be set.') end - def strict_verion_vendor_accept_header_presence_check + def strict_version_vendor_accept_header_presence_check return unless versions.present? return if an_accept_header_with_version_and_vendor_is_present? fail_with_invalid_accept_header!('API vendor or version not found.') @@ -85,6 +87,11 @@ def fail_with_invalid_accept_header!(message) .new(message, error_headers) end + def fail_with_invalid_version_header!(message) + fail Grape::Exceptions::InvalidVersionHeader + .new(message, error_headers) + end + def available_media_types available_media_types = [] @@ -107,9 +114,15 @@ def available_media_types available_media_types.flatten end - def headers_contain_wrong_vendor_or_version? + def headers_contain_wrong_vendor? + header.values.all? do |header_value| + vendor?(header_value) && request_vendor(header_value) != vendor + end + end + + def headers_contain_wrong_version? header.values.all? do |header_value| - vendor?(header_value) || version?(header_value) + version?(header_value) end end @@ -159,6 +172,11 @@ def vendor?(media_type) subtype[/\Avnd\.[a-z0-9*.]+/] end + def request_vendor(media_type) + _, subtype = Rack::Accept::Header.parse_media_type(media_type) + subtype.match(VENDOR_VERSION_HEADER_REGEX)[1] + end + # @param [String] media_type a content type # @return [Boolean] whether the content type sets an API version def version?(media_type) diff --git a/spec/grape/exceptions/invalid_accept_header_spec.rb b/spec/grape/exceptions/invalid_accept_header_spec.rb index 924d08359..d308620cc 100644 --- a/spec/grape/exceptions/invalid_accept_header_spec.rb +++ b/spec/grape/exceptions/invalid_accept_header_spec.rb @@ -54,13 +54,6 @@ def app end context 'that receives' do - context 'an invalid version in the request' do - before do - get '/beer', {}, 'HTTP_ACCEPT' => 'application/vnd.vendorname-v77', - 'CONTENT_TYPE' => 'application/json' - end - it_should_behave_like 'a rescued request' - end context 'an invalid vendor in the request' do before do get '/beer', {}, 'HTTP_ACCEPT' => 'application/vnd.invalidvendor-v99', @@ -128,13 +121,6 @@ def app end context 'that receives' do - context 'an invalid version in the request' do - before do - get '/beer', {}, 'HTTP_ACCEPT' => 'application/vnd.vendorname-v77', - 'CONTENT_TYPE' => 'application/json' - end - it_should_behave_like 'a rescued request' - end context 'an invalid vendor in the request' do before do get '/beer', {}, 'HTTP_ACCEPT' => 'application/vnd.invalidvendor-v99', diff --git a/spec/grape/middleware/versioner/header_spec.rb b/spec/grape/middleware/versioner/header_spec.rb index 1949db2a2..c0c5e824e 100644 --- a/spec/grape/middleware/versioner/header_spec.rb +++ b/spec/grape/middleware/versioner/header_spec.rb @@ -89,7 +89,7 @@ expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader) expect(exception.headers).to eql('X-Cascade' => 'pass') expect(exception.status).to eql 406 - expect(exception.message).to include 'API vendor or version not found' + expect(exception.message).to include 'API vendor not found' end end @@ -116,7 +116,7 @@ expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader) expect(exception.headers).to eql('X-Cascade' => 'pass') expect(exception.status).to eql 406 - expect(exception.message).to include('API vendor or version not found') + expect(exception.message).to include('API vendor not found') end end end @@ -141,10 +141,10 @@ 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::InvalidAcceptHeader) + expect(exception).to be_a(Grape::Exceptions::InvalidVersionHeader) expect(exception.headers).to eql('X-Cascade' => 'pass') expect(exception.status).to eql 406 - expect(exception.message).to include('API vendor or version not found') + expect(exception.message).to include('API version not found') end end end @@ -244,10 +244,60 @@ 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::InvalidAcceptHeader) + expect(exception).to be_a(Grape::Exceptions::InvalidVersionHeader) expect(exception.headers).to eql('X-Cascade' => 'pass') expect(exception.status).to eql 406 - expect(exception.message).to include('API vendor or version not found') + expect(exception.message).to include('API version not found') + end + end + end + + context 'when there are multiple versions specified with rescue_from :all' do + subject { + Class.new(Grape::API) do + rescue_from :all + end + } + + let(:v1_app) { + Class.new(Grape::API) do + version 'v1', using: :header, vendor: 'test' + resources :users do + get :hello do + 'one' + end + end + end + } + + let(:v2_app) { + Class.new(Grape::API) do + version 'v2', using: :header, vendor: 'test' + resources :users do + get :hello do + 'two' + end + end + end + } + + def app + subject.mount v1_app + subject.mount v2_app + subject + end + + context 'with header versioned endpoints and a rescue_all block defined' do + it 'responds correctly to a v1 request' do + versioned_get '/users/hello', 'v1', using: :header, vendor: 'test' + expect(last_response.body).to eq('one') + expect(last_response.body).not_to include('API vendor or version not found') + end + + it 'responds correctly to a v2 request' do + versioned_get '/users/hello', 'v2', using: :header, vendor: 'test' + expect(last_response.body).to eq('two') + expect(last_response.body).not_to include('API vendor or version not found') end end end