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

Add number validation #109

Closed
wants to merge 12 commits into from
Closed

Conversation

justinmmott
Copy link

also adds in multiple_of support for default values

@justinmmott
Copy link
Author

Wanted to get something of the effect of this change in before working on the bounded numbers todo mentioned in the README

@justinmmott justinmmott changed the title Add number validation that matches integer Add number validation Sep 18, 2022
@justinmmott
Copy link
Author

Wanted to get something of the effect of this change in before working on the bounded numbers todo mentioned in the README

Added the bounded numbers

@justinmmott justinmmott marked this pull request as draft September 18, 2022 16:49
@justinmmott justinmmott marked this pull request as ready for review September 18, 2022 21:20
typify-impl/src/value.rs Outdated Show resolved Hide resolved
typify-impl/src/value.rs Outdated Show resolved Hide resolved
@justinmmott justinmmott marked this pull request as draft September 19, 2022 17:58
@ahl
Copy link
Collaborator

ahl commented Sep 19, 2022

Thanks for taking a swing here; I'm a little underwater, but will do my best to take a look this week!

@justinmmott
Copy link
Author

Thanks for taking a swing here; I'm a little underwater, but will do my best to take a look this week!

Awesome thanks! I realized that what I wrote here doesn't work as I intended it to. But I think I have a solution that I will hopefully fix that. Hoping to push it later tonight

(TypeEntry::new_integer(ty), metadata)
} else {
(TypeEntry::new_float(ty), metadata)
});
}
Copy link
Author

Choose a reason for hiding this comment

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

I couldn't really tell what this was for?

Copy link
Author

Choose a reason for hiding this comment

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

This is in reference the the deleted mutations of max and min

@justinmmott justinmmott marked this pull request as ready for review September 22, 2022 01:04
@justinmmott
Copy link
Author

justinmmott commented Sep 22, 2022

@ahl Okay I fixed it. I think I have it working the same way as Strings do with the try_from. I was looking into doing it a little differently before I figured out the functionality of the newType. But I think it is def something to think about. I was thinking to rewrite some of the nonzero types macros to be used to the bounded types. That way you can do something like this (using the veggie codegen.rs example below). This way you can't create a bounded num with just type_name() and need to use type_name::new() so that we can always run the bounds check. ATM, if you don't use the type_name::try_from I think you can still create an out of bounds number.

Also, no worries about being swapped. Please take your time to review. Just think what you guys are doing is really cool and wanted to help out!

#[derive(Clone, Debug)]
pub struct Veggie {
    #[doc = "Do I like this vegetable?"]
    pub veggie_like: bool,
    #[doc = "The name of the vegetable."]
    pub veggie_name: String,
}
impl Veggie {
    pub fn builder() -> builder::Veggie {
        builder::Veggie::default()
    }
}
// putting this inside the bounded mod disallows users from
// creating a new VeggiesIdNum using VeggiesIdNum() and forces
// them to use ::new()
mod bounded {
    #[derive(Clone, Debug)]
    pub struct VeggiesIdNum(pub(crate) u8);

    impl VeggiesIdNum {
        pub fn builder() -> super::builder::Veggie {
            super::builder::Veggie::default()
        }
    }
}

#[doc = "A representation of a person, company, organization, or place"]
#[derive(Clone, Debug)]
pub struct Veggies {
    pub fruits: Vec<String>,
    pub id_num: Option<bounded::VeggiesIdNum>,
    pub vegetables: Vec<Veggie>,
}
impl Veggies {
    pub fn builder() -> builder::Veggies {
        builder::Veggies::default()
    }
}
mod builder {
    #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)]
    pub struct VeggiesIdNum(u8);

    impl VeggiesIdNum {
        // for this part I was just taking the bare minimum from the
        // nonzero int macro to get it to work so not really sure 
        // if this function is even needed but left in in just in case
        pub unsafe fn new_unchecked(n: u8) -> Self {
            // SAFETY: this is guaranteed to be safe by the caller.
            Self(n)
        }

        const MAX: Option<u8> = Some(30);
        const MIN: Option<u8> = Some(20);
        const MULTIPLE_OF: Option<u8> = Some(5);

        pub fn new(n: u8) -> Option<Self> {
            // SAFETY: we just checked that there's no `0`
            if let Some(m) = VeggiesIdNum::MULTIPLE_OF {
                if n % m != 0 {
                    return None;
                }
            }
            if let Some(m) = VeggiesIdNum::MIN {
                if n <= m {
                    return None;
                }
            }
            if let Some(m) = VeggiesIdNum::MAX {
                if n >= m {
                    return None;
                }
            }
            Some(unsafe { VeggiesIdNum::new_unchecked(n) })
        }
    }

    impl Default for VeggiesIdNum {
        fn default() -> Self {
            Self { 0: 0 }
        }
    }

    impl std::convert::TryFrom<VeggiesIdNum> for super::bounded::VeggiesIdNum {
        type Error = String;
        fn try_from(value: VeggiesIdNum) -> Result<Self, Self::Error> {
            Ok(Self { 0: value.0 })
        }
    }

    pub struct Veggie {
        veggie_like: Result<bool, String>,
        veggie_name: Result<String, String>,
    }
    impl Default for Veggie {
        fn default() -> Self {
            Self {
                veggie_like: Err("no value supplied for veggie_like".to_string()),
                veggie_name: Err("no value supplied for veggie_name".to_string()),
            }
        }
    }
    impl Veggie {
        pub fn veggie_like<T>(mut self, value: T) -> Self
        where
            T: std::convert::TryInto<bool>,
            T::Error: std::fmt::Display,
        {
            self.veggie_like = value
                .try_into()
                .map_err(|e| format!("error converting supplied value for veggie_like: {}", e));
            self
        }
        pub fn veggie_name<T>(mut self, value: T) -> Self
        where
            T: std::convert::TryInto<String>,
            T::Error: std::fmt::Display,
        {
            self.veggie_name = value
                .try_into()
                .map_err(|e| format!("error converting supplied value for veggie_name: {}", e));
            self
        }
    }
    impl std::convert::TryFrom<Veggie> for super::Veggie {
        type Error = String;
        fn try_from(value: Veggie) -> Result<Self, Self::Error> {
            Ok(Self {
                veggie_like: value.veggie_like?,
                veggie_name: value.veggie_name?,
            })
        }
    }
    pub struct Veggies {
        fruits: Result<Vec<String>, String>,
        id_num: Result<Option<super::bounded::VeggiesIdNum>, String>,
        vegetables: Result<Vec<super::Veggie>, String>,
    }
    impl Default for Veggies {
        fn default() -> Self {
            Self {
                fruits: Ok(Default::default()),
                id_num: Ok(Default::default()),
                vegetables: Ok(Default::default()),
            }
        }
    }
    impl Veggies {
        pub fn fruits<T>(mut self, value: T) -> Self
        where
            T: std::convert::TryInto<Vec<String>>,
            T::Error: std::fmt::Display,
        {
            self.fruits = value
                .try_into()
                .map_err(|e| format!("error converting supplied value for fruits: {}", e));
            self
        }
        pub fn id_num<T>(mut self, value: T) -> Self
        where
            T: std::convert::TryInto<Option<super::bounded::VeggiesIdNum>>,
            T::Error: std::fmt::Display,
        {
            self.id_num = value
                .try_into()
                .map_err(|e| format!("error converting supplied value for id_num: {}", e));
            self
        }
        pub fn vegetables<T>(mut self, value: T) -> Self
        where
            T: std::convert::TryInto<Vec<super::Veggie>>,
            T::Error: std::fmt::Display,
        {
            self.vegetables = value
                .try_into()
                .map_err(|e| format!("error converting supplied value for vegetables: {}", e));
            self
        }
    }
    impl std::convert::TryFrom<Veggies> for super::Veggies {
        type Error = String;
        fn try_from(value: Veggies) -> Result<Self, Self::Error> {
            Ok(Self {
                fruits: value.fruits?,
                vegetables: value.vegetables?,
                id_num: value.id_num?,
            })
        }
    }
}

Copy link
Contributor

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

WFM. Thanks

@jayvdb
Copy link
Contributor

jayvdb commented Dec 23, 2022

@justinmmott, there are now conflicts which need to be resolved.

@jayvdb
Copy link
Contributor

jayvdb commented Dec 28, 2022

I did a rebase locally. One very easy conflict to resolve:

diff --cc typify-impl/src/convert.rs
index 1c12154,c7b4837..0000000
--- a/typify-impl/src/convert.rs
+++ b/typify-impl/src/convert.rs
@@@ -6,11 -6,11 +6,11 @@@ use crate::type_entry::
      EnumTagType, TypeEntry, TypeEntryDetails, TypeEntryEnum, TypeEntryNewtype, TypeEntryStruct,
      Variant, VariantDetails,
  };
 -use crate::util::{all_mutually_exclusive, none_or_single, recase, Case};
 +use crate::util::{all_mutually_exclusive, none_or_single, recase, Case, StringValidator};
  use log::info;
  use schemars::schema::{
-     ArrayValidation, InstanceType, Metadata, ObjectValidation, Schema, SchemaObject, SingleOrVec,
-     StringValidation, SubschemaValidation,
+     ArrayValidation, InstanceType, Metadata, NumberValidation, ObjectValidation, Schema,
 -    SchemaObject, SingleOrVec, SubschemaValidation,
++    SchemaObject, SingleOrVec, StringValidation, SubschemaValidation,
  };
  
  use crate::util::get_type_name;

@ahl
Copy link
Collaborator

ahl commented Dec 28, 2022

I had a different approach in mind: I was thinking about a crate with constrained types of the following form:

struct ConstrainedI64<
    const MIN: i64 = { i64::MIN },
    const MAX: i64 = { i64::MAX },
    const MULTIPLE_OF: i64 = 1,
>(i64);

This would be like a more generic NonZeroU64 for example.

In addition, I'm not sure consumers would want these by default... they're fairly fussy... in addition to being uncommon. If an API does make use of these, I'd rather not generate tons of code for each type. @jayvdb , what do you think?

@jayvdb
Copy link
Contributor

jayvdb commented Dec 30, 2022

@ahl, I don't see where this proposed change generates more code for each type ... ?

It is only adding support for floats having min/max/multiple, like is already implemented for integers. I believe min/max/multiple for floats are very common. The most common use of these for floats that I have seen is min:0.0,max:1.0, which has lots of varied use cases, more in science-y APIs than business/internet-y APIs.

afaics, this proposed change does not expose any API differences outside of the crate, so it doesnt lock this approach into stone - i.e. it can be improved upon over time without effecting users of the crate.

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

Successfully merging this pull request may close these issues.

None yet

3 participants