From dba841c12d31a6c2cbb88752c92650075da440a2 Mon Sep 17 00:00:00 2001
From: Thomas Markwalder <tmark@isc.org>
Date: Fri, 24 Jan 2025 09:17:03 -0500
Subject: [PATCH] [#3463] Added getter UTs and dup behavior

modified:   src/hooks/dhcp/lease_cmds/binding_variables.cc
modified:   src/hooks/dhcp/lease_cmds/binding_variables.h
modified:   src/hooks/dhcp/lease_cmds/tests/binding_variables_unittest.cc
---
 .../dhcp/lease_cmds/binding_variables.cc      |  7 +--
 src/hooks/dhcp/lease_cmds/binding_variables.h | 12 +++-
 .../tests/binding_variables_unittest.cc       | 60 ++++++++++++++++++-
 3 files changed, 72 insertions(+), 7 deletions(-)

diff --git a/src/hooks/dhcp/lease_cmds/binding_variables.cc b/src/hooks/dhcp/lease_cmds/binding_variables.cc
index ecbd05bb0c..e594724ae9 100644
--- a/src/hooks/dhcp/lease_cmds/binding_variables.cc
+++ b/src/hooks/dhcp/lease_cmds/binding_variables.cc
@@ -61,14 +61,12 @@ BindingVariable::evaluate(PktPtr packet) const {
     } 
 }
 
-/// @todo Not sure we need CfgElement derivation
 ElementPtr
 BindingVariable::toElement() const {
     ElementPtr map = Element::createMap();
     map->set("name", Element::create(name_));
     map->set("expression_str", Element::create(expression_str_));
     map->set("source", Element::create((source_ == QUERY ? "query" : "response")));
-    // family_ is contextual
     return (map);
 }
 
@@ -76,10 +74,11 @@ BindingVariableCache::BindingVariableCache()
     : variables_(), mutex_(new std::mutex) {
 }
 
-void
+bool
 BindingVariableCache::cacheVariable(BindingVariablePtr variable) {
     util::MultiThreadingLock lock(*mutex_);
-    variables_.push_back(variable);
+    auto retpair = variables_.push_back(variable);
+    return(retpair.second);
 }
 
 void 
diff --git a/src/hooks/dhcp/lease_cmds/binding_variables.h b/src/hooks/dhcp/lease_cmds/binding_variables.h
index 6c7c88c698..12925c75bd 100644
--- a/src/hooks/dhcp/lease_cmds/binding_variables.h
+++ b/src/hooks/dhcp/lease_cmds/binding_variables.h
@@ -99,7 +99,10 @@ public:
         return (family_);
     }
 
-    /// @todo Not sure we need CfgElement derivation
+    /// @brief Creates an Element tree for the variable's configurable
+    /// members.
+    ///
+    /// @return ElementPtr containing the configurable members.
     virtual data::ElementPtr toElement() const;
 
 private:
@@ -183,8 +186,13 @@ public:
 
     /// @brief Adds (or replaces) the variable in the cache.
     ///
+    /// Variables must be unique by name. If the variable to
+    /// be added is a duplicate, the add fails and the function
+    /// returns false.
+    ///
     /// @param variable pointer to the variable to store.
-    void cacheVariable(BindingVariablePtr variable);
+    /// @return true if the variable was added, false otherwise.
+    bool cacheVariable(BindingVariablePtr variable);
 
     /// @brief Delete all the entries in the cache.
     void clear();
diff --git a/src/hooks/dhcp/lease_cmds/tests/binding_variables_unittest.cc b/src/hooks/dhcp/lease_cmds/tests/binding_variables_unittest.cc
index 8bd8beef56..479a8d4727 100644
--- a/src/hooks/dhcp/lease_cmds/tests/binding_variables_unittest.cc
+++ b/src/hooks/dhcp/lease_cmds/tests/binding_variables_unittest.cc
@@ -123,8 +123,30 @@ TEST(BindingVariableTest, invalidConstructor) {
     }
 }
 
-TEST(BindingVariableCacheTest, basics) {
+/// @brief Tests BindingVariable::toElement().
+TEST(BindingVariableTest, toElement) {
+    BindingVariablePtr bv;
 
+    ASSERT_NO_THROW_LOG(bv.reset(new BindingVariable("myvar",
+                                                     "pkt4.mac",
+                                                     BindingVariable::QUERY,
+                                                     AF_INET)));
+    ASSERT_TRUE(bv);
+    ElementPtr elem;
+
+    ASSERT_NO_THROW_LOG(elem = bv->toElement());
+    std::stringstream ss;
+    elem->toJSON(ss);
+    std::string expected_json = "{ \"expression_str\": \"pkt4.mac\","
+                                " \"name\": \"myvar\", \"source\": \"query\" }";
+    EXPECT_EQ(ss.str(), expected_json);
+}
+
+/// @brief Verifies basic operation of the cache including
+/// construction, all getters and cache clearing.
+TEST(BindingVariableCacheTest, basics) {
+    // Save start time of test.  We use seconds because that's that
+    // BaseStampedElement uses.
     auto ref_time = boost::posix_time::second_clock::local_time();
 
     // Create a new cache.
@@ -231,4 +253,40 @@ TEST(BindingVariableCacheTest, basics) {
     EXPECT_GT(cache->getLastFlushTime(), ref_time);
 }
 
+/// @brief Verifies cache behavior for handling duplicate
+/// entries.
+TEST(BindingVariableCacheTest, duplicateEntries) {
+    // Create a new cache.
+    BindingVariableCachePtr cache(new BindingVariableCache());
+
+
+    std::string valid_v4_exp = "pkt4.mac";
+    BindingVariablePtr var1(new BindingVariable("one", valid_v4_exp,
+                                                BindingVariable::QUERY,
+                                                AF_INET));
+
+    BindingVariablePtr var2(new BindingVariable("one", valid_v4_exp,
+                                                BindingVariable::RESPONSE,
+                                                AF_INET));
+
+    bool add_flag;
+    ASSERT_NO_THROW_LOG(add_flag = cache->cacheVariable(var1));
+    EXPECT_TRUE(add_flag);
+    EXPECT_EQ(cache->size(), 1);
+
+    // Make sure getByName returns the added variable.
+    BindingVariablePtr found_var;
+    ASSERT_NO_THROW_LOG(found_var = cache->getByName("one"));
+    ASSERT_EQ(found_var->getSource(), BindingVariable::QUERY);
+
+    // Adding a duplicate should fail.
+    ASSERT_NO_THROW_LOG(add_flag = cache->cacheVariable(var2));
+    EXPECT_FALSE(add_flag);
+    EXPECT_EQ(cache->size(), 1);
+
+    // Make sure getByName returns the original variable.
+    ASSERT_NO_THROW_LOG(found_var = cache->getByName("one"));
+    ASSERT_EQ(found_var->getSource(), BindingVariable::QUERY);
+}
+
 } // end of anonymous namespace