Skip to content

Commit a284f24

Browse files
rnubeldblock
authored andcommitted
[Bugfix] Handle deeply-nested dependent params (#1661)
* Repro issue #1659 * [Fix] Handle deeply-nested dependencies with `given`. Behind the scenes, each call to `requires` or other params DSL method pushes an entry onto a flat list of validators. The nesting structure that your parameters can take on is tracked as an up-tree separately on each scope, but that relationship isn't used to traverse the validations. So, when I moved the dependency checking out of `should_validate?` and into the actual validation, the `given` dependency stopped taking effect after you nested parameters more than one level deep underneath. To restore the behavior, I made the validation check recursively upwards to see if it should or should not validate that scope. * Add changelog entry.
1 parent 36573be commit a284f24

File tree

4 files changed

+47
-2
lines changed

4 files changed

+47
-2
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#### Fixes
99

1010
* [#1652](https://github.com/ruby-grape/grape/pull/1652): Fix missing backtrace that was not being bubbled up to the `error_formatter` - [@dcsg](https://github.com/dcsg).
11+
* [#1661](https://github.com/ruby-grape/grape/pull/1661): Handle deeply-nested dependencies correctly - [@rnubel](https://github.com/rnubel), [@jnardone](https://github.com/jnardone).
1112

1213
### 1.0.0 (7/3/2017)
1314

lib/grape/validations/params_scope.rb

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,11 @@ def should_validate?(parameters)
4646
parent.should_validate?(parameters)
4747
end
4848

49-
def meets_dependency?(params)
49+
def meets_dependency?(params, request_params)
50+
if @parent.present? && !@parent.meets_dependency?(@parent.params(request_params), request_params)
51+
return false
52+
end
53+
5054
return true unless @dependent_on
5155

5256
params = params.with_indifferent_access

lib/grape/validations/validators/base.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ def validate!(params)
3939
array_errors = []
4040
attributes.each do |resource_params, attr_name|
4141
next unless @required || (resource_params.respond_to?(:key?) && resource_params.key?(attr_name))
42-
next unless @scope.meets_dependency?(resource_params)
42+
next unless @scope.meets_dependency?(resource_params, params)
4343

4444
begin
4545
validate_param!(attr_name, resource_params)

spec/grape/validations/params_scope_spec.rb

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,46 @@ def initialize(value)
418418
end.to raise_error(Grape::Exceptions::UnknownParameter)
419419
end
420420

421+
it 'does not validate nested requires when given is false' do
422+
subject.params do
423+
requires :a, type: String, allow_blank: false, values: %w(x y z)
424+
given a: ->(val) { val == 'x' } do
425+
requires :inner1, type: Hash, allow_blank: false do
426+
requires :foo, type: Integer, allow_blank: false
427+
end
428+
end
429+
given a: ->(val) { val == 'y' } do
430+
requires :inner2, type: Hash, allow_blank: false do
431+
requires :bar, type: Integer, allow_blank: false
432+
requires :baz, type: Array, allow_blank: false do
433+
requires :baz_category, type: String, allow_blank: false
434+
end
435+
end
436+
end
437+
given a: ->(val) { val == 'z' } do
438+
requires :inner3, type: Array, allow_blank: false do
439+
requires :bar, type: Integer, allow_blank: false
440+
requires :baz, type: Array, allow_blank: false do
441+
requires :baz_category, type: String, allow_blank: false
442+
end
443+
end
444+
end
445+
end
446+
subject.get('/varying') { declared(params).to_json }
447+
448+
get '/varying', a: 'x', inner1: { foo: 1 }
449+
expect(last_response.status).to eq(200)
450+
451+
get '/varying', a: 'y', inner2: { bar: 2, baz: [{ baz_category: 'barstools' }] }
452+
expect(last_response.status).to eq(200)
453+
454+
get '/varying', a: 'y', inner2: { bar: 2, baz: [{ unrelated: 'yep' }] }
455+
expect(last_response.status).to eq(400)
456+
457+
get '/varying', a: 'z', inner3: [{ bar: 3, baz: [{ baz_category: 'barstools' }] }]
458+
expect(last_response.status).to eq(200)
459+
end
460+
421461
it 'includes the parameter within #declared(params)' do
422462
get '/test', a: true, b: true
423463

0 commit comments

Comments
 (0)