[3121] Moved validation logic to build method for DdnsDomain parsing

Modified code for domain parsing.

src/bin/d2/d2_config.cc

@@ -302,6 +302,10 @@ build(isc::data::ConstElementPtr key_list){
         parser->build(key_config);
         parsers_.push_back(parser);
     }
+
+    // Now that we know we have a valid list, commit that list to the
+    // area given to us during construction (i.e. to the d2 context).
+    *keys_ = *local_keys_;
 }
 
 void
@@ -311,10 +315,6 @@ TSIGKeyInfoListParser::commit() {
     BOOST_FOREACH(isc::dhcp::ParserPtr parser, parsers_) {
         parser->commit();
     }
-
-    // Now that we know we have a valid list, commit that list to the
-    // area given to us during construction (i.e. to the d2 context).
-    *keys_ = *local_keys_;
 }
 
 // *********************** DnsServerInfoParser  *************************
@@ -488,34 +488,8 @@ DdnsDomainParser::build(isc::data::ConstElementPtr domain_config) {
         parser->build(config_pair.second);
         parser->commit();
     }
-}
-
-isc::dhcp::ParserPtr
-DdnsDomainParser::createConfigParser(const std::string& config_id) {
-    DhcpConfigParser* parser = NULL;
-    // Based on the configuration id of the element, create the appropriate
-    // parser. Scalars are set to use the parser's local scalar storage.
-    if ((config_id == "name")  ||
-        (config_id == "key_name")) {
-        parser = new isc::dhcp::StringParser(config_id,
-                                             local_scalars_.getStringStorage());
-    } else if (config_id == "dns_servers") {
-       // Server list parser is given in our local server storage. It will pass
-       // this down to its server parsers and is where they will write their
-       // server instances upon commit.
-       parser = new DnsServerInfoListParser(config_id, local_servers_);
-    } else {
-       isc_throw(NotImplemented,
-                "parser error: DdnsDomain parameter not supported: "
-                << config_id);
-    }
-
-    // Return the new domain parser instance.
-    return (isc::dhcp::ParserPtr(parser));
-}
 
-void
-DdnsDomainParser::commit() {
+    // Now construct the domain.
     std::string name;
     std::string key_name;
 
@@ -549,7 +523,35 @@ DdnsDomainParser::commit() {
     DdnsDomainPtr domain(new DdnsDomain(name, key_name, local_servers_));
 
     // Add the new domain to the domain storage.
-    (*domains_)[name]=domain;
+    (*domains_)[name] = domain;
+}
+
+isc::dhcp::ParserPtr
+DdnsDomainParser::createConfigParser(const std::string& config_id) {
+    DhcpConfigParser* parser = NULL;
+    // Based on the configuration id of the element, create the appropriate
+    // parser. Scalars are set to use the parser's local scalar storage.
+    if ((config_id == "name")  ||
+        (config_id == "key_name")) {
+        parser = new isc::dhcp::StringParser(config_id,
+                                             local_scalars_.getStringStorage());
+    } else if (config_id == "dns_servers") {
+       // Server list parser is given in our local server storage. It will pass
+       // this down to its server parsers and is where they will write their
+       // server instances upon commit.
+       parser = new DnsServerInfoListParser(config_id, local_servers_);
+    } else {
+       isc_throw(NotImplemented,
+                "parser error: DdnsDomain parameter not supported: "
+                << config_id);
+    }
+
+    // Return the new domain parser instance.
+    return (isc::dhcp::ParserPtr(parser));
+}
+
+void
+DdnsDomainParser::commit() {
 }
 
 // *********************** DdnsDomainListParser  *************************
@@ -622,6 +624,9 @@ DdnsDomainListMgrParser::build(isc::data::ConstElementPtr domain_config) {
         parser->build(config_pair.second);
         parser->commit();
     }
+
+    // Add the new domain to the domain storage.
+    mgr_->setDomains(local_domains_);
 }
 
 isc::dhcp::ParserPtr
@@ -643,8 +648,6 @@ DdnsDomainListMgrParser::createConfigParser(const std::string& config_id) {
 
 void
 DdnsDomainListMgrParser::commit() {
-    // Add the new domain to the domain storage.
-    mgr_->setDomains(local_domains_);
 }
 
 

src/bin/d2/d2_config.h

@@ -611,6 +611,9 @@ public:
     virtual void build(isc::data::ConstElementPtr key_list_config);
 
     /// @brief Commits the list of TSIG keys
+    ///
+    /// Iterates over the internal list of TSIGKeyInfoParsers, invoking
+    /// commit on each one.
     virtual void commit();
 
 private:
@@ -723,6 +726,9 @@ public:
     virtual void build(isc::data::ConstElementPtr server_list_config);
 
     /// @brief Commits the list of DnsServerInfos
+    ///
+    /// Iterates over the internal list of DdnsServerInfoParsers, invoking
+    /// commit on each one.
     virtual void commit();
 
 private:
@@ -759,8 +765,10 @@ public:
     virtual ~DdnsDomainParser();
 
     /// @brief Performs the actual parsing of the given  "ddns_domain" element.
-    /// The results of the parsing are retained internally for use during
-    /// commit.
+    ///
+    /// Parses a configuration for the elements needed to instantiate a
+    /// DdnsDomain, validates those entries, creates a DdnsDomain instance
+    /// then attempts to add it to a list of domains.
     ///
     /// @param domain_config is the "ddns_domain" configuration to parse
     virtual void build(isc::data::ConstElementPtr domain_config);
@@ -779,8 +787,7 @@ public:
     virtual isc::dhcp::ParserPtr createConfigParser(const std::string&
                                                     config_id);
 
-    /// @brief Instantiates a DdnsDomain from internal data values
-    /// saves it to the storage area pointed to by domains_.
+    /// @brief commits the configured DdnsDomain
     virtual void commit();
 
 private:
@@ -832,7 +839,7 @@ public:
 
     /// @brief Performs the actual parsing of the given list "ddns_domain"
     /// elements.
-    /// It iterates over each server entry in the list:
+    /// It iterates over each domain entry in the list:
     ///   1. Instantiate a DdnsDomainParser for the entry
     ///   2. Pass the element configuration to the parser's build method
     ///   3. Add the parser instance to local storage
@@ -844,10 +851,10 @@ public:
     /// parse.
     virtual void build(isc::data::ConstElementPtr domain_list_config);
 
-    /// @brief Iterates over the internal list of DdnsDomainParsers, invoking
-    /// commit on each.  This causes each parser to instantiate a DdnsDomain
-    /// from its internal data values and add that domain instance to the
-    /// storage area, domains_.
+    /// @brief Commits the list of DdnsDomains
+    ///
+    /// Iterates over the internal list of DdnsDomainParsers, invoking
+    /// commit on each one.
     virtual void commit();
 
 private:
@@ -892,8 +899,10 @@ public:
     virtual ~DdnsDomainListMgrParser();
 
     /// @brief Performs the actual parsing of the given manager element.
-    /// The results of the parsing are retained internally for use during
-    /// commit.
+    ///
+    /// Parses a configuration for the elements needed to instantiate a
+    /// DdnsDomainListMgr, validates those entries, then creates a
+    /// DdnsDomainListMgr.
     ///
     /// @param mgr_config is the manager configuration to parse
     virtual void build(isc::data::ConstElementPtr mgr_config);
@@ -910,8 +919,7 @@ public:
     virtual isc::dhcp::ParserPtr createConfigParser(const std::string&
                                                     config_id);
 
-    /// @brief Populates the DdnsDomainListMgr from internal data values
-    /// set during parsing.
+    /// @brief Commits the configured DdsnDomainListMgr
     virtual void commit();
 
 private:

src/bin/d2/tests/d2_cfg_mgr_unittests.cc

@@ -609,9 +609,8 @@ TEST_F(DdnsDomainTest, invalidDdnsDomainEntry) {
                          "    \"port\": 300 } ] } ";
     ASSERT_TRUE(fromJSON(config));
 
-    // Verify that the domain configuration builds but commit fails.
-    ASSERT_NO_THROW(parser_->build(config_set_));
-    ASSERT_THROW(parser_->commit(), isc::dhcp::DhcpConfigError);
+    // Verify that the domain configuration builds fails.
+    EXPECT_THROW(parser_->build(config_set_), isc::dhcp::DhcpConfigError);
 
     // Create a domain configuration with an empty server list.
     config = "{ \"name\": \"tmark.org\" , "
@@ -621,7 +620,7 @@ TEST_F(DdnsDomainTest, invalidDdnsDomainEntry) {
     ASSERT_TRUE(fromJSON(config));
 
     // Verify that the domain configuration build fails.
-    ASSERT_THROW(parser_->build(config_set_), D2CfgError);
+    EXPECT_THROW(parser_->build(config_set_), D2CfgError);
 
     // Create a domain configuration with a mal-formed server entry.
     config = "{ \"name\": \"tmark.org\" , "
@@ -632,7 +631,7 @@ TEST_F(DdnsDomainTest, invalidDdnsDomainEntry) {
     ASSERT_TRUE(fromJSON(config));
 
     // Verify that the domain configuration build fails.
-    ASSERT_THROW(parser_->build(config_set_), isc::BadValue);
+    EXPECT_THROW(parser_->build(config_set_), isc::BadValue);
 
     // Create a domain configuration without an defined key name
     config = "{ \"name\": \"tmark.org\" , "
@@ -642,9 +641,8 @@ TEST_F(DdnsDomainTest, invalidDdnsDomainEntry) {
              "    \"port\": 300 } ] } ";
     ASSERT_TRUE(fromJSON(config));
 
-    // Verify that the domain configuration build succeeds but commit fails.
-    ASSERT_NO_THROW(parser_->build(config_set_));
-    ASSERT_THROW(parser_->commit(), D2CfgError);
+    // Verify that the domain configuration build fails.
+    EXPECT_THROW(parser_->build(config_set_), D2CfgError);
 }
 
 /// @brief Verifies the basics of parsing DdnsDomains.
@@ -839,9 +837,8 @@ TEST_F(DdnsDomainTest, duplicateDomain) {
     ASSERT_NO_THROW(list_parser.reset(
                     new DdnsDomainListParser("test", domains_, keys_)));
 
-    // Verify that the parse build succeeds but the commit fails.
-    ASSERT_NO_THROW(list_parser->build(config_set_));
-    ASSERT_THROW(list_parser->commit(), D2CfgError);
+    // Verify that the parse build fails.
+    EXPECT_THROW(list_parser->build(config_set_), D2CfgError);
 }
 
 /// @brief Tests construction of D2CfgMgr