Fix coerce_nil when Array of Type #2040

ericproulx · 2020-04-16T23:15:16Z

Since #2019, I found a regression (our tests suite fails with 1.3.2) when params type is an Array of Type and the api is called with a nil value. It should return the default value which is [].

ericproulx · 2020-04-16T23:33:43Z

Should it equals NilClass ?

dblock · 2020-04-17T01:45:31Z

lib/grape/validations/validators/coerce.rb

@@ -76,7 +76,7 @@ def coerce_value(val)
      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 []      if type == Array || type.is_a?(Array)


Don't you love ruby?

2.6.6 :001 > Array.is_a?(Array) => false

This obviously makes sense because a single type is not an array of types, but still funny.

dblock · 2020-04-17T01:48:55Z

spec/grape/validations/validators/coerce_spec.rb

+              params[:arry]
+            end
+
+            get '/array', arry: nil


I don't see a default value anywhere here. You're passing a nil, and the API expects an array of integers. Feels like it should fail?

I can add

default: []

and it still fails on master. I found that since arry is an actual params, the default validator doesn't do anything. Actually it returns immediatly here

Anyway, the default is kind of tricky for structures.
Array, Set and Hash will return a default value even if not explicitly written in the param declaration

So these specs are working

[[Array, 'Array'], [Set, 'Set'], [Hash, 'ActiveSupport::HashWithIndifferentAccess']].each do |type, expected_class| context 'structures default value' do it 'Empty array' do subject.params do requires :arry, type: type end subject.get '/array' do params[:arry].class end get '/array', arry: nil expect(last_response.status).to eq(200) expect(last_response.body).to eq(expected_class) end end end

Therefore, I was expecting that an array of types with nil like Array[Integer] as param value would behave like a plain Array.

Ok, please do add those specs and let's make sure to document this behavior?

dblock · 2020-04-17T13:24:16Z

ruboocop -a ; rubocop --auto-gen-config for build failures

dblock · 2020-04-17T18:53:09Z

spec/grape/validations/validators/coerce_spec.rb

@@ -752,19 +776,6 @@ def self.parsed?(value)
        expect(last_response.body).to eq('String')
      end

-      it 'respects nil values' do


This is no longer working? Aren't we breaking something?

this spec is actually the opposite and yes it's breaking #2019. But #2019 is why we have this conversation :)

I might be flip-flopping on what we want, let's step back. For this example alone, when I say :a can be either an array of File or a String, and I pass nil, don't I want a nil and not []?

For this example alone, when I say :a can be either an array of File or a String, and I pass nil, don't I want a nil and not []?

That was the behavior of Grape prior to v1.3.0. v1.3.x broke our tests with that, which is why I filed #2018.

dblock · 2020-04-17T18:54:05Z

README.md

@@ -1070,6 +1071,26 @@ params do
 end
 ```

+### Structures Implicit Default Value


Reading this it feels like it's wrong. Yes, it works this way. But if I send a nil, why am I getting a non-nil?

dblock · 2020-04-17T22:02:59Z

@stanhu @ericproulx

I understand old behavior of Grape was to coerce nil into default array values.
We accidentally fixed it for some scenarios in 1.3.x.
We decided it was a regression, and fixed it again, but not completely.
We are trying to fix it for real in this PR.
I am regretting (1), don't think this was Intentional.

Here's what I think we want:

nil values become nil values for all types, including arrays, sets and hashes
nil values can become default values when default: is specified, users who relied on this behavior will have to follow UPGRADING
We have specs for all the combinations so we are sure not to break this in the future

My question is, why would we not want to do what I am suggesting?

stanhu · 2020-04-18T06:17:40Z

@dblock That sounds like the right approach to me. I don't see any issues with that at the moment.

ericproulx · 2020-04-18T12:26:19Z

@dblock sound good to me too. For the default part (2), will need to adjust the default validator because nil doesn't apply with default. It's only applied when the param is missing.
For instance, this spec is failing

context 'integer default value when nil' do
        it 'respects the default value' do
          subject.params do
            optional :int, type: Integer, default: 1
          end
          subject.get '/default_value' do
            params[:int].class
          end

          get '/default_value', int: nil
          expect(last_response.status).to eq(200)
          expect(last_response.body).to eq("Integer")
        end
      end

but this one works

context 'integer default value when nil' do
        it 'respects the default value' do
          subject.params do
            optional :int, type: Integer, default: 1
          end
          subject.get '/default_value' do
            params[:int].class
          end

          get '/default_value'
          expect(last_response.status).to eq(200)
          expect(last_response.body).to eq("Integer")
        end
      end

It's just a matter the presence or not of the param.

So should we adjust the default validator behavior ?

dblock · 2020-04-18T13:10:31Z

So should we adjust the default validator behavior ?

Yes, I believe so. A nil value is a nil value, not the absence of a parameter.

ericproulx · 2020-04-18T14:17:18Z

@dblock @stanhu I've just pushed new commits. Could you validate the behavior ? Ill add the upgrading part after

dblock

This is looking good! Looks like we're not breaking any unrelated existing behavior, and I feel good about the thoroughness of tests.

spec/grape/validations/validators/default_spec.rb

spec/grape/validations/validators/regexp_spec.rb

dblock · 2020-04-20T13:24:07Z

LGTM, needs README and UPGRADING and I think it's good to go. Thank you!

Would appreciate another pair of eyes, maybe from @dnesteryuk, @stanhu.

stanhu · 2020-04-20T20:41:11Z

Thanks, this is looking good to me too. I ran this branch against our test suite. As expected, I found a few tests that failed because we had Integer and Boolean types with default values, but the tests were passing in nil values expecting the requests to be rejected. I'm not actually sure you pass in a nil value via an HTTP request, so I think the behavior of making them use the default values makes sense. Updating the CHANGELOG, README, and UPGRADING.md is going to be important here. :)

dblock · 2020-04-21T11:32:08Z

Let's also bump version to 1.4.

ericproulx · 2020-04-21T12:22:54Z

@dblock I found a real simple case that doesn't work which is Array[Integer] :(

Dry::Types::CoercionError: nil cannot be coerced to array

ericproulx · 2020-04-21T14:03:52Z

We'll have to discuss about the current spec failures @dblock

dnesteryuk · 2020-04-22T06:32:40Z

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


To be honest, it is something I would like to avoid. If a parent node is missing, why do we need to check children? 🤔

I would put it in "nice to have". Generally, you could see it both ways. If the info is not there, you see an error, then you add a parent, then it tells you that you're still missing children. Get frustrated, repeat.

spec/grape/validations/validators/default_spec.rb

dnesteryuk · 2020-04-22T07:14:49Z

In general, the idea and PR look right to me, there is only one concern about validating children in a structure when a parent node is missing.

expect(last_response.body).to eq('children[0][parents] is missing, children[0][parents][0][name] is missing')

But, I know that it is super hard to fix, validations are flat, so validation failure for parents doesn't interrupt validation for children.

We might leave it as it is in this PR.

ericproulx · 2020-04-22T07:41:38Z

What about this test that returns 200 instead of 400 ?

  params do
    requires :names, type: { value: Array[String], message: 'can\'t be nil' }, regexp: { value: /^[a-z]+$/, message: 'format is invalid' }
  end
  get 'regexp_with_array' do
  end

  it 'refuses nil instead of array' do
    get '/regexp_with_array', names: nil
    expect(last_response.status).to eq(400)
    expect(last_response.body).to eq('{"error":"names can\'t be nil"}')
  end

There are 3 similar specs that returns 200 now instead of 400

1) Grape::Validations::RegexpValidator custom validation message regexp with array refuses nil instead of array
     Failure/Error: expect(last_response.status).to eq(400)
     
       expected: 400
            got: 200
     
       (compared using ==)
     # ./spec/grape/validations/validators/regexp_spec.rb:103:in `block (4 levels) in <top (required)>'
  2) Grape::Validations::RegexpValidator regexp with array rejects nil instead of array
     Failure/Error: expect(last_response.status).to eq(400)
     
       expected: 400
            got: 200
     
       (compared using ==)
     # ./spec/grape/validations/validators/regexp_spec.rb:159:in `block (3 levels) in <top (required)>'
  3) Grape::Validations::ValuesValidator nil value for a parameter rejects for an optional param with a list of values
     Failure/Error: expect(last_response.status).to eq 400
     
       expected: 400
            got: 200
     
       (compared using ==)
     # ./spec/grape/validations/validators/values_spec.rb:324:in `block (3 levels) in <top (required)>'

dblock · 2020-04-22T13:38:18Z

What about this test that returns 200 instead of 400 ?

In these specs we are saying that the value has to be an Array[String], where each string matches /^[a-z]+$/. Feels like the expectation is correct, by passing a nil value for the Array, Grape should reject it because there's no default value?

Add CHANGELOG.md Add documentation in README.md Add specs for future proof return nil instead of InvalidValue Remove Structures Implicit Default Value section Not returning when param's value is nil Remove default value for structures Add specs for nil values and default values Add non-empty value spec when params is nil Typo change Rubocop Array[Integer] case doesn't work with default rejects Fix Array[Integer] Fix specs where nil is now a valid value Replaced non-empty-json by non-empty-string

ericproulx · 2020-04-23T11:28:45Z

So a fixed the remaining specs based on your last comments. I guess we're up to CHANGELOG, README, and UPGRADING.md :)

dblock · 2020-04-23T11:59:45Z

Awesome work @ericproulx. Yes please on README/UPGRADING.

ZachWileman · 2020-04-24T20:27:37Z

By chance, do you guys have any time estimate on when a new Grape version will be released with this fix in it? It looks like the change is mostly done besides a couple doc updates.

ericproulx · 2020-04-24T20:34:41Z

I’ll work the doc this weekend

…

On Apr 24, 2020, at 22:27, Zachary Wileman ***@***.***> wrote: By chance, do you guys have any time estimate on when a new Grape version will be released with this fix in it? It looks like the change is mostly done besides a couple doc updates. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2040 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAHJI543DOQATCT5AIHX4DROHY4PANCNFSM4MKJFCAA>.

ericproulx · 2020-04-28T12:36:20Z

I’ll work the doc this weekend
…
On Apr 24, 2020, at 22:27, Zachary Wileman @.***> wrote: By chance, do you guys have any time estimate on when a new Grape version will be released with this fix in it? It looks like the change is mostly done besides a couple doc updates. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2040 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACAHJI543DOQATCT5AIHX4DROHY4PANCNFSM4MKJFCAA.

Unfortunately, I didnt do it. If someone wants to write the DOC that would be great. I'm very busy at work right now probably like you.

dblock · 2020-05-03T17:32:47Z

This is a tremendous job @ericproulx. Thank you.

ericproulx · 2020-05-03T17:40:34Z

Thank you :)

…

On May 3, 2020, at 19:33, Daniel Doubrovkine (dB.) @dblockdotorg ***@***.***> wrote: This is a tremendous job @ericproulx <https://github.com/ericproulx>. Thank you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2040 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAHJI3O5F2CGFFC3YFKBP3RPWTEZANCNFSM4MKJFCAA>.

ZachWileman · 2020-05-15T20:19:23Z

Does anyone know when a new version of Grape is going to be released that includes this fix?

ghiculescu · 2020-05-26T20:21:58Z

@ericproulx just FYI this is causing us a bit of grief - not sure if it's an error you've seen too #1717 (comment)

ericproulx · 2020-05-26T20:44:19Z

@ericproulx just FYI this is causing us a bit of grief - not sure if it's an error you've seen too #1717 (comment)

Sorry about that, I'll investigate. First thought : default is applied when param is nil or when param is missing. So missing and nil are equivalent but should it ? @dblock

ghiculescu · 2020-05-26T20:57:08Z

That sounds right. https://github.com/ruby-grape/grape#parameter-validation-and-coercion doesn't specifically clarify this I assumed default is only used if the value is provided, but nil.

ghiculescu · 2020-05-26T21:00:19Z

Hmmm, actually, this implies the behaviour I'm seeing, since mutually_exclusive is also a validation:

If we decide to keep this behaviour, I'll make a PR to make the docs more explicit.

dblock · 2020-05-27T00:34:46Z

This is the correct behavior. I particularly find the mutually_exclusive example in #1717 (comment) interesting as a confirmation that this is the correct design with predictable, future-proof results.

ericproulx force-pushed the fix_coerce_nil_array branch from 433374b to 44e0498 Compare April 16, 2020 23:16

dblock reviewed Apr 17, 2020

View reviewed changes

ericproulx force-pushed the fix_coerce_nil_array branch from 44e0498 to aba1f3c Compare April 17, 2020 17:19

dblock reviewed Apr 17, 2020

View reviewed changes

dblock reviewed Apr 19, 2020

View reviewed changes

ericproulx force-pushed the fix_coerce_nil_array branch from 9b03676 to be04f92 Compare April 20, 2020 11:38

dnesteryuk reviewed Apr 22, 2020

View reviewed changes

spec/grape/validations/validators/default_spec.rb Outdated Show resolved Hide resolved

ericproulx force-pushed the fix_coerce_nil_array branch from 72541d9 to 3bcdc29 Compare April 23, 2020 11:27

Add UPGRADING notes

26711e6

dblock merged commit 01ca08c into ruby-grape:master May 3, 2020

magni- mentioned this pull request May 21, 2020

Release v. Next #2030

Closed

stanhu mentioned this pull request Jun 4, 2020

Make it possible to coerce nil Array of Types to [] #2068

Open

fonsecadeline mentioned this pull request Mar 2, 2021

timewindow start is zero by default Mapotempo/optimizer-api#143

Merged

braktar mentioned this pull request Mar 2, 2021

coerce_with should be called for params with nil value #2164

Merged

Fix coerce_nil when Array of Type #2040

Fix coerce_nil when Array of Type #2040

Conversation

ericproulx commented Apr 16, 2020 • edited Loading

ericproulx commented Apr 16, 2020

dblock Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Apr 17, 2020

dblock Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanhu Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Apr 17, 2020 • edited Loading

stanhu commented Apr 18, 2020

ericproulx commented Apr 18, 2020 • edited Loading

dblock commented Apr 18, 2020

ericproulx commented Apr 18, 2020

dblock left a comment

Choose a reason for hiding this comment

dblock commented Apr 20, 2020

stanhu commented Apr 20, 2020

dblock commented Apr 21, 2020

ericproulx commented Apr 21, 2020 • edited Loading

ericproulx commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnesteryuk commented Apr 22, 2020

ericproulx commented Apr 22, 2020

dblock commented Apr 22, 2020

ericproulx commented Apr 23, 2020

dblock commented Apr 23, 2020

ZachWileman commented Apr 24, 2020

ericproulx commented Apr 24, 2020 via email

ericproulx commented Apr 28, 2020

dblock commented May 3, 2020

ericproulx commented May 3, 2020 via email

ZachWileman commented May 15, 2020

ghiculescu commented May 26, 2020

ericproulx commented May 26, 2020 • edited Loading

ghiculescu commented May 26, 2020

ghiculescu commented May 26, 2020

dblock commented May 27, 2020 • edited Loading

ericproulx commented Apr 16, 2020 •

edited

Loading

dblock Apr 17, 2020 •

edited

Loading

dblock Apr 17, 2020 •

edited

Loading

stanhu Apr 17, 2020 •

edited

Loading

dblock commented Apr 17, 2020 •

edited

Loading

ericproulx commented Apr 18, 2020 •

edited

Loading

ericproulx commented Apr 21, 2020 •

edited

Loading

ericproulx commented May 26, 2020 •

edited

Loading

dblock commented May 27, 2020 •

edited

Loading