feat: added description and comment columns in all existing tables and apis #284

sauraww · 2024-11-14T11:01:28Z

Problem

Added description and comment columns in all existing tables and apis

Solution

Provide a brief summary of your solution so that reviewers can understand your code

Environment variable changes

What ENVs need to be added or changed

Pre-deployment activity

Things needed to be done before deploying this change (if any)

Post-deployment activity

Things needed to be done after deploying this change (if any)

API changes

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

mahatoankitkumar · 2024-11-14T11:35:37Z

...ntext_aware_config/migrations/2024-11-06-123105_add_description_and_comment_columns/down.sql

@@ -0,0 +1,19 @@
+-- This file should undo anything in `up.sql`
+-- contexts table


Are these needed?

No , I will remove it

mahatoankitkumar

Lets make the fields non-nullable, as per the discussion.

Datron

Description should be mandatory. I think comment too but we can discuss this

Datron · 2024-11-18T11:59:05Z

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

@@ -378,9 +378,21 @@ fn construct_new_payload(
            },
        )?;

+    let description = res
+        .get("description")
+        .and_then(|val| val.as_str())


This returns result right? We should handle that

Datron · 2024-11-18T11:59:24Z

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

+    let description = res
+        .get("description")
+        .and_then(|val| val.as_str())
+        .map(|s| s.to_string());


You can do this inside and_then

Datron · 2024-11-18T12:01:31Z

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

+    pub description: Option<String>,
+    pub comment: Option<String>,


This two should be mandatory in this case, the frontend should just send ""

pratikmishra356 · 2024-12-18T13:55:11Z

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

+    let existing_context = contexts_table
+        .filter(context_id.eq(context_id_value))
+        .first::<Context>(transaction_conn)
+        .optional()?; // Query the database for existing context


why we are making this optional ?
it should return db error

pratikmishra356 · 2024-12-18T13:57:19Z

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

+    match existing_context {
+        Some(ctx) => Ok(ctx.description),
+        None => Err(superposition::AppError::NotFound(format!(
+            "Description Not Provided in existing context",


error should be something different because if context is there , description will also be there
so if db gives not found error , then we will return description not provided
if something else error then we can say something went wrong , or could not fetch description

pratikmishra356 · 2024-12-18T14:02:40Z

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

+            let mut req_mut = req.into_inner();
+
+            // Use the helper function to ensure the description
+            if req_mut.description.is_none() {


not a blocker , also this can be argued
here can we do like this ,
let description = either from request_mut or if it is none then from ensure_description , also assign in req_mut.description
so that here description will be of type and we will not have to use unwrap_or_default
also declare change_reason here only, so that we wont have to cline the whole req_mut

pratikmishra356 · 2024-12-18T14:06:55Z

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

+    let description = if req.description.is_none() {
+        ensure_description(ctx_condition_value.clone(), conn)?
+    } else {
+        req.description.expect("Description should not be empty")


just a suggestion
I know this will work , but if we can use match expression or abstract the some part without expect or unwrap
would be great

expect will cause a panic, this error message won't be logged properly as well. We should throw an AppError here

pratikmishra356 · 2024-12-18T14:19:57Z

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

                        log::error!(
                            "Failed to execute query while recomputing weight, error: {err}"
                        );
                        db_error!(err)
                    })?;
            }
-            let version_id = add_config_version(&state, tags, transaction_conn)?;
+            let description = contexts_new_weight.iter().map(|(_, _, description, _)| description.clone()).collect::<Vec<String>>().join(",");


instead of using change_reason and description of all contexts,
lets add only this
description: weight recomputed
change_reason: weight recomputed

pratikmishra356 · 2024-12-18T14:20:45Z

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

@@ -75,12 +81,24 @@ pub struct FunctionsInfo {
    pub code: Option<String>,
 }

+#[derive(Serialize)]


we are not using this anymore

Datron

Main concern: the error message for a context description not being found.

Nitpicks: no expect calls though they are safe since it throws off clippy checks

Datron · 2025-01-03T09:34:31Z

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

+            log::error!("construct new payload: Description is not a valid string");
+            return Err(bad_argument!("Description must be a string"));
+        }
+        None => None,


@sauraww construct_new_payload function is used by reduce, I don't know if we can just get the description like this. I think we'll need to combine all the descriptions being reduced into one or

insert a new description

Are you suggesting that we take it as input in the reduce api itself ?

Datron · 2025-01-03T09:37:40Z

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

+    } else {
+        req.description
+            .clone()
+            .expect("Description should not be empty")


No expects, throw an error here

Datron · 2025-01-03T09:39:42Z

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

@@ -198,6 +198,15 @@ fn create_ctx_from_put_req(
    tenant_config: &TenantConfig,
 ) -> superposition::Result<Context> {
    let ctx_condition = req.context.to_owned().into_inner();
+    let description = if req.description.is_none() {
+        let ctx_condition_value = Value::Object(ctx_condition.clone().into());


nitpick: would json!(ctx_condition) work?

Datron · 2025-01-03T09:43:34Z

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

+        Err(diesel::result::Error::NotFound) => Err(superposition::AppError::NotFound(
+            "Description not provided in existing context".to_string(),
+        )),


NotFound error message is confusing. A context was not found, but this message indicates only the description is missing. Probably based on the message the error code should be bad request

Datron · 2025-01-03T09:47:00Z

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

+    let description = if req.description.is_none() {
+        ensure_description(ctx_condition_value.clone(), conn)?
+    } else {
+        req.description.expect("Description should not be empty")


expect will cause a panic, this error message won't be logged properly as well. We should throw an AppError here

Datron · 2025-01-03T09:47:42Z

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

+                        } else {
+                            put_req
+                                .description
+                                .expect("Description should not be empty")


No expect

Datron · 2025-01-03T09:51:01Z

crates/superposition_types/src/database/diesel.toml

@@ -1,6 +1,6 @@
 [print_schema]
 file = "schema.rs"
-patch_file = "schema.patch"
+#patch_file = "schema.patch"


we need schema.patch right?

unavailable

sauraww requested a review from a team as a code owner November 14, 2024 11:01

sauraww force-pushed the feat/add-description-and-commit branch from e80798d to 8f36918 Compare November 14, 2024 11:02

mahatoankitkumar approved these changes Nov 14, 2024

View reviewed changes

sauraww force-pushed the feat/add-description-and-commit branch 11 times, most recently from a37e9b2 to dfc8172 Compare November 14, 2024 15:02

mahatoankitkumar previously requested changes Nov 15, 2024

View reviewed changes

Datron reviewed Nov 18, 2024

View reviewed changes

sauraww force-pushed the feat/add-description-and-commit branch 14 times, most recently from c3b2999 to 0dd0666 Compare November 21, 2024 12:57

pratikmishra356 requested review from Datron and pratikmishra356 December 17, 2024 10:15

sauraww force-pushed the feat/add-description-and-commit branch 3 times, most recently from aeb97be to a9ecce7 Compare December 18, 2024 13:36

sauraww changed the base branch from main to saas December 18, 2024 13:37

pratikmishra356 reviewed Dec 18, 2024

View reviewed changes

sauraww force-pushed the feat/add-description-and-commit branch 3 times, most recently from 15db62c to a5b0055 Compare December 18, 2024 19:47

Datron force-pushed the saas branch from 061b47f to 9270000 Compare January 2, 2025 08:36

Datron requested a review from pratikmishra356 January 2, 2025 10:11

sauraww force-pushed the feat/add-description-and-commit branch 4 times, most recently from 8a27413 to ab85eb7 Compare January 3, 2025 08:10

Datron approved these changes Jan 3, 2025

View reviewed changes

feat: added description and comment

8d03179

sauraww force-pushed the feat/add-description-and-commit branch from ab85eb7 to 8d03179 Compare January 3, 2025 10:49

pratikmishra356 approved these changes Jan 3, 2025

View reviewed changes

sauraww merged commit 8311f36 into saas Jan 3, 2025
4 checks passed

sauraww deleted the feat/add-description-and-commit branch January 3, 2025 11:15

ShubhranshuSanjeev pushed a commit that referenced this pull request Jan 15, 2025

feat: added description and comment (#284)

f9a2b9c

Datron pushed a commit that referenced this pull request Jan 20, 2025

feat: added description and comment (#284)

e98473f

Datron pushed a commit that referenced this pull request Jan 23, 2025

feat: added description and comment (#284)

01ba0f5

Datron pushed a commit that referenced this pull request Jan 23, 2025

feat: added description and comment (#284)

4e0006c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added description and comment columns in all existing tables and apis #284

feat: added description and comment columns in all existing tables and apis #284

sauraww commented Nov 14, 2024

mahatoankitkumar Nov 14, 2024

sauraww Nov 14, 2024

mahatoankitkumar left a comment

Datron left a comment

Datron Nov 18, 2024

Datron Nov 18, 2024

Datron Nov 18, 2024

pratikmishra356 Dec 18, 2024

pratikmishra356 Dec 18, 2024

pratikmishra356 Dec 18, 2024 •

edited

Loading

pratikmishra356 Dec 18, 2024 •

edited

Loading

Datron Jan 3, 2025

pratikmishra356 Dec 18, 2024

pratikmishra356 Dec 18, 2024

Datron left a comment

Datron Jan 3, 2025

sauraww Jan 3, 2025

Datron Jan 3, 2025

Datron Jan 3, 2025

Datron Jan 3, 2025

Datron Jan 3, 2025

Datron Jan 3, 2025

Datron Jan 3, 2025

		@@ -0,0 +1,19 @@
		-- This file should undo anything in `up.sql`
		-- contexts table

		pub description: Option<String>,
		pub comment: Option<String>,

feat: added description and comment columns in all existing tables and apis #284

feat: added description and comment columns in all existing tables and apis #284

Conversation

sauraww commented Nov 14, 2024

Problem

Solution

Environment variable changes

Pre-deployment activity

Post-deployment activity

API changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahatoankitkumar left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratikmishra356 Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

pratikmishra356 Dec 18, 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

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

pratikmishra356 Dec 18, 2024 •

edited

Loading

pratikmishra356 Dec 18, 2024 •

edited

Loading