[5098] Improved checks and unit tests

src/bin/dhcp4/json_config_parser.cc

@@ -647,7 +647,9 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
 
             if (config_pair.first == "client-classes") {
                 ClientClassDefListParser parser;
-                parser.parse(config_pair.second, AF_INET);
+                ClientClassDictionaryPtr dictionary =
+                    parser.parse(config_pair.second, AF_INET);
+                CfgMgr::instance().getStagingCfg()->setClientClassDictionary(dictionary);
                 continue;
             }
 

src/bin/dhcp6/json_config_parser.cc

@@ -923,11 +923,13 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
                 continue;
             }
 
-	    if (config_pair.first =="client-classes") {
-		ClientClassDefListParser parser;
-		parser.parse(config_pair.second, AF_INET6);
-		continue;
-	    }
+            if (config_pair.first =="client-classes") {
+                ClientClassDefListParser parser;
+                ClientClassDictionaryPtr dictionary =
+                    parser.parse(config_pair.second, AF_INET6);
+                CfgMgr::instance().getStagingCfg()->setClientClassDictionary(dictionary);
+                continue;
+            }
 
             ParserPtr parser(createGlobal6DhcpConfigParser(config_pair.first,
                                                            config_pair.second));

src/lib/dhcpsrv/parsers/client_class_def_parser.cc

@@ -28,12 +28,10 @@ namespace dhcp {
 
 // ********************** ExpressionParser ****************************
 
-ExpressionParser::ExpressionParser(ExpressionPtr& expression)
-    : local_expression_(ExpressionPtr()), expression_(expression) {
-}
-
 void
-ExpressionParser::parse(ConstElementPtr expression_cfg, uint16_t family) {
+ExpressionParser::parse(ExpressionPtr& expression,
+                        ConstElementPtr expression_cfg,
+                        uint16_t family) {
     if (expression_cfg->getType() != Element::string) {
         isc_throw(DhcpConfigError, "expression ["
             << expression_cfg->str() << "] must be a string, at ("
@@ -47,8 +45,8 @@ ExpressionParser::parse(ConstElementPtr expression_cfg, uint16_t family) {
     try {
         EvalContext eval_ctx(family == AF_INET ? Option::V4 : Option::V6);
         eval_ctx.parseString(value);
-        local_expression_.reset(new Expression());
-        *local_expression_ = eval_ctx.expression;
+        expression.reset(new Expression());
+        *expression = eval_ctx.expression;
     } catch (const std::exception& ex) {
         // Append position if there is a failure.
         isc_throw(DhcpConfigError,
@@ -56,27 +54,22 @@ ExpressionParser::parse(ConstElementPtr expression_cfg, uint16_t family) {
                   <<  "] error: " << ex.what() << " at ("
                   <<  expression_cfg->getPosition() << ")");
     }
-
-    // Success so commit.
-    expression_ = local_expression_;
 }
 
 // ********************** ClientClassDefParser ****************************
 
-ClientClassDefParser::ClientClassDefParser(ClientClassDictionaryPtr& class_dictionary)
-    : match_expr_(ExpressionPtr()),
-      options_(new CfgOption()),
-      class_dictionary_(class_dictionary) {
-}
-
 void
-ClientClassDefParser::parse(ConstElementPtr class_def_cfg, uint16_t family) {
+ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary,
+                            ConstElementPtr class_def_cfg,
+                            uint16_t family) {
 
     try {
         std::string name;
         std::string next_server_txt = "0.0.0.0";
         std::string sname;
         std::string filename;
+        ExpressionPtr match_expr;
+        CfgOptionPtr options(new CfgOption());
 
         // Parse the elements that make up the client class definition.
         BOOST_FOREACH(ConfigPair param, class_def_cfg->mapValue()) {
@@ -87,12 +80,12 @@ ClientClassDefParser::parse(ConstElementPtr class_def_cfg, uint16_t family) {
                 name = value->stringValue();
 
             } else if (entry == "test") {
-                ExpressionParser parser(match_expr_);
-                parser.parse(value, family);
+                ExpressionParser parser;
+                parser.parse(match_expr, value, family);
                 
             } else if (entry == "option-data") {
                 OptionDataListParser opts_parser(family);
-                opts_parser.parse(options_, value);
+                opts_parser.parse(options, value);
 
             } else if (entry == "next-server") {
                 next_server_txt = value->stringValue();
@@ -109,7 +102,11 @@ ClientClassDefParser::parse(ConstElementPtr class_def_cfg, uint16_t family) {
             }
         }
 
-        // Make name mandatory?
+        // name is now mandatory
+        if (name.empty()) {
+            isc_throw(DhcpConfigError,
+                      "not empty parameter 'name' is required");
+        }
 
         // Let's parse the next-server field
         IOAddress next_server("0.0.0.0");
@@ -145,8 +142,8 @@ ClientClassDefParser::parse(ConstElementPtr class_def_cfg, uint16_t family) {
         }
 
         // Add the client class definition
-        class_dictionary_->addClass(name, match_expr_, options_, next_server,
-                                    sname, filename);
+        class_dictionary->addClass(name, match_expr, options, next_server,
+                                   sname, filename);
     } catch (const std::exception& ex) {
         isc_throw(DhcpConfigError, ex.what()
                   << " (" << class_def_cfg->getPosition() << ")");
@@ -155,20 +152,16 @@ ClientClassDefParser::parse(ConstElementPtr class_def_cfg, uint16_t family) {
 
 // ****************** ClientClassDefListParser ************************
 
-ClientClassDefListParser::ClientClassDefListParser()
-    : local_dictionary_(new ClientClassDictionary()) {
-}
-
-void
+ClientClassDictionaryPtr
 ClientClassDefListParser::parse(ConstElementPtr client_class_def_list,
                                 uint16_t family) {
+    ClientClassDictionaryPtr dictionary(new ClientClassDictionary());
     BOOST_FOREACH(ConstElementPtr client_class_def,
                   client_class_def_list->listValue()) {
-        ClientClassDefParser parser(local_dictionary_);
-        parser.parse(client_class_def, family);
+        ClientClassDefParser parser;
+        parser.parse(dictionary, client_class_def, family);
     }
-    // Success so commit
-    CfgMgr::instance().getStagingCfg()->setClientClassDictionary(local_dictionary_);
+    return (dictionary);
 }
 
 } // end of namespace isc::dhcp

src/lib/dhcpsrv/parsers/client_class_def_parser.h

@@ -53,25 +53,16 @@ namespace dhcp {
 /// stored into the ExpressionPtr reference passed into the constructor.
 class ExpressionParser : public isc::data::SimpleParser {
 public:
-    /// @brief Constructor.
-    ///
-    /// @param expression variable in which to store the new expression
-    ExpressionParser(ExpressionPtr& expression);
 
     /// @brief Parses an expression configuration element into an Expression
     ///
+    /// @param expression variable in which to store the new expression
     /// @param expression_cfg the configuration entry to be parsed.
     /// @param family the address family of the expression.
     ///
     /// @throw DhcpConfigError if parsing was unsuccessful.
-    void parse(isc::data::ConstElementPtr expression_cfg, uint16_t family);
-
-private:
-    /// @brief Local storage for the parsed expression
-    ExpressionPtr local_expression_;
-
-    /// @brief Storage into which the parsed expression should be committed
-    ExpressionPtr& expression_;
+    void parse(ExpressionPtr& expression,
+               isc::data::ConstElementPtr expression_cfg, uint16_t family);
 };
 
 /// @brief Parser for a single client class definition.
@@ -79,31 +70,18 @@ private:
 /// This parser creates an instance of a client class definition.
 class ClientClassDefParser : public isc::data::SimpleParser {
 public:
-    /// @brief Constructor.
-    ///
-    /// @param class_dictionary dictionary into which the class should be added
-    ClientClassDefParser(ClientClassDictionaryPtr& class_dictionary);
 
     /// @brief Parses an entry that describes single client class definition.
     ///
     /// Attempts to add the new class directly into the given dictionary.
     /// This done here to detect duplicate classes prior to commit().
+    /// @param class_dictionary dictionary into which the class should be added
     /// @param client_class_def a configuration entry to be parsed.
     /// @param family the address family of the client class.
     ///
     /// @throw DhcpConfigError if parsing was unsuccessful.
-    void parse(isc::data::ConstElementPtr client_class_def, uint16_t family);
-
-private:
-
-    /// @brief Storage for the class match expression
-    ExpressionPtr match_expr_;
-
-    /// @brief Storage for the class options
-    CfgOptionPtr options_;
-
-    /// @brief Dictionary to which the new class should be added
-    ClientClassDictionaryPtr& class_dictionary_;
+    void parse(ClientClassDictionaryPtr& class_dictionary,
+               isc::data::ConstElementPtr client_class_def, uint16_t family);
 };
 
 /// @brief Defines a pointer to a ClientClassDefParser
@@ -118,26 +96,19 @@ typedef boost::shared_ptr<ClientClassDefParser> ClientClassDefParserPtr;
 class ClientClassDefListParser : public isc::data::SimpleParser {
 public:
 
-    /// @brief Constructor.
-    ClientClassDefListParser();
-
     /// @brief Parse configuration entries.
     ///
     /// This function parses configuration entries, creates instances
-    /// of client class definitions and tries to adds them to the a
-    /// local dictionary. At the end class definitions are committed
-    /// to CfgMgr's global storage.
+    /// of client class definitions and tries to adds them to the
+    /// local dictionary. At the end the dictionary is returned.
     ///
     /// @param class_def_list pointer to an element that holds entries
     /// for client class definitions.
     /// @param family the address family of the client class definitions.
+    /// @return a pointer to the filled dictionary
     /// @throw DhcpConfigError if configuration parsing fails.
-    void parse(isc::data::ConstElementPtr class_def_list, uint16_t family);
-
-    /// @brief Local class dictionary to store classes as they are being parsed
-    ///
-    /// Left public for easier unit testing.
-    ClientClassDictionaryPtr local_dictionary_;
+    ClientClassDictionaryPtr
+    parse(isc::data::ConstElementPtr class_def_list, uint16_t family);
 };
 
 } // end of namespace isc::dhcp

src/lib/dhcpsrv/srv_config.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -364,7 +364,7 @@ public:
         class_dictionary_ = dictionary;
     }
 
-    /// @brief Copies the currnet configuration to a new configuration.
+    /// @brief Copies the current configuration to a new configuration.
     ///
     /// This method copies the parameters stored in the configuration to
     /// an object passed as parameter. The configuration sequence is not

src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc

@@ -51,13 +51,13 @@ protected:
                              const std::string& expression,
                              const std::string& option_string) {
         ExpressionPtr parsed_expr;
-        ExpressionParser parser(parsed_expr);
+        ExpressionParser parser;
 
         // Turn config into elements.  This may emit exceptions.
         ElementPtr config_element = Element::fromJSON(expression);
 
         // Expression should parse.
-        ASSERT_NO_THROW(parser.parse(config_element, family));
+        ASSERT_NO_THROW(parser.parse(parsed_expr, config_element, family));
 
         // Parsed expression should exist.
         ASSERT_TRUE(parsed_expr);
@@ -98,15 +98,15 @@ protected:
     /// or by the parsing itself are not caught
     ClientClassDefPtr parseClientClassDef(const std::string& config,
                                           uint16_t family) {
-        // Create local dicitonary to which the parser add the class.
+        // Create local dictionary to which the parser add the class.
         ClientClassDictionaryPtr dictionary(new ClientClassDictionary());
 
         // Turn config into elements.  This may emit exceptions.
         ElementPtr config_element = Element::fromJSON(config);
 
         // Parse the configuration. This may emit exceptions.
-        ClientClassDefParser parser(dictionary);
-        parser.parse(config_element, family);
+        ClientClassDefParser parser;
+        parser.parse(dictionary, config_element, family);
 
         // If we didn't throw, then return the first and only class
         ClientClassDefMapPtr classes = dictionary->getClasses();
@@ -144,10 +144,7 @@ protected:
 
         // Parse the configuration. This may emit exceptions.
         ClientClassDefListParser parser;
-        parser.parse(config_element, family);
-
-        // Return the parser's local dicationary
-        return (parser.local_dictionary_);
+        return (parser.parse(config_element, family));
     }
 };
 
@@ -223,10 +220,11 @@ TEST_F(ExpressionParserTest, invalidExpressionElement) {
 
     // Create the parser.
     ExpressionPtr parsed_expr;
-    ExpressionParser parser(parsed_expr);
+    ExpressionParser parser;
 
     // Expression parsing should fail.
-    ASSERT_THROW(parser.parse(config_element, AF_INET), DhcpConfigError);
+    ASSERT_THROW(parser.parse(parsed_expr, config_element, AF_INET6),
+                 DhcpConfigError);
 }
 
 // Verifies that given an invalid expression with a syntax error,
@@ -241,10 +239,25 @@ TEST_F(ExpressionParserTest, expressionSyntaxError) {
 
     // Create the parser.
     ExpressionPtr parsed_expr;
-    ExpressionParser parser(parsed_expr);
+    ExpressionParser parser;
 
     // Expression parsing should fail.
-    ASSERT_THROW(parser.parse(config_element, AF_INET), DhcpConfigError);
+    ASSERT_THROW(parser.parse(parsed_expr, config_element, AF_INET),
+                 DhcpConfigError);
+}
+
+// Verifies that the name parameter is required and must not be empty
+TEST_F(ExpressionParserTest, nameEmpty) {
+    std::string cfg_txt = "{ \"name\": \"\" }";
+    ElementPtr config_element = Element::fromJSON(cfg_txt);
+
+    // Create the parser.
+    ExpressionPtr parsed_expr;
+    ExpressionParser parser;
+
+    // Expression parsing should fail.
+    ASSERT_THROW(parser.parse(parsed_expr, config_element, AF_INET6),
+                 DhcpConfigError);
 }
 
 // Verifies you can create a class with only a name
@@ -279,6 +292,7 @@ TEST_F(ClientClassDefParserTest, nameOnlyValid) {
 
 // Verifies you can create a class with a name, expression,
 // but no options.
+// @todo same with AF_INET6
 TEST_F(ClientClassDefParserTest, nameAndExpressionClass) {
 
     std::string cfg_text =
@@ -320,6 +334,7 @@ TEST_F(ClientClassDefParserTest, nameAndExpressionClass) {
 
 // Verifies you can create a class with a name and options,
 // but no expression.
+// @todo same with AF_INET6
 TEST_F(ClientClassDefParserTest, nameAndOptionsClass) {
 
     std::string cfg_text =
@@ -355,6 +370,7 @@ TEST_F(ClientClassDefParserTest, nameAndOptionsClass) {
 
 // Verifies you can create a class with a name, expression,
 // and options.
+// @todo same with AF_INET6
 TEST_F(ClientClassDefParserTest, basicValidClass) {
 
     std::string cfg_text =
@@ -466,7 +482,7 @@ TEST_F(ClientClassDefParserTest, invalidExpression) {
         "} \n";
 
     ClientClassDefPtr cclass;
-    ASSERT_THROW(cclass = parseClientClassDef(cfg_text, AF_INET),
+    ASSERT_THROW(cclass = parseClientClassDef(cfg_text, AF_INET6),
                  DhcpConfigError);
 }
 
@@ -503,7 +519,7 @@ TEST_F(ClientClassDefListParserTest, simpleValidList) {
 
     // Parsing the list should succeed.
     ClientClassDictionaryPtr dictionary;
-    ASSERT_NO_THROW(dictionary = parseClientClassDefList(cfg_text, AF_INET));
+    ASSERT_NO_THROW(dictionary = parseClientClassDefList(cfg_text, AF_INET6));
     ASSERT_TRUE(dictionary);
 
     // We should have three classes in the dictionary.
@@ -526,13 +542,6 @@ TEST_F(ClientClassDefListParserTest, simpleValidList) {
     // For good measure, make sure we can't find a non-existant class.
     ASSERT_NO_THROW(cclass = dictionary->findClass("bogus"));
     EXPECT_FALSE(cclass);
-
-    // Verify that the dictionary was pushed to the CfgMgr's staging config.
-    SrvConfigPtr staging = CfgMgr::instance().getStagingCfg();
-    ASSERT_TRUE(staging);
-    ClientClassDictionaryPtr staged_dictionary = staging->getClientClassDictionary();
-    ASSERT_TRUE(staged_dictionary);
-    EXPECT_TRUE(*staged_dictionary == *dictionary);
 }
 
 // Verifies that class list containing a duplicate class entries, fails
@@ -568,12 +577,13 @@ TEST_F(ClientClassDefListParserTest, invalidClass) {
         "] \n";
 
     ClientClassDictionaryPtr dictionary;
-    ASSERT_THROW(dictionary = parseClientClassDefList(cfg_text, AF_INET),
+    ASSERT_THROW(dictionary = parseClientClassDefList(cfg_text, AF_INET6),
                  DhcpConfigError);
 }
 
 // Test verifies that without any class specified, the fixed fields have their
 // default, empty value.
+// @todo same with AF_INET6
 TEST_F(ClientClassDefParserTest, noFixedFields) {
 
     std::string cfg_text =