Merge pull request #1488 from ruby-grape/fix-1429

Ensure calling before filters when receiving OPTIONS request

Author: dblock

.rubocop_todo.yml

@@ -1,12 +1,12 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2016-04-27 13:48:11 -0500 using RuboCop version 0.39.0.
+# on 2016-09-11 17:59:25 +0900 using RuboCop version 0.39.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.
 
-# Offense count: 39
+# Offense count: 42
 Metrics/AbcSize:
   Max: 44
 
@@ -17,37 +17,37 @@ Metrics/BlockNesting:
 # Offense count: 7
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 266
+  Max: 277
 
-# Offense count: 24
+# Offense count: 28
 Metrics/CyclomaticComplexity:
   Max: 14
 
-# Offense count: 874
+# Offense count: 933
 # Configuration parameters: AllowHeredoc, AllowURI, URISchemes.
 # URISchemes: http, https
 Metrics/LineLength:
   Max: 215
 
-# Offense count: 48
+# Offense count: 52
 # Configuration parameters: CountComments.
 Metrics/MethodLength:
-  Max: 34
+  Max: 33
 
-# Offense count: 7
+# Offense count: 8
 # Configuration parameters: CountComments.
 Metrics/ModuleLength:
   Max: 212
 
-# Offense count: 15
+# Offense count: 18
 Metrics/PerceivedComplexity:
-  Max: 16
+  Max: 14
 
-# Offense count: 111
+# Offense count: 114
 Style/Documentation:
   Enabled: false
 
-# Offense count: 14
+# Offense count: 16
 # Configuration parameters: EnforcedStyle, SupportedStyles.
 # SupportedStyles: compact, exploded
 Style/RaiseArgs:

CHANGELOG.md

@@ -3,11 +3,13 @@
 
 * [#1480](https://github.com/ruby-grape/grape/pull/1480): Use the ruby-grape-danger gem for PR linting - [@dblock](https://github.com/dblock).
 * [#1486](https://github.com/ruby-grape/grape/pull/1486): Implemented except in values validator - [@jonmchan](https://github.com/jonmchan).
+* [#1470](https://github.com/ruby-grape/grape/pull/1470): Drop support for ruby-2.0 - [@namusyaka](https://github.com/namusyaka).
 * Your contribution here.
 
 #### Fixes
 
 * [#1479](https://github.com/ruby-grape/grape/pull/1479): Support inserting middleware before/after anonymous classes in the middleware stack - [@rosa](https://github.com/rosa).
+* [#1488](https://github.com/ruby-grape/grape/pull/1488): Ensure calling before filters when receiving OPTIONS request - [@namusyaka](https://github.com/namusyaka), [@jlfaber](https://github.com/jlfaber).
 
 0.17.0 (7/29/2016)
 ==================
@@ -20,7 +22,6 @@
 * [#1398](https://github.com/ruby-grape/grape/pull/1398): Add `rescue_from :grape_exceptions` - allow Grape to use the built-in `Grape::Exception` handing and use `rescue :all` behavior for everything else - [@mmclead](https://github.com/mmclead).
 * [#1443](https://github.com/ruby-grape/grape/pull/1443): Extend `given` to receive a `Proc` - [@glaucocustodio](https://github.com/glaucocustodio).
 * [#1455](https://github.com/ruby-grape/grape/pull/1455): Add an automated PR linter - [@orta](https://github.com/orta).
-* [#1470](https://github.com/ruby-grape/grape/pull/1470): Drop support for ruby-2.0 - [@namusyaka](https://github.com/namusyaka).
 * Your contribution here.
 
 #### Fixes

lib/grape/api.rb

@@ -173,7 +173,6 @@ def add_head_not_allowed_methods_and_options_methods
         without_versioning do
           routes_map.each do |_, config|
             methods = config[:methods]
-            path = config[:path]
             allowed_methods = methods.dup
 
             unless self.class.namespace_inheritable(:do_not_route_head)
@@ -182,8 +181,8 @@ def add_head_not_allowed_methods_and_options_methods
 
             allow_header = (self.class.namespace_inheritable(:do_not_route_options) ? allowed_methods : [Grape::Http::Headers::OPTIONS] | allowed_methods).join(', ')
 
-            unless self.class.namespace_inheritable(:do_not_route_options)
-              generate_options_method(path, allow_header, config) unless allowed_methods.include?(Grape::Http::Headers::OPTIONS)
+            unless self.class.namespace_inheritable(:do_not_route_options) || allowed_methods.include?(Grape::Http::Headers::OPTIONS)
+              config[:endpoint].options[:options_route_enabled] = true
             end
 
             attributes = config.merge(allowed_methods: allowed_methods, allow_header: allow_header)
@@ -193,15 +192,6 @@ def add_head_not_allowed_methods_and_options_methods
       end
     end
 
-    # Generate an 'OPTIONS' route for a pre-exisiting user defined route
-    def generate_options_method(path, allow_header, options = {})
-      self.class.options(path, options) do
-        header 'Allow', allow_header
-        status 204
-        ''
-      end
-    end
-
     # Generate a route that returns an HTTP 405 response for a user defined
     # path on methods not specified
     def generate_not_allowed_method(pattern, attributes = {})

lib/grape/endpoint.rb

@@ -248,16 +248,21 @@ def run
 
         run_filters befores, :before
 
-        allowed_methods = env[Grape::Env::GRAPE_METHOD_NOT_ALLOWED]
-        raise Grape::Exceptions::MethodNotAllowed, header.merge('Allow' => allowed_methods) if allowed_methods
+        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
 
-        response_object = @block ? @block.call(self) : nil
+        response_object =
+          if options?
+            header 'Allow', allowed_methods
+            status 204
+            ''
+          else
+            @block ? @block.call(self) : nil
+          end
         run_filters afters, :after
         cookies.write(header)
 
@@ -373,5 +378,10 @@ def afters
     def validations
       route_setting(:saved_validations) || []
     end
+
+    def options?
+      options[:options_route_enabled] &&
+        env[Grape::Http::Headers::REQUEST_METHOD] == Grape::Http::Headers::OPTIONS
+    end
   end
 end

lib/grape/middleware/versioner/header.rb

@@ -32,7 +32,7 @@ class Header < Base
         def before
           strict_header_checks if strict?
 
-          if media_type || env[Grape::Env::GRAPE_METHOD_NOT_ALLOWED]
+          if media_type || env[Grape::Env::GRAPE_ALLOWED_METHODS]
             media_type_header_handler
           elsif headers_contain_wrong_vendor?
             fail_with_invalid_accept_header!('API vendor not found.')

lib/grape/router.rb

@@ -95,7 +95,7 @@ def transaction(env)
       neighbor = greedy_match?(input)
       return unless neighbor
 
-      (!cascade && neighbor) ? method_not_allowed(env, neighbor.allow_header, neighbor.endpoint) : nil
+      (!cascade && neighbor) ? call_with_allow_headers(env, neighbor.allow_header, neighbor.endpoint) : nil
     end
 
     def make_routing_args(default_args, route, input)
@@ -132,8 +132,8 @@ def greedy_match?(input)
       @neutral_map.detect { |route| last_match["_#{route.index}"] }
     end
 
-    def method_not_allowed(env, methods, endpoint)
-      env[Grape::Env::GRAPE_METHOD_NOT_ALLOWED] = methods
+    def call_with_allow_headers(env, methods, endpoint)
+      env[Grape::Env::GRAPE_ALLOWED_METHODS] = methods
       endpoint.call(env)
     end
 

lib/grape/util/env.rb

@@ -18,6 +18,6 @@ module Env
     GRAPE_REQUEST_HEADERS = 'grape.request.headers'.freeze
     GRAPE_REQUEST_PARAMS = 'grape.request.params'.freeze
     GRAPE_ROUTING_ARGS = 'grape.routing_args'.freeze
-    GRAPE_METHOD_NOT_ALLOWED = 'grape.method_not_allowed'.freeze
+    GRAPE_ALLOWED_METHODS = 'grape.allowed_methods'.freeze
   end
 end

spec/grape/api_spec.rb

@@ -665,6 +665,47 @@ class DummyFormatClass
       end
     end
 
+    describe 'adds an OPTIONS route for namespaced endpoints that' do
+      before do
+        subject.before { header 'X-Custom-Header', 'foo' }
+        subject.namespace :example do
+          before { header 'X-Custom-Header-2', 'foo' }
+          get :inner do
+            'example/inner'
+          end
+        end
+        options '/example/inner'
+      end
+
+      it 'returns a 204' do
+        expect(last_response.status).to eql 204
+      end
+
+      it 'has an empty body' do
+        expect(last_response.body).to be_blank
+      end
+
+      it 'has an Allow header' do
+        expect(last_response.headers['Allow']).to eql 'OPTIONS, GET, HEAD'
+      end
+
+      it 'calls the outer before filter' do
+        expect(last_response.headers['X-Custom-Header']).to eql 'foo'
+      end
+
+      it 'calls the inner before filter' do
+        expect(last_response.headers['X-Custom-Header-2']).to eql 'foo'
+      end
+
+      it 'has no Content-Type' do
+        expect(last_response.content_type).to be_nil
+      end
+
+      it 'has no Content-Length' do
+        expect(last_response.content_length).to be_nil
+      end
+    end
+
     describe 'adds a 405 Not Allowed route that' do
       before do
         subject.before { header 'X-Custom-Header', 'foo' }