diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a06cd25..a1ed3981 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Remove the deprecated `Savepoint::rollback()` alias; use `rollbackTo()` instead (#585) +### Fixed + +- Track manually rolled-back transactions as finished and report destructor rollback failures through assertions (#586) + ## [3.4.0] - 2026-09-21 ### Added diff --git a/include/SQLiteCpp/Transaction.h b/include/SQLiteCpp/Transaction.h index 7cb5cfc0..d987df9d 100644 --- a/include/SQLiteCpp/Transaction.h +++ b/include/SQLiteCpp/Transaction.h @@ -76,7 +76,7 @@ class SQLITECPP_API Transaction Transaction& operator=(const Transaction&) = delete; /** - * @brief Safely rollback the transaction if it has not been committed. + * @brief Safely rollback the transaction if it has not already been committed or rolled back. */ ~Transaction(); @@ -92,7 +92,7 @@ class SQLITECPP_API Transaction private: Database& mDatabase; ///< Reference to the SQLite Database Connection - bool mbCommited = false; ///< True when commit has been called + bool mbFinished = false; ///< True when the transaction has been committed or rolled back }; } // namespace SQLite diff --git a/src/Transaction.cpp b/src/Transaction.cpp index 31ea1e2e..82166466 100644 --- a/src/Transaction.cpp +++ b/src/Transaction.cpp @@ -46,46 +46,42 @@ Transaction::Transaction(Database &aDatabase) : mDatabase.exec("BEGIN TRANSACTION"); } -// Safely rollback the transaction if it has not been committed. +// Safely rollback the transaction if it has not already been committed or rolled back. Transaction::~Transaction() { - if (false == mbCommited) + if (!mbFinished) { - try - { - mDatabase.exec("ROLLBACK TRANSACTION"); - } - catch (...) - { - // Never throw an exception in a destructor: error if already rollbacked, but no harm is caused by this. - } + const int ret = mDatabase.tryExec("ROLLBACK TRANSACTION"); + (void)ret; // Avoid an unused-variable warning when assertions are disabled. + SQLITECPP_ASSERT(SQLITE_OK == ret, mDatabase.getErrorMsg()); } } // Commit the transaction. void Transaction::commit() { - if (false == mbCommited) + if (!mbFinished) { mDatabase.exec("COMMIT TRANSACTION"); - mbCommited = true; + mbFinished = true; } else { - throw SQLite::Exception("Transaction already committed."); + throw SQLite::Exception("Transaction already finished."); } } // Rollback the transaction void Transaction::rollback() { - if (false == mbCommited) + if (!mbFinished) { mDatabase.exec("ROLLBACK TRANSACTION"); + mbFinished = true; } else { - throw SQLite::Exception("Transaction already committed."); + throw SQLite::Exception("Transaction already finished."); } } diff --git a/tests/Transaction_test.cpp b/tests/Transaction_test.cpp index 65256069..b6e8d4a1 100644 --- a/tests/Transaction_test.cpp +++ b/tests/Transaction_test.cpp @@ -38,10 +38,10 @@ TEST(Transaction, commitRollback) // Commit transaction transaction.commit(); - // Commit again throw an exception + // Committing an already finished transaction throws. EXPECT_THROW(transaction.commit(), SQLite::Exception); - // Rollback after commit also throws an exception + // Rolling back an already finished transaction also throws. EXPECT_THROW(transaction.rollback(), SQLite::Exception); } @@ -59,29 +59,29 @@ TEST(Transaction, commitRollback) EXPECT_THROW(SQLite::Transaction(db, static_cast(-1)), SQLite::Exception); } - // Auto rollback if no commit() before the end of scope + // Automatic rollback if commit() is not called before the end of scope. { // Begin transaction SQLite::Transaction transaction(db); - // Insert a second value (that will be rollbacked) + // Insert a second value (that will be rolled back) EXPECT_EQ(1, db.exec("INSERT INTO test VALUES (NULL, 'third')")); EXPECT_EQ(2, db.getLastInsertRowid()); // end of scope: automatic rollback } - // Auto rollback of a transaction on error/exception + // Automatic rollback when leaving scope because of an exception. try { // Begin transaction SQLite::Transaction transaction(db); - // Insert a second value (that will be rollbacked) + // Insert a second value (that will be rolled back) EXPECT_EQ(1, db.exec("INSERT INTO test VALUES (NULL, 'second')")); EXPECT_EQ(2, db.getLastInsertRowid()); - // Execute with an error => exception with auto-rollback + // Trigger an exception; stack unwinding destroys the transaction and rolls it back. db.exec("DesiredSyntaxError to raise an exception to rollback the transaction"); GTEST_FATAL_FAILURE_("we should never get there"); @@ -93,22 +93,19 @@ TEST(Transaction, commitRollback) // expected error, see above } - // Double rollback with a manual command before the end of scope + // Manual rollback before the end of scope { - // Begin transaction SQLite::Transaction transaction(db); - // Insert a second value (that will be rollbacked) + // Insert a second value that will be rolled back. EXPECT_EQ(1, db.exec("INSERT INTO test VALUES (NULL, 'third')")); EXPECT_EQ(2, db.getLastInsertRowid()); - // Execute a manual rollback + // A manual rollback finishes the transaction; the destructor has nothing left to do. transaction.rollback(); - - // end of scope: the automatic rollback should not raise an error because it is harmless } - // Check the results (expect only one row of result, as all other one have been rollbacked) + // Only the explicitly committed first row should remain; all later rows were rolled back. SQLite::Statement query(db, "SELECT * FROM test"); int nbRows = 0; while (query.executeStep()) @@ -119,3 +116,27 @@ TEST(Transaction, commitRollback) } EXPECT_EQ(1, nbRows); } + +TEST(Transaction, manualRollbackFinishesTransaction) +{ + SQLite::Database db(":memory:", SQLite::OPEN_READWRITE | SQLite::OPEN_CREATE); + db.exec("CREATE TABLE test (id INTEGER PRIMARY KEY, value TEXT)"); + + { + SQLite::Transaction transaction(db); + transaction.rollback(); + + // The Transaction object is finished after rollback(). A new transaction + // on the same connection must therefore be left untouched by its destructor. + db.exec("BEGIN TRANSACTION"); + EXPECT_EQ(1, db.exec("INSERT INTO test VALUES (NULL, 'kept')")); + } + + // If the finished Transaction destructor issued another ROLLBACK, this COMMIT + // would fail and the inserted row would be lost. + EXPECT_NO_THROW(db.exec("COMMIT TRANSACTION")); + + SQLite::Statement query(db, "SELECT value FROM test"); + ASSERT_TRUE(query.executeStep()); + EXPECT_STREQ("kept", query.getColumn(0).getText()); +}