feat: added newtype for condition and override in context apis #146

pratikmishra356 · 2024-07-02T11:30:39Z

Problem

Request Data Validation was not present in context apis

Solution

Created newtype for condition and override and added validation there

Environment variable changes

NA

Pre-deployment activity

NA

Post-deployment activity

NA

API changes

Endpoint	Method	Request body	Response Body
API	GET/POST, etc	request	response

Possible Issues in the future

NA

Datron · 2024-07-03T05:43:52Z

@pratikmishra356 Can we add tests to check these validations?

Datron

Are all validations for context and overrides included here or more expected in another PR? Can we add tests to check the negative cases for validations so we can guarantee that they work in the future too?

Datron · 2024-07-03T05:45:36Z

crates/service_utils/src/result.rs

+            AppError::ValidationError(msg)
+            | AppError::BadArgument(msg)
+            | AppError::NotFound(msg) => msg.to_owned(),
+            AppError::UnexpectedError(_) => String::from("Something went wrong"),


Lets stop doing "Something went wrong". Need a better error message here

Also log and throw errors if appropriate

@Datron The string returned from message function should be same as what string returned using error macro.
For logging , wherever we pass error in these macro like bad_argument!(err) -> we are logging error there itself. Like you can check new function.
This function just helps in getting the message which Apperror sends , so that we can pass the same message into other error types like here serde deserialisation error

Datron · 2024-07-03T05:48:22Z

crates/context_aware_config/src/api/context/types.rs

+pub struct Override(Map<String, Value>);
+
+impl Override {
+    pub fn new(override_map: Map<String, Value>) -> superposition::Result<Self> {


Should we validate the schema of the override value as well using json schema?

Datron · 2024-07-03T05:49:26Z

crates/context_aware_config/src/api/context/types.rs

+impl Condition {
+    pub fn new(condition_map: Map<String, Value>) -> superposition::Result<Self> {
+        if !condition_map.is_empty() {
+            jsonlogic::apply(&json!(condition_map), &json!({})).map_err(|msg| {


We can use jsonlogic::compile here if we want to check validity instead of applying it

Datron · 2024-07-03T05:50:13Z

crates/context_aware_config/src/api/context/helpers.rs

 ) -> superposition::Result<()> {
    use dimensions::dsl;
-    let context = extract_dimensions(context)?;
+    let context = extract_dimensions(&json!(**condition))?;


derive_more has an auto deref trait. Can we use that to avoid our code looking like C++?

Can we also add a to_json() method on newtypes? Skip one more step here

Datron · 2024-07-03T05:50:52Z

crates/context_aware_config/src/api/context/handlers.rs

-    let ctx_condition = Value::Object(req.context.to_owned());
-    let ctx_override: Value = req.r#override.to_owned().into();
+    let ctx_condition = req.context.to_owned();
+    let condition_val = json!(*ctx_condition);


Use derive_more Deref to avoid this.

Datron · 2024-07-03T05:53:39Z

crates/context_aware_config/src/api/config/handlers.rs

-fn construct_new_payload(req_payload: &Map<String, Value>) -> web::Json<PutReq> {
+fn construct_new_payload(
+    req_payload: &Map<String, Value>,
+) -> superposition::Result<web::Json<PutReq>> {
    let mut res = req_payload.clone();
    res.remove("to_be_deleted");
    res.remove("override_id");
    res.remove("id");
    if let Some(Value::Object(res_context)) = res.get("context") {


Can we do negative checks first to avoid this nesting?

Suggested change

if let Some(Value::Object(res_context)) = res.get("context") {

if res.get("context").is_none() {

Throw error if true, process if false

ayushjain17 · 2024-07-16T12:26:53Z

crates/superposition_types/src/lib.rs

@@ -74,3 +77,276 @@ impl FromRequest for User {
        }
    }
 }
+
+#[derive(Debug, Clone, PartialEq)]


can we use something like this:

Suggested change

#[derive(Debug, Clone, PartialEq)]

#[derive(Debug, Clone, PartialEq)]

pub enum ValidationType {

DEFAULT,

EXPERIMENTAL,

#[cfg(feature = "db_type_validation_disabled")]

DB,

}

pub fn get_db_validation_type() -> ValidationType {

#[cfg(feature = "db_type_validation_disabled")]

return ValidationType::DB;

#[cfg(not(feature = "db_type_validation_disabled"))]

return ValidationType::DEFAULT;

}

we can utilise rust features here, and anyone who has old version of code can go ahead with db_type_validation_disabled feature (or whatever you want to call it) as newer versions will not have discrepancy in DB data

wherever you want to use ValidationType::DB, just use this function get_db_validation_type directly, which will resolve the value based on the feature which the user of superposition requires

later on, when we are sure that data is not longer corrupt in DB, we can get the feature removed altogether

PS: db_type_validation_disabled should be a superposition_types level feature

places having a switch case can use something like:

#[cfg(feature = "db_type_validation_disabled")] ValidationType::DB => (),

mahatoankitkumar · 2024-07-16T12:14:40Z

crates/context_aware_config/src/api/config/handlers.rs

+        .and_then(|val| val.as_object())
+        .map_or_else(
+            || {
+                log::error!("construct new payload Context not present");


Suggested change

log::error!("construct new payload Context not present");

log::error!("construct new payload: Context not present");

mahatoankitkumar · 2024-07-16T12:27:09Z

crates/context_aware_config/src/helpers.rs

+    let mut contexts = Vec::new();
+    let mut overrides: HashMap<String, Overrides> = HashMap::new();
+
+    for (id, condition, priority_, override_id, override_) in contexts_vec.iter() {


Could had used fold here also.

can be used , but it gives no advantage over for loop and readability reduces as multiple variables are being used here

mahatoankitkumar · 2024-07-16T12:29:18Z

crates/context_aware_config/src/api/config/types.rs

    pub default_configs: Map<String, Value>,
 }

 #[derive(Serialize, Clone, Deserialize)]
 pub struct Context {
    pub id: String,
-    pub condition: Value,
+    pub condition: Condition,
    pub priority: i32,
    pub override_with_keys: [String; 1],


Maybe we can make a type alias for override_with_keys also.

@mahatoankitkumar [String; 1] seems enough for this validation and scope of this pr is only for context and overrides

ayushjain17

Can we avoid json! macro and specify Value::Object directly and keep parameter or variables name same as condition for Condition types to avoid inconsistency

ayushjain17 · 2024-07-31T08:14:16Z

crates/service_utils/src/helpers.rs

        Some(conditions_json) => conditions_json
            .as_array()
            .ok_or(result::AppError::BadArgument("Error extracting dimensions, failed parsing conditions as an array. Ensure the context provided obeys the rules of JSON logic".into()))?
            .clone(),
-        None => vec![context_json.clone()],
+        None => vec![json!(*context)],


can we make it direct instead of using json macro here

Suggested change

None => vec![json!(*context)],

None => vec![Value::Object(*context)],

ayushjain17 · 2024-07-31T10:12:10Z

crates/experimentation_platform/src/api/experiments/handlers.rs

@@ -264,7 +270,7 @@ async fn create(
        override_keys: unique_override_keys.to_vec(),
        traffic_percentage: 0,
        status: ExperimentStatusType::CREATED,
-        context: req.context.clone(),
+        context: json!(req.context.clone()),


here also,

Suggested change

context: json!(req.context.clone()),

context: Value::Object(req.context.clone()),

ayushjain17 · 2024-07-31T10:15:23Z

crates/experimentation_platform/src/api/experiments/handlers.rs

+        experiment
+            .context
+            .as_object()
+            .unwrap_or(&Map::new())


can we do something like this over here

Suggested change

.unwrap_or(&Map::new())

.map_or_else(|| Map::new(), |ctx| ctx.clone())

else, in the current implementation, in the None case, we are creating a new Map and then right after that cloning it, which doesn't make sense

ayushjain17 · 2024-07-31T10:15:59Z

crates/experimentation_platform/src/api/experiments/helpers.rs

+                active_experiment
+                    .context
+                    .as_object()
+                    .unwrap_or(&Map::new())


same here

Suggested change

.unwrap_or(&Map::new())

.map_or_else(|| Map::new(), |ctx| ctx.clone())

ayushjain17 · 2024-07-31T10:16:29Z

crates/experimentation_platform/src/api/experiments/helpers.rs

@@ -222,7 +221,7 @@ pub fn add_variant_dimension_to_ctx(
                    "Failed parsing conditions as an array. Ensure the context provided obeys the rules of JSON logic"
                ))?
                .clone(),
-            None => vec![context_json.clone()],
+            None => vec![json!(context.clone())],


here as well

Suggested change

None => vec![json!(context.clone())],

None => vec![Value::Object(context.clone())],

ayushjain17 · 2024-07-31T10:17:28Z

crates/experimentation_platform/src/api/experiments/helpers.rs

@@ -198,7 +200,7 @@ pub fn validate_experiment(
 }

 pub fn add_variant_dimension_to_ctx(
-    context_json: &Value,
+    context_json: &Condition,


Suggested change

context_json: &Condition,

context: &Condition,

ayushjain17 · 2024-07-31T10:26:12Z

crates/experimentation_platform/src/api/experiments/helpers.rs

-    let context = context_json.as_object().ok_or(bad_argument!(
-        "Context not an object. Ensure the context provided obeys the rules of JSON logic"
-    ))?;
+    let context: Map<String, Value> = context_json.to_owned().into();


wouldn't it be better to go this way, after the above comment change is added

Suggested change

let context: Map<String, Value> = context_json.to_owned().into();

let context_json = context.clone().into_inner();

ayushjain17 · 2024-07-31T10:38:53Z

crates/context_aware_config/src/api/context/types.rs

+        let mut expected_condition = Map::new();
+        expected_condition.insert("foo".to_string(), json!("bar"));
+        expected_condition.insert("bar".to_string(), json!({ "baz": "baz"}));
+        let condition = Cac::<Condition>::try_from(expected_condition)


to maintain consistency

Suggested change

let condition = Cac::<Condition>::try_from(expected_condition)

let context = Cac::<Condition>::try_from(expected_condition)

ayushjain17 · 2024-07-31T10:39:54Z

crates/context_aware_config/src/api/context/helpers.rs

@@ -24,10 +24,10 @@ type DBConnection = PooledConnection<ConnectionManager<PgConnection>>;

 pub fn validate_condition_with_functions(
    conn: &mut DBConnection,
-    context: &Value,
+    condition: &Condition,


to maintain consistency

Suggested change

condition: &Condition,

context: &Condition,

ayushjain17 · 2024-07-31T11:41:19Z

crates/context_aware_config/src/api/context/handlers.rs

@@ -399,7 +401,7 @@ fn r#move(
 ) -> superposition::Result<PutResp> {
    use contexts::dsl;
    let req = req.into_inner();
-    let ctx_condition = Value::Object(req.context);
+    let ctx_condition = json!(req.context.into_inner());


can we let it be Value::Object itself

Suggested change

let ctx_condition = json!(req.context.into_inner());

let ctx_condition = Value::Object(req.context.into_inner());

Datron

few nitpicks but overall looks good

Datron · 2024-08-01T09:03:17Z

crates/superposition_types/src/lib.rs

@@ -74,3 +77,307 @@ impl FromRequest for User {
        }
    }
 }
+
+#[derive(Clone, Debug, PartialEq, Copy, Serialize)]
+pub struct Cac<T>(T);


Can we implement deref trait instead of into_inner?

@Datron into_inner gives the type wrapped inside i.e T,
Deref trait mainly dereference the the value ,
and also Deref wont work in this case as Deref on CAC requires copy trait for Condition , and we cant have Copy trait for condition as it wraps serde Map which does not have the Copy trait either

cannot move out of dereference of `Cac<superposition_types::Condition>` move occurs because value has type `superposition_types::Condition`, which does not implement the `Copy` trait

pratikmishra356 requested a review from a team as a code owner July 2, 2024 11:30

pratikmishra356 linked an issue Jul 2, 2024 that may be closed by this pull request

Add new type for condition and override in context #142

Closed

Datron requested changes Jul 3, 2024

View reviewed changes

pratikmishra356 force-pushed the api-request-validation branch 6 times, most recently from 9da99ba to 1590454 Compare July 9, 2024 09:32

pratikmishra356 requested a review from Datron July 9, 2024 09:48

pratikmishra356 force-pushed the api-request-validation branch 2 times, most recently from f50ff60 to 390606b Compare July 16, 2024 11:53

ayushjain17 reviewed Jul 16, 2024

View reviewed changes

mahatoankitkumar requested changes Jul 16, 2024

View reviewed changes

pratikmishra356 force-pushed the api-request-validation branch from 16cd13e to c82dee6 Compare July 18, 2024 11:56

pratikmishra356 force-pushed the api-request-validation branch from 48f963c to fadcfee Compare July 25, 2024 12:37

mahatoankitkumar requested review from mahatoankitkumar and ayushjain17 July 29, 2024 07:18

pratikmishra356 force-pushed the api-request-validation branch 2 times, most recently from 5af29db to eb4e3a2 Compare July 29, 2024 11:35

Datron force-pushed the api-request-validation branch from eb4e3a2 to 17c9a95 Compare July 31, 2024 07:03

ayushjain17 reviewed Jul 31, 2024

View reviewed changes

pratikmishra356 force-pushed the api-request-validation branch from 17c9a95 to 28574d7 Compare August 1, 2024 06:44

pratikmishra356 requested a review from ayushjain17 August 1, 2024 06:44

Datron approved these changes Aug 1, 2024

View reviewed changes

pratikmishra356 force-pushed the api-request-validation branch 2 times, most recently from 1cffea5 to 50b0507 Compare August 1, 2024 11:03

ayushjain17 approved these changes Aug 1, 2024

View reviewed changes

pratikmishra356 force-pushed the api-request-validation branch from 50b0507 to bc32633 Compare August 1, 2024 12:00

fix: api validation with new types

8edfa52

pratikmishra356 force-pushed the api-request-validation branch from bc32633 to 8edfa52 Compare August 1, 2024 12:26

mahatoankitkumar approved these changes Aug 1, 2024

View reviewed changes

mahatoankitkumar merged commit 66ad741 into main Aug 1, 2024
4 checks passed

mahatoankitkumar deleted the api-request-validation branch August 1, 2024 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added newtype for condition and override in context apis #146

feat: added newtype for condition and override in context apis #146

pratikmishra356 commented Jul 2, 2024

Datron commented Jul 3, 2024

Datron left a comment

Datron Jul 3, 2024

Datron Jul 3, 2024

pratikmishra356 Jul 3, 2024

Datron Jul 3, 2024

Datron Jul 3, 2024

Datron Jul 3, 2024

Datron Jul 3, 2024

Datron Jul 3, 2024

Datron Jul 3, 2024

ayushjain17 Jul 16, 2024 •

edited

Loading

ayushjain17 Jul 16, 2024

mahatoankitkumar Jul 16, 2024

mahatoankitkumar Jul 16, 2024

pratikmishra356 Jul 19, 2024

mahatoankitkumar Jul 16, 2024

pratikmishra356 Jul 18, 2024

ayushjain17 left a comment

ayushjain17 Jul 31, 2024

ayushjain17 Jul 31, 2024

ayushjain17 Jul 31, 2024

ayushjain17 Jul 31, 2024

ayushjain17 Jul 31, 2024

ayushjain17 Jul 31, 2024

ayushjain17 Jul 31, 2024

ayushjain17 Jul 31, 2024

ayushjain17 Jul 31, 2024

ayushjain17 Jul 31, 2024

Datron left a comment

Datron Aug 1, 2024

pratikmishra356 Aug 1, 2024

	if let Some(Value::Object(res_context)) = res.get("context") {
	if res.get("context").is_none() {

-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, PartialEq)]
+pub enum ValidationType {
+    DEFAULT,
+    EXPERIMENTAL,
+    #[cfg(feature = "db_type_validation_disabled")]
+    DB,
+}
+pub fn get_db_validation_type() -> ValidationType {
+    #[cfg(feature = "db_type_validation_disabled")]
+    return ValidationType::DB;
+    #[cfg(not(feature = "db_type_validation_disabled"))]
+    return ValidationType::DEFAULT;
+}

	log::error!("construct new payload Context not present");
	log::error!("construct new payload: Context not present");

	None => vec![json!(*context)],
	None => vec![Value::Object(*context)],

	context: json!(req.context.clone()),
	context: Value::Object(req.context.clone()),

	.unwrap_or(&Map::new())
	.map_or_else(\|\| Map::new(), \|ctx\| ctx.clone())

	None => vec![json!(context.clone())],
	None => vec![Value::Object(context.clone())],

	let context: Map<String, Value> = context_json.to_owned().into();
	let context_json = context.clone().into_inner();

	let condition = Cac::<Condition>::try_from(expected_condition)
	let context = Cac::<Condition>::try_from(expected_condition)

	let ctx_condition = json!(req.context.into_inner());
	let ctx_condition = Value::Object(req.context.into_inner());

feat: added newtype for condition and override in context apis #146

feat: added newtype for condition and override in context apis #146

Conversation

pratikmishra356 commented Jul 2, 2024

Problem

Solution

Environment variable changes

Pre-deployment activity

Post-deployment activity

API changes

Possible Issues in the future

Datron commented Jul 3, 2024

Datron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushjain17 Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushjain17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushjain17 Jul 16, 2024 •

edited

Loading