Sqlite: Transaction catch exception in destructor

Fixes coverity warning. We want to throw an exception in the destructor
in this case, which is save because it is a RAII pattern and holds
only trivial values. Actually this is the only way to communicate that
rollback was failing. It is quite rare that it will be happen without an
exception is already thrown.

There is a non throwing variant too if you want to use it as a class
member.

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

src/libs/sqlite/sqlitetransaction.h

@@ -27,6 +27,7 @@
 
 #include "sqliteglobal.h"
 
+#include <exception>
 #include <mutex>
 
 namespace Sqlite {
@@ -52,12 +53,6 @@ public:
 class AbstractTransaction
 {
 public:
-    ~AbstractTransaction()
-    {
-        if (!m_isAlreadyCommited)
-            m_interface.rollback();
-    }
-
     AbstractTransaction(const AbstractTransaction &) = delete;
     AbstractTransaction &operator=(const AbstractTransaction &) = delete;
 
@@ -73,39 +68,91 @@ protected:
     {
     }
 
-private:
+protected:
     TransactionInterface &m_interface;
     bool m_isAlreadyCommited = false;
 };
 
-class DeferredTransaction final : public AbstractTransaction
+class AbstractThrowingTransaction : public AbstractTransaction
 {
 public:
-    DeferredTransaction(TransactionInterface &interface)
+    ~AbstractThrowingTransaction() noexcept(false)
+    {
+        try {
+            if (!m_isAlreadyCommited)
+                m_interface.rollback();
+        } catch (...) {
+            if (!std::uncaught_exception())
+                throw;
+        }
+    }
+
+protected:
+    AbstractThrowingTransaction(TransactionInterface &interface)
         : AbstractTransaction(interface)
+    {
+    }
+};
+
+class AbstractNonThrowingDestructorTransaction : public AbstractTransaction
+{
+public:
+    ~AbstractNonThrowingDestructorTransaction()
+    {
+        try {
+            if (!m_isAlreadyCommited)
+                m_interface.rollback();
+        } catch (...) {
+        }
+    }
+
+protected:
+    AbstractNonThrowingDestructorTransaction(TransactionInterface &interface)
+        : AbstractTransaction(interface)
+    {
+    }
+};
+
+template <typename BaseTransaction>
+class BasicDeferredTransaction final : public BaseTransaction
+{
+public:
+    BasicDeferredTransaction(TransactionInterface &interface)
+        : BaseTransaction(interface)
     {
         interface.deferredBegin();
     }
 };
 
-class ImmediateTransaction final : public AbstractTransaction
+using DeferredTransaction = BasicDeferredTransaction<AbstractThrowingTransaction>;
+using DeferredNonThrowingDestructorTransaction = BasicDeferredTransaction<AbstractNonThrowingDestructorTransaction>;
+
+template <typename BaseTransaction>
+class BasicImmediateTransaction final : public BaseTransaction
 {
 public:
-    ImmediateTransaction(TransactionInterface &interface)
-        : AbstractTransaction(interface)
+    BasicImmediateTransaction(TransactionInterface &interface)
+        : BaseTransaction(interface)
     {
         interface.immediateBegin();
     }
 };
 
-class ExclusiveTransaction final : public AbstractTransaction
+using ImmediateTransaction = BasicImmediateTransaction<AbstractThrowingTransaction>;
+using ImmediateNonThrowingDestructorTransaction = BasicImmediateTransaction<AbstractNonThrowingDestructorTransaction>;
+
+template <typename BaseTransaction>
+class BasicExclusiveTransaction final : public BaseTransaction
 {
 public:
-    ExclusiveTransaction(TransactionInterface &interface)
-        : AbstractTransaction(interface)
+    BasicExclusiveTransaction(TransactionInterface &interface)
+        : BaseTransaction(interface)
     {
         interface.exclusiveBegin();
     }
 };
 
+using ExclusiveTransaction = BasicExclusiveTransaction<AbstractThrowingTransaction>;
+using ExclusiveNonThrowingDestructorTransaction = BasicExclusiveTransaction<AbstractNonThrowingDestructorTransaction>;
+
 } // namespace Sqlite

src/tools/clangrefactoringbackend/source/storagesqlitestatementfactory.h

@@ -110,7 +110,7 @@ public:
     }
 
 public:
-    Sqlite::ImmediateTransaction transaction;
+    Sqlite::ImmediateNonThrowingDestructorTransaction transaction;
     Database &database;
     Sqlite::Table newSymbolsTablet{createNewSymbolsTable()};
     Sqlite::Table newLocationsTable{createNewLocationsTable()};

tests/unit/unittest/sqlitetransaction-test.cpp

@@ -28,6 +28,7 @@
 #include "mocksqlitetransactionbackend.h"
 
 #include <sqlitetransaction.h>
+#include <sqliteexception.h>
 #include <mocksqlitedatabase.h>
 
 namespace {
@@ -35,11 +36,14 @@ namespace {
 using Sqlite::DeferredTransaction;
 using Sqlite::ImmediateTransaction;
 using Sqlite::ExclusiveTransaction;
+using Sqlite::DeferredNonThrowingDestructorTransaction;
+using Sqlite::ImmediateNonThrowingDestructorTransaction;
+using Sqlite::ExclusiveNonThrowingDestructorTransaction;
 
 class SqliteTransaction : public testing::Test
 {
 protected:
-    MockSqliteTransactionBackend mockTransactionBackend;
+    NiceMock<MockSqliteTransactionBackend> mockTransactionBackend;
 };
 
 TEST_F(SqliteTransaction, DeferredTransactionCommit)
@@ -102,6 +106,58 @@ TEST_F(SqliteTransaction, ExclusiveTransactionRollBack)
     ExclusiveTransaction transaction{mockTransactionBackend};
 }
 
+TEST_F(SqliteTransaction, DeferredTransactionBeginThrows)
+{
+    ON_CALL(mockTransactionBackend, deferredBegin())
+            .WillByDefault(Throw(Sqlite::Exception("foo")));
+
+    ASSERT_THROW(DeferredTransaction{mockTransactionBackend},
+                 Sqlite::Exception);
 }
 
+TEST_F(SqliteTransaction, ImmediateTransactionBeginThrows)
+{
+    ON_CALL(mockTransactionBackend, immediateBegin())
+            .WillByDefault(Throw(Sqlite::Exception("foo")));
 
+    ASSERT_THROW(ImmediateTransaction{mockTransactionBackend},
+                 Sqlite::Exception);
+}
+
+TEST_F(SqliteTransaction, ExclusiveTransactionBeginThrows)
+{
+    ON_CALL(mockTransactionBackend, exclusiveBegin())
+            .WillByDefault(Throw(Sqlite::Exception("foo")));
+
+    ASSERT_THROW(ExclusiveTransaction{mockTransactionBackend},
+                 Sqlite::Exception);
+}
+
+TEST_F(SqliteTransaction, TransactionCommitThrows)
+{
+    ON_CALL(mockTransactionBackend, commit())
+            .WillByDefault(Throw(Sqlite::Exception("foo")));
+    ImmediateTransaction transaction{mockTransactionBackend};
+
+    ASSERT_THROW(transaction.commit(),
+                 Sqlite::Exception);
+}
+
+TEST_F(SqliteTransaction, TransactionRollbackInDestructorThrows)
+{
+    ON_CALL(mockTransactionBackend, rollback())
+            .WillByDefault(Throw(Sqlite::Exception("foo")));
+
+    ASSERT_THROW(ExclusiveTransaction{mockTransactionBackend},
+                 Sqlite::Exception);
+}
+
+TEST_F(SqliteTransaction, TransactionRollbackInDestructorDontThrows)
+{
+    ON_CALL(mockTransactionBackend, rollback())
+            .WillByDefault(Throw(Sqlite::Exception("foo")));
+
+    ASSERT_NO_THROW(ExclusiveNonThrowingDestructorTransaction{mockTransactionBackend});
+}
+
+}