[#3583] Minor refactoring

Added ClientClasses::fromElement()

/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc
/src/hooks/dhcp/pgsql_cb/pgsql_cb_impl.cc

/src/lib/dhcp/classify.cc
/src/lib/dhcp/classify.h

/src/lib/dhcp/tests/classify_unittest.cc
    TEST(ClassifyTest, ClientClassesFromElement) - new test

diff --git a/src/hooks/dhcp/mysql/mysql_cb_impl.cc b/src/hooks/dhcp/mysql/mysql_cb_impl.cc
index c604c6045a15e0287f4cbac5644fffedea9394de..590700a7066fdcc976cf8332d5b00a6f35cf506c 100644 (file)
--- a/src/hooks/dhcp/mysql/mysql_cb_impl.cc
+++ b/src/hooks/dhcp/mysql/mysql_cb_impl.cc
@@ -884,20 +884,12 @@ MySqlConfigBackendImpl::processOptionRow(const Option::Universe& universe,
 
     // Get client classes list
     ElementPtr client_classes = (*(first_binding + 13))->getJSON();
-    if (client_classes) {
-        if (client_classes->getType() != Element::list) {
-            isc_throw(BadValue, "invalid client_classes value "
-                                << (*(first_binding + 13))->getString());
-        }
-
-        for (auto i = 0; i < client_classes->size(); ++i) {
-            auto cclass = client_classes->get(i);
-                if (cclass->getType() != Element::string) {
-                    isc_throw(BadValue, "elements of client_classes list must be valid strings");
-                }
-
-            desc->addClientClass(cclass->stringValue());
-        }
+    try {
+        desc->client_classes_.fromElement(client_classes);
+    } catch (const std::exception& ex) {
+        isc_throw(BadValue, "invalid 'client_classes' : "
+                            << (*(first_binding + 13))->getString()
+                            << ex.what());
     }
 
     return (desc);
@@ -1128,7 +1120,7 @@ MySqlConfigBackendImpl::getPort() const {
     return (0);
 }
 
-db::MySqlBindingPtr 
+db::MySqlBindingPtr
 MySqlConfigBackendImpl::createInputClientClassesBinding(const ClientClasses& client_classes) {
     if (client_classes.empty()) {
         return(db::MySqlBinding::createNull());

diff --git a/src/hooks/dhcp/pgsql/pgsql_cb_impl.cc b/src/hooks/dhcp/pgsql/pgsql_cb_impl.cc
index 3d1d6ca0c95d197fa53954c5af821eeef23645eb..bad28bad92472afc5eea5696f2e33ca15e70ac6d 100644 (file)
--- a/src/hooks/dhcp/pgsql/pgsql_cb_impl.cc
+++ b/src/hooks/dhcp/pgsql/pgsql_cb_impl.cc
@@ -856,20 +856,13 @@ PgSqlConfigBackendImpl::setClientClasses(PgSqlResultRowWorker& worker, size_t co
     }
 
     ElementPtr cclasses_element = worker.getJSON(col);
-    if (cclasses_element->getType() != Element::list) {
+    // Get client classes list
+    try {
+        client_classes.fromElement(cclasses_element);
+    } catch (const std::exception& ex) {
         std::ostringstream ss;
         cclasses_element->toJSON(ss);
-        isc_throw(BadValue, "invalid client_classes value " << ss.str());
-    }
-
-    for (auto i = 0; i < cclasses_element->size(); ++i) {
-        auto cclasses_item = cclasses_element->get(i);
-        if (cclasses_item->getType() != Element::string) {
-            isc_throw(BadValue, "elements of client_classes list must"
-                                "be valid strings");
-        }
-
-        client_classes.insert(cclasses_item->stringValue());
+        isc_throw(BadValue, "invalid 'client_classes' : " << ss.str() << ex.what());
     }
 }
 

diff --git a/src/lib/dhcp/classify.cc b/src/lib/dhcp/classify.cc
index 784cc4a895535f679d25be749f757221226b02f9..b5ed577761f7d5ebe784e8bbd5a65ceab35d80aa 100644 (file)
--- a/src/lib/dhcp/classify.cc
+++ b/src/lib/dhcp/classify.cc
@@ -81,6 +81,25 @@ ClientClasses::toElement() const {
     return (result);
 }
 
+void
+ClientClasses::fromElement(isc::data::ElementPtr cc_list) {
+    if (cc_list) {
+        clear();
+        if (cc_list->getType() != Element::list) {
+            isc_throw(BadValue, "not a List element");
+        }
+
+        for (auto i = 0; i < cc_list->size(); ++i) {
+            auto cclass = cc_list->get(i);
+            if (cclass->getType() != Element::string) {
+                isc_throw(BadValue, "elements of list must be valid strings"); 
+            }
+
+            insert(cclass->stringValue());
+        }
+    }
+}
+
 bool
 ClientClasses::equals(const ClientClasses& other) const {
  return ((size() == other.size()) && std::equal(cbegin(), cend(), other.cbegin()));

diff --git a/src/lib/dhcp/classify.h b/src/lib/dhcp/classify.h
index cb5340dfe59595cdf3e8fe75e256dff5d4235428..b07e22f68b04a60f67bb1570186a38b493c9b050 100644 (file)
--- a/src/lib/dhcp/classify.h
+++ b/src/lib/dhcp/classify.h
@@ -225,6 +225,12 @@ namespace dhcp {
         /// @return the list
         isc::data::ElementPtr toElement() const;
 
+        /// @brief Sets contents from a ListElement
+        ///
+        /// @throw BadValue if the element is not a list or contents
+        /// are invalid
+        void fromElement(isc::data::ElementPtr list);
+
     private:
         /// @brief container part
         ClientClassContainer container_;

diff --git a/src/lib/dhcp/tests/classify_unittest.cc b/src/lib/dhcp/tests/classify_unittest.cc
index 49bfc9c48f5c1b3a723888ffa3dafa0ae9af3689..b0e58073ca93de6c3620a8a5b3333afa49d495d6 100644 (file)
--- a/src/lib/dhcp/tests/classify_unittest.cc
+++ b/src/lib/dhcp/tests/classify_unittest.cc
@@ -6,9 +6,15 @@
 
 #include <config.h>
 #include <dhcp/classify.h>
+#include <exceptions/exceptions.h>
+#include <cc/data.h>
+#include <testutils/gtest_utils.h>
+
 #include <gtest/gtest.h>
 
+using namespace isc;
 using namespace isc::dhcp;
+using namespace isc::data;
 
 // Trivial test for now as ClientClass is a std::string.
 TEST(ClassifyTest, ClientClass) {
@@ -193,3 +199,58 @@ TEST(ClassifyTest, Erase) {
     EXPECT_FALSE(classes.contains("alpha"));
     EXPECT_FALSE(classes.contains("beta"));
 }
+
+// Check that the ClientClasses::fromElement function.
+TEST(ClassifyTest, ClientClassesFromElement) {
+    // No classes.
+    ClientClasses classes;
+    EXPECT_TRUE(classes.toElement()->empty());
+
+    ElementPtr cclasses_element;
+    // Verify a empty element pointer is harmless.
+    ASSERT_NO_THROW(classes.fromElement(cclasses_element));
+
+    // Verify A non-list element is caught.
+    cclasses_element = Element::create("bogus");
+    ASSERT_THROW_MSG(classes.fromElement(cclasses_element), BadValue,
+                    "not a List element");
+
+    // Verify an empty list is harmless.
+    cclasses_element = Element::createList();
+
+    // Verify a empty element pointer is harmless.
+    ASSERT_NO_THROW(classes.fromElement(cclasses_element));
+
+    // Verify an invalid list is caught.
+    cclasses_element->add(Element::create(123));
+    ASSERT_THROW_MSG(classes.fromElement(cclasses_element), BadValue,
+                    "elements of list must be valid strings");
+
+    cclasses_element = Element::createList();
+    cclasses_element->add(Element::create("one"));
+    cclasses_element->add(Element::create("two"));
+
+    // Verify a valid list works.
+    ASSERT_NO_THROW(classes.fromElement(cclasses_element));
+    EXPECT_TRUE(classes.contains("one"));
+    EXPECT_TRUE(classes.contains("two"));
+
+    // Verify a second invocation replaces the contents.
+    cclasses_element = Element::createList();
+    cclasses_element->add(Element::create("three"));
+    ASSERT_NO_THROW(classes.fromElement(cclasses_element));
+    EXPECT_FALSE(classes.contains("one"));
+    EXPECT_FALSE(classes.contains("two"));
+    EXPECT_TRUE(classes.contains("three"));
+
+    // Verify another invocation with an empty pointer is harmless. 
+    cclasses_element.reset();
+    ASSERT_NO_THROW(classes.fromElement(cclasses_element));
+    EXPECT_TRUE(classes.contains("three"));
+
+    // Verify another third invocation with an empty list
+    // clears the contents.
+    cclasses_element = Element::createList();
+    ASSERT_NO_THROW(classes.fromElement(cclasses_element));
+    EXPECT_TRUE(classes.empty());
+}