Rewrite derive macro implementation

[pintariching]

A proposal on rewriting the derive macro implementation using the darling crate. It massively simplifies the macro. The only two validators I got up an working are the length and email just for show. Also there is no error handling for now.

Also I had to upgrade syn version to 2 as darling would work with version 1.

[pintariching]

I've rewritten the custom validation a bit. If you want arguments for your function you can specify them like:

#[derive(Validate)]
struct TestStruct {
    #[validate(custom(
        function = "valid_reference_with_lifetime",
        arg = "len: i64",
        arg = "something: bool",
        arg = "name: String"
    ))]
    value: String,
}

let test = TestStruct { name: "asdfg".to_string() };
let res = test.validate(TestStructArgs { len: 123, something: false, name: "Something".into() });

Also the TestStructArgs could be given a builder pattern if we wanna be fancy, so you get

TestStructArgs::build().len(123).something(false).name("Something").build()

How does this look? I'm now wrangling with lifetimes so for now you can only pass values and not references. Should I continue with this or go back to tuples like it's now?

[Keats]

What do you think of the approach from garde: https://github.com/jprochazk/garde/tree/main#custom-validation ?

[pintariching]

I think I like gardes way more, it feels simpler and more "proper". I would only add the context attribute in the custom attribute, so you can have multiple contexts for different fields and pass them in the validate function with a tuple and lifetimes and generics should also work for example

#[derive(Validate)]
struct Test {
    #[validate(custom(function = "custom_password", context = "PasswordContext")]
    password: String,
    #[validate(custom(function = "custom_name", context = "&mut NameContext<u32>")]
    name: String
}

struct PasswordContext { ... }
struct NameContext<T> { ... }

fn custom_password(value: &str, contex: PasswordContext) -> Result<(), ValidationErrors> { ... }
fn custom_name(value: &str, context: &mut NameContext<u32>) -> Result<(), ValidationErrors> { ... }

let test = Test { password: ..., name: ... };
test.validate((PaswordContext { ... }, { NameContext<u32> { ... }));

[pintariching]

Ok I tried to do something similar to garde but couldn't get the lifetimes to parse with darling, so I tried a different way with closures and I think it's a bit better. Here's an example straight from the tests for convenience:

fn valid_fn(_: String, _arg: i32) -> Result<(), ValidationError> {
    Ok(())
}

#[derive(Validate)]
struct TestStruct {
    #[validate(custom)]
    value: String,
}

let test_struct = TestStruct { value: "Something".to_string() };
assert!(test_struct.validate(|v| valid_fn(v, 123)).is_ok());

The value gets cloned into the closure, that can be changed to a reference maybe with an attribute or to use a reference by default

[Keats]

The issue is that it doesn't work with multiple custom function does it?

[pintariching]

Check the validator_derive_tests::custom_args tests. I have one with multiple custom functions on multiple fields. But I guess I haven't tried multiple custom functions on a single field?

EDIT: This test here

[Keats]

No need to handle multiple custom on a single field i think, but different function on different fields should work

[pintariching]

The only thing I'm unsure of is, I'm defining the ValidateArgs inside the macro , so it's not using the validator::ValidateArgs trait. With this, I think I could ditch the trait and just do an impl block for the struct with the validate function being in it.

[pintariching]

Ok changed a lot of stuff, all the tests pass except validator_derive_tests::complex::failed_validation_points_to_original_field_name. darling doesn't seem to support this out of the box, so I might need to copy over the parsing from the previous version.

[Keats]

Ignore that one for now, because we don't really support all the rename options from serde. Let's get everything working first and then see if we can point back to the original name later.

[Keats]

Have you seen https://github.com/Keats/validator/issues/264 ?

[pintariching]

This might require that we either support one or the other, or require that the regex is passed into the validate function as an argument like the custom validator.

In the macro, I don't think I can extract the type of a variable (Regex or OnceLock<Regex>) from the path crate::REGEX.

I added the ValidateRegex trait so another option would be to use generics instead of Regex like T: Into<Regex> but I'm not sure if OnceLock supports that trait

[pintariching]

I have implemented a change to allow the use of either OnceLock<Regex>, Mutex<OnceCell<Regex>> or a lazy_static macro regex. The only change you have to do, is to dereference the type that is generated by lazy_static, but the others work fine. Also you have to specify the path to the static variable without double qoutes. Check the tests for exapmles.

[pintariching]

I still need to get the UI tests working and support renamed fields, however everything else seems to be working. Also just saw the checks and there seems to be a version mismatch for hashbrown.

[Keats]

Thanks, I'll have a look asap! For the CI it might just be a bump needed for the pinned Rust version

[pintariching]

Had to bump MSRV to 1.70.0, because we use OnceCell and OnceLock.

[Keats]

I had a quick look - that looks good! I don't really understand how the custom validation arguments work though when there are multiple. Is it taking the closure based on the order of the fields?

[pintariching]

Yes it orders the closures in the tuple based on the order of fields. It puts them in a Vec and pushes any schema validation the struct has on the end.

[Keats]

I think it's better to have a single context rather than multiple ones, otherwise nested custom is going to require the user to do their own book-keeping and easy to introduce mistakes when moving/adding/removing fields

[pintariching]

That's something I forgot, nested custom validations don't work right now. I couldn't find a way to get the required arguments for the validate function in a nested struct when doing the proc macro on the parent struct.

[pintariching]

Okay, how about this. Nested custom validations with arguments aren't supported. You can however specify the function name inside the attribute like #[validate(custom(function = foo))] but you can't add arguments to it. If you want arguments, you have to use the attribute #[validate(custom)] and pass them in the validate(...) function call.

[Keats]

I think if you pass a single struct for all validation (which could be likely, eg a db connection and maybe some Option for a specific field), you get nested custom for free?

[pintariching]

I'm not entirely sure what you think. If we pass a single struct into the validate() function and have it used by all custom validations for the current struct and all the children? I see that garde has a similar thing going on.

This could work for nested functions, but all custom functions would have to have it as an argument or accept multiple contexts I think.

[Keats]

I was thinking of something like garde yes. Every custom function would then get it as argument

[pintariching]

Ok I got something working. However now the call validate(()) requires an empty tuple to be passed in everywhere, even if custom validation is not used. I'm not sure how to fix this.

[Keats]

Can we do like currently and have both validate() and validate_with_args() and validate() would just call validate_with_args(())?

[pintariching]

Ok I got it working. Now the only thing that's bugging me is, that I can't implement ValidateNested for both T and &T. Currently the test nested::test_can_validate_option_fields_with_lifetime doesn't pass because of this.

[pintariching]

The custom validation with a given context is giving me quite a bit of trouble. It works now but is very finicky especially with generics and lifetimes. This all stems from the fact that we're implementing a trait, which can't accept different arguments. How about we ditch the Validate and ValidateArgs and impl directly on the struct. We can then name the custom arguments and you can much easier pass in everything you want with closures, ie:

#[derive(Validate)]
struct Test {
    #[validate(custom)]
    val: String
}

fn custom_validation(val: &String, context: &mut SomeContext, index: i32) -> Result<(), ValidationError> { ... }

/// This impl is autogenerated
impl Test {
    impl validate(&self, val_custom_closure: FnOnce(&String) -> Result<(), ValidationError> {
        val_custom_closure(self.val)
    }
}

let some_context = ...;
let index = ...;
// user gets to choose what arguments they want to use
test.validate(|val| custom_validation(val, some_context, index);

The argument fields are all named, so with multiple custom functions, you see the name of each parameter.

Also a builder pattern could be used something along the lines of

test
    .validation_builder()
    .val_custom_validation(|val| ...)
    .other_custom_validation(...)
    .validate();

[Keats]

I don't like the builder pattern. How about we do like currently and only allow a single lifetime?

[pintariching]

Ok I think I got everything working except the serde original field name. The issues this should solve or partially solve are: #264, #241, #236, #205, possibly #191, #176, #172, #170, #83, maybe #73

Now I'm working on the errors and getting all the compile-fail and run-pass tests to work, but otherwise I think this could be used right now.

[Keats]

I'll have a look asap once the tests pass

[pintariching]

The UI tests now pass. I had to put them behind a feature to avoid running them on anything other than stable due to the errors that keep changing and reordering. Also for some reason the error produced by compile-fail::range::wrong_type test was different in the CI and locally, even though both use stable, so I removed it.

[Keats]

Just a couple more tests that are currently commented out, can I merge and uncomment them or is there a reason they are commented out?

[pintariching]

Sorry for the wait, forgot about this. No the tests don't pass currently if you uncomment them I think. I was thinking gating the serde original name behind a feature.

[Keats]

ah it's all related to that feature? You can just remove it for now then

[pintariching]

Ok I removed the original field name from serde test and added back multiple schema validation. I think that makes everything? Also the documentation should be updated, this PR is a breaking change after all.

[Keats]

Let's update the docs later, we might have more things.

Thanks a lot!