Add length validator

[dhruvCW]

This validator validates that the length of the parameter (if the parameter supports #length is within the specified limits).

I've also added the min_length and max_length limits to the attribute documentation so that they can be used to specify the limits for an array type in swagger.

[dblock]

Looks good, some nits below, plus:

1. Add README documentation.
2. Add tests for zero.
3. Handle/add tests for min = max, min > max and min > max. Probably needs to error on some.

[dblock]

I think this needs a bit more work around types.

1. Length is not supported on types such as Integer, and should fail accordingly. Needs tests.
2. Semantically, length and size aren't the same thing, even though hashes for example support both length and size. Should we have interchangeable length and size validators?
3. What happens with comparable types, e.g. when I say min: 'xyz'?
4. Should this be a more generic validator that works on anything Comparable?

[dhruvCW]

1. Length is not supported on types such as Integer, and should fail accordingly. Needs tests.

makes sense should an argument error be raised in this case 🤔

2. Semantically, length and size aren't the same thing, even though hashes for example support both length and size. Should we have interchangeable length and size validators?

While I agree, I see that in ruby both are interchangable so maybe an alias for length validator 🤔

3. What happens with comparable types, e.g. when I say min: 'xyz'?
4. Should this be a more generic validator that works on anything Comparable?

IMO that won't make sense. I can restrict the min and max values to be integer numbers (and maybe also that they not negative).

[dblock]

IMO that won't make sense. I can restrict the min and max values to be integer numbers (and maybe also that they not negative).

Yes, I think that would be the correct behavior.

[dblock]

1. Length is not supported on types such as Integer, and should fail accordingly. Needs tests.

makes sense should an argument error be raised in this case 🤔

Yes, I think anything being sent that doesn't have .length should yield a clear error.

2. Semantically, length and size aren't the same thing, even though hashes for example support both length and size. Should we have interchangeable length and size validators?

While I agree, I see that in ruby both are interchangable so maybe an alias for length validator 🤔

I think I'd like to write length: ... which causes the validator to invoke .length, and size: which causes the validator to invoke .size. I would implement both validators with a base "comparable" class that we can reuse for a future (or part of this PR maybe even) "compare: { min: max: }" validator that would work for strings too?

[dhruvCW]

1. Length is not supported on types such as Integer, and should fail accordingly. Needs tests.

makes sense should an argument error be raised in this case 🤔

Yes, I think anything being sent that doesn't have .length should yield a clear error.

2. Semantically, length and size aren't the same thing, even though hashes for example support both length and size. Should we have interchangeable length and size validators?

While I agree, I see that in ruby both are interchangable so maybe an alias for length validator 🤔

I think I'd like to write length: ... which causes the validator to invoke .length, and size: which causes the validator to invoke .size. I would implement both validators with a base "comparable" class that we can reuse for a future (or part of this PR maybe even) "compare: { min: max: }" validator that would work for strings too?

I have restricted the validator to only validate String or Array types. I am happy to alias the validator to support size: as well but I think for this PR I want to be restrictive and only support these two concrete types instead of any class that has a length or size method.

[dblock]

We can leave the size/length for a future PR. But I am not sure restricting to Array and String is correct.

[dblock]

I noticed the compact. That forces an interesting behavior of removing nil entries, which I don't think is right, nils are very valid values too, especially in content types like JSON, so let's remove it (compact can be introduced as a future option). Add tests with a nil value that would fail with a .compact.

[dhruvCW]

I noticed the compact. That forces an interesting behavior of removing nil entries, which I don't think is right, nils are very valid values too, especially in content types like JSON, so let's remove it (compact can be introduced as a future option). Add tests with a nil value that would fail with a .compact.

the problem is when an empty array is passed as the parameter the validator receives [nil]. How do you propose we handle this situation 🤔

[dblock]

I have restricted the validator to only validate String or Array types. I am happy to alias the validator to support size: as well but I think for this PR I want to be restrictive and only support these two concrete types instead of any class that has a length or size method.

Why? I think Hash is an important one to support for a length validator, as an example.

[dhruvCW]

But I am not sure restricting to Array and String is correct.

could you maybe elaborate on your concern 🤔 .

My line of thinking is that a type with a variable "length" (number of contained elements) like String or Array should have such a validation but for a type like Hash which is usually well specified (known set of keys) it doesn't make sense to validate the "length" or "size" of the object.

[dblock]

I noticed the compact. That forces an interesting behavior of removing nil entries, which I don't think is right, nils are very valid values too, especially in content types like JSON, so let's remove it (compact can be introduced as a future option). Add tests with a nil value that would fail with a .compact.

the problem is when an empty array is passed as the parameter the validator receives [nil]. How do you propose we handle this situation 🤔

Either way, if a client sends an empty array, the server is expected to receive an empty array ([nil] is not an empty array). I believe that is a side effect of rack-test or rack parser that we don't control for GETs or POSTs with encoded form content type or something like that. There's a bunch of issues filed along those lines (e.g. #1370). For the tests for arrays switch to sending JSON which will match exactly what you're sending server-side.

[dblock]

could you maybe elaborate on your concern 🤔 .

My line of thinking is that a type with a variable "length" (number of contained elements) like String or Array should have such a validation but for a type like Hash which is usually well specified (known set of keys) it doesn't make sense to validate the "length" or "size" of the object.

Take a typical rollup API that receives some stats for a given, say SKU.

{
   'sku1' : {
      total: 123.
      name: 'sku one'
   },
   'sku2' : {
      total: 234.
      name: 'sku two'
   }
}

I choose a hash because I don't want sku's to repeat themselves, and with the length validator I'd be able to limit how many sku's one could send at any given time.

The other concern is that if we restruct length on certain types it will be part of the API forever. Relaxing it would require a backwards-incompatible change. If we make it respond_to? :length, we would defer to the broadest range of capabilities (and allow people to shoot themselves in the foot, which is fine for an API library).

WDYT?

[dblock]

Btw, thank you for hanging in here with me! Really appreciate your time and work on this.

[dhruvCW]

The other concern is that if we restrict length on certain types it will be part of the API forever. Relaxing it would require a backwards-incompatible change. If we make it respond_to? :length, we would defer to the broadest range of capabilities (and allow people to shoot themselves in the foot, which is fine for an API library).

WDYT?

Ah makes sense ✅

and allow people to shoot themselves in the foot, which is fine for an API library

😅 wasn't sure if this is the library stance or not 🙈

[dhruvCW]

I noticed the compact. That forces an interesting behavior of removing nil entries, which I don't think is right, nils are very valid values too, especially in content types like JSON, so let's remove it (compact can be introduced as a future option). Add tests with a nil value that would fail with a .compact.

the problem is when an empty array is passed as the parameter the validator receives [nil]. How do you propose we handle this situation 🤔

Either way, if a client sends an empty array, the server is expected to receive an empty array ([nil] is not an empty array). I believe that is a side effect of rack-test or rack parser that we don't control for GETs or POSTs with encoded form content type or something like that. There's a bunch of issues filed along those lines (e.g. #1370). For the tests for arrays switch to sending JSON which will match exactly what you're sending server-side.

just tried to change the tests to match the type to [JSON] instead of [Integer] and now when passing an empty array it fails in the coercer (as an invalid type). (pushed the failing test). fixed the specs.

[dblock]

Merged, thank you!

Want to try to implement size without duplicating all the code on top of this?

[dhruvCW]

Merged, thank you!

Want to try to implement size without duplicating all the code on top of this?

😅 For now no. I'll try the next time I have some time to spare.

[dhruvCW]

@dblock will you be releasing 2.1.0 soon ?

[dblock]

@dhruvCW I'll take are of it this week