Keats/validator

Major undocumented silently breaking change to nested validations

kyrias opened this issue · 8 comments

It seems like the new derive implementation completely changed what's necessary for nested validations to work in a way where what previously worked now fails completely silently and there is no mention of this in the changelog. Additionally, the README still recommends the old way which silently fails.

The README still says to just slap #[validate] on a field for nested validation, which now does nothing at all, and that the following structs would return validation errors if there's something wrong in either the ContactDetails or Preference structs, which is no longer the case. Apparently you need to both slap a #[validate(nested)] on the actual child type that you want the validation to nest into and on the field in the parent struct. This is rather confusing and completely undiscoverable, and if you just add #[validate(nested)] to the field you get a completely opaque compiler error.

#[derive(Debug, Validate, Deserialize)]
struct SignupData {
   #[validate]
   contact_details: ContactDetails,
   #[validate]
   preferences: Vec<Preference>,
   #[validate(required)]
   allow_cookies: Option<bool>,
}

#[derive(Debug, Validate, Deserialize)]
struct ContactDetails {
   #[validate(email)]
   mail: String,
}

#[derive(Debug, Validate, Deserialize)]
struct Preference {
   #[validate(length(min = 4))]
   name: String,
   value: bool,
}

Yeah I noticed that when fixing bugs last weekend. That sounds like a bug imo

Given that the tests were changed to work with the new structure it seemed intentional to me:

#[test]
fn fails_nested_validation() {
#[derive(Validate)]
struct Root<'a> {
#[validate(length(min = 5, max = 10))]
value: String,
#[validate(nested)]
a: &'a A,
}
#[derive(Validate)]
#[validate(nested)]
struct A {
#[validate(length(min = 5, max = 10))]
value: String,
#[validate(nested)]
b: B,
}
#[derive(Validate)]
#[validate(nested)]
struct B {
#[validate(length(min = 5, max = 10))]
value: String,
}
let root = Root {
value: "valid".to_string(),
a: &A { value: "invalid value".to_string(), b: B { value: "valid".to_string() } },
};
assert!(root.validate().is_err());
let root = Root {
value: "valid".to_string(),
a: &A { value: "valid".to_string(), b: B { value: "invalid value".to_string() } },
};
assert!(root.validate().is_err());
}

Yes but I didn't notice that so I'd want to revert to the previous behaviour.

So I'm thinking of requiring #[validate(nested)] rather than just #[validate] for nested validation to be explicit on the field (not on the struct). The reason to require the nested part is that it allows giving better error message if you use only #[validate].

What do you think?

So I'm thinking of requiring #[validate(nested)] rather than just #[validate] for nested validation to be explicit on the field (not on the struct). The reason to require the nested part is that it allows giving better error message if you use only #[validate].

What do you think?

I'm fine with that.

I think this is fixed in #304
Hacky fix but the whole error system will be rewritten for 0.19 so i don't want to spend time on it.

Can people try this branch?

Seems to work as expected, and +1 on the compile error telling you what to do.