Allow invalid header version errors to bubble up

suan · Suan-Aik Yeo · commit 2fa517c4ef4f · 2015-08-26T18:55:31.000-04:00
fixes #968
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -30,24 +30,26 @@ 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
 
         private
 
         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
           return unless header.qvalues.empty?
           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
@@ -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
@@ -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
