From f01a002ca857686c1c1451229cdc1950f9a637be Mon Sep 17 00:00:00 2001 From: Pratik Mishra Date: Wed, 29 Jan 2025 15:15:21 +0530 Subject: [PATCH] fix: update queries refactor --- .../src/api/config/handlers.rs | 16 ++- .../src/api/context/handlers.rs | 106 +++++++++++------- .../src/api/context/helpers.rs | 15 +-- .../src/api/context/operations.rs | 12 +- .../src/api/default_config/handlers.rs | 64 +++++------ .../src/api/default_config/types.rs | 34 +++++- .../src/api/dimension/handlers.rs | 43 +++---- .../src/api/dimension/types.rs | 32 +++++- .../src/api/functions/handlers.rs | 39 +------ .../src/api/functions/types.rs | 25 ++++- 10 files changed, 223 insertions(+), 163 deletions(-) diff --git a/crates/context_aware_config/src/api/config/handlers.rs b/crates/context_aware_config/src/api/config/handlers.rs index b9455dec3..3bf37df53 100644 --- a/crates/context_aware_config/src/api/config/handlers.rs +++ b/crates/context_aware_config/src/api/config/handlers.rs @@ -41,8 +41,11 @@ use superposition_types::{ }; use uuid::Uuid; -use crate::{api::context, helpers::DimensionData}; use crate::{api::context::PutReq, helpers::generate_cac}; +use crate::{ + api::context::{self, helpers::ensure_description}, + helpers::DimensionData, +}; use crate::{ api::dimension::{get_dimension_data, get_dimension_data_map}, helpers::calculate_context_weight, @@ -495,8 +498,19 @@ async fn reduce_config_key( if is_approve { let _ = context::delete(cid.clone(), user, conn, schema_name); if let Ok(put_req) = construct_new_payload(request_payload) { + let description = match put_req.description.clone() { + Some(val) => val, + None => ensure_description( + Value::Object( + put_req.context.clone().into_inner().into(), + ), + conn, + &schema_name, + )?, + }; let _ = context::put( put_req, + description, conn, false, &user, diff --git a/crates/context_aware_config/src/api/context/handlers.rs b/crates/context_aware_config/src/api/context/handlers.rs index 1bba909c6..30eeec3df 100644 --- a/crates/context_aware_config/src/api/context/handlers.rs +++ b/crates/context_aware_config/src/api/context/handlers.rs @@ -67,21 +67,23 @@ async fn put_handler( schema_name: SchemaName, ) -> superposition::Result { let tags = parse_config_tags(custom_headers.config_tags)?; + let description = match req.description.clone() { + Some(val) => val, + None => ensure_description( + Value::Object(req.context.clone().into_inner().into()), + &mut db_conn, + &schema_name, + )?, + }; + let req_change_reason = req.change_reason.clone(); let (put_response, version_id) = db_conn .transaction::<_, superposition::AppError, _>(|transaction_conn| { - let mut req_mut = req.into_inner(); - // Use the helper function to ensure the description - if req_mut.description.is_none() { - req_mut.description = Some(ensure_description( - Value::Object(req_mut.context.clone().into_inner().into()), - transaction_conn, - &schema_name, - )?); - } + let put_response = operations::put( - Json(req_mut.clone()), + Json(req.into_inner()), + description.clone(), transaction_conn, true, &user, @@ -92,14 +94,12 @@ async fn put_handler( log::info!("context put failed with error: {:?}", err); err })?; - let description = req_mut.description.unwrap_or_default(); - let change_reason = req_mut.change_reason; let version_id = add_config_version( &state, tags, description, - change_reason, + req_change_reason, transaction_conn, &schema_name, )?; @@ -131,18 +131,20 @@ async fn update_override_handler( schema_name: SchemaName, ) -> superposition::Result { let tags = parse_config_tags(custom_headers.config_tags)?; + let description = match req.description.clone() { + Some(val) => val, + None => ensure_description( + Value::Object(req.context.clone().into_inner().into()), + &mut db_conn, + &schema_name, + )?, + }; + let req_change_reason = req.change_reason.clone(); let (override_resp, version_id) = db_conn .transaction::<_, superposition::AppError, _>(|transaction_conn| { - let mut req_mut = req.into_inner(); - if req_mut.description.is_none() { - req_mut.description = Some(ensure_description( - Value::Object(req_mut.context.clone().into_inner().into()), - transaction_conn, - &schema_name, - )?); - } let override_resp = operations::put( - Json(req_mut.clone()), + Json(req.into_inner()), + description.clone(), transaction_conn, true, &user, @@ -156,8 +158,8 @@ async fn update_override_handler( let version_id = add_config_version( &state, tags, - req_mut.description.unwrap().clone(), - req_mut.change_reason.clone(), + description.clone(), + req_change_reason.clone(), transaction_conn, &schema_name, )?; @@ -189,11 +191,22 @@ async fn move_handler( schema_name: SchemaName, ) -> superposition::Result { let tags = parse_config_tags(custom_headers.config_tags)?; + + let description = match req.description.clone() { + Some(val) => val, + None => ensure_description( + Value::Object(req.context.clone().into_inner().into()), + &mut db_conn, + &schema_name, + )?, + }; + let (move_response, version_id) = db_conn .transaction::<_, superposition::AppError, _>(|transaction_conn| { let move_response = operations::r#move( path.into_inner(), req, + description, transaction_conn, true, &user, @@ -423,8 +436,26 @@ async fn bulk_operations( for action in reqs.into_inner().into_iter() { match action { ContextAction::Put(put_req) => { + let ctx_condition = put_req.context.to_owned().into_inner(); + let ctx_condition_value = + Value::Object(ctx_condition.clone().into()); + + let description = if put_req.description.is_none() { + ensure_description( + ctx_condition_value.clone(), + transaction_conn, + &schema_name, + )? + } else { + put_req + .description + .clone() + .expect("Description should not be empty") + }; + let put_resp = operations::put( Json(put_req.clone()), + description.clone(), transaction_conn, true, &user, @@ -439,21 +470,6 @@ async fn bulk_operations( err })?; - let ctx_condition = put_req.context.to_owned().into_inner(); - let ctx_condition_value = - Value::Object(ctx_condition.clone().into()); - - let description = if put_req.description.is_none() { - ensure_description( - ctx_condition_value.clone(), - transaction_conn, - &schema_name, - )? - } else { - put_req - .description - .expect("Description should not be empty") - }; all_descriptions.push(description); all_change_reasons.push(put_req.change_reason.clone()); response.push(ContextBulkResponse::Put(put_resp)); @@ -498,9 +514,21 @@ async fn bulk_operations( }; } ContextAction::Move((old_ctx_id, move_req)) => { + let description = match move_req.description.clone() { + Some(val) => val, + None => ensure_description( + Value::Object( + move_req.context.clone().into_inner().into(), + ), + transaction_conn, + &schema_name, + )?, + }; + let move_context_resp = operations::r#move( old_ctx_id, Json(move_req), + description, transaction_conn, true, &user, diff --git a/crates/context_aware_config/src/api/context/helpers.rs b/crates/context_aware_config/src/api/context/helpers.rs index cd7f1bc9f..bedb72921 100644 --- a/crates/context_aware_config/src/api/context/helpers.rs +++ b/crates/context_aware_config/src/api/context/helpers.rs @@ -8,9 +8,9 @@ use base64::prelude::*; use cac_client::utils::json_to_sorted_string; use chrono::Utc; use diesel::{ExpressionMethods, QueryDsl, RunQueryDsl, SelectableHelper}; -use serde_json::{json, Map, Value}; +use serde_json::{Map, Value}; use service_utils::{helpers::extract_dimensions, service::types::SchemaName}; -use superposition_macros::{bad_argument, unexpected_error, validation_error}; +use superposition_macros::{unexpected_error, validation_error}; use superposition_types::{ database::{ models::cac::Context, @@ -213,6 +213,7 @@ pub fn ensure_description( pub fn create_ctx_from_put_req( req: Json, + req_description: String, conn: &mut DBConnection, user: &User, schema_name: &SchemaName, @@ -221,14 +222,6 @@ pub fn create_ctx_from_put_req( let condition_val = Value::Object(ctx_condition.clone().into()); let r_override = req.r#override.clone().into_inner(); let ctx_override = Value::Object(r_override.clone().into()); - let description = if req.description.is_none() { - let ctx_condition_value = json!(ctx_condition); - ensure_description(ctx_condition_value, conn, schema_name)? - } else { - req.description - .clone() - .ok_or_else(|| bad_argument!("Description should not be empty"))? - }; let workspace_settings = get_workspace(schema_name, conn)?; @@ -260,7 +253,7 @@ pub fn create_ctx_from_put_req( last_modified_at: Utc::now().naive_utc(), last_modified_by: user.get_email(), weight, - description, + description: req_description, change_reason, }) } diff --git a/crates/context_aware_config/src/api/context/operations.rs b/crates/context_aware_config/src/api/context/operations.rs index 08378ef99..6a1d815e7 100644 --- a/crates/context_aware_config/src/api/context/operations.rs +++ b/crates/context_aware_config/src/api/context/operations.rs @@ -35,6 +35,7 @@ use super::{ pub fn put( req: Json, + description: String, conn: &mut PooledConnection>, already_under_txn: bool, user: &User, @@ -42,7 +43,7 @@ pub fn put( replace: bool, ) -> result::Result { use contexts::dsl::contexts; - let new_ctx = create_ctx_from_put_req(req, conn, user, schema_name)?; + let new_ctx = create_ctx_from_put_req(req, description, conn, user, schema_name)?; if already_under_txn { diesel::sql_query("SAVEPOINT put_ctx_savepoint").execute(conn)?; @@ -75,6 +76,7 @@ pub fn put( pub fn r#move( old_ctx_id: String, req: Json, + req_description: String, conn: &mut PooledConnection>, already_under_txn: bool, user: &User, @@ -84,12 +86,6 @@ pub fn r#move( let req = req.into_inner(); let ctx_condition = req.context.to_owned().into_inner(); let ctx_condition_value = Value::Object(ctx_condition.clone().into()); - let description = if req.description.is_none() { - ensure_description(ctx_condition_value.clone(), conn, schema_name)? - } else { - req.description - .ok_or_else(|| bad_argument!("Description should not be empty"))? - }; let change_reason = req.change_reason.clone(); let new_ctx_id = hash(&ctx_condition_value); @@ -134,7 +130,7 @@ pub fn r#move( last_modified_at: Utc::now().naive_utc(), last_modified_by: user.get_email(), weight, - description, + description: req_description, change_reason, }; diff --git a/crates/context_aware_config/src/api/default_config/handlers.rs b/crates/context_aware_config/src/api/default_config/handlers.rs index 07b7788e8..bba89a2c8 100644 --- a/crates/context_aware_config/src/api/default_config/handlers.rs +++ b/crates/context_aware_config/src/api/default_config/handlers.rs @@ -185,41 +185,26 @@ async fn update_default_config( let description = req .description + .clone() .unwrap_or_else(|| existing.description.clone()); - let change_reason = req.change_reason; + let change_reason = req.change_reason.clone(); - let value = req.value.unwrap_or_else(|| existing.value.clone()); + let value = req.value.clone().unwrap_or_else(|| existing.value.clone()); let schema = req .schema + .clone() .map(Value::Object) .unwrap_or_else(|| existing.schema.clone()); - let function_name = match req.function_name { - Some(FunctionNameEnum::Name(func_name)) => Some(func_name), - Some(FunctionNameEnum::Remove) => None, - None => existing.function_name.clone(), - }; - let updated_config = DefaultConfig { - key: key_str.to_owned(), - value, - schema, - function_name: function_name.clone(), - created_by: existing.created_by.clone(), - created_at: existing.created_at, - last_modified_at: Utc::now().naive_utc(), - last_modified_by: user.get_email(), - description: description.clone(), - change_reason: change_reason.clone(), - }; let jschema = JSONSchema::options() .with_draft(Draft::Draft7) - .compile(&updated_config.schema) + .compile(&schema) .map_err(|e| { log::info!("Failed to compile JSON schema: {e}"); bad_argument!("Invalid JSON schema.") })?; - if let Err(e) = jschema.validate(&updated_config.value) { + if let Err(e) = jschema.validate(&value) { let verrors = e.collect::>(); log::info!("Validation failed: {:?}", verrors); return Err(validation_error!( @@ -230,31 +215,34 @@ async fn update_default_config( )); } + let function_name = match req.function_name.clone() { + Some(FunctionNameEnum::Name(func_name)) => Some(func_name), + Some(FunctionNameEnum::Remove) => None, + None => existing.function_name.clone(), + }; + if let Err(e) = validate_and_get_function_code( &mut conn, - updated_config.function_name.as_ref(), - &updated_config.key, - &updated_config.value, + function_name.as_ref(), + &key_str, + &value, &schema_name, ) { log::info!("Validation failed: {:?}", e); return Err(e); } - let version_id = + let (db_row, version_id) = conn.transaction::<_, superposition::AppError, _>(|transaction_conn| { - diesel::insert_into(dsl::default_configs) - .values(&updated_config) - .on_conflict(dsl::key) - .do_update() - .set(&updated_config) - .returning(DefaultConfig::as_returning()) + let val = diesel::update(dsl::default_configs) + .filter(dsl::key.eq(key_str.clone())) + .set(( + req.clone().as_changeset(), + dsl::last_modified_at.eq(Utc::now().naive_utc()), + dsl::last_modified_by.eq(user.get_email()), + )) .schema_name(&schema_name) - .execute(transaction_conn) - .map_err(|e| { - log::info!("Update failed: {e}"); - unexpected_error!("Failed to update DefaultConfig") - })?; + .get_result::(transaction_conn)?; let version_id = add_config_version( &state, @@ -265,7 +253,7 @@ async fn update_default_config( &schema_name, )?; - Ok(version_id) + Ok((val, version_id)) })?; #[cfg(feature = "high-performance-mode")] @@ -276,7 +264,7 @@ async fn update_default_config( AppHeader::XConfigVersion.to_string(), version_id.to_string(), )); - Ok(http_resp.json(updated_config)) + Ok(http_resp.json(db_row)) } fn validate_and_get_function_code( diff --git a/crates/context_aware_config/src/api/default_config/types.rs b/crates/context_aware_config/src/api/default_config/types.rs index c8a65174f..017462d24 100644 --- a/crates/context_aware_config/src/api/default_config/types.rs +++ b/crates/context_aware_config/src/api/default_config/types.rs @@ -1,7 +1,8 @@ use derive_more::{AsRef, Deref, DerefMut, Into}; +use diesel::AsChangeset; use serde::{Deserialize, Deserializer}; use serde_json::{Map, Value}; -use superposition_types::RegexEnum; +use superposition_types::{database::schema::default_configs, RegexEnum}; #[derive(Debug, Deserialize)] pub struct CreateReq { @@ -13,7 +14,7 @@ pub struct CreateReq { pub change_reason: String, } -#[derive(Debug, Deserialize)] +#[derive(Debug, Deserialize, Clone)] pub struct UpdateReq { #[serde(default, deserialize_with = "deserialize_option")] pub value: Option, @@ -23,6 +24,35 @@ pub struct UpdateReq { pub change_reason: String, } +impl UpdateReq { + pub fn as_changeset(self) -> UpdateReqChangeset { + UpdateReqChangeset { + value: self.value, + schema: self.schema.map(Value::Object), + function_name: match self.function_name { + Some(FunctionNameEnum::Name(val)) => Some(Some(val)), + Some(FunctionNameEnum::Remove) => Some(None), + _ => None, + }, + description: self.description, + change_reason: self.change_reason, + } + } +} + +//changeset type for update request type +#[derive(AsChangeset)] +#[diesel(table_name = default_configs)] +pub struct UpdateReqChangeset { + pub value: Option, + pub schema: Option, + //function_name is nullable column, so to support null update with changeset + //we had to make it Option> + pub function_name: Option>, + pub description: Option, + pub change_reason: String, +} + #[derive(Debug, Clone)] pub enum FunctionNameEnum { Name(String), diff --git a/crates/context_aware_config/src/api/dimension/handlers.rs b/crates/context_aware_config/src/api/dimension/handlers.rs index 44bd5634d..feb53789b 100644 --- a/crates/context_aware_config/src/api/dimension/handlers.rs +++ b/crates/context_aware_config/src/api/dimension/handlers.rs @@ -23,7 +23,7 @@ use superposition_types::{ use crate::{ api::dimension::{ - types::{CreateReq, FunctionNameEnum}, + types::{CreateReq, Position}, utils::{get_dimension_usage_context_ids, validate_dimension_position}, }, helpers::{get_workspace, validate_jsonschema}, @@ -143,10 +143,11 @@ async fn update( use dimensions::dsl; let DbConnection(mut conn) = db_conn; - let mut dimension_row: Dimension = dsl::dimensions + let existing_position: i32 = dsl::dimensions + .select(dimensions::position) .filter(dimensions::dimension.eq(name.clone())) .schema_name(&schema_name) - .get_result::(&mut conn)?; + .get_result::(&mut conn)?; let num_rows = dimensions .count() @@ -157,39 +158,27 @@ async fn update( db_error!(err) })?; - let update_req = req.into_inner(); + let mut update_req = req.into_inner(); - if let Some(schema_value) = update_req.schema { + if let Some(schema_value) = update_req.clone().schema { validate_jsonschema(&state.meta_schema, &schema_value)?; - dimension_row.schema = schema_value; } - dimension_row.change_reason = update_req.change_reason; - dimension_row.description = update_req - .description - .unwrap_or_else(|| dimension_row.description); - - dimension_row.function_name = match update_req.function_name { - Some(FunctionNameEnum::Name(func_name)) => Some(func_name), - Some(FunctionNameEnum::Remove) => None, - None => dimension_row.function_name, - }; - let result = conn.transaction::<_, superposition::AppError, _>(|transaction_conn| { - let mut new_position = dimension_row.position; + let mut new_position = existing_position.clone(); - if let Some(position_val) = update_req.position { + if let Some(position_val) = update_req.clone().position { new_position = position_val.clone().into(); validate_dimension_position( path.into_inner(), position_val, num_rows - 1, )?; - let previous_position = dimension_row.position; + let previous_position = existing_position.clone(); diesel::update(dimensions) - .filter(dsl::dimension.eq(&dimension_row.dimension)) + .filter(dsl::dimension.eq(name.clone())) .set(( dsl::last_modified_at.eq(Utc::now().naive_utc()), dsl::last_modified_by.eq(user.get_email()), @@ -225,17 +214,15 @@ async fn update( .execute(transaction_conn)? }; } - + let update_position = + Position::try_from(new_position).map_err(|err| unexpected_error!(err))?; + update_req.position = Some(update_position); diesel::update(dimensions) - .filter(dsl::dimension.eq(&dimension_row.dimension)) + .filter(dsl::dimension.eq(name)) .set(( + update_req.as_changeset(), dimensions::last_modified_at.eq(Utc::now().naive_utc()), dimensions::last_modified_by.eq(user.get_email()), - dimensions::function_name.eq(dimension_row.function_name), - dimensions::schema.eq(dimension_row.schema), - dimensions::position.eq(new_position), - dimensions::description.eq(dimension_row.description), - dimensions::change_reason.eq(dimension_row.change_reason), )) .returning(Dimension::as_returning()) .schema_name(&schema_name) diff --git a/crates/context_aware_config/src/api/dimension/types.rs b/crates/context_aware_config/src/api/dimension/types.rs index 6eb015d5f..56dd41911 100644 --- a/crates/context_aware_config/src/api/dimension/types.rs +++ b/crates/context_aware_config/src/api/dimension/types.rs @@ -1,7 +1,8 @@ use derive_more::{AsRef, Deref, DerefMut, Into}; +use diesel::AsChangeset; use serde::{Deserialize, Deserializer}; use serde_json::Value; -use superposition_types::RegexEnum; +use superposition_types::{database::schema::dimensions, RegexEnum}; #[derive(Debug, Deserialize)] pub struct CreateReq { @@ -48,6 +49,35 @@ pub struct UpdateReq { pub change_reason: String, } +impl UpdateReq { + pub fn as_changeset(self) -> UpdateReqChangeset { + UpdateReqChangeset { + position: self.position.map(|x| x.into()), + schema: self.schema, + function_name: match self.function_name { + Some(FunctionNameEnum::Name(val)) => Some(Some(val)), + Some(FunctionNameEnum::Remove) => Some(None), + _ => None, + }, + description: self.description, + change_reason: self.change_reason, + } + } +} + +//changeset type for update request type +#[derive(AsChangeset)] +#[diesel(table_name = dimensions)] +pub struct UpdateReqChangeset { + pub position: Option, + pub schema: Option, + //function_name is nullable column, so to support null update with changeset + //we had to make it Option> + pub function_name: Option>, + pub description: Option, + pub change_reason: String, +} + #[derive(Debug, Clone)] pub enum FunctionNameEnum { Name(String), diff --git a/crates/context_aware_config/src/api/functions/handlers.rs b/crates/context_aware_config/src/api/functions/handlers.rs index 742254ae2..e451a2f8f 100644 --- a/crates/context_aware_config/src/api/functions/handlers.rs +++ b/crates/context_aware_config/src/api/functions/handlers.rs @@ -116,47 +116,18 @@ async fn update( let req = request.into_inner(); let f_name: String = params.into_inner().into(); - let result = match fetch_function(&f_name, &mut conn, &schema_name) { - Ok(val) => val, - Err(superposition::AppError::DbError(diesel::result::Error::NotFound)) => { - log::error!("Function not found."); - return Err(bad_argument!("Function {} doesn't exists", f_name)); - } - Err(e) => { - log::error!("Failed to update Function with error: {e}"); - return Err(unexpected_error!("Failed to update Function")); - } - }; - // Function Linter Check if let Some(function) = &req.function { compile_fn(function)?; } - let new_function = Function { - function_name: f_name.to_owned(), - draft_code: req.function.map_or_else( - || result.draft_code.clone(), - |func| BASE64_STANDARD.encode(func), - ), - draft_runtime_version: req - .runtime_version - .unwrap_or(result.draft_runtime_version), - description: req.description.unwrap_or(result.description), - draft_edited_by: user.get_email(), - draft_edited_at: Utc::now().naive_utc(), - published_code: result.published_code, - published_at: result.published_at, - published_by: result.published_by, - published_runtime_version: result.published_runtime_version, - last_modified_at: Utc::now().naive_utc(), - last_modified_by: user.get_email(), - change_reason: req.change_reason, - }; - let mut updated_function = diesel::update(functions) .filter(schema::functions::function_name.eq(f_name)) - .set(new_function) + .set(( + req.as_changeset(), + dsl::draft_edited_by.eq(user.get_email()), + dsl::draft_edited_at.eq(Utc::now().naive_utc()), + )) .returning(Function::as_returning()) .schema_name(&schema_name) .get_result::(&mut conn)?; diff --git a/crates/context_aware_config/src/api/functions/types.rs b/crates/context_aware_config/src/api/functions/types.rs index 93d4503b3..8fb3b5144 100644 --- a/crates/context_aware_config/src/api/functions/types.rs +++ b/crates/context_aware_config/src/api/functions/types.rs @@ -1,7 +1,9 @@ +use base64::prelude::*; use derive_more::{AsRef, Deref, DerefMut, Into}; +use diesel::AsChangeset; use serde::{Deserialize, Serialize}; use serde_json::Value; -use superposition_types::RegexEnum; +use superposition_types::{database::schema::functions, RegexEnum}; #[derive(Debug, Deserialize)] pub struct UpdateFunctionRequest { @@ -11,6 +13,27 @@ pub struct UpdateFunctionRequest { pub change_reason: String, } +impl UpdateFunctionRequest { + pub fn as_changeset(self) -> UpdateFunctionRequestChangeset { + UpdateFunctionRequestChangeset { + draft_code: self.function.map(|x| BASE64_STANDARD.encode(x)), + draft_runtime_version: self.runtime_version, + description: self.description, + change_reason: self.change_reason, + } + } +} + +//changeset type for update request type +#[derive(AsChangeset)] +#[diesel(table_name = functions)] +pub struct UpdateFunctionRequestChangeset { + pub draft_code: Option, + pub draft_runtime_version: Option, + pub description: Option, + pub change_reason: String, +} + #[derive(Debug, Deserialize)] pub struct CreateFunctionRequest { pub function_name: FunctionName,