From 31b071a7f10493e6421d742e9112afa5c19cf2ef Mon Sep 17 00:00:00 2001 From: Jack Forrest Date: Fri, 28 Aug 2015 14:13:05 -0400 Subject: [PATCH] Makes #declared unavailable to before filters In response to issue #1074 This adds a mechanism by which Grape::DSL::InsideRoute methods may be overriden after certain filters are run. The first problem case of a filter being utilized before it should that necessitated this enhancement is the `#declared` method, which was returning un-coerced params in `before` filters. There may be other problem cases, which may be rectified using this same mechanism. --- README.md | 4 ++ lib/grape/dsl/inside_route.rb | 85 +++++++++++++++++++---------- lib/grape/endpoint.rb | 1 + spec/grape/dsl/inside_route_spec.rb | 5 +- spec/grape/endpoint_spec.rb | 10 ++++ 5 files changed, 74 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index c4c5f4e4e3..f0a0e9a33c 100644 --- a/README.md +++ b/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"] ``` + +`#declared` 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: diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 8e314b8de2..3ec9dd3aa5 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -6,46 +6,73 @@ 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) || [] - key.each_pair do |parent, children| - output_key = options[:stringify] ? parent.to_s : parent.to_sym + 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 - next unless options[:include_missing] || params.key?(parent) + key.each_pair do |parent, children| + output_key = options[:stringify] ? parent.to_s : parent.to_sym - hash[output_key] = if children - children_params = params[parent] || (children.is_a?(Array) ? [] : {}) - declared(children_params, options, Array(children)) - else - params[parent] - end + 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 + 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(*params) + raise 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] diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 63d25572ed..0291b27e17 100644 --- a/lib/grape/endpoint.rb +++ b/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 diff --git a/spec/grape/dsl/inside_route_spec.rb b/spec/grape/dsl/inside_route_spec.rb index efb164343b..b7b995d569 100644 --- a/spec/grape/dsl/inside_route_spec.rb +++ b/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 diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index b0da9aa978..f1b18678ea 100644 --- a/spec/grape/endpoint_spec.rb +++ b/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