[4276] Addressed review comments

Minor cleanups, added some unit testing of PgSqlExchange functions.

src/lib/dhcpsrv/pgsql_connection.cc

@@ -9,15 +9,6 @@
 #include <dhcpsrv/dhcpsrv_log.h>
 #include <dhcpsrv/pgsql_connection.h>
 
-#include <boost/static_assert.hpp>
-
-#include <iostream>
-#include <iomanip>
-#include <limits>
-#include <sstream>
-#include <string>
-#include <time.h>
-
 // 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

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.

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 << " : ";

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 <dhcpsrv/pgsql_connection.h>
 
@@ -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 <column>))".
     ///
+    /// 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

src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc

@@ -14,6 +14,8 @@
 #include <dhcpsrv/testutils/pgsql_schema.h>
 #include <exceptions/exceptions.h>
 
+#include <boost/lexical_cast.hpp>
+
 #include <gtest/gtest.h>
 
 #include <algorithm>
@@ -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<std::string>(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
+