BIND 10 #2908: Python wrapper for ZoneTableAccessor and getZoneTableAccessor

[BIND 10 Development]

#2908: Python wrapper for ZoneTableAccessor and getZoneTableAccessor
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  pselkirk
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130611
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => pselkirk


Comment:

 Hello

 '''Interface'''

 I don't think the python interface needs the explicit methods, like
 `is_last`. I believe the python iteration protocol is enough and these
 methods would only confuse python programmers.

 Second, I know the ticket says so, but it seems to be that „zone table“
 sounds already like the container holding zones, so it should be possible
 to iterate directly over it, like:

 {{{#!python
 for zone in datasrc_clients.get_zone_table("sqlite3", True):
         # ...
 }}}

 The call of get_zones would suggest there's some other iterator to iterate
 by as well, something more common (like in case of dict, where you can
 iterate directly over it, or over its `.values()`).

 Also, the `get_zone_table` takes empty string to signify „whatever you
 want to give me“. It would make some sense to allow None there instead of
 empty string, which would look more the python way.

 '''Tests'''

 Why does this throw? I'm not claiming it should not, it is just unclear
 from the test why it should and it would require a comment.
 {{{#!python
         self.assertRaises(isc.datasrc.Error,
                           self.clist.get_zone_table, "", False)
         # bogus datasrc
         self.assertIsNone(self.clist.get_zone_table("bogus", True))
 }}}

 With the iterator tests, I think it should be tested inside the `for`
 construct too, not only by creating list from it. Also, it should be
 examined what is being returned as the values from the iterator, not only
 that it returns the right number of items.

 We probably want to test successful `get_zones` with giving name. Only
 unsuccessful and with empty name are tested.

 '''Potential problems'''

 Is it OK to delete the accessor before the client list it comes from? If
 it should not happen, we should ensure it with the `base_obj` trick like
 in the case of the iterator.

 In `ZoneTableIterator_next` (unless you agree to delete it), the catch-all
 omits setting the error string.

 You should be returning by py_ReturnTrue and pyReturFalse (not sure about
 the exact spelling) here, not by integer. Python has booleans.
 {{{#!c++
 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);
     }
 }}}

 There should be a wrapper for isc::dns::Name, returning string is wrong.
 {{{#!c++
     try {
         const isc::datasrc::ZoneSpec& zs = self->cppobj->getCurrent();
         return (Py_BuildValue("is", zs.index,
 zs.origin.toText().c_str()));
     } catch (const std::exception& ex) {
 }}}

 '''Style'''

 Things like this look awkward:
 {{{#!c++
 accessor.reset(Py_BuildValue(""));
 }}}

 Should it be just py_ReturnNone?

 If the python methods are named `get_zone_table` and `get_zones`, the C++
 wrapper functions should be renamed accordingly.

 The indentation looks wrong here. Is it a tab?:

 {{{
 +    def test_get_zone_table(self):
 +        """
 +       Test that we can get the zone table accessor and, thereby,
 +        the zone table iterator.
 +        """
 }}}

 The commented out includes should probably be removed.
 {{{#!c++
 +//#include <util/python/pycppwrapper_util.h>
 +//#include <datasrc/exceptions.h>
 +#include <datasrc/zone_table_accessor.h>
 }}}

 {{{#!c++
 +//#include <util/python/pycppwrapper_util.h>
 +#include <datasrc/zone_table_accessor.h>
 }}}

 The reset here is because it needed to be deleted before the base object.
 If there is no base object (and unless you need to return it, as mentioned
 above), it is useless:

 {{{#!c++
 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);
 }}}

 '''Documentation'''

 Generally, it would be good to read the documentation after copy-pasting
 it or generating it. Specifically:

 This still talks about accessors:
 {{{#!c++
 const char* const ZoneTableAccessor_doc = "\
 An accessor to a zone table for a data source.\n\
 \n\
 }}}

 This includes `base_obj` in the description, but the parameter is not
 there:
 {{{#!c++
 /// \brief Create a ZoneTableAccessor python object
 ///
 /// \param source The zone iterator pointer to wrap
 /// \param base_obj An optional PyObject that this ZoneFinder depends on
 ///                 Its refcount is increased, and will be decreased when
 ///                 this zone iterator is destroyed, making sure that the
 ///                 base object is never destroyed before this zonefinder.
 PyObject* createZoneTableAccessorObject(
     isc::datasrc::ConstZoneTableAccessorPtr source);
 }}}

 This talks about C++, while the documentation is for python:

 {{{#!c++
 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\
 C++ implementation will be different depending on the actual data source\
 used. This is the abstract interface.\n\
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/2908#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development