Skip to content

Commit

Permalink
Avoid coercing parameter with multiple types to an empty Array
Browse files Browse the repository at this point in the history
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 #2018
  • Loading branch information
stanhu committed Mar 17, 2020
1 parent 8deebb5 commit 31d9ff0
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 5 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.
* [#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)

Expand Down
13 changes: 8 additions & 5 deletions lib/grape/validations/validators/coerce.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,7 @@ def valid_type?(val)
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
return coerce_nil(val) if val.nil?

converter.call(val)

Expand All @@ -80,6 +76,13 @@ def coerce_value(val)
Types::InvalidValue.new
end

def coerce_nil(val)
return [] if type == Array
return Set.new if type == Set
return {} if type == Hash
return val
end

# Type to which the parameter will be coerced.
#
# @return [Class]
Expand Down
30 changes: 30 additions & 0 deletions spec/grape/validations/validators/coerce_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand Down

0 comments on commit 31d9ff0

Please sign in to comment.