Skip to content

[WIP]Move FormField's custom option validation to field construction #304

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.

Sign up for GitHub

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

[WIP]Move FormField's custom option validation to field construction #304

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3036946
Move OptionField validation to Option Initialization
ElijahAhianyo Apr 29, 2025
8f7f34b
add a validation test for email
ElijahAhianyo Apr 29, 2025
ed7a058
remove dead test
ElijahAhianyo Apr 29, 2025
13ff380
Merge branch 'master' into validate_options
ElijahAhianyo Apr 29, 2025
fd69bff
add some docs to please clippy
ElijahAhianyo Apr 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 99 additions & 26 deletions cot/src/form/fields.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,14 @@ macro_rules! impl_form_field {
fn with_options(
options: FormFieldOptions,
custom_options: Self::CustomOptions,
) -> Self {
Self {
) -> Self
where Self: Sized
{
custom_options.validate_options()
.unwrap_or_else(|err| {
panic!("Could not initialize {}: {}", stringify!($field_options_type_name), err)
});
Self {
options,
custom_options,
value: None,
Expand All @@ -55,6 +61,58 @@ macro_rules! impl_form_field {
};
}

/// A trait for validating form field options.
///
/// This trait is used to validate the custom options for form fields when they
/// are created. It ensures that the options provided are valid before a form
/// field is constructed.
///
/// Implementing this trait allows for custom validation logic to be applied to
/// field options.
///
/// # Examples
///
/// ```
/// use cot::form::fields::ValidateOptions;
/// use cot::form::{FormFieldOptions, FormFieldValidationError};
///
/// struct MyFieldOptions {
/// min: Option<u32>,
/// max: Option<u32>,
/// }
///
/// impl ValidateOptions for MyFieldOptions {
/// fn validate_options(&self) -> Result<(), FormFieldValidationError> {
/// if let (Some(min), Some(max)) = (self.min, self.max) {
/// if min > max {
/// return Err(FormFieldValidationError::from_string(format!(
/// "min ({min}) cannot be greater than max ({max})"
/// )));
/// }
/// }
/// Ok(())
/// }
/// }
/// ```
pub trait ValidateOptions {
Copy link
Member

@m4tx m4tx May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think adding a new trait for this isn't ideal. The problem is that it has to be implemented for each Options struct, and it's not even very useful for the end users—note we only use it in a helper macro (not even a public one). This means that the documentation "The validation is performed when FormField::with_options is called" is not really true, as you can always provide your own with_options implementation.

I can see the following alternatives, in the order of my preference:

  1. Changing the macro so that it can optionally call validate() or some similar method (not defined in any trait) on the options struct, if needed (the macro call would change to something like impl_form_field!(StringField, StringFieldOptions, "a string", with_validate) for the structs that need to be validated, fro instance.
  2. Making the trait private (there is no promise for the users, so there's no problem, but we still have to add impl for every options struct)
  3. Making the helper macro public (but there still is no obligation to use the macro, so the trait promise is still broken)

What do you think?

Copy link
Member

@seqre seqre May 31, 2025

Choose a reason for hiding this comment

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

I like option 2 the most and it might not require as much work in the end, we could incorporate the default implementation into the impl_form_field! macro, but leave an escape hatch for custom validation.

Go from this:

macro_rules! impl_form_field {
    ($field_type_name:ident, $field_options_type_name:ident, $purpose:literal $(, $generic_param:ident $(: $generic_param_bound:ident $(+ $generic_param_bound_more:ident)*)?)?) => {
        ...original impl...
}

To this:

macro_rules! impl_form_field {
    ($field_type_name:ident, $field_options_type_name:ident, $purpose:literal $(, $generic_param:ident $(: $generic_param_bound:ident $(+ $generic_param_bound_more:ident)*)?)?) => {
        impl_form_field!(...arguments from above..., custom-validation)

        impl ValidateOptions for $field_options_type_name {}
}
    ($field_type_name:ident, $field_options_type_name:ident, $purpose:literal $(, $generic_param:ident $(: $generic_param_bound:ident $(+ $generic_param_bound_more:ident)*)?)?, custom-validation) => {
        ...original impl...
}

Copy link
Member

@m4tx m4tx May 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@seqre how do you specify custom validation code with your approach? ah, I see the custom-validation part. In this case, it's okay (although I don't have a strong preference of 1 or 2).

/// Validates the form field options when a field is created.
///
/// This method is used to check that the provided options are valid and
/// consistent before a form field is constructed. The validation is
/// performed when `FormField::with_options` is called.
///
/// The default implementation performs no validation and always returns
/// `Ok(())`.
///
/// # Errors
///
/// Returns an error if the options are invalid or inconsistent with each
/// other.
fn validate_options(&self) -> Result<(), FormFieldValidationError> {
Ok(())
}
}

impl_form_field!(StringField, StringFieldOptions, "a string");

/// Custom options for a [`StringField`].
Expand Down Expand Up @@ -84,6 +142,8 @@ impl Display for StringField {
}
}

impl ValidateOptions for StringFieldOptions {}

impl HtmlSafe for StringField {}

impl AsFormField for String {
Expand Down Expand Up @@ -153,6 +213,7 @@ impl Display for PasswordField {
}
}

impl ValidateOptions for PasswordFieldOptions {}
impl HtmlSafe for PasswordField {}

impl AsFormField for Password {
Expand Down Expand Up @@ -273,6 +334,19 @@ impl AsFormField for Email {
}
}

impl ValidateOptions for EmailFieldOptions {
fn validate_options(&self) -> Result<(), FormFieldValidationError> {
if let (Some(min), Some(max)) = (self.min_length, self.max_length) {
if min > max {
return Err(FormFieldValidationError::from_string(format!(
"min_length ({min}) exceeds max_length ({max})"
)));
}
}
Ok(())
}
}

impl HtmlSafe for EmailField {}

impl_form_field!(IntegerField, IntegerFieldOptions, "an integer", T: Integer);
Expand Down Expand Up @@ -319,6 +393,8 @@ impl<T: Integer> Display for IntegerField<T> {
}
}

impl<T: Integer> ValidateOptions for IntegerFieldOptions<T> {}

impl<T: Integer> HtmlSafe for IntegerField<T> {}

/// A trait for numerical types that optionally have minimum and maximum values.
Expand Down Expand Up @@ -472,6 +548,7 @@ impl Display for BoolField {
}
}

impl ValidateOptions for BoolFieldOptions {}
impl HtmlSafe for BoolField {}

/// Implementation of `AsFormField` for `bool`.
Expand Down Expand Up @@ -842,30 +919,6 @@ mod tests {
));
}

#[test]
fn email_field_clean_invalid_length_options() {
let mut field = EmailField::with_options(
FormFieldOptions {
id: "email_test".to_owned(),
name: "email_test".to_owned(),
required: true,
},
EmailFieldOptions {
min_length: Some(50),
max_length: Some(10),
},
);

field.set_value(Cow::Borrowed("user@example.com"));
let result = Email::clean_value(&field);

assert!(result.is_err());
if let Err(err) = result {
let msg = err.to_string();
assert!(msg.contains("min_length") && msg.contains("exceeds max_length"));
}
}

#[test]
fn integer_field_clean_value() {
let mut field = IntegerField::<i32>::with_options(
Expand Down Expand Up @@ -900,4 +953,24 @@ mod tests {
let value = bool::clean_value(&field).unwrap();
assert!(value);
}

#[test]
#[should_panic(
expected = "Could not initialize EmailFieldOptions: min_length (50) exceeds max_length (10)"
)]
fn email_field_validate_options() {
let bad_opts = EmailFieldOptions {
min_length: Some(50),
max_length: Some(10),
};

let _ = EmailField::with_options(
FormFieldOptions {
id: "foo".into(),
name: "foo".into(),
required: true,
},
bad_opts,
);
}
}
Loading