Can't validate member struct with multiple members

[abedra]

Say I've got the following definition

#[derive(Serialize, Deserialize, Clone, Debug, Validate)]
pub struct Scenario {
    pub name: String,
    #[validate(range(min=100, message="Sample size must be 100 or greater"))]
    pub sample_size: i32,
    #[validate(range(exclusive_min=0.0, message="Threat Event Frequency must be greater than 0.0"))]
    pub threat_event_frequency: f32,
    #[validate(range(exclusive_min=0.0, message="Probable Loss must be greater than 0.0"))]
    pub probable_loss_magnitude: f32,
    #[validate(range(exclusive_min=0.0, message="Worst Case Loss must be greater than 0.0"))]
    pub worst_case_loss_magnitude: f32,
    #[validate(nested)]
    pub control_strength: ControlStrength,
    pub threat_capability: ThreatCapability,
}

and the following definition for ControlStrength

#[derive(Serialize, Deserialize, Clone, Debug, Validate)]
pub struct ControlStrength {
    #[validate(range(exclusive_min=0.0, message="Control Strength min must be greater than 0.0"))]
    pub min: f32,
    #[validate(range(exclusive_min=0.0, message="Control Strength max must be greater than 0.0"))]
    pub max: f32,
    #[validate(range(exclusive_min=0.0, message="Control Strength most likely must be greater than 0.0"))]
    pub most_likely: f32,
}

Writing a test to validate the failures causes the following error

Attempt to replace non-empty ValidationErrors entry
thread 'types::test::control_strength' panicked at /home/abedra/.cargo/registry/src/index.crates.io-6f17d22bba15001f/validator-0.18.0/src/types.rs:186:13:
Attempt to replace non-empty ValidationErrors entry
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/std/src/panicking.rs:647:5
   1: core::panicking::panic_fmt
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/panicking.rs:72:14
   2: validator::types::ValidationErrors::add_nested
             at /home/abedra/.cargo/registry/src/index.crates.io-6f17d22bba15001f/validator-0.18.0/src/types.rs:186:13
   3: validator::types::ValidationErrors::merge_self
             at /home/abedra/.cargo/registry/src/index.crates.io-6f17d22bba15001f/validator-0.18.0/src/types.rs:78:25
   4: <fair_api::types::Scenario as validator::traits::ValidateArgs>::validate_with_args
             at ./src/types.rs:35:48
   5: <fair_api::types::Scenario as validator::traits::Validate>::validate
             at ./src/types.rs:35:48
   6: fair_api::types::test::control_strength
             at ./src/types.rs:163:19
   7: fair_api::types::test::control_strength::{{closure}}
             at ./src/types.rs:155:26

If I'm reading things correctly, it's trying to replace the error for the field instead of accumulating errors for the field.

[Keats]

~With 0.18?~ Nevermind, saw it in the crate path It shouldn't be too hard to fix if someone wants to try but wee need to add a test first

[kyrias]

Hmm, the logic in merge_self seems very strange. For every ValidationErrorsKind::Field it's called with, it tries to add all of the errors, so if you have two failing fields, it tries to add both fields twice.

It seems like different parts of the library expects it to function in very different ways though, and since it doesn't have any comments it's not clear what's the right way to use it. Based on the function signatures I'd expect merge and merge_self to do the same thing and just differ in whether it mutates self or takes ownership of the parent, but they behave completely differently, hmm..

[Keats]

The plan for the next version is to completely revamp the error system since tbh I don't understand it anymore but it would be nice to fix it temporarily.

[kyrias]

Say you have the following test case for example:

#[test]
fn foo() {
    #[derive(Validate)]
    struct Root<'a> {
        #[validate(length(min = 5, max = 10))]
        value: String,
        #[validate(nested)]
        a: &'a A,
    }

    #[derive(Validate)]
    struct A {
        #[validate(length(min = 5, max = 10))]
        value1: String,
        #[validate(length(min = 5, max = 10))]
        value2: String,
    }

    let root = Root {
        value: "valid".to_string(),
        a: &A { value1: "invalid value".to_string(), value2: "invalid value".to_string() },
    };

    assert!(root.validate().is_err());
}

This ends up calling merge_self with

[validator/src/types.rs:73:9] &field = "a"
[validator/src/types.rs:73:9] &child = Err(
    ValidationErrors(
        {
            "value2": Field(
                [
                    ValidationError {
                        code: "length",
                        message: None,
                        params: {
                            "min": Number(5),
                            "max": Number(10),
                            "value": String("invalid value"),
                        },
                    },
                ],
            ),
            "value1": Field(
                [
                    ValidationError {
                        code: "length",
                        message: None,
                        params: {
                            "max": Number(10),
                            "value": String("invalid value"),
                            "min": Number(5),
                        },
                    },
                ],
            ),
        },
    ),
)

This is of course simple enough to do by just calling add_self as per merge. But this completely breaks how the collections are implemented.

If we instead have the following test case:

#[test]
fn foo() {
    #[derive(Validate)]
    struct Root<'a> {
        #[validate(length(min = 5, max = 10))]
        value: String,
        #[validate(nested)]
        a: &'a [A],
    }

    #[derive(Validate)]
    struct A {
        #[validate(length(min = 5, max = 10))]
        value: String,
    }

    let root = Root {
        value: "valid".to_string(),
        a: &[A { value: "invalid value".to_string() }, A { value: "invalid value".to_string() }],
    };

    assert!(root.validate().is_err());
}

then merge_self gets called with

[validator/src/types.rs:73:9] &field = "a"
[validator/src/types.rs:73:9] &child = Err(
    ValidationErrors(
        {
            "_tmp_validator": List(
                {
                    0: ValidationErrors(
                        {
                            "value": Field(
                                [
                                    ValidationError {
                                        code: "length",
                                        message: None,
                                        params: {
                                            "max": Number(10),
                                            "value": String("invalid value"),
                                            "min": Number(5),
                                        },
                                    },
                                ],
                            ),
                        },
                    ),
                    1: ValidationErrors(
                        {
                            "value": Field(
                                [
                                    ValidationError {
                                        code: "length",
                                        message: None,
                                        params: {
                                            "min": Number(5),
                                            "value": String("invalid value"),
                                            "max": Number(10),
                                        },
                                    },
                                ],
                            ),
                        },
                    ),
                },
            ),
        },
    ),
)

Because the collection validation calls return a ValidationErrors with a fake name ("_tmpvalidator"), and then we're expected to strip off that outer part, which we're not expected to do for non-collection nested validation.

I'm honestly not quite sure of how to fix this. I guess we'd have to explicitly check for the presence of _tmp_validator, but that's rather ugly? :/

[kyrias]

Welp, I guess in typing all of that out I figured out the only way to fix it with the way the types and collection support currently works. It's definitely ugly, but on the other hand I'd argue that [...].validate() returning something containing a _tmp_validator entry is also pretty ugly, so what can we do.