Skip to content

[WEB-2706] fix: Fix issue with SQLite transactions #5919

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.

Sign up for GitHub

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

Merged
pushya22 merged 1 commit into from
Oct 28, 2024
Merged

[WEB-2706] fix: Fix issue with SQLite transactions #5919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions web/core/local-db/utils/load-issues.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,14 @@ export const addIssuesBulk = async (issues: any, batchSize = 100) => {
export const deleteIssueFromLocal = async (issue_id: any) => {
if (!rootStore.user.localDBEnabled || !persistence.db) return;

const deleteQuery = `delete from issues where id='${issue_id}'`;
const deleteQuery = `DELETE from issues where id='${issue_id}'`;
const deleteMetaQuery = `delete from issue_meta where issue_id='${issue_id}'`;

persistence.db.exec("BEGIN;");
persistence.db.exec(deleteQuery);
persistence.db.exec(deleteMetaQuery);
persistence.db.exec("COMMIT;");
await persistence.db.exec("BEGIN;");

await persistence.db.exec(deleteQuery);
await persistence.db.exec(deleteMetaQuery);
await persistence.db.exec("COMMIT;");
Comment on lines +43 to +50
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add SQL injection protection and error handling.

The current implementation has two security/reliability concerns:

  1. Direct string interpolation in SQL queries makes it vulnerable to SQL injection
  2. Missing error handling could leave transactions in an inconsistent state

Consider refactoring to use parameterized queries and add error handling:

 export const deleteIssueFromLocal = async (issue_id: any) => {
   if (!rootStore.user.localDBEnabled || !persistence.db) return;
 
-  const deleteQuery = `DELETE from issues where id='${issue_id}'`;
-  const deleteMetaQuery = `delete from issue_meta where issue_id='${issue_id}'`;
+  try {
+    await persistence.db.exec("BEGIN;");
 
-  await persistence.db.exec("BEGIN;");
+    await persistence.db.exec({
+      sql: "DELETE FROM issues WHERE id = ?",
+      bind: [issue_id]
+    });
+    await persistence.db.exec({
+      sql: "DELETE FROM issue_meta WHERE issue_id = ?",
+      bind: [issue_id]
+    });
 
-  await persistence.db.exec(deleteQuery);
-  await persistence.db.exec(deleteMetaQuery);
-  await persistence.db.exec("COMMIT;");
+    await persistence.db.exec("COMMIT;");
+  } catch (error) {
+    await persistence.db.exec("ROLLBACK;");
+    throw error;
+  }
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const deleteQuery = `DELETE from issues where id='${issue_id}'`;
const deleteMetaQuery = `delete from issue_meta where issue_id='${issue_id}'`;
persistence.db.exec("BEGIN;");
persistence.db.exec(deleteQuery);
persistence.db.exec(deleteMetaQuery);
persistence.db.exec("COMMIT;");
await persistence.db.exec("BEGIN;");
await persistence.db.exec(deleteQuery);
await persistence.db.exec(deleteMetaQuery);
await persistence.db.exec("COMMIT;");
try {
await persistence.db.exec("BEGIN;");
await persistence.db.exec({
sql: "DELETE FROM issues WHERE id = ?",
bind: [issue_id]
});
await persistence.db.exec({
sql: "DELETE FROM issue_meta WHERE issue_id = ?",
bind: [issue_id]
});
await persistence.db.exec("COMMIT;");
} catch (error) {
await persistence.db.exec("ROLLBACK;");
throw error;
}

};
// @todo: Update deletes the issue description from local. Implement a separate update.
export const updateIssue = async (issue: TIssue & { is_local_update: number }) => {
Expand All @@ -55,7 +56,7 @@ export const updateIssue = async (issue: TIssue & { is_local_update: number }) =
const issue_id = issue.id;
// delete the issue and its meta data
await deleteIssueFromLocal(issue_id);
addIssue(issue);
await addIssue(issue);
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for the update operation.

While the await addition is correct, consider adding error handling to ensure atomicity of the delete-then-add operation.

 export const updateIssue = async (issue: TIssue & { is_local_update: number }) => {
   if (document.hidden || !rootStore.user.localDBEnabled || !persistence.db) return;
 
   const issue_id = issue.id;
-  // delete the issue and its meta data
-  await deleteIssueFromLocal(issue_id);
-  await addIssue(issue);
+  try {
+    await persistence.db.exec("BEGIN;");
+    await deleteIssueFromLocal(issue_id);
+    await addIssue(issue);
+    await persistence.db.exec("COMMIT;");
+  } catch (error) {
+    await persistence.db.exec("ROLLBACK;");
+    throw error;
+  }
 };

Committable suggestion was skipped due to low confidence.

};

export const syncDeletesToLocal = async (workspaceId: string, projectId: string, queries: any) => {
Expand Down Expand Up @@ -98,27 +99,33 @@ const stageIssueInserts = async (issue: any) => {
.join(", ");

const query = `INSERT OR REPLACE INTO issues (${columns}) VALUES (${values});`;
persistence.db.exec(query);
await persistence.db.exec(query);
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Protect against SQL injection in the main issue insert.

The main issue insert query is constructed using string interpolation, making it vulnerable to SQL injection.

Consider using parameterized queries here as well:

-  const query = `INSERT OR REPLACE INTO issues (${columns}) VALUES (${values});`;
-  await persistence.db.exec(query);
+  await persistence.db.exec({
+    sql: `INSERT OR REPLACE INTO issues (${columns}) VALUES (${placeholders})`,
+    bind: Object.values(sanitizedIssue)
+  });

Committable suggestion was skipped due to low confidence.


await persistence.db.exec({
sql: `DELETE from issue_meta where issue_id='${issue_id}'`,
});

const metaPromises: Promise<any>[] = [];

ARRAY_FIELDS.forEach((field) => {
const values = issue[field];
if (values && values.length) {
values.forEach((val: any) => {
persistence.db.exec({
const p = persistence.db.exec({
sql: `INSERT OR REPLACE into issue_meta(issue_id,key,value) values (?,?,?) `,
bind: [issue_id, field, val],
});
metaPromises.push(p);
});
} else {
// Added for empty fields?
persistence.db.exec({
const p = persistence.db.exec({
sql: `INSERT OR REPLACE into issue_meta(issue_id,key,value) values (?,?,?) `,
bind: [issue_id, field, ""],
});
metaPromises.push(p);
}
});

await Promise.all(metaPromises);
};
2 changes: 1 addition & 1 deletion web/core/local-db/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export const updatePersistentLayer = async (issueIds: string | string[]) => {
"type_id",
"description_html",
]);
updateIssue({ ...issuePartial, is_local_update: 1 });
await updateIssue({ ...issuePartial, is_local_update: 1 });
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix async operation handling in forEach loop.

While adding await to updateIssue is a step in the right direction, using forEach with async operations is problematic as it doesn't wait for all promises to complete and can lead to race conditions.

Refactor the function to properly handle concurrent updates:

 export const updatePersistentLayer = async (issueIds: string | string[]) => {
   if (typeof issueIds === "string") {
     issueIds = [issueIds];
   }
-  issueIds.forEach(async (issueId) => {
+  await Promise.all(issueIds.map(async (issueId) => {
     const dbIssue = await persistence.getIssue(issueId);
     const issue = rootStore.issue.issues.getIssueById(issueId);
 
     if (issue) {
       const issuePartial = pick({ ...dbIssue, ...JSON.parse(JSON.stringify(issue)) }, [
         // ... fields ...
       ]);
 
-      await updateIssue({ ...issuePartial, is_local_update: 1 });
+      try {
+        await updateIssue({ ...issuePartial, is_local_update: 1 });
+      } catch (error) {
+        logError(error);
+        throw error; // Re-throw to ensure Promise.all catches the failure
+      }
     }
-  });
+  }));
 };

This change:

  1. Uses Promise.all with map to properly wait for all updates
  2. Adds error handling for individual update failures
  3. Prevents transaction overlap by ensuring all updates complete before the function returns
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await updateIssue({ ...issuePartial, is_local_update: 1 });
export const updatePersistentLayer = async (issueIds: string | string[]) => {
if (typeof issueIds === "string") {
issueIds = [issueIds];
}
await Promise.all(issueIds.map(async (issueId) => {
const dbIssue = await persistence.getIssue(issueId);
const issue = rootStore.issue.issues.getIssueById(issueId);
if (issue) {
const issuePartial = pick({ ...dbIssue, ...JSON.parse(JSON.stringify(issue)) }, [
// ... fields ...
]);
try {
await updateIssue({ ...issuePartial, is_local_update: 1 });
} catch (error) {
logError(error);
throw error; // Re-throw to ensure Promise.all catches the failure
}
}
}));
};

}
});
};
Expand Down
31 changes: 19 additions & 12 deletions web/core/local-db/worker/db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ interface SQLiteInstance {
export class DBClass {
private instance: SQLiteInstance = {} as SQLiteInstance;
private sqlite3: any;
private tp: Promise<any> | null = null;
private tpResolver: any;
private tp: Promise<any>[] = [];
private tpResolver: any = [];
Comment on lines +27 to +28
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use more descriptive variable names and explicit types for clarity

The variables tp and tpResolver are not descriptive, which can make the code harder to understand and maintain. Additionally, tpResolver is typed as any, reducing type safety.

Consider renaming and typing the variables to improve readability and maintainability:

- private tp: Promise<any>[] = [];
- private tpResolver: any = [];
+ private transactionPromises: Promise<void>[] = [];
+ private transactionResolvers: Array<{ resolve: Function; reject: Function }> = [];
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private tp: Promise<any>[] = [];
private tpResolver: any = [];
private transactionPromises: Promise<void>[] = [];
private transactionResolvers: Array<{ resolve: Function; reject: Function }> = [];

async init(dbName: string) {
if (!dbName || typeof dbName !== "string") {
throw new Error("Invalid database name");
Expand Down Expand Up @@ -57,8 +57,19 @@ export class DBClass {
}

async exec(props: string | TQueryProps) {
if (this.tp && props === "BEGIN;") {
await this.tp;
if (props === "BEGIN;") {
let promiseToAwait;
if (this.tp.length > 0) {
promiseToAwait = this.tp.shift();
}
const p = new Promise((resolve, reject) => {
this.tpResolver.push({ resolve, reject });
});
this.tp.push(p);

if (promiseToAwait) {
await promiseToAwait;
}
Comment on lines +60 to +72
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Refactor transaction management to prevent potential misalignment issues

Managing transactions using separate arrays for promises and resolvers (tp and tpResolver) may lead to misalignment and bugs if not handled carefully. This approach can make the code complex and harder to debug.

Refactor the transaction management to use a unified structure that pairs each promise with its resolver. This enhances code clarity and reduces the risk of synchronization issues.

Apply the following changes:

- if (props === "BEGIN;") {
-   let promiseToAwait;
-   if (this.tp.length > 0) {
-     promiseToAwait = this.tp.shift();
-   }
-   const p = new Promise((resolve, reject) => {
-     this.tpResolver.push({ resolve, reject });
-   });
-   this.tp.push(p);
-
-   if (promiseToAwait) {
-     await promiseToAwait;
-   }
- }

+ if (props === "BEGIN;") {
+   let previousTransaction;
+   if (this.transactionQueue.length > 0) {
+     previousTransaction = this.transactionQueue[this.transactionQueue.length - 1].promise;
+   }
+   let resolveFunction;
+   const promise = new Promise<void>((resolve, reject) => {
+     resolveFunction = resolve;
+   });
+   this.transactionQueue.push({ promise, resolve: resolveFunction });
+
+   if (previousTransaction) {
+     await previousTransaction;
+   }
+ }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (props === "BEGIN;") {
let promiseToAwait;
if (this.tp.length > 0) {
promiseToAwait = this.tp.shift();
}
const p = new Promise((resolve, reject) => {
this.tpResolver.push({ resolve, reject });
});
this.tp.push(p);
if (promiseToAwait) {
await promiseToAwait;
}
if (props === "BEGIN;") {
let previousTransaction;
if (this.transactionQueue.length > 0) {
previousTransaction = this.transactionQueue[this.transactionQueue.length - 1].promise;
}
let resolveFunction;
const promise = new Promise<void>((resolve, reject) => {
resolveFunction = resolve;
});
this.transactionQueue.push({ promise, resolve: resolveFunction });
if (previousTransaction) {
await previousTransaction;
}
}

}
let sql: string, bind: any[];
if (typeof props === "string") {
Expand All @@ -84,16 +95,12 @@ export class DBClass {
}
}

if (sql === "BEGIN;") {
this.tp = new Promise((resolve, reject) => {
this.tpResolver = { resolve, reject };
});
}

if (sql === "COMMIT;" && this.tp) {
await this.instance.exec(sql);
this.tpResolver.resolve();
this.tp = null;
if (this.tp.length > 0) {
const { resolve } = this.tpResolver.shift();
resolve();
}
return;
}
return await this.instance.exec(sql);
Expand Down