Avoid coercing parameter with multiple types to an empty Array

stanhu · stanhu · commit 633d7cf529d5 · 2020-03-17T09:34:25.000-07:00
If an optional parameter has multiple types (e.g. [File, String]) and a nil parameter is passed, Grape would previously coerce the value to an empty Array. This can break certain APIs that treat nil values differently from an empty Array. To fix this, we refactor the nil handling into a `coerce_nil` method and remove the special case check for handling an Array of types. Closes ruby-grape#2018
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@
 #### Fixes
 
 * Your contribution here.
+* [#2019](https://github.com/ruby-grape/grape/pull/2018): Avoid coercing parameter with multiple types to an empty Array - [@stanhu](https://github.com/stanhu).
 
 ### 1.3.1 (2020/03/11)
 
diff --git a/lib/grape/validations/validators/coerce.rb b/lib/grape/validations/validators/coerce.rb
@@ -65,21 +65,21 @@ def valid_type?(val)
       end
 
       def coerce_value(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
-        if val.nil?
-          return []      if type == Array || type.is_a?(Array)
-          return Set.new if type == Set
-          return {}      if type == Hash
-        end
-
-        converter.call(val)
-
+        val.nil? ? coerce_nil(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/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb
@@ -180,6 +180,23 @@ def self.parsed?(value)
         expect(last_response.body).to eq(integer_class_name)
       end
 
+      it 'String' do
+        subject.params do
+          requires :string, coerce: String
+        end
+        subject.get '/string' do
+          params[:string].class
+        end
+
+        get '/string', string: 45
+        expect(last_response.status).to eq(200)
+        expect(last_response.body).to eq('String')
+
+        get '/string', string: nil
+        expect(last_response.status).to eq(200)
+        expect(last_response.body).to eq('NilClass')
+      end
+
       it 'is a custom type' do
         subject.params do
           requires :uri, coerce: SecureURIOnly
@@ -647,6 +664,19 @@ 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]
