Issue: Panic when trying to use custom validators combined with nested validators

[gonzedge]

While trying to use a combination of nested and custom validators, I've been getting panics with the message Attempt to replace non-empty ValidationErrors entry.

I've boiled down the failing case to this minimum setup (contained within this PR):

validator/validator_derive_tests/tests/nested.rs

Lines 85 to 130 in 1728fd7

	#[test]
	fn fails_nested_validation_multiple_members_and_custom_validations() {
	#[derive(Validate)]
	struct Root {
	#[validate(length(min = 5, max = 10))]
	value: String,
	#[validate(nested, custom(function = "all_value1s_are_unique"))]
	a: Vec<A>,
	}
	#[derive(Serialize, Validate)]
	struct A {
	#[validate(length(min = 5, max = 10))]
	value1: String,
	#[validate(length(min = 5, max = 10))]
	value2: String,
	}
	// Top-level custom validation
	fn all_value1s_are_unique(items: &[A]) -> Result<(), ValidationError> {
	let unique_value1s: HashSet<String> =
	HashSet::from_iter(items.iter().map(|a| a.value1.to_string()));
	match unique_value1s.len().cmp(&items.len()) {
	Ordering::Equal => Ok(()),
	_ => Err(ValidationError::new("not all value1s are unique")),
	}
	}
	let root = Root {
	value: "valid".to_string(),
	// "a" should be invalid because multiple items have the same "value1"
	// but also "a[1].value2" should be invalid because it is too long.
	a: vec![
	A { value1: "non unique".to_string(), value2: "a value 2".to_string() },
	A { value1: "non unique".to_string(), value2: "b value 2 too long".to_string() },
	A { value1: "unique-ish".to_string(), value2: "c value 2".to_string() },
	],
	};
	// This is currently panicking with "Attempt to replace non-empty ValidationErrors entry"
	let result = root.validate();
	// Not sure what the correct full assertion should be,
	// but the panic isn't letting me get any further.
	assert_ne!(result, Ok(()))
	}

I'm trying to get familiar with the code, but have been unable to correct the issue so far. Any help would be appreciated.

---

Full panic stack backtrace:

Attempt to replace non-empty ValidationErrors entry
thread 'fails_nested_validation_multiple_members_and_custom_validations' panicked at validator/src/types.rs:185:13:
Attempt to replace non-empty ValidationErrors entry
stack backtrace:
   0: rust_begin_unwind
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:662:5
   1: core::panicking::panic_fmt
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:74:14
   2: validator::types::ValidationErrors::add_nested
             at <redacted>/validator/validator/src/types.rs:185:13
   3: validator::types::ValidationErrors::merge_self
             at <redacted>/tools/validator/validator/src/types.rs:80:21
   4: <nested::fails_nested_validation_multiple_members_and_custom_validations::Root as validator::traits::ValidateArgs>::validate_with_args
             at ./tests/nested.rs:87:14
   5: <nested::fails_nested_validation_multiple_members_and_custom_validations::Root as validator::traits::Validate>::validate
             at ./tests/nested.rs:87:14
   6: nested::fails_nested_validation_multiple_members_and_custom_validations
             at ./tests/nested.rs:125:18
   7: nested::fails_nested_validation_multiple_members_and_custom_validations::{{closure}}
             at ./tests/nested.rs:86:69
   8: core::ops::function::FnOnce::call_once
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/ops/function.rs:250:5
   9: core::ops::function::FnOnce::call_once
             at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/ops/function.rs:250:5

[Keats]

It's in /~https://github.com/Keats/validator/blob/master/validator/src/types.rs#L181-L187 but honestly the whole error types need to be rewritten, it's super confusing imo

[pvichivanives]

Interesting, I have a pretty similar case that doesn't panic. Will experiment it a bit more to see if I can find out why and how to get a solution.

So far I got that it has to be multiple validates in the nested.

[gonzedge]

but honestly the whole error types need to be rewritten, it's super confusing imo

LOL. My initial thought is: would it be possible to use the list type in all cases and make it so that any resulting errors (whether one or multiple, nested or not) can be appended to a list within the underlying hash map?

I took a stab at it but it got outta hand pretty quickly.

[pvichivanives]

So.. I think I have a solution (note that child is a quick and dirty test just so I can test it out).

This does have the issue though of conflicting with a different field called a_child.

Seems like options are:

1. Stick a word at the end like a_nested which will fix most cases and note it in the documentation that this is an issue currently being worked on.

Pros:

• Easy to implement
• Readable

Cons:

• Still leaves a bug, not long term solution

2. Stick a random string (UUID?) at the end.

Pros:

• Easy to implement
• Solves with extremely high probability for every use case

Cons:

• Means people validation errors see a random string appended to their field.

3. Write Join Logic for the merge_self:

Pros:
-Clean and permament

Cons:

• Would add more levels of infrastructure

IMO 3 is the way to go for a long term fix but putting out the options out there (we could put out 1 asap as a mitigation for people first if we want)

For ref code expansion to trace this in case you spot something out

fn fails_nested_validation_multiple_members_and_custom_validations() {
    struct Root {
        #[validate(custom(function = "all_value1s_are_unique"), nested)]
        a: Vec<A>,
    }
    impl ::validator::Validate for Root {
        fn validate(&self) -> ::std::result::Result<(), ::validator::ValidationErrors> {
            use ::validator::ValidateArgs;
            self.validate_with_args(())
        }
    }
    impl<'v_a> ::validator::ValidateArgs<'v_a> for Root {
        type Args = ();
        fn validate_with_args(
            &self,
            args: Self::Args,
        ) -> ::std::result::Result<(), ::validator::ValidationErrors> {
            let mut errors = ::validator::ValidationErrors::new();
            match all_value1s_are_unique(&self.a) {
                ::std::result::Result::Ok(()) => {}
                ::std::result::Result::Err(mut err) => {
                    err.add_param(::std::borrow::Cow::from("value"), &&self.a);
                    errors.add("a", err);
                }
            }
            errors.merge_self("a", (&self.a).validate());
            if errors.is_empty() {
                ::std::result::Result::Ok(())
            } else {
                ::std::result::Result::Err(errors)
            }
        }
    }
    struct A {
        value1: String,
        #[validate(length(min = 5, max = 10))]
        value2: String,
    }
    #[doc(hidden)]
    #[allow(non_upper_case_globals, unused_attributes, unused_qualifications)]
    const _: () = {
        #[allow(unused_extern_crates, clippy::useless_attribute)]
        extern crate serde as _serde;
        #[automatically_derived]
        impl _serde::Serialize for A {
            fn serialize<__S>(
                &self,
                __serializer: __S,
            ) -> _serde::__private::Result<__S::Ok, __S::Error>
            where
                __S: _serde::Serializer,
            {
                let mut __serde_state = _serde::Serializer::serialize_struct(
                    __serializer,
                    "A",
                    false as usize + 1 + 1,
                )?;
                _serde::ser::SerializeStruct::serialize_field(
                    &mut __serde_state,
                    "value1",
                    &self.value1,
                )?;
                _serde::ser::SerializeStruct::serialize_field(
                    &mut __serde_state,
                    "value2",
                    &self.value2,
                )?;
                _serde::ser::SerializeStruct::end(__serde_state)
            }
        }
    };
    impl ::validator::Validate for A {
        fn validate(&self) -> ::std::result::Result<(), ::validator::ValidationErrors> {
            use ::validator::ValidateArgs;
            self.validate_with_args(())
        }
    }
    impl<'v_a> ::validator::ValidateArgs<'v_a> for A {
        type Args = ();
        fn validate_with_args(
            &self,
            args: Self::Args,
        ) -> ::std::result::Result<(), ::validator::ValidationErrors> {
            use ::validator::ValidateLength;
            let mut errors = ::validator::ValidationErrors::new();
            if !self.value2.validate_length(Some(5), Some(10), None) {
                let mut err = ::validator::ValidationError::new("length");
                err.add_param(::std::borrow::Cow::from("min"), &5);
                err.add_param(::std::borrow::Cow::from("max"), &10);
                err.add_param(::std::borrow::Cow::from("value"), &self.value2);
                errors.add("value2", err);
            }
            if errors.is_empty() {
                ::std::result::Result::Ok(())
            } else {
                ::std::result::Result::Err(errors)
            }
        }
    }
    fn all_value1s_are_unique(items: &[A]) -> Result<(), ValidationError> {
        let unique_value1s: HashSet<String> = HashSet::from_iter(
            items.iter().map(|a| a.value1.to_string()),
        );
        match unique_value1s.len().cmp(&items.len()) {
            Ordering::Equal => Ok(()),
            _ => Err(ValidationError::new("not all value1s are unique")),
        }
    }
    let root = Root {
        a: <[_]>::into_vec(
            #[rustc_box]
            ::alloc::boxed::Box::new([
                A {
                    value1: "non unique".to_string(),
                    value2: "a value 2".to_string(),
                },
                A {
                    value1: "non unique".to_string(),
                    value2: "b value 2 too long".to_string(),
                },
                A {
                    value1: "unique-ish".to_string(),
                    value2: "c value 2".to_string(),
                },
            ]),
        ),
    };
    let result = root.validate();
    match (&result, &Ok(())) {
        (left_val, right_val) => {
            if *left_val == *right_val {
                let kind = ::core::panicking::AssertKind::Ne;
                ::core::panicking::assert_failed(
                    kind,
                    &*left_val,
                    &*right_val,
                    ::core::option::Option::None,
                );
            }
        }
    }
}

<\details>

[pvichivanives]

Option 3 was much easier to implement than expected: #359 I didn't include the test since it should be merged from this PR but I did copy and paste it and it runs fine.