From be4677149e83225cf89586980d3158073b8fc95f Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Tue, 16 Jan 2024 20:31:59 +0100 Subject: [PATCH] Address review comments --- include/soci/blob.h | 7 ++++++- include/soci/firebird/soci-firebird.h | 12 ++++++------ include/soci/oracle/soci-oracle.h | 2 ++ include/soci/soci-backend.h | 4 ++-- include/soci/type-traits.h | 9 ++++++--- src/backends/firebird/blob.cpp | 16 ++++++---------- src/backends/firebird/standard-use-type.cpp | 4 ++-- src/backends/firebird/vector-use-type.cpp | 2 +- src/backends/mysql/standard-use-type.cpp | 3 +++ src/backends/oracle/blob.cpp | 8 +++----- src/backends/postgresql/blob.cpp | 18 +++++++++--------- src/backends/sqlite3/statement.cpp | 9 +++++---- tests/common-tests.h | 21 +-------------------- tests/db2/test-db2.cpp | 2 -- tests/firebird/test-firebird.cpp | 2 -- tests/mysql/test-mysql.h | 9 ++------- tests/odbc/test-odbc-access.cpp | 2 -- tests/odbc/test-odbc-db2.cpp | 2 -- tests/odbc/test-odbc-mssql.cpp | 2 -- tests/odbc/test-odbc-mysql.cpp | 7 +++++-- tests/odbc/test-odbc-postgresql.cpp | 2 -- tests/oracle/test-oracle.cpp | 2 -- tests/postgresql/test-postgresql.cpp | 3 --- tests/sqlite3/test-sqlite3.cpp | 2 -- 24 files changed, 59 insertions(+), 91 deletions(-) diff --git a/include/soci/blob.h b/include/soci/blob.h index ec7caa455..db77c3652 100644 --- a/include/soci/blob.h +++ b/include/soci/blob.h @@ -44,7 +44,12 @@ class SOCI_DECL blob [[deprecated("Use read_from_start instead")]] std::size_t read(std::size_t offset, char * buf, std::size_t toRead); - // offset starts from 0 + // Extracts data from this blob into the given buffer. + // At most toRead bytes are extracted (and copied into buf). + // The amount of actually read bytes is returned. + // + // Note: Using an offset > 0 on a blob whose size is less than + // or equal to offset, will throw an exception. std::size_t read_from_start(char * buf, std::size_t toRead, std::size_t offset = 0); diff --git a/include/soci/firebird/soci-firebird.h b/include/soci/firebird/soci-firebird.h index baedd4fed..647ed2bf5 100644 --- a/include/soci/firebird/soci-firebird.h +++ b/include/soci/firebird/soci-firebird.h @@ -265,11 +265,11 @@ struct firebird_blob_backend : details::blob_backend std::size_t append(char const *buf, std::size_t toWrite) override; void trim(std::size_t newLen) override; - /// Writes the current data into the database by allocating a new BLOB - /// object for it. - /// - /// @returns The ID of the newly created BLOB object - ISC_QUAD write_to_db(); + // Writes the current data into the database by allocating a new BLOB + // object for it. + // + // Returns The ID of the newly created BLOB object + ISC_QUAD save_to_db(); void assign(ISC_QUAD const & bid); private: @@ -278,7 +278,7 @@ struct firebird_blob_backend : details::blob_backend void load(); void writeBuffer(std::size_t offset, char const * buf, std::size_t toWrite); - void closeBlob(bool keepData); + void closeBlob(); firebird_session_backend &session_; ISC_QUAD blob_id_; diff --git a/include/soci/oracle/soci-oracle.h b/include/soci/oracle/soci-oracle.h index 30b4db06c..21dbbf220 100644 --- a/include/soci/oracle/soci-oracle.h +++ b/include/soci/oracle/soci-oracle.h @@ -304,6 +304,8 @@ struct oracle_blob_backend : details::blob_backend locator_t lobp_; + // If this is true, then the locator lobp_ points to something useful + // (instead of being the equivalent to a pointer with random value) bool initialized_; }; diff --git a/include/soci/soci-backend.h b/include/soci/soci-backend.h index 751810f3d..563811e50 100644 --- a/include/soci/soci-backend.h +++ b/include/soci/soci-backend.h @@ -341,10 +341,10 @@ class trivial_blob_backend : public blob_backend throw soci_error("Can't read past-the-end of BLOB data."); } - toRead = (std::min)(toRead, buffer_.size() - offset); - // make sure that we don't try to read // past the end of the data + toRead = (std::min)(toRead, buffer_.size() - offset); + memcpy(buf, buffer_.data() + offset, toRead); return toRead; diff --git a/include/soci/type-traits.h b/include/soci/type-traits.h index c46e37722..344616acb 100644 --- a/include/soci/type-traits.h +++ b/include/soci/type-traits.h @@ -5,8 +5,8 @@ // http://www.boost.org/LICENSE_1_0.txt) // -#ifndef SOCI_PRIVATE_SOCI_TYPE_TRAITS_H_INCLUDED -#define SOCI_PRIVATE_SOCI_TYPE_TRAITS_H_INCLUDED +#ifndef SOCI_SOCI_TYPE_TRAITS_H_INCLUDED +#define SOCI_SOCI_TYPE_TRAITS_H_INCLUDED #include @@ -23,6 +23,9 @@ using false_type = std::integral_constant; using true_type = std::integral_constant; // Implementation from https://blog.tartanllama.xyz/detection-idiom/ +// Note, this is a stub that we require until standard C++ gets support +// for the detection idiom that is not experimental (and thus can be +// assumed to be present). namespace detector_detail { @@ -43,4 +46,4 @@ using is_detected = typename detector_detail::is_detected: } -#endif // SOCI_PRIVATE_SOCI_TYPE_TRAITS_H_INCLUDED +#endif // SOCI_SOCI_TYPE_TRAITS_H_INCLUDED diff --git a/src/backends/firebird/blob.cpp b/src/backends/firebird/blob.cpp index f1a29e54c..a7f587195 100644 --- a/src/backends/firebird/blob.cpp +++ b/src/backends/firebird/blob.cpp @@ -21,7 +21,7 @@ firebird_blob_backend::firebird_blob_backend(firebird_session_backend &session) firebird_blob_backend::~firebird_blob_backend() { - closeBlob(false); + closeBlob(); } std::size_t firebird_blob_backend::get_len() @@ -152,17 +152,12 @@ void firebird_blob_backend::open() data_.resize(blob_size); } -void firebird_blob_backend::closeBlob(bool keepData) +void firebird_blob_backend::closeBlob() { from_db_ = false; loaded_ = false; max_seg_size_ = 0; - if (!keepData) - { - data_.resize(0); - } - if (blob_handle_ != 0) { // close blob @@ -231,7 +226,7 @@ void firebird_blob_backend::load() loaded_ = true; } -ISC_QUAD firebird_blob_backend::write_to_db() +ISC_QUAD firebird_blob_backend::save_to_db() { // close old blob if necessary ISC_STATUS stat[20]; @@ -280,14 +275,15 @@ ISC_QUAD firebird_blob_backend::write_to_db() // In any case, BLOBs in Firebird can't be updated anyway - one always has to create a new BLOB object // (with a new ID) and then use that to modify the existing one (replace the ID in the corresponding table). // Therefore, keeping the Blob open for subsequent modification is not needed. - closeBlob(true); + closeBlob(); return blob_id_; } void firebird_blob_backend::assign(const ISC_QUAD &id) { - closeBlob(false); + closeBlob(); + data_.clear(); blob_id_ = id; from_db_ = true; diff --git a/src/backends/firebird/standard-use-type.cpp b/src/backends/firebird/standard-use-type.cpp index 92c4d3e65..c2bc6803c 100644 --- a/src/backends/firebird/standard-use-type.cpp +++ b/src/backends/firebird/standard-use-type.cpp @@ -158,7 +158,7 @@ void firebird_standard_use_type_backend::exchangeData() throw soci_error("Can't get Firebid BLOB BackEnd"); } - ISC_QUAD blob_id = blob->write_to_db(); + ISC_QUAD blob_id = blob->save_to_db(); memcpy(buf_, &blob_id, sizeof(blob_id)); } break; @@ -185,7 +185,7 @@ void firebird_standard_use_type_backend::copy_to_blob(const std::string& in) blob_->append(in.c_str(), in.length()); - ISC_QUAD blob_id = blob_->write_to_db(); + ISC_QUAD blob_id = blob_->save_to_db(); memcpy(buf_, &blob_id, sizeof(blob_id)); } diff --git a/src/backends/firebird/vector-use-type.cpp b/src/backends/firebird/vector-use-type.cpp index a4cc21c53..47d561a7a 100644 --- a/src/backends/firebird/vector-use-type.cpp +++ b/src/backends/firebird/vector-use-type.cpp @@ -195,7 +195,7 @@ void firebird_vector_use_type_backend::copy_to_blob(const std::string &in) blob_ = new firebird_blob_backend(statement_.session_); blob_->append(in.c_str(), in.length()); - ISC_QUAD blob_id = blob_->write_to_db(); + ISC_QUAD blob_id = blob_->save_to_db(); memcpy(buf_, &blob_id, sizeof(blob_id)); } diff --git a/src/backends/mysql/standard-use-type.cpp b/src/backends/mysql/standard-use-type.cpp index 036d1c25e..c0b4c17eb 100644 --- a/src/backends/mysql/standard-use-type.cpp +++ b/src/backends/mysql/standard-use-type.cpp @@ -178,6 +178,9 @@ void mysql_standard_use_type_backend::pre_use(indicator const *ind) } else { + // Note: since the entire MySQL works by assembling the query in text form, + // we can't make use of proper blob objects (that'd require a more sophisticated + // way of using the MySQL API). buf_ = new char[hex_size + 1]; bbe->write_hex_str(buf_, hex_size); // Add NULL terminator diff --git a/src/backends/oracle/blob.cpp b/src/backends/oracle/blob.cpp index 375e6bacd..a06f3c9f4 100644 --- a/src/backends/oracle/blob.cpp +++ b/src/backends/oracle/blob.cpp @@ -14,8 +14,6 @@ #include #include -#include - #ifdef _MSC_VER #pragma warning(disable:4355) #endif @@ -97,11 +95,11 @@ std::size_t oracle_blob_backend::read_from_start(char *buf, std::size_t toRead, std::size_t oracle_blob_backend::write_from_start(char const *buf, std::size_t toWrite, std::size_t offset) { - if (offset > get_len()) - { + if (offset > get_len()) + { // If offset == length, the operation is to be understood as appending (and is therefore allowed) throw soci_error("Can't start writing far past-the-end of BLOB data."); - } + } ensure_initialized(); diff --git a/src/backends/postgresql/blob.cpp b/src/backends/postgresql/blob.cpp index 3b6ac66a8..140809762 100644 --- a/src/backends/postgresql/blob.cpp +++ b/src/backends/postgresql/blob.cpp @@ -253,7 +253,7 @@ void postgresql_blob_backend::clone() } blob_details old_details = details_; - details_ = {}; + details_ = {}; reset(); init(); @@ -282,15 +282,15 @@ void postgresql_blob_backend::clone() offset += sizeof(buf); } while (read_bytes == sizeof(buf)); - // Dispose old BLOB object - if (destroy_on_close_) + // Dispose old BLOB object + if (destroy_on_close_) { - // Remove the large object from the DB completely - lo_unlink(session_.conn_, old_details.fd); - } + // Remove the large object from the DB completely + lo_unlink(session_.conn_, old_details.fd); + } else { - // Merely close our handle to the large object - lo_close(session_.conn_, old_details.fd); - } + // Merely close our handle to the large object + lo_close(session_.conn_, old_details.fd); + } } diff --git a/src/backends/sqlite3/statement.cpp b/src/backends/sqlite3/statement.cpp index 35c426d69..9843b2da2 100644 --- a/src/backends/sqlite3/statement.cpp +++ b/src/backends/sqlite3/statement.cpp @@ -327,10 +327,11 @@ sqlite3_statement_backend::bind_and_execute(int number) break; case db_blob: - // Since we don't own the buffer_ pointer we are passing here, we can't make any lifetime guarantees other than - // it is currently valid. Thus, we ask SQLite to make a copy of the underlying buffer to ensure - // the database can always access a valid buffer. - bindRes = sqlite3_bind_blob(stmt_, pos, col.buffer_.constData_, static_cast(col.buffer_.size_), SQLITE_TRANSIENT); + // Since we don't own the buffer_ pointer we are passing here, we can't make any lifetime + // guarantees other than it is currently valid. Thus, we ask SQLite to make a copy of the + // underlying buffer to ensure the database can always access a valid buffer. + bindRes = sqlite3_bind_blob(stmt_, pos, col.buffer_.constData_, + static_cast(col.buffer_.size_), SQLITE_TRANSIENT); break; case db_xml: diff --git a/tests/common-tests.h b/tests/common-tests.h index 3ef70687c..bd295a1b2 100644 --- a/tests/common-tests.h +++ b/tests/common-tests.h @@ -363,18 +363,6 @@ class function_creator_base SOCI_NOT_COPYABLE(function_creator_base) }; -enum class Backend -{ - Empty, - SQLite, - MySQL, - PostgreSQL, - ODBC, - Oracle, - Firebird, - DB2, -}; - // This is a singleton class, at any given time there is at most one test // context alive and common_tests fixture class uses it. class test_context_base @@ -416,8 +404,6 @@ class test_context_base return connectString_; } - virtual Backend get_backend() const = 0; - virtual std::string to_date_time(std::string const &dateTime) const = 0; virtual table_creator_base* table_creator_1(session&) const = 0; @@ -6404,7 +6390,7 @@ TEST_CASE_METHOD(common_tests, "BLOB", "[core][blob]") return; } - const char dummy_data[] = "abcdefghijklmnopüqrstuvwxyz"; + const char dummy_data[] = "abcdefghijklmnopqrstuvwxyz"; // Cross-DB usage of BLOBs is only possible if the entire lifetime of the blob object // is covered in an active transaction. @@ -6636,10 +6622,6 @@ TEST_CASE_METHOD(common_tests, "BLOB", "[core][blob]") containedData = true; const soci::row ¤tRow = *it; - soci::data_type type = currentRow.get_properties(0).get_data_type(); - soci::data_type expectedType = tc_.get_backend() != Backend::Oracle ? soci::dt_integer : soci::dt_long_long; - CHECK(type == expectedType); - CHECK(currentRow.get_properties(1).get_data_type() == soci::dt_blob); } CHECK(containedData); @@ -6750,7 +6732,6 @@ TEST_CASE_METHOD(common_tests, "BLOB", "[core][blob]") CHECK(!select_stmt.fetch()); } } - transaction.rollback(); } TEST_CASE_METHOD(common_tests, "Logger", "[core][log]") diff --git a/tests/db2/test-db2.cpp b/tests/db2/test-db2.cpp index 4feefd3ab..5501f1da1 100644 --- a/tests/db2/test-db2.cpp +++ b/tests/db2/test-db2.cpp @@ -68,8 +68,6 @@ class test_context :public test_context_base test_context(backend_factory const & pi_back_end, std::string const & pi_connect_string) : test_context_base(pi_back_end, pi_connect_string) {} - Backend get_backend() const override { return Backend::DB2; } - table_creator_base* table_creator_1(soci::session & pr_s) const override { pr_s << "SET CURRENT SCHEMA = 'DB2INST1'"; diff --git a/tests/firebird/test-firebird.cpp b/tests/firebird/test-firebird.cpp index 4a8ac8552..cfe688957 100644 --- a/tests/firebird/test-firebird.cpp +++ b/tests/firebird/test-firebird.cpp @@ -1191,8 +1191,6 @@ class test_context : public tests::test_context_base : test_context_base(backEnd, connectString) {} - tests::Backend get_backend() const override { return tests::Backend::Firebird; } - tests::table_creator_base* table_creator_1(soci::session& s) const override { return new TableCreator1(s); diff --git a/tests/mysql/test-mysql.h b/tests/mysql/test-mysql.h index b47d488f9..dde8c8718 100644 --- a/tests/mysql/test-mysql.h +++ b/tests/mysql/test-mysql.h @@ -70,8 +70,6 @@ class test_context : public test_context_base std::string const &connectString) : test_context_base(backEnd, connectString) {} - Backend get_backend() const override { return Backend::MySQL; } - table_creator_base* table_creator_1(soci::session& s) const override { return new table_creator_one(s); @@ -97,13 +95,10 @@ class test_context : public test_context_base return "\'" + datdt_string + "\'"; } -#ifndef SOCI_INCLUDED_FROM_ODBC_TEST - // ODBC backend doesn't support BLOBs yet - table_creator_base* table_creator_blob(soci::session& s) const override + table_creator_base * table_creator_blob(soci::session &s) const override { - return new table_creator_for_blob(s); + return new table_creator_for_blob(s); } -#endif bool has_fp_bug() const override { diff --git a/tests/odbc/test-odbc-access.cpp b/tests/odbc/test-odbc-access.cpp index af266900c..2783646f4 100644 --- a/tests/odbc/test-odbc-access.cpp +++ b/tests/odbc/test-odbc-access.cpp @@ -73,8 +73,6 @@ class test_context : public test_context_base test_context(backend_factory const &backend, std::string const &connstr) : test_context_base(backend, connstr) {} - Backend get_backend() const override { return Backend::ODBC; } - table_creator_base * table_creator_1(soci::session& s) const { return new table_creator_one(s); diff --git a/tests/odbc/test-odbc-db2.cpp b/tests/odbc/test-odbc-db2.cpp index 85b5b6cf0..91788949e 100644 --- a/tests/odbc/test-odbc-db2.cpp +++ b/tests/odbc/test-odbc-db2.cpp @@ -69,8 +69,6 @@ class test_context : public test_context_base std::string const &connectString) : test_context_base(backEnd, connectString) {} - Backend get_backend() const override { return Backend::ODBC; } - table_creator_base * table_creator_1(soci::session& s) const { return new table_creator_one(s); diff --git a/tests/odbc/test-odbc-mssql.cpp b/tests/odbc/test-odbc-mssql.cpp index fd20a6067..8b3ecd740 100644 --- a/tests/odbc/test-odbc-mssql.cpp +++ b/tests/odbc/test-odbc-mssql.cpp @@ -156,8 +156,6 @@ class test_context : public test_context_base std::string const &connstr) : test_context_base(backend, connstr) {} - Backend get_backend() const override { return Backend::ODBC; } - table_creator_base* table_creator_1(soci::session& s) const override { return new table_creator_one(s); diff --git a/tests/odbc/test-odbc-mysql.cpp b/tests/odbc/test-odbc-mysql.cpp index f66001898..f66e0ab86 100644 --- a/tests/odbc/test-odbc-mysql.cpp +++ b/tests/odbc/test-odbc-mysql.cpp @@ -12,9 +12,7 @@ #include #include -#define SOCI_INCLUDED_FROM_ODBC_TEST #include "mysql/test-mysql.h" -#undef SOCI_INCLUDED_FROM_ODBC_TEST std::string connectString; backend_factory const &backEnd = *soci::factory_odbc(); @@ -34,6 +32,11 @@ class test_context_odbc : public test_context // but we use an older version in the AppVeyor builds. return true; } + + table_creator_base* table_creator_blob(soci::session& s) const override + { + return new table_creator_for_blob(s); + } }; int main(int argc, char** argv) diff --git a/tests/odbc/test-odbc-postgresql.cpp b/tests/odbc/test-odbc-postgresql.cpp index fb2f66f34..4dbfa3c46 100644 --- a/tests/odbc/test-odbc-postgresql.cpp +++ b/tests/odbc/test-odbc-postgresql.cpp @@ -161,8 +161,6 @@ class test_context : public test_context_base std::cout << "Using ODBC driver version " << m_verDriver << "\n"; } - Backend get_backend() const override { return Backend::ODBC; } - table_creator_base * table_creator_1(soci::session& s) const override { return new table_creator_one(s); diff --git a/tests/oracle/test-oracle.cpp b/tests/oracle/test-oracle.cpp index 8fcffbe82..d3531fd13 100644 --- a/tests/oracle/test-oracle.cpp +++ b/tests/oracle/test-oracle.cpp @@ -1407,8 +1407,6 @@ class test_context :public test_context_base std::string const &connectString) : test_context_base(backEnd, connectString) {} - Backend get_backend() const override { return Backend::Oracle; } - table_creator_base* table_creator_1(soci::session& s) const override { return new table_creator_one(s); diff --git a/tests/postgresql/test-postgresql.cpp b/tests/postgresql/test-postgresql.cpp index f5cb94633..651685866 100644 --- a/tests/postgresql/test-postgresql.cpp +++ b/tests/postgresql/test-postgresql.cpp @@ -15,7 +15,6 @@ #include #include #include -#include using namespace soci; using namespace soci::tests; @@ -1346,8 +1345,6 @@ class test_context : public test_context_base : test_context_base(backend, connstr) {} - Backend get_backend() const override { return Backend::PostgreSQL; } - table_creator_base* table_creator_1(soci::session& s) const override { return new table_creator_one(s); diff --git a/tests/sqlite3/test-sqlite3.cpp b/tests/sqlite3/test-sqlite3.cpp index b015267ca..0aa91f76d 100644 --- a/tests/sqlite3/test-sqlite3.cpp +++ b/tests/sqlite3/test-sqlite3.cpp @@ -794,8 +794,6 @@ class test_context : public test_context_base std::string const &connstr) : test_context_base(backend, connstr) {} - Backend get_backend() const override { return Backend::SQLite; } - table_creator_base* table_creator_1(soci::session& s) const override { return new table_creator_one(s);