Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Struct-level custom rule #61

Open
enerdgumen opened this issue Aug 4, 2023 · 11 comments · May be fixed by #144
Open

Struct-level custom rule #61

enerdgumen opened this issue Aug 4, 2023 · 11 comments · May be fixed by #144
Labels
enhancement New feature or request

Comments

@enerdgumen
Copy link

As far as I understand, it's not possible to derive the validation trait and define a validator at struct level at the same time (for example, to check that different fields are consistent with each other).

You can implement the trait yourself, but then you cannot take advantage of the derive feature if you have other validations in that struct.

It would be nice to have a feature like the one provided by the validator crate (see struct level validation).

@jprochazk
Copy link
Owner

for example, to check that different fields are consistent with each other

This would be possible with #2

Is there a different use-case for container-level rules besides field matching?

Only other thing I can think of is mutually-exclusive Option fields:

struct Test {
   // at least one of `foo`/`bar` must be `Some`
   // but not both at the same time
   foo: Option<u32>,
   bar: Option<u32>,
}

@jprochazk jprochazk added the enhancement New feature or request label Aug 4, 2023
@enerdgumen
Copy link
Author

I'm thinking of a more general approach.

In my use case I have two amounts and I need to ensure that one is lower than the other, for example:

struct Test {
  principal: i32,
  // deposit should be in range 0..principal
  deposit: i32,
  ...
}

So this particular case could also be supported by the range rule if it could refer to other fields in the struct.

@jprochazk
Copy link
Owner

jprochazk commented Aug 4, 2023

Your use-case is already supported, although completely by accident:

#[derive(garde::Validate)]
#[garde(allow_unvalidated)]
struct Test {
    principal: i32,
    #[garde(range(min=0, max=self.principal))]
    deposit: i32,
}
Derive macro expansion
impl ::garde::Validate for Foo {
    type Context = ();
    #[allow(clippy::needless_borrow)]
    fn validate(
        &self,
        __garde_user_ctx: &Self::Context,
    ) -> ::core::result::Result<(), ::garde::error::Errors> {
        ({
            let Self { deposit, .. } = self;
            ::garde::error::Errors::fields(|__garde_errors| {
                __garde_errors.insert("deposit", {
                    let __garde_binding = &*deposit;
                    ::garde::error::Errors::simple(|__garde_errors| {
                        if let Err(__garde_error) = (::garde::rules::range::apply)(
                            &*__garde_binding,
                            (Some(0), Some(self.principal)),
                        ) {
                            __garde_errors.push(__garde_error)
                        }
                    })
                });
            })
        })
        .finish()
    }
}

You can expect this to continue to work, even if it's not "advertised". I added a test for it specifically here: dfa1fed

@jprochazk
Copy link
Owner

I think I'm generally against adding container-level validation if the use-cases can be served by more specific rules. The case of "I want custom validation on the container" should only be supported by manually implementing Validate.

@enerdgumen
Copy link
Author

Your use-case is already supported

Oh great, thanks!

I think I'm generally against adding container-level validation if the use-cases can be served by more specific rules.

Ok, but in general it could be a nice feature to have, as there could be cases where you cannot perform the check using the existing rules, and if you implement the Validate trait manually, you cannot derive it anymore.

Just an option, what about supporting a custom validation rule at struct level?

#[derive(garde::Validate)]
#[garde(allow_unvalidated)]
#[garde(custom(validate_things)) // the validate_things would receives the whole struct
struct Test {
    principal: i32,
    #[garde(range(min=0, max=self.principal))]
    deposit: i32,
    ...
}

I don't know if it's feasible, but it seems like a natural approach.

@jprochazk
Copy link
Owner

jprochazk commented Aug 4, 2023

you cannot derive it anymore

The fact that you can't derive parts of that Validate impl seems reasonable to me. Everything in this library is built with extensibility in mind, so there's an expectation that if Garde doesn't support your use-case exactly, then you implement it yourself, or open a feature request (preferably the latter, or both).

I used GitHub code search to find usages of validator's version of this feature. Most of it was for field matching, some for checking mutual exclusivity of Option fields, and a terrifying amount of fully manual implementations of the validation logic, maybe because they wanted early-out semantics. It was definitely not an exhaustive search, but I believe there aren't any use-cases we couldn't support by adding a new rule or two.

I don't know if it's feasible, but it seems like a natural approach.

It is feasible, and it is a natural approach, but in terms of the implementation, it means:

  • Supporting an extra kind of error (because there's no way to emit a "container-level" error that isn't attached to a field or array index)
  • Adding an extra path to the top-level emit code in the derive macro, which would contribute to a combinatorial explosion of sorts (struct/enum + custom/no-custom), and would require at least doubling the size of the test suite

After thinking about it some more, I'm going to close this as wontfix, but thank you for the suggestion!

@jprochazk jprochazk closed this as not planned Won't fix, can't repro, duplicate, stale Aug 4, 2023
@omid
Copy link

omid commented Jun 20, 2024

I also faced this issue... In my case, I have multiple arrays and hashmaps as properties of the struct. And there should be at least one element in any of them.

struct X {
  a: Vec<i32>,
  b: MyVec<i32>,
  c: HashMap<i32, i32>,
}

@jprochazk
Copy link
Owner

It's not pretty, but you can work around the lack of struct-level validation using a newtype and a custom rule on the inner type:

#[derive(garde::Validate)]
#[garde(transparent)]
struct X(#[garde(custom(at_least_1))] Inner);

struct Inner {
    a: Vec<i32>,
    b: Vec<i32>,
    c: Vec<i32>,
}

fn at_least_1(v: &Inner, _: &()) -> garde::Result {
    if v.a.is_empty() && v.b.is_empty() && v.c.is_empty() {
        Err(garde::Error::new("at least one element must be present"))
    } else {
        Ok(())
    }
}

Now that I think about it, things have changed since this issue was initially opened. It is now straightforward to create a struct-level error, the error path would just be empty. The emit for this should also be pretty simple, call the custom rule here if present:

#ty

@jprochazk jprochazk reopened this Jun 20, 2024
@jprochazk jprochazk changed the title Feature request: struct level validation Struct-level custom rule Jun 20, 2024
@omid
Copy link

omid commented Jun 20, 2024

Is it possible to bypass it similar to this?

#[derive(garde::Validate)]
struct X {
    #[garde(custom(at_least_1(&self)))]
    a: Vec<i32>,
    b: Vec<i32>,
    c: Vec<i32>,
}

fn at_least_1(_self: &X) -> impl FnOnce(&X, &()) -> garde::Result + '_ {
    move |_self, _ctx| {
        Ok(())
    }
}

@jprochazk
Copy link
Owner

I'm not sure if that would do what you want, the resulting error's path will contain the a field, value.a: at least one element must be present

#[derive(garde::Validate)]
struct X {
    #[garde(custom(at_least_1(&self)))]
    a: Vec<i32>,
    #[garde(skip)]
    b: Vec<i32>,
    #[garde(skip)]
    c: Vec<i32>,
}

fn at_least_1<'a, T>(v: &'a X) -> impl FnOnce(&T, &()) -> garde::Result + 'a {
    move |_, _| {
        if v.a.is_empty() && v.b.is_empty() && v.c.is_empty() {
            Err(garde::Error::new("at least one element must be present"))
        } else {
            Ok(())
        }
    }
}

@omid
Copy link

omid commented Jun 20, 2024

Thanks a lot 🙏🏼
That's better than manually writing a validation function and call two validation methods.

@lasantosr lasantosr linked a pull request Jan 21, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants