Keats / validator

Simple validation for Rust structs
MIT License
1.94k stars 140 forks source link

Allow `length` to be used for all `std::collections` containers #175

Open nu11ptr opened 2 years ago

nu11ptr commented 2 years ago

For example, when trying to validate a BTreeSet I get this:

error: Validator `length` can only be used on types `String`, `&str`, Cow<'_,str> or `Vec` but found `BTreeSet<VLANPool>` for field `vlan_pools`

Can we get length implemented for all standard containers? I assume anything that has len should be reasonably easy to add?

Keats commented 2 years ago

Yep it's easy to add: https://github.com/Keats/validator/blob/master/validator/src/traits.rs

nu11ptr commented 2 years ago

PR #178 opened to implement this

nu11ptr commented 2 years ago

As an aside, did you ever consider a single blanket implementation of HasLen for all types with exact sized iterators? The only downside I can see to this is you would not be able to iterate over chars for your String types (due to lack of specialization), but have to revert to bytes, but otherwise you'd gain a ton of types for "free"

impl<I> HasLen for I where for<'a> &'a I: IntoIterator, for<'a> <&'a I as IntoIterator>::IntoIter: ExactSizeIterator { 
    fn length(&self) -> u64 {
        self.into_iter().len() as u64
    }
}
Keats commented 2 years ago

That would consume the object no? A single blanket impl would be nicer yes

nu11ptr commented 2 years ago

No, because this is implemented for references only. playground

A better idea might be to keep length just for string types so it can count chars and then introduce a new attribute size with a blanket implementation for container types.

This of course assumes we don't need an implementation for both I and &I which you have today. I didn't look further into why this was required, but if there is no way to break that requirement this probably won't work unfortunately (as implementing for I will try to consume).

Keats commented 2 years ago

Oh interesting I thought into_iter required self which wouldn't work with references

I didn't look further into why this was required, but if there is no way to break that requirement this probably won't work unfortunately

I wouldn't remember tbh, it's probably like that for no particular reason or for completeness sake? Those can still be implemented manually anyway.

Keats commented 2 years ago

Let's do that on your PR and keep some manual impl for the rest?

nu11ptr commented 2 years ago

Just to clarify, you are talking about length just for string types (likely each a manual impl over chars) and size for other types (single blanket via IntoIterator/ExactSizeIterator)? Happy to contribute that - will also need another trait for size (HasSize?). Very easy to do that, but I'm not sure I'd know how to integrate it without some help/guidance on where to look (but perhaps I can figure it out... I see that it is used in validate_length so we would just need a validate_size and then figure out how that hooks in, etc., etc.)

Lastly, I would think you'd likely want a new major or minor version as well as this would break Vec for length (more I think of it, nothing really stopping us from just keeping Vec as well and allowed either size or length. Could even declare deprecated and remove next time you do a major version bump).

Keats commented 2 years ago

Hmm yeah thinking about it it wouldn't be ideal. Let's avoid that and do one by one as in your original PR.

buinauskas commented 1 year ago

This has been a headache to me, is there any reason type aliases or newtypes that implement HasLen trait would not allow adding length validation attributes?

type Set<T> = BTreeSet<T>;

struct NewtypeSet(BTreeSet<u64>);

impl HasLen for NewtypeSet {
    fn length(&self) -> u64 {
        self.0.length()
    }
}

#[derive(Debug, Validate)]
struct Req {
    #[validate(length(min = 1))]
    ids1: BTreeSet<u64>,

    #[validate(length(min = 1))]
    ids2: Set<u64>,

    #[validate(length(min = 1))]
    ids3: NewtypeSet,
}

Errors from cargo check:

❯ cargo check
    Checking playground v0.1.0 (/Users/evaldas.buinauskas@vinted.com/vinted/playground)
error: Validator `length` can only be used on types `String`, `&str`, Cow<'_,str>, `Vec`, slice, or map/set types (BTree/Hash/Index) but found `Set<u64>` for field `ids2`
   --> src/main.rs:179:11
    |
179 |     ids2: Set<u64>,
    |           ^^^

error: could not compile `playground` due to previous error

playground on  master [?] is 📦 v0.1.0 via 💎 v2.7.2 via 🦀 v1.64.0 
❯ cargo check
    Checking playground v0.1.0 (/Users/evaldas.buinauskas@vinted.com/vinted/playground)
error: Validator `length` can only be used on types `String`, `&str`, Cow<'_,str>, `Vec`, slice, or map/set types (BTree/Hash/Index) but found `NewtypeSet` for field `ids3`
   --> src/main.rs:179:11
    |
179 |     ids3: NewtypeSet,
    |           ^^^^^^^^^^

error: could not compile `playground` due to previous error

Obviously I could just implement my own custom validator to validate that collections are not empty and it would work. but my assumption was that implementing HasLen would be enough to allow using lengh validator in attributes.

Thanks!

Keats commented 1 year ago

It was done in the macro to provide error message. The whole macro should be rewritten to avoid stuff like that