[#2426] Addressed review comments

src/lib/dhcpsrv/memfile_lease_limits.cc
    ClassLeaseCounter::getLeaseClientClasses()
    - improved error handling

src/lib/dhcpsrv/memfile_lease_limits.h
    ClassLeaseCounter::getLeaseClientClasses()
    - changed from const to static

src/lib/dhcpsrv/memfile_lease_mgr.cc
    Memfile_LeaseMgr::checkLimits4() fixed count type
    Memfile_LeaseMgr::checkLimits6() fixed exception

dhcpsrv/memfile_lease_mgr.h
    fixed typos

src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
    GenericLeaseMgrTest::testLeaseLimits4()
    GenericLeaseMgrTest::testLeaseLimits6()
    - removed unecesary CfgMgr logic

src/lib/dhcpsrv/tests/memfile_lease_limits_unittest.cc
    TEST_F(ClassLeaseCounterTest, adjustClassCountsTest4)
    TEST_F(ClassLeaseCounterTest, adjustClassCountsTest6)
    TEST_F(ClassLeaseCounterTest, getLeaseClientClassesTest)
    - new tests

src/lib/dhcpsrv/memfile_lease_limits.cc

@@ -54,15 +54,23 @@ ClassLeaseCounter::adjustClassCount(const ClientClass& client_class, int offset,
 
 
 ConstElementPtr
-ClassLeaseCounter::getLeaseClientClasses(LeasePtr lease) const {
+ClassLeaseCounter::getLeaseClientClasses(LeasePtr lease) {
+    if (!lease) {
+        isc_throw(BadValue, "getLeaseClientCLasses - lease cannot be empty");
+    }
+
     ConstElementPtr classes;
     auto ctx = lease->getContext();
-    if (ctx) {
+    try {
-        classes = ctx->find("ISC/client-classes");
+        if (ctx) {
-        if (classes && classes->getType() != Element::list) {
+            classes = ctx->find("ISC/client-classes");
-            isc_throw(Unexpected, "getLeaseClientClasses: "
+            if (classes && classes->getType() != Element::list) {
-                      << lease->toText() << " is not a list!");
+                isc_throw(BadValue, "client-classes is not a list");
+            }
         }
+    } catch (const std::exception& ex) {
+        isc_throw(BadValue, "getLeaseClientClasses - invalid context: "
+                  << data::prettyPrint(ctx) << ", " << ex.what());
     }
 
     return (classes);
@@ -84,7 +92,7 @@ ClassLeaseCounter::adjustClassCounts(ConstElementPtr classes, int offset,
 void
 ClassLeaseCounter::addLease(LeasePtr lease) {
     if (!lease) {
-        isc_throw(BadValue, "addLease(): lease cannot be empty");
+        isc_throw(BadValue, "addLease - lease cannot be empty");
     }
 
     ConstElementPtr classes = getLeaseClientClasses(lease);
@@ -102,11 +110,11 @@ void
 ClassLeaseCounter::updateLease(LeasePtr new_lease, LeasePtr old_lease) {
     // Sanity checks.
     if (!new_lease) {
-        isc_throw(BadValue, "updateLease(): new_lease cannot be empty");
+        isc_throw(BadValue, "updateLease - new_lease cannot be empty");
     }
 
     if (!old_lease) {
-        isc_throw(BadValue, "updateLease(): old_lease cannot be empty");
+        isc_throw(BadValue, "updateLease - old_lease cannot be empty");
     }
 
     ConstElementPtr new_classes = getLeaseClientClasses(new_lease);
@@ -135,7 +143,7 @@ ClassLeaseCounter::updateLease(LeasePtr new_lease, LeasePtr old_lease) {
 void
 ClassLeaseCounter::removeLease(LeasePtr lease) {
     if (!lease) {
-        isc_throw(BadValue, "removeLease(): lease cannot be empty");
+        isc_throw(BadValue, "removeLease - lease cannot be empty");
     }
 
     ConstElementPtr classes = getLeaseClientClasses(lease);

src/lib/dhcpsrv/memfile_lease_limits.h

@@ -129,7 +129,7 @@ public:
     /// @param lease lease from which to fetch classes
     /// @return ElementPtr to an Element::List containing the client classes or an
     /// empty List.
-    data::ConstElementPtr getLeaseClientClasses(LeasePtr lease) const;
+    static data::ConstElementPtr getLeaseClientClasses(LeasePtr lease);
 
 private:
     /// @brief Fetches the map used to count the given lease type.

src/lib/dhcpsrv/memfile_lease_mgr.cc

@@ -2191,7 +2191,7 @@ Memfile_LeaseMgr::checkLimits4(isc::data::ConstElementPtr const& user_context) c
         if (getLeaseLimit(subnet_elem, Lease::TYPE_V4, limit)) {
             // If the limit is > 0 look up the subnet lease count. Limit of 0 always
             // denies the lease.
-            auto lease_count = 0;
+            int64_t lease_count = 0;
             if (limit) {
                 lease_count = getSubnetStat(subnet_id, "assigned-addresses");
             }
@@ -2211,7 +2211,7 @@ Memfile_LeaseMgr::checkLimits4(isc::data::ConstElementPtr const& user_context) c
 }
 
 std::string
-Memfile_LeaseMgr:: checkLimits6(isc::data::ConstElementPtr const& user_context) const {
+Memfile_LeaseMgr::checkLimits6(isc::data::ConstElementPtr const& user_context) const {
     if (!user_context) {
         return ("");
     }
@@ -2231,7 +2231,7 @@ Memfile_LeaseMgr:: checkLimits6(isc::data::ConstElementPtr const& user_context)
             // Get class name.
             ConstElementPtr name_elem = class_elem->get("name");
             if (!name_elem) {
-                isc_throw(BadValue, "client-class.name is missing: "
+                isc_throw(BadValue, "checkLimits6 - client-class.name is missing: "
                           << prettyPrint(limits));
             }
 
@@ -2272,7 +2272,7 @@ Memfile_LeaseMgr:: checkLimits6(isc::data::ConstElementPtr const& user_context)
         // Get the subnet id.
         ConstElementPtr id_elem = subnet_elem->get("id");
         if (!id_elem) {
-            isc_throw(BadValue, "subnet.id is missing: "
+            isc_throw(BadValue, "checkLimits6 - subnet.id is missing: "
                       << prettyPrint(limits));
         }
 
@@ -2291,7 +2291,7 @@ Memfile_LeaseMgr:: checkLimits6(isc::data::ConstElementPtr const& user_context)
 
         // If the limit is > 0 look up the class lease count.  Limit of 0 always
         // denies the lease.
-        size_t lease_count = 0;
+        int64_t lease_count = 0;
         if (limit) {
             lease_count = getSubnetStat(subnet_id, (ltype == Lease::TYPE_NA ?
                                                     "assigned-nas" : "assigned-pds"));

src/lib/dhcpsrv/memfile_lease_mgr.h

@@ -843,7 +843,7 @@ private:
     /// @brief Fetches the most recent value for a subnet statistic
     ///
     /// @param subnet_id subnet id of the subnet for which the stat is desired
-    /// @param stat_label name of the statisitic desired (e.g. "assigned-addresses")
+    /// @param stat_label name of the statistic desired (e.g. "assigned-addresses")
     ///
     /// @return Value of the statistic or zero if there are no entries found.
     int64_t getSubnetStat(const SubnetID& subnet_id, const std::string& stat_label) const;

src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc

@@ -3906,15 +3906,6 @@ GenericLeaseMgrTest::testLeaseStatsQueryAttribution6() {
 
 void
 GenericLeaseMgrTest::testLeaseLimits4() {
-    // Create a subnet.  We need the subnet to exist so statistics will exist.
-    CfgSubnets4Ptr cfg = CfgMgr::instance().getStagingCfg()->getCfgSubnets4();
-    Subnet4Ptr subnet;
-
-    subnet.reset(new Subnet4(IOAddress("192.0.1.0"), 24, 1, 2, 3, 1));
-    cfg->add(subnet);
-
-    ASSERT_NO_THROW(CfgMgr::instance().commit());
-
     std::string text;
     ElementPtr user_context;
 
@@ -3963,15 +3954,6 @@ GenericLeaseMgrTest::testLeaseLimits4() {
 
 void
 GenericLeaseMgrTest::testLeaseLimits6() {
-    // Create a subnet.  We need the subnet to exist so statistics will exist.
-    CfgSubnets4Ptr cfg = CfgMgr::instance().getStagingCfg()->getCfgSubnets4();
-    Subnet4Ptr subnet;
-
-    subnet.reset(new Subnet4(IOAddress("192.0.1.0"), 24, 1, 2, 3, 1));
-    cfg->add(subnet);
-
-    ASSERT_NO_THROW(CfgMgr::instance().commit());
-
     std::string text;
     ElementPtr user_context;
 

src/lib/dhcpsrv/tests/memfile_lease_limits_unittest.cc

@@ -195,7 +195,7 @@ public:
     /// #brief Test updating type of lease for given old and new states and class lists
     ///
     /// The test creates an old and new version of the same lease and passes them into
-    /// @c ClassLeaseCount::updateLease().  It then verifies the class lease counts
+    /// @c ClassLeaseCounter::updateLease().  It then verifies the class lease counts
     /// against expected count lists.
     ///
     /// @param old_state state of the old lease
@@ -304,6 +304,26 @@ public:
 
     }
 
+    /// @brief Verifies ClassLeaseCounter::adjustClassCounts
+    ///
+    /// @param ltype type of lease to count.
+    void adjustClassCountsTest(const Lease::Type ltype) {
+        ConstElementPtr class_list1 =  ctx1_->find("ISC/client-classes");
+        ASSERT_TRUE(class_list1);
+
+        // Increment the classes by 2 and verify.
+        ASSERT_NO_THROW_LOG(clc_.adjustClassCounts(class_list1, 2, ltype));
+        checkClassCounts(classes1_, std::list<size_t>({ 2, 2, 2 }), ltype);
+
+        // Decrement the classes by 1 and verify.
+        ASSERT_NO_THROW_LOG(clc_.adjustClassCounts(class_list1, -1, ltype));
+        checkClassCounts(classes1_, std::list<size_t>({ 1, 1, 1 }), ltype);
+
+        // Decrement the classes by 2 and verify that roll-over is avoided..
+        ASSERT_NO_THROW_LOG(clc_.adjustClassCounts(class_list1, -2, ltype));
+        checkClassCounts(classes1_, std::list<size_t>({ 0, 0, 0 }), ltype);
+    }
+
     /// @brief First test list of classes
     std::list<ClientClass> classes1_;
 
@@ -372,6 +392,19 @@ TEST_F(ClassLeaseCounterTest, basicCountingTests4) {
     EXPECT_EQ(39, count);
 }
 
+// Tests getting and adjusting basic class counts for
+// a Lease::TYPE_V4.
+TEST_F(ClassLeaseCounterTest, adjustClassCountsTest4) {
+    adjustClassCountsTest(Lease::TYPE_V4);
+}
+
+// Tests getting and adjusting basic class counts for
+// a Lease::TYPE_NA and TYPE_PD.
+TEST_F(ClassLeaseCounterTest, adjustClassCountsTest6) {
+    adjustClassCountsTest(Lease::TYPE_NA);
+    adjustClassCountsTest(Lease::TYPE_PD);
+}
+
 // Tests getting and adjusting basic class counts for
 // a Lease::TYPE_NA and TYPE_PD.
 TEST_F(ClassLeaseCounterTest, basicCountingTests6) {
@@ -436,6 +469,110 @@ TEST_F(ClassLeaseCounterTest, basicCountingTests6) {
     EXPECT_EQ(0, count);
 }
 
+// Exercises ClassLeaseCounter::getLeaseClientClasses()
+// No need for v4 and v6 versions of this test, getLeaseClientClasses()
+// is not protocol specific.
+TEST_F(ClassLeaseCounterTest, getLeaseClientClassesTest) {
+    LeasePtr lease;
+
+    // Describes an invalid context scenario, that
+    // is expected to cause an exception throw.
+    struct InvalidScenario {
+        std::string ctx_json_;      // User context contents in JSON form.
+        std::string exp_message_;   // Expected exception text.
+    };
+
+    // Invalid context scenarios.
+    std::list<InvalidScenario> invalid_scenarios {
+        {
+          " \"bogus\" ",
+          "getLeaseClientClasses - invalid context: \"bogus\","
+          " find(string) called on a non-map Element in (<string>:1:2)"
+        },
+        {
+            "{\"ISC\": \"bogus\" }",
+            "getLeaseClientClasses - invalid context: {\n  \"ISC\": \"bogus\"\n},"
+            " find(string) called on a non-map Element in (<string>:1:9)"
+        },
+        {
+            "{\"ISC\": { \"client-classes\": \"bogus\" } }",
+            "getLeaseClientClasses - invalid context:"
+            " {\n  \"ISC\": {\n    \"client-classes\": \"bogus\"\n  }\n},"
+            " client-classes is not a list"
+        }
+    };
+
+    // Iterate over the invalid scenarios.
+    for (auto scenario : invalid_scenarios) {
+        // Cosntruct the lease and context.
+        lease = leaseFactory(Lease::TYPE_V4);
+        ElementPtr ctx;
+        ASSERT_NO_THROW(ctx = Element::fromJSON(scenario.ctx_json_))
+                        << "test is broken" << scenario.ctx_json_;
+        lease->setContext(ctx);
+
+        // Calling getLeaseClientClasses() should throw.
+        ASSERT_THROW_MSG(clc_.getLeaseClientClasses(lease), BadValue, scenario.exp_message_);
+    }
+
+    // Describes an valid context scenario, that is expected
+    // to return normally.
+    struct ValidScenario {
+        std::string ctx_json_;      // User context contents in JSON form.
+        std::string exp_classes_;   // Expected class list in JSON form.
+    };
+
+    // Valid scenarios.
+    std::list<ValidScenario> valid_scenarios {
+        {
+            // No context
+            "",
+            ""
+        },
+        {
+            // No client-classes element
+            "{\"ISC\": {} }",
+            ""
+        },
+        {
+            // Empty client-classes list
+            "{\"ISC\": { \"client-classes\": []} }",
+             "[]"
+        },
+        {
+            "{\"ISC\": { \"client-classes\": [ \"one\", \"two\", \"three\" ]} }",
+            "[ \"one\", \"two\", \"three\" ]"
+        }
+    };
+
+    // Iterate over the scenarios.
+    for (auto scenario : valid_scenarios) {
+        // Cosntruct the lease and context.
+        lease = leaseFactory(Lease::TYPE_V4);
+        if (!scenario.ctx_json_.empty()) {
+            ElementPtr ctx;
+            ASSERT_NO_THROW(ctx = Element::fromJSON(scenario.ctx_json_))
+                            << "test is broken" << scenario.ctx_json_;
+            lease->setContext(ctx);
+        }
+
+        // Call getLeaseClientClasses().
+        ConstElementPtr classes;
+        ASSERT_NO_THROW_LOG(classes = clc_.getLeaseClientClasses(lease));
+
+        // Verify we got the expected outcome for a class list.
+        if (scenario.exp_classes_.empty()) {
+            ASSERT_FALSE(classes);
+        } else {
+            ASSERT_TRUE(classes);
+            ElementPtr exp_classes;
+            ASSERT_NO_THROW(exp_classes = Element::fromJSON(scenario.exp_classes_))
+                            << "test is broken" << scenario.exp_classes_;
+            EXPECT_EQ(*classes, *exp_classes);
+        }
+    }
+}
+
 TEST_F(ClassLeaseCounterTest, addRemoveLeaseTest4) {
     addAndRemoveLeaseTest(leaseFactory(Lease::TYPE_V4));
 }