juspay / superposition

Superposition is a context-based configuration management system designed to facilitate safe and flexible rollout of config changes
Apache License 2.0
20 stars 6 forks source link

Request body validations using Newtype #118

Open ShubhranshuSanjeev opened 3 weeks ago

ShubhranshuSanjeev commented 3 weeks ago
pratikmishra356 commented 3 weeks ago

Validation for Context (write Apis) - below validations are missing

  1. put ("") (create or update context) -> validate does the request body contains malformed conditions(jsonlogic) and validate empty overrides

  2. update overrides -> need to validate all the things as in put request and also we should not create the new one, since this is update overrides only

  3. move context ->. it accepts condition and contexts ids -> we need to validate the conditions either it should empty or valid jsonlogic

  4. put ( bulk operations ) -> we accept putreq and movereq type , so will have to perform the same validation as in 1 and 3

    approach - to validate condition , we use jsonlogic::apply and check for error , for empty we can check if condition and overrides map is empty for update if can check if context_id present in db or not

pratikmishra356 commented 3 weeks ago

Validation for Default-Config key

  1. create or update -> all kinds of validation already present , question is do we need to validate overrides containing same key when we are trying to update schema of any default config key. Also we are validating schema in the request on a predefined jsonschema so should we have this restriction?

  2. Delete Api -> validation already present - we are not allowing deletion of key if it is being used in any overrides

pratikmishra356 commented 3 weeks ago

Validation for Dimension key

We dont have different api for create and update

  1. create or update -> currently we dont have restriction on dimension name , should we validate schema on the existing conditions containing same dimension ? also we are validating schema on a predefined schema , so should we have that restriction? we are not enforcing priority value uniqueness approach we will do regex match on dimension name
pratikmishra356 commented 1 week ago

Validation for Experiment Apis

  1. create -> we are not validating override emptiness and experiment name emptiness
  2. update overrides -> validation for empty override is not present
  3. ramp -> accepting u64 , it should be u8
pratikmishra356 commented 1 week ago

Validation for Function Apis

  1. create -> validate function name
pratikmishra356 commented 1 week ago

For String type validation we can follow the below approach : Create Newtype , for example DimensionName(String), ExperimentName(String), FunctionName(String) Next, define fn new implementation for each types like

impl DimensionName {
    pub fn new(raw_email: &str) -> Result<Self, String> {
        if raw_email.to_owned().len() > 3 {
            Ok(Self(lower))
        } else {
            Err("Invalid Dimension Name minimum length 4".to_string())
        }
    }

also implement TryFrom trait for each NewType

impl TryFrom for DimensionName {
    type Err = String;
    fn try_from(s: String) -> Result<Self, String> {
        DimensionName::new(s)
    }
}

Then add a generic deserialize function

fn deserialize_string<'de, T, D>(deserializer: D) -> Result<T, D::Error>
where
    T: TryFrom<String>,
    T::Error: Display,
    D: Deserializer<'de>,
{
    let name: String = Deserialize::deserialize(deserializer)?;
    T::try_from(name).map_err( |op| de::Error::custom(op.to_string()))
}

Add this in the request body type as custom deserialize option

#[derive(Debug, Deserialize)]
pub struct CreateReq {
    #[serde(deserialize_with = "deserialize_string")]
    pub dimension: DimensionName,
    pub priority: u16,
    pub schema: Value,
    #[serde(default, deserialize_with = "deserialize_option")]
    pub function_name: Option<Value>,
}

We can follow similar approach for Overrides to check empty map , number data type like ramp , priority etc

Any other suggestions ?

pratikmishra356 commented 1 week ago

Update on the above Approach , its more clean we just have to define tryfrom Trait where we can add he data validation as needed: We can declare newtype as below

#[derive(Clone, Debug, Deserialize)]
#[serde(try_from = "String")]
pub struct DimensionName(String);

We dont have to define deserialization method and will not have to include the below line while declaring any struct where we will use newtype #[serde(deserialize_with = "deserialize_string")] // this is not required

pratikmishra356 commented 1 week ago

Validation of Dimension and Default Config's schema

We have custom type where user can define types using json-schema, so we should remove the jsonschema validation for Default Config and for Dimension will have to allow more types in the current jsonschema restriction like remove the pattern as required field if string , include array of numbers etc