[2379] Add more tests and catches

And update doc and do a bit of style cleanup

src/lib/python/isc/datasrc/Makefile.am

@@ -36,6 +36,7 @@ EXTRA_DIST += finder_inc.cc
 EXTRA_DIST += iterator_inc.cc
 EXTRA_DIST += updater_inc.cc
 EXTRA_DIST += journal_reader_inc.cc
+EXTRA_DIST += zone_loader_inc.cc
 
 CLEANDIRS = __pycache__
 

src/lib/python/isc/datasrc/tests/Makefile.am

@@ -10,7 +10,10 @@ EXTRA_DIST += testdata/example.com.sqlite3
 EXTRA_DIST += testdata/newschema.sqlite3
 EXTRA_DIST += testdata/oldschema.sqlite3
 EXTRA_DIST += testdata/new_minor_schema.sqlite3
+EXTRA_DIST += testdata/example.com
+EXTRA_DIST += testdata/example.com.ch
 CLEANFILES = $(abs_builddir)/rwtest.sqlite3.copied
+CLEANFILES += $(abs_builddir)/zoneloadertest.sqlite3
 
 # If necessary (rare cases), explicitly specify paths to dynamic libraries
 # required by loadable python modules.

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

@@ -26,10 +26,12 @@ TESTDATA_PATH = os.environ['TESTDATA_PATH']
 TESTDATA_WRITE_PATH = os.environ['TESTDATA_WRITE_PATH']
 
 ZONE_FILE = TESTDATA_PATH + '/example.com'
-
+STATIC_ZONE_FILE = '../../../../datasrc/static.zone'
+SOURCE_DB_FILE = TESTDATA_PATH + '/example.com.source.sqlite3'
 ORIG_DB_FILE = TESTDATA_PATH + '/example.com.sqlite3'
 DB_FILE = TESTDATA_WRITE_PATH + '/zoneloadertest.sqlite3'
 DB_CLIENT_CONFIG = '{ "database_file": "' + DB_FILE + '"}'
+DB_SOURCE_CLIENT_CONFIG = '{ "database_file": "' + SOURCE_DB_FILE + '"}'
 
 ORIG_SOA_TXT = 'example.com. 3600 IN SOA master.example.com. ' +\
                'admin.example.com. 1234 3600 1800 2419200 7200\n'
@@ -59,7 +61,7 @@ class ZoneLoaderTests(unittest.TestCase):
 
     def check_zone_soa(self, soa_txt):
         """
-        Check that the given RRset exists and matches the expected string
+        Check that the given SOA RR exists and matches the expected string
         """
         result, finder = self.client.find_zone(self.test_name)
         self.assertEqual(self.client.SUCCESS, result)
@@ -67,21 +69,27 @@ class ZoneLoaderTests(unittest.TestCase):
         self.assertEqual(finder.SUCCESS, result)
         self.assertEqual(soa_txt, rrset.to_text())
 
-    def test_load_file(self):
+    def check_load(self, loader):
         self.check_zone_soa(ORIG_SOA_TXT)
-
-        # Create loader and load the zone
-        loader = isc.datasrc.ZoneLoader(self.client, self.test_name, self.test_file)
         loader.load()
-
         self.check_zone_soa(NEW_SOA_TXT)
 
-    def test_load_incremental(self):
-        self.check_zone_soa(ORIG_SOA_TXT)
+        # And after that, it should throw
+        self.assertRaises(isc.dns.InvalidOperation, loader.load)
 
-        # Create loader and load the zone
-        loader = isc.datasrc.ZoneLoader(self.client, self.test_name, self.test_file)
+    def test_load_from_file(self):
+        loader = isc.datasrc.ZoneLoader(self.client, self.test_name,
+                                        self.test_file)
+        self.check_load(loader)
 
+    def test_load_from_client(self):
+        source_client = isc.datasrc.DataSourceClient('sqlite3',
+                                                     DB_SOURCE_CLIENT_CONFIG)
+        loader = isc.datasrc.ZoneLoader(self.client, self.test_name,
+                                        source_client)
+        self.check_load(loader)
+
+    def check_load_incremental(self, loader):
         # New zone has 8 RRs
         # After 5, it should return False
         self.assertFalse(loader.load_incremental(5))
@@ -94,14 +102,74 @@ class ZoneLoaderTests(unittest.TestCase):
         self.check_zone_soa(NEW_SOA_TXT)
 
         # And after that, it should throw
-        self.assertRaises(isc.datasrc.Error, loader.load_incremental, 5)
+        self.assertRaises(isc.dns.InvalidOperation, loader.load_incremental, 5)
+
+    def test_load_from_file_incremental(self):
+        # Create loader and load the zone
+        loader = isc.datasrc.ZoneLoader(self.client, self.test_name,
+                                        self.test_file)
+        self.check_load_incremental(loader)
+
+    def test_load_from_client_incremental(self):
+        source_client = isc.datasrc.DataSourceClient('sqlite3',
+                                                     DB_SOURCE_CLIENT_CONFIG)
+        loader = isc.datasrc.ZoneLoader(self.client, self.test_name,
+                                        source_client)
+        self.check_load_incremental(loader)
 
     def test_bad_file(self):
         self.check_zone_soa(ORIG_SOA_TXT)
-        loader = isc.datasrc.ZoneLoader(self.client, self.test_name, "no such file")
+        loader = isc.datasrc.ZoneLoader(self.client, self.test_name,
+                                        'no such file')
         self.assertRaises(isc.datasrc.MasterFileError, loader.load)
         self.check_zone_soa(ORIG_SOA_TXT)
 
+    def test_bad_file_incremental(self):
+        self.check_zone_soa(ORIG_SOA_TXT)
+        loader = isc.datasrc.ZoneLoader(self.client, self.test_name,
+                                        'no such file')
+        self.assertRaises(isc.datasrc.MasterFileError,
+                          loader.load_incremental, 1)
+        self.check_zone_soa(ORIG_SOA_TXT)
+
+    def test_no_such_zone_in_target(self):
+        self.assertRaises(isc.datasrc.Error, isc.datasrc.ZoneLoader,
+                          self.client, isc.dns.Name("unknownzone"),
+                          self.test_file)
+
+    def test_no_such_zone_in_source(self):
+        source_client = isc.datasrc.DataSourceClient('sqlite3',
+                                                     DB_SOURCE_CLIENT_CONFIG)
+        self.assertRaises(isc.datasrc.Error, isc.datasrc.ZoneLoader,
+                          self.client, isc.dns.Name("unknownzone"),
+                          source_client)
+
+    def test_no_ds_load_support(self):
+        # This may change in the future, but atm, the in-mem ds does
+        # not support the API the zone loader uses (it has direct load calls)
+        inmem_client = isc.datasrc.DataSourceClient('memory',
+                                                    '{ "type": "memory" }');
+        self.assertRaises(isc.datasrc.NotImplemented,
+                          isc.datasrc.ZoneLoader,
+                          inmem_client, self.test_name, self.test_file)
+
+    def test_wrong_class_from_file(self):
+        # If the file has wrong class, it is not detected until load time
+        loader = isc.datasrc.ZoneLoader(self.client, self.test_name,
+                                        self.test_file + '.ch')
+        self.assertRaises(isc.datasrc.MasterFileError, loader.load)
+
+    def test_wrong_class_from_client(self):
+        # For ds->ds loading, wrong class is detected upon construction
+        # Need a bit of the extended setup for CH source client
+        clientlist = isc.datasrc.ConfigurableClientList(isc.dns.RRClass.CH())
+        clientlist.configure('[ { "type": "static", "params": "' +
+                             STATIC_ZONE_FILE +'" } ]', False)
+        source_client, _, _ = clientlist.find(isc.dns.Name("bind."),
+                                              False, False)
+        self.assertRaises(isc.dns.InvalidParameter, isc.datasrc.ZoneLoader,
+                          self.client, isc.dns.Name("bind."), source_client)
+
     def test_exception(self):
         # Just check if masterfileerror is subclass of datasrc.Error
         self.assertTrue(issubclass(isc.datasrc.MasterFileError,

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

@@ -1,16 +1,82 @@
 namespace {
 const char* const ZoneLoader_doc = "\
 \n\
+Class to load data into a data source client.\n\
+\n\
+This is a small wrapper class that is able to load data into a data source.\n\
+It can load either from another data source or from a master file. The\n\
+purpose of the class is only to hold the state for incremental loading.\n\
+\n\
+The old content of zone is discarded and no journal is stored.\n\
+\n\
+The constructor takes three arguments:\n\
+- The datasource (isc.datasrc.DataSourceClient) to load the zone into\n\
+- The name (isc.dns.Name) to load\n\
+- either a string (for a file) or another DataSourceClient to load from\n\
+\n\
+Upon construction, no loading is done yet.\n\
+\n\
+It can throw:\n\
+DataSourceError, in case the zone does not exist in destination.\n\
+    This class does not support creating brand new zones, only loading\n\
+    data into them. In case a new zone is needed, it must be created\n\
+    beforehand (with create_zone()).\n\
+    DataSourceError is also thrown in case the zone is not present in the\n\
+    source DataSourceClient, and in case of other possibly low-level\n\
+    errors.\n\
+InvalidParameter, in case the class of destination and source\n\
+    differs.\n\
+NotImplemented in case target data source client doesn't provide an updater\n\
+    or the source data source client doesn't provide an iterator.\n\
 \n\
 ";
 
 const char* const ZoneLoader_loadIncremental_doc = "\
 \n\
+Load up to limit RRs.\n\
+\n\
+This performs a part of the loading. In case there's enough data in the\n\
+source, it copies limit RRs. It can copy less RRs during the final call\n\
+(when there's less than limit left).\n\
+\n\
+This can be called repeatedly until the whole zone is loaded, having\n\
+pauses in the loading for some purposes (for example reporting\n\
+progress).\n\
+\n\
+It has one parameter: limit (integer), The maximum allowed number of RRs\n\
+to be loaded during this call.\n\
+\n\
+Returns True in case the loading is completed, and False if there's more\n\
+to load.\n\
+\n\
+It can throw:\n\
+InvalidOperation, in case the loading was already completed before this\n\
+    call (by load() or by a loadIncremental that returned true).\n\
+DataSourceError, in case some error (possibly low-level) happens.\n\
+MasterFileError when the master_file is badly formatted or some similar\n\
+    problem is found when loading the master file.\n\
+\n\
+Note: If the limit is exactly the number of RRs available to be loaded,\n\
+      the method still returns false and true'll be returned on the next\n\
+      call (which will load 0 RRs). This is because the end of iterator or\n\
+      master file is detected when reading past the end, not when the last\n\
+      one is read.\n\
 \n\
 ";
 
 const char* const ZoneLoader_load_doc = "\
 \n\
+Performs the entire load operation.\n\
+\n\
+Depending on zone size, this could take a long time.\n\
+\n\
+This method has no parameters and does not return anything.\n\
+\n\
+It can throw:\n\
+InvalidOperation, in case the loading was already completed before this call.\n\
+MasterFileError, when the master_file is badly formatted or some\n\
+                 similar problem is found when loading the master file.\n\
+DataSourceError, in case some error (possibly low-level) happens.\n\
 \n\
 ";
 

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

@@ -22,26 +22,16 @@
 
 #include <util/python/pycppwrapper_util.h>
 
-#include <datasrc/client.h>
-#include <datasrc/database.h>
-#include <datasrc/data_source.h>
-#include <datasrc/sqlite3_accessor.h>
-#include <datasrc/iterator.h>
-#include <datasrc/zone.h>
 #include <datasrc/zone_loader.h>
-
 #include <dns/python/name_python.h>
-//#include <dns/python/rrset_python.h>
-//#include <dns/python/rrclass_python.h>
-//#include <dns/python/rrtype_python.h>
 #include <dns/python/pydnspp_common.h>
+#include <exceptions/exceptions.h>
 
 #include "client_python.h"
 #include "datasrc.h"
 #include "zone_loader_inc.cc"
 
 using namespace std;
-using namespace isc::util::python;
 using namespace isc::dns::python;
 using namespace isc::datasrc;
 using namespace isc::datasrc::python;
@@ -57,34 +47,50 @@ public:
     PyObject* client;
 };
 
-// Shortcut type which would be convenient for adding class variables safely.
-typedef CPPPyObjectContainer<s_ZoneLoader, ZoneLoader> ZoneLoaderContainer;
-
 // General creation and destruction
 int
 ZoneLoader_init(PyObject* po_self, PyObject* args, PyObject*) {
     s_ZoneLoader* self = static_cast<s_ZoneLoader*>(po_self);
-    PyObject *po_ds_client;
-    PyObject *po_name;
+    PyObject *po_target_client = NULL;
+    PyObject *po_source_client = NULL;
+    PyObject *po_name = NULL;
     char* master_file;
-    if (PyArg_ParseTuple(args, "O!O!s", &datasourceclient_type, &po_ds_client, &name_type, &po_name, &master_file)) {
-        try {
-            Py_INCREF(po_ds_client);
-            self->client = po_ds_client;
-            self->cppobj = new ZoneLoader(PyDataSourceClient_ToDataSourceClient(po_ds_client), PyName_ToName(po_name), master_file);
-            return (0);
-        } catch (const isc::datasrc::MasterFileError& mfe) {
-            PyErr_SetString(getDataSourceException("MasterFileError"), mfe.what());
-        } catch (const isc::datasrc::DataSourceError& dse) {
-            PyErr_SetString(getDataSourceException("Error"), dse.what());
-        } catch (const isc::NotImplemented& ni) {
-            PyErr_SetString(getDataSourceException("NotImplemented"), ni.what());
-        } catch (const std::exception& stde) {
-            PyErr_SetString(getDataSourceException("Error"), stde.what());
-        } catch (...) {
-            PyErr_SetString(getDataSourceException("Error"),
-                            "Unexpected exception");
+    if (!PyArg_ParseTuple(args, "O!O!s", &datasourceclient_type,
+                          &po_target_client, &name_type, &po_name,
+                          &master_file) &&
+        !PyArg_ParseTuple(args, "O!O!O!", &datasourceclient_type,
+                          &po_target_client, &name_type, &po_name,
+                          &datasourceclient_type, &po_source_client)
+       ) {
+        return (-1);
+    }
+    PyErr_Clear();
+    try {
+        Py_INCREF(po_target_client);
+        self->client = po_target_client;
+        if (po_source_client != NULL) {
+            self->cppobj = new ZoneLoader(
+                PyDataSourceClient_ToDataSourceClient(po_target_client),
+                PyName_ToName(po_name),
+                PyDataSourceClient_ToDataSourceClient(po_source_client));
+        } else {
+            self->cppobj = new ZoneLoader(
+                PyDataSourceClient_ToDataSourceClient(po_target_client),
+                PyName_ToName(po_name),
+                master_file);
         }
+        return (0);
+    } catch (const isc::InvalidParameter& ivp) {
+        PyErr_SetString(po_InvalidParameter, ivp.what());
+    } catch (const isc::datasrc::DataSourceError& dse) {
+        PyErr_SetString(getDataSourceException("Error"), dse.what());
+    } catch (const isc::NotImplemented& ni) {
+        PyErr_SetString(getDataSourceException("NotImplemented"), ni.what());
+    } catch (const std::exception& stde) {
+        PyErr_SetString(getDataSourceException("Error"), stde.what());
+    } catch (...) {
+        PyErr_SetString(getDataSourceException("Error"),
+                        "Unexpected exception");
     }
     return (-1);
 }
@@ -105,9 +111,15 @@ PyObject* ZoneLoader_load(PyObject* po_self, PyObject*) {
     try {
         self->cppobj->load();
         Py_RETURN_NONE;
+    } catch (const isc::InvalidOperation& ivo) {
+        PyErr_SetString(po_InvalidOperation, ivo.what());
+        return (NULL);
     } catch (const isc::datasrc::MasterFileError& mfe) {
         PyErr_SetString(getDataSourceException("MasterFileError"), mfe.what());
         return (NULL);
+    } catch (const isc::datasrc::DataSourceError& dse) {
+        PyErr_SetString(getDataSourceException("Error"), dse.what());
+        return (NULL);
     } catch (const std::exception& exc) {
         PyErr_SetString(getDataSourceException("Error"), exc.what());
         return (NULL);
@@ -137,9 +149,15 @@ PyObject* ZoneLoader_loadIncremental(PyObject* po_self, PyObject* args) {
         } else {
             Py_RETURN_FALSE;
         }
+    } catch (const isc::InvalidOperation& ivo) {
+        PyErr_SetString(po_InvalidOperation, ivo.what());
+        return (NULL);
     } catch (const isc::datasrc::MasterFileError& mfe) {
         PyErr_SetString(getDataSourceException("MasterFileError"), mfe.what());
         return (NULL);
+    } catch (const isc::datasrc::DataSourceError& dse) {
+        PyErr_SetString(getDataSourceException("Error"), dse.what());
+        return (NULL);
     } catch (const std::exception& exc) {
         PyErr_SetString(getDataSourceException("Error"), exc.what());
         return (NULL);
@@ -157,15 +175,9 @@ PyObject* ZoneLoader_loadIncremental(PyObject* po_self, PyObject* args) {
 // 3. Argument type
 // 4. Documentation
 PyMethodDef ZoneLoader_methods[] = {
-/*
-    { "get_origin", ZoneLoader_getOrigin, METH_NOARGS,
-       ZoneLoader_getOrigin_doc },
-    { "get_class", ZoneLoader_getClass, METH_NOARGS, ZoneLoader_getClass_doc },
-    { "find", ZoneLoader_find, METH_VARARGS, ZoneLoader_find_doc },
-    { "find_all", ZoneLoader_find_all, METH_VARARGS, ZoneLoader_findAll_doc },
-*/
     { "load", ZoneLoader_load, METH_NOARGS, ZoneLoader_load_doc },
-    { "load_incremental", ZoneLoader_loadIncremental, METH_VARARGS, ZoneLoader_loadIncremental_doc },
+    { "load_incremental", ZoneLoader_loadIncremental, METH_VARARGS,
+      ZoneLoader_loadIncremental_doc },
     { NULL, NULL, 0, NULL }
 };
 
@@ -225,8 +237,6 @@ PyTypeObject zone_loader_type = {
     0                                   // tp_version_tag
 };
 
-PyObject* po_MasterFileError;
-
 } // namespace python
 } // namespace datasrc
 } // namespace isc