Sqlite: Improve SqliteStatement column check

We have done it for every getter. Now we do it only once as we ask for
the values. It simplifies the code and the test and could even improve
performance.

Change-Id: Ia7d4a33a77ec7c0a5fda548424fbf8b192f07511
Reviewed-by: Thomas Hartmann <thomas.hartmann@qt.io>

src/libs/sqlite/sqlitebasestatement.cpp

@@ -41,11 +41,10 @@
 namespace Sqlite {
 
 BaseStatement::BaseStatement(Utils::SmallStringView sqlStatement, Database &database)
-    : m_compiledStatement(nullptr, deleteCompiledStatement),
+    : m_compiledStatement(nullptr, deleteCompiledStatement)
-      m_database(database),
+    , m_database(database)
-      m_bindingParameterCount(0),
+    , m_bindingParameterCount(0)
-      m_columnCount(0),
+    , m_columnCount(0)
-      m_isReadyToFetchValues(false)
 {
     prepare(sqlStatement);
     setBindingParameterCount();
@@ -107,11 +106,8 @@ void BaseStatement::reset() const
 {
     int resultCode = sqlite3_reset(m_compiledStatement.get());
 
-    if (resultCode != SQLITE_OK) {
+    if (resultCode != SQLITE_OK)
         checkForResetError(resultCode);
-
-        m_isReadyToFetchValues = false;
-    }
 }
 
 bool BaseStatement::next() const
@@ -127,8 +123,6 @@ bool BaseStatement::next() const
 
     } while (resultCode == SQLITE_LOCKED);
 
-    setIfIsReadyToFetchValues(resultCode);
-
     if (resultCode == SQLITE_ROW)
         return true;
     else if (resultCode == SQLITE_DONE)
@@ -299,33 +293,10 @@ void BaseStatement::checkForBindingError(int resultCode) const
     throwUnknowError("SqliteStatement::bind: unknown error has happened");
 }
 
-void BaseStatement::setIfIsReadyToFetchValues(int resultCode) const
+void BaseStatement::checkColumnCount(int columnCount) const
 {
-    if (resultCode == SQLITE_ROW)
+    if (columnCount != m_columnCount)
-        m_isReadyToFetchValues = true;
+        throw ColumnCountDoesNotMatch("SqliteStatement::values: column count does not match!");
-    else
-        m_isReadyToFetchValues = false;
-
-}
-
-void BaseStatement::checkIfIsReadyToFetchValues() const
-{
-    if (!m_isReadyToFetchValues)
-        throwNoValuesToFetch("SqliteStatement::value: there are no values to fetch!");
-}
-
-void BaseStatement::checkColumnsAreValid(const std::vector<int> &columns) const
-{
-    for (int column : columns) {
-        if (column < 0 || column >= m_columnCount)
-            throwInvalidColumnFetched("SqliteStatement::values: column index out of bound!");
-    }
-}
-
-void BaseStatement::checkColumnIsValid(int column) const
-{
-    if (column < 0 || column >= m_columnCount)
-        throwInvalidColumnFetched("SqliteStatement::values: column index out of bound!");
 }
 
 void BaseStatement::checkBindingName(int index) const
@@ -387,11 +358,6 @@ void BaseStatement::throwNoValuesToFetch(const char *whatHasHappened) const
     throw NoValuesToFetch(whatHasHappened);
 }
 
-void BaseStatement::throwInvalidColumnFetched(const char *whatHasHappened) const
-{
-    throw InvalidColumnFetched(whatHasHappened);
-}
-
 void BaseStatement::throwBindingIndexIsOutOfRange(const char *whatHasHappened) const
 {
     throw BindingIndexIsOutOfRange(whatHasHappened, sqlite3_errmsg(sqliteDatabaseHandle()));
@@ -472,8 +438,6 @@ StringType convertToTextForColumn(sqlite3_stmt *sqlStatment, int column)
 
 int BaseStatement::fetchIntValue(int column) const
 {
-    checkIfIsReadyToFetchValues();
-    checkColumnIsValid(column);
     return sqlite3_column_int(m_compiledStatement.get(), column);
 }
 
@@ -496,8 +460,6 @@ long BaseStatement::fetchValue<long>(int column) const
 
 long long BaseStatement::fetchLongLongValue(int column) const
 {
-    checkIfIsReadyToFetchValues();
-    checkColumnIsValid(column);
     return sqlite3_column_int64(m_compiledStatement.get(), column);
 }
 
@@ -509,16 +471,11 @@ long long BaseStatement::fetchValue<long long>(int column) const
 
 double BaseStatement::fetchDoubleValue(int column) const
 {
-    checkIfIsReadyToFetchValues();
-    checkColumnIsValid(column);
     return sqlite3_column_double(m_compiledStatement.get(), column);
 }
 
 Utils::span<const byte> BaseStatement::fetchBlobValue(int column) const
 {
-    checkIfIsReadyToFetchValues();
-    checkColumnIsValid(column);
-
     return convertToBlobForColumn(m_compiledStatement.get(), column);
 }
 
@@ -531,8 +488,6 @@ double BaseStatement::fetchValue<double>(int column) const
 template<typename StringType>
 StringType BaseStatement::fetchValue(int column) const
 {
-    checkIfIsReadyToFetchValues();
-    checkColumnIsValid(column);
     return convertToTextForColumn<StringType>(m_compiledStatement.get(), column);
 }
 

src/libs/sqlite/sqlitebasestatement.h

@@ -101,9 +101,7 @@ public:
     [[noreturn]] void checkForPrepareError(int resultCode) const;
     [[noreturn]] void checkForBindingError(int resultCode) const;
     void setIfIsReadyToFetchValues(int resultCode) const;
-    void checkIfIsReadyToFetchValues() const;
+    void checkColumnCount(int columnCount) const;
-    void checkColumnsAreValid(const std::vector<int> &columns) const;
-    void checkColumnIsValid(int column) const;
     void checkBindingName(int index) const;
     void setBindingParameterCount();
     void setColumnCount();
@@ -132,7 +130,6 @@ private:
     Database &m_database;
     int m_bindingParameterCount;
     int m_columnCount;
-    mutable bool m_isReadyToFetchValues;
 };
 
 template <> SQLITE_EXPORT int BaseStatement::fetchValue<int>(int column) const;
@@ -180,6 +177,8 @@ public:
               int ResultTypeCount = 1>
     std::vector<ResultType> values(std::size_t reserveSize)
     {
+        BaseStatement::checkColumnCount(ResultTypeCount);
+
         Resetter resetter{*this};
         std::vector<ResultType> resultValues;
         resultValues.reserve(std::max(reserveSize, m_maximumResultCount));
@@ -199,6 +198,8 @@ public:
               typename... QueryTypes>
     std::vector<ResultType> values(std::size_t reserveSize, const QueryTypes&... queryValues)
     {
+        BaseStatement::checkColumnCount(ResultTypeCount);
+
         Resetter resetter{*this};
         std::vector<ResultType> resultValues;
         resultValues.reserve(std::max(reserveSize, m_maximumResultCount));
@@ -221,6 +222,8 @@ public:
     std::vector<ResultType> values(std::size_t reserveSize,
                                    const std::vector<QueryElementType> &queryValues)
     {
+        BaseStatement::checkColumnCount(ResultTypeCount);
+
         std::vector<ResultType> resultValues;
         resultValues.reserve(std::max(reserveSize, m_maximumResultCount));
 
@@ -245,6 +248,8 @@ public:
     std::vector<ResultType> values(std::size_t reserveSize,
                                    const std::vector<std::tuple<QueryElementTypes...>> &queryTuples)
     {
+        BaseStatement::checkColumnCount(ResultTypeCount);
+
         using Container = std::vector<ResultType>;
         Container resultValues;
         resultValues.reserve(std::max(reserveSize, m_maximumResultCount));
@@ -269,6 +274,8 @@ public:
               typename... QueryTypes>
     Utils::optional<ResultType> value(const QueryTypes&... queryValues)
     {
+        BaseStatement::checkColumnCount(ResultTypeCount);
+
         Resetter resetter{*this};
         Utils::optional<ResultType> resultValue;
 
@@ -287,6 +294,8 @@ public:
     {
         StatementImplementation statement(sqlStatement, database);
 
+        statement.checkColumnCount(1);
+
         statement.next();
 
         return statement.template fetchValue<Type>(0);

src/libs/sqlite/sqliteexception.h

@@ -119,13 +119,12 @@ public:
     }
 };
 
-class InvalidColumnFetched : public Exception
+class ColumnCountDoesNotMatch : public Exception
 {
 public:
-    InvalidColumnFetched(const char *whatErrorHasHappen)
+    ColumnCountDoesNotMatch(const char *whatErrorHasHappen)
         : Exception(whatErrorHasHappen)
-    {
+    {}
-    }
 };
 
 class BindingIndexIsOutOfRange : public Exception

tests/unit/unittest/mocksqlitestatement.h

@@ -54,6 +54,8 @@ public:
     MOCK_METHOD2(bind, void (int, long));
 
     MOCK_METHOD1(prepare, void(Utils::SmallStringView sqlStatement));
+
+    MOCK_METHOD1(checkColumnCount, void(int));
 };
 
 template<>

tests/unit/unittest/sqlitestatement-test.cpp

@@ -178,37 +178,6 @@ TEST_F(SqliteStatement, Value)
     ASSERT_THAT(statement.fetchValueView(2), Eq(2));
 }
 
-TEST_F(SqliteStatement, ThrowNoValuesToFetchForNotSteppedStatement)
-{
-    SqliteTestStatement statement("SELECT name, number FROM test", database);
-
-    ASSERT_THROW(statement.fetchValue<int>(0), Sqlite::NoValuesToFetch);
-}
-
-TEST_F(SqliteStatement, ThrowNoValuesToFetchForDoneStatement)
-{
-    SqliteTestStatement statement("SELECT name, number FROM test", database);
-    while (statement.next()) {}
-
-    ASSERT_THROW(statement.fetchValue<int>(0), Sqlite::NoValuesToFetch);
-}
-
-TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForNegativeColumn)
-{
-    SqliteTestStatement statement("SELECT name, number FROM test", database);
-    statement.next();
-
-    ASSERT_THROW(statement.fetchValue<int>(-1), Sqlite::InvalidColumnFetched);
-}
-
-TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForNotExistingColumn)
-{
-    SqliteTestStatement statement("SELECT name, number FROM test", database);
-    statement.next();
-
-    ASSERT_THROW(statement.fetchValue<int>(2), Sqlite::InvalidColumnFetched);
-}
-
 TEST_F(SqliteStatement, ToIntegerValue)
 {
     auto value = ReadStatement::toValue<int>("SELECT number FROM test WHERE name='foo'", database);
@@ -567,7 +536,7 @@ TEST_F(SqliteStatement, GetValuesForMultipleOutputValuesAndContainerQueryValues)
 TEST_F(SqliteStatement, GetValuesForSingleOutputValuesAndContainerQueryValues)
 {
     std::vector<double> queryValues = {40, 23.3};
-    ReadStatement statement("SELECT name, number FROM test WHERE number=?", database);
+    ReadStatement statement("SELECT name FROM test WHERE number=?", database);
 
     std::vector<Utils::SmallString> values = statement.values<Utils::SmallString>(3, queryValues);
 
@@ -591,7 +560,7 @@ TEST_F(SqliteStatement, GetValuesForSingleOutputValuesAndContainerQueryTupleValu
 {
     using Tuple = std::tuple<Utils::SmallString, Utils::SmallString>;
     std::vector<Tuple> queryValues = {{"poo", "40"}, {"bar", "blah"}};
-    ReadStatement statement("SELECT name, number FROM test WHERE name= ? AND number=?", database);
+    ReadStatement statement("SELECT name FROM test WHERE name= ? AND number=?", database);
 
     std::vector<Utils::SmallString> values = statement.values<Utils::SmallString>(3, queryValues);
 
@@ -855,4 +824,47 @@ TEST_F(SqliteStatement, ResetIfExecuteThrowsException)
 
     ASSERT_ANY_THROW(mockStatement.execute());
 }
+
+TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForValue)
+{
+    SqliteTestStatement statement("SELECT name, number FROM test", database);
+
+    ASSERT_THROW(statement.value<int>(), Sqlite::ColumnCountDoesNotMatch);
+}
+
+TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForValues)
+{
+    SqliteTestStatement statement("SELECT name, number FROM test", database);
+
+    ASSERT_THROW(statement.values<int>(1), Sqlite::ColumnCountDoesNotMatch);
+}
+
+TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForValuesWithArguments)
+{
+    SqliteTestStatement statement("SELECT name, number FROM test WHERE name=?", database);
+
+    ASSERT_THROW(statement.values<int>(1, 2), Sqlite::ColumnCountDoesNotMatch);
+}
+
+TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForValuesWithVectorArguments)
+{
+    SqliteTestStatement statement("SELECT name, number FROM test", database);
+
+    ASSERT_THROW(statement.values<int>(1, std::vector<int>{}), Sqlite::ColumnCountDoesNotMatch);
+}
+
+TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForValuesWithTupleArguments)
+{
+    SqliteTestStatement statement("SELECT name, number FROM test", database);
+
+    ASSERT_THROW(statement.values<int>(1, std::vector<std::tuple<int>>{}),
+                 Sqlite::ColumnCountDoesNotMatch);
+}
+
+TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForToValues)
+{
+    ASSERT_THROW(SqliteTestStatement::toValue<int>("SELECT name, number FROM test", database),
+                 Sqlite::ColumnCountDoesNotMatch);
+}
+
 } // namespace