[#683,!390] Addressed review comments.

diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc
index f555f89c81c00842ff0de9c4901d709e53ebb710..c8e3973fc2e52a98dce0307406d7e0390d2e4998 100644 (file)
--- a/src/hooks/dhcp/lease_cmds/lease_cmds.cc
+++ b/src/hooks/dhcp/lease_cmds/lease_cmds.cc
@@ -11,6 +11,7 @@
 #include <cc/data.h>
 #include <asiolink/io_address.h>
 #include <dhcpsrv/cfgmgr.h>
+#include <dhcpsrv/dhcpsrv_exceptions.h>
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/subnet_id.h>
@@ -124,7 +125,8 @@ public:
 
     /// @brief lease6-bulk-apply command handler
     ///
-    /// Provides the implementation for the @ref isc::lease_cmds::LeaseCmds::lease6BulkHandler.
+    /// Provides the implementation for the
+    /// @ref isc::lease_cmds::LeaseCmds::lease6BulkApplyHandler.
     ///
     /// @param handle Callout context - which is expected to contain the
     /// add command JSON text in the "command" argument
@@ -271,10 +273,10 @@ public:
     /// @param lease_type lease type.
     /// @param lease_address lease address.
     /// @param duid DUID of the client.
-    ElementPtr getFailedLeaseMap(const SubnetID& subnet_id,
-                                 const Lease::Type& lease_type,
-                                 const IOAddress& lease_address,
-                                 const DuidPtr&duid) const;
+    ElementPtr createFailedLeaseMap(const SubnetID& subnet_id,
+                                    const Lease::Type& lease_type,
+                                    const IOAddress& lease_address,
+                                    const DuidPtr&duid) const;
 };
 
 int
@@ -919,8 +921,8 @@ LeaseCmdsImpl::lease6BulkApplyHandler(CalloutHandle& handle) {
                     if (!failed_deleted_list) {
                          failed_deleted_list = Element::createList();
                     }
-                    failed_deleted_list->add(getFailedLeaseMap(p.subnet_id, p.lease_type,
-                                                               p.addr, p.duid));
+                    failed_deleted_list->add(createFailedLeaseMap(p.subnet_id, p.lease_type,
+                                                                  p.addr, p.duid));
                 }
             }
         }
@@ -934,20 +936,17 @@ LeaseCmdsImpl::lease6BulkApplyHandler(CalloutHandle& handle) {
             for (auto lease : parsed_leases_list) {
 
                 Lease6Parser parser;
-                bool force_update;
 
                 try {
-                    // Check if the lease already exists.
-                    auto existing_lease = LeaseMgrFactory::instance().getLease6(lease->type_,
-                                                                                lease->addr_);
-                    // If the lease exists, we should update it. Otherwise, we add
-                    // the new lease.
-                    if (existing_lease) {
+                    try {
+                        // Try to update.
                         LeaseMgrFactory::instance().updateLease6(lease);
 
-                    } else {
+                    } catch (const NoSuchLease& ex) {
+                        // Lease to be updated not found, so add it.
                         LeaseMgrFactory::instance().addLease(lease);
                     }
+
                     ++success_count;
 
                 } catch (...) {
@@ -955,10 +954,10 @@ LeaseCmdsImpl::lease6BulkApplyHandler(CalloutHandle& handle) {
                     if (!failed_leases_list) {
                          failed_leases_list = Element::createList();
                     }
-                    failed_leases_list->add(getFailedLeaseMap(lease->subnet_id_,
-                                                              lease->type_,
-                                                              lease->addr_,
-                                                              lease->duid_));
+                    failed_leases_list->add(createFailedLeaseMap(lease->subnet_id_,
+                                                                 lease->type_,
+                                                                 lease->addr_,
+                                                                 lease->duid_));
                 }
             }
         }
@@ -1264,10 +1263,10 @@ LeaseCmdsImpl::getIPv6AddressForDelete(const Parameters& parameters) const {
 }
 
 ElementPtr
-LeaseCmdsImpl::getFailedLeaseMap(const SubnetID& subnet_id,
-                                 const Lease::Type& lease_type,
-                                 const IOAddress& lease_address,
-                                 const DuidPtr&duid) const {
+LeaseCmdsImpl::createFailedLeaseMap(const SubnetID& subnet_id,
+                                    const Lease::Type& lease_type,
+                                    const IOAddress& lease_address,
+                                    const DuidPtr&duid) const {
     auto failed_lease_map = Element::createMap();
     failed_lease_map->set("subnet-id",
                           Element::create(static_cast<long int>(subnet_id)));

diff --git a/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc b/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc
index 4aabae3aca9eb76b84ed944ecb33d622b78d05b3..fd9e489ce29b9b273a2fdf76fb97153e8b20c4a4 100644 (file)
--- a/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc
+++ b/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc
@@ -3999,7 +3999,7 @@ TEST_F(LeaseCmdsTest, Lease6BulkApply) {
 // with the lease6-bulk-apply.
 TEST_F(LeaseCmdsTest, Lease6BulkApplyAddsOnly) {
 
-    initLeaseMgr(true, true); // (true = v6, true = create leases)
+    initLeaseMgr(true, false); // (true = v6, true = create leases)
 
     // Now send the command.
     string cmd =
@@ -4032,6 +4032,49 @@ TEST_F(LeaseCmdsTest, Lease6BulkApplyAddsOnly) {
     EXPECT_TRUE(lmptr_->getLease6(Lease::TYPE_NA, IOAddress("2001:db8:2::123")));
 }
 
+// This test verifies that it is possible to update leases with
+// the lease6-bulk-apply.
+TEST_F(LeaseCmdsTest, Lease6BulkApplyUpdatesOnly) {
+
+    initLeaseMgr(true, true); // (true = v6, true = create leases)
+
+    // Now send the command.
+    string cmd =
+        "{\n"
+        "    \"command\": \"lease6-bulk-apply\",\n"
+        "    \"arguments\": {"
+        "        \"leases\": ["
+        "            {"
+        "                \"subnet-id\": 66,\n"
+        "                \"ip-address\": \"2001:db8:1::1\",\n"
+        "                \"duid\": \"11:11:11:11:11:11\",\n"
+        "                \"iaid\": 1234\n"
+        "            },"
+        "            {"
+        "                \"subnet-id\": 66,\n"
+        "                \"ip-address\": \"2001:db8:1::2\",\n"
+        "                \"duid\": \"22:22:22:22:22:22\",\n"
+        "                \"iaid\": 1234\n"
+        "            }"
+        "        ]"
+        "    }"
+        "}";
+    string exp_rsp = "Bulk apply of 2 IPv6 leases completed.";
+
+    // The status expected is success.
+    testCommand(cmd, CONTROL_RESULT_SUCCESS, exp_rsp);
+
+    //  Check that the leases we inserted are stored.
+    Lease6Ptr lease1 = lmptr_->getLease6(Lease::TYPE_NA, IOAddress("2001:db8:1::1"));
+    Lease6Ptr lease2 = lmptr_->getLease6(Lease::TYPE_NA, IOAddress("2001:db8:1::2"));
+    ASSERT_TRUE(lease1);
+    ASSERT_TRUE(lease2);
+
+    // The IAIDs should have been updated for the existing leases.
+    EXPECT_EQ(1234, lease1->iaid_);
+    EXPECT_EQ(1234, lease2->iaid_);
+}
+
 // This test verifies that it is possible to only delete leases
 // with the lease6-bulk-apply.
 TEST_F(LeaseCmdsTest, Lease6BulkApplyDeletesOnly) {
@@ -4091,7 +4134,18 @@ TEST_F(LeaseCmdsTest, Lease6BulkApplyDeleteNonExiting) {
     string exp_rsp = "Bulk apply of 0 IPv6 leases completed.";
 
     // The status expected is success.
-    testCommand(cmd, CONTROL_RESULT_EMPTY, exp_rsp);
+    auto resp = testCommand(cmd, CONTROL_RESULT_EMPTY, exp_rsp);
+    ASSERT_TRUE(resp);
+    ASSERT_EQ(Element::map, resp->getType());
+
+    auto args = resp->get("arguments");
+    ASSERT_TRUE(args);
+    ASSERT_EQ(Element::map, args->getType());
+
+    auto failed_deleted_leases = args->get("failed-deleted-leases");
+    ASSERT_TRUE(failed_deleted_leases);
+    ASSERT_EQ(Element::list, failed_deleted_leases->getType());
+    ASSERT_EQ(2, failed_deleted_leases->size());
 }
 
 // Check that changes for other leases are not applied if one of