From 42ecd2ed3d919b3016416da6768b2a52aeaa5488 Mon Sep 17 00:00:00 2001
From: Marco Bubke <marco.bubke@qt.io>
Date: Wed, 4 Apr 2018 10:41:11 +0200
Subject: [PATCH] Clang: Fix reset for write and execute in the sqlite
 statement

We were not reset a statement if we got an exception. There are now test
for it.

Change-Id: Ife7b4437fece9369767605ba7387bd0564c1bb8d
Reviewed-by: Ivan Donchevskii <ivan.donchevskii@qt.io>
---
 src/libs/sqlite/sqlitebasestatement.cpp      |  9 +-----
 src/libs/sqlite/sqlitebasestatement.h        | 16 +++++++--
 src/libs/sqlite/sqlitedatabasebackend.cpp    |  2 +-
 src/libs/sqlite/sqlitetransaction.h          |  3 +-
 tests/unit/unittest/mocksqlitestatement.h    |  2 +-
 tests/unit/unittest/sqlitestatement-test.cpp | 34 ++++++++++++++++++++
 6 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/src/libs/sqlite/sqlitebasestatement.cpp b/src/libs/sqlite/sqlitebasestatement.cpp
index 15e4a980436..01cd43facb2 100644
--- a/src/libs/sqlite/sqlitebasestatement.cpp
+++ b/src/libs/sqlite/sqlitebasestatement.cpp
@@ -54,8 +54,7 @@ BaseStatement::BaseStatement(Utils::SmallStringView sqlStatement, Database &data
 
 void BaseStatement::deleteCompiledStatement(sqlite3_stmt *compiledStatement)
 {
-    if (compiledStatement)
-        sqlite3_finalize(compiledStatement);
+    sqlite3_finalize(compiledStatement);
 }
 
 class UnlockNotification
@@ -145,12 +144,6 @@ void BaseStatement::step() const
     next();
 }
 
-void BaseStatement::execute() const
-{
-    next();
-    reset();
-}
-
 int BaseStatement::columnCount() const
 {
     return m_columnCount;
diff --git a/src/libs/sqlite/sqlitebasestatement.h b/src/libs/sqlite/sqlitebasestatement.h
index 5e629e5ec9e..86d03fd8553 100644
--- a/src/libs/sqlite/sqlitebasestatement.h
+++ b/src/libs/sqlite/sqlitebasestatement.h
@@ -60,7 +60,6 @@ public:
 
     bool next() const;
     void step() const;
-    void execute() const;
     void reset() const;
 
     int fetchIntValue(int column) const;
@@ -159,6 +158,13 @@ class StatementImplementation : public BaseStatement
 public:
     using BaseStatement::BaseStatement;
 
+    void execute()
+    {
+        Resetter resetter{*this};
+        BaseStatement::next();
+        resetter.reset();
+    }
+
     void bindValues()
     {
     }
@@ -172,8 +178,10 @@ public:
     template<typename... ValueType>
     void write(const ValueType&... values)
     {
+        Resetter resetter{*this};
         bindValuesByIndex(1, values...);
-        BaseStatement::execute();
+        BaseStatement::next();
+        resetter.reset();
     }
 
     template<typename... ValueType>
@@ -185,8 +193,10 @@ public:
     template<typename... ValueType>
     void writeNamed(const ValueType&... values)
     {
+        Resetter resetter{*this};
         bindValuesByName(values...);
-        BaseStatement::execute();
+        BaseStatement::next();
+        resetter.reset();
     }
 
     template <typename ResultType,
diff --git a/src/libs/sqlite/sqlitedatabasebackend.cpp b/src/libs/sqlite/sqlitedatabasebackend.cpp
index c6400e53628..0e088c549df 100644
--- a/src/libs/sqlite/sqlitedatabasebackend.cpp
+++ b/src/libs/sqlite/sqlitedatabasebackend.cpp
@@ -178,7 +178,7 @@ void DatabaseBackend::setLastInsertedRowId(int64_t rowId)
 void DatabaseBackend::execute(Utils::SmallStringView sqlStatement)
 {
     ReadWriteStatement statement(sqlStatement, m_database);
-    statement.step();
+    statement.execute();
 }
 
 void DatabaseBackend::close()
diff --git a/src/libs/sqlite/sqlitetransaction.h b/src/libs/sqlite/sqlitetransaction.h
index 0c053a00375..5c2c0fe0f8f 100644
--- a/src/libs/sqlite/sqlitetransaction.h
+++ b/src/libs/sqlite/sqlitetransaction.h
@@ -64,6 +64,7 @@ public:
     {
         m_interface.commit();
         m_isAlreadyCommited = true;
+        m_locker.unlock();
     }
 
 protected:
@@ -76,7 +77,7 @@ protected:
 
 protected:
     TransactionInterface &m_interface;
-    std::lock_guard<TransactionInterface> m_locker{m_interface};
+    std::unique_lock<TransactionInterface> m_locker{m_interface};
     bool m_isAlreadyCommited = false;
     bool m_rollback = false;
 };
diff --git a/tests/unit/unittest/mocksqlitestatement.h b/tests/unit/unittest/mocksqlitestatement.h
index ab919b9f4e2..26f5b0de6e0 100644
--- a/tests/unit/unittest/mocksqlitestatement.h
+++ b/tests/unit/unittest/mocksqlitestatement.h
@@ -35,7 +35,6 @@ class BaseMockSqliteStatement
 public:
     MOCK_METHOD0(next, bool ());
     MOCK_METHOD0(step, void ());
-    MOCK_METHOD0(execute, void ());
     MOCK_METHOD0(reset, void ());
 
     MOCK_CONST_METHOD1(fetchIntValue, int (int));
@@ -53,6 +52,7 @@ public:
     MOCK_METHOD2(bind, void (int, double));
     MOCK_METHOD2(bind, void (int, Utils::SmallStringView));
     MOCK_METHOD2(bind, void (int, long));
+    MOCK_CONST_METHOD1(bindingIndexForName, int (Utils::SmallStringView name));
 
     MOCK_METHOD1(prepare, void (Utils::SmallStringView sqlStatement));
 };
diff --git a/tests/unit/unittest/sqlitestatement-test.cpp b/tests/unit/unittest/sqlitestatement-test.cpp
index f033c07ed73..cb8206d8757 100644
--- a/tests/unit/unittest/sqlitestatement-test.cpp
+++ b/tests/unit/unittest/sqlitestatement-test.cpp
@@ -639,6 +639,40 @@ TEST_F(SqliteStatement, ThrowExceptionOnlyInReset)
                  Sqlite::StatementHasError);
 }
 
+TEST_F(SqliteStatement, ResetIfWriteIsThrowingException)
+{
+    MockSqliteStatement mockStatement;
+
+    EXPECT_CALL(mockStatement, bind(1, TypedEq<Utils::SmallStringView>("bar")))
+            .WillOnce(Throw(Sqlite::StatementIsBusy("")));
+    EXPECT_CALL(mockStatement, reset());
+
+    ASSERT_ANY_THROW(mockStatement.write("bar"));
+}
+
+TEST_F(SqliteStatement, ResetIfWriteNamedIsThrowingException)
+{
+    MockSqliteStatement mockStatement;
+
+    EXPECT_CALL(mockStatement, bindingIndexForName(TypedEq<Utils::SmallStringView>("@foo")))
+            .WillOnce(Return(1));
+    EXPECT_CALL(mockStatement, bind(1, TypedEq<Utils::SmallStringView>("bar")))
+            .WillOnce(Throw(Sqlite::StatementIsBusy("")));
+    EXPECT_CALL(mockStatement, reset());
+
+    ASSERT_ANY_THROW(mockStatement.writeNamed("@foo", "bar"));
+}
+
+TEST_F(SqliteStatement, ResetIfExecuteThrowsException)
+{
+    MockSqliteStatement mockStatement;
+
+    EXPECT_CALL(mockStatement, next()).WillOnce(Throw(Sqlite::StatementIsBusy("")));
+    EXPECT_CALL(mockStatement, reset());
+
+    ASSERT_ANY_THROW(mockStatement.execute());
+}
+
 void SqliteStatement::SetUp()
 {
     database.execute("CREATE TABLE test(name TEXT UNIQUE, number NUMERIC, value NUMERIC)");