[4096] Integrated use of Eval expression parsing

src/lib/dhcpsrv/client_class_def.cc
    - updated comment on empty expressions in ctor,
    - cleaned up whitespace

src/lib/dhcpsrv/parsers/client_class_def_parser.cc
    - ExpressionParser::build() - integrated use of Eval
    parsing in ExpressionParser
    - cleaned up whitespace

src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
    Updated tests to verify that expressions are actually
    parsed and function.

src/lib/dhcpsrv/client_class_def.cc

@@ -29,7 +29,9 @@ ClientClassDef::ClientClassDef(const std::string& name,
     if (name_.empty()) {
         isc_throw(BadValue, "ClientClassDef name cannot be empty");
     }
-    // @todo Does it make sense for a class to NOT have match expression?
+
+    // We permit an empty expression for now.  This will likely be useful
+    // for automatic classes such as vendor class.
 
     // For classes without options, make sure we have an empty collection
     if (!cfg_option_) {
@@ -37,7 +39,7 @@ ClientClassDef::ClientClassDef(const std::string& name,
     }
 }
 
-ClientClassDef::ClientClassDef(const ClientClassDef& rhs) 
+ClientClassDef::ClientClassDef(const ClientClassDef& rhs)
     : name_(rhs.name_), match_expr_(ExpressionPtr()),
       cfg_option_(new CfgOption()) {
 
@@ -88,10 +90,10 @@ bool
 ClientClassDef::equals(const ClientClassDef& other) const {
     return ((name_ == other.name_) &&
         ((!match_expr_ && !other.match_expr_) ||
-        (match_expr_ && other.match_expr_ && 
+        (match_expr_ && other.match_expr_ &&
          (*match_expr_ == *(other.match_expr_)))) &&
         ((!cfg_option_ && !other.cfg_option_) ||
-        (cfg_option_ && other.cfg_option_ && 
+        (cfg_option_ && other.cfg_option_ &&
          (*cfg_option_ == *other.cfg_option_))));
 }
 
@@ -168,9 +170,9 @@ ClientClassDictionary::equals(const ClientClassDictionary& other) const {
 
     ClientClassDefMap::iterator this_class = classes_->begin();
     ClientClassDefMap::iterator other_class = other.classes_->begin();
-    while (this_class != classes_->end() && 
+    while (this_class != classes_->end() &&
            other_class != other.classes_->end()) {
-        if (!(*this_class).second || !(*other_class).second || 
+        if (!(*this_class).second || !(*other_class).second ||
             (*(*this_class).second) != (*(*other_class).second)) {
                 return false;
         }

src/lib/dhcpsrv/parsers/client_class_def_parser.cc

@@ -16,6 +16,8 @@
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/client_class_def.h>
 #include <dhcpsrv/parsers/client_class_def_parser.h>
+#include <eval/eval_context.h>
+
 #include <boost/foreach.hpp>
 
 using namespace isc::data;
@@ -31,27 +33,33 @@ namespace dhcp {
 
 ExpressionParser::ExpressionParser(const std::string&,
     ExpressionPtr& expression, ParserContextPtr global_context)
-    : local_expression_(ExpressionPtr()), expression_(expression), 
+    : local_expression_(ExpressionPtr()), expression_(expression),
       global_context_(global_context) {
 }
 
 void
 ExpressionParser::build(ConstElementPtr expression_cfg) {
-    // Here is where we would call bison parser with our string
-    // For now we'll just create an expression with a string token
-    // containing the expression text
+    if (expression_cfg->getType() != Element::string) {
+        isc_throw(DhcpConfigError, "expression ["
+            << expression_cfg->str() << "] must be a string, at ("
+            << expression_cfg->getPosition() << ")");
+    }
+
+    // Get the expression's text via getValue() as the text returned
+    // by str() enclosed in quotes.
+    std::string value;
+    expression_cfg->getValue(value);
     try {
-        if (expression_cfg->getType() != Element::string) {
-            isc_throw(TypeError, "expression value must be a string");
-        } 
-        std::string expression_text = expression_cfg->str();
-        TokenPtr dummy(new TokenString(expression_text));
+        EvalContext eval_ctx;
+        eval_ctx.parseString(value);
         local_expression_.reset(new Expression());
-        local_expression_->push_back(dummy);
+        *local_expression_ = eval_ctx.expression;
     } catch (const std::exception& ex) {
         // Append position if there is a failure.
-        isc_throw(DhcpConfigError, ex.what() << " ("
-                  << expression_cfg->getPosition() << ")");
+        isc_throw(DhcpConfigError,
+                  "expression: [" << value
+                  <<  "] error: " << ex.what() << " at ("
+                  <<  expression_cfg->getPosition() << ")");
     }
 }
 
@@ -121,9 +129,9 @@ ClientClassDefParser::build(ConstElementPtr class_def_cfg) {
 // ****************** ClientClassDefListParser ************************
 
 ClientClassDefListParser::ClientClassDefListParser(const std::string&,
-                                                   ParserContextPtr 
+                                                   ParserContextPtr
                                                    global_context)
-    : local_dictionary_(new ClientClassDictionary()), 
+    : local_dictionary_(new ClientClassDictionary()),
       global_context_(global_context) {
 }
 
@@ -135,10 +143,10 @@ ClientClassDefListParser::build(ConstElementPtr client_class_def_list) {
                   << client_class_def_list->getPosition() << ")");
     }
 
-    BOOST_FOREACH(ConstElementPtr client_class_def, 
+    BOOST_FOREACH(ConstElementPtr client_class_def,
                   client_class_def_list->listValue()) {
         boost::shared_ptr<ClientClassDefParser>
-            parser(new ClientClassDefParser("client-class-def", 
+            parser(new ClientClassDefParser("client-class-def",
                                             local_dictionary_,
                                             global_context_));
         parser->build(client_class_def);

src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc

@@ -15,8 +15,10 @@
 #include <config.h>
 
 #include <cc/data.h>
+#include <dhcp/option_string.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/parsers/client_class_def_parser.h>
+#include <eval/evaluate.h>
 #include <gtest/gtest.h>
 #include <sstream>
 #include <stdint.h>
@@ -109,50 +111,78 @@ protected:
     }
 };
 
-
-// Verifies basic operation of an ExpressionParser. Until we tie
-// this into the actual Bison parsing there's not much to test.
-TEST(ExpressionParserTest, simpleStringExpression) {
+// Verifies that given a valid expression, the ExpressionParser
+// produces an Expression which can be evaluated against a v4 packet.
+TEST(ExpressionParserTest, validExpression4) {
     ParserContextPtr context(new ParserContext(Option::V4));
     ExpressionParserPtr parser;
     ExpressionPtr parsed_expr;
 
     // Turn config into elements.  This may emit exceptions.
-    std::string cfg_txt = "\"astring\"";
+    std::string cfg_txt = "\"option[100] == 'hundred4'\"";
     ElementPtr config_element = Element::fromJSON(cfg_txt);
 
     // Create the parser.
     ASSERT_NO_THROW(parser.reset(new ExpressionParser("", parsed_expr,
                                                       context)));
     // Expression should parse and commit.
+    (parser->build(config_element));
     ASSERT_NO_THROW(parser->build(config_element));
     ASSERT_NO_THROW(parser->commit());
 
     // Parsed expression should exist.
     ASSERT_TRUE(parsed_expr);
 
-    // Evaluate it. For now the result will be the
-    // expression string as dummy ExpressionParser
-    // just makes an expression of one TokenString
-    // containing the expression string itself.
-    ValueStack vstack;
-    Pkt4Ptr pkt4(new Pkt4(DHCPDISCOVER, 12345));
-    (*parsed_expr)[0]->evaluate(*pkt4, vstack);
-    EXPECT_EQ(vstack.top(), "\"astring\"");
+    // Build a packet that will fail evaluation.
+    Pkt4Ptr pkt4(new Pkt4(DHCPDISCOVER, 123));
+    EXPECT_FALSE(evaluate(*parsed_expr, *pkt4));
+
+    // Now add the option so it will pass.
+    OptionPtr opt(new OptionString(Option::V4, 100, "hundred4"));
+    pkt4->addOption(opt);
+    EXPECT_TRUE(evaluate(*parsed_expr, *pkt4));
+}
+
+// Verifies that given a valid expression, the ExpressionParser
+// produces an Expression which can be evaluated against a v6 packet.
+TEST(ExpressionParserTest, validExpression6) {
+    ParserContextPtr context(new ParserContext(Option::V6));
+    ExpressionParserPtr parser;
+    ExpressionPtr parsed_expr;
+
+    // Turn config into elements.  This may emit exceptions.
+    std::string cfg_txt = "\"option[100] == 'hundred6'\"";
+    ElementPtr config_element = Element::fromJSON(cfg_txt);
+
+    // Create the parser.
+    ASSERT_NO_THROW(parser.reset(new ExpressionParser("", parsed_expr,
+                                                      context)));
+    // Expression should parse and commit.
+    (parser->build(config_element));
+    ASSERT_NO_THROW(parser->build(config_element));
+    ASSERT_NO_THROW(parser->commit());
+
+    // Parsed expression should exist.
+    ASSERT_TRUE(parsed_expr);
+
+    // Build a packet that will fail evaluation.
+    Pkt6Ptr pkt6(new Pkt6(DHCPDISCOVER, 123));
+    EXPECT_FALSE(evaluate(*parsed_expr, *pkt6));
+
+    // Now add the option so it will pass.
+    OptionPtr opt(new OptionString(Option::V6, 100, "hundred6"));
+    pkt6->addOption(opt);
+    EXPECT_TRUE(evaluate(*parsed_expr, *pkt6));
 }
 
-// Verifies that given an invalid expression, the Expression parser
-// will throw a DhdpConfigError.  Note this is not intended to be
-// an exhaustive test or expression syntax.  It is simply to ensure
-// that if the parser fails, it does so properly.  For now, since
-// our parser is a dummy parser which only checks that it's given
-// Element::string so send it an integer.
-TEST(ExpressionParserTest, invalidExpression) {
+
+// Verifies that an the ExpressionParser only accepts StringElements.
+TEST(ExpressionParserTest, invalidExpressionElement) {
     ParserContextPtr context(new ParserContext(Option::V4));
     ExpressionParserPtr parser;
     ExpressionPtr parsed_expr;
 
-    // Turn config into elements.
+    // This will create an integer element should fail.
     std::string cfg_txt = "777";
     ElementPtr config_element = Element::fromJSON(cfg_txt);
 
@@ -163,6 +193,27 @@ TEST(ExpressionParserTest, invalidExpression) {
     ASSERT_THROW(parser->build(config_element), DhcpConfigError);
 }
 
+// Verifies that given an invalid expression with a syntax error,
+// the Expression parser will throw a DhdpConfigError.  Note this
+// is not intended to be an exhaustive test or expression syntax.
+// It is simply to ensure that if the parser fails, it does so
+// Properly.
+TEST(ExpressionParserTest, expressionSyntaxError) {
+    ParserContextPtr context(new ParserContext(Option::V4));
+    ExpressionParserPtr parser;
+    ExpressionPtr parsed_expr;
+
+    // Turn config into elements.
+    std::string cfg_txt = "\"option 'bogus'\"";
+    ElementPtr config_element = Element::fromJSON(cfg_txt);
+
+    // Create the parser.
+    ASSERT_NO_THROW(parser.reset(new ExpressionParser("", parsed_expr,
+                                                      context)));
+    // Expressionn build() should fail.
+    ASSERT_THROW(parser->build(config_element), DhcpConfigError);
+}
+
 // Verifies you can create a class with only a name
 // Whether that's useful or not, remains to be seen.
 // For now the class allows it.
@@ -199,8 +250,8 @@ TEST_F(ClientClassDefParserTest, nameAndExpressionClass) {
 
     std::string cfg_text =
         "{ \n"
-        "    \"name\": \"MICROSOFT\", \n"
-        "    \"test\": \"vendor-class-identifier == 'MSFT'\" \n"
+        "    \"name\": \"class_one\", \n"
+        "    \"test\": \"option[100] == 'works right'\" \n"
         "} \n";
 
     ClientClassDefPtr cclass;
@@ -208,7 +259,7 @@ TEST_F(ClientClassDefParserTest, nameAndExpressionClass) {
 
     // We should find our class.
     ASSERT_TRUE(cclass);
-    EXPECT_EQ("MICROSOFT", cclass->getName());
+    EXPECT_EQ("class_one", cclass->getName());
 
     // CfgOption should be a non-null pointer but there
     // should be no options.  Currently there's no good
@@ -224,15 +275,14 @@ TEST_F(ClientClassDefParserTest, nameAndExpressionClass) {
     ExpressionPtr match_expr = cclass->getMatchExpr();
     ASSERT_TRUE(match_expr);
 
-    // Evaluate it. For now the result will be the
-    // expression string as dummy ExpressionParser
-    // just makes an expression of one TokenString
-    // containing the expression string itself.
-    ValueStack vstack;
-    Pkt4Ptr pkt4;
-    pkt4.reset(new Pkt4(DHCPDISCOVER, 12345));
-    (*match_expr)[0]->evaluate(*pkt4, vstack);
-    EXPECT_EQ(vstack.top(), "\"vendor-class-identifier == 'MSFT'\"");
+    // Build a packet that will fail evaluation.
+    Pkt4Ptr pkt4(new Pkt4(DHCPDISCOVER, 123));
+    EXPECT_FALSE(evaluate(*match_expr, *pkt4));
+
+    // Now add the option so it will pass.
+    OptionPtr opt(new OptionString(Option::V4, 100, "works right"));
+    pkt4->addOption(opt);
+    EXPECT_TRUE(evaluate(*match_expr, *pkt4));
 }
 
 // Verifies you can create a class with a name and options,
@@ -277,7 +327,7 @@ TEST_F(ClientClassDefParserTest, basicValidClass) {
     std::string cfg_text =
         "{ \n"
         "    \"name\": \"MICROSOFT\", \n"
-        "    \"test\": \"vendor-class-identifier == 'MSFT'\", \n"
+        "    \"test\": \"option[100] == 'booya'\", \n"
         "    \"option-data\": [ \n"
         "        { \n"
         "           \"name\": \"domain-name-servers\", \n"
@@ -305,15 +355,14 @@ TEST_F(ClientClassDefParserTest, basicValidClass) {
     ExpressionPtr match_expr = cclass->getMatchExpr();
     ASSERT_TRUE(match_expr);
 
-    // Evaluate it. For now the result will be the
-    // expression string as dummy ExpressionParser
-    // just makes an expression of one TokenString
-    // containing the expression string itself.
-    ValueStack vstack;
-    Pkt4Ptr pkt4;
-    pkt4.reset(new Pkt4(DHCPDISCOVER, 12345));
-    (*match_expr)[0]->evaluate(*pkt4, vstack);
-    EXPECT_EQ(vstack.top(), "\"vendor-class-identifier == 'MSFT'\"");
+    // Build a packet that will fail evaluation.
+    Pkt4Ptr pkt4(new Pkt4(DHCPDISCOVER, 123));
+    EXPECT_FALSE(evaluate(*match_expr, *pkt4));
+
+    // Now add the option so it will pass.
+    OptionPtr opt(new OptionString(Option::V4, 100, "booya"));
+    pkt4->addOption(opt);
+    EXPECT_TRUE(evaluate(*match_expr, *pkt4));
 }
 
 // Verifies that a class with no name, fails to parse.