[2559] Remove database access string from the parser class

This is now constructed "ont the fly" when the configuration change
is committed.

src/lib/dhcpsrv/dbaccess_parser.cc

@@ -13,6 +13,8 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <dhcpsrv/dbaccess_parser.h>
+#include <dhcpsrv/dhcpsrv_log.h>
+#include <dhcpsrv/lease_mgr_factory.h>
 
 #include <boost/foreach.hpp>
 
@@ -26,10 +28,14 @@ using namespace isc::data;
 namespace isc {
 namespace dhcp {
 
-typedef map<string, ConstElementPtr> ConfigPairMap;
-typedef pair<string, ConstElementPtr> ConfigPair;
-typedef map<string, string> StringPairMap;
-typedef pair<string, string> StringPair;
+
+// Factory function to build the parser
+DbAccessParser::DbAccessParser(const std::string& param_name) : values_()
+{
+    if (param_name != "lease-database") {
+        LOG_WARN(dhcpsrv_logger, DHCPSRV_UNEXPECTED_NAME).arg(param_name);
+    }
+}
 
 // Parse the configuration and check that the various keywords are consistent.
 void
@@ -46,7 +52,7 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
     map<string, string> values_copy = values_;
 
     // 2. Update the copy with the passed keywords.
-    BOOST_FOREACH(ConfigPair param, config_value->mapValue()) {
+    BOOST_FOREACH(DbAccessParser::ConfigPair param, config_value->mapValue()) {
         values_copy[param.first] = param.second->stringValue();
     }
 
@@ -71,28 +77,32 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
 
     // 4. If all is OK, update the stored keyword/value pairs.
     values_ = values_copy;
+}
+
+// Commit the changes - reopen the database with the new parameters
+void
+DbAccessParser::commit() {
+    // Close current lease manager.
+    LeaseMgrFactory::destroy();
 
-    // 5. Construct the updated database access string: omit keywords where
-    //    the value string is empty.
-    dbaccess_ = "";
+    // Construct the database access string from all keywords and values in the
+    // parameter map where the value is not null.
+    string dbaccess;
     BOOST_FOREACH(StringPair keyval, values_) {
         if (!keyval.second.empty()) {
 
             // Separate keyword/value pair from predecessor (if there is one).
-            if (! dbaccess_.empty()) {
-                dbaccess_ += std::string(" ");
+            if (! dbaccess.empty()) {
+                dbaccess += std::string(" ");
             }
 
             // Add the keyword/value pair to the access string.
-            dbaccess_ += (keyval.first + std::string("=") + keyval.second);
+            dbaccess += (keyval.first + std::string("=") + keyval.second);
         }
     }
-}
 
-// Commit the changes - reopen the database with the new parameters
-void
-DbAccessParser::commit() {
-    std::cout << "DB_ACCESS_PARSER_COMMIT: " << dbaccess_ << "\n";
+    // ... and open the database using that access string.
+    LeaseMgrFactory::create(dbaccess);
 }
 
 };  // namespace dhcp

src/lib/dhcpsrv/dbaccess_parser.h

@@ -44,13 +44,20 @@ public:
 /// depend on the datbase chosen.
 class DbAccessParser: public DhcpConfigParser {
 public:
+    /// @brief Combination of parameter name and configuration contents
+    typedef std::pair<std::string, isc::data::ConstElementPtr> ConfigPair;
+
+    /// @brief Keyword and associated value
+    typedef std::pair<std::string, std::string> StringPair;
+
+    /// @brief Keyword/value collection of database access parameters
+    typedef std::map<std::string, std::string> StringPairMap;
 
     /// @brief Default constructor
     ///
-    /// @param param_name Name of the configuration parameter being parsed.
-    DbAccessParser(const std::string& param_name)
-        : dbaccess_(), param_name_(param_name), values_()
-    {}
+    /// @param param_name Name of the parameter under which the database
+    ///        access details are held.
+    DbAccessParser(const std::string& param_name);
 
     /// The destructor.
     virtual ~DbAccessParser()
@@ -89,23 +96,28 @@ public:
     /// @brief Factory method to create parser
     ///
     /// Creates an instance of this parser.
+    ///
+    /// @param name Name of the parameter used to access the configuration.
+    ///
+    /// @return Pointer to a DbAccessParser.  The caller is responsible for
+    ///         destroying the parser after use.
     static DhcpConfigParser* factory(const std::string& param_name) {
         return (new DbAccessParser(param_name));
     }
 
-    /// @brief Get database access string
+    /// @brief Get database access parameters
     ///
     /// Used in testing to check that the configuration information has been
     /// parsed corrected.
-    std::string getDbAccessString() const {
-        return (dbaccess_);
+    ///
+    /// @return Map of keyword/value pairs representing database access
+    ///         information.
+    const StringPairMap& getDbAccessParameters() const {
+        return (values_);
     }
 
 private:
-    std::string     dbaccess_;      ///< Database access string
-    std::string     param_name_;    ///< Parameter name
-    std::map<std::string, std::string> values_;
-                                    ///< Stored parameter values
+    std::map<std::string, std::string> values_; ///< Stored parameter values
 };
 
 };  // namespace dhcp

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -233,6 +233,13 @@ a database backend, but where no 'type' keyword has been included in
 the access string.  The access string (less any passwords) is included
 in the message.
 
+% DHCPSRV_UNEXPECTED_NAME database access parameters passed through '%1', expected 'lease-database'
+The parameters for access the lease database were passed to the server through
+the named configuration parameter, but the code was expecting them to be
+passed via the parameter named "lease-database".  If the database opens
+successfully, there is no impact on server operation.  However, as this does
+indicate an error in the source code, please submit a bug report.
+
 % DHCPSRV_UNKNOWN_DB unknown database type: %1
 The database access string specified a database type (given in the
 message) that is unknown to the software.  This is a configuration error.

src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc

@@ -101,15 +101,15 @@ public:
     ///
     /// @param trace_string String that will be used to set the value of a
     ///        SCOPED_TRACE for this call.
-    /// @param dbaccess Database access string to check
+    /// @param dbaccess set of database access parameters to check
     /// @param keyval Array of "const char*" strings in the order keyword,
     ///        value, keyword, value ...  A NULL entry terminates the list.
-    void checkAccessString(const char* trace_string, std::string& dbaccess,
+    void checkAccessString(const char* trace_string,
+                           const DbAccessParser::StringPairMap& parameters,
                            const char* keyval[]) {
         SCOPED_TRACE(trace_string);
 
-        // Construct a map of keyword value pairs.  Check that no keyword
-        // is repeated.
+        // Construct a map of keyword value pairs.
         map<string, string> expected;
         size_t expected_count = 0;
         for (size_t i = 0; keyval[i] != NULL; i += 2) {
@@ -123,20 +123,18 @@ public:
             ++expected_count;
         }
 
-        // Check no duplicates in the supplied keywords
+        // Check no duplicates in the test set of reference keywords.
         ASSERT_EQ(expected_count, expected.size()) << 
             "Supplied reference keyword/value list contains duplicate keywords";
 
-        // Split the database access string.
-        const LeaseMgr::ParameterMap dbamap = LeaseMgrFactory::parse(dbaccess);
-
-        // It should have the same number keyword value pairs as the
-        EXPECT_EQ(expected_count, dbamap.size());
+        // The passed parameter map should have the same number of entries as
+        // the reference set of keywords.
+        EXPECT_EQ(expected_count, parameters.size());
 
         // Check that the keywords and keyword values are the same: loop
         // through the keywords in the database access string.
-        for (LeaseMgr::ParameterMap::const_iterator actual = dbamap.begin();
-             actual != dbamap.end(); ++actual) {
+        for (LeaseMgr::ParameterMap::const_iterator actual = parameters.begin();
+             actual != parameters.end(); ++actual) {
 
             // Does the keyword exist in the set of expected keywords?
             map<string, string>::iterator corresponding =
@@ -160,8 +158,23 @@ TEST_F(DbAccessParserTest, validTypeMemfile) {
 
     DbAccessParser parser("lease-database");
     EXPECT_NO_THROW(parser.build(json_elements));
-    string dbaccess = parser.getDbAccessString();
-    checkAccessString("Valid memfile", dbaccess, config);
+    checkAccessString("Valid memfile", parser.getDbAccessParameters(), config);
+}
+
+// Check that the parser works with a simple configuration that
+// includes empty elements.
+TEST_F(DbAccessParserTest, emptyKeyword) {
+    const char* config[] = {"type", "memfile",
+                            "name", "",
+                            NULL};
+
+    string json_config = toJson(config);
+    ConstElementPtr json_elements = Element::fromJSON(json_config);
+    EXPECT_TRUE(json_elements);
+
+    DbAccessParser parser("lease-database");
+    EXPECT_NO_THROW(parser.build(json_elements));
+    checkAccessString("Valid memfile", parser.getDbAccessParameters(), config);
 }
 
 // Check that the parser works with a valid MySQL configuration
@@ -179,8 +192,7 @@ TEST_F(DbAccessParserTest, validTypeMysql) {
 
     DbAccessParser parser("lease-database");
     EXPECT_NO_THROW(parser.build(json_elements));
-    string dbaccess = parser.getDbAccessString();
-    checkAccessString("Valid mysql", dbaccess, config);
+    checkAccessString("Valid mysql", parser.getDbAccessParameters(), config);
 }
 
 // A missing 'type' keyword should cause an exception to be thrown.
@@ -234,8 +246,7 @@ TEST_F(DbAccessParserTest, factory) {
     // Access the "raw" parser.
     DbAccessParser* dbap = dynamic_cast<DbAccessParser*>(parser.get());
     EXPECT_NE(static_cast<DbAccessParser*>(NULL), dbap);
-    string dbaccess = dbap->getDbAccessString();
-    checkAccessString("Valid mysql", dbaccess, config);
+    checkAccessString("Valid mysql", dbap->getDbAccessParameters(), config);
 }
 
 // Check reconfiguration.  Checks that incremental changes applied to the
@@ -273,12 +284,12 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
     // incremental4 is a compatible change and should cause a transition
     // to config4.
     const char* incremental4[] = {"user",     "them",
-                                  "password", "themagain",
+                                  "password", "",
                                   NULL};
     const char* config4[] = {"type",     "mysql",
                              "host",     "erewhon",
                              "user",     "them",
-                             "password", "themagain",
+                             "password", "",
                              "name",     "keatest",
                              NULL};
 
@@ -291,8 +302,8 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
     EXPECT_TRUE(json_elements);
 
     EXPECT_NO_THROW(parser.build(json_elements));
-    string dbaccess = parser.getDbAccessString();
-    checkAccessString("Initial configuration", dbaccess, config1);
+    checkAccessString("Initial configuration", parser.getDbAccessParameters(),
+                      config1);
 
     // Applying a wholesale change will cause the access string to change
     // to a representation of the new configuration.
@@ -301,8 +312,8 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
     EXPECT_TRUE(json_elements);
 
     EXPECT_NO_THROW(parser.build(json_elements));
-    dbaccess = parser.getDbAccessString();
-    checkAccessString("Subsequent configuration", dbaccess, config2);
+    checkAccessString("Subsequent configuration", parser.getDbAccessParameters(),
+                      config2);
 
     // Applying an incremental change will cause the representation to change
     // incrementally.
@@ -311,8 +322,8 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
     EXPECT_TRUE(json_elements);
 
     EXPECT_NO_THROW(parser.build(json_elements));
-    dbaccess = parser.getDbAccessString();
-    checkAccessString("Incremental configuration", dbaccess, config3);
+    checkAccessString("Incremental configuration", parser.getDbAccessParameters(),
+                      config3);
 
     // Applying the next incremental change should cause an exception to be
     // thrown and there be no change to the access string.
@@ -321,8 +332,8 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
     EXPECT_TRUE(json_elements);
 
     EXPECT_THROW(parser.build(json_elements), BadValue);
-    dbaccess = parser.getDbAccessString();
-    checkAccessString("Incompatible incremental change", dbaccess, config3);
+    checkAccessString("Incompatible incremental change", parser.getDbAccessParameters(),
+                      config3);
 
     // Applying an incremental change will cause the representation to change
     // incrementally.
@@ -331,65 +342,8 @@ TEST_F(DbAccessParserTest, incrementalChanges) {
     EXPECT_TRUE(json_elements);
 
     EXPECT_NO_THROW(parser.build(json_elements));
-    dbaccess = parser.getDbAccessString();
-    checkAccessString("Compatible incremental change", dbaccess, config4);
-}
-
-// Check reconfiguration and that elements set to an empty string are omitted.
-TEST_F(DbAccessParserTest, emptyStringOmission) {
-    const char* config1[] = {"type", "memfile",
-                             NULL};
-
-    // Applying config2 will cause a wholesale change.
-    const char* config2[] = {"type",     "mysql",
-                             "host",     "erewhon",
-                             "user",     "kea",
-                             "password", "keapassword",
-                             "name",     "keatest",
-                             NULL};
-
-    // Applying incremental2 should cause a change to config3.
-    const char* incremental2[] = {"user",     "me",
-                                  "password", "",
-                                  "host",     "",
-                                  NULL};
-
-    const char* config3[] = {"type",     "mysql",
-                             "user",     "me",
-                             "name",     "keatest",
-                             NULL};
-
-    DbAccessParser parser("lease-database");
-
-    // First configuration string should cause a representation of that string
-    // to be held.
-    string json_config = toJson(config1);
-    ConstElementPtr json_elements = Element::fromJSON(json_config);
-    EXPECT_TRUE(json_elements);
-
-    EXPECT_NO_THROW(parser.build(json_elements));
-    string dbaccess = parser.getDbAccessString();
-    checkAccessString("Initial configuration", dbaccess, config1);
-
-    // Applying a wholesale change will cause the access string to change
-    // to a representation of the new configuration.
-    json_config = toJson(config2);
-    json_elements = Element::fromJSON(json_config);
-    EXPECT_TRUE(json_elements);
-
-    EXPECT_NO_THROW(parser.build(json_elements));
-    dbaccess = parser.getDbAccessString();
-    checkAccessString("Subsequent configuration", dbaccess, config2);
-
-    // Applying an incremental change will cause the representation to change
-    // incrementally.
-    json_config = toJson(incremental2);
-    json_elements = Element::fromJSON(json_config);
-    EXPECT_TRUE(json_elements);
-
-    EXPECT_NO_THROW(parser.build(json_elements));
-    dbaccess = parser.getDbAccessString();
-    checkAccessString("Incremental configuration", dbaccess, config3);
+    checkAccessString("Compatible incremental change", parser.getDbAccessParameters(),
+                      config4);
 }
 
 };  // Anonymous namespace