diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b02b34310..0c46d4a150 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) diff --git a/lib/grape/validations/types/array_coercer.rb b/lib/grape/validations/types/array_coercer.rb index 4f456a7412..8dd819e0d7 100644 --- a/lib/grape/validations/types/array_coercer.rb +++ b/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) diff --git a/lib/grape/validations/types/dry_type_coercer.rb b/lib/grape/validations/types/dry_type_coercer.rb index a1c5fb36f7..06d234d550 100644 --- a/lib/grape/validations/types/dry_type_coercer.rb +++ b/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 diff --git a/lib/grape/validations/types/variant_collection_coercer.rb b/lib/grape/validations/types/variant_collection_coercer.rb index 9495c891c5..34982e1338 100644 --- a/lib/grape/validations/types/variant_collection_coercer.rb +++ b/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 diff --git a/lib/grape/validations/validators/coerce.rb b/lib/grape/validations/validators/coerce.rb index 8934b2c495..5b6f960a63 100644 --- a/lib/grape/validations/validators/coerce.rb +++ b/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] diff --git a/lib/grape/validations/validators/default.rb b/lib/grape/validations/validators/default.rb index c6c16529ea..ee620db460 100644 --- a/lib/grape/validations/validators/default.rb +++ b/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) diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index f0feff1769..e6537ff542 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/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] diff --git a/spec/grape/validations/validators/default_spec.rb b/spec/grape/validations/validators/default_spec.rb index f110fe0e86..10220eb58e 100644 --- a/spec/grape/validations/validators/default_spec.rb +++ b/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 diff --git a/spec/grape/validations/validators/values_spec.rb b/spec/grape/validations/validators/values_spec.rb index b975c3feb0..491b328341 100644 --- a/spec/grape/validations/validators/values_spec.rb +++ b/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 diff --git a/spec/grape/validations_spec.rb b/spec/grape/validations_spec.rb index e690ccb7d9..7bde6104af 100644 --- a/spec/grape/validations_spec.rb +++ b/spec/grape/validations_spec.rb @@ -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 @@ -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 @@ -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 @@ -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) @@ -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)