From cf84366fb165cf309d12a95599566066e2af1599 Mon Sep 17 00:00:00 2001 From: Andreas Holzner Date: Tue, 18 Sep 2018 20:08:29 +0200 Subject: [PATCH] Improve sqlite iterator interface (#33) * GH-84 Cleanup: conform to ROS2 naming conventions * GH-84 Allow only a single non-end iterator for query results Reason: multiple iterators for the same query result would interfere with each other as a single result row can only be retrieved once. These side effects would be very surprising and are therefore forbidden. * GH-84 Do not use noexcept for gcc < 6 --- .../sqlite/sqlite_statement_wrapper.hpp | 24 +++++++++++++++---- .../sqlite/sqlite_statement_wrapper.cpp | 4 ++-- .../sqlite_wrapper_integration_test.cpp | 14 ++++++++++- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/rosbag2_storage_default_plugins/include/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.hpp b/rosbag2_storage_default_plugins/include/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.hpp index acd5d007a3..d5cbac035d 100644 --- a/rosbag2_storage_default_plugins/include/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.hpp +++ b/rosbag2_storage_default_plugins/include/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.hpp @@ -69,6 +69,12 @@ class ROSBAG2_STORAGE_DEFAULT_PLUGINS_PUBLIC SqliteStatementWrapper } } + Iterator(const Iterator &) = delete; + Iterator & operator=(const Iterator &) = delete; + + Iterator(Iterator &&) = default; + Iterator & operator=(Iterator &&) = default; + Iterator & operator++() { if (next_row_idx_ != POSITION_END) { @@ -102,11 +108,11 @@ class ROSBAG2_STORAGE_DEFAULT_PLUGINS_PUBLIC SqliteStatementWrapper return row; } - bool operator==(Iterator other) const + bool operator==(const Iterator & other) const { return statement_ == other.statement_ && next_row_idx_ == other.next_row_idx_; } - bool operator!=(Iterator other) const + bool operator!=(const Iterator & other) const { return !(*this == other); } @@ -140,11 +146,12 @@ class ROSBAG2_STORAGE_DEFAULT_PLUGINS_PUBLIC SqliteStatementWrapper }; explicit QueryResult(std::shared_ptr statement) - : statement_(statement) + : statement_(statement), is_already_accessed_(false) {} Iterator begin() { + try_access_data(); return Iterator(statement_, 0); } Iterator end() @@ -158,7 +165,16 @@ class ROSBAG2_STORAGE_DEFAULT_PLUGINS_PUBLIC SqliteStatementWrapper } private: + void try_access_data() + { + if (is_already_accessed_) { + throw SqliteException("Only one iterator per query result is supported!"); + } + is_already_accessed_ = true; + } + std::shared_ptr statement_; + bool is_already_accessed_; }; std::shared_ptr execute_and_reset(); @@ -177,7 +193,7 @@ class ROSBAG2_STORAGE_DEFAULT_PLUGINS_PUBLIC SqliteStatementWrapper private: bool step(); - bool isQueryOk(int return_code); + bool is_query_ok(int return_code); void obtain_column_value(size_t index, int & value) const; void obtain_column_value(size_t index, rcutils_time_point_value_t & value) const; diff --git a/rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.cpp b/rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.cpp index 9d1ef8a115..fecb13c912 100644 --- a/rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.cpp +++ b/rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.cpp @@ -51,14 +51,14 @@ SqliteStatementWrapper::~SqliteStatementWrapper() std::shared_ptr SqliteStatementWrapper::execute_and_reset() { int return_code = sqlite3_step(statement_); - if (!isQueryOk(return_code)) { + if (!is_query_ok(return_code)) { throw SqliteException("Error processing SQLite statement. Return code: " + std::to_string(return_code)); } return reset(); } -bool SqliteStatementWrapper::isQueryOk(int return_code) +bool SqliteStatementWrapper::is_query_ok(int return_code) { return return_code == SQLITE_OK || return_code == SQLITE_DONE || return_code == SQLITE_ROW; } diff --git a/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper_integration_test.cpp b/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper_integration_test.cpp index ac3212b25c..f1af2fd770 100644 --- a/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper_integration_test.cpp +++ b/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper_integration_test.cpp @@ -40,7 +40,8 @@ class SqliteWrapperTestFixture : public StorageTestFixture TEST_F(SqliteWrapperTestFixture, querying_empty_adhoc_results_gives_empty_query_result) { auto result = db_.prepare_statement("SELECT 1 WHERE 1=2;")->execute_query(); - ASSERT_THAT(result.begin(), Eq(result.end())); + // ASSERT_THAT cannot be used as the iterators can only be moved and not copied. + ASSERT_TRUE(result.begin() == result.end()); } TEST_F(SqliteWrapperTestFixture, querying_adhoc_results_with_normal_data_gives_content) { @@ -93,6 +94,7 @@ TEST_F(SqliteWrapperTestFixture, all_result_rows_are_available) { ASSERT_THAT(row_value, Eq(4)); query->reset(); + result = query->execute_query(); // iterator access row_value = 1; @@ -104,6 +106,16 @@ TEST_F(SqliteWrapperTestFixture, all_result_rows_are_available) { ASSERT_THAT(row_value, Eq(4)); } +TEST_F(SqliteWrapperTestFixture, only_a_single_iterator_is_allowed_per_result) { + auto result = db_.prepare_statement("SELECT 1;")->execute_query(); + + auto row = result.begin(); + + EXPECT_THROW(result.get_single_line(), rosbag2_storage_plugins::SqliteException); + EXPECT_THROW(result.begin(), rosbag2_storage_plugins::SqliteException); + EXPECT_NO_THROW(result.end()); +} + TEST_F(SqliteWrapperTestFixture, ros_specific_types_are_supported_for_reading_and_writing) { db_.prepare_statement("CREATE TABLE test (timestamp INTEGER, data BLOB);")->execute_and_reset(); rcutils_time_point_value_t time = 1099511627783;