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

Support #[serde(validate = "function")] for container, compatible with validator crate #2891

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

zao111222333
Copy link

@zao111222333 zao111222333 commented Feb 11, 2025

This PR aims to sovle #939, #642

The basic idea is only specify validate function in container attribute, since it is the most general way.
One can directly use validator::Validate::validate as validation function, so it is not necessary to have field validator in serde.
The Error needs to be impl Display, to convert it by serde::de::Error::custom.

For general usage, one can use arbitrary fn (&T) -> Result<(), impl Display> function to validate.

#[derive(Deserialize)]
#[serde(validate = "validate_struct")]
struct Struct {
    a: u16,
}

fn validate_struct(deserialized: &Struct) -> Result<(), impl Display> {
    if deserialized.a == 0 {
        return Err("field `a` can not be zero");
    }
    Ok(())
}

And for validator user, there is no extra overhead to do validate.

#[derive(validator::Validate, Deserialize)]
#[serde(validate = "validator::Validate::validate")]
struct ValidatorDemo {
    #[validate(email)]
    mail: String,
}

The full unit-test & demo see here.
I am glad to write documents if this idea looks good to you :)

@zao111222333
Copy link
Author

And I notice that the fn (&T) -> Result<(), impl Display> way is also compatible with https://github.com/ohkami-rs/serdev

Copy link
Member

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll need to read up on the issue discussions and the extent to which we may want to simplify validator crate support

@zao111222333
Copy link
Author

I'll need to read up on the issue discussions and the extent to which we may want to simplify validator crate support

The most user friendly API should like that

#[derive(Deserialize)]
#[serde(validator)]
struct ValidatorDemo {
    #[validate(email)]
    mail: String,
}

However it needs to expand into follows, assume the validator is a block box to us, and we only wrap it up

#[derive(validator::Validate, Deserialize)]
#[serde(validate = "validator::Validate::validate")]
struct ValidatorDemo {
    #[validate(email)]
    mail: String,
}

And I think it is impossible to make the Deserialize proc_macro_derive to introduce another proc_macro_derive for the same container ...

@oli-obk
Copy link
Member

oli-obk commented Feb 12, 2025

I think it's ok for Validate to be required in the list of derives, but I do like the idea of an emtpy serde(validator) to just expand to validator::Validate::validate and hope that it's in scope.

@zao111222333
Copy link
Author

Great, I will do that

@zao111222333
Copy link
Author

Now we have two ways to use validator:

#[derive(validator::Validate, Deserialize)]
#[serde(validate = "validator::Validate::validate")]
struct ValidateStruct {
    #[validate(email)]
    mail: String,
}

#[derive(validator::Validate, Deserialize)]
#[serde(validator)]
struct ValidatorStruct {
    #[validate(email)]
    mail: String,
}

And it will send error message when both serde(validator) and serde(validate = "...") are defined.
I add complie failed tests in test_suite/tests/ui/validate.

Comment on lines 4 to 5
#[serde(validate = "validator::Validate::validate")]
#[serde(validator)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not really wrong to have two validation schemes running at the same time. serde(validate = "..") could be specified twice, too (add a test?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean user can define a list of validation schemes?
I think that's a good idea.

@zao111222333
Copy link
Author

Now there can be multiple validations, I add two test cases

  1. Multiple serde(validate = "..."), see here
  2. Both serde(validate = "...") and serde(validator), see here

@Mingun
Copy link
Contributor

Mingun commented Feb 13, 2025

What the advantage of using #[serde(validator)] over #[serde(validate = "...")]? It seems to me that this is unnecessary introduction of implicit dependency.

Because Rust already supports using paths in attributes, why you choosed using string for validate values instead of a path?

.iter()
.map(|validate| {
quote! {
#validate(&body).map_err(#serde::de::Error::custom)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it is better to use From trait here and below?

Copy link
Author

@zao111222333 zao111222333 Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the signature of validation should be fn <E: serde::de::Error>(&T) -> Result<(), impl Into<E>>?

In this case, if we want to call #[serde(validate = "validator::Validate::validate")] to use validator, the only way is to add follow code within serde, since Rust only allow local impl for trait E

impl<E: serde::de::Error> From<validator::ValidationErrors> for E { }

It will introduce a new dependence for serde, I don't know if we can do that

Well, there are only serde::de::Error::cutom and serde::de::Error::invalid_value that are suitable for validation, I think just use serde::de::Error::cutom is a good approach :)

@zao111222333
Copy link
Author

zao111222333 commented Feb 13, 2025

Hi @Mingun, thank you for your review.

What the advantage of using #[serde(validator)] over #[serde(validate = "...")]? It seems to me that this is unnecessary introduction of implicit dependency.

#[serde(validator)] is the short of #[serde(validate = "validator::Validate::validate")]
#[serde(validate = "...")] is the most general way, and I am open for whether or not use #[serde(validator)]

Because Rust already supports using paths in attributes, why you choosed using string for validate values instead of a path?

Considering the user interface consistency, I just reuse the serialize_with's internal APIs.
If you think we can use a path to do #[serde(validate = ...)], I can change to it.

@zao111222333
Copy link
Author

Hi, I am wondering is there any suggestions for this PR? So that I can modify under that.
Take your time and thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants