[trac979] Implement the creator and its tests

It takes some test utilities from inside the loader tests and places
them to a header, to be shared by this test as well.

This includes small fix of loader interface, some of its methods can
(and should) be const.

src/lib/acl/loader.h

@@ -263,7 +263,7 @@ public:
      * \param description The JSON description of the check.
      */
     boost::shared_ptr<Check<Context> > loadCheck(const data::ConstElementPtr&
-                                                 description)
+                                                 description) const
     {
         // Get the description as a map
         typedef std::map<std::string, data::ConstElementPtr> Map;
@@ -290,7 +290,7 @@ public:
      * \param description The JSON list of ACL.
      */
     boost::shared_ptr<ACL<Context, Action> > load(const data::ConstElementPtr&
-                                                  description)
+                                                  description) const
     {
         // We first check it's a list, so we can use the list reference
         // (the list may be huge)
@@ -346,7 +346,7 @@ private:
      *     the map.
      */
     boost::shared_ptr<Check<Context> > loadCheck(const data::ConstElementPtr&
-                                                 description, Map& map)
+                                                 description, Map& map) const
     {
         // Remove the action keyword
         map.erase("action");

src/lib/acl/logic_check.h

@@ -58,16 +58,8 @@ public:
  * reasons.
  */
 template<typename Mode, typename Context>
-class LogicOperator : public CompoundCheck<Context>, boost::noncopyable {
+class LogicOperator : public CompoundCheck<Context> {
 public:
-    /// Destructor
-    ~ LogicOperator() {
-        for (typename CompoundCheck<Context>::Checks::iterator
-                 i(checks_.begin());
-             i != checks_.end(); ++i) {
-            delete *i;
-        }
-    }
     /**
      * \brief Add another subexpression.
      *
@@ -79,19 +71,21 @@ public:
      * or to remove it. It might turn out it would be needed in future to
      * optimise or it might even turn out we need shared pointers for it.
      *
-     * \param expr The new expression to put inside. This takes ownership
-     *     of the object pointed to by expr (eg. if this function does not
-     *     throw - and it throws only std::badalloc, you don't delete it,
-     *     it will be deleted when the expression is deleted.
+     * \param expr The new expression to put inside.
      */
-    void addSubexpression(const Check<Context>* expr) {
+    void addSubexpression(const boost::shared_ptr<Check<Context> >& expr) {
         checks_.push_back(expr);
     }
     /**
      * \brief The current list of subexpressions.
      */
     virtual typename CompoundCheck<Context>::Checks getSubexpressions() const {
-        return (checks_);
+        typename CompoundCheck<Context>::Checks result;
+        for (typename Checks::const_iterator i(checks_.begin());
+             i != checks_.end(); ++i) {
+            result.push_back(&**i);
+        }
+        return (result);
     }
     /**
      * \brief The match of the check.
@@ -112,8 +106,7 @@ public:
          * return that one right away. If it meets no such expression, we
          * get to the end and return the default.
          */
-        for (typename CompoundCheck<Context>::Checks::const_iterator
-                 i(checks_.begin());
+        for (typename Checks::const_iterator i(checks_.begin());
              i != checks_.end(); ++i) {
             if (Mode::terminate((*i)->matches(context))) {
                 return (!Mode::start());
@@ -123,20 +116,88 @@ public:
     }
 private:
     /// \brief List of subexpressions
-    typename CompoundCheck<Context>::Checks checks_;
+    typedef typename std::vector<boost::shared_ptr<Check<Context> > > Checks;
+    Checks checks_;
 };
 
+/**
+ * \brief Creator for the LogicOperator compound check.
+ *
+ * This class can load the ANY and ALL operators from JSON. They expect
+ * a list of subexpressions as a parameter, eg. like this:
+ *
+ * \verbatim
+ * {"ANY": [
+ *    {"ip": "1.2.3.4"},
+ *    {"ip": "5.6.7.8"}
+ * ]}
+ * \endverbatim
+ *
+ * It uses the loader to load the subexpressions, therefore whatever is
+ * supported there is supported here as well.
+ *
+ * The Mode template parameter has the same meaning as with LogicOperator,
+ * it is used to know which operators to create.
+ */
 template<typename Mode, typename Context, typename Action = BasicAction>
 class LogicCreator : public Loader<Context, Action>::CheckCreator {
 public:
-    LogicCreator(const std::string& name);
-    virtual std::vector<std::string> names() const;
+    /**
+     * \brief Constructor.
+     *
+     * \param name The name for which the loader will work. In practice,
+     *     it will usually be ANY or ALL (depending on the mode), but
+     *     anything else can be used as well.
+     */
+    LogicCreator(const std::string& name) :
+        name_(name)
+    {}
+    /// \brief Returns vector containing the name.
+    virtual std::vector<std::string> names() const {
+        std::vector<std::string> result;
+        result.push_back(name_);
+        return (result);
+    }
+    /**
+     * \brief Converts a JSON description into the logic operator.
+     *
+     * This is the place where the actual loading happens. It creates
+     * the logic operator and calls the loader on each of the list
+     * elements, placing the result into the logic operator.
+     *
+     * The first parameter is ignored and is there only to match interface.
+     *
+     * \param definition The JSON definition of the subexpressions. This must
+     *     be a list (if it isn't, the LoaderError is thrown) and the elements
+     *     must be loadable by the loader (the exceptions from it are not
+     *     caught).
+     * \param loader The loader to use for loading of subexpressions.
+     */
     virtual boost::shared_ptr<Check<Context> > create(const std::string&,
                                                       data::ConstElementPtr
                                                       definition,
                                                       const Loader<Context,
-                                                      Action>& loader);
+                                                      Action>& loader)
+    {
+        std::vector<data::ConstElementPtr> subexprs;
+        try {
+            subexprs = definition->listValue();
+        }
+        catch (const data::TypeError&) {
+            isc_throw_1(LoaderError, "Logic operator takes list", definition);
+        }
+        boost::shared_ptr<LogicOperator<Mode, Context> >
+            result(new LogicOperator<Mode, Context>);
+        for (std::vector<data::ConstElementPtr>::const_iterator
+                 i(subexprs.begin());
+             i != subexprs.end(); ++i) {
+            result->addSubexpression(loader.loadCheck(*i));
+        }
+        return (result);
+    }
     virtual bool allowListAbbreviation() const { return (false); }
+private:
+    const std::string name_;
 };
 
 }

src/lib/acl/tests/creators.h

@@ -0,0 +1,154 @@
+// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+// This is not a public header, but some code shared between tests
+// This one contains various creators to test the loader and other creators
+
+#ifndef CREATORS_H
+#define CREATORS_H
+
+#include "logcheck.h"
+#include <acl/loader.h>
+#include <string>
+
+using isc::data::ConstElementPtr;
+using namespace std;
+using namespace boost;
+
+namespace {
+
+// Just for convenience, create JSON objects from JSON string
+ConstElementPtr el(const string& JSON) {
+    return (isc::data::Element::fromJSON(JSON));
+}
+
+// A check that doesn't check anything but remembers it's own name
+// and data
+class NamedCheck : public Check<Log> {
+public:
+    NamedCheck(const string& name, ConstElementPtr data) :
+        name_(name),
+        data_(data)
+    {}
+    virtual bool matches(const Log&) const { return (true); }
+    const string name_;
+    const ConstElementPtr data_;
+};
+
+// The creator of NamedCheck
+class NamedCreator : public Loader<Log>::CheckCreator {
+public:
+    NamedCreator(const string& name, bool abbreviatedList = true) :
+        abbreviated_list_(abbreviatedList)
+    {
+        names_.push_back(name);
+    }
+    NamedCreator(const vector<string>& names) :
+        names_(names),
+        abbreviated_list_(true)
+    {}
+    vector<string> names() const {
+        return (names_);
+    }
+    shared_ptr<Check<Log> > create(const string& name, ConstElementPtr data,
+                                   const Loader<Log>&)
+    {
+        bool found(false);
+        for (vector<string>::const_iterator i(names_.begin());
+             i != names_.end(); ++i) {
+            if (*i == name) {
+                found = true;
+                break;
+            }
+        }
+        EXPECT_TRUE(found) << "Name " << name << " passed to creator which "
+            "doesn't handle it.";
+        return (shared_ptr<Check<Log> >(new NamedCheck(name, data)));
+    }
+    bool allowListAbbreviation() const {
+        return (abbreviated_list_);
+    }
+private:
+    vector<string> names_;
+    const bool abbreviated_list_;
+};
+
+// To be thrown in tests internally
+class TestCreatorError {};
+
+// This will throw every time it should create something
+class ThrowCreator : public Loader<Log>::CheckCreator {
+public:
+    vector<string> names() const {
+        vector<string> result;
+        result.push_back("throw");
+        return (result);
+    }
+    shared_ptr<Check<Log> > create(const string&, ConstElementPtr,
+                                   const Loader<Log>&)
+    {
+        throw TestCreatorError();
+    }
+};
+
+// This throws whenever the match is called on it
+class ThrowCheck : public Check<Log> {
+public:
+    virtual bool matches(const Log&) const {
+        throw TestCreatorError();
+    }
+};
+
+// And creator for it
+class ThrowCheckCreator : public Loader<Log>::CheckCreator {
+public:
+    vector<string> names() const {
+        vector<string> result;
+        result.push_back("throwcheck");
+        return (result);
+    }
+    shared_ptr<Check<Log> > create(const string&, ConstElementPtr,
+                                   const Loader<Log>&)
+    {
+        return (shared_ptr<Check<Log> >(new ThrowCheck()));
+    }
+};
+
+class LogCreator : public Loader<Log>::CheckCreator {
+public:
+    vector<string> names() const {
+        vector<string> result;
+        result.push_back("logcheck");
+        return (result);
+    }
+    /*
+     * For simplicity, we just take two values as a list, first is the
+     * logging cell used, the second is result of the check. No error checking
+     * is done, if there's bug in the test, it will throw TypeError for us.
+     */
+    shared_ptr<Check<Log> > create(const string&, ConstElementPtr definition,
+                                   const Loader<Log>&)
+    {
+        vector<ConstElementPtr> list(definition->listValue());
+        int logpos(list[0]->intValue());
+        bool accept(list[1]->boolValue());
+        return (shared_ptr<ConstCheck>(new ConstCheck(accept, logpos)));
+    }
+    // We take a list, so don't interpret it for us
+    virtual bool allowListAbbreviation() const { return (false); }
+};
+
+}
+
+#endif

src/lib/acl/tests/loader_test.cc

@@ -12,22 +12,16 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include "logcheck.h"
+#include "creators.h"
 #include <acl/loader.h>
 #include <string>
 #include <gtest/gtest.h>
 
 using namespace std;
 using namespace boost;
-using isc::data::ConstElementPtr;
 
 namespace {
 
-// Just for convenience, create JSON objects from JSON string
-ConstElementPtr el(const string& JSON) {
-    return (isc::data::Element::fromJSON(JSON));
-}
-
 // We don't use the EXPECT_THROW macro, as it doesn't allow us
 // to examine the exception. We want to check the element is stored
 // there as well.
@@ -61,122 +55,6 @@ TEST(LoaderHelpers, DefaultActionLoader) {
     testActionLoaderException("{}");
 }
 
-// A check that doesn't check anything but remembers it's own name
-// and data
-class NamedCheck : public Check<Log> {
-public:
-    NamedCheck(const string& name, ConstElementPtr data) :
-        name_(name),
-        data_(data)
-    {}
-    virtual bool matches(const Log&) const { return (true); }
-    const string name_;
-    const ConstElementPtr data_;
-};
-
-// The creator of NamedCheck
-class NamedCreator : public Loader<Log>::CheckCreator {
-public:
-    NamedCreator(const string& name, bool abbreviatedList = true) :
-        abbreviated_list_(abbreviatedList)
-    {
-        names_.push_back(name);
-    }
-    NamedCreator(const vector<string>& names) :
-        names_(names),
-        abbreviated_list_(true)
-    {}
-    vector<string> names() const {
-        return (names_);
-    }
-    shared_ptr<Check<Log> > create(const string& name, ConstElementPtr data,
-                                   const Loader<Log>&)
-    {
-        bool found(false);
-        for (vector<string>::const_iterator i(names_.begin());
-             i != names_.end(); ++i) {
-            if (*i == name) {
-                found = true;
-                break;
-            }
-        }
-        EXPECT_TRUE(found) << "Name " << name << " passed to creator which "
-            "doesn't handle it.";
-        return (shared_ptr<Check<Log> >(new NamedCheck(name, data)));
-    }
-    bool allowListAbbreviation() const {
-        return (abbreviated_list_);
-    }
-private:
-    vector<string> names_;
-    const bool abbreviated_list_;
-};
-
-// To be thrown in tests internally
-class TestCreatorError {};
-
-// This will throw every time it should create something
-class ThrowCreator : public Loader<Log>::CheckCreator {
-public:
-    vector<string> names() const {
-        vector<string> result;
-        result.push_back("throw");
-        return (result);
-    }
-    shared_ptr<Check<Log> > create(const string&, ConstElementPtr,
-                                   const Loader<Log>&)
-    {
-        throw TestCreatorError();
-    }
-};
-
-// This throws whenever the match is called on it
-class ThrowCheck : public Check<Log> {
-public:
-    virtual bool matches(const Log&) const {
-        throw TestCreatorError();
-    }
-};
-
-// And creator for it
-class ThrowCheckCreator : public Loader<Log>::CheckCreator {
-public:
-    vector<string> names() const {
-        vector<string> result;
-        result.push_back("throwcheck");
-        return (result);
-    }
-    shared_ptr<Check<Log> > create(const string&, ConstElementPtr,
-                                   const Loader<Log>&)
-    {
-        return (shared_ptr<Check<Log> >(new ThrowCheck()));
-    }
-};
-
-class LogCreator : public Loader<Log>::CheckCreator {
-public:
-    vector<string> names() const {
-        vector<string> result;
-        result.push_back("logcheck");
-        return (result);
-    }
-    /*
-     * For simplicity, we just take two values as a list, first is the
-     * logging cell used, the second is result of the check. No error checking
-     * is done, if there's bug in the test, it will throw TypeError for us.
-     */
-    shared_ptr<Check<Log> > create(const string&, ConstElementPtr definition,
-                                   const Loader<Log>&)
-    {
-        vector<ConstElementPtr> list(definition->listValue());
-        int logpos(list[0]->intValue());
-        bool accept(list[1]->boolValue());
-        return (shared_ptr<ConstCheck>(new ConstCheck(accept, logpos)));
-    }
-    // We take a list, so don't interpret it for us
-    virtual bool allowListAbbreviation() const { return (false); }
-};
-
 class LoaderTest : public ::testing::Test {
 public:
     LoaderTest() :

src/lib/acl/tests/logcheck.h

@@ -12,6 +12,9 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
+#ifndef LOGCHECK_H
+#define LOGCHECK_H
+
 #include <gtest/gtest.h>
 #include <acl/acl.h>
 #include <cassert>
@@ -84,3 +87,5 @@ private:
 };
 
 }
+
+#endif

src/lib/acl/tests/logic_check_test.cc

@@ -12,7 +12,7 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include "logcheck.h"
+#include "creators.h"
 #include <acl/logic_check.h>
 
 using namespace isc::acl;
@@ -46,10 +46,15 @@ testCheck(bool emptyResult) {
     EXPECT_EQ(emptyResult, oper.matches(log));
     log.checkFirst(0);
     // Fill it with some subexpressions
-    oper.addSubexpression(new ConstCheck(emptyResult, 0));
-    oper.addSubexpression(new ConstCheck(emptyResult, 1));
-    oper.addSubexpression(new ConstCheck(!emptyResult, 2));
-    oper.addSubexpression(new ConstCheck(!emptyResult, 3));
+    typedef shared_ptr<ConstCheck> CheckPtr;
+    oper.addSubexpression(CheckPtr(new ConstCheck(emptyResult, 0)));
+    oper.addSubexpression(CheckPtr(new ConstCheck(emptyResult, 1)));
+    // Check what happens when only the default-valued are there
+    EXPECT_EQ(2, oper.getSubexpressions().size());
+    EXPECT_EQ(emptyResult, oper.matches(log));
+    log.checkFirst(2);
+    oper.addSubexpression(CheckPtr(new ConstCheck(!emptyResult, 2)));
+    oper.addSubexpression(CheckPtr(new ConstCheck(!emptyResult, 3)));
     // They are listed there
     EXPECT_EQ(4, oper.getSubexpressions().size());
     // Now, the last one kills it, but the first ones will run, the fourth
@@ -66,4 +71,96 @@ TEST(LogicOperators, AnyOf) {
     testCheck<AnyOfSpec>(false);
 }
 
+// Fixture for the tests of the creators
+class LogicCreatorTest : public ::testing::Test {
+private:
+    typedef shared_ptr<Loader<Log>::CheckCreator> CreatorPtr;
+public:
+    // Register some creators, both tested ones and some auxiliary ones for
+    // help
+    LogicCreatorTest():
+        loader_(REJECT)
+    {
+        loader_.registerCreator(CreatorPtr(new
+            LogicCreator<AnyOfSpec, Log>("ANY")));
+        loader_.registerCreator(CreatorPtr(new
+            LogicCreator<AllOfSpec, Log>("ALL")));
+        loader_.registerCreator(CreatorPtr(new ThrowCreator));
+        loader_.registerCreator(CreatorPtr(new LogCreator));
+    }
+    // To mark which parts of the check did run
+    Log log_;
+    // The loader
+    Loader<Log> loader_;
+    // Some convenience shortcut names
+    typedef LogicOperator<AnyOfSpec, Log> AnyOf;
+    typedef LogicOperator<AllOfSpec, Log> AllOf;
+    typedef shared_ptr<AnyOf> AnyOfPtr;
+    typedef shared_ptr<AllOf> AllOfPtr;
+    // Loads the JSON as a check and tries to convert it to the given check
+    // subclass
+    template<typename Result> shared_ptr<Result> load(const string& JSON) {
+        shared_ptr<Check<Log> > result;
+        EXPECT_NO_THROW(result = loader_.loadCheck(el(JSON)));
+        shared_ptr<Result>
+            resultConverted(dynamic_pointer_cast<Result>(result));
+        EXPECT_NE(shared_ptr<Result>(), resultConverted);
+        return (resultConverted);
+    }
+};
+
+// Test it can load empty ones
+TEST_F(LogicCreatorTest, empty) {
+    AnyOfPtr emptyAny(load<AnyOf>("{\"ANY\": []}"));
+    EXPECT_EQ(0, emptyAny->getSubexpressions().size());
+    AllOfPtr emptyAll(load<AllOf>("{\"ALL\": []}"));
+    EXPECT_EQ(0, emptyAll->getSubexpressions().size());
+}
+
+// Test it rejects invalid inputs (not a list as a parameter)
+TEST_F(LogicCreatorTest, invalid) {
+    EXPECT_THROW(loader_.loadCheck(el("{\"ANY\": null}")), LoaderError);
+    EXPECT_THROW(loader_.loadCheck(el("{\"ANY\": {}}")), LoaderError);
+    EXPECT_THROW(loader_.loadCheck(el("{\"ANY\": true}")), LoaderError);
+    EXPECT_THROW(loader_.loadCheck(el("{\"ANY\": 42}")), LoaderError);
+    EXPECT_THROW(loader_.loadCheck(el("{\"ANY\": \"hello\"}")), LoaderError);
+    EXPECT_THROW(loader_.loadCheck(el("{\"ALL\": null}")), LoaderError);
+    EXPECT_THROW(loader_.loadCheck(el("{\"ALL\": {}}")), LoaderError);
+    EXPECT_THROW(loader_.loadCheck(el("{\"ALL\": true}")), LoaderError);
+    EXPECT_THROW(loader_.loadCheck(el("{\"ALL\": 42}")), LoaderError);
+    EXPECT_THROW(loader_.loadCheck(el("{\"ALL\": \"hello\"}")), LoaderError);
+}
+
+// Exceptions from subexpression creation isn't caught
+TEST_F(LogicCreatorTest, propagate) {
+    EXPECT_THROW(loader_.loadCheck(el("{\"ANY\": [{\"throw\": null}]}")),
+                 TestCreatorError);
+    EXPECT_THROW(loader_.loadCheck(el("{\"ALL\": [{\"throw\": null}]}")),
+                 TestCreatorError);
+}
+
+// We can create more complex ANY check and run it correctly
+TEST_F(LogicCreatorTest, anyRun) {
+    AnyOfPtr any(load<AnyOf>("{\"ANY\": ["
+                             "    {\"logcheck\": [0, false]},"
+                             "    {\"logcheck\": [1, true]},"
+                             "    {\"logcheck\": [2, true]}"
+                             "]}"));
+    EXPECT_EQ(3, any->getSubexpressions().size());
+    EXPECT_TRUE(any->matches(log_));
+    log_.checkFirst(2);
+}
+
+// We can create more complex ALL check and run it correctly
+TEST_F(LogicCreatorTest, allRun) {
+    AllOfPtr any(load<AllOf>("{\"ALL\": ["
+                             "    {\"logcheck\": [0, true]},"
+                             "    {\"logcheck\": [1, false]},"
+                             "    {\"logcheck\": [2, false]}"
+                             "]}"));
+    EXPECT_EQ(3, any->getSubexpressions().size());
+    EXPECT_FALSE(any->matches(log_));
+    log_.checkFirst(2);
+}
+
 }