auto OPTIONS should not check params or call _validation hooks

Fixes #1506
ruby-grape · Oct 21, 2016 · eb1711a · eb1711a
1 parent aa81fff
commit eb1711a
Show file tree

Hide file tree

Showing 5 changed files with 96 additions and 35 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,11 +1,17 @@
 Next Release
 ============
 
+#### Features
+
 * [#1503](https://github.com/ruby-grape/grape/pull/1503): Allow to use regexp validator with arrays - [@akoltun](https://github.com/akoltun).
 * [#1507](https://github.com/ruby-grape/grape/pull/1507): Add group attributes for parameter definitions - [@304](https://github.com/304).
 * [#1512](https://github.com/ruby-grape/grape/pull/1512): Fix for deeply nested params TypeError situation - [@krbs](https://github.com/krbs).
 * Your contribution here.
 
+#### Fixes
+
+* [#1505](https://github.com/ruby-grape/grape/pull/1505): Run `before` and `after` callbacks, but skip the rest when handling OPTIONS - [@jlfaber](https://github.com/jlfaber).
+
 0.18.0 (10/7/2016)
 ==================
 

diff --git a/README.md b/README.md
@@ -1807,7 +1807,9 @@ end
 
 When you add a route for a resource, a route for the `OPTIONS`
 method will also be added. The response to an OPTIONS request will
-include an "Allow" header listing the supported methods.
+include an "Allow" header listing the supported methods. If the resource
+has `before` and `after` callbacks they will be executed, but no other callbacks will
+run.
 
 ```ruby
 class API < Grape::API
@@ -1837,7 +1839,9 @@ curl -v -X OPTIONS http://localhost:3000/rt_count
 You can disable this behavior with `do_not_route_options!`.
 
 If a request for a resource is made with an unsupported HTTP method, an
-HTTP 405 (Method Not Allowed) response will be returned.
+HTTP 405 (Method Not Allowed) response will be returned. If the resource
+has `before` callbacks they will be executed, but no other callbacks will
+run.
 
 ``` shell
 curl -X DELETE -v http://localhost:3000/rt_count/
@@ -2797,6 +2801,14 @@ Before and after callbacks execute in the following order:
 
 Steps 4, 5 and 6 only happen if validation succeeds.
 
+If a request for a resource is made with an unsupported HTTP method (returning
+HTTP 405) only `before` callbacks will be executed.  The remaining callbacks will
+be bypassed.
+
+If a request for a resource is made that triggers the built-in `OPTIONS` handler,
+only `before` and `after` callbacks will be executed.  The remaining callbacks will
+be bypassed.
+
 #### Examples
 
 Using a simple `before` block to set a header

diff --git a/UPGRADING.md b/UPGRADING.md
@@ -1,6 +1,14 @@
 Upgrading Grape
 ===============
 
+### Upgrading to >= 0.18.1
+
+#### Removed param processing from built-in OPTIONS handler
+
+When a request is made to the built-in `OPTIONS` handler, only the `before` and `after`
+callbacks associated with the resource will be run.  The `before_validation` and
+`after_validation` callbacks and parameter validations will be skipped.
+
 ### Upgrading to >= 0.17.0
 
 #### Removed official support for Ruby < 2.2.2

diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb
@@ -124,8 +124,8 @@ def method_name
       [options[:method],
        Namespace.joined_space(namespace_stackable(:namespace)),
        (namespace_stackable(:mount_path) || []).join('/'),
-       options[:path].join('/')
-      ].join(' ')
+       options[:path].join('/')]
+        .join(' ')
     end
 
     def routes
@@ -248,21 +248,18 @@ def run
 
         run_filters befores, :before
 
-        allowed_methods = env[Grape::Env::GRAPE_ALLOWED_METHODS]
-        raise Grape::Exceptions::MethodNotAllowed, header.merge('Allow' => allowed_methods) if !options? && allowed_methods
-
-        run_filters before_validations, :before_validation
-        run_validators validations, request
-        run_filters after_validations, :after_validation
+        if (allowed_methods = env[Grape::Env::GRAPE_ALLOWED_METHODS])
+          raise Grape::Exceptions::MethodNotAllowed, header.merge('Allow' => allowed_methods) unless options?
+          header 'Allow', allowed_methods
+          response_object = ''
+          status 204
+        else
+          run_filters before_validations, :before_validation
+          run_validators validations, request
+          run_filters after_validations, :after_validation
+          response_object = @block ? @block.call(self) : nil
+        end
 
-        response_object =
-          if options?
-            header 'Allow', allowed_methods
-            status 204
-            ''
-          else
-            @block ? @block.call(self) : nil
-          end
         run_filters afters, :after
         cookies.write(header)
 

diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb
@@ -516,9 +516,15 @@ class DummyFormatClass
         end
         send(verb, '/example')
         expect(last_response.body).to eql verb == 'head' ? '' : verb
-        # Call it with a method other than the properly constrained one.
-        send(used_verb = verbs[(verbs.index(verb) + 2) % verbs.size], '/example')
-        expect(last_response.status).to eql used_verb == 'options' ? 204 : 405
+        # Call it with all methods other than the properly constrained one.
+        (verbs - [verb]).each do |other_verb|
+          send(other_verb, '/example')
+          expected_rc = if other_verb == 'options' then 204
+                        elsif other_verb == 'head' && verb == 'get' then 200
+                        else 405
+                        end
+          expect(last_response.status).to eql expected_rc
+        end
       end
     end
 
@@ -549,6 +555,7 @@ class DummyFormatClass
         before_validation { raise 'before_validation filter should not run' }
         after_validation  { raise 'after_validation filter should not run' }
         after             { raise 'after filter should not run' }
+        params { requires :only_for_get }
         get
       end
 
@@ -573,6 +580,26 @@ class DummyFormatClass
       expect(last_response.headers['X-Custom-Header']).to eql 'foo'
     end
 
+    it 'runs all filters and body with a custom OPTIONS method' do
+      subject.namespace :example do
+        before            { header 'X-Custom-Header-1', 'foo' }
+        before_validation { header 'X-Custom-Header-2', 'foo' }
+        after_validation  { header 'X-Custom-Header-3', 'foo' }
+        after             { header 'X-Custom-Header-4', 'foo' }
+        options { 'yup' }
+        get
+      end
+
+      options '/example'
+      expect(last_response.status).to eql 200
+      expect(last_response.body).to eql 'yup'
+      expect(last_response.headers['Allow']).to be_nil
+      expect(last_response.headers['X-Custom-Header-1']).to eql 'foo'
+      expect(last_response.headers['X-Custom-Header-2']).to eql 'foo'
+      expect(last_response.headers['X-Custom-Header-3']).to eql 'foo'
+      expect(last_response.headers['X-Custom-Header-4']).to eql 'foo'
+    end
+
     context 'when format is xml' do
       it 'returns a 405 for an unsupported method' do
         subject.format :xml
@@ -594,8 +621,8 @@ class DummyFormatClass
     context 'when accessing env' do
       it 'returns a 405 for an unsupported method' do
         subject.before do
-          _custom_header_1 = headers['X-Custom-Header']
-          _custom_header_2 = env['HTTP_X_CUSTOM_HEADER']
+          _customheader1 = headers['X-Custom-Header']
+          _customheader2 = env['HTTP_X_CUSTOM_HEADER']
         end
         subject.get 'example' do
           'example'
@@ -630,7 +657,11 @@ class DummyFormatClass
 
     describe 'adds an OPTIONS route that' do
       before do
-        subject.before { header 'X-Custom-Header', 'foo' }
+        subject.before            { header 'X-Custom-Header', 'foo' }
+        subject.before_validation { header 'X-Custom-Header-2', 'bar' }
+        subject.after_validation  { header 'X-Custom-Header-3', 'baz' }
+        subject.after             { header 'X-Custom-Header-4', 'bing' }
+        subject.params { requires :only_for_get }
         subject.get 'example' do
           'example'
         end
@@ -652,10 +683,22 @@ class DummyFormatClass
         expect(last_response.headers['Allow']).to eql 'OPTIONS, GET, HEAD'
       end
 
-      it 'has a X-Custom-Header' do
+      it 'calls before hook' do
         expect(last_response.headers['X-Custom-Header']).to eql 'foo'
       end
 
+      it 'does not call before_validation hook' do
+        expect(last_response.headers.key?('X-Custom-Header-2')).to be false
+      end
+
+      it 'does not call after_validation hook' do
+        expect(last_response.headers.key?('X-Custom-Header-3')).to be false
+      end
+
+      it 'calls after hook' do
+        expect(last_response.headers['X-Custom-Header-4']).to eq 'bing'
+      end
+
       it 'has no Content-Type' do
         expect(last_response.content_type).to be_nil
       end
@@ -2555,13 +2598,11 @@ def static
           params: {
             'param1' => { required: true },
             'param2' => { required: false }
-          }
-        },
+          } },
         { description: 'global description',
           params: {
             'param2' => { required: false }
-          }
-        }
+          } }
       ]
     end
     it 'merges the parameters of the namespace with the parameters of the method' do
@@ -2585,8 +2626,7 @@ def static
           params: {
             'ns_param' => { required: true, desc: 'namespace parameter' },
             'method_param' => { required: false, desc: 'method parameter' }
-          }
-        }
+          } }
       ]
     end
     it 'merges the parameters of nested namespaces' do
@@ -2618,8 +2658,7 @@ def static
             'ns1_param' => { required: true, desc: 'ns1 param' },
             'ns2_param' => { required: true, desc: 'ns2 param' },
             'method_param' => { required: false, desc: 'method param' }
-          }
-        }
+          } }
       ]
     end
     it 'groups nested params and prevents overwriting of params with same name in different groups' do
@@ -2662,8 +2701,7 @@ def static
             'root_param' => { required: true, desc: 'root param' },
             'nested' => { required: true, type: 'Array' },
             'nested[nested_param]' => { required: true, desc: 'nested param' }
-          }
-        }
+          } }
       ]
     end
     it 'allows to set the type attribute on :group element' do