From af45dff688130291b535abf6f97ffbbf84bf69f2 Mon Sep 17 00:00:00 2001 From: Mikhail Doronin Date: Fri, 20 Mar 2020 17:12:40 +0100 Subject: [PATCH 1/2] Fix a regression in coerce_with compared to 1.2.x when coercion returns nil Previously nil was handled as valid value for coercion for integer parameters. With refactoring introduced in 1.3.0 integer parameter coerced with a custom method can't coerce to nil any more. This change reverts back to previous behavior where coercion to nil was allowed. --- CHANGELOG.md | 1 + .../validations/types/custom_type_coercer.rb | 2 +- .../validations/validators/coerce_spec.rb | 37 +++++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f37b7ea6d..bf64678251 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ #### Fixes * Your contribution here. +* [#2026](https://github.com/ruby-grape/grape/pull/2026): Fix a regression in coerce_with when coercion returns nil - [@misdoro](https://github.com/misdoro). * [#2025](https://github.com/ruby-grape/grape/pull/2025): Fix Decimal type category - [@kdoya](https://github.com/kdoya). * [#2019](https://github.com/ruby-grape/grape/pull/2019): Avoid coercing parameter with multiple types to an empty Array - [@stanhu](https://github.com/stanhu). diff --git a/lib/grape/validations/types/custom_type_coercer.rb b/lib/grape/validations/types/custom_type_coercer.rb index 70ac7b20a9..f6a4e0cf9d 100644 --- a/lib/grape/validations/types/custom_type_coercer.rb +++ b/lib/grape/validations/types/custom_type_coercer.rb @@ -60,7 +60,7 @@ def call(val) end def coerced?(val) - @type_check.call val + val.nil? || @type_check.call(val) end private diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index ba6cd86722..7087548ad4 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/spec/grape/validations/validators/coerce_spec.rb @@ -525,6 +525,43 @@ def self.parsed?(value) expect(last_response.body).to eq('3') end + it 'allows coerce_with to return nil' do + subject.params do + requires :int, type: Integer, coerce_with: (lambda do |val| + if val == '0' + nil + elsif val.match?(/^-?\d+$/) + val.to_i + else + val + end + end) + end + subject.get '/' do + params[:int].class.to_s + end + + get '/', int: '0' + + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('NilClass') + + get '/', int: '1' + + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('Integer') + + get '/', int: '-1' + + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('Integer') + + get '/', int: 'lol' + + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('int is invalid') + end + it 'must be supplied with :type or :coerce' do expect do subject.params do From adb2672b8fbc5737bf1147e22b17901c9bb2ef38 Mon Sep 17 00:00:00 2001 From: Mikhail Doronin Date: Fri, 20 Mar 2020 17:44:28 +0100 Subject: [PATCH 2/2] Take review comments into account --- CHANGELOG.md | 2 +- .../validations/validators/coerce_spec.rb | 59 ++++++++++--------- 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf64678251..22cdf8f606 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ #### Fixes * Your contribution here. -* [#2026](https://github.com/ruby-grape/grape/pull/2026): Fix a regression in coerce_with when coercion returns nil - [@misdoro](https://github.com/misdoro). +* [#2026](https://github.com/ruby-grape/grape/pull/2026): Fix a regression in `coerce_with` when coercion returns `nil` - [@misdoro](https://github.com/misdoro). * [#2025](https://github.com/ruby-grape/grape/pull/2025): Fix Decimal type category - [@kdoya](https://github.com/kdoya). * [#2019](https://github.com/ruby-grape/grape/pull/2019): Avoid coercing parameter with multiple types to an empty Array - [@stanhu](https://github.com/stanhu). diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index 7087548ad4..2badb4c4ce 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/spec/grape/validations/validators/coerce_spec.rb @@ -525,41 +525,44 @@ def self.parsed?(value) expect(last_response.body).to eq('3') end - it 'allows coerce_with to return nil' do - subject.params do - requires :int, type: Integer, coerce_with: (lambda do |val| - if val == '0' - nil - elsif val.match?(/^-?\d+$/) - val.to_i - else - val - end - end) - end - subject.get '/' do - params[:int].class.to_s + context 'Integer type and coerce_with potentially returning nil' do + before do + subject.params do + requires :int, type: Integer, coerce_with: (lambda do |val| + if val == '0' + nil + elsif val.match?(/^-?\d+$/) + val.to_i + else + val + end + end) + end + subject.get '/' do + params[:int].class.to_s + end end - get '/', int: '0' + it 'accepts value that coerces to nil' do + get '/', int: '0' - expect(last_response.status).to eq(200) - expect(last_response.body).to eq('NilClass') - - get '/', int: '1' - - expect(last_response.status).to eq(200) - expect(last_response.body).to eq('Integer') + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('NilClass') + end - get '/', int: '-1' + it 'coerces to Integer' do + get '/', int: '1' - expect(last_response.status).to eq(200) - expect(last_response.body).to eq('Integer') + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('Integer') + end - get '/', int: 'lol' + it 'returns invalid value if coercion returns a wrong type' do + get '/', int: 'lol' - expect(last_response.status).to eq(400) - expect(last_response.body).to eq('int is invalid') + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('int is invalid') + end end it 'must be supplied with :type or :coerce' do