From a9a205486d734fcdbba561f5fac248ca530b6d14 Mon Sep 17 00:00:00 2001 From: Marco Bubke Date: Wed, 27 May 2020 00:09:19 +0200 Subject: [PATCH] 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 --- src/libs/sqlite/sqlitebasestatement.cpp | 61 ++------------- src/libs/sqlite/sqlitebasestatement.h | 19 +++-- src/libs/sqlite/sqliteexception.h | 7 +- tests/unit/unittest/mocksqlitestatement.h | 4 +- tests/unit/unittest/sqlitestatement-test.cpp | 78 +++++++++++--------- 5 files changed, 73 insertions(+), 96 deletions(-) diff --git a/src/libs/sqlite/sqlitebasestatement.cpp b/src/libs/sqlite/sqlitebasestatement.cpp index 890c6639974..f8dadeb425b 100644 --- a/src/libs/sqlite/sqlitebasestatement.cpp +++ b/src/libs/sqlite/sqlitebasestatement.cpp @@ -41,11 +41,10 @@ namespace Sqlite { BaseStatement::BaseStatement(Utils::SmallStringView sqlStatement, Database &database) - : m_compiledStatement(nullptr, deleteCompiledStatement), - m_database(database), - m_bindingParameterCount(0), - m_columnCount(0), - m_isReadyToFetchValues(false) + : m_compiledStatement(nullptr, deleteCompiledStatement) + , m_database(database) + , m_bindingParameterCount(0) + , m_columnCount(0) { 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) - m_isReadyToFetchValues = true; - 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 &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!"); + if (columnCount != m_columnCount) + throw ColumnCountDoesNotMatch("SqliteStatement::values: column count does not match!"); } 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(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(int column) const double BaseStatement::fetchDoubleValue(int column) const { - checkIfIsReadyToFetchValues(); - checkColumnIsValid(column); return sqlite3_column_double(m_compiledStatement.get(), column); } Utils::span BaseStatement::fetchBlobValue(int column) const { - checkIfIsReadyToFetchValues(); - checkColumnIsValid(column); - return convertToBlobForColumn(m_compiledStatement.get(), column); } @@ -531,8 +488,6 @@ double BaseStatement::fetchValue(int column) const template StringType BaseStatement::fetchValue(int column) const { - checkIfIsReadyToFetchValues(); - checkColumnIsValid(column); return convertToTextForColumn(m_compiledStatement.get(), column); } diff --git a/src/libs/sqlite/sqlitebasestatement.h b/src/libs/sqlite/sqlitebasestatement.h index 48076c37da0..2480a4674a6 100644 --- a/src/libs/sqlite/sqlitebasestatement.h +++ b/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 checkColumnsAreValid(const std::vector &columns) const; - void checkColumnIsValid(int column) const; + void checkColumnCount(int columnCount) const; void checkBindingName(int index) const; void setBindingParameterCount(); void setColumnCount(); @@ -128,11 +126,10 @@ protected: ~BaseStatement() = default; private: - std::unique_ptr m_compiledStatement; + std::unique_ptr m_compiledStatement; Database &m_database; int m_bindingParameterCount; int m_columnCount; - mutable bool m_isReadyToFetchValues; }; template <> SQLITE_EXPORT int BaseStatement::fetchValue(int column) const; @@ -180,6 +177,8 @@ public: int ResultTypeCount = 1> std::vector values(std::size_t reserveSize) { + BaseStatement::checkColumnCount(ResultTypeCount); + Resetter resetter{*this}; std::vector resultValues; resultValues.reserve(std::max(reserveSize, m_maximumResultCount)); @@ -199,6 +198,8 @@ public: typename... QueryTypes> std::vector values(std::size_t reserveSize, const QueryTypes&... queryValues) { + BaseStatement::checkColumnCount(ResultTypeCount); + Resetter resetter{*this}; std::vector resultValues; resultValues.reserve(std::max(reserveSize, m_maximumResultCount)); @@ -221,6 +222,8 @@ public: std::vector values(std::size_t reserveSize, const std::vector &queryValues) { + BaseStatement::checkColumnCount(ResultTypeCount); + std::vector resultValues; resultValues.reserve(std::max(reserveSize, m_maximumResultCount)); @@ -245,6 +248,8 @@ public: std::vector values(std::size_t reserveSize, const std::vector> &queryTuples) { + BaseStatement::checkColumnCount(ResultTypeCount); + using Container = std::vector; Container resultValues; resultValues.reserve(std::max(reserveSize, m_maximumResultCount)); @@ -269,6 +274,8 @@ public: typename... QueryTypes> Utils::optional value(const QueryTypes&... queryValues) { + BaseStatement::checkColumnCount(ResultTypeCount); + Resetter resetter{*this}; Utils::optional resultValue; @@ -287,6 +294,8 @@ public: { StatementImplementation statement(sqlStatement, database); + statement.checkColumnCount(1); + statement.next(); return statement.template fetchValue(0); diff --git a/src/libs/sqlite/sqliteexception.h b/src/libs/sqlite/sqliteexception.h index dcbbfcd1bef..7f8f60ca423 100644 --- a/src/libs/sqlite/sqliteexception.h +++ b/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 diff --git a/tests/unit/unittest/mocksqlitestatement.h b/tests/unit/unittest/mocksqlitestatement.h index 351c6708a57..7be7f3f05e0 100644 --- a/tests/unit/unittest/mocksqlitestatement.h +++ b/tests/unit/unittest/mocksqlitestatement.h @@ -53,7 +53,9 @@ public: MOCK_METHOD2(bind, void (int, Utils::SmallStringView)); MOCK_METHOD2(bind, void (int, long)); - MOCK_METHOD1(prepare, void (Utils::SmallStringView sqlStatement)); + MOCK_METHOD1(prepare, void(Utils::SmallStringView sqlStatement)); + + MOCK_METHOD1(checkColumnCount, void(int)); }; template<> diff --git a/tests/unit/unittest/sqlitestatement-test.cpp b/tests/unit/unittest/sqlitestatement-test.cpp index 2b8942dbf11..4b59d52af29 100644 --- a/tests/unit/unittest/sqlitestatement-test.cpp +++ b/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(0), Sqlite::NoValuesToFetch); -} - -TEST_F(SqliteStatement, ThrowNoValuesToFetchForDoneStatement) -{ - SqliteTestStatement statement("SELECT name, number FROM test", database); - while (statement.next()) {} - - ASSERT_THROW(statement.fetchValue(0), Sqlite::NoValuesToFetch); -} - -TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForNegativeColumn) -{ - SqliteTestStatement statement("SELECT name, number FROM test", database); - statement.next(); - - ASSERT_THROW(statement.fetchValue(-1), Sqlite::InvalidColumnFetched); -} - -TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForNotExistingColumn) -{ - SqliteTestStatement statement("SELECT name, number FROM test", database); - statement.next(); - - ASSERT_THROW(statement.fetchValue(2), Sqlite::InvalidColumnFetched); -} - TEST_F(SqliteStatement, ToIntegerValue) { auto value = ReadStatement::toValue("SELECT number FROM test WHERE name='foo'", database); @@ -567,7 +536,7 @@ TEST_F(SqliteStatement, GetValuesForMultipleOutputValuesAndContainerQueryValues) TEST_F(SqliteStatement, GetValuesForSingleOutputValuesAndContainerQueryValues) { std::vector 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 values = statement.values(3, queryValues); @@ -591,7 +560,7 @@ TEST_F(SqliteStatement, GetValuesForSingleOutputValuesAndContainerQueryTupleValu { using Tuple = std::tuple; std::vector 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 values = statement.values(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(), Sqlite::ColumnCountDoesNotMatch); +} + +TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForValues) +{ + SqliteTestStatement statement("SELECT name, number FROM test", database); + + ASSERT_THROW(statement.values(1), Sqlite::ColumnCountDoesNotMatch); +} + +TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForValuesWithArguments) +{ + SqliteTestStatement statement("SELECT name, number FROM test WHERE name=?", database); + + ASSERT_THROW(statement.values(1, 2), Sqlite::ColumnCountDoesNotMatch); +} + +TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForValuesWithVectorArguments) +{ + SqliteTestStatement statement("SELECT name, number FROM test", database); + + ASSERT_THROW(statement.values(1, std::vector{}), Sqlite::ColumnCountDoesNotMatch); +} + +TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForValuesWithTupleArguments) +{ + SqliteTestStatement statement("SELECT name, number FROM test", database); + + ASSERT_THROW(statement.values(1, std::vector>{}), + Sqlite::ColumnCountDoesNotMatch); +} + +TEST_F(SqliteStatement, ThrowInvalidColumnFetchedForToManyArgumentsForToValues) +{ + ASSERT_THROW(SqliteTestStatement::toValue("SELECT name, number FROM test", database), + Sqlite::ColumnCountDoesNotMatch); +} + } // namespace