Merge pull request #1142 from jrforrest/bugfix/declared_scope

Makes #declared unavailable to before filters

Author: dblock

.rubocop_todo.yml

@@ -22,7 +22,7 @@ Metrics/BlockNesting:
 # Offense count: 5
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 249
+  Max: 250
 
 # Offense count: 23
 Metrics/CyclomaticComplexity:
@@ -41,7 +41,7 @@ Metrics/MethodLength:
 # Offense count: 8
 # Configuration parameters: CountComments.
 Metrics/ModuleLength:
-  Max: 271
+  Max: 272
 
 # Offense count: 17
 Metrics/PerceivedComplexity:

CHANGELOG.md

@@ -9,6 +9,7 @@
 
 #### Fixes
 
+* [#1142](https://github.com/ruby-grape/grape/pull/1114): Makes #declared unavailable to before filters - [@jrforrest](https://github.com/jrforrest)
 * [#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).

README.md

@@ -534,6 +534,10 @@ The returned hash is a `Hashie::Mash` instance, allowing you to access parameter
   declared(params).user == declared(params)["user"]
 ```
 
+
+The `#declared` method is not available to `before` filters, as those are evaluated prior
+to parameter coercion.
+
 ### Include missing
 
 By default `declared(params)` includes parameters that have `nil` values. If you want to return only the parameters that are not `nil`, you can use the `include_missing` option. By default, `include_missing` is set to `true`. Consider the following API:

UPGRADING.md

@@ -3,6 +3,12 @@ Upgrading Grape
 
 ### Upgrading to >= 0.13.1
 
+#### Changes to availability of DSL methods in filters
+
+The `#declared` method of the route DSL is no longer available in the `before` filter.  Using `declared` in a `before` filter will now raise `Grape::DSL::InsideRoute::MethodNotYetAvailable`.
+
+See [#1074](https://github.com/ruby-grape/grape/issues/1074) for discussion of the issue.
+
 #### 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.

lib/grape/dsl/inside_route.rb

@@ -6,46 +6,71 @@ module InsideRoute
       extend ActiveSupport::Concern
       include Grape::DSL::Settings
 
-      # A filtering method that will return a hash
-      # consisting only of keys that have been declared by a
-      # `params` statement against the current/target endpoint or parent
-      # namespaces.
-      #
-      # @param params [Hash] The initial hash to filter. Usually this will just be `params`
-      # @param options [Hash] Can pass `:include_missing`, `:stringify` and `:include_parent_namespaces`
-      # options. `:include_parent_namespaces` defaults to true, hence must be set to false if
-      # you want only to return params declared against the current/target endpoint.
-      def declared(params, options = {}, declared_params = nil)
-        options = options.reverse_merge(include_missing: true, include_parent_namespaces: true)
+      # Denotes a situation where a DSL method has been invoked in a
+      # filter which it should not yet be available in
+      class MethodNotYetAvailable < StandardError; end
 
-        declared_params ||= (!options[:include_parent_namespaces] ? route_setting(:declared_params) : (route_setting(:saved_declared_params) || [])).flatten(1) || []
+      # @param type [Symbol] The type of filter for which evaluation has been
+      #   completed
+      # @return [Module] A module containing method overrides suitable for the
+      #   position in the filter evaluation sequence denoted by +type+.  This
+      #   defaults to an empty module if no overrides are defined for the given
+      #   filter +type+.
+      def self.post_filter_methods(type)
+        @post_filter_modules ||= { before: PostBeforeFilter }
+        @post_filter_modules[type] || Module.new
+      end
 
-        fail ArgumentError, 'Tried to filter for declared parameters but none exist.' unless declared_params
+      # Methods which should not be available in filters until the before filter
+      # has completed
+      module PostBeforeFilter
+        def declared(params, options = {}, declared_params = nil)
+          options = options.reverse_merge(include_missing: true, include_parent_namespaces: true)
 
-        if params.is_a? Array
-          params.map do |param|
-            declared(param || {}, options, declared_params)
-          end
-        else
-          declared_params.each_with_object(Hashie::Mash.new) do |key, hash|
-            key = { key => nil } unless key.is_a? Hash
+          declared_params ||= (!options[:include_parent_namespaces] ? route_setting(:declared_params) : (route_setting(:saved_declared_params) || [])).flatten(1) || []
+
+          fail ArgumentError, 'Tried to filter for declared parameters but none exist.' unless declared_params
+
+          if params.is_a? Array
+            params.map do |param|
+              declared(param || {}, options, declared_params)
+            end
+          else
+            declared_params.each_with_object(Hashie::Mash.new) do |key, hash|
+              key = { key => nil } unless key.is_a? Hash
 
-            key.each_pair do |parent, children|
-              output_key = options[:stringify] ? parent.to_s : parent.to_sym
+              key.each_pair do |parent, children|
+                output_key = options[:stringify] ? parent.to_s : parent.to_sym
 
-              next unless options[:include_missing] || params.key?(parent)
+                next unless options[:include_missing] || params.key?(parent)
 
-              hash[output_key] = if children
-                                   children_params = params[parent] || (children.is_a?(Array) ? [] : {})
-                                   declared(children_params, options, Array(children))
-                                 else
-                                   params[parent]
-                                 end
+                hash[output_key] = if children
+                                     children_params = params[parent] || (children.is_a?(Array) ? [] : {})
+                                     declared(children_params, options, Array(children))
+                                   else
+                                     params[parent]
+                                   end
+              end
             end
           end
         end
       end
 
+      # A filtering method that will return a hash
+      # consisting only of keys that have been declared by a
+      # `params` statement against the current/target endpoint or parent
+      # namespaces.
+      #
+      # @see +PostBeforeFilter#declared+
+      #
+      # @param params [Hash] The initial hash to filter. Usually this will just be `params`
+      # @param options [Hash] Can pass `:include_missing`, `:stringify` and `:include_parent_namespaces`
+      # options. `:include_parent_namespaces` defaults to true, hence must be set to false if
+      # you want only to return params declared against the current/target endpoint.
+      def declared(*)
+        fail MethodNotYetAvailable, '#declared is not available prior to parameter validation.'
+      end
+
       # The API version as specified in the URL.
       def version
         env[Grape::Env::API_VERSION]

lib/grape/endpoint.rb

@@ -317,6 +317,7 @@ def run_filters(filters, type = :other)
           instance_eval(&filter)
         end
       end
+      extend DSL::InsideRoute.post_filter_methods(type)
     end
 
     def befores

spec/grape/dsl/inside_route_spec.rb

@@ -343,8 +343,9 @@ def initialize
       describe '#declared' do
         # see endpoint_spec.rb#declared for spec coverage
 
-        it 'returns an empty hash' do
-          expect(subject.declared({})).to eq({})
+        it 'is not available by default' do
+          expect { subject.declared({}) }.to raise_error(
+            Grape::DSL::InsideRoute::MethodNotYetAvailable)
         end
       end
     end

spec/grape/endpoint_spec.rb

@@ -239,6 +239,16 @@ def app
       end
     end
 
+    it 'does not work in a before filter' do
+      subject.before do
+        declared(params)
+      end
+      subject.get('/declared') { declared(params) }
+
+      expect { get('/declared') }.to raise_error(
+        Grape::DSL::InsideRoute::MethodNotYetAvailable)
+    end
+
     it 'has as many keys as there are declared params' do
       inner_params = nil
       subject.get '/declared' do