From 39fc253cd5b8076e5d5fde54938e8616ed7f7b79 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 24 Jan 2022 15:05:40 -0500 Subject: [#2284] Fixed asserts in PostgresSQL support code src/hooks/dhcp/pgsql_cb/pgsql_cb_dhcp4.cc Fixed an incorrect tagged statement src/lib/pgsql/pgsql_connection.* PgSqlConnection::executePreparedStatement() - new convenience function to executing prepared statements PgSqlConnection::selectQuery() PgSqlConnection::insertQuery() PgSqlConnection::updateDeleteQuery() - now calls executePreparedStatement() src/lib/pgsql/pgsql_exchange.cc PsqlBindArray::toText() - emits text for empty array src/lib/pgsql/tests/pgsql_connection_unittest.cc TEST_F(PgSqlConnectionTest, executePreparedStatement) - new test --- src/lib/pgsql/pgsql_connection.cc | 70 +++++++++++++----------- src/lib/pgsql/pgsql_connection.h | 21 +++++++ src/lib/pgsql/pgsql_exchange.cc | 4 ++ src/lib/pgsql/pgsql_exchange.h | 2 + src/lib/pgsql/tests/pgsql_connection_unittest.cc | 63 ++++++++++++++++++++- 5 files changed, 128 insertions(+), 32 deletions(-) (limited to 'src/lib/pgsql') diff --git a/src/lib/pgsql/pgsql_connection.cc b/src/lib/pgsql/pgsql_connection.cc index c958e78d10..c1efe81a8d 100644 --- a/src/lib/pgsql/pgsql_connection.cc +++ b/src/lib/pgsql/pgsql_connection.cc @@ -466,26 +466,49 @@ PgSqlConnection::executeSQL(const std::string& sql) { checkStatementError(r, statement); } +PgSqlResultPtr +PgSqlConnection::executePreparedStatement(PgSqlTaggedStatement& statement, + const PsqlBindArray& in_bindings) { + checkUnusable(); + + if (statement.nbparams != in_bindings.size()) { + isc_throw (InvalidOperation, "executePreparedStatement:" + << " expected: " << statement.nbparams + << " parameters, given: " << in_bindings.size() + << ", statement: " << statement.name + << ", SQL: " << statement.text); + } + + const char* const* values = 0; + const int * lengths = 0; + const int * formats = 0; + if (statement.nbparams > 0) { + values = static_cast(&in_bindings.values_[0]); + lengths = static_cast(&in_bindings.lengths_[0]); + formats = static_cast(&in_bindings.formats_[0]); + } + + PgSqlResultPtr result_set; + result_set.reset(new PgSqlResult(PQexecPrepared(conn_, statement.name, statement.nbparams, + values, lengths, formats, 0))); + + checkStatementError(*result_set, statement); + return(result_set); +} + void PgSqlConnection::selectQuery(PgSqlTaggedStatement& statement, const PsqlBindArray& in_bindings, ConsumeResultRowFun process_result_row) { - checkUnusable(); - - PgSqlResult result_set(PQexecPrepared(conn_, statement.name, - statement.nbparams, - &in_bindings.values_[0], - &in_bindings.lengths_[0], - &in_bindings.formats_[0], 0)); - - checkStatementError(result_set, statement); + // Execute the prepared statement. + PgSqlResultPtr result_set = executePreparedStatement(statement, in_bindings); // Iterate over the returned rows and invoke the row consumption // function on each one. - int rows = result_set.getRows(); + int rows = result_set->getRows(); for (int row = 0; row < rows; ++row) { try { - process_result_row(result_set, row); + process_result_row(*result_set, row); } catch (const std::exception& ex) { // Rethrow the exception with a bit more data. isc_throw(BadValue, ex.what() << ". Statement is <" << @@ -497,32 +520,17 @@ PgSqlConnection::selectQuery(PgSqlTaggedStatement& statement, void PgSqlConnection::insertQuery(PgSqlTaggedStatement& statement, const PsqlBindArray& in_bindings) { - checkUnusable(); - - PgSqlResult result_set(PQexecPrepared(conn_, statement.name, - statement.nbparams, - &in_bindings.values_[0], - &in_bindings.lengths_[0], - &in_bindings.formats_[0], 0)); - - checkStatementError(result_set, statement); + // Execute the prepared statement. + PgSqlResultPtr result_set = executePreparedStatement(statement, in_bindings); } uint64_t PgSqlConnection::updateDeleteQuery(PgSqlTaggedStatement& statement, const PsqlBindArray& in_bindings) { - checkUnusable(); - - PgSqlResult result_set(PQexecPrepared(conn_, statement.name, - statement.nbparams, - &in_bindings.values_[0], - &in_bindings.lengths_[0], - &in_bindings.formats_[0], 0)); - - checkStatementError(result_set, statement); + // Execute the prepared statement. + PgSqlResultPtr result_set = executePreparedStatement(statement, in_bindings); - // Return the number of affected rows. - return (boost::lexical_cast(PQcmdTuples(result_set))); + return (boost::lexical_cast(PQcmdTuples(*result_set))); } } // end of isc::db namespace diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h index 2f11474ea0..5422613fd3 100644 --- a/src/lib/pgsql/pgsql_connection.h +++ b/src/lib/pgsql/pgsql_connection.h @@ -406,6 +406,27 @@ public: } } + /// @brief Excutes a prepared SQL statement. + /// + /// It executes the given prepared SQL statement, after checking + /// for usability and input parameter sanity. After the statement + /// is excuted @c checkStatementError() is invoked to ensure we detect + /// connectivity issues properly. Upon successful execution, the + /// the result set is returned. It may be used for any form of + /// prepared SQL statement (e.g query, insert, udpate, delete...), + /// with or without input parameters. + /// + /// @param statement PgSqlTaggedStatement describing the prepared + /// statement to execute. + /// @param in_bindings array of input parameter bindings. If the SQL + /// statement requires no input arguments, this parameter should either + /// be omitted or an empty PsqlBindArray should be supplied. + /// @throw InvalidOperation if the number of parameters expected + /// by the statement does not match the size of the input bind array. + PgSqlResultPtr executePreparedStatement(PgSqlTaggedStatement& statement, + const PsqlBindArray& in_bindings + = PsqlBindArray()); + /// @brief Executes SELECT query using prepared statement. /// /// The statement parameter refers to an existing prepared statement diff --git a/src/lib/pgsql/pgsql_exchange.cc b/src/lib/pgsql/pgsql_exchange.cc index a16d8e057c..7246166548 100644 --- a/src/lib/pgsql/pgsql_exchange.cc +++ b/src/lib/pgsql/pgsql_exchange.cc @@ -260,6 +260,10 @@ std::string PsqlBindArray::toText() const { std::ostringstream stream; + if (values_.size() == 0) { + return ("bindarray is empty"); + } + for (int i = 0; i < values_.size(); ++i) { stream << i << " : "; diff --git a/src/lib/pgsql/pgsql_exchange.h b/src/lib/pgsql/pgsql_exchange.h index d81474da86..2dec0a83f4 100644 --- a/src/lib/pgsql/pgsql_exchange.h +++ b/src/lib/pgsql/pgsql_exchange.h @@ -151,6 +151,8 @@ typedef boost::shared_ptr PgSqlResultPtr; typedef boost::shared_ptr ConstStringPtr; struct PsqlBindArray { + PsqlBindArray() : values_(0), lengths_(0), formats_(0) {}; + /// @brief Vector of pointers to the data values. std::vector values_; /// @brief Vector of data lengths for each value. diff --git a/src/lib/pgsql/tests/pgsql_connection_unittest.cc b/src/lib/pgsql/tests/pgsql_connection_unittest.cc index 64be3234db..8b7ab7a701 100644 --- a/src/lib/pgsql/tests/pgsql_connection_unittest.cc +++ b/src/lib/pgsql/tests/pgsql_connection_unittest.cc @@ -88,6 +88,8 @@ public: DELETE_BY_INT_RANGE, INSERT_VALUE, UPDATE_BY_INT_VALUE, + GET_ALL_ROWS, + DELETE_ALL_ROWS, NUM_STATEMENTS }; @@ -113,7 +115,13 @@ public: { 2, { OID_INT4, OID_TEXT }, "UPDATE_BY_INT_VALUE", "UPDATE basics SET text_col = $2" - " WHERE int_col = $1" } + " WHERE int_col = $1" }, + + { 0, { OID_NONE }, "GET_ALL_ROWS", + "SELECT int_col, text_col FROM basics" }, + + { 0, { OID_NONE }, "DELETE_ALL_ROWS", + "DELETE FROM basics" } }}; /// @brief Structure for holding data values describing a single @@ -271,6 +279,59 @@ public: } }; +/// @brief Verifies basics of input parameter sanity checking and statement +/// execution enforced by executePreparedStatement. Higher order tests +/// verify actual data CRUD results. +TEST_F(PgSqlConnectionTest, executePreparedStatement) { + + // Executing with no paramters when they are required should throw. + // First we'll omit the bindings (defaults to empty). + PgSqlResultPtr r; + ASSERT_THROW_MSG(r = conn_->executePreparedStatement(tagged_statements[INSERT_VALUE]), + InvalidOperation, + "executePreparedStatement: expected: 2 parameters, given: 0," + " statement: INSERT_INT_TEXT, SQL: INSERT INTO basics " + "(int_col,text_col) VALUES ($1, $2)"); + + // Now we'll pass in an empty array. + PsqlBindArray in_bindings; + ASSERT_THROW_MSG(r = conn_->executePreparedStatement(tagged_statements[INSERT_VALUE], + in_bindings), + InvalidOperation, + "executePreparedStatement: expected: 2 parameters, given: 0," + " statement: INSERT_INT_TEXT, SQL: INSERT INTO basics " + "(int_col,text_col) VALUES ($1, $2)"); + + // Executing without parameters when none are expected should be fine. + // First we'll simply omit the array. + ASSERT_NO_THROW(r = conn_->executePreparedStatement(tagged_statements[GET_ALL_ROWS])); + + // Now with an empty array. + ASSERT_NO_THROW(r = conn_->executePreparedStatement(tagged_statements[GET_ALL_ROWS], in_bindings)); + + // Executing with parameters when none are required should throw. + in_bindings.add(1); + in_bindings.add(2); + ASSERT_THROW_MSG(r = conn_->executePreparedStatement(tagged_statements[GET_ALL_ROWS], + in_bindings), + InvalidOperation, + "executePreparedStatement: expected: 0 parameters, given: 2," + " statement: GET_ALL_ROWS, SQL: SELECT int_col, text_col FROM basics"); + + // Executing with the correct number of parameters should work. + ASSERT_NO_THROW(r = conn_->executePreparedStatement(tagged_statements[GET_BY_INT_RANGE], + in_bindings)); + + // Executing with too many parameters should fail. + in_bindings.add(3); + ASSERT_THROW_MSG(r = conn_->executePreparedStatement(tagged_statements[GET_BY_INT_RANGE], + in_bindings), + InvalidOperation, + "executePreparedStatement: expected: 2 parameters, given: 3," + " statement: GET_BY_INT_RANGE, SQL: SELECT int_col, text_col" + " FROM basics WHERE int_col >= $1 and int_col <= $2"); +} + /// @brief Verify that we can insert rows with /// PgSqlConnection::insertQuery() and fetch /// them using PgSqlConnection::selectQuery(). -- cgit v1.2.3