The `length` validator only takes effect for parameters with types that support `#length` method #2464

OuYangJinTing · 2024-06-28T08:57:01Z

Thanks to @dhruvCW for adding the length validator(#2437).

I found that in the case of optional parameters, passing in nil will lead to an exception. This behavior is not very friendly.
For nil, it should be a better choice to skip verification directly.

lib/grape/validations/validators/length_validator.rb

dhruvCW · 2024-06-28T11:17:31Z

I found that in the case of optional parameters, passing in nil will lead to an exception. This behavior is not very friendly. For nil, it should be a better choice to skip verification directly.

Ah nice catch 👏 for some reason I thought the validator won't be called if the parameter was not present (and optional) 😅

dblock · 2024-06-29T19:13:02Z

@OuYangJinTing care to answer the q above and update CHANGELOG, please?

OuYangJinTing · 2024-07-02T03:58:47Z

Ah nice catch 👏 for some reason I thought the validator won't be called if the parameter was not present (and optional) 😅

If the optional parameters are only passed, the verification will also be performed.
https://github.com/ruby-grape/grape/blob/v2.1.2/lib/grape/validations/validators/base.rb#L53

OuYangJinTing · 2024-07-02T04:02:27Z

@dblock Done ～

dblock

I have some nits, but more importantly, do we have a test for "If you do not allow nil, you can use the allow_blank: false option." If not please add?

spec/grape/validations/validators/length_spec.rb

README.md

CHANGELOG.md

OuYangJinTing · 2024-07-03T03:56:38Z

I have some nits, but more importantly, do we have a test for "If you do not allow nil, you can use the allow_blank: false option." If not please add?

@dblock Thank you for your feedback. At present, all feedback has been resolved. Please take a look when you have time.

dblock

Looks good, one more typo below.

dblock · 2024-07-03T15:49:27Z

spec/grape/validations/validators/length_spec.rb

+      end
+    end
+
+    context 'When a nil value is passed' do


Nit: lowercase "When" please?

dblock · 2024-07-03T15:50:15Z

README.md

@@ -1713,6 +1713,7 @@ end
 #### `length`

 Parameters with types that support `#length` method can be restricted to have a specific length with the `:length` option.
+In addition, if the received parameter value is `nil`, the length validation will not be triggered. If you do not allow `nil`, you can use the `allow_blank: false` option.


Add "want to", so "If you do not want to allow..."

um this will be problematic if you want to allow zero length parameters. For example

requires :some_list, type: [Integer], length: { min: 0, max: 10 }, allow_blank: false requires :some_string, type: String, length: { min: 0 }, allow_blank: false

would both have validation errors if we provided the following parameters { some_list: [], some_string: '' }

IMO this is incorrect behaviour.

I think this is exactly the expected behavior. Why do you think this is incorrect @dhruvCW?

I see like I mentioned combining the two can lead to unexpected results since allow_blank validates both nil as well as empty values. I personally then would appreciate this specific case to be documented so that users are aware about this potential pitfall.

Yes, we should, @OuYangJinTing add specs for ^ and README pls?

@dblock @dhruvCW Sorry, I'm a little busy with work these two days, and I didn't give timely feedback.
Based on this consideration, I think it should be better to add an allow_nil option to the Grape::Validations::Validators::LengthValidator. e.g:

module Grape module Validations module Validators class LengthValidator < Base def initialize(attrs, options, required, scope, **opts) @min = options[:min] @max = options[:max] @allow_nil = options.key?(:allow_nil) ? options[:allow_nil] : true # allow `allow_nil` option, default to `true` super raise ArgumentError, 'min must be an integer greater than or equal to zero' if !@min.nil? && (!@min.is_a?(Integer) || @min.negative?) raise ArgumentError, 'max must be an integer greater than or equal to zero' if !@max.nil? && (!@max.is_a?(Integer) || @max.negative?) raise ArgumentError, "min #{@min} cannot be greater than max #{@max}" if !@min.nil? && !@max.nil? && @min > @max end def validate_param!(attr_name, params) param = params[attr_name] return if param.nil? && @allow_nil # when `allow_nil` is `true`, `nil` is allowed raise ArgumentError, "parameter #{param} does not support #length" unless param.respond_to?(:length) return unless (!@min.nil? && param.length < @min) || (!@max.nil? && param.length > @max) raise Grape::Exceptions::Validation.new(params: [@scope.full_name(attr_name)], message: build_message) end ... end

If you agree, I will be happy to change it.

I agree but I think the default should be false. maybe ignore_nil would be a better name ? since we are skipping the validation for nil values.

@dhruvCW Thank you for your suggestions.

I agree that the default value be to false.

ignore_nil is a good name, but I use the name allow_nil, considering that there is already allow_blank, and it should be better to use a similar name.

ignore_nil is a good name, but I use the name allow_nil, considering that there is already allow_blank, and it should be better to use a similar name.

makes sense to keep the names consistent 😅 👍

Yes, the other reason not to change these is to remain positive vs. negative.

dhruvCW · 2024-07-03T16:06:51Z

lib/grape/validations/validators/length_validator.rb

@@ -18,6 +18,8 @@ def initialize(attrs, options, required, scope, **opts)
        def validate_param!(attr_name, params)
          param = params[attr_name]

+          return if param.nil?


Um should we ignore the validator if the parameter is required and the value is nil ?

IMO this is unexpected behavior right ? If a parameter is expected (thus marked as required) a nil value is technically incorrect. If we want to allow nil values we should mark the parameter as optional.

WDYT @dblock ?

A nil value is a perfectly valid value, that's why we have allow_blank (to distinguish the cases when the parameter itself is required and a value for the parameter is required).

I see I guess I contravened required with nil values. 😅 Sorry about that.

I know it's confusing. We have gone back and forth on this 🙈

dblock

I’ll wait to merge this till everyone above is happy, including myself :) Please say “I’m good with merging this” when you are! And thank you so much for having an interesting conversation!

OuYangJinTing · 2024-07-08T14:06:34Z

@dhruvCW @dblock Hello.
According to the above discussion, I refactored the code once. Please help review it when you have time.

dblock · 2024-07-08T14:34:47Z

~~How does this interact with allow_blank? Document the new validator in https://github.com/ruby-grape/grape?tab=readme-ov-file#built-in-validators.~~

Ok, it's just an option on length, I understand. What happens if I allow_blank: true on the length validator? I assume it throws an error that it's not something known?

README.md

dblock · 2024-07-08T14:38:55Z

spec/grape/validations/validators/length_spec.rb

+        requires :list, type: [Integer], length: { min: 0, allow_nil: true }
+      end
+      post 'allow_nil_param' do
+      end


Add a test with an explicit allow_nil: false.

We still need one test with the default (without any specification of allow_nil).

dblock · 2024-07-08T14:39:58Z

I'd love some other people's opinion on whether we do want the allow_nil option or reuse allow_blank for the length validator, maybe @ericproulx?

OuYangJinTing · 2024-07-09T03:35:11Z

Ok, it's just an option on length, I understand. What happens if I allow_blank: true on the length validator? I assume it throws an error that it's not something known?

I think it's not suitable to add the allow_blank option here because there is already an allow_blank validator.
Instead of adding an allow_blank option, it's better to use the allow_blank validator directly.

dhruvCW · 2024-07-09T11:59:42Z

lib/grape/validations/validators/length_validator.rb

@@ -18,6 +19,12 @@ def initialize(attrs, options, required, scope, **opts)
        def validate_param!(attr_name, params)
          param = params[attr_name]

+          if param.nil?
+            return if @allow_nil


I was looking into the code to understand how allow_blank interops with other validators and saw that it's an attribute in the base class.

So should we just use this option instead of introducing allow_nil ?

That was my thinking. 🤷‍♂️

I can help decide: does allow_nil actually behave like allow_blank in this implementation? Meaning, from the API point-of-view is nil vs. [] treated the same?

In Ruby it's obviously not.

2.7.7 :001 > [].blank? => true 2.7.7 :002 > nil.blank? => true

vs.

2.7.7 :003 > [].nil? => false 2.7.7 :004 > nil.nil? => true

If they are, then allow_blank makes sense. But if they aren't, then we should keep allow_nil because it explicitly checks for nil. Then, we should have an implementation for allow_blank? as well to distinguish the two.

technically no since allow_nil is for only nil values while allow_blank would mean ignoring both nil and empty values. Not sure if we want to use allow_blank or allow_nil.

I personally do prefer more specific constraints thus allow_nil but I am also okay if we decide to ignore blank values if the allow_blank flag is set to true.

@OuYangJinTing write specs for these scenarios and let's decide?

Again, thanks so much for being patient. API changes are one-way, we can't easily undo them, so it's worth spending the time.

@dhruvCW Introducing allow_nil here only will make it potentially different from other parameter types. Without looking at specs and current behavior I would say allow_blank for arrays/lists should behave like blank? and if we wanted a allow_nil then we should probably implement it for all other parameter types next to allow_blank.

@OuYangJinTing write specs for these scenarios and let's decide?

Again, thanks so much for being patient. API changes are one-way, we can't easily undo them, so it's worth spending the time.

@dblock Sorry, I'm a little confused.
Do you need to rollback to commit 171407aeef935ab918335c5ea324ab18a96a19d4 and provide a more detailed explanation?

Let's wait for @ericproulx to help us out with a strong opinion

Hmm, I'll make the changes after everyone finished feedback.

ericproulx · 2024-07-10T09:56:35Z

I'd love some other people's opinion on whether we do want the allow_nil option or reuse allow_blank for the length validator, maybe @ericproulx?

Sorry for my late response. Quite busy. I'll take a closer look this week-end

ericproulx · 2024-07-13T16:23:35Z

There's is still something confusing about the allow_nil in association with an array. For instance, if we declare the following:

params do
  requires :list, type: [Integer], length: { min: 1, allow_nil: true }
end

Does it mean that [nil] and nil are expected to be valid ? Unfortunately, the coerce_validator would raise an error since nil is not an Integer.

Although, I've noticed something regarding the allow_blank validator. If I'm not mistaken, its evaluation depends on where its declared. For instance, the following test will ✅ since it declared before the length validator (I've added the fail_fast on purpose to see which error will be triggered)

describe 'when allow_blank is declared before length validator' do
  subject { last_response }

  let(:app) do
    Class.new(Grape::API) do
      params do
        requires :list, type: [Integer], allow_blank: false, length: { max: 1 }, fail_fast: true
      end
      get do
      end
    end
  end
  
  before do
    get '/', list: nil
  end

  it { is_expected.to be_bad_request}
end

but this one will ❌ and raise the ArgumentError: parameter does not support #length

describe 'when allow_blank is declared after length validator' do
  subject { last_response }

  let(:app) do
    Class.new(Grape::API) do
      params do
        requires :list, type: [Integer], length: { max: 1 }, fail_fast: true, allow_blank: false
      end
      get do
      end
    end
  end
  before do
    get '/', list: nil
  end

  it { is_expected.to be_bad_request}
end

Based on that behavior, the length validator should not raise an ArgumentError when param doesn't respond_to?(:length) but return to let the allow_blank catch the case.

Also, regarding the min and max possible values, I feel that both need to be greater than 0 to also let allow_blank cover some cases. allow_blank: false would not allow the nil, empty strings, strings containing only whitespace characters and empty? arrays, hashes and sets.

OuYangJinTing · 2024-07-15T06:20:11Z

@ericproulx Thank you for your detailed insights and explanations with examples.👍

Considering the above, in order to avoid bring too much complexity.
I think the length validator should simply skip nil and explain this situation in the document.

cc @dhruvCW @dblock

dhruvCW · 2024-07-15T06:24:33Z

@ericproulx Thank you for your detailed insights and explanations with examples.👍

Considering the above, in order to avoid bring too much complexity. I think the length validator should simply skip nil and explain this situation in the document.

cc @dhruvCW @dblock

Should we simply skip all values that don't respond to length ? I assume the only time this is true is nil since the type coercer should either fail or wrap say an integer if that's passed as the parameter.

dblock · 2024-07-15T12:36:21Z

@ericproulx Thank you for your detailed insights and explanations with examples.👍
Considering the above, in order to avoid bring too much complexity. I think the length validator should simply skip nil and explain this situation in the document.
cc @dhruvCW @dblock

Should we simply skip all values that don't respond to length ? I assume the only time this is true is nil since the type coercer should either fail or wrap say an integer if that's passed as the parameter.

We should see what that breaks.

I like the idea that blank validator works for anything that respond_to?(:blank?) and nil validator for everything that respond_to?(:nil?) which is I suppose everything. It's probably easier to grok it that way.

OuYangJinTing · 2024-07-25T04:12:25Z

@dblock @dhruvCW According to the feedback, I have adjusted the PR again. Please help me check if it is correct when you are free. Thank you.

dhruvCW

Thanks for hanging in there 🍻 LGTM 🚀

dblock

Thanks!

Is this a breaking change or a bug fix? If it's a breaking change, then we need to major-bump version 😱 . If it's a bug fix, then move the CHANGELOG line to fixes.

Either way please add a section to UPGRADING.md.

…at support `#length` method

OuYangJinTing · 2024-07-26T04:06:31Z

Thanks!

Is this a breaking change or a bug fix? If it's a breaking change, then we need to major-bump version 😱 . If it's a bug fix, then move the CHANGELOG line to fixes.

Either way please add a section to UPGRADING.md.

I think this is a bug fix, because before, if parameters with types that don't support #length method, it directly throws an exception. In order to avoid exceptions, developers need to correct the api parameters by themselves, and after merging this PR, this situation can be avoided, this should be a smooth change.

dblock · 2024-07-26T22:54:21Z

Thanks!
Is this a breaking change or a bug fix? If it's a breaking change, then we need to major-bump version 😱 . If it's a bug fix, then move the CHANGELOG line to fixes.
Either way please add a section to UPGRADING.md.

I think this is a bug fix, because before, if parameters with types that don't support #length method, it directly throws an exception. In order to avoid exceptions, developers need to correct the api parameters by themselves, and after merging this PR, this situation can be avoided, this should be a smooth change.

@ericproulx Could use a second opinion, please?

ericproulx · 2024-07-27T18:35:09Z

Thanks!
Is this a breaking change or a bug fix? If it's a breaking change, then we need to major-bump version 😱 . If it's a bug fix, then move the CHANGELOG line to fixes.
Either way please add a section to UPGRADING.md.

I think this is a bug fix, because before, if parameters with types that don't support #length method, it directly throws an exception. In order to avoid exceptions, developers need to correct the api parameters by themselves, and after merging this PR, this situation can be avoided, this should be a smooth change.

@ericproulx Could use a second opinion, please?

Feels like a bug fix to me :). Nonetheless, I'm not sure about the UPGRADING comments since its a bug fix.

dblock · 2024-07-27T20:39:41Z

Feels like a bug fix to me :). Nonetheless, I'm not sure about the UPGRADING comments since its a bug fix.

I think better safe than sorry. Merging.

dhruvCW reviewed Jun 28, 2024

View reviewed changes

lib/grape/validations/validators/length_validator.rb Outdated Show resolved Hide resolved

OuYangJinTing force-pushed the length_validator_allow_nil branch from 33d25b7 to c7d4d8e Compare July 2, 2024 03:53

dblock reviewed Jul 2, 2024

View reviewed changes

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

README.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

OuYangJinTing force-pushed the length_validator_allow_nil branch from c7d4d8e to 171407a Compare July 3, 2024 03:51

dblock reviewed Jul 3, 2024

View reviewed changes

dhruvCW reviewed Jul 3, 2024

View reviewed changes

dblock requested changes Jul 5, 2024

View reviewed changes

OuYangJinTing force-pushed the length_validator_allow_nil branch 3 times, most recently from 5f6c279 to 99f66eb Compare July 8, 2024 14:00

OuYangJinTing requested review from dhruvCW and dblock July 8, 2024 14:01

OuYangJinTing changed the title ~~Length validator allow nil~~ Support allow nil in length validator Jul 8, 2024

dblock reviewed Jul 8, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

dblock reviewed Jul 8, 2024

View reviewed changes

OuYangJinTing force-pushed the length_validator_allow_nil branch from 99f66eb to 63534f3 Compare July 9, 2024 03:24

dhruvCW reviewed Jul 9, 2024

View reviewed changes

OuYangJinTing force-pushed the length_validator_allow_nil branch 2 times, most recently from ee5298f to ec83cc0 Compare July 15, 2024 06:07

OuYangJinTing changed the title ~~Support allow nil in length validator~~ Allow nil in length validator Jul 15, 2024

OuYangJinTing force-pushed the length_validator_allow_nil branch 2 times, most recently from b7a7701 to 5bf5a70 Compare July 25, 2024 04:07

OuYangJinTing requested review from dblock and dhruvCW July 25, 2024 04:08

ericproulx approved these changes Jul 25, 2024

View reviewed changes

dhruvCW approved these changes Jul 25, 2024

View reviewed changes

dblock requested changes Jul 25, 2024

View reviewed changes

The length validator only takes effect for parameters with types th…

ba84149

…at support `#length` method

OuYangJinTing force-pushed the length_validator_allow_nil branch from 5bf5a70 to ba84149 Compare July 26, 2024 04:04

OuYangJinTing changed the title ~~Allow nil in length validator~~ The length validator only takes effect for parameters with types that support #length method Jul 26, 2024

OuYangJinTing requested a review from dblock July 26, 2024 04:12

dblock approved these changes Jul 26, 2024

View reviewed changes

dblock merged commit 04e69ea into ruby-grape:master Jul 27, 2024
52 checks passed

The length validator only takes effect for parameters with types that support #length method #2464

The length validator only takes effect for parameters with types that support #length method #2464

Conversation

OuYangJinTing commented Jun 28, 2024 • edited Loading

dhruvCW commented Jun 28, 2024

dblock commented Jun 29, 2024

OuYangJinTing commented Jul 2, 2024

OuYangJinTing commented Jul 2, 2024

dblock left a comment

Choose a reason for hiding this comment

OuYangJinTing commented Jul 3, 2024

dblock left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvCW Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

OuYangJinTing commented Jul 8, 2024

dblock commented Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

dblock Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

dblock commented Jul 8, 2024

OuYangJinTing commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Jul 9, 2024 • 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

OuYangJinTing Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericproulx commented Jul 10, 2024

ericproulx commented Jul 13, 2024 • edited Loading

OuYangJinTing commented Jul 15, 2024 • edited Loading

dhruvCW commented Jul 15, 2024

dblock commented Jul 15, 2024

OuYangJinTing commented Jul 25, 2024

dhruvCW left a comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

OuYangJinTing commented Jul 26, 2024 • edited Loading

dblock commented Jul 26, 2024

ericproulx commented Jul 27, 2024 • edited Loading

dblock commented Jul 27, 2024

The `length` validator only takes effect for parameters with types that support `#length` method #2464

The `length` validator only takes effect for parameters with types that support `#length` method #2464

OuYangJinTing commented Jun 28, 2024 •

edited

Loading

dhruvCW Jul 3, 2024 •

edited

Loading

dblock commented Jul 8, 2024 •

edited

Loading

dblock Jul 9, 2024 •

edited

Loading

dblock Jul 9, 2024 •

edited

Loading

OuYangJinTing Jul 11, 2024 •

edited

Loading

ericproulx commented Jul 13, 2024 •

edited

Loading

OuYangJinTing commented Jul 15, 2024 •

edited

Loading

OuYangJinTing commented Jul 26, 2024 •

edited

Loading

ericproulx commented Jul 27, 2024 •

edited

Loading