Sqlite: Introduce implicit transaction

Sqlite adds an implicit transaction if no transaction is used. That can
be problematic if you use multiple statements but it is okay for one
read statement.

Write and read write statements should use immediate transactions
because the acquire the write lock. Otherwise the write lock is only
acquired as you try to write. From there it is much harder to recover.

Change-Id: I04b0be7447f2b82b6921738d789c33cbbaa8de6e
Reviewed-by: Thomas Hartmann <thomas.hartmann@qt.io>
Reviewed-by: Qt CI Patch Build Bot <ci_patchbuild_bot@qt.io>

src/libs/sqlite/sqlitereadstatement.h

@@ -34,7 +34,7 @@ public:
     template<typename ResultType, typename... QueryTypes>
     auto valueWithTransaction(const QueryTypes &...queryValues)
     {
-        return withDeferredTransaction(Base::database(), [&] {
+        return withImplicitTransaction(Base::database(), [&] {
             return Base::template value<ResultType>(queryValues...);
         });
     }
@@ -42,7 +42,7 @@ public:
     template<typename ResultType, typename... QueryTypes>
     auto optionalValueWithTransaction(const QueryTypes &...queryValues)
     {
-        return withDeferredTransaction(Base::database(), [&] {
+        return withImplicitTransaction(Base::database(), [&] {
             return Base::template optionalValue<ResultType>(queryValues...);
         });
     }
@@ -50,7 +50,7 @@ public:
     template<typename ResultType, std::size_t capacity = 32, typename... QueryTypes>
     auto valuesWithTransaction(const QueryTypes &...queryValues)
     {
-        return withDeferredTransaction(Base::database(), [&] {
+        return withImplicitTransaction(Base::database(), [&] {
             return Base::template values<ResultType, capacity>(queryValues...);
         });
     }
@@ -58,7 +58,7 @@ public:
     template<typename Callable, typename... QueryTypes>
     void readCallbackWithTransaction(Callable &&callable, const QueryTypes &...queryValues)
     {
-        withDeferredTransaction(Base::database(), [&] {
+        withImplicitTransaction(Base::database(), [&] {
             Base::readCallback(std::forward<Callable>(callable), queryValues...);
         });
     }
@@ -66,7 +66,7 @@ public:
     template<typename Container, typename... QueryTypes>
     void readToWithTransaction(Container &container, const QueryTypes &...queryValues)
     {
-        withDeferredTransaction(Base::database(), [&] { Base::readTo(container, queryValues...); });
+        withImplicitTransaction(Base::database(), [&] { Base::readTo(container, queryValues...); });
     }
 
 protected:

src/libs/sqlite/sqlitetransaction.h

@@ -67,6 +67,24 @@ protected:
     bool m_rollback = false;
 };
 
+template<typename TransactionInterface>
+class ImplicitTransaction
+{
+public:
+    using Transaction = TransactionInterface;
+
+    ~ImplicitTransaction() = default;
+    ImplicitTransaction(TransactionInterface &transactionInterface)
+        : m_locker(transactionInterface)
+    {}
+
+    ImplicitTransaction(const ImplicitTransaction &) = delete;
+    ImplicitTransaction &operator=(const ImplicitTransaction &) = delete;
+
+protected:
+    std::unique_lock<TransactionInterface> m_locker;
+};
+
 template<typename TransactionInterface>
 class AbstractThrowingSessionTransaction
 {
@@ -201,6 +219,18 @@ auto withTransaction(TransactionInterface &transactionInterface, Callable &&call
     }
 }
 
+template<typename TransactionInterface, typename Callable>
+auto withImplicitTransaction(TransactionInterface &transactionInterface, Callable &&callable)
+{
+    ImplicitTransaction transaction{transactionInterface};
+
+    if constexpr (std::is_void_v<std::invoke_result_t<Callable>>) {
+        callable();
+    } else {
+        return callable();
+    }
+}
+
 template<typename TransactionInterface, typename Callable>
 auto withDeferredTransaction(TransactionInterface &transactionInterface, Callable &&callable)
 {

tests/unit/tests/unittests/sqlite/sqlitetransaction-test.cpp

@@ -323,6 +323,59 @@ TEST_F(SqliteTransaction, immediate_session_transaction_begin_throws_and_not_rol
     ASSERT_ANY_THROW(ImmediateSessionTransaction{mockTransactionBackend});
 }
 
+TEST_F(SqliteTransaction, with_implicit_transaction_no_return_does_not_commit)
+{
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
+    EXPECT_CALL(mockTransactionBackend, deferredBegin()).Times(0);
+    EXPECT_CALL(callableMock, Call());
+    EXPECT_CALL(mockTransactionBackend, commit()).Times(0);
+    EXPECT_CALL(mockTransactionBackend, unlock());
+
+    Sqlite::withImplicitTransaction(mockTransactionBackend, callableMock.AsStdFunction());
+}
+
+TEST_F(SqliteTransaction, with_implicit_transaction_with_return_does_not_commit)
+{
+    InSequence s;
+
+    EXPECT_CALL(mockTransactionBackend, lock());
+    EXPECT_CALL(mockTransactionBackend, deferredBegin()).Times(0);
+    EXPECT_CALL(callableWithReturnMock, Call());
+    EXPECT_CALL(mockTransactionBackend, commit()).Times(0);
+    EXPECT_CALL(mockTransactionBackend, unlock());
+
+    Sqlite::withImplicitTransaction(mockTransactionBackend, callableWithReturnMock.AsStdFunction());
+}
+
+TEST_F(SqliteTransaction, with_implicit_transaction_returns_value)
+{
+    auto callable = callableWithReturnMock.AsStdFunction();
+
+    auto value = Sqlite::withImplicitTransaction(mockTransactionBackend,
+                                                 callableWithReturnMock.AsStdFunction());
+
+    ASSERT_THAT(value, Eq(212));
+}
+
+TEST_F(SqliteTransaction, with_implicit_transaction_do_calls_rollsback_for_exception)
+{
+    InSequence s;
+    ON_CALL(callableMock, Call()).WillByDefault(Throw(std::exception{}));
+
+    EXPECT_CALL(mockTransactionBackend, lock());
+    EXPECT_CALL(mockTransactionBackend, deferredBegin()).Times(0);
+    EXPECT_CALL(callableMock, Call());
+    EXPECT_CALL(mockTransactionBackend, rollback()).Times(0);
+    EXPECT_CALL(mockTransactionBackend, unlock());
+
+    try {
+        Sqlite::withImplicitTransaction(mockTransactionBackend, callableMock.AsStdFunction());
+    } catch (...) {
+    }
+}
+
 TEST_F(SqliteTransaction, with_deferred_transaction_no_return_commit)
 {
     InSequence s;