Skip to content

Commit

Permalink
Add is_a?(Array) to coerce_nil because of Array with a type.
Browse files Browse the repository at this point in the history
Add CHANGELOG.md

Add documentation in README.md
Add specs for future proof

return nil instead of InvalidValue

Remove Structures Implicit Default Value section

Not returning when param's value is nil

Remove default value for structures

Add specs for nil values and default values

Add non-empty value spec when params is nil

Typo change

Rubocop

Array[Integer] case doesn't work with default

rejects

Fix Array[Integer]

Fix specs where nil is now a valid value

Replaced non-empty-json by non-empty-string
  • Loading branch information
ericproulx committed Apr 23, 2020
1 parent e0dfb9c commit 3bcdc29
Show file tree
Hide file tree
Showing 10 changed files with 207 additions and 31 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#### Fixes

* Your contribution here.
* [#2040](https://github.com/ruby-grape/grape/pull/2040): Fix a regression with Array of type nil - [@ericproulx](https://github.com/ericproulx).

### 1.3.2 (2020/04/12)

Expand Down
2 changes: 2 additions & 0 deletions lib/grape/validations/types/array_coercer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ def call(_val)
protected

def coerce_elements(collection)
return if collection.nil?

collection.each_with_index do |elem, index|
return InvalidValue.new if reject?(elem)

Expand Down
2 changes: 2 additions & 0 deletions lib/grape/validations/types/dry_type_coercer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ def initialize(type, strict = false)
#
# @param val [Object]
def call(val)
return if val.nil?

@coercer[val]
rescue Dry::Types::CoercionError => _e
InvalidValue.new
Expand Down
2 changes: 1 addition & 1 deletion lib/grape/validations/types/variant_collection_coercer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def initialize(types, method = nil)
# the coerced result, or an instance
# of {InvalidValue} if the value could not be coerced.
def call(value)
return InvalidValue.new unless value.is_a? Array
return unless value.is_a? Array

value =
if @method
Expand Down
11 changes: 1 addition & 10 deletions lib/grape/validations/validators/coerce.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,21 +67,12 @@ def valid_type?(val)
end

def coerce_value(val)
val.nil? ? coerce_nil(val) : converter.call(val)
converter.call(val)
# Some custom types might fail, so it should be treated as an invalid value
rescue StandardError
Types::InvalidValue.new
end

def coerce_nil(val)
# define default values for structures, the dry-types lib which is used
# for coercion doesn't accept nil as a value, so it would fail
return [] if type == Array
return Set.new if type == Set
return {} if type == Hash
val
end

# Type to which the parameter will be coerced.
#
# @return [Class]
Expand Down
1 change: 0 additions & 1 deletion lib/grape/validations/validators/default.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ def initialize(attrs, options, required, scope, opts = {})
end

def validate_param!(attr_name, params)
return if params.key? attr_name
params[attr_name] = if @default.is_a? Proc
@default.call
elsif @default.frozen? || !duplicatable?(@default)
Expand Down
86 changes: 73 additions & 13 deletions spec/grape/validations/validators/coerce_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,79 @@ def self.parsed?(value)
expect(last_response.status).to eq(200)
expect(last_response.body).to eq(integer_class_name)
end

context 'nil values' do
context 'primitive types' do
Grape::Validations::Types::PRIMITIVES.each do |type|
it 'respects the nil value' do
subject.params do
requires :param, type: type
end
subject.get '/nil_value' do
params[:param].class
end

get '/nil_value', param: nil
expect(last_response.status).to eq(200)
expect(last_response.body).to eq('NilClass')
end
end
end

context 'structures types' do
Grape::Validations::Types::STRUCTURES.each do |type|
it 'respects the nil value' do
subject.params do
requires :param, type: type
end
subject.get '/nil_value' do
params[:param].class
end

get '/nil_value', param: nil
expect(last_response.status).to eq(200)
expect(last_response.body).to eq('NilClass')
end
end
end

context 'special types' do
Grape::Validations::Types::SPECIAL.each_key do |type|
it 'respects the nil value' do
subject.params do
requires :param, type: type
end
subject.get '/nil_value' do
params[:param].class
end

get '/nil_value', param: nil
expect(last_response.status).to eq(200)
expect(last_response.body).to eq('NilClass')
end
end

context 'variant-member-type collections' do
[
Array[Integer, String],
[Integer, String, Array[Integer, String]]
].each do |type|
it 'respects the nil value' do
subject.params do
requires :param, type: type
end
subject.get '/nil_value' do
params[:param].class
end

get '/nil_value', param: nil
expect(last_response.status).to eq(200)
expect(last_response.body).to eq('NilClass')
end
end
end
end
end
end

context 'using coerce_with' do
Expand Down Expand Up @@ -752,19 +825,6 @@ def self.parsed?(value)
expect(last_response.body).to eq('String')
end

it 'respects nil values' do
subject.params do
optional :a, types: [File, String]
end
subject.get '/' do
params[:a].class.to_s
end

get '/', a: nil
expect(last_response.status).to eq(200)
expect(last_response.body).to eq('NilClass')
end

it 'fails when no coercion is possible' do
subject.params do
requires :a, types: [Boolean, Integer]
Expand Down
121 changes: 121 additions & 0 deletions spec/grape/validations/validators/default_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -298,4 +298,125 @@ def app
end
end
end

context 'optional with nil as value' do
subject do
Class.new(Grape::API) do
default_format :json
end
end

def app
subject
end

context 'primitive types' do
[
[Integer, 0],
[Integer, 42],
[Float, 0.0],
[Float, 4.2],
[BigDecimal, 0.0],
[BigDecimal, 4.2],
[Numeric, 0],
[Numeric, 42],
[Date, Date.today],
[DateTime, DateTime.now],
[Time, Time.now],
[Time, Time.at(0)],
[Grape::API::Boolean, false],
[String, ''],
[String, 'non-empty-string'],
[Symbol, :symbol],
[TrueClass, true],
[FalseClass, false]
].each do |type, default|
it 'respects the default value' do
subject.params do
optional :param, type: type, default: default
end
subject.get '/default_value' do
params[:param]
end

get '/default_value', param: nil
expect(last_response.status).to eq(200)
expect(last_response.body).to eq(default.to_json)
end
end
end

context 'structures types' do
[
[Hash, {}],
[Hash, { test: 'non-empty' }],
[Array, []],
[Array, ['non-empty']],
[Array[Integer], []],
[Set, []],
[Set, [1]]
].each do |type, default|
it 'respects the default value' do
subject.params do
optional :param, type: type, default: default
end
subject.get '/default_value' do
params[:param]
end

get '/default_value', param: nil
expect(last_response.status).to eq(200)
expect(last_response.body).to eq(default.to_json)
end
end
end

context 'special types' do
[
[JSON, ''],
[JSON, { test: 'non-empty-string' }.to_json],
[Array[JSON], []],
[Array[JSON], [{ test: 'non-empty-string' }.to_json]],
[::File, ''],
[::File, { test: 'non-empty-string' }.to_json],
[Rack::Multipart::UploadedFile, ''],
[Rack::Multipart::UploadedFile, { test: 'non-empty-string' }.to_json]
].each do |type, default|
it 'respects the default value' do
subject.params do
optional :param, type: type, default: default
end
subject.get '/default_value' do
params[:param]
end

get '/default_value', param: nil
expect(last_response.status).to eq(200)
expect(last_response.body).to eq(default.to_json)
end
end
end

context 'variant-member-type collections' do
[
[Array[Integer, String], [0, '']],
[Array[Integer, String], [42, 'non-empty-string']],
[[Integer, String, Array[Integer, String]], [0, '', [0, '']]],
[[Integer, String, Array[Integer, String]], [42, 'non-empty-string', [42, 'non-empty-string']]]
].each do |type, default|
it 'respects the default value' do
subject.params do
optional :param, type: type, default: default
end
subject.get '/default_value' do
params[:param]
end

get '/default_value', param: nil
expect(last_response.status).to eq(200)
expect(last_response.body).to eq(default.to_json)
end
end
end
end
end
2 changes: 1 addition & 1 deletion spec/grape/validations/validators/values_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ def app
expect(last_response.status).to eq 200
end

it 'allows for an optional param with a list of values' do
it 'accepts for an optional param with a list of values' do
put('/optional_with_array_of_string_values', optional: nil)
expect(last_response.status).to eq 200
end
Expand Down
10 changes: 5 additions & 5 deletions spec/grape/validations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ def validate_param!(attr_name, params)
# NOTE: with body parameters in json or XML or similar this
# should actually fail with: children[parents][name] is missing.
expect(last_response.status).to eq(400)
expect(last_response.body).to eq('children[1][parents] is missing')
expect(last_response.body).to eq('children[1][parents] is missing, children[0][parents][1][name] is missing, children[0][parents][1][name] is empty')
end

it 'errors when a parameter is not present in array within array' do
Expand Down Expand Up @@ -615,7 +615,7 @@ def validate_param!(attr_name, params)

get '/within_array', children: [name: 'Jay']
expect(last_response.status).to eq(400)
expect(last_response.body).to eq('children[0][parents] is missing')
expect(last_response.body).to eq('children[0][parents] is missing, children[0][parents][0][name] is missing, children[0][parents][0][name] is empty')
end

it 'errors when param is not an Array' do
Expand Down Expand Up @@ -763,7 +763,7 @@ def validate_param!(attr_name, params)
expect(last_response.status).to eq(200)
put_with_json '/within_array', children: [name: 'Jay']
expect(last_response.status).to eq(400)
expect(last_response.body).to eq('children[0][parents] is missing')
expect(last_response.body).to eq('children[0][parents] is missing, children[0][parents][0][name] is missing')
end
end

Expand Down Expand Up @@ -838,7 +838,7 @@ def validate_param!(attr_name, params)
it 'does internal validations if the outer group is present' do
get '/nested_optional_group', items: [{ key: 'foo' }]
expect(last_response.status).to eq(400)
expect(last_response.body).to eq('items[0][required_subitems] is missing')
expect(last_response.body).to eq('items[0][required_subitems] is missing, items[0][required_subitems][0][value] is missing')

get '/nested_optional_group', items: [{ key: 'foo', required_subitems: [{ value: 'bar' }] }]
expect(last_response.status).to eq(200)
Expand All @@ -858,7 +858,7 @@ def validate_param!(attr_name, params)
it 'handles validation within arrays' do
get '/nested_optional_group', items: [{ key: 'foo' }]
expect(last_response.status).to eq(400)
expect(last_response.body).to eq('items[0][required_subitems] is missing')
expect(last_response.body).to eq('items[0][required_subitems] is missing, items[0][required_subitems][0][value] is missing')

get '/nested_optional_group', items: [{ key: 'foo', required_subitems: [{ value: 'bar' }] }]
expect(last_response.status).to eq(200)
Expand Down

0 comments on commit 3bcdc29

Please sign in to comment.