From 5278ecab3ff307615c7783894d9cff4988dafc78 Mon Sep 17 00:00:00 2001 From: Tim Hogg Date: Wed, 10 May 2017 16:10:00 -0600 Subject: [PATCH] use correct params class in declared use correct params class in declared add changelog entry add tests for declared class fix rubocop make changelog entry better remove puts in tests one more puts change test names conform changelog entry return dynamic class name parse response instead remove params Instrument validators with ActiveSupport::Notifications Suppress `warning: method redefined` Bugfix: Correctly handle `given` in Array params (#1625) * Bugfix: Correctly handle `given` in Array params Array parameters are handled as a parameter that opens a scope with `type: Array`; `given` opens up a new scope, but was always setting the type to Hash. This patch fixes that, as well as correctly labeling the error messages associated with array fields. * Code review fix * Add CHANGELOG entry Add ability to include an array of modules as helpers Changing helpers DSL to allow the inclusion of many modules. This attemps to bring a better readability, since it seems to be more intuitive to send a list of modules when the message in question is called helpers. ensure we return a string from test return json in tests Update README according to new `helpers` macro interface Silence warnings (#1632) * silence warnings initialize vars in initializers subclass hashie mash to silence warnings rubocop fixes add changelog entry Revert "use correct params class in declared" This reverts commit 61f0c8ef9f470b9a2b520437e6635bfc409298cf. fix tests * remove disable_warnings in hashie mash * make rubocop happy * fix hashie tests --- .rubocop_todo.yml | 21 ++++----- CHANGELOG.md | 5 +++ README.md | 24 ++++++++-- lib/grape/dsl/helpers.rb | 50 +++++++++++++-------- lib/grape/dsl/inside_route.rb | 4 +- lib/grape/dsl/routing.rb | 2 +- lib/grape/dsl/settings.rb | 2 +- lib/grape/endpoint.rb | 25 +++++++---- lib/grape/extensions/hashie/mash.rb | 1 - lib/grape/validations/params_scope.rb | 34 ++++++++------ lib/grape/validations/validators/base.rb | 1 + lib/grape/validations/validators/values.rb | 3 ++ spec/grape/dsl/helpers_spec.rb | 31 ++++++++++--- spec/grape/endpoint_spec.rb | 47 +++++++++++++++++++ spec/grape/validations/params_scope_spec.rb | 23 ++++++++++ 15 files changed, 206 insertions(+), 67 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 1d7f66c286..83021bf3dd 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,35 +1,30 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2017-02-19 15:40:46 -0500 using RuboCop version 0.47.1. +# on 2017-06-12 13:25:24 -0600 using RuboCop version 0.47.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: 43 +# Offense count: 44 Metrics/AbcSize: Max: 44 -# Offense count: 265 +# Offense count: 277 # Configuration parameters: CountComments, ExcludedMethods. Metrics/BlockLength: Max: 3104 -# Offense count: 1 -# Configuration parameters: CountBlocks. -Metrics/BlockNesting: - Max: 4 - # Offense count: 8 # Configuration parameters: CountComments. Metrics/ClassLength: - Max: 281 + Max: 287 -# Offense count: 26 +# Offense count: 28 Metrics/CyclomaticComplexity: Max: 14 -# Offense count: 993 +# Offense count: 1098 # Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns. # URISchemes: http, https Metrics/LineLength: @@ -40,12 +35,12 @@ Metrics/LineLength: Metrics/MethodLength: Max: 33 -# Offense count: 9 +# Offense count: 10 # Configuration parameters: CountComments. Metrics/ModuleLength: Max: 212 -# Offense count: 16 +# Offense count: 17 Metrics/PerceivedComplexity: Max: 14 diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dccef1db0..7f6ec47e5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,16 @@ * [#1594](https://github.com/ruby-grape/grape/pull/1594): Replace `Hashie::Mash` parameters with `ActiveSupport::HashWithIndifferentAccess` - [@james2m](https://github.com/james2m), [@dblock](https://github.com/dblock). * [#1622](https://github.com/ruby-grape/grape/pull/1622): Add `except_values` validator to replace `except` option of `values` validator - [@jlfaber](https://github.com/jlfaber). +* [#1635](https://github.com/ruby-grape/grape/pull/1635): Instrument validators with ActiveSupport::Notifications - [@ktimothy](https://github.com/ktimothy). +* [#1646](https://github.com/ruby-grape/grape/pull/1646): Add ability to include an array of modules as helpers - [@pablonahuelgomez](https://github.com/pablonahuelgomez). * Your contribution here. #### Fixes +* [#1631](https://github.com/ruby-grape/grape/pull/1631): Declared now returns declared options using the class that params is set to use - [@thogg4](https://github.com/thogg4). +* [#1632](https://github.com/ruby-grape/grape/pull/1632): Silence warnings - [@thogg4](https://github.com/thogg4). * [#1615](https://github.com/ruby-grape/grape/pull/1615): Fix default and type validator when values is a Hash with no value attribute - [@jlfaber](https://github.com/jlfaber). +* [#1625](https://github.com/ruby-grape/grape/pull/1625): Handle `given` correctly when nested in Array params - [@rnubel](https://github.com/rnubel), [@avellable](https://github.com/avellable). * Your contribution here. ### 0.19.2 (4/12/2017) diff --git a/README.md b/README.md index c7dda05922..a3cc7bfc41 100644 --- a/README.md +++ b/README.md @@ -1643,7 +1643,7 @@ end ## Helpers You can define helper methods that your endpoints can use with the `helpers` -macro by either giving a block or a module. +macro by either giving a block or an array of modules. ```ruby module StatusHelpers @@ -1652,6 +1652,12 @@ module StatusHelpers end end +module HttpCodesHelpers + def unauthorized + 401 + end +end + class API < Grape::API # define helpers with a block helpers do @@ -1660,8 +1666,12 @@ class API < Grape::API end end - # or mix in a module - helpers StatusHelpers + # or mix in an array of modules + helpers StatusHelpers, HttpCodesHelpers + + before do + error!('Access Denied', unauthorized) unless current_user + end get 'info' do # helpers available in your endpoint and filters @@ -3367,6 +3377,14 @@ The execution of the main content block of the endpoint. * *filters* - The filters being executed * *type* - The type of filters (before, before_validation, after_validation, after) +#### endpoint_run_validators.grape + +The execution of validators. + +* *endpoint* - The endpoint instance +* *validators* - The validators being executed +* *request* - The request being validated + See the [ActiveSupport::Notifications documentation](http://api.rubyonrails.org/classes/ActiveSupport/Notifications.html) for information on how to subscribe to these events. ### Monitoring Products diff --git a/lib/grape/dsl/helpers.rb b/lib/grape/dsl/helpers.rb index 9d2200a7cd..d58e2cf020 100644 --- a/lib/grape/dsl/helpers.rb +++ b/lib/grape/dsl/helpers.rb @@ -13,7 +13,7 @@ module ClassMethods # When called without a block, all known helpers within this scope # are included. # - # @param [Module] new_mod optional module of methods to include + # @param [Array] new_modules optional array of modules to include # @param [Block] block optional block of methods to include # # @example Define some helpers. @@ -26,28 +26,42 @@ module ClassMethods # end # end # - def helpers(new_mod = nil, &block) - if block_given? || new_mod - mod = new_mod || Module.new - define_boolean_in_mod(mod) - inject_api_helpers_to_mod(mod) if new_mod + # @example Include many modules + # + # class ExampleAPI < Grape::API + # helpers Authentication, Mailer, OtherModule + # end + # + def helpers(*new_modules, &block) + include_new_modules(new_modules) if new_modules.any? + include_block(block) if block_given? + include_all_in_scope if !block_given? && new_modules.empty? + end - inject_api_helpers_to_mod(mod) do - mod.class_eval(&block) - end if block_given? + protected - namespace_stackable(:helpers, mod) - else - mod = Module.new - namespace_stackable(:helpers).each do |mod_to_include| - mod.send :include, mod_to_include - end - change! - mod + def include_new_modules(modules) + modules.each { |mod| make_inclusion(mod) } + end + + def include_block(block) + Module.new.tap do |mod| + make_inclusion(mod) { mod.class_eval(&block) } end end - protected + def make_inclusion(mod, &block) + define_boolean_in_mod(mod) + inject_api_helpers_to_mod(mod, &block) + namespace_stackable(:helpers, mod) + end + + def include_all_in_scope + Module.new.tap do |mod| + namespace_stackable(:helpers).each { |mod_to_include| mod.send :include, mod_to_include } + change! + end + end def define_boolean_in_mod(mod) return if defined? mod::Boolean diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 281ff834ec..d060b3e10f 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -46,7 +46,7 @@ def declared_array(passed_params, options, declared_params) end def declared_hash(passed_params, options, declared_params) - declared_params.each_with_object({}) do |declared_param, memo| + declared_params.each_with_object(passed_params.class.new) do |declared_param, memo| # If it is not a Hash then it does not have children. # Find its value or set it to nil. if !declared_param.is_a?(Hash) @@ -56,7 +56,7 @@ def declared_hash(passed_params, options, declared_params) declared_param.each_pair do |declared_parent_param, declared_children_params| next unless options[:include_missing] || passed_params.key?(declared_parent_param) - passed_children_params = passed_params[declared_parent_param] || {} + passed_children_params = passed_params[declared_parent_param] || passed_params.class.new memo[optioned_param_key(declared_parent_param, options)] = declared(passed_children_params, options, declared_children_params) end end diff --git a/lib/grape/dsl/routing.rb b/lib/grape/dsl/routing.rb index 24b9d5f3a5..2f3236d7de 100644 --- a/lib/grape/dsl/routing.rb +++ b/lib/grape/dsl/routing.rb @@ -7,7 +7,7 @@ module Routing include Grape::DSL::Configuration module ClassMethods - attr_reader :endpoints, :routes + attr_reader :endpoints # Specify an API version. # diff --git a/lib/grape/dsl/settings.rb b/lib/grape/dsl/settings.rb index f83d61645f..8d94cff555 100644 --- a/lib/grape/dsl/settings.rb +++ b/lib/grape/dsl/settings.rb @@ -9,7 +9,7 @@ module DSL module Settings extend ActiveSupport::Concern - attr_accessor :inheritable_setting, :top_level_setting + attr_writer :inheritable_setting, :top_level_setting # Fetch our top-level settings, which apply to all endpoints in the API. def top_level_setting diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 3ff2d6c380..6e5df43785 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -96,6 +96,11 @@ def initialize(new_settings, options = {}, &block) @lazy_initialized = nil @block = nil + @status = nil + @file = nil + @body = nil + @proc = nil + return unless block_given? @source = block @@ -336,15 +341,17 @@ def lazy_initialize! def run_validators(validators, request) validation_errors = [] - validators.each do |validator| - begin - validator.validate(request) - rescue Grape::Exceptions::Validation => e - validation_errors << e - break if validator.fail_fast? - rescue Grape::Exceptions::ValidationArrayErrors => e - validation_errors += e.errors - break if validator.fail_fast? + ActiveSupport::Notifications.instrument('endpoint_run_validators.grape', endpoint: self, validators: validators, request: request) do + validators.each do |validator| + begin + validator.validate(request) + rescue Grape::Exceptions::Validation => e + validation_errors << e + break if validator.fail_fast? + rescue Grape::Exceptions::ValidationArrayErrors => e + validation_errors += e.errors + break if validator.fail_fast? + end end end diff --git a/lib/grape/extensions/hashie/mash.rb b/lib/grape/extensions/hashie/mash.rb index 50958f3224..6afa106d95 100644 --- a/lib/grape/extensions/hashie/mash.rb +++ b/lib/grape/extensions/hashie/mash.rb @@ -4,7 +4,6 @@ module Hashie module Mash module ParamBuilder extend ::ActiveSupport::Concern - included do namespace_inheritable(:build_params_with, Grape::Extensions::Hashie::Mash::ParamBuilder) end diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index c57ca5f142..c4c109688a 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -42,37 +42,45 @@ def should_validate?(parameters) return false if @optional && (params(parameters).blank? || any_element_blank?(parameters)) + return true if parent.nil? + parent.should_validate?(parameters) + end + + def meets_dependency?(params) + return true unless @dependent_on + + params = params.with_indifferent_access + @dependent_on.each do |dependency| if dependency.is_a?(Hash) dependency_key = dependency.keys[0] proc = dependency.values[0] - return false unless proc.call(params(parameters).try(:[], dependency_key)) - elsif params(parameters).try(:[], dependency).blank? + return false unless proc.call(params.try(:[], dependency_key)) + elsif params.respond_to?(:key?) && params.try(:[], dependency).blank? return false end - end if @dependent_on + end - return true if parent.nil? - parent.should_validate?(parameters) + true end # @return [String] the proper attribute name, with nesting considered. - def full_name(name) + def full_name(name, index: nil) if nested? # Find our containing element's name, and append ours. - "#{@parent.full_name(@element)}#{array_index}[#{name}]" + [@parent.full_name(@element), [@index || index, name].map(&method(:brackets))].compact.join elsif lateral? - # Find the name of the element as if it was at the - # same nesting level as our parent. - @parent.full_name(name) + # Find the name of the element as if it was at the same nesting level + # as our parent. We need to forward our index upward to achieve this. + @parent.full_name(name, index: @index) else # We must be the root scope, so no prefix needed. name.to_s end end - def array_index - "[#{@index}]" if @index.present? + def brackets(val) + "[#{val}]" if val end # @return [Boolean] whether or not this scope is the root-level scope @@ -187,7 +195,7 @@ def new_lateral_scope(options, &block) element: nil, parent: self, options: @optional, - type: Hash, + type: type == Array ? Array : Hash, dependent_on: options[:dependent_on], &block ) diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index 7b3a2e9655..ad3b440fa7 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -41,6 +41,7 @@ def validate!(params) array_errors = [] attributes.each do |resource_params, attr_name| next unless @required || (resource_params.respond_to?(:key?) && resource_params.key?(attr_name)) + next unless @scope.meets_dependency?(resource_params) begin validate_param!(attr_name, resource_params) diff --git a/lib/grape/validations/validators/values.rb b/lib/grape/validations/validators/values.rb index 2204692252..776dc314ad 100644 --- a/lib/grape/validations/validators/values.rb +++ b/lib/grape/validations/validators/values.rb @@ -14,6 +14,9 @@ def initialize(attrs, options, required, scope, opts = {}) warn '[DEPRECATION] The values validator proc option is deprecated. ' \ 'The lambda expression can now be assigned directly to values.' if @proc else + @excepts = nil + @values = nil + @proc = nil @values = options end super diff --git a/spec/grape/dsl/helpers_spec.rb b/spec/grape/dsl/helpers_spec.rb index 8611140882..ba4e48ec4c 100644 --- a/spec/grape/dsl/helpers_spec.rb +++ b/spec/grape/dsl/helpers_spec.rb @@ -6,8 +6,12 @@ module HelpersSpec class Dummy include Grape::DSL::Helpers - def self.mod - namespace_stackable(:helpers).first + def self.mods + namespace_stackable(:helpers) + end + + def self.first_mod + mods.first end end end @@ -36,23 +40,38 @@ def test expect(subject).to receive(:namespace_stackable).with(:helpers).and_call_original subject.helpers(&proc) - expect(subject.mod.instance_methods).to include(:test) + expect(subject.first_mod.instance_methods).to include(:test) end it 'uses provided modules' do mod = Module.new - expect(subject).to receive(:namespace_stackable).with(:helpers, kind_of(Grape::DSL::Helpers::BaseHelper)).and_call_original + expect(subject).to receive(:namespace_stackable).with(:helpers, kind_of(Grape::DSL::Helpers::BaseHelper)).and_call_original.exactly(2).times expect(subject).to receive(:namespace_stackable).with(:helpers).and_call_original subject.helpers(mod, &proc) - expect(subject.mod).to eq mod + expect(subject.first_mod).to eq mod + end + + it 'uses many provided modules' do + mod = Module.new + mod2 = Module.new + mod3 = Module.new + + expect(subject).to receive(:namespace_stackable).with(:helpers, kind_of(Grape::DSL::Helpers::BaseHelper)).and_call_original.exactly(4).times + expect(subject).to receive(:namespace_stackable).with(:helpers).and_call_original.exactly(3).times + + subject.helpers(mod, mod2, mod3, &proc) + + expect(subject.mods).to include(mod) + expect(subject.mods).to include(mod2) + expect(subject.mods).to include(mod3) end context 'with an external file' do it 'sets Boolean as a Virtus::Attribute::Boolean' do subject.helpers BooleanParam - expect(subject.mod::Boolean).to eq Virtus::Attribute::Boolean + expect(subject.first_mod::Boolean).to eq Virtus::Attribute::Boolean end end end diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 1af9b73b94..8d678b6a9f 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -298,6 +298,47 @@ def app end end + context 'when params are not built with default class' do + it 'returns an object that corresponds with the params class - hash with indifferent access' do + subject.params do + build_with Grape::Extensions::ActiveSupport::HashWithIndifferentAccess::ParamBuilder + end + subject.get '/declared' do + d = declared(params, include_missing: true) + { declared_class: d.class.to_s } + end + + get '/declared?first=present' + expect(JSON.parse(last_response.body)['declared_class']).to eq('ActiveSupport::HashWithIndifferentAccess') + end + + it 'returns an object that corresponds with the params class - hashie mash' do + subject.params do + build_with Grape::Extensions::Hashie::Mash::ParamBuilder + end + subject.get '/declared' do + d = declared(params, include_missing: true) + { declared_class: d.class.to_s } + end + + get '/declared?first=present' + expect(JSON.parse(last_response.body)['declared_class']).to eq('Hashie::Mash') + end + + it 'returns an object that corresponds with the params class - hash' do + subject.params do + build_with Grape::Extensions::Hash::ParamBuilder + end + subject.get '/declared' do + d = declared(params, include_missing: true) + { declared_class: d.class.to_s } + end + + get '/declared?first=present' + expect(JSON.parse(last_response.body)['declared_class']).to eq('Hash') + end + end + it 'should show nil for nested params if include_missing is true' do subject.get '/declared' do declared(params, include_missing: true) @@ -1403,6 +1444,9 @@ def memoized have_attributes(name: 'endpoint_run_filters.grape', payload: { endpoint: a_kind_of(Grape::Endpoint), filters: [], type: :before_validation }), + have_attributes(name: 'endpoint_run_validators.grape', payload: { endpoint: a_kind_of(Grape::Endpoint), + validators: [], + request: a_kind_of(Grape::Request) }), have_attributes(name: 'endpoint_run_filters.grape', payload: { endpoint: a_kind_of(Grape::Endpoint), filters: [], type: :after_validation }), @@ -1424,6 +1468,9 @@ def memoized have_attributes(name: 'endpoint_run_filters.grape', payload: { endpoint: a_kind_of(Grape::Endpoint), filters: [], type: :before_validation }), + have_attributes(name: 'endpoint_run_validators.grape', payload: { endpoint: a_kind_of(Grape::Endpoint), + validators: [], + request: a_kind_of(Grape::Request) }), have_attributes(name: 'endpoint_run_filters.grape', payload: { endpoint: a_kind_of(Grape::Endpoint), filters: [], type: :after_validation }), diff --git a/spec/grape/validations/params_scope_spec.rb b/spec/grape/validations/params_scope_spec.rb index 5b1c4e41a5..7b659e5cee 100644 --- a/spec/grape/validations/params_scope_spec.rb +++ b/spec/grape/validations/params_scope_spec.rb @@ -473,6 +473,29 @@ def initialize(value) end end + context 'when validations are dependent on a parameter within an array param' do + before do + subject.params do + requires :foos, type: Array do + optional :foo_type, :baz_type + given :foo_type do + requires :bar + end + end + end + subject.post('/test') { declared(params).to_json } + end + + it 'applies the constraint within each value' do + post '/test', + { foos: [{ foo_type: 'a' }, { baz_type: 'c' }] }.to_json, + 'CONTENT_TYPE' => 'application/json' + + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('foos[0][bar] is missing') + end + end + context 'when validations are dependent on a parameter with specific value' do # build test cases from all combinations of declarations and options a_decls = %i(optional requires)