Fix coerce_nil when Array of Type (#2040)

CHANGELOG.md

@@ -11,6 +11,7 @@
 
 * [#2043](https://github.com/ruby-grape/grape/pull/2043): Modify declared for nested array and hash - [@kadotami](https://github.com/kadotami).
 * 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)
 

UPGRADING.md

@@ -1,6 +1,65 @@
 Upgrading Grape
 ===============
 
+### Upgrading to >= 1.4.0
+
+#### Nil values for structures
+
+Nil values always been a special case when dealing with types especially with the following structures:
+ - Array
+ - Hash
+ - Set
+
+The behaviour for these structures has change through out the latest releases. For instance:
+
+```ruby
+class Api < Grape::API
+ params do
+ require :my_param, type: Array[Integer]
+ end
+
+ get 'example' do
+ params[:my_param]
+ end
+ get '/example', params: { my_param: nil }
+ # 1.3.1 = []
+ # 1.3.2 = nil
+end
+```
+For now on, `nil` values stay `nil` values for all types, including arrays, sets and hashes.
+
+If you want to have the same behavior as 1.3.1, apply a `default` validator
+
+```ruby
+class Api < Grape::API
+ params do
+ require :my_param, type: Array[Integer], default: []
+ end
+
+ get 'example' do
+ params[:my_param]
+ end
+ get '/example', params: { my_param: nil } # => []
+end
+```
+
+#### Default validator
+
+Default validator is now applied for `nil` values.
+
+```ruby
+class Api < Grape::API
+ params do
+ requires :my_param, type: Integer, default: 0
+ end
+
+ get 'example' do
+ params[:my_param]
+ end
+ get '/example', params: { my_param: nil } #=> before: nil, after: 0
+end
+```
+
 ### Upgrading to >= 1.3.0
 
 #### Ruby

lib/grape/validations/types/array_coercer.rb

@@ -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)
 

lib/grape/validations/types/dry_type_coercer.rb

@@ -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

lib/grape/validations/types/variant_collection_coercer.rb

@@ -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

lib/grape/validations/validators/coerce.rb

@@ -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]

lib/grape/validations/validators/default.rb

@@ -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)

spec/grape/validations/validators/coerce_spec.rb

@@ -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
@@ -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]

spec/grape/validations/validators/default_spec.rb

@@ -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

spec/grape/validations/validators/values_spec.rb

@@ -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