Clang: Cleanup transaction statements in the database

Change-Id: I795248492fe952c230fd3a9cf8bf16f3c831f161
Reviewed-by: Ivan Donchevskii <ivan.donchevskii@qt.io>

src/libs/sqlite/sqlitedatabase.cpp

@@ -35,13 +35,29 @@ using namespace std::chrono_literals;
 
 namespace Sqlite {
 
+class Database::Statements
+{
+public:
+    Statements(Database &database)
+        : database(database)
+    {}
+
+public:
+    Database &database;
+    ReadWriteStatement deferredBegin{"BEGIN", database};
+    ReadWriteStatement immediateBegin{"BEGIN IMMEDIATE", database};
+    ReadWriteStatement exclusiveBegin{"BEGIN EXCLUSIVE", database};
+    ReadWriteStatement commitBegin{"COMMIT", database};
+    ReadWriteStatement rollbackBegin{"ROLLBACK", database};
+};
+
 Database::Database()
     : m_databaseBackend(*this)
 {
 }
 
 Database::Database(Utils::PathString &&databaseFilePath, JournalMode journalMode)
-    : Database(std::move(databaseFilePath), 0ms, journalMode)
+    : Database(std::move(databaseFilePath), 1000ms, journalMode)
 {
 }
 
@@ -76,6 +92,7 @@ void Database::open(Utils::PathString &&databaseFilePath)
 void Database::close()
 {
     m_isOpen = false;
+    deleteTransactionStatements();
     m_databaseBackend.close();
 }
 
@@ -147,40 +164,45 @@ void Database::initializeTables()
 
 void Database::registerTransactionStatements()
 {
-    m_deferredBeginStatement = std::make_unique<ReadWriteStatement>("BEGIN", *this);
+    m_statements = std::make_unique<Statements>(*this);
-    m_immediateBeginStatement = std::make_unique<ReadWriteStatement>("BEGIN IMMEDIATE", *this);
+}
-    m_exclusiveBeginStatement = std::make_unique<ReadWriteStatement>("BEGIN EXCLUSIVE", *this);
+
-    m_commitBeginStatement = std::make_unique<ReadWriteStatement>("COMMIT", *this);
+void Database::deleteTransactionStatements()
-    m_rollbackBeginStatement = std::make_unique<ReadWriteStatement>("ROLLBACK", *this);
+{
+    m_statements.reset();
 }
 
 void Database::deferredBegin()
 {
-    m_databaseMutex.lock();
+    m_statements->deferredBegin.execute();
-    m_deferredBeginStatement->execute();
 }
 
 void Database::immediateBegin()
 {
-    m_databaseMutex.lock();
+    m_statements->immediateBegin.execute();
-    m_immediateBeginStatement->execute();
 }
 
 void Database::exclusiveBegin()
 {
-    m_databaseMutex.lock();
+    m_statements->exclusiveBegin.execute();
-    m_exclusiveBeginStatement->execute();
 }
 
 void Database::commit()
 {
-    m_commitBeginStatement->execute();
+    m_statements->commitBegin.execute();
-    m_databaseMutex.unlock();
 }
 
 void Database::rollback()
 {
-    m_rollbackBeginStatement->execute();
+    m_statements->rollbackBegin.execute();
+}
+
+void Database::lock()
+{
+    m_databaseMutex.lock();
+}
+void Database::unlock()
+{
     m_databaseMutex.unlock();
 }
 

src/libs/sqlite/sqlitedatabase.h

@@ -38,9 +38,10 @@
 
 namespace Sqlite {
 
+using namespace std::chrono_literals;
+
 class ReadStatement;
 class WriteStatement;
-class ReadWriteStatement;
 
 class SQLITE_EXPORT Database final : public TransactionInterface
 {
@@ -55,10 +56,10 @@ public:
 
     Database();
     Database(Utils::PathString &&databaseFilePath,
-             JournalMode journalMode=JournalMode::Delete);
+             JournalMode journalMode=JournalMode::Wal);
     Database(Utils::PathString &&databaseFilePath,
-             std::chrono::milliseconds busyTimeout = {},
+             std::chrono::milliseconds busyTimeout = 1000ms,
-             JournalMode journalMode=JournalMode::Delete);
+             JournalMode journalMode=JournalMode::Wal);
     ~Database();
 
     Database(const Database &) = delete;
@@ -106,27 +107,28 @@ public:
         return m_databaseBackend.totalChangesCount();
     }
 
+private:
     void deferredBegin();
     void immediateBegin();
     void exclusiveBegin();
     void commit();
     void rollback();
+    void lock();
+    void unlock();
 
-private:
     void initializeTables();
     void registerTransactionStatements();
+    void deleteTransactionStatements();
     std::mutex &databaseMutex() { return m_databaseMutex; }
 
+    class Statements;
+
 private:
     Utils::PathString m_databaseFilePath;
     DatabaseBackend m_databaseBackend;
     std::vector<Table> m_sqliteTables;
-    std::unique_ptr<ReadWriteStatement> m_deferredBeginStatement;
-    std::unique_ptr<ReadWriteStatement> m_immediateBeginStatement;
-    std::unique_ptr<ReadWriteStatement> m_exclusiveBeginStatement;
-    std::unique_ptr<ReadWriteStatement> m_commitBeginStatement;
-    std::unique_ptr<ReadWriteStatement> m_rollbackBeginStatement;
     std::mutex m_databaseMutex;
+    std::unique_ptr<Statements> m_statements;
     std::chrono::milliseconds m_busyTimeout;
     JournalMode m_journalMode = JournalMode::Wal;
     OpenMode m_openMode = OpenMode::ReadWrite;

src/libs/sqlite/sqlitetransaction.h

@@ -47,6 +47,8 @@ public:
     virtual void exclusiveBegin() = 0;
     virtual void commit() = 0;
     virtual void rollback() = 0;
+    virtual void lock() = 0;
+    virtual void unlock() = 0;
 
 protected:
     ~TransactionInterface() = default;
@@ -74,6 +76,7 @@ protected:
 
 protected:
     TransactionInterface &m_interface;
+    std::lock_guard<TransactionInterface> m_locker{m_interface};
     bool m_isAlreadyCommited = false;
     bool m_rollback = false;
 };

tests/unit/unittest/filepathstorage-test.cpp

@@ -419,11 +419,15 @@ TEST_F(FilePathStorage, RestartFetchSourceNameIfTheDatabaseIsBusyInBegin)
 {
     InSequence s;
 
+    EXPECT_CALL(mockDatabase, lock());
     EXPECT_CALL(mockDatabase, deferredBegin()).WillOnce(Throw(Sqlite::StatementIsBusy("busy")));
     EXPECT_CALL(mockDatabase, rollback()).Times(0);
+    EXPECT_CALL(mockDatabase, unlock());
+    EXPECT_CALL(mockDatabase, lock());
     EXPECT_CALL(mockDatabase, deferredBegin());
     EXPECT_CALL(selectSourceNameFromSourcesBySourceId, valueReturnSmallString(42));
     EXPECT_CALL(mockDatabase, commit());
+    EXPECT_CALL(mockDatabase, unlock());
 
     storage.fetchSourceName(42);
 }
@@ -432,10 +436,15 @@ TEST_F(FilePathStorage, RestartFetchDirectoryPathIfTheDatabaseIsBusyInBegin)
 {
     InSequence s;
 
+    EXPECT_CALL(mockDatabase, lock());
     EXPECT_CALL(mockDatabase, deferredBegin()).WillOnce(Throw(Sqlite::StatementIsBusy("busy")));
     EXPECT_CALL(mockDatabase, rollback()).Times(0);
+    EXPECT_CALL(mockDatabase, unlock());
+    EXPECT_CALL(mockDatabase, lock());
     EXPECT_CALL(mockDatabase, deferredBegin());
-    EXPECT_CALL(selectDirectoryPathFromDirectoriesByDirectoryId, valueReturnPathString(5));    EXPECT_CALL(mockDatabase, commit());
+    EXPECT_CALL(selectDirectoryPathFromDirectoriesByDirectoryId, valueReturnPathString(5));
+    EXPECT_CALL(mockDatabase, commit());
+    EXPECT_CALL(mockDatabase, unlock());
 
     storage.fetchDirectoryPath(5);
 }
@@ -444,11 +453,15 @@ TEST_F(FilePathStorage, RestartFetchAllDirectoriesIfBeginIsBusy)
 {
     InSequence s;
 
+    EXPECT_CALL(mockDatabase, lock());
     EXPECT_CALL(mockDatabase, deferredBegin()).WillOnce(Throw(Sqlite::StatementIsBusy("busy")));
     EXPECT_CALL(mockDatabase, rollback()).Times(0);
+    EXPECT_CALL(mockDatabase, unlock());
+    EXPECT_CALL(mockDatabase, lock());
     EXPECT_CALL(mockDatabase, deferredBegin());
     EXPECT_CALL(selectAllDirectories, valuesReturnStdVectorDirectory(256));
     EXPECT_CALL(mockDatabase, commit());
+    EXPECT_CALL(mockDatabase, unlock());
 
     storage.fetchAllDirectories();
 }
@@ -457,11 +470,15 @@ TEST_F(FilePathStorage, RestartFetchAllSourcesIfBeginIsBusy)
 {
     InSequence s;
 
+    EXPECT_CALL(mockDatabase, lock());
     EXPECT_CALL(mockDatabase, deferredBegin()).WillOnce(Throw(Sqlite::StatementIsBusy("busy")));
     EXPECT_CALL(mockDatabase, rollback()).Times(0);
+    EXPECT_CALL(mockDatabase, unlock());
+    EXPECT_CALL(mockDatabase, lock());
     EXPECT_CALL(mockDatabase, deferredBegin());
     EXPECT_CALL(selectAllSources, valuesReturnStdVectorSource(8192));
     EXPECT_CALL(mockDatabase, commit());
+    EXPECT_CALL(mockDatabase, unlock());
 
     storage.fetchAllSources();
 }

tests/unit/unittest/mocksqlitetransactionbackend.h

@@ -38,4 +38,6 @@ public:
     MOCK_METHOD0(exclusiveBegin, void ());
     MOCK_METHOD0(commit, void ());
     MOCK_METHOD0(rollback, void ());
+    MOCK_METHOD0(lock, void ());
+    MOCK_METHOD0(unlock, void ());
 };

tests/unit/unittest/sqlitedatabase-test.cpp

@@ -47,12 +47,27 @@ using Sqlite::Table;
 class SqliteDatabase : public ::testing::Test
 {
 protected:
-    void SetUp() override;
+    void SetUp() override
-    void TearDown() override;
+    {
+        database.setJournalMode(JournalMode::Memory);
+        database.setDatabaseFilePath(databaseFilePath);
+        auto &table = database.addTable();
+        table.setName("test");
+        table.addColumn("name");
+
+        database.open();
+    }
+
+    void TearDown() override
+    {
+        if (database.isOpen())
+            database.close();
+    }
 
     SpyDummy spyDummy;
-    QString databaseFilePath = QStringLiteral(":memory:");
+    QString databaseFilePath{":memory:"};
     Sqlite::Database database;
+    Sqlite::TransactionInterface &transactionInterface = database;
 };
 
 TEST_F(SqliteDatabase, SetDatabaseFilePath)
@@ -142,53 +157,37 @@ TEST_F(SqliteDatabase, LastRowId)
 
 TEST_F(SqliteDatabase, DeferredBegin)
 {
-    ASSERT_ANY_THROW(database.deferredBegin());
+    ASSERT_NO_THROW(transactionInterface.deferredBegin());
 
-    database.commit();
+    transactionInterface.commit();
 }
 
 TEST_F(SqliteDatabase, ImmediateBegin)
 {
-    ASSERT_ANY_THROW(database.immediateBegin());
+    ASSERT_NO_THROW(transactionInterface.immediateBegin());
 
-    database.commit();
+    transactionInterface.commit();
 }
 
 TEST_F(SqliteDatabase, ExclusiveBegin)
 {
-    ASSERT_ANY_THROW(database.exclusiveBegin());
+    ASSERT_NO_THROW(transactionInterface.exclusiveBegin());
 
-    database.commit();
+    transactionInterface.commit();
 }
 
 TEST_F(SqliteDatabase, Commit)
 {
-    database.deferredBegin();
+    transactionInterface.deferredBegin();
 
-    ASSERT_ANY_THROW(database.commit());
+    ASSERT_NO_THROW(transactionInterface.commit());
 }
 
 TEST_F(SqliteDatabase, Rollback)
 {
-    database.deferredBegin();
+    transactionInterface.deferredBegin();
 
-    ASSERT_ANY_THROW(database.rollback());
+    ASSERT_NO_THROW(transactionInterface.rollback());
 }
 
-void SqliteDatabase::SetUp()
-{
-    database.setJournalMode(JournalMode::Memory);
-    database.setDatabaseFilePath(databaseFilePath);
-    auto &table = database.addTable();
-    table.setName("test");
-    table.addColumn("name");
-
-    database.open();
-}
-
-void SqliteDatabase::TearDown()
-{
-    if (database.isOpen())
-        database.close();
-}
 }

tests/unit/unittest/sqlitetransaction-test.cpp

@@ -48,8 +48,12 @@ protected:
 
 TEST_F(SqliteTransaction, DeferredTransactionCommit)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, deferredBegin());
     EXPECT_CALL(mockTransactionBackend, commit());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     DeferredTransaction transaction{mockTransactionBackend};
     transaction.commit();
@@ -57,8 +61,12 @@ TEST_F(SqliteTransaction, DeferredTransactionCommit)
 
 TEST_F(SqliteTransaction, DeferredTransactionCommitCallsInterface)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, deferredBegin());
     EXPECT_CALL(mockTransactionBackend, commit());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     DeferredTransaction transaction{mockTransactionBackend};
     transaction.commit();
@@ -66,16 +74,24 @@ TEST_F(SqliteTransaction, DeferredTransactionCommitCallsInterface)
 
 TEST_F(SqliteTransaction, DeferredTransactionRollBack)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, deferredBegin());
     EXPECT_CALL(mockTransactionBackend, rollback());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     DeferredTransaction transaction{mockTransactionBackend};
 }
 
 TEST_F(SqliteTransaction, ImmediateTransactionCommit)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, immediateBegin());
     EXPECT_CALL(mockTransactionBackend, commit());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     ImmediateTransaction transaction{mockTransactionBackend};
     transaction.commit();
@@ -83,16 +99,24 @@ TEST_F(SqliteTransaction, ImmediateTransactionCommit)
 
 TEST_F(SqliteTransaction, ImmediateTransactionRollBack)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, immediateBegin());
     EXPECT_CALL(mockTransactionBackend, rollback());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     ImmediateTransaction transaction{mockTransactionBackend};
 }
 
 TEST_F(SqliteTransaction, ExclusiveTransactionCommit)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, exclusiveBegin());
     EXPECT_CALL(mockTransactionBackend, commit());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     ExclusiveTransaction transaction{mockTransactionBackend};
     transaction.commit();
@@ -100,16 +124,24 @@ TEST_F(SqliteTransaction, ExclusiveTransactionCommit)
 
 TEST_F(SqliteTransaction, ExclusiveTransactionRollBack)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, exclusiveBegin());
     EXPECT_CALL(mockTransactionBackend, rollback());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     ExclusiveTransaction transaction{mockTransactionBackend};
 }
 
 TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionCommit)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, deferredBegin());
     EXPECT_CALL(mockTransactionBackend, commit());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     DeferredNonThrowingDestructorTransaction transaction{mockTransactionBackend};
     transaction.commit();
@@ -117,8 +149,12 @@ TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionCommit)
 
 TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionCommitCallsInterface)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, deferredBegin());
     EXPECT_CALL(mockTransactionBackend, commit());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     DeferredNonThrowingDestructorTransaction transaction{mockTransactionBackend};
     transaction.commit();
@@ -126,16 +162,24 @@ TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionCommitCallsInt
 
 TEST_F(SqliteTransaction, DeferredNonThrowingDestructorTransactionRollBack)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, deferredBegin());
     EXPECT_CALL(mockTransactionBackend, rollback());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     DeferredNonThrowingDestructorTransaction transaction{mockTransactionBackend};
 }
 
 TEST_F(SqliteTransaction, ImmediateNonThrowingDestructorTransactionCommit)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, immediateBegin());
     EXPECT_CALL(mockTransactionBackend, commit());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     ImmediateNonThrowingDestructorTransaction transaction{mockTransactionBackend};
     transaction.commit();
@@ -143,16 +187,24 @@ TEST_F(SqliteTransaction, ImmediateNonThrowingDestructorTransactionCommit)
 
 TEST_F(SqliteTransaction, ImmediateNonThrowingDestructorTransactionRollBack)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, immediateBegin());
     EXPECT_CALL(mockTransactionBackend, rollback());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     ImmediateNonThrowingDestructorTransaction transaction{mockTransactionBackend};
 }
 
 TEST_F(SqliteTransaction, ExclusiveNonThrowingDestructorTransactionCommit)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, exclusiveBegin());
     EXPECT_CALL(mockTransactionBackend, commit());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     ExclusiveNonThrowingDestructorTransaction transaction{mockTransactionBackend};
     transaction.commit();
@@ -160,8 +212,12 @@ TEST_F(SqliteTransaction, ExclusiveNonThrowingDestructorTransactionCommit)
 
 TEST_F(SqliteTransaction, ExclusiveTNonThrowingDestructorransactionRollBack)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, exclusiveBegin());
     EXPECT_CALL(mockTransactionBackend, rollback());
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     ExclusiveNonThrowingDestructorTransaction transaction{mockTransactionBackend};
 }
@@ -195,30 +251,40 @@ TEST_F(SqliteTransaction, ExclusiveTransactionBeginThrows)
 
 TEST_F(SqliteTransaction, DeferredTransactionBeginThrowsAndNotRollback)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, deferredBegin())
             .WillOnce(Throw(Sqlite::Exception("foo")));
-
     EXPECT_CALL(mockTransactionBackend, rollback()).Times(0);
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     ASSERT_ANY_THROW(DeferredTransaction{mockTransactionBackend});
 }
 
 TEST_F(SqliteTransaction, ImmediateTransactionBeginThrowsAndNotRollback)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, immediateBegin())
             .WillOnce(Throw(Sqlite::Exception("foo")));
-
     EXPECT_CALL(mockTransactionBackend, rollback()).Times(0);
+    EXPECT_CALL(mockTransactionBackend, unlock());
+
 
     ASSERT_ANY_THROW(ImmediateTransaction{mockTransactionBackend});
 }
 
 TEST_F(SqliteTransaction, ExclusiveTransactionBeginThrowsAndNotRollback)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
     EXPECT_CALL(mockTransactionBackend, exclusiveBegin())
             .WillOnce(Throw(Sqlite::Exception("foo")));
-
     EXPECT_CALL(mockTransactionBackend, rollback()).Times(0);
+    EXPECT_CALL(mockTransactionBackend, unlock());
 
     ASSERT_ANY_THROW(ExclusiveTransaction{mockTransactionBackend});
 }