Length validator allow nil

[OuYangJinTing]

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.

[dhruvCW]

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]

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

[OuYangJinTing]

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]

@dblock Done ～

[OuYangJinTing]

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.

[OuYangJinTing]

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

[dblock]

~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?

[dblock]

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]

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.

[ericproulx]

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]

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]

@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]

@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]

@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]

@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.

[OuYangJinTing]

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]

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]

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]

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.