From: Thomas Markwalder Date: Mon, 23 May 2016 17:59:00 +0000 (-0400) Subject: [4276] Addressed review comments X-Git-Tag: trac4106_update_base~12^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ebfa39dc312e63ba690d2d41e770917b6ab45061;p=thirdparty%2Fkea.git [4276] Addressed review comments Minor cleanups, added some unit testing of PgSqlExchange functions. --- diff --git a/src/lib/dhcpsrv/pgsql_connection.cc b/src/lib/dhcpsrv/pgsql_connection.cc index c820fb4188..016b06154d 100644 --- a/src/lib/dhcpsrv/pgsql_connection.cc +++ b/src/lib/dhcpsrv/pgsql_connection.cc @@ -9,15 +9,6 @@ #include #include -#include - -#include -#include -#include -#include -#include -#include - // PostgreSQL errors should be tested based on the SQL state code. Each state // code is 5 decimal, ASCII, digits, the first two define the category of // error, the last three are the specific error. PostgreSQL makes the state diff --git a/src/lib/dhcpsrv/pgsql_connection.h b/src/lib/dhcpsrv/pgsql_connection.h index d20cfb2545..9a292ca21a 100755 --- a/src/lib/dhcpsrv/pgsql_connection.h +++ b/src/lib/dhcpsrv/pgsql_connection.h @@ -76,7 +76,7 @@ const size_t OID_VARCHAR = 1043; /// guarantees that the resources are released even if the an exception is /// thrown. -class PgSqlResult { +class PgSqlResult : public boost::noncopyable { public: /// @brief Constructor /// @@ -130,7 +130,7 @@ public: /// @brief Constructor /// - /// Initialize PgSql + /// Sets the Postgresql API connector handle to NULL. /// PgSqlHolder() : pgconn_(NULL) { } @@ -144,6 +144,9 @@ public: } } + /// @brief Sets the connection to the value given + /// + /// @param connection - pointer to the Postgresql connection instance void setConnection(PGconn* connection) { if (pgconn_ != NULL) { // Already set? Release the current connection first. diff --git a/src/lib/dhcpsrv/pgsql_exchange.cc b/src/lib/dhcpsrv/pgsql_exchange.cc index f1a90ee217..436af0b0d6 100644 --- a/src/lib/dhcpsrv/pgsql_exchange.cc +++ b/src/lib/dhcpsrv/pgsql_exchange.cc @@ -42,7 +42,7 @@ void PsqlBindArray::add(const bool& value) { add(value ? TRUE_STR : FALSE_STR); } -std::string PsqlBindArray::toText() { +std::string PsqlBindArray::toText() const { std::ostringstream stream; for (int i = 0; i < values_.size(); ++i) { stream << i << " : "; diff --git a/src/lib/dhcpsrv/pgsql_exchange.h b/src/lib/dhcpsrv/pgsql_exchange.h index a5af449b53..20d9d6e223 100644 --- a/src/lib/dhcpsrv/pgsql_exchange.h +++ b/src/lib/dhcpsrv/pgsql_exchange.h @@ -4,8 +4,8 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. -#ifndef PGSQL_EXCHANGE_MGR_H -#define PGSQL_EXCHANGE_MGR_H +#ifndef PGSQL_EXCHANGE_H +#define PGSQL_EXCHANGE_H #include @@ -20,7 +20,7 @@ namespace dhcp { /// PostgreSQL execute call. /// /// Note that the data values are stored as pointers. These pointers need to -/// valid for the duration of the PostgreSQL statement execution. In other +/// be valid for the duration of the PostgreSQL statement execution. In other /// words populating them with pointers to values that go out of scope before /// statement is executed is a bad idea. struct PsqlBindArray { @@ -44,14 +44,14 @@ struct PsqlBindArray { /// @brief Fetches the number of entries in the array. /// @return Returns size_t containing the number of entries. - size_t size() { + size_t size() const { return (values_.size()); } /// @brief Indicates it the array is empty. /// @return Returns true if there are no entries in the array, false /// otherwise. - bool empty() { + bool empty() const { return (values_.empty()); } @@ -91,7 +91,7 @@ struct PsqlBindArray { /// @brief Dumps the contents of the array to a string. /// @return std::string containing the dump - std::string toText(); + std::string toText() const; }; /// @brief Base class for marshalling data to and from PostgreSQL. @@ -126,6 +126,9 @@ public: /// when stored. Likewise, these columns are automatically adjusted /// upon retrieval unless fetched via "extract(epoch from ))". /// + /// Unless we start using binary input, timestamp columns must be input as + /// date/time strings. + /// /// @param cltt Client last transmit time /// @param valid_lifetime Valid lifetime /// @@ -137,6 +140,9 @@ public: /// @brief Converts time stamp from the database to a time_t /// + /// We're fetching timestamps as an integer string of seconds since the + /// epoch. This method converts such a string int a time_t. + /// /// @param db_time_val timestamp to be converted. This value /// is expected to be the number of seconds since the epoch /// expressed as base-10 integer string. @@ -240,4 +246,4 @@ protected: }; // end of isc::dhcp namespace }; // end of isc namespace -#endif // PGSQL_EXCHANGE_MGR_H +#endif // PGSQL_EXCHANGE_H diff --git a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc index 5dc97e4fef..8e6596de78 100755 --- a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc @@ -14,6 +14,8 @@ #include #include +#include + #include #include @@ -386,4 +388,49 @@ TEST_F(PgSqlLeaseMgrTest, getExpiredLeases6) { testGetExpiredLeases6(); } -}; +/// @brief Basic checks on time conversion functions in PgSqlExchange +/// We input timestamps as date/time strings and we output them as +/// an integer string of seconds since the epoch. There is no meangingful +/// way to test them round-trip without Postgres involved. +TEST(PgSqlExchange, convertTimeTest) { + // Get a reference time and time string + time_t ref_time; + struct tm tinfo; + char buffer[20]; + + time(&ref_time); + localtime_r(&ref_time, &tinfo); + strftime(buffer, sizeof(buffer), "%Y-%m-%d %H:%M:%S", &tinfo); + std::string ref_time_str(buffer); + + // Verify convertToDatabaseTime gives us the expected localtime string + std::string time_str = PgSqlExchange::convertToDatabaseTime(ref_time); + EXPECT_EQ(ref_time_str, time_str); + + // Verify convertToDatabaseTime with valid_lifetime = 0 gives us the + // expected localtime string + time_str = PgSqlExchange::convertToDatabaseTime(ref_time, 0); + EXPECT_EQ(time_str, ref_time_str); + + // Add a day, we should get a string that's greater than the reference + // string. Ok, maybe not the most exacting test, but you want I should + // parse this? + std::string time_str2; + ASSERT_NO_THROW(time_str2 = PgSqlExchange::convertToDatabaseTime(ref_time, + 24*3600)); + EXPECT_GT(time_str2, ref_time_str); + + // Verify too large of a value is detected. + ASSERT_THROW(PgSqlExchange::convertToDatabaseTime(DatabaseConnection:: + MAX_DB_TIME, 24*3600), + isc::BadValue); + + // Make sure Conversion "from" database time functions + std::string ref_secs_str = boost::lexical_cast(ref_time); + time_t from_time = PgSqlExchange::convertFromDatabaseTime(ref_secs_str); + from_time = PgSqlExchange::convertFromDatabaseTime(ref_secs_str); + EXPECT_EQ(ref_time, from_time); +} + +}; // namespace +