-
Notifications
You must be signed in to change notification settings - Fork 156
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
base: master
Are you sure you want to change the base?
Conversation
That should be an option on the current validate macro rather than a complete new impl. |
@@ -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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
)
I've rewritten it has a struct attribute ( 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. |
With this change, all the fields have to be nested structs, otherwise the macro calls I can get this working in the rewrite. |
Yep you would definitely need a skip attribute as well |
I included this! I just didn't call it skip. I called it |
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undesirable whitespace change
IMO skip is the usual term used for this. |
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:
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:you can simply put:
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 onbackup_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 exampleMarketingSource
) 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 theValidate
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 useValidateAll
but some fields genuinely don't need validation.