Skip to content

Commit

Permalink
Fix validation error for nested array when requires => optional =…
Browse files Browse the repository at this point in the history
…> `requires`

This bug is due to the params parsing code return `{}` when the value is not found.
This is fine in most cases, however in the instance that the value was optional
and has nested required values.

I was not able to find a way to confind the changes to just the parameter parsing
as this resulted in the indexing being incorrect if any of the other array objects
had an error.

I have used a class to indicate that an Optional Value was missing as this avoid issues
with the value actually being in the response.
  • Loading branch information
dwhenry committed Nov 4, 2020
1 parent 338663d commit 1714f21
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#### Fixes

* Your contribution here.
* : Fix validation error when Required Array nested inside an optional array - [@dwhenry](https://github/dwhenry).
* [#2127](https://github.com/ruby-grape/grape/pull/2127): Fix a performance issue with dependent params - [@dnesteryuk](https://github.com/dnesteryuk).
* [#2126](https://github.com/ruby-grape/grape/pull/2126): Fix warnings about redefined attribute accessors in `AttributeTranslator` - [@samsonjs](https://github.com/samsonjs).
* [#2121](https://github.com/ruby-grape/grape/pull/2121): Fix 2.7 deprecation warning in validator_factory - [@Legogris](https://github.com/Legogris).
Expand Down
10 changes: 7 additions & 3 deletions lib/grape/dsl/parameters.rb
Original file line number Diff line number Diff line change
Expand Up @@ -227,13 +227,17 @@ def declared_param?(param)

alias group requires

def map_params(params, element)
class EmptyOptionalValue; end

def map_params(params, element, is_array = false)
if params.is_a?(Array)
params.map do |el|
map_params(el, element)
map_params(el, element, true)
end
elsif params.is_a?(Hash)
params[element] || {}
params[element] || (@optional && is_array ? EmptyOptionalValue : {})
elsif params == EmptyOptionalValue
EmptyOptionalValue
else
{}
end
Expand Down
12 changes: 11 additions & 1 deletion lib/grape/validations/single_attribute_iterator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,20 @@ class SingleAttributeIterator < AttributesIterator

def yield_attributes(val, attrs)
attrs.each do |attr_name|
yield val, attr_name, empty?(val)
yield val, attr_name, empty?(val), skip?(val)
end
end


# This is a special case so that we can ignore tree's where option
# values are missing lower down. Unfortunately we can remove this
# are the parameter parsing stage as they are required to ensure
# the correct indexing is maintained
def skip?(val)
# return false
val == Grape::DSL::Parameters::EmptyOptionalValue
end

# Primitives like Integers and Booleans don't respond to +empty?+.
# It could be possible to use +blank?+ instead, but
#
Expand Down
3 changes: 2 additions & 1 deletion lib/grape/validations/validators/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ def validate!(params)
# there may be more than one error per field
array_errors = []

attributes.each do |val, attr_name, empty_val|
attributes.each do |val, attr_name, empty_val, skip_value|
next if skip_value
next if !@scope.required? && empty_val
next unless @scope.meets_dependency?(val, params)
begin
Expand Down
28 changes: 28 additions & 0 deletions spec/grape/validations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -883,6 +883,34 @@ def validate_param!(attr_name, params)
end
expect(declared_params).to eq([items: [:key, { optional_subitems: [:value] }, { required_subitems: [:value] }]])
end

it "does not report errors when required array inside missing optional array" do
subject.params do
requires :orders, type: Array do
requires :id, type: Integer
optional :drugs, type: Array do
requires :batches, type: Array do
requires :batch_no, type: String
end
end
end
end

subject.get '/validate_required_arrays_under_optional_arrays' do
'validate_required_arrays_under_optional_arrays works!'
end

data = {
orders: [
{ id: 77, drugs: [{batches: [{batch_no: "A1234567"}]}]},
{ id: 70 }
]
}

get '/validate_required_arrays_under_optional_arrays', data
expect(last_response.body).to eq("validate_required_arrays_under_optional_arrays works!")
expect(last_response.status).to eq(200)
end
end

context 'multiple validation errors' do
Expand Down

0 comments on commit 1714f21

Please sign in to comment.