[WEB-2706] fix: Fix issue with SQLite transactions#5919
Conversation
WalkthroughThis pull request introduces several enhancements to the asynchronous handling of database operations across multiple files. In Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
web/core/local-db/utils/load-issues.ts (1)
108-130: Good use of Promise.all for concurrent metadata operations.The refactoring to use Promise.all for metadata operations is a good improvement for performance. However, consider adding error handling to ensure all promises are properly handled.
Consider adding error handling:
const metaPromises: Promise<any>[] = []; ARRAY_FIELDS.forEach((field) => { const values = issue[field]; if (values && values.length) { values.forEach((val: any) => { 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 { 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); + try { + await Promise.all(metaPromises); + } catch (error) { + console.error('Failed to insert metadata:', error); + throw error; + }🧰 Tools
🪛 Biome
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- web/core/local-db/utils/load-issues.ts (3 hunks)
- web/core/local-db/utils/utils.ts (1 hunks)
- web/core/local-db/worker/db.ts (3 hunks)
🧰 Additional context used
🪛 Biome
web/core/local-db/utils/load-issues.ts
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
web/core/local-db/utils/utils.ts (1)
62-62: Verify transaction handling in the database layer.Since this PR aims to fix SQLite transaction issues, let's verify the transaction handling in the related database operations.
✅ Verification successful
Let me gather more specific information about the database operations and transaction handling in the local-db layer.
Let me search more specifically for the transaction handling and updateIssue implementation.
Let me search for the addIssue implementation since it's called by updateIssue, and also look for any database initialization code.
Based on the code analysis, I can now provide a final response about the transaction handling:
Transaction handling is properly implemented in the database layer.
The code shows proper transaction management:
updateIssuefunction uses transactions through its called functions:
- Calls
deleteIssueFromLocalwhich uses explicitBEGIN/COMMITtransaction- Calls
addIssuewhich also uses explicitBEGIN/COMMITtransactionAll major database operations use explicit transactions:
addIssue: UsesBEGIN;andCOMMIT;addIssuesBulk: UsesBEGIN;andCOMMIT;for batch operationsdeleteIssueFromLocal: UsesBEGIN;andCOMMIT;- Table creation: Uses
BEGIN;andCOMMIT;- Workspace data loading: Uses
BEGIN TRANSACTION;andCOMMITThe implementation ensures data consistency by wrapping related operations in transactions, particularly for operations that modify multiple tables like issue updates which affect both the issues and issue_meta tables.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for transaction-related code in the database layer echo "Searching for transaction handling in database operations..." rg -l "transaction|begin|commit|rollback" --type ts # Look for potential transaction overlap points echo "Checking for concurrent database operations..." ast-grep --pattern 'await $db.$_($$$)'Length of output: 1439
Script:
#!/bin/bash # Search for transaction-related code specifically in the local-db directory echo "Searching for transaction handling in local-db..." rg -A 5 "transaction|begin|commit|rollback" web/core/local-db/ # Look for updateIssue implementation echo "Looking for updateIssue implementation..." ast-grep --pattern 'updateIssue = async ($$$) => { $$$ }' # Search for any database operation wrappers echo "Searching for database operation wrappers..." rg -A 5 "class.*Database|interface.*Database" web/core/local-db/Length of output: 131567
Script:
#!/bin/bash # Look for updateIssue implementation and surrounding context echo "Searching for updateIssue implementation with context..." rg -A 10 "updateIssue" web/core/local-db/ # Look for transaction-related code in local-db echo "Searching for transaction handling..." rg -A 5 "transaction|begin|commit|rollback" web/core/local-db/ --type ts # Look for database wrapper implementation echo "Looking for database wrapper..." rg -A 10 "class.*Database" web/core/local-db/ --type tsLength of output: 8932
Script:
#!/bin/bash # Look for addIssue implementation echo "Searching for addIssue implementation..." rg -A 15 "addIssue" web/core/local-db/ # Look for database initialization and setup echo "Searching for database initialization..." rg -A 10 "persistence.db|initDB|setupDB" web/core/local-db/ # Look for SQL statements related to issues echo "Searching for issue-related SQL..." rg -A 5 "INSERT INTO.*issues|UPDATE.*issues|DELETE FROM.*issues" web/core/local-db/Length of output: 19765
web/core/local-db/worker/db.ts (1)
100-103: Ensure proper resolution of transaction promises on commitIn the
COMMIT;block, the code resolves the first resolver fromtpResolver. If there is any misalignment betweentpandtpResolver, this could cause unexpected behavior.To verify that transaction promises and resolvers are correctly managed, run the following script to check for potential mismatches:
Ensure that after every push or shift operation, the lengths of
tpandtpResolverremain consistent.✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the concern.
Transaction promises and resolvers are correctly synchronized
The code maintains proper alignment between
tpandtpResolverarrays:
- In the
BEGIN;block:
- A new promise is created and pushed to
tp- Its resolver is simultaneously pushed to
tpResolver- In the
COMMIT;block:
- The code checks
tp.length > 0before attempting to resolve- Both arrays are modified using shift() operations in sync
The implementation ensures one-to-one correspondence between promises and their resolvers, preventing any misalignment issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the lengths of the transaction promises and resolvers arrays are always equal. # Test: Search for all modifications to tp and tpResolver and check their lengths. rg 'this\.tp' -A 3 -B 3 rg 'this\.tpResolver' -A 3 -B 3Length of output: 2195
| // delete the issue and its meta data | ||
| await deleteIssueFromLocal(issue_id); | ||
| addIssue(issue); | ||
| await addIssue(issue); |
There was a problem hiding this comment.
🛠️ 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.
| 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;"); |
There was a problem hiding this comment.
Add SQL injection protection and error handling.
The current implementation has two security/reliability concerns:
- Direct string interpolation in SQL queries makes it vulnerable to SQL injection
- 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.
| 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; | |
| } |
|
|
||
| const query = `INSERT OR REPLACE INTO issues (${columns}) VALUES (${values});`; | ||
| persistence.db.exec(query); | ||
| await persistence.db.exec(query); |
There was a problem hiding this comment.
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.
| "description_html", | ||
| ]); | ||
| updateIssue({ ...issuePartial, is_local_update: 1 }); | ||
| await updateIssue({ ...issuePartial, is_local_update: 1 }); |
There was a problem hiding this comment.
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:
- Uses
Promise.allwithmapto properly wait for all updates - Adds error handling for individual update failures
- 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.
| 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 | |
| } | |
| } | |
| })); | |
| }; |
| private tp: Promise<any>[] = []; | ||
| private tpResolver: any = []; |
There was a problem hiding this comment.
🛠️ 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.
| private tp: Promise<any>[] = []; | |
| private tpResolver: any = []; | |
| private transactionPromises: Promise<void>[] = []; | |
| private transactionResolvers: Array<{ resolve: Function; reject: Function }> = []; |
| 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; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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.
| 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; | |
| } | |
| } |
3 participants
Implement transaction Q to avoid overlapped transactions.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor