From 1e101e8faa9d4c463114ee39dade49b359caf0bc Mon Sep 17 00:00:00 2001 From: Naor Peled Date: Sat, 1 Mar 2025 22:01:59 +0200 Subject: [PATCH] fix(transactions): rollback upon query error --- README.md | 17 ++++ index.js | 64 ++++++++++++--- test/integration/transaction-js-error.spec.js | 81 +++++++++++++++++++ 3 files changed, 149 insertions(+), 13 deletions(-) create mode 100644 test/integration/transaction-js-error.spec.js diff --git a/README.md b/README.md index e45123c..797bdf6 100644 --- a/README.md +++ b/README.md @@ -383,6 +383,23 @@ let results = await mysql.transaction() If the record to `DELETE` doesn't exist, the `UPDATE` will not be performed. If the `UPDATE` fails, the `DELETE` will be rolled back. +JavaScript errors thrown inside query functions will also trigger a rollback. For example: + +```javascript +let results = await mysql.transaction() + .query('INSERT INTO table (x) VALUES(?)', [1]) + .query(() => { + if (someCondition) { + throw new Error('Invalid condition'); // This will trigger a rollback + } + return ['UPDATE table SET x = 1']; + }) + .rollback(e => { /* do something with the error */ }) // optional + .commit() // execute the queries +``` + +If `someCondition` is true, the error will be thrown, the transaction will be rolled back, and no queries will be executed. + **NOTE:** Transaction support is designed for InnoDB tables (default). Other table types may not behave as expected. ## Reusing Persistent Connections diff --git a/index.js b/index.js index d21a7b0..9543b1b 100644 --- a/index.js +++ b/index.js @@ -445,23 +445,61 @@ module.exports = (params) => { const commit = async (queries, rollback) => { let results = [] // keep track of results + let transactionStarted = false - // Start a transaction - await query('START TRANSACTION') + try { + // Start a transaction + await query('START TRANSACTION') + transactionStarted = true + + // Loop through queries + for (let i = 0; i < queries.length; i++) { + try { + // Get the query arguments by calling the function + const queryArgs = queries[i](results[results.length - 1], results) + + // If queryArgs is null or undefined, skip this query + if (queryArgs === null || queryArgs === undefined) { + continue + } - // Loop through queries - for (let i = 0; i < queries.length; i++) { - // Execute the queries, pass the rollback as context - let result = await query.apply({ rollback }, queries[i](results[results.length - 1], results)) - // Add the result to the main results accumulator - results.push(result) - } + // Execute the queries, pass the rollback as context + let result = await query.apply({ rollback }, queryArgs) + // Add the result to the main results accumulator + results.push(result) + } catch (err) { + // If there's a JavaScript error in the query function, roll back and rethrow + if (transactionStarted) { + await query('ROLLBACK') + } + throw err + } + } - // Commit our transaction - await query('COMMIT') + // Commit our transaction + await query('COMMIT') + + // Return the results + return results + } catch (err) { + // If there's an error during the transaction, roll back if needed + if (transactionStarted) { + try { + await query('ROLLBACK') + } catch (rollbackErr) { + // If rollback fails, log it but throw the original error + onError(rollbackErr) + } + } - // Return the results - return results + // Call the rollback handler if provided + if (typeof rollback === 'function') { + rollback(err) + } + + // Rethrow the original error + throw err + } } diff --git a/test/integration/transaction-js-error.spec.js b/test/integration/transaction-js-error.spec.js new file mode 100644 index 0000000..9e7769e --- /dev/null +++ b/test/integration/transaction-js-error.spec.js @@ -0,0 +1,81 @@ +const { expect } = require('chai'); +const { + createTestConnection, + setupTestTable, + cleanupTestTable, + closeConnection +} = require('./helpers/setup'); + +describe('Transaction JavaScript Error Tests', function () { + this.timeout(15000); + + let db; + const TEST_TABLE = 'test_transaction_js_error'; + const TABLE_SCHEMA = ` + id INT AUTO_INCREMENT PRIMARY KEY, + created_at DATETIME, + description VARCHAR(255) + `; + + before(async function () { + // Initialize the database connection + db = createTestConnection({ manageConns: false }); + await setupTestTable(db, TEST_TABLE, TABLE_SCHEMA); + }); + + after(async function () { + try { + await cleanupTestTable(db, TEST_TABLE); + } finally { + await closeConnection(db); + } + }); + + beforeEach(async function () { + // Clear the table before each test + await db.query(`TRUNCATE TABLE ${TEST_TABLE}`); + }); + + it('should not execute stale queries when a JavaScript error is thrown in a transaction', async function () { + const firstDate = new Date(2023, 0, 1); // January 1, 2023 + + // First transaction attempt with a JavaScript error + try { + await db + .transaction() + .query(() => [`INSERT INTO ${TEST_TABLE} (created_at, description) VALUES(?, ?)`, [firstDate, 'First transaction']]) + .query(() => { + throw new Error('Abort transaction'); + }) + .commit(); + + // Should not reach here + expect.fail('Transaction should have failed'); + } catch (error) { + expect(error.message).to.equal('Abort transaction'); + } + + // Check that no records were inserted from the failed transaction + const result1 = await db.query(`SELECT COUNT(*) as count FROM ${TEST_TABLE}`); + expect(result1[0].count).to.equal(0, 'No records should be inserted after the failed transaction'); + + // Second transaction - this one should succeed + const secondDate = new Date(2023, 1, 1); // February 1, 2023 + await db + .transaction() + .query(() => [`INSERT INTO ${TEST_TABLE} (created_at, description) VALUES(?, ?)`, [secondDate, 'Second transaction']]) + .commit(); + + // Check that only the second transaction's records were inserted + const result2 = await db.query(`SELECT * FROM ${TEST_TABLE} ORDER BY created_at`); + expect(result2.length).to.equal(1, 'Only one record should be inserted'); + expect(result2[0].description).to.equal('Second transaction', 'Only the second transaction should be committed'); + + // The key test: Make sure the first transaction's query wasn't executed + const firstTransactionRecords = await db.query( + `SELECT COUNT(*) as count FROM ${TEST_TABLE} WHERE description = ?`, + ['First transaction'] + ); + expect(firstTransactionRecords[0].count).to.equal(0, 'First transaction should not have been executed'); + }); +}); \ No newline at end of file