[2908] numerous fixes from review

- remove ZoneTableIterator.next, is_last
- rename get_zone_table -> get_zone_table_accessor
- rename get_zones -> get_iterator
- allow get_zone_table_accessor(None) instead of "" for "any data source"
- add s_ZoneTableAccessor.base_obj to ensure in-order destruction
- get_current returns Name instead of string
- additional test cases for iterator
- general code cleanup and doc cleanup

src/lib/python/isc/datasrc/configurableclientlist_python.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013  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
@@ -178,16 +178,17 @@ ConfigurableClientList_getZoneTableAccessor(PyObject* po_self, PyObject* args) {
     try {
         const char* datasrc_name;
         int use_cache;
-        if (PyArg_ParseTuple(args, "si", &datasrc_name, &use_cache)) {
+        if (PyArg_ParseTuple(args, "zi", &datasrc_name, &use_cache)) {
+            // python 'None' will be read as NULL, which we convert to an
+            // empty string, meaning "any data source"
+            const std::string name(datasrc_name ? datasrc_name : "");
             const ConstZoneTableAccessorPtr
-                z(self->cppobj->getZoneTableAccessor(datasrc_name, use_cache));
+                z(self->cppobj->getZoneTableAccessor(name, use_cache));
-            PyObjectContainer accessor;
             if (z == NULL) {
-                accessor.reset(Py_BuildValue(""));
+                Py_RETURN_NONE;
             } else {
-                accessor.reset(createZoneTableAccessorObject(z));
+                return (createZoneTableAccessorObject(z, po_self));
             }
-            return (Py_BuildValue("O", accessor.get()));
         } else {
             return (NULL);
         }
@@ -243,9 +244,10 @@ you don't need it, but if you do need it, it is better to set it to True\
 instead of getting it from the datasrc_client later.\n\
 \n\
 If no answer is found, the datasrc_client and zone_finder are None." },
-    { "get_zone_table", ConfigurableClientList_getZoneTableAccessor,
+    { "get_zone_table_accessor", ConfigurableClientList_getZoneTableAccessor,
       METH_VARARGS,
-"get_zone_table(datasrc_name, use_cache) -> isc.datasrc.ZoneTableAccessor\n\
+"get_zone_table_accessor(datasrc_name, use_cache) -> \
+isc.datasrc.ZoneTableAccessor\n\
 \n\
 Create a ZoneTableAccessor object for the specified data source.\n\
 \n\

src/lib/python/isc/datasrc/tests/clientlist_test.py

@@ -151,16 +151,16 @@ class ClientListTest(unittest.TestCase):
         self.assertRaises(TypeError, self.clist.find, "example.org")
         self.assertRaises(TypeError, self.clist.find)
 
-    def test_get_zone_table(self):
+    def test_get_zone_table_accessor(self):
         """
-       Test that we can get the zone table accessor and, thereby,
+        Test that we can get the zone table accessor and, thereby,
         the zone table iterator.
         """
         self.clist = isc.datasrc.ConfigurableClientList(isc.dns.RRClass.IN)
 
         # null configuration
         self.clist.configure("[]", True)
-        self.assertIsNone(self.clist.get_zone_table("", True))
+        self.assertIsNone(self.clist.get_zone_table_accessor(None, True))
 
         # empty configuration
         self.clist.configure('''[{
@@ -169,15 +169,13 @@ class ClientListTest(unittest.TestCase):
             "cache-enable": true
         }]''', True)
         # bogus datasrc
-        self.assertIsNone(self.clist.get_zone_table("bogus", True))
+        self.assertIsNone(self.clist.get_zone_table_accessor("bogus", True))
         # first datasrc - empty zone table
-        table = self.clist.get_zone_table("", True)
+        table = self.clist.get_zone_table_accessor(None, True)
         self.assertIsNotNone(table)
-        zones = table.get_zones()
+        iterator = table.get_iterator()
-        self.assertIsNotNone(zones)
+        self.assertIsNotNone(iterator)
-        self.assertTrue(zones.is_last())
+        self.assertRaises(isc.datasrc.Error, iterator.get_current)
-        self.assertRaises(isc.datasrc.Error, zones.get_current)
-        self.assertRaises(isc.datasrc.Error, zones.next)
 
         # normal configuration
         self.clist.configure('''[{
@@ -187,31 +185,27 @@ class ClientListTest(unittest.TestCase):
             },
             "cache-enable": true
         }]''', True)
-        # use_cache = false
+        # !use_cache => NotImplemented
         self.assertRaises(isc.datasrc.Error,
-                          self.clist.get_zone_table, "", False)
+                          self.clist.get_zone_table_accessor, None, False)
         # bogus datasrc
-        self.assertIsNone(self.clist.get_zone_table("bogus", True))
+        self.assertIsNone(self.clist.get_zone_table_accessor("bogus", True))
 
         # first datasrc
-        table = self.clist.get_zone_table("", True)
+        table = self.clist.get_zone_table_accessor(None, True)
         self.assertIsNotNone(table)
-        zones = table.get_zones()
+        iterator = table.get_iterator()
-        self.assertIsNotNone(zones)
+        self.assertIsNotNone(iterator)
-        self.assertFalse(zones.is_last())
+        index, origin = iterator.get_current()
-        index, origin = zones.get_current()
+        self.assertEqual(origin.to_text(), "example.org.")
-        self.assertEqual(origin, "example.org.")
+        self.assertEqual(1, len(list(iterator)))
-        zones.next()
-        self.assertTrue(zones.is_last())
-        # reset iterator and count zones
-        zones = table.get_zones()
-        self.assertEqual(1, len(list(zones)))
 
         # named datasrc
-        zones = self.clist.get_zone_table("MasterFiles", True).get_zones()
+        table = self.clist.get_zone_table_accessor("MasterFiles", True)
-        index, origin = zones.get_current()
+        iterator = table.get_iterator()
-        self.assertEqual(origin, "example.org.")
+        index, origin = iterator.get_current()
-        self.assertEqual(1, len(list(zones)))
+        self.assertEqual(origin.to_text(), "example.org.")
+        self.assertEqual(1, len(list(iterator)))
 
         # longer zone list for non-trivial iteration
         self.clist.configure('''[{
@@ -225,9 +219,23 @@ class ClientListTest(unittest.TestCase):
             },
             "cache-enable": true
         }]''', True)
-        zonelist = list(self.clist.get_zone_table("", True).get_zones())
+        zonelist = list(self.clist.get_zone_table_accessor(None, True).
+                        get_iterator())
         self.assertEqual(5, len(zonelist))
-        self.assertTrue((0, "example.net.") in zonelist)
+        self.assertTrue((0, isc.dns.Name("example.net.")) in zonelist)
+
+        # ensure the iterator returns exactly and only the zones we expect
+        zonelist = [
+            isc.dns.Name("example.org"),
+            isc.dns.Name("example.com"),
+            isc.dns.Name("example.net"),
+            isc.dns.Name("example.biz"),
+            isc.dns.Name("example.edu")]
+        table = self.clist.get_zone_table_accessor("MasterFiles", True)
+        for index, zone in table.get_iterator():
+            self.assertTrue(zone in zonelist)
+            zonelist.remove(zone)
+        self.assertEqual(0, len(zonelist))                
 
 if __name__ == "__main__":
     isc.log.init("bind10")

src/lib/python/isc/datasrc/zonetable_accessor_python.cc

@@ -20,8 +20,6 @@
 // http://docs.python.org/py3k/extending/extending.html#a-simple-example
 #include <Python.h>
 
-//#include <util/python/pycppwrapper_util.h>
-//#include <datasrc/exceptions.h>
 #include <datasrc/zone_table_accessor.h>
 
 #include "datasrc.h"
@@ -38,8 +36,36 @@ class s_ZoneTableAccessor : public PyObject {
 public:
     s_ZoneTableAccessor() : cppobj(ConstZoneTableAccessorPtr()) {};
     ConstZoneTableAccessorPtr cppobj;
+    // This is a reference to a base object; if the object of this class
+    // depends on another object to be in scope during its lifetime,
+    // we use INCREF the base object upon creation, and DECREF it at
+    // the end of the destructor
+    // This is an optional argument to createXXX(). If NULL, it is ignored.
+    PyObject* base_obj;
 };
 
+int
+ZoneTableAccessor_init(PyObject*, PyObject*, PyObject*) {
+    // can't be called directly
+    PyErr_SetString(PyExc_TypeError,
+                    "ZoneTableAccessor cannot be constructed directly");
+
+    return (-1);
+}
+
+void
+ZoneTableAccessor_destroy(PyObject* po_self) {
+    s_ZoneTableAccessor* const self =
+        static_cast<s_ZoneTableAccessor*>(po_self);
+    // cppobj is a shared ptr, but to make sure things are not destroyed in
+    // the wrong order, we reset it here.
+    self->cppobj.reset();
+    if (self->base_obj != NULL) {
+        Py_DECREF(self->base_obj);
+    }
+    Py_TYPE(self)->tp_free(self);
+}
+
 PyObject*
 ZoneTableAccessor_getIterator(PyObject* po_self, PyObject* args) {
     s_ZoneTableAccessor* const self =
@@ -64,9 +90,9 @@ ZoneTableAccessor_getIterator(PyObject* po_self, PyObject* args) {
 // 3. Argument type
 // 4. Documentation
 PyMethodDef ZoneTableAccessor_methods[] = {
-    { "get_zones",
+    { "get_iterator",
       ZoneTableAccessor_getIterator, METH_NOARGS,
-"get_zones() -> isc.datasrc.ZoneTableIterator\n\
+"get_iterator() -> isc.datasrc.ZoneTableIterator\n\
 \n\
 Return a zone table iterator.\n\
 \n" },
@@ -79,25 +105,6 @@ An accessor to a zone table for a data source.\n\
 This class object is intended to be used by applications that load zones\
 into memory, so that the application can get a list of zones to be loaded.";
 
-int
-ZoneTableAccessor_init(PyObject*, PyObject*, PyObject*) {
-    // can't be called directly
-    PyErr_SetString(PyExc_TypeError,
-                    "ZoneTableAccessor cannot be constructed directly");
-
-    return (-1);
-}
-
-void
-ZoneTableAccessor_destroy(PyObject* po_self) {
-    s_ZoneTableAccessor* const self =
-        static_cast<s_ZoneTableAccessor*>(po_self);
-    // cppobj is a shared ptr, but to make sure things are not destroyed in
-    // the wrong order, we reset it here.
-    self->cppobj.reset();
-    Py_TYPE(self)->tp_free(self);
-}
-
 } // end anonymous namespace
 
 namespace isc {
@@ -157,11 +164,17 @@ PyTypeObject zonetableaccessor_type = {
 };
 
 PyObject*
-createZoneTableAccessorObject(isc::datasrc::ConstZoneTableAccessorPtr source) {
+createZoneTableAccessorObject(isc::datasrc::ConstZoneTableAccessorPtr source,
+                              PyObject* base_obj)
+{
     s_ZoneTableAccessor* py_zt = static_cast<s_ZoneTableAccessor*>(
         zonetableaccessor_type.tp_alloc(&zonetableaccessor_type, 0));
     if (py_zt != NULL) {
         py_zt->cppobj = source;
+        py_zt->base_obj = base_obj;
+        if (base_obj != NULL) {
+            Py_INCREF(base_obj);
+        }
     }
     return (py_zt);
 }

src/lib/python/isc/datasrc/zonetable_accessor_python.h

@@ -31,7 +31,7 @@ extern PyTypeObject zonetableaccessor_type;
 ///                 this zone iterator is destroyed, making sure that the
 ///                 base object is never destroyed before this zonefinder.
 PyObject* createZoneTableAccessorObject(
-    isc::datasrc::ConstZoneTableAccessorPtr source);
+    isc::datasrc::ConstZoneTableAccessorPtr source, PyObject* base_obj);
 
 } // namespace python
 } // namespace datasrc

src/lib/python/isc/datasrc/zonetable_iterator_python.cc

@@ -20,8 +20,8 @@
 // http://docs.python.org/py3k/extending/extending.html#a-simple-example
 #include <Python.h>
 
-//#include <util/python/pycppwrapper_util.h>
 #include <datasrc/zone_table_accessor.h>
+#include <dns/python/name_python.h>
 
 #include "datasrc.h"
 #include "zonetable_iterator_python.h"
@@ -73,38 +73,12 @@ ZoneTableIterator_destroy(s_ZoneTableIterator* const self) {
 // the type definition of the object, since both can use the other
 //
 PyObject*
-ZoneTableIterator_isLast(PyObject* po_self, PyObject*) {
-    s_ZoneTableIterator* self = static_cast<s_ZoneTableIterator*>(po_self);
-    try {
-        return (Py_BuildValue("i", self->cppobj->isLast()));
-    } catch (...) {
-        return (NULL);
-    }
-}
-
-PyObject*
-ZoneTableIterator_next(PyObject* po_self, PyObject*) {
-    s_ZoneTableIterator* self = static_cast<s_ZoneTableIterator*>(po_self);
-    try {
-        self->cppobj->next();
-        return (Py_BuildValue(""));
-    } catch (const std::exception& ex) {
-        // isc::InvalidOperation is thrown when we call next()
-        // when we are already done iterating ('iterating past end')
-        // We could also simply return None again
-        PyErr_SetString(getDataSourceException("Error"), ex.what());
-        return (NULL);
-    } catch (...) {
-        return (NULL);
-    }
-}
-
-PyObject*
 ZoneTableIterator_getCurrent(PyObject* po_self, PyObject*) {
     s_ZoneTableIterator* self = static_cast<s_ZoneTableIterator*>(po_self);
     try {
         const isc::datasrc::ZoneSpec& zs = self->cppobj->getCurrent();
-        return (Py_BuildValue("is", zs.index, zs.origin.toText().c_str()));
+        return (Py_BuildValue("iO", zs.index,
+                              isc::dns::python::createNameObject(zs.origin)));
     } catch (const std::exception& ex) {
         // isc::InvalidOperation is thrown when we call getCurrent()
         // when we are already done iterating ('iterating past end')
@@ -119,13 +93,13 @@ ZoneTableIterator_getCurrent(PyObject* po_self, PyObject*) {
 }
 
 PyObject*
-ZoneTableIterator_piter(PyObject *self) {
+ZoneTableIterator_iter(PyObject *self) {
     Py_INCREF(self);
     return (self);
 }
 
 PyObject*
-ZoneTableIterator_pnext(PyObject* po_self) {
+ZoneTableIterator_next(PyObject* po_self) {
     s_ZoneTableIterator* self = static_cast<s_ZoneTableIterator*>(po_self);
     if (!self->cppobj || self->cppobj->isLast()) {
         return (NULL);
@@ -145,14 +119,6 @@ ZoneTableIterator_pnext(PyObject* po_self) {
 }
 
 PyMethodDef ZoneTableIterator_methods[] = {
-    { "is_last", ZoneTableIterator_isLast, METH_NOARGS,
-"is_last() -> bool\n\
-\n\
-Return whether the iterator is at the end of the zone table.\n" },
-    { "next", ZoneTableIterator_next, METH_NOARGS,
-"next()\n\
-\n\
-Move the iterator to the next zone of the table.\n" },
     { "get_current", ZoneTableIterator_getCurrent, METH_NOARGS,
 "get_current() -> isc.datasrc.ZoneSpec\n\
 \n\
@@ -168,9 +134,7 @@ const char* const ZoneTableIterator_doc = "\
 Read-only iterator to a zone table.\n\
 \n\
 You can get an instance of the ZoneTableIterator from the\
-ZoneTableAccessor.get_iterator() method. The actual concrete\
+ZoneTableAccessor.get_iterator() method.\n\
-C++ implementation will be different depending on the actual data source\
-used. This is the abstract interface.\n\
 \n\
 There's no way to start iterating from the beginning again or return.\n\
 \n\
@@ -209,8 +173,8 @@ PyTypeObject zonetableiterator_type = {
     NULL,                               // tp_clear
     NULL,                               // tp_richcompare
     0,                                  // tp_weaklistoffset
-    ZoneTableIterator_piter,            // tp_iter
+    ZoneTableIterator_iter,             // tp_iter
-    ZoneTableIterator_pnext,            // tp_iternext
+    ZoneTableIterator_next,             // tp_iternext
     ZoneTableIterator_methods,          // tp_methods
     NULL,                               // tp_members
     NULL,                               // tp_getset