From: Thomas Markwalder Date: Thu, 3 Oct 2024 15:16:53 +0000 (-0400) Subject: [#3583] Minor refactoring X-Git-Tag: Kea-2.7.4~68 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=02e59abd64d5dca8c1aa9ec8a6bb86eb53863c12;p=thirdparty%2Fkea.git [#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 c604c6045a..590700a706 100644 --- 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 3d1d6ca0c9..bad28bad92 100644 --- 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 784cc4a895..b5ed577761 100644 --- 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 cb5340dfe5..b07e22f68b 100644 --- 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 49bfc9c48f..b0e58073ca 100644 --- a/src/lib/dhcp/tests/classify_unittest.cc +++ b/src/lib/dhcp/tests/classify_unittest.cc @@ -6,9 +6,15 @@ #include #include +#include +#include +#include + #include +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()); +}