Keats / validator

Simple validation for Rust structs
MIT License
1.91k stars 140 forks source link

Major undocumented silently breaking change to nested validations #307

Open kyrias opened 4 months ago

kyrias commented 4 months ago

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,
}
Keats commented 4 months ago

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

kyrias commented 4 months ago

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

https://github.com/Keats/validator/blob/53f22b8145b8fba3450ce5dcb6fe133970a11406/validator_derive_tests/tests/nested.rs#L44-L83

Keats commented 4 months ago

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

Keats commented 4 months ago

cc @pintariching

Keats commented 4 months ago

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?

jvanz commented 4 months ago

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.

Keats commented 4 months ago

I think this is fixed in https://github.com/Keats/validator/pull/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?

kyrias commented 4 months ago

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