Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ValidateAll derive macro which automatically adds nested validation to all fields #267

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

davidlang42
Copy link

My suggestion here is to add a second derive macro ValidateAll which operates similarly to the original (and unmodified) Validate macro, except if any fields don't have any validation, they are assumed to have Nested validation (as if they had the attribute #[validate].

The use case here is when you have a Struct with many fields, most of which are more nested Structs. Down a few layers you probably have a Struct with more typical validation, such as:

#[derive(Validate)]
struct Address {
    name: String,
    #[validate(range(max = 99))]
    house_number: u8
    street: String
    #[validate(phone)]
    phone_number: String
    suburb: String
}

but in the upper layers you have many fields, all of which need Nested validation, writing #[validate] on every 2nd line would be quite verbose. So instead of having this:

#[derive(Validate)]
struct Order {
    #[validate]
    customer_number: u16,
    #[validate]
    billing_address: Address,
    #[validate]
    postal_address: Address,
    #[validate(email)]
    email: String,
    backup_address: Address
    #[validate]
    item_type: Item,
    #[validate(range(min = 1))]
    item_quantity: u8,
    #[validate]
    payment_method: Payment,
    #[validate]
    referrer: MarketingSource
}

you can simply put:

#[derive(ValidateAll)]
struct Order {
    customer_number: CustomerId,
    billing_address: Address,
    postal_address: Address,
    #[validate(email)]
    email: String,
    backup_address: Address
    item_type: Item,
    #[validate(range(min = 1))]
    item_quantity: u8,
    payment_method: Payment,
    referrer: MarketingSource
}

However there is another, and much more important catch here. The first option is not just verbose but its also prone to missing a field, meaning you think you are validating the nested data but you actually aren't. If you look closely, you'll see I forgot the #[validate] attribute on backup_address in the first example, but its not obvious at all. Once I use ValidateAll instead, it not only does the nesting for me, but if any of the types don't implement Validate (for example MarketingSource) then I'll get a compiler error telling me I forgot something.

I can't help but think this will benefit many people in many scenarios, but also I'm not tied to the exact implementation. If you don't like the extra derive macro ValidateAll another idea I had was adding a struct level attribute like #[validate(mandatory_on_all_fields)] which makes the Validate derive macro do the same thing as above.

Also in this PR is a new field level attribute #[validate(always_valid)] for the odd time when you want to use ValidateAll but some fields genuinely don't need validation.

@Keats
Copy link
Owner

Keats commented Aug 29, 2023

That should be an option on the current validate macro rather than a complete new impl.
There is a full rewrite ongoing (#262) so that's probably not going to be merged.

cc @pintariching

@@ -208,112 +211,153 @@ fn quote_field_validations(
}

/// Find if a struct has some schema validation and returns the info if so
fn find_struct_validation(attr: &syn::Attribute) -> SchemaValidation {
fn find_schema_validation(path: &syn::Path, nested: &syn::punctuated::Punctuated<syn::NestedMeta, syn::token::Comma>) -> SchemaValidation {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a huge change but actually its just renaming the function and pulling the first 2 lines of the if_chain into the calling function.

if let Ok(syn::Meta::List(syn::MetaList { ref nested, .. })) = attr.parse_meta();
if let syn::NestedMeta::Meta(syn::Meta::List(syn::MetaList { ref path, ref nested, .. })) = nested[0];

then {
Copy link
Author

@davidlang42 davidlang42 Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything from here down (in this function) is just whitespace (no code change)

.filter(|attribute| attribute.path == parse_quote!(validate))
.map(find_struct_validation)
.collect()
fn collect_struct_validations(struct_attrs: &[syn::Attribute]) -> (Vec<SchemaValidation>, bool) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual change, most code is just for handling the many cases of meta types (similarly to collect_field_validations)

@davidlang42
Copy link
Author

I've rewritten it has a struct attribute (#[validate(nest_all_fields)]) rather than new macro, as you suggested.

I understand the rewrite is ongoing, but as it doesn't seem very close to being merged I'm hoping you might consider merging this version (not being pushy though, just putting it out there).

It was actually a fairly small change to make this work (once I understood how the existing derive macro worked) so thanks for structuring well in the first place!

Also, the unit tests were quite helpful in making sure I didn't break this, they all pass now and I've added 2 more to account for the new functionality I've added.

@pintariching
Copy link
Contributor

With this change, all the fields have to be nested structs, otherwise the macro calls .validate() on any other type. You would need a #[validate(skip)] on fields you don't want validated and I don't think it's implemented in the main branch?

I can get this working in the rewrite.

@Keats
Copy link
Owner

Keats commented Aug 30, 2023

With this change, all the fields have to be nested structs, otherwise the macro calls .validate() on any other type. You would need a #[validate(skip)] on fields you don't want validated and I don't think it's implemented in the main branch?

Yep you would definitely need a skip attribute as well

@davidlang42
Copy link
Author

With this change, all the fields have to be nested structs, otherwise the macro calls .validate() on any other type. You would need a #[validate(skip)] on fields you don't want validated and I don't think it's implemented in the main branch?

I can get this working in the rewrite.

I included this! I just didn't call it skip. I called it alwaysvalid but I can rename to skip if you'd rather :)

@@ -90,4 +90,4 @@ pub use traits::{Contains, HasLen, Validate, ValidateArgs};
pub use types::{ValidationError, ValidationErrors, ValidationErrorsKind};

#[cfg(feature = "derive")]
pub use validator_derive::Validate;
pub use validator_derive::Validate;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undesirable whitespace change

@jayvdb
Copy link

jayvdb commented Feb 12, 2024

IMO skip is the usual term used for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants