Skip to content

Commit

Permalink
careful check for empty params
Browse files Browse the repository at this point in the history
Integers don't respond to the `empty?` method. It caused issue with optional
params as described in #1847.
  • Loading branch information
dnesteryuk committed Dec 22, 2019
1 parent 28d34ee commit 1342559
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 14 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#### Fixes

* [#1947](https://github.com/ruby-grape/grape/pull/1947): Careful check for empty params - [@dnesteryuk](https://github.com/dnesteryuk).
* [#1931](https://github.com/ruby-grape/grape/pull/1946): Fixes issue when using namespaces in `Grape::API::Instance` mounted directly - [@myxoh](https://github.com/myxoh).

### 1.2.5 (2019/12/01)
Expand Down
4 changes: 1 addition & 3 deletions lib/grape/validations/attributes_iterator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ def do_each(params_to_process, parent_indicies = [], &block)

if @scope.type == Array
next unless @original_params.is_a?(Array) # do not validate content of array if it isn't array
inside_array = true
end
if inside_array

# fill current and parent scopes with correct array indicies
parent_scope = @scope.parent
parent_indicies.each do |parent_index|
Expand Down
13 changes: 11 additions & 2 deletions lib/grape/validations/single_attribute_iterator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,20 @@ module Validations
class SingleAttributeIterator < AttributesIterator
private

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

# Primitives like Integers and Booleans don't respond to +empty?+.
# It could be possible to use +blank?+ instead, but
#
# false.blank?
# => true
def empty?(val)
val.respond_to?(:empty?) ? val.empty? : val.nil?
end
end
end
end
10 changes: 5 additions & 5 deletions lib/grape/validations/validators/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ def validate!(params)
# there may be more than one error per field
array_errors = []

attributes.each do |resource_params, attr_name|
next if !@scope.required? && resource_params.empty?
next unless @scope.meets_dependency?(resource_params, params)
attributes.each do |val, attr_name, empty_val|
next if !@scope.required? && empty_val
next unless @scope.meets_dependency?(val, params)
begin
if @required || resource_params.respond_to?(:key?) && resource_params.key?(attr_name)
validate_param!(attr_name, resource_params)
if @required || val.respond_to?(:key?) && val.key?(attr_name)
validate_param!(attr_name, val)
end
rescue Grape::Exceptions::Validation => e
array_errors << e
Expand Down
20 changes: 16 additions & 4 deletions spec/grape/validations/single_attribute_iterator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
describe '#each' do
subject(:iterator) { described_class.new(validator, scope, params) }
let(:scope) { Grape::Validations::ParamsScope.new(api: Class.new(Grape::API)) }
let(:validator) { double(attrs: %i[first second third]) }
let(:validator) { double(attrs: %i[first second]) }

context 'when params is a hash' do
let(:params) do
Expand All @@ -15,7 +15,7 @@

it 'yields params and every single attribute from the list' do
expect { |b| iterator.each(&b) }
.to yield_successive_args([params, :first], [params, :second], [params, :third])
.to yield_successive_args([params, :first, false], [params, :second, false])
end
end

Expand All @@ -26,10 +26,22 @@

it 'yields every single attribute from the list for each of the array elements' do
expect { |b| iterator.each(&b) }.to yield_successive_args(
[params[0], :first], [params[0], :second], [params[0], :third],
[params[1], :first], [params[1], :second], [params[1], :third]
[params[0], :first, false], [params[0], :second, false],
[params[1], :first, false], [params[1], :second, false]
)
end

context 'empty values' do
let(:params) { [{}, '', 10] }

it 'marks params with empty values' do
expect { |b| iterator.each(&b) }.to yield_successive_args(
[params[0], :first, true], [params[0], :second, true],
[params[1], :first, true], [params[1], :second, true],
[params[2], :first, false], [params[2], :second, false]
)
end
end
end
end
end

0 comments on commit 1342559

Please sign in to comment.