From 83c21a70ca3dee1218b9f877228203534f9ac5f4 Mon Sep 17 00:00:00 2001 From: Peerat Vichivanives Date: Mon, 16 Dec 2024 15:16:54 -0800 Subject: [PATCH 1/3] fix by only running nested if outer is not run --- CHANGELOG.md | 5 +++++ validator/src/types.rs | 5 +++-- validator_derive/src/tokens/nested.rs | 4 +++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2edf8d4d..b2edaf1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ ## Changelog +## 0.19.1 (unreleased) + +- Bug fix for nested issue with custom only running nested if outer passes + + ## 0.19.0 (2024/11/03) - Swap to using proc-macro-error-2 instead of proc-macro-error for Syn diff --git a/validator/src/types.rs b/validator/src/types.rs index 861f2d28..349979fe 100644 --- a/validator/src/types.rs +++ b/validator/src/types.rs @@ -181,9 +181,10 @@ impl ValidationErrors { fn add_nested(&mut self, field: &'static str, errors: ValidationErrorsKind) { if let Vacant(entry) = self.0.entry(field) { entry.insert(errors); - } else { - panic!("Attempt to replace non-empty ValidationErrors entry"); } + // Else case here is simply do nothing. Pretty sure this is caught earlier to not run but since + // If we get here, just not adding it does the same thing (although waste extra compute) + // probably better to just ignore here. } #[must_use] diff --git a/validator_derive/src/tokens/nested.rs b/validator_derive/src/tokens/nested.rs index 3af6ffbf..b74aab4d 100644 --- a/validator_derive/src/tokens/nested.rs +++ b/validator_derive/src/tokens/nested.rs @@ -5,6 +5,8 @@ pub fn nested_tokens( field_name_str: &str, ) -> proc_macro2::TokenStream { quote! { - errors.merge_self(#field_name_str, (&#field_name).validate()); + if let std::collections::hash_map::Entry::Vacant(entry) = errors.0.entry(#field_name_str) { + errors.merge_self(#field_name_str, (&#field_name).validate()); + } } } From 2799c01e6e6db40b20453b25db8ff55b1e0a6bf8 Mon Sep 17 00:00:00 2001 From: Peerat Vichivanives Date: Mon, 16 Dec 2024 15:19:57 -0800 Subject: [PATCH 2/3] nested test will no longer fail --- validator_derive_tests/tests/nested.rs | 53 +------------------------- 1 file changed, 1 insertion(+), 52 deletions(-) diff --git a/validator_derive_tests/tests/nested.rs b/validator_derive_tests/tests/nested.rs index 3f3b33ee..af1a5438 100644 --- a/validator_derive_tests/tests/nested.rs +++ b/validator_derive_tests/tests/nested.rs @@ -1,6 +1,7 @@ use serde::Serialize; use std::{ borrow::Cow, + cmp::Ordering, collections::{HashMap, HashSet}, }; use validator::{ @@ -847,58 +848,6 @@ fn test_field_validations_take_priority_over_nested_validations() { } } -#[test] -#[should_panic(expected = "Attempt to replace non-empty ValidationErrors entry")] -#[allow(unused)] -fn test_field_validation_errors_replaced_with_nested_validations_fails() { - #[derive(Debug)] - struct ParentWithOverridingStructValidations { - child: Vec, - } - - #[derive(Debug, Validate, Serialize)] - struct Child { - #[validate(length(min = 1))] - value: String, - } - - impl Validate for ParentWithOverridingStructValidations { - // Evaluating structs after fields validations have discovered errors should fail because - // field validations are expected to take priority over nested struct validations - #[allow(unused_mut)] - fn validate(&self) -> Result<(), ValidationErrors> { - // First validate the length of the vector: - let mut errors = ValidationErrors::new(); - if !self.child.validate_length(Some(2u64), None, None) { - let mut err = ValidationError::new("length"); - err.add_param(Cow::from("min"), &2u64); - err.add_param(Cow::from("value"), &&self.child); - errors.add("child", err); - } - - // Then validate the nested vector of structs without checking for existing field errors: - let mut result = if errors.is_empty() { Ok(()) } else { Err(errors) }; - { - let results: Vec<_> = self - .child - .iter() - .map(|child| { - let mut result = Ok(()); - result = ValidationErrors::merge(result, "child", child.validate()); - result - }) - .collect(); - result = ValidationErrors::merge_all(result, "child", results); - } - result - } - } - - let instance = - ParentWithOverridingStructValidations { child: vec![Child { value: String::new() }] }; - instance.validate(); -} - #[test] #[should_panic( expected = "Attempt to add field validation to a non-Field ValidationErrorsKind instance" From 3bb3e92df382490352a23aaaf2c0594d3946d26e Mon Sep 17 00:00:00 2001 From: Peerat Vichivanives Date: Thu, 19 Dec 2024 10:38:45 -0800 Subject: [PATCH 3/3] add back else case --- CHANGELOG.md | 2 -- validator/src/types.rs | 5 ++--- validator_derive_tests/tests/nested.rs | 1 - 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd96d903..ff1810ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,9 @@ ## Changelog - ## Unreleased - Implement `AsRegex` for `std::sync::LazyLock` - Bug fix for nested issue with custom only running nested if outer passes - ## 0.19.0 (2024/11/03) - Swap to using proc-macro-error-2 instead of proc-macro-error for Syn diff --git a/validator/src/types.rs b/validator/src/types.rs index 349979fe..861f2d28 100644 --- a/validator/src/types.rs +++ b/validator/src/types.rs @@ -181,10 +181,9 @@ impl ValidationErrors { fn add_nested(&mut self, field: &'static str, errors: ValidationErrorsKind) { if let Vacant(entry) = self.0.entry(field) { entry.insert(errors); + } else { + panic!("Attempt to replace non-empty ValidationErrors entry"); } - // Else case here is simply do nothing. Pretty sure this is caught earlier to not run but since - // If we get here, just not adding it does the same thing (although waste extra compute) - // probably better to just ignore here. } #[must_use] diff --git a/validator_derive_tests/tests/nested.rs b/validator_derive_tests/tests/nested.rs index af1a5438..ee1abd4c 100644 --- a/validator_derive_tests/tests/nested.rs +++ b/validator_derive_tests/tests/nested.rs @@ -1,7 +1,6 @@ use serde::Serialize; use std::{ borrow::Cow, - cmp::Ordering, collections::{HashMap, HashSet}, }; use validator::{