Skip to content

Commit

Permalink
[Bugfix] Handle deeply-nested dependent params (ruby-grape#1661)
Browse files Browse the repository at this point in the history
* Repro issue ruby-grape#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.
  • Loading branch information
rnubel authored and jdurand committed Jan 25, 2019
1 parent 5e09f74 commit 1b0fc4f
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 3 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

#### Fixes

* [#1661](https://github.com/ruby-grape/grape/pull/1661): Handle deeply-nested dependencies correctly - [@rnubel](https://github.com/rnubel), [@jnardone](https://github.com/jnardone).
* [#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)

Expand Down
6 changes: 5 additions & 1 deletion lib/grape/validations/params_scope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ def should_validate?(parameters)
parent.should_validate?(parameters)
end

def meets_dependency?(params)
def meets_dependency?(params, request_params)
if @parent.present? && !@parent.meets_dependency?(@parent.params(request_params), request_params)
return false
end

return true unless @dependent_on

params = params.with_indifferent_access
Expand Down
2 changes: 1 addition & 1 deletion lib/grape/validations/validators/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +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)
next unless @scope.meets_dependency?(resource_params, params)

begin
validate_param!(attr_name, resource_params)
Expand Down
40 changes: 40 additions & 0 deletions spec/grape/validations/params_scope_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,46 @@ def initialize(value)
end.to raise_error(Grape::Exceptions::UnknownParameter)
end

it 'does not validate nested requires when given is false' do
subject.params do
requires :a, type: String, allow_blank: false, values: %w(x y z)
given a: ->(val) { val == 'x' } do
requires :inner1, type: Hash, allow_blank: false do
requires :foo, type: Integer, allow_blank: false
end
end
given a: ->(val) { val == 'y' } do
requires :inner2, type: Hash, allow_blank: false do
requires :bar, type: Integer, allow_blank: false
requires :baz, type: Array, allow_blank: false do
requires :baz_category, type: String, allow_blank: false
end
end
end
given a: ->(val) { val == 'z' } do
requires :inner3, type: Array, allow_blank: false do
requires :bar, type: Integer, allow_blank: false
requires :baz, type: Array, allow_blank: false do
requires :baz_category, type: String, allow_blank: false
end
end
end
end
subject.get('/varying') { declared(params).to_json }

get '/varying', a: 'x', inner1: { foo: 1 }
expect(last_response.status).to eq(200)

get '/varying', a: 'y', inner2: { bar: 2, baz: [{ baz_category: 'barstools' }] }
expect(last_response.status).to eq(200)

get '/varying', a: 'y', inner2: { bar: 2, baz: [{ unrelated: 'yep' }] }
expect(last_response.status).to eq(400)

get '/varying', a: 'z', inner3: [{ bar: 3, baz: [{ baz_category: 'barstools' }] }]
expect(last_response.status).to eq(200)
end

it 'includes the parameter within #declared(params)' do
get '/test', a: true, b: true

Expand Down

0 comments on commit 1b0fc4f

Please sign in to comment.