Fix validations nested under a given

Multi-param and default validators, all of which implemented their
own `validate!` method, were not respecting the `given` dependency
as expected. This change copies the check into all those validators.

The bug was likely introduced by ruby-grape#1625, which required that the
dependency check be moved out of the scope itself and into the
validators. Rolling back that change would require fundamentally
re-architecting how array parameters are validated (possibly with
a new kind of scope).

CHANGELOG.md

@@ -6,7 +6,7 @@
 
 #### Fixes
 
-* Your contribution here.
+* [#1691](https://github.com/ruby-grape/grape/pull/1691): Fix validations nested under `given` - [@rnubel](https://github.com/rnubel).
 
 ### 1.0.1 (9/8/2017)
 

lib/grape/validations/validators/all_or_none.rb

@@ -4,16 +4,20 @@ module Validations
  class AllOrNoneOfValidator < MultipleParamsBase
  def validate!(params)
  super
- if scope_requires_params && only_subset_present
+ if scope_requires_params && only_subset_present(params)
  raise Grape::Exceptions::Validation, params: all_keys, message: message(:all_or_none)
  end
  params
  end
 
  private
 
- def only_subset_present
- scoped_params.any? { |resource_params| !keys_in_common(resource_params).empty? && keys_in_common(resource_params).length < attrs.length }
+ def only_subset_present(params)
+ scoped_params.any? do |resource_params|
+ return unless scope_should_validate?(resource_params, params)
+
+ !keys_in_common(resource_params).empty? && keys_in_common(resource_params).length < attrs.length
+ end
  end
  end
  end

lib/grape/validations/validators/at_least_one_of.rb

@@ -4,16 +4,20 @@ module Validations
  class AtLeastOneOfValidator < MultipleParamsBase
  def validate!(params)
  super
- if scope_requires_params && no_exclusive_params_are_present
+ if scope_requires_params && no_exclusive_params_are_present(params)
  raise Grape::Exceptions::Validation, params: all_keys, message: message(:at_least_one)
  end
  params
  end
 
  private
 
- def no_exclusive_params_are_present
- scoped_params.any? { |resource_params| keys_in_common(resource_params).empty? }
+ def no_exclusive_params_are_present(params)
+ scoped_params.any? do |resource_params|
+ next unless scope_should_validate?(resource_params, params)
+
+ keys_in_common(resource_params).empty?
+ end
  end
  end
  end

lib/grape/validations/validators/default.rb

@@ -20,6 +20,8 @@ def validate_param!(attr_name, params)
  def validate!(params)
  attrs = AttributesIterator.new(self, @scope, params)
  attrs.each do |resource_params, attr_name|
+ next unless @scope.meets_dependency?(resource_params, params)
+
  if resource_params.is_a?(Hash) && resource_params[attr_name].nil?
  validate_param!(attr_name, resource_params)
  end

lib/grape/validations/validators/exactly_one_of.rb

@@ -4,7 +4,7 @@ module Validations
  class ExactlyOneOfValidator < MutualExclusionValidator
  def validate!(params)
  super
- if scope_requires_params && none_of_restricted_params_is_present
+ if scope_requires_params && none_of_restricted_params_is_present(params)
  raise Grape::Exceptions::Validation, params: all_keys, message: message(:exactly_one)
  end
  params
@@ -21,8 +21,12 @@ def message(default_key = nil)
 
  private
 
- def none_of_restricted_params_is_present
- scoped_params.any? { |resource_params| keys_in_common(resource_params).empty? }
+ def none_of_restricted_params_is_present(params)
+ scoped_params.any? do |resource_params|
+ next unless scope_should_validate?(resource_params, params)
+
+ keys_in_common(resource_params).empty?
+ end
  end
  end
  end

lib/grape/validations/validators/multiple_params_base.rb

@@ -10,6 +10,10 @@ def validate!(params)
 
  private
 
+ def scope_should_validate?(scoped_params, params)
+ @scope.meets_dependency?(scoped_params, params)
+ end
+
  def scope_requires_params
  @scope.required? || scoped_params.any?(&:any?)
  end

lib/grape/validations/validators/mutual_exclusion.rb

@@ -6,16 +6,18 @@ class MutualExclusionValidator < MultipleParamsBase
 
  def validate!(params)
  super
- if two_or_more_exclusive_params_are_present
+ if two_or_more_exclusive_params_are_present(params)
  raise Grape::Exceptions::Validation, params: processing_keys_in_common, message: message(:mutual_exclusion)
  end
  params
  end
 
  private
 
- def two_or_more_exclusive_params_are_present
+ def two_or_more_exclusive_params_are_present(params)
  scoped_params.any? do |resource_params|
+ next unless scope_should_validate?(resource_params, params)
+
  @processing_keys_in_common = keys_in_common(resource_params)
  @processing_keys_in_common.length > 1
  end

spec/grape/validations/params_scope_spec.rb

@@ -407,6 +407,60 @@ def initialize(value)
  expect(last_response.status).to eq(200)
  end
 
+ it 'sets default values only if the dependency is met' do
+ subject.params do
+ optional :a
+ given :a do
+ optional :b, default: 'default value'
+ end
+ end
+ subject.get('/defaults') { params.to_json }
+
+ get '/defaults'
+ expect(last_response.status).to eq(200)
+ expect(last_response.body).to eq('{}')
+
+ get '/defaults', a: true
+ expect(last_response.status).to eq(200)
+ expect(JSON.parse(last_response.body)).to eq({ "a" => "true", "b" => "default value" })
+ end
+
+ it 'conditionally applies all multi-param validators' do
+ subject.params do
+ optional :a
+ given :a do
+ optional :b, :c, :d, :e, :f, :g
+
+ exactly_one_of :b, :c
+ all_or_none_of :d, :e
+ at_least_one_of :f, :g
+ end
+ end
+ subject.get('/exactly_one') { params.to_json }
+
+ get '/exactly_one'
+ expect(last_response.status).to eq(200)
+ expect(last_response.body).to eq('{}')
+
+ get '/exactly_one', a: true, b: true, f: true
+ expect(last_response.status).to eq(200)
+
+ get '/exactly_one', a: true, c: true, f: true
+ expect(last_response.status).to eq(200)
+
+ get '/exactly_one', a: true, b: true, c: true, f: true
+ expect(last_response.status).to eq(400)
+
+ get '/exactly_one', d: true
+ expect(last_response.status).to eq(200)
+
+ get '/exactly_one', a: true, b: true, d: true, e: true, f: true
+ expect(last_response.status).to eq(200)
+
+ get '/exactly_one', a: true, b: true, f: true, g: true
+ expect(last_response.status).to eq(200)
+ end
+
  it 'applies only the appropriate validation' do
  subject.params do
  optional :a