From 04d7d8e5be8c3c6177c549251efc3532203beaf5 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 7 Nov 2024 01:04:31 +0100 Subject: [PATCH 01/12] Move parsing space-separated strings to connection_parameters Move code present in Firebird backend, which is just one of several different versions of the code doing the same thing, into a common core class to allow reusing it in the other backends later. Also add tests for this code now that it is actually testable. --- include/soci/connection-parameters.h | 12 ++ src/backends/firebird/session.cpp | 194 ++------------------------- src/core/connection-parameters.cpp | 155 +++++++++++++++++++++ tests/CMakeLists.txt | 1 + tests/common/test-common.cpp | 2 + tests/common/test-connparams.cpp | 112 ++++++++++++++++ 6 files changed, 290 insertions(+), 186 deletions(-) create mode 100644 tests/common/test-connparams.cpp diff --git a/include/soci/connection-parameters.h b/include/soci/connection-parameters.h index 44f356260..7977ec68b 100644 --- a/include/soci/connection-parameters.h +++ b/include/soci/connection-parameters.h @@ -51,6 +51,18 @@ class SOCI_DECL connection_parameters void set_connect_string(const std::string & connectString) { connectString_ = connectString; } std::string const & get_connect_string() const { return connectString_; } + + // For some (but not all) backends the connection string consists of + // space-separated name=value pairs. This function parses the string + // assuming it uses this syntax and sets the options accordingly. + // + // If it detects invalid syntax, e.g. a name without a corresponding value, + // it throws an exception. + // + // Note that currently unknown options are simply ignored. + void extract_options_from_space_separated_string(); + + // Set the value of the given option, overwriting any previous value. void set_option(const char * name, std::string const & value) { diff --git a/src/backends/firebird/session.cpp b/src/backends/firebird/session.cpp index 612e261db..fa4d7ce51 100644 --- a/src/backends/firebird/session.cpp +++ b/src/backends/firebird/session.cpp @@ -9,7 +9,6 @@ #include "soci/firebird/soci-firebird.h" #include "firebird/error-firebird.h" #include "soci/session.h" -#include #include #include #include @@ -20,182 +19,6 @@ using namespace soci::details::firebird; namespace { -// Helpers of explodeISCConnectString() for reading words from a string. "Word" -// here is defined very loosely as just a sequence of non-space characters. -// -// All these helper functions update the input iterator to point to the first -// character not consumed by them. - -// Advance the input iterator until the first non-space character or end of the -// string. -void skipWhiteSpace(std::string::const_iterator& i, std::string::const_iterator const &end) -{ - std::locale const loc; - for (; i != end; ++i) - { - if (!std::isspace(*i, loc)) - break; - } -} - -// Return the string of all characters until the first space or the specified -// delimiter. -// -// Throws if the first non-space character after the end of the word is not the -// delimiter. However just returns en empty string, without throwing, if -// nothing is left at all in the string except for white space. -std::string -getWordUntil(std::string const &s, std::string::const_iterator &i, char delim) -{ - std::string::const_iterator const end = s.end(); - skipWhiteSpace(i, end); - - // We need to handle this case specially because it's not an error if - // nothing at all remains in the string. But if anything does remain, then - // we must have the delimiter. - if (i == end) - return std::string(); - - // Simply put anything until the delimiter into the word, stopping at the - // first white space character. - std::string word; - std::locale const loc; - for (; i != end; ++i) - { - if (*i == delim) - break; - - if (std::isspace(*i, loc)) - { - skipWhiteSpace(i, end); - if (i == end || *i != delim) - { - std::ostringstream os; - os << "Expected '" << delim << "' at position " - << (i - s.begin() + 1) - << " in Firebird connection string \"" - << s << "\"."; - - throw soci_error(os.str()); - } - - break; - } - - word += *i; - } - - if (i == end) - { - std::ostringstream os; - os << "Expected '" << delim - << "' not found before the end of the string " - << "in Firebird connection string \"" - << s << "\"."; - - throw soci_error(os.str()); - } - - ++i; // Skip the delimiter itself. - - return word; -} - -// Return a possibly quoted word, i.e. either just a sequence of non-space -// characters or everything inside a double-quoted string. -// -// Throws if the word is quoted and the closing quote is not found. However -// doesn't throw, just returns an empty string if there is nothing left. -std::string -getPossiblyQuotedWord(std::string const &s, std::string::const_iterator &i) -{ - std::string::const_iterator const end = s.end(); - skipWhiteSpace(i, end); - - std::string word; - - if (i != end && *i == '"') - { - for (;;) - { - if (++i == end) - { - std::ostringstream os; - os << "Expected '\"' not found before the end of the string " - "in Firebird connection string \"" - << s << "\"."; - - throw soci_error(os.str()); - } - - if (*i == '"') - { - ++i; - break; - } - - word += *i; - } - } - else // Not quoted. - { - std::locale const loc; - for (; i != end; ++i) - { - if (std::isspace(*i, loc)) - break; - - word += *i; - } - } - - return word; -} - -// retrieves parameters from the uniform connect string which is supposed to be -// in the form "key=value[ key2=value2 ...]" and the values may be quoted to -// allow including spaces into them. Notice that currently there is no way to -// include both a space and a double quote in a value. -std::map -explodeISCConnectString(std::string const &connectString) -{ - std::map parameters; - - std::string key, value; - for (std::string::const_iterator i = connectString.begin(); ; ) - { - key = getWordUntil(connectString, i, '='); - if (key.empty()) - break; - - value = getPossiblyQuotedWord(connectString, i); - - parameters.insert(std::pair(key, value)); - } - - return parameters; -} - -// extracts given parameter from map previusly build with explodeISCConnectString -bool getISCConnectParameter(std::map const & m, std::string const & key, - std::string & value) -{ - std::map :: const_iterator i; - value.clear(); - - i = m.find(key); - - if (i != m.end()) - { - value = i->second; - return true; - } - else - { - return false; - } -} - void setDPBOption(std::string& dpb, int const option, std::string const & value) { @@ -216,36 +39,35 @@ firebird_session_backend::firebird_session_backend( connection_parameters const & parameters) : dbhp_(0), trhp_(0) , decimals_as_strings_(false) { - // extract connection parameters - std::map - params(explodeISCConnectString(parameters.get_connect_string())); + auto params = parameters; + params.extract_options_from_space_separated_string(); ISC_STATUS stat[stat_size]; std::string param; // preparing connection options std::string dpb; - if (getISCConnectParameter(params, "user", param)) + if (params.get_option("user", param)) { setDPBOption(dpb, isc_dpb_user_name, param); } - if (getISCConnectParameter(params, "password", param)) + if (params.get_option("password", param)) { setDPBOption(dpb, isc_dpb_password, param); } - if (getISCConnectParameter(params, "role", param)) + if (params.get_option("role", param)) { setDPBOption(dpb, isc_dpb_sql_role_name, param); } - if (getISCConnectParameter(params, "charset", param)) + if (params.get_option("charset", param)) { setDPBOption(dpb, isc_dpb_lc_ctype, param); } - if (getISCConnectParameter(params, "service", param) == false) + if (!params.get_option("service", param)) { throw soci_error("Service name not specified."); } @@ -258,7 +80,7 @@ firebird_session_backend::firebird_session_backend( throw_iscerror(stat); } - if (getISCConnectParameter(params, "decimals_as_strings", param)) + if (params.get_option("decimals_as_strings", param)) { decimals_as_strings_ = param == "1" || param == "Y" || param == "y"; } diff --git a/src/core/connection-parameters.cpp b/src/core/connection-parameters.cpp index 161450c17..88b42f919 100644 --- a/src/core/connection-parameters.cpp +++ b/src/core/connection-parameters.cpp @@ -171,4 +171,159 @@ void connection_parameters::reset_after_move() backendRef_ = nullptr; } +namespace +{ + +// Helpers of extract_options_from_space_separated_string() for reading words +// from a string. "Word" here is defined very loosely as just a sequence of +// non-space characters. + +// We could use std::isspace() but it doesn't seem worth it to create a locale +// just for this. +inline bool isSpace(std::string::const_iterator i) +{ + return *i == ' ' || *i == '\t'; +} + +// All the functions below update the input iterator to point to the first +// character not consumed by them. + +// Advance the input iterator until the first non-space character or end of the +// string. +void +skipWhiteSpace(std::string::const_iterator& i, + std::string::const_iterator const& end) +{ + for (; i != end; ++i) + { + if (!isSpace(i)) + break; + } +} + +// Return the string of all characters until the first space or the specified +// delimiter. +// +// Throws if the first non-space character after the end of the word is not the +// delimiter. However just returns en empty string, without throwing, if +// nothing is left at all in the string except for white space. +std::string +getWordUntil(std::string const &s, std::string::const_iterator &i, char delim) +{ + std::string::const_iterator const end = s.end(); + skipWhiteSpace(i, end); + + // We need to handle this case specially because it's not an error if + // nothing at all remains in the string. But if anything does remain, then + // we must have the delimiter. + if (i == end) + return std::string(); + + // Simply put anything until the delimiter into the word, stopping at the + // first white space character. + std::string word; + for (; i != end; ++i) + { + if (*i == delim) + break; + + if (isSpace(i)) + { + skipWhiteSpace(i, end); + if (i == end || *i != delim) + { + std::ostringstream os; + os << "Expected '" << delim << "' at position " + << (i - s.begin() + 1) + << " in the connection string \"" + << s << "\"."; + + throw soci_error(os.str()); + } + + break; + } + + word += *i; + } + + if (i == end) + { + std::ostringstream os; + os << "Expected '" << delim + << "' not found before the end of the string " + << "in the connection string \"" + << s << "\"."; + + throw soci_error(os.str()); + } + + ++i; // Skip the delimiter itself. + + return word; +} + +// Return a possibly quoted word, i.e. either just a sequence of non-space +// characters or everything inside a double-quoted string. +// +// Throws if the word is quoted and the closing quote is not found. However +// doesn't throw, just returns an empty string if there is nothing left. +std::string +getPossiblyQuotedWord(std::string const &s, std::string::const_iterator &i) +{ + std::string::const_iterator const end = s.end(); + skipWhiteSpace(i, end); + + std::string word; + + if (i != end && *i == '"') + { + for (;;) + { + if (++i == end) + { + std::ostringstream os; + os << "Expected '\"' not found before the end of the string " + "in the connection string \"" + << s << "\"."; + + throw soci_error(os.str()); + } + + if (*i == '"') + { + ++i; + break; + } + + word += *i; + } + } + else // Not quoted. + { + for (; i != end; ++i) + { + if (isSpace(i)) + break; + + word += *i; + } + } + + return word; +} + +} // namespace anonymous +void connection_parameters::extract_options_from_space_separated_string() +{ + for (std::string::const_iterator i = connectString_.begin(); ; ) + { + const auto name = getWordUntil(connectString_, i, '='); + if (name.empty()) + break; + + options_[name] = getPossiblyQuotedWord(connectString_, i); + } +} + } // namespace soci diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d24bbdde8..69766d8eb 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -28,6 +28,7 @@ add_library(soci_tests_common STATIC common/test-main.cpp common/test-boost.cpp common/test-common.cpp + common/test-connparams.cpp common/test-custom.cpp common/test-dynamic.cpp common/test-lob.cpp diff --git a/tests/common/test-common.cpp b/tests/common/test-common.cpp index 16f2318a1..e6ea22bdb 100644 --- a/tests/common/test-common.cpp +++ b/tests/common/test-common.cpp @@ -3730,6 +3730,7 @@ TEST_CASE_METHOD(common_tests, "Logger", "[core][log]") // // These variables are defined in other files we want to force linking with. extern volatile bool soci_use_test_boost; +extern volatile bool soci_use_test_connparams; extern volatile bool soci_use_test_custom; extern volatile bool soci_use_test_dynamic; extern volatile bool soci_use_test_lob; @@ -3742,6 +3743,7 @@ test_context_common::test_context_common() soci_use_test_boost = true; #endif + soci_use_test_connparams = true; soci_use_test_custom = true; soci_use_test_dynamic = true; soci_use_test_lob = true; diff --git a/tests/common/test-connparams.cpp b/tests/common/test-connparams.cpp new file mode 100644 index 000000000..88d13d509 --- /dev/null +++ b/tests/common/test-connparams.cpp @@ -0,0 +1,112 @@ +// +// Copyright (C) 2024 Vadim Zeitlin +// Distributed under the Boost Software License, Version 1.0. +// (See accompanying file LICENSE_1_0.txt or copy at +// http://www.boost.org/LICENSE_1_0.txt) +// + +#include "soci/soci.h" + +#include + +#include "test-context.h" + +namespace soci +{ + +namespace tests +{ + +// This variable is referenced from test-common.cpp to force linking this file. +volatile bool soci_use_test_connparams = false; + +// A helper to check that parsing the given connection string works. +connection_parameters parse_connection_string(std::string const& connstr) +{ + connection_parameters params(backEnd, connstr); + + REQUIRE_NOTHROW(params.extract_options_from_space_separated_string()); + + return params; +} + +// A similar one checking that the given connection string is invalid. +void check_invalid_connection_string(std::string const& connstr) +{ + INFO(R"(Parsing invalid connection string ")" << connstr << R"(")"); + + connection_parameters params(backEnd, connstr); + + CHECK_THROWS_AS(params.extract_options_from_space_separated_string(), + soci_error); +} + +// Another helper to check that the given option has the expected value. +void +check_option(connection_parameters const& params, + char const* name, + std::string const& expected) +{ + std::string value; + if ( params.get_option(name, value) ) + { + CHECK(value == expected); + } + else + { + FAIL_CHECK(R"(Option ")" << name << R"(" not found)"); + } +} + +TEST_CASE("Connection string parsing", "[core][connstring]") +{ + SECTION("Invalid") + { + connection_parameters params(backEnd, ""); + + // Missing value. + check_invalid_connection_string("foo"); + check_invalid_connection_string("foo=ok bar"); + + // Missing quote. + check_invalid_connection_string(R"(foo=")"); + check_invalid_connection_string(R"(foo="bar)"); + check_invalid_connection_string(R"(foo="bar" baz="quux )"); + + // This one is not invalid (empty values are allowed), but it check + // because it used to dereference an invalid iterator (see #1175). + REQUIRE_NOTHROW(parse_connection_string("bloordyblop=")); + } + + SECTION("Typical") + { + std::string const s = "dbname=/some/path host=some.where readonly=1 port=1234"; + INFO(R"(Parsing connection string ")" << s << R"(")"); + + auto params = parse_connection_string(s); + + check_option(params, "dbname", "/some/path"); + check_option(params, "host", "some.where"); + check_option(params, "port", "1234"); + check_option(params, "readonly", "1"); + + std::string value; + CHECK_FALSE(params.get_option("user", value)); + } + + SECTION("Quotes") + { + std::string const s = R"(user="foo" pass="" service="bar baz")"; + INFO(R"(Parsing connection string ")" << s << R"(")"); + + auto params = parse_connection_string(s); + + check_option(params, "user", "foo"); + check_option(params, "pass", ""); + check_option(params, "service", "bar baz"); + } +} + +} // namespace tests + +} // namespace soci From 53370cc3561b230126811f6ead63b90463f505c7 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 7 Nov 2024 22:59:30 +0100 Subject: [PATCH 02/12] Reuse common connection_parameters function in Oracle backend Replace the use of Oracle-specific get_key_value() with the common extract_options_from_space_separated_string() to ensure consistent behaviour across the backends and fix a bug with dereferencing an invalid iterator (one past end) in Oracle-specific code. Closes #1175. --- src/backends/oracle/factory.cpp | 150 ++++++++------------------------ 1 file changed, 38 insertions(+), 112 deletions(-) diff --git a/src/backends/oracle/factory.cpp b/src/backends/oracle/factory.cpp index 31fa8cc77..67e16e9bd 100644 --- a/src/backends/oracle/factory.cpp +++ b/src/backends/oracle/factory.cpp @@ -22,53 +22,6 @@ using namespace soci; using namespace soci::details; -// iterates the string pointed by i, searching for pairs of key value. -// it returns the position after the value -std::string::const_iterator get_key_value(std::string::const_iterator & i, - std::string::const_iterator const & end, - std::string & key, - std::string & value) -{ - bool in_value = false; - bool quoted = false; - - key.clear(); - value.clear(); - - while (i != end) - { - if (in_value == false) - { - if (*i == '=') - { - in_value = true; - if (i != end && *(i + 1) == '"') - { - quoted = true; - ++i; // jump over the quote - } - } - else if (!isspace(*i)) - { - key += *i; - } - } - else - { - if ((quoted == true && *i == '"') || (quoted == false && isspace(*i))) - { - return ++i; - } - else - { - value += *i; - } - } - ++i; - } - return i; -} - // decode charset and ncharset names int charset_code(const std::string & name) { @@ -105,84 +58,57 @@ int charset_code(const std::string & name) return code; } -// retrieves service name, user name and password from the -// uniform connect string -void chop_connect_string(std::string const & connectString, - std::string & serviceName, std::string & userName, - std::string & password, int & mode, bool & decimals_as_strings, - int & charset, int & ncharset) +oracle_session_backend * oracle_backend_factory::make_session( + connection_parameters const & parameters) const { - serviceName.clear(); - userName.clear(); - password.clear(); - mode = OCI_DEFAULT; - decimals_as_strings = false; - charset = 0; - ncharset = 0; - - std::string key, value; - std::string::const_iterator i = connectString.begin(); - while (i != connectString.end()) + std::string value; + + auto params = parameters; + params.extract_options_from_space_separated_string(); + + std::string serviceName, userName, password; + params.get_option("service", serviceName); + params.get_option("user", userName); + params.get_option("password", password); + + int mode = OCI_DEFAULT; + if (params.get_option("mode", value)) { - i = get_key_value(i, connectString.end(), key, value); - if (key == "service") - { - serviceName = value; - } - else if (key == "user") - { - userName = value; - } - else if (key == "password") - { - password = value; - } - else if (key == "mode") + if (value == "sysdba") { - if (value == "sysdba") - { - mode = OCI_SYSDBA; - } - else if (value == "sysoper") - { - mode = OCI_SYSOPER; - } - else if (value == "default") - { - mode = OCI_DEFAULT; - } - else - { - throw soci_error("Invalid connection mode."); - } + mode = OCI_SYSDBA; } - else if (key == "decimals_as_strings") + else if (value == "sysoper") { - decimals_as_strings = value == "1" || value == "Y" || value == "y"; + mode = OCI_SYSOPER; } - else if (key == "charset") + else if (value == "default") { - charset = charset_code(value); + mode = OCI_DEFAULT; } - else if (key == "ncharset") + else { - ncharset = charset_code(value); + throw soci_error("Invalid connection mode."); } } -} -// concrete factory for Empty concrete strategies -oracle_session_backend * oracle_backend_factory::make_session( - connection_parameters const & parameters) const -{ - std::string serviceName, userName, password; - int mode; - bool decimals_as_strings; - int charset; - int ncharset; + bool decimals_as_strings = false; + if (params.get_option("decimals_as_strings", value)) + { + decimals_as_strings = value == "1" || value == "Y" || value == "y"; + } - chop_connect_string(parameters.get_connect_string(), serviceName, userName, password, - mode, decimals_as_strings, charset, ncharset); + int charset = 0; + if (params.get_option("charset", value)) + { + charset = charset_code(value); + } + + int ncharset = 0; + if (params.get_option("ncharset", value)) + { + ncharset = charset_code(value); + } return new oracle_session_backend(serviceName, userName, password, mode, decimals_as_strings, charset, ncharset); From ae5ec3b7df12bdd4b68e95064027a49dfed86dd9 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 7 Nov 2024 23:03:30 +0100 Subject: [PATCH 03/12] Reuse cstring_to_unsigned() in Oracle backend code Replace the use of std::istringstream with the SOCI function which is marginally more efficient but, more importantly, more correct, as it checks for overflow. --- src/backends/oracle/factory.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/backends/oracle/factory.cpp b/src/backends/oracle/factory.cpp index 67e16e9bd..b20398a66 100644 --- a/src/backends/oracle/factory.cpp +++ b/src/backends/oracle/factory.cpp @@ -9,11 +9,13 @@ #include "soci/oracle/soci-oracle.h" #include "soci/connection-parameters.h" #include "soci/backend-loader.h" + +#include "soci-cstrtoi.h" + #include #include #include #include -#include #ifdef _MSC_VER #pragma warning(disable:4355) @@ -45,11 +47,7 @@ int charset_code(const std::string & name) else { // allow explicit number value - - std::istringstream ss(name); - - ss >> code; - if (!ss) + if (!cstring_to_unsigned(code, name.c_str())) { throw soci_error("Invalid character set name."); } From 11173788a58436e59a0444f62130621f7eb0eb05 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 8 Nov 2024 16:08:04 +0100 Subject: [PATCH 04/12] Allow options without values in connection strings At least some backends, e.g. SQLite3, allow specifying boolean options without parameters to enable them (e.g. just "readonly" instead of "readonly=1") and this seems to makes sense, so do allow such syntax. However still forbid values without names, e.g. "=1", which was silently allowed before even though it didn't make any sense. Also effectively inline getWordUntil() into the function calling it, as it makes it simpler to distinguish between having a value and not. Adjust the tests to the new behaviour. --- src/core/connection-parameters.cpp | 110 ++++++++++++----------------- tests/common/test-connparams.cpp | 6 +- 2 files changed, 49 insertions(+), 67 deletions(-) diff --git a/src/core/connection-parameters.cpp b/src/core/connection-parameters.cpp index 88b42f919..b994efe12 100644 --- a/src/core/connection-parameters.cpp +++ b/src/core/connection-parameters.cpp @@ -201,68 +201,6 @@ skipWhiteSpace(std::string::const_iterator& i, } } -// Return the string of all characters until the first space or the specified -// delimiter. -// -// Throws if the first non-space character after the end of the word is not the -// delimiter. However just returns en empty string, without throwing, if -// nothing is left at all in the string except for white space. -std::string -getWordUntil(std::string const &s, std::string::const_iterator &i, char delim) -{ - std::string::const_iterator const end = s.end(); - skipWhiteSpace(i, end); - - // We need to handle this case specially because it's not an error if - // nothing at all remains in the string. But if anything does remain, then - // we must have the delimiter. - if (i == end) - return std::string(); - - // Simply put anything until the delimiter into the word, stopping at the - // first white space character. - std::string word; - for (; i != end; ++i) - { - if (*i == delim) - break; - - if (isSpace(i)) - { - skipWhiteSpace(i, end); - if (i == end || *i != delim) - { - std::ostringstream os; - os << "Expected '" << delim << "' at position " - << (i - s.begin() + 1) - << " in the connection string \"" - << s << "\"."; - - throw soci_error(os.str()); - } - - break; - } - - word += *i; - } - - if (i == end) - { - std::ostringstream os; - os << "Expected '" << delim - << "' not found before the end of the string " - << "in the connection string \"" - << s << "\"."; - - throw soci_error(os.str()); - } - - ++i; // Skip the delimiter itself. - - return word; -} - // Return a possibly quoted word, i.e. either just a sequence of non-space // characters or everything inside a double-quoted string. // @@ -314,15 +252,59 @@ getPossiblyQuotedWord(std::string const &s, std::string::const_iterator &i) } } // namespace anonymous + void connection_parameters::extract_options_from_space_separated_string() { + constexpr char delim = '='; + + std::string::const_iterator const end = connectString_.end(); for (std::string::const_iterator i = connectString_.begin(); ; ) { - const auto name = getWordUntil(connectString_, i, '='); + skipWhiteSpace(i, end); + + // Anything until the delimiter or space is the name. + std::string name; + std::string value; + for (;;) + { + if (i == end || isSpace(i)) + break; + + if (*i == delim) + { + if (name.empty()) + { + std::ostringstream os; + os << "Unexpected '" + << delim + << "' without a name at position " + << (i - connectString_.begin() + 1) + << " in the connection string \"" + << connectString_ + << "\"."; + + throw soci_error(os.str()); + } + + ++i; // Skip the delimiter itself. + + // And get the option value which follows it. + value = getPossiblyQuotedWord(connectString_, i); + break; + } + + name += *i++; + } + if (name.empty()) + { + // We've reached the end of the string and there is nothing left. break; + } - options_[name] = getPossiblyQuotedWord(connectString_, i); + // Note that value may be empty here, we intentionally allow specifying + // options without values, e.g. just "switch" instead of "switch=1". + options_[name] = value; } } diff --git a/tests/common/test-connparams.cpp b/tests/common/test-connparams.cpp index 88d13d509..5467bc6e9 100644 --- a/tests/common/test-connparams.cpp +++ b/tests/common/test-connparams.cpp @@ -64,9 +64,9 @@ TEST_CASE("Connection string parsing", "[core][connstring]") { connection_parameters params(backEnd, ""); - // Missing value. - check_invalid_connection_string("foo"); - check_invalid_connection_string("foo=ok bar"); + // Missing name. + check_invalid_connection_string("="); + check_invalid_connection_string("foo=ok =bar"); // Missing quote. check_invalid_connection_string(R"(foo=")"); From ae2ca287a800d131b1a5838d2fdf4a4ed1a6701b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 8 Nov 2024 01:10:14 +0100 Subject: [PATCH 05/12] Reuse connection_parameters parsing in SQLite3 backend Don't duplicate the code for parsing space-separated possibly quoted words in SQLite3 backend but just use the common function for it too. Also correct handling of some boolean parameters, where previously specifying any value, even "0" or "false", would enable it and document the exact values accepted for them. Add tests checking that parsing connection string works as expected. --- docs/backends/sqlite3.md | 13 ++- src/backends/sqlite3/session.cpp | 131 +++++++++++++++++-------------- tests/sqlite3/test-sqlite3.cpp | 39 +++++++++ 3 files changed, 123 insertions(+), 60 deletions(-) diff --git a/docs/backends/sqlite3.md b/docs/backends/sqlite3.md index a77a7f2a5..37f9a74af 100644 --- a/docs/backends/sqlite3.md +++ b/docs/backends/sqlite3.md @@ -47,14 +47,21 @@ session sql("sqlite3", "db=db.sqlite timeout=2 shared_cache=true"); The set of parameters used in the connection string for SQLite is: -* `dbname` or `db` +* `dbname` or `db` - this parameter is required unless the entire connection +string is just the database name, in which case it must not contain any `=` +signs. * `timeout` - set sqlite busy timeout (in seconds) ([link](http://www.sqlite.org/c3ref/busy_timeout.html)) * `readonly` - open database in read-only mode instead of the default read-write (note that the database file must already exist in this case, see [the documentation](https://www.sqlite.org/c3ref/open.html)) * `nocreate` - open an existing database without creating a new one if it doesn't already exist (by default, a new database file is created). * `synchronous` - set the pragma synchronous flag ([link](http://www.sqlite.org/pragma.html#pragma_synchronous)) -* `shared_cache` - should be `true` ([link](http://www.sqlite.org/c3ref/enable_shared_cache.html)) +* `shared_cache` - enable or disabled shared pager cache ([link](http://www.sqlite.org/c3ref/enable_shared_cache.html)) * `vfs` - set the SQLite VFS used to as OS interface. The VFS should be registered before opening the connection, see [the documenation](https://www.sqlite.org/vfs.html) -* `foreign_keys` - set the pragma foreign_keys flag ([link](https://www.sqlite.org/pragma.html#pragma_foreign_keys)). +* `foreign_keys` - set the pragma `foreign_keys` flag ([link](https://www.sqlite.org/pragma.html#pragma_foreign_keys)). + +Boolean options `readonly`, `nocreate`, and `shared_cache` can be either +specified without any value, which is equivalent to setting them to `1`, or set +to one of `1`, `yes`, `true` or `on` to enable the option or `0`, `no`, `false` +or `off` to disable it. Specifying any other value results in an error. Once you have created a `session` object as shown above, you can use it to access the database, for example: diff --git a/src/backends/sqlite3/session.cpp b/src/backends/sqlite3/session.cpp index 8c20d46bd..d860aa42a 100644 --- a/src/backends/sqlite3/session.cpp +++ b/src/backends/sqlite3/session.cpp @@ -105,6 +105,38 @@ static bool check_if_sequence_table_exists(sqlite_api::sqlite3* conn) return sequence_table_exists; } +// Helper function returning true if the given option was found with any of the +// values that SQLite considers to be true, false if it was found with any +// value considered false by SQLite or not found at all and throws an exception +// if it was found with any other value. +bool check_bool_option(connection_parameters const& params, const char* name) +{ + std::string val; + + if (!params.get_option(name, val)) + return false; + + // At least for compatibility (but also because this is convenient and + // makes sense), we accept "readonly" as synonym for "readonly=1" etc. + if (val.empty()) + return true; + + // See https://www.sqlite.org/pragma.html + if (val == "1" || val == "yes" || val == "true" || val == "on") + return true; + + if (val == "0" || val == "no" || val == "false" || val == "off") + return false; + + std::ostringstream ss; + ss << R"(Invalid value ")" + << val + << R"(" for boolean option ")" + << name + << '"'; + throw sqlite3_soci_error(ss.str(), 0); +}; + sqlite3_session_backend::sqlite3_session_backend( connection_parameters const & parameters) : sequence_table_exists_(false) @@ -115,66 +147,51 @@ sqlite3_session_backend::sqlite3_session_backend( std::string synchronous; std::string foreignKeys; std::string const & connectString = parameters.get_connect_string(); - std::string dbname(connectString); - std::stringstream ssconn(connectString); - while (!ssconn.eof() && ssconn.str().find('=') != std::string::npos) - { - std::string key, val; - std::getline(ssconn, key, '='); - std::getline(ssconn, val, ' '); - - if (val.size()>0 && val[0]=='\"') - { - std::string quotedVal = val.erase(0, 1); + std::string dbname; - if (quotedVal[quotedVal.size()-1] == '\"') - { - quotedVal.erase(val.size()-1); - } - else // space inside value string - { - std::getline(ssconn, val, '\"'); - quotedVal = quotedVal + " " + val; - std::string keepspace; - std::getline(ssconn, keepspace, ' '); - } + auto params = parameters; + if (connectString.find('=') == std::string::npos) + { + // The entire connection string must be just the database name. + dbname = connectString; + } + else + { + params.extract_options_from_space_separated_string(); + } - val = quotedVal; - } + std::string val; + if (params.get_option("dbname", val) || params.get_option("db", val)) + { + dbname = val; + } + if (params.get_option("timeout", val)) + { + std::istringstream converter(val); + converter >> timeout; + } + if (params.get_option("synchronous", val)) + { + synchronous = val; + } + if (check_bool_option(params, "readonly")) + { + connection_flags = (connection_flags | SQLITE_OPEN_READONLY) & ~(SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE); + } + if (check_bool_option(params, "nocreate")) + { + connection_flags &= ~SQLITE_OPEN_CREATE; + } + if (check_bool_option(params, "shared_cache")) + { + connection_flags |= SQLITE_OPEN_SHAREDCACHE; + } + params.get_option("vfs", vfs); + params.get_option("foreign_keys", foreignKeys); - if ("dbname" == key || "db" == key) - { - dbname = val; - } - else if ("timeout" == key) - { - std::istringstream converter(val); - converter >> timeout; - } - else if ("synchronous" == key) - { - synchronous = val; - } - else if ("readonly" == key) - { - connection_flags = (connection_flags | SQLITE_OPEN_READONLY) & ~(SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE); - } - else if ("nocreate" == key) - { - connection_flags &= ~SQLITE_OPEN_CREATE; - } - else if ("shared_cache" == key && "true" == val) - { - connection_flags |= SQLITE_OPEN_SHAREDCACHE; - } - else if ("vfs" == key) - { - vfs = val; - } - else if ("foreign_keys" == key) - { - foreignKeys = val; - } + if (dbname.empty()) + { + throw sqlite3_soci_error("Database name must be specified", 0); } int res = sqlite3_open_v2(dbname.c_str(), &conn_, connection_flags, (vfs.empty()?NULL:vfs.c_str())); diff --git a/tests/sqlite3/test-sqlite3.cpp b/tests/sqlite3/test-sqlite3.cpp index 80a327438..89a5e8232 100644 --- a/tests/sqlite3/test-sqlite3.cpp +++ b/tests/sqlite3/test-sqlite3.cpp @@ -17,6 +17,45 @@ using namespace soci::tests; std::string connectString; backend_factory const &backEnd = *soci::factory_sqlite3(); +TEST_CASE("SQLite connection string", "[sqlite][connstring]") +{ + CHECK_THROWS_WITH(soci::session(backEnd, ""), + Catch::Contains("Database name must be specified")); + CHECK_THROWS_WITH(soci::session(backEnd, "readonly=1"), + Catch::Contains("Database name must be specified")); + + CHECK_THROWS_WITH(soci::session(backEnd, "readonly=\""), + Catch::Contains("Expected '\"'")); + CHECK_THROWS_WITH(soci::session(backEnd, "readonly=maybe"), + Catch::Contains("Invalid value")); + + CHECK_THROWS_WITH(soci::session(backEnd, "db=no-such-file nocreate=1"), + Catch::Contains("Cannot establish connection")); + + CHECK_NOTHROW(soci::session(backEnd, "dbname=:memory: nocreate")); + CHECK_NOTHROW(soci::session(backEnd, "dbname=:memory: foreign_keys=on")); + + // Also check an alternative way of specifying the connection parameters. + connection_parameters params(backEnd, "dbname=still-no-such-file"); + params.set_option("foreign_keys", "1"); + params.set_option("nocreate", "1"); + CHECK_THROWS_WITH(soci::session(params), + Catch::Contains("Cannot establish connection")); + + // Finally allow testing arbitrary connection strings by specifying them in + // the environment variables. + if (auto const connstr = std::getenv("SOCI_TEST_CONNSTR_GOOD")) + { + CHECK_NOTHROW(soci::session(backEnd, connstr)); + } + + if (auto const connstr = std::getenv("SOCI_TEST_CONNSTR_BAD")) + { + CHECK_THROWS_AS(soci::session(backEnd, connstr), soci_error); + } + +} + // ROWID test // In sqlite3 the row id can be called ROWID, _ROWID_ or oid TEST_CASE("SQLite rowid", "[sqlite][rowid][oid]") From 9e2faf452671c4ca2883fbaead0cab0a025ecb75 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 8 Nov 2024 16:21:42 +0100 Subject: [PATCH 06/12] Correct example of PostgreSQL connection string Use something in valid PostgreSQL syntax, i.e. space, and not semicolon, separated and using "dbname" instead of "database". --- tests/postgresql/test-postgresql.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/postgresql/test-postgresql.cpp b/tests/postgresql/test-postgresql.cpp index 0b60cfca7..0a7e80896 100644 --- a/tests/postgresql/test-postgresql.cpp +++ b/tests/postgresql/test-postgresql.cpp @@ -1483,7 +1483,7 @@ class test_context : public test_context_common std::string get_example_connection_string() const override { - return "Host=localhost;Port=5432;Database=test;User=postgres;Password=postgres"; + return "host=localhost port=5432 dbname=test user=postgres password=postgres"; } table_creator_base* table_creator_1(soci::session& s) const override From ce6066e06aee1c98a09b3510f785270c1dc3a1d8 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 8 Nov 2024 16:33:56 +0100 Subject: [PATCH 07/12] Remove remaining SOCI_POSTGRESQL_NOSINGLEROWMODE checks We don't support ancient versions of PostgreSQL without single row mode support, so don't bother checking whether it is available. This should have been part of e3bb60e1 (Remove vestigial traces of SOCI_POSTGRESQL_NOSINLGEROWMODE, 2024-03-27). --- docs/backends/postgresql.md | 5 ----- src/backends/postgresql/statement.cpp | 30 --------------------------- 2 files changed, 35 deletions(-) diff --git a/docs/backends/postgresql.md b/docs/backends/postgresql.md index 3cbcd8c9d..bf9454c75 100644 --- a/docs/backends/postgresql.md +++ b/docs/backends/postgresql.md @@ -62,11 +62,6 @@ Note that in the single-row operation: * bulk queries are not supported, and * in order to fulfill the expectations of the underlying client library, the complete rowset has to be exhausted before executing further queries on the same session. -Also please note that single rows mode requires PostgreSQL 9 or later, both at -compile- and run-time. If you need to support earlier versions of PostgreSQL, -you can define `SOCI_POSTGRESQL_NOSINGLEROWMODE` when building the library to -disable it. - Once you have created a `session` object as shown above, you can use it to access the database, for example: ```cpp diff --git a/src/backends/postgresql/statement.cpp b/src/backends/postgresql/statement.cpp index f65934753..5de3c182b 100644 --- a/src/backends/postgresql/statement.cpp +++ b/src/backends/postgresql/statement.cpp @@ -23,7 +23,6 @@ namespace // unnamed { // used only with asynchronous operations in single-row mode -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE void wait_until_operation_complete(postgresql_session_backend & session) { for (;;) @@ -49,7 +48,6 @@ void throw_soci_error(PGconn * conn, const char * msg) throw soci_error(description); } -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE } // unnamed namespace @@ -61,12 +59,6 @@ postgresql_statement_backend::postgresql_statement_backend( hasIntoElements_(false), hasVectorIntoElements_(false), hasUseElements_(false), hasVectorUseElements_(false) { -#ifdef SOCI_POSTGRESQL_NOSINGLEROWMODE - if (single_row_mode) - { - throw soci_error("Single row mode not supported in this version of the library"); - } -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE } postgresql_statement_backend::~postgresql_statement_backend() @@ -229,7 +221,6 @@ void postgresql_statement_backend::prepare(std::string const & query, // if it fails to prepare it we can't DEALLOCATE it. std::string statementName = session_.get_next_statement_name(); -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_) { // prepare for single-row retrieval @@ -245,7 +236,6 @@ void postgresql_statement_backend::prepare(std::string const & query, wait_until_operation_complete(session_); } else -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE { // default multi-row query execution @@ -265,12 +255,10 @@ void postgresql_statement_backend::prepare(std::string const & query, statement_backend::exec_fetch_result postgresql_statement_backend::execute(int number) { -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_ && (number > 1)) { throw soci_error("Bulk operations are not supported with single-row mode."); } -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE // If the statement was "just described", then we know that // it was actually executed with all the use elements @@ -365,7 +353,6 @@ postgresql_statement_backend::execute(int number) { // this query was separately prepared -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_) { int result = PQsendQueryPrepared(session_.conn_, @@ -386,7 +373,6 @@ postgresql_statement_backend::execute(int number) } } else -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE { // default multi-row execution @@ -401,7 +387,6 @@ postgresql_statement_backend::execute(int number) // this query was not separately prepared and should // be executed as a one-time query -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_) { int result = PQsendQueryParams(session_.conn_, query_.c_str(), @@ -421,7 +406,6 @@ postgresql_statement_backend::execute(int number) } } else -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE { // default multi-row execution @@ -462,7 +446,6 @@ postgresql_statement_backend::execute(int number) { // this query was separately prepared -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_) { int result = PQsendQueryPrepared(session_.conn_, @@ -481,7 +464,6 @@ postgresql_statement_backend::execute(int number) } } else -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE { // default multi-row execution @@ -491,7 +473,6 @@ postgresql_statement_backend::execute(int number) } else // stType_ == st_one_time_query { -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_) { int result = PQsendQuery(session_.conn_, query_.c_str()); @@ -509,7 +490,6 @@ postgresql_statement_backend::execute(int number) } } else -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE { // default multi-row execution @@ -520,7 +500,6 @@ postgresql_statement_backend::execute(int number) } bool process_result; -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_) { if (justDescribed_) @@ -537,7 +516,6 @@ postgresql_statement_backend::execute(int number) process_result = result_.check_for_data("Cannot execute query."); } else -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE { // default multi-row execution @@ -579,12 +557,10 @@ postgresql_statement_backend::execute(int number) statement_backend::exec_fetch_result postgresql_statement_backend::fetch(int number) { -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_ && (number > 1)) { throw soci_error("Bulk operations are not supported with single-row mode."); } -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE // Note: // In the multi-row mode this function does not actually fetch anything from anywhere @@ -599,7 +575,6 @@ postgresql_statement_backend::fetch(int number) if (currentRow_ >= numberOfRows_) { -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_) { PGresult* res = PQgetResult(session_.conn_); @@ -626,7 +601,6 @@ postgresql_statement_backend::fetch(int number) } } else -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE { // default multi-row execution @@ -639,7 +613,6 @@ postgresql_statement_backend::fetch(int number) { if (currentRow_ + number > numberOfRows_) { -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_) { rowsToConsume_ = 1; @@ -647,7 +620,6 @@ postgresql_statement_backend::fetch(int number) return ef_success; } else -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE { // default multi-row execution @@ -661,13 +633,11 @@ postgresql_statement_backend::fetch(int number) } else { -#ifndef SOCI_POSTGRESQL_NOSINGLEROWMODE if (single_row_mode_) { rowsToConsume_ = 1; } else -#endif // !SOCI_POSTGRESQL_NOSINGLEROWMODE { rowsToConsume_ = number; } From c5cc10eaee0753440e7ea8921ccd862a3e2f2027 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 8 Nov 2024 18:28:49 +0100 Subject: [PATCH 08/12] Extract case conversion functions to a header Even though these functions are trivial, still collect them in a single place instead of repeating them in several different ones. This will also make replacing them with something less trivial, e.g. using ICU, simpler in the future. --- include/private/soci-case.h | 58 ++++++++++++++++++++++++++++++ src/backends/sqlite3/statement.cpp | 10 ++---- src/core/row.cpp | 8 ++--- 3 files changed, 64 insertions(+), 12 deletions(-) create mode 100644 include/private/soci-case.h diff --git a/include/private/soci-case.h b/include/private/soci-case.h new file mode 100644 index 000000000..e41627295 --- /dev/null +++ b/include/private/soci-case.h @@ -0,0 +1,58 @@ +// +// Copyright (C) 2024 Vadim Zeitlin. +// Distributed under the Boost Software License, Version 1.0. +// (See accompanying file LICENSE_1_0.txt or copy at +// http://www.boost.org/LICENSE_1_0.txt) +// + +#ifndef SOCI_PRIVATE_SOCI_CASE_H_INCLUDED +#define SOCI_PRIVATE_SOCI_CASE_H_INCLUDED + +#include +#include + +namespace soci +{ + +namespace details +{ + +// Simplistic conversions of strings to upper/lower case. +// +// This doesn't work correctly for arbitrary Unicode strings for well-known +// reasons (such conversions can't be done correctly on char by char basis), +// but they do work for ASCII strings that we deal with and for anything else +// we'd need ICU -- which we could start using later, if necessary, by just +// replacing these functions with the versions using ICU functions instead. + +inline std::string string_toupper(std::string const& s) +{ + std::string res; + res.reserve(s.size()); + + for (char c : s) + { + res += static_cast(std::toupper(static_cast(c))); + } + + return res; +} + +inline std::string string_tolower(std::string const& s) +{ + std::string res; + res.reserve(s.size()); + + for (char c : s) + { + res += static_cast(std::tolower(static_cast(c))); + } + + return res; +} + +} // namespace details + +} // namespace soci + +#endif // SOCI_PRIVATE_SOCI_CASE_H_INCLUDED diff --git a/src/backends/sqlite3/statement.cpp b/src/backends/sqlite3/statement.cpp index 099cbb512..24b23b661 100644 --- a/src/backends/sqlite3/statement.cpp +++ b/src/backends/sqlite3/statement.cpp @@ -16,16 +16,12 @@ #include #include +#include "soci-case.h" + #ifdef _MSC_VER #pragma warning(disable:4355) #endif -// This is used instead of tolower() just to avoid warnings about int to char -// casts inside MSVS std::transform() implementation. -char toLowerCh(char c) { - return static_cast( std::tolower(c) ); -} - using namespace soci; using namespace soci::details; using namespace sqlite_api; @@ -550,7 +546,7 @@ void sqlite3_statement_backend::describe_column(int colNum, dt.resize(siter - dt.begin()); // do all comparisons in lower case - std::transform(dt.begin(), dt.end(), dt.begin(), toLowerCh); + dt = string_tolower(dt); sqlite3_data_type_map::const_iterator iter = dataTypeMap.find(dt); if (iter != dataTypeMap.end()) diff --git a/src/core/row.cpp b/src/core/row.cpp index fe59e1412..ab94c3dba 100644 --- a/src/core/row.cpp +++ b/src/core/row.cpp @@ -10,10 +10,11 @@ #include "soci/type-holder.h" #include -#include #include #include +#include "soci-case.h" + using namespace soci; using namespace details; @@ -40,10 +41,7 @@ void row::add_properties(column_properties const &cp) std::string const & originalName = cp.get_name(); if (uppercaseColumnNames_) { - for (std::size_t i = 0; i != originalName.size(); ++i) - { - columnName.push_back(static_cast(std::toupper(originalName[i]))); - } + columnName = string_toupper(originalName); // rewrite the column name in the column_properties object // as well to retain consistent views From ff20d620c804e04bc48c9e086dbfc8c13c97cc8c Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 9 Nov 2024 15:49:33 +0100 Subject: [PATCH 09/12] Add connection_parameters::is_true_value() Extract this function from SQLite3 backend, as it will be useful in the other ones too, and also change it to use case-insensitive comparison. --- include/soci/connection-parameters.h | 15 ++++++++--- src/backends/sqlite3/session.cpp | 38 +++------------------------- src/core/connection-parameters.cpp | 28 ++++++++++++++++++++ 3 files changed, 43 insertions(+), 38 deletions(-) diff --git a/include/soci/connection-parameters.h b/include/soci/connection-parameters.h index 7977ec68b..2b7ad7831 100644 --- a/include/soci/connection-parameters.h +++ b/include/soci/connection-parameters.h @@ -82,12 +82,21 @@ class SOCI_DECL connection_parameters return true; } - // Return true if the option with the given name was found with option_true - // value. + // Return true if the option with the given name has one of the values + // considered to be true, i.e. "1", "yes", "true" or "on" or is empty. + // Return false if the value is one of "0", "no", "false" or "off" or the + // option was not specified at all. + // + // Throw an exception if the option was given but the value is none of + // the above, comparing case-insensitively. + static bool is_true_value(const char * name, std::string const & value); + + // Return true if the option with the given name was found with a "true" + // value in the sense of is_true_value() above. bool is_option_on(const char * name) const { std::string value; - return get_option(name, value) && value == option_true; + return get_option(name, value) && is_true_value(name, value); } private: diff --git a/src/backends/sqlite3/session.cpp b/src/backends/sqlite3/session.cpp index d860aa42a..77f8c8a65 100644 --- a/src/backends/sqlite3/session.cpp +++ b/src/backends/sqlite3/session.cpp @@ -105,38 +105,6 @@ static bool check_if_sequence_table_exists(sqlite_api::sqlite3* conn) return sequence_table_exists; } -// Helper function returning true if the given option was found with any of the -// values that SQLite considers to be true, false if it was found with any -// value considered false by SQLite or not found at all and throws an exception -// if it was found with any other value. -bool check_bool_option(connection_parameters const& params, const char* name) -{ - std::string val; - - if (!params.get_option(name, val)) - return false; - - // At least for compatibility (but also because this is convenient and - // makes sense), we accept "readonly" as synonym for "readonly=1" etc. - if (val.empty()) - return true; - - // See https://www.sqlite.org/pragma.html - if (val == "1" || val == "yes" || val == "true" || val == "on") - return true; - - if (val == "0" || val == "no" || val == "false" || val == "off") - return false; - - std::ostringstream ss; - ss << R"(Invalid value ")" - << val - << R"(" for boolean option ")" - << name - << '"'; - throw sqlite3_soci_error(ss.str(), 0); -}; - sqlite3_session_backend::sqlite3_session_backend( connection_parameters const & parameters) : sequence_table_exists_(false) @@ -174,15 +142,15 @@ sqlite3_session_backend::sqlite3_session_backend( { synchronous = val; } - if (check_bool_option(params, "readonly")) + if (params.is_option_on("readonly")) { connection_flags = (connection_flags | SQLITE_OPEN_READONLY) & ~(SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE); } - if (check_bool_option(params, "nocreate")) + if (params.is_option_on("nocreate")) { connection_flags &= ~SQLITE_OPEN_CREATE; } - if (check_bool_option(params, "shared_cache")) + if (params.is_option_on("shared_cache")) { connection_flags |= SQLITE_OPEN_SHAREDCACHE; } diff --git a/src/core/connection-parameters.cpp b/src/core/connection-parameters.cpp index b994efe12..c8766e8e7 100644 --- a/src/core/connection-parameters.cpp +++ b/src/core/connection-parameters.cpp @@ -10,6 +10,8 @@ #include "soci/soci-backend.h" #include "soci/backend-loader.h" +#include "soci-case.h" + char const * soci::option_reconnect = "reconnect"; char const * soci::option_true = "1"; @@ -171,6 +173,32 @@ void connection_parameters::reset_after_move() backendRef_ = nullptr; } +/* static */ +bool +connection_parameters::is_true_value(char const* name, std::string const& value) +{ + // At least for compatibility (but also because this is convenient and + // makes sense), we accept "readonly" as synonym for "readonly=1" etc. + if (value.empty()) + return true; + + std::string const val = details::string_tolower(value); + + if (val == "1" || val == "yes" || val == "true" || val == "on") + return true; + + if (val == "0" || val == "no" || val == "false" || val == "off") + return false; + + std::ostringstream os; + os << R"(Invalid value ")" + << value + << R"(" for boolean option ")" + << name + << '"'; + throw soci_error(os.str()); +} + namespace { From 7739a042bd6162e19e6f2f57971d49ee1328bef2 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 9 Nov 2024 15:57:34 +0100 Subject: [PATCH 10/12] Add connection_parameters::extract_option() This is similar to get_option() but removes the option from the connection parameters and will be used in PostgreSQL backend, which needs to pass the string with only the options known to libpq, but not those specific to SOCI, when opening the database connection soon. --- include/soci/connection-parameters.h | 14 ++++++++++++++ tests/common/test-connparams.cpp | 17 +++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/include/soci/connection-parameters.h b/include/soci/connection-parameters.h index 2b7ad7831..82c9ca2ec 100644 --- a/include/soci/connection-parameters.h +++ b/include/soci/connection-parameters.h @@ -82,6 +82,20 @@ class SOCI_DECL connection_parameters return true; } + // Same as get_option(), but also removes the option from the connection + // string if it was present in it. + bool extract_option(const char * name, std::string & value) + { + Options::iterator const it = options_.find(name); + if (it == options_.end()) + return false; + + value = it->second; + options_.erase(it); + + return true; + } + // Return true if the option with the given name has one of the values // considered to be true, i.e. "1", "yes", "true" or "on" or is empty. // Return false if the value is one of "0", "no", "false" or "off" or the diff --git a/tests/common/test-connparams.cpp b/tests/common/test-connparams.cpp index 5467bc6e9..524c5e200 100644 --- a/tests/common/test-connparams.cpp +++ b/tests/common/test-connparams.cpp @@ -107,6 +107,23 @@ TEST_CASE("Connection string parsing", "[core][connstring]") } } +TEST_CASE("connection_parameters::extract_option", "[core][connstring]") +{ + std::string value; + + connection_parameters params(backEnd, "foo bar=baz"); + params.extract_options_from_space_separated_string(); + + // Extracting an option must remove it. + CHECK(params.extract_option("foo", value)); + CHECK(!params.get_option("foo", value)); + + CHECK_FALSE(params.extract_option("baz", value)); + + CHECK(params.extract_option("bar", value)); + CHECK(!params.get_option("bar", value)); +} + } // namespace tests } // namespace soci From 1e084138aa28bc41f9311c3c4eb9753d41e1f77c Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 9 Nov 2024 16:03:55 +0100 Subject: [PATCH 11/12] Add connection_parameters::build_string_from_options() For now this will be only used to create the connection string in libpq format, but might be used for the other backends too in the future, so add it to the core. --- include/soci/connection-parameters.h | 4 +++ src/core/connection-parameters.cpp | 44 ++++++++++++++++++++++++++++ tests/common/test-connparams.cpp | 10 +++++++ 3 files changed, 58 insertions(+) diff --git a/include/soci/connection-parameters.h b/include/soci/connection-parameters.h index 82c9ca2ec..028e57df0 100644 --- a/include/soci/connection-parameters.h +++ b/include/soci/connection-parameters.h @@ -62,6 +62,10 @@ class SOCI_DECL connection_parameters // Note that currently unknown options are simply ignored. void extract_options_from_space_separated_string(); + // Build a space-separated string from the options, quoting the options + // values using the provided quote character. + std::string build_string_from_options(char quote) const; + // Set the value of the given option, overwriting any previous value. void set_option(const char * name, std::string const & value) diff --git a/src/core/connection-parameters.cpp b/src/core/connection-parameters.cpp index c8766e8e7..930758ea3 100644 --- a/src/core/connection-parameters.cpp +++ b/src/core/connection-parameters.cpp @@ -336,4 +336,48 @@ void connection_parameters::extract_options_from_space_separated_string() } } +std::string +connection_parameters::build_string_from_options(char quote) const +{ + std::string res; + + for (auto const& option : options_) + { + if (!res.empty()) + { + res += ' '; + } + + res += option.first; + res += '='; + + // Quote the value if it contains spaces or the quote character itself. + auto const& value = option.second; + if (value.empty() || + value.find(' ') != std::string::npos || + value.find(quote) != std::string::npos) + { + res += quote; + + for (char c : value) + { + if (c == quote) + { + res += '\\'; + } + + res += c; + } + + res += quote; + } + else + { + res += value; + } + } + + return res; +} + } // namespace soci diff --git a/tests/common/test-connparams.cpp b/tests/common/test-connparams.cpp index 524c5e200..84d601f6d 100644 --- a/tests/common/test-connparams.cpp +++ b/tests/common/test-connparams.cpp @@ -124,6 +124,16 @@ TEST_CASE("connection_parameters::extract_option", "[core][connstring]") CHECK(!params.get_option("bar", value)); } +TEST_CASE("connection_parameters::build_string_from_options", "[core][connstring]") +{ + connection_parameters params(backEnd, R"(foo bar="baz" quux="1 2")"); + params.extract_options_from_space_separated_string(); + + // Check that unecessary quotes are removed, empty value are explicitly + // specified and the required quotes are kept. + CHECK(params.build_string_from_options('\'') == "bar=baz foo='' quux='1 2'"); +} + } // namespace tests } // namespace soci From 1a6607bc2fe31a909e7b7e00ec2a0c5a8afd7eae Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 9 Nov 2024 16:10:16 +0100 Subject: [PATCH 12/12] Remove manual connection string parsing from PostgreSQL backend Use connection_parameters instead for simplicity and correctness: this fixes the equivalent of #1175 in this backend too. --- include/soci/postgresql/soci-postgresql.h | 3 +- src/backends/postgresql/factory.cpp | 95 +---------------------- src/backends/postgresql/session.cpp | 33 +++++++- tests/postgresql/test-postgresql.cpp | 11 +++ 4 files changed, 43 insertions(+), 99 deletions(-) diff --git a/include/soci/postgresql/soci-postgresql.h b/include/soci/postgresql/soci-postgresql.h index 3be655798..03e3c1f02 100644 --- a/include/soci/postgresql/soci-postgresql.h +++ b/include/soci/postgresql/soci-postgresql.h @@ -384,8 +384,7 @@ class SOCI_POSTGRESQL_DECL postgresql_blob_backend : public details::blob_backen struct SOCI_POSTGRESQL_DECL postgresql_session_backend : details::session_backend { - postgresql_session_backend(connection_parameters const & parameters, - bool single_row_mode); + explicit postgresql_session_backend(connection_parameters const & parameters); ~postgresql_session_backend() override; diff --git a/src/backends/postgresql/factory.cpp b/src/backends/postgresql/factory.cpp index dbd7b6309..8e9f41783 100644 --- a/src/backends/postgresql/factory.cpp +++ b/src/backends/postgresql/factory.cpp @@ -18,103 +18,10 @@ using namespace soci; using namespace soci::details; -namespace // unnamed -{ - -// iterates the string pointed by i, searching for pairs of key value. -// it returns the position after the value -std::string::const_iterator get_key_value(std::string::const_iterator & i, - std::string::const_iterator const & end, - std::string & key, - std::string & value) -{ - bool in_value = false; - bool quoted = false; - - key.clear(); - value.clear(); - - while (i != end) - { - if (in_value == false) - { - if (*i == '=') - { - in_value = true; - if (i != end && *(i + 1) == '"') - { - quoted = true; - ++i; // jump over the quote - } - } - else if (!isspace(*i)) - { - key += *i; - } - } - else - { - if ((quoted == true && *i == '"') || (quoted == false && isspace(*i))) - { - return ++i; - } - else - { - value += *i; - } - } - ++i; - } - return i; -} - -// retrieves specific parameters from the -// uniform connect string -std::string chop_connect_string(std::string const & connectString, - bool & single_row_mode) -{ - std::string pruned_conn_string; - - single_row_mode = false; - - std::string key, value; - std::string::const_iterator i = connectString.begin(); - while (i != connectString.end()) - { - i = get_key_value(i, connectString.end(), key, value); - if (key == "singlerow" || key == "singlerows") - { - single_row_mode = (value == "true" || value == "yes"); - } - else - { - if (pruned_conn_string.empty() == false) - { - pruned_conn_string += ' '; - } - - pruned_conn_string += key + '=' + value; - } - } - - return pruned_conn_string; -} - -} // unnamed namespace - -// concrete factory for Empty concrete strategies postgresql_session_backend * postgresql_backend_factory::make_session( connection_parameters const & parameters) const { - bool single_row_mode; - - const std::string pruned_conn_string = - chop_connect_string(parameters.get_connect_string(), single_row_mode); - - connection_parameters pruned_parameters(parameters); - pruned_parameters.set_connect_string(pruned_conn_string); - - return new postgresql_session_backend(pruned_parameters, single_row_mode); + return new postgresql_session_backend(parameters); } postgresql_backend_factory const soci::postgresql; diff --git a/src/backends/postgresql/session.cpp b/src/backends/postgresql/session.cpp index 782d038e5..9a75e9adc 100644 --- a/src/backends/postgresql/session.cpp +++ b/src/backends/postgresql/session.cpp @@ -141,10 +141,10 @@ std::string create_case_list_of_strings(const std::vector& list) } // namespace unnamed postgresql_session_backend::postgresql_session_backend( - connection_parameters const& parameters, bool single_row_mode) + connection_parameters const& parameters) : statementCount_(0), conn_(0) { - single_row_mode_ = single_row_mode; + single_row_mode_ = false; connect(parameters); } @@ -152,7 +152,34 @@ postgresql_session_backend::postgresql_session_backend( void postgresql_session_backend::connect( connection_parameters const& parameters) { - PGconn* conn = PQconnectdb(parameters.get_connect_string().c_str()); + auto params = parameters; + params.extract_options_from_space_separated_string(); + + // Extract SOCI-specific options, i.e. check if they're present and remove + // them from params to avoid passing them to PQconnectdb() below. + std::string value; + + // This one is not used by this backend, but can be present in the + // connection string if we're called from session::reconnect(). + params.extract_option(option_reconnect, value); + + // Notice that we accept both variants only for compatibility. + char const* name; + if (params.extract_option("singlerow", value)) + name = "singlerow"; + else if (params.extract_option("singlerows", value)) + name = "singlerows"; + else + name = nullptr; + + if (name) + { + single_row_mode_ = connection_parameters::is_true_value(name, value); + } + + // We can't use SOCI connection string with PQconnectdb() directly because + // libpq uses single quotes instead of double quotes used by SOCI. + PGconn* conn = PQconnectdb(params.build_string_from_options('\'').c_str()); if (0 == conn || CONNECTION_OK != PQstatus(conn)) { std::string msg = "Cannot establish connection to the database."; diff --git a/tests/postgresql/test-postgresql.cpp b/tests/postgresql/test-postgresql.cpp index 0a7e80896..36533155d 100644 --- a/tests/postgresql/test-postgresql.cpp +++ b/tests/postgresql/test-postgresql.cpp @@ -130,6 +130,17 @@ struct oid_table_creator : public table_creator_base } }; +TEST_CASE("PostgreSQL connection string", "[postgresql][connstring]") +{ + // There are no required parts in libpq connection string, so we can only + // test that invalid options are detected. + CHECK_THROWS_WITH(soci::session(backEnd, "bloordyblop=1"), + Catch::Contains(R"(invalid connection option "bloordyblop")")); + + CHECK_THROWS_WITH(soci::session(backEnd, "sslmode=bloordyblop"), + Catch::Contains(R"(invalid sslmode value: "bloordyblop")")); +} + // ROWID test // Note: in PostgreSQL, there is no ROWID, there is OID. // It is still provided as a separate type for "portability",