Closed dhruvCW closed 2 months ago
- 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 🤔
- Semantically,
length
andsize
aren't the same thing, even though hashes for example support both length and size. Should we have interchangeablelength
andsize
validators?
While I agree, I see that in ruby both are interchangable so maybe an alias for length validator 🤔
- What happens with comparable types, e.g. when I say
min: 'xyz'
?- 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).
IMO that won't make sense. I can restrict the
min
andmax
values to be integer numbers (and maybe also that they not negative).
Yes, I think that would be the correct behavior.
- 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.
- Semantically,
length
andsize
aren't the same thing, even though hashes for example support both length and size. Should we have interchangeablelength
andsize
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?
- 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.
- Semantically,
length
andsize
aren't the same thing, even though hashes for example support both length and size. Should we have interchangeablelength
andsize
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
, andsize:
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.
I noticed the compact
. That forces an interesting behavior of removing nil
entries, which I don't think is right, nil
s 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
.
I noticed the
compact
. That forces an interesting behavior of removingnil
entries, which I don't think is right,nil
s 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 anil
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 🤔
I have restricted the validator to only validate
String
orArray
types. I am happy to alias the validator to supportsize:
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 alength
orsize
method.
Why? I think Hash
is an important one to support for a length validator, as an example.
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.
I noticed the
compact
. That forces an interesting behavior of removingnil
entries, which I don't think is right,nil
s 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 anil
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. https://github.com/ruby-grape/grape/issues/1370). For the tests for arrays switch to sending JSON which will match exactly what you're sending server-side.
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
orArray
should have such a validation but for a type likeHash
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?
Btw, thank you for hanging in here with me! Really appreciate your time and work on this.
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 itrespond_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 🙈
I noticed the
compact
. That forces an interesting behavior of removingnil
entries, which I don't think is right,nil
s 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 anil
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 fixed the specs.[JSON]
instead of [Integer]
and now when passing an empty array it fails in the coercer (as an invalid type). (pushed the failing test).
Merged, thank you!
Want to try to implement size
without duplicating all the code on top of this?
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.
@dblock will you be releasing 2.1.0 soon ?
@dhruvCW I'll take are of it this week
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
andmax_length
limits to the attribute documentation so that they can be used to specify the limits for an array type in swagger.