[1456] Add 'single-update-mode' to xfrin/diff.py

This mode is useful for DDNS-type updates; it allows for additions and deletions to be done in any order (keeping track of them separately).
It does have some extra restrictions; SOA records must be the first deleted and the first added type, and may not occur later anymore (causes ValueError).

For now, I left this in python/isc/xfrin, even though it is not purely xfrin anymore. Should we move it to datasrc? or util?

src/lib/python/isc/xfrin/diff.py

@@ -15,11 +15,12 @@
 
 """
 This helps the XFR in process with accumulating parts of diff and applying
-it to the datasource.
+it to the datasource. It also has a 'single update mode' which is useful
+for DDNS.
 
 The name of the module is not yet fully decided. We might want to move it
-under isc.datasrc or somewhere else, because we might want to reuse it with
-future DDNS process. But until then, it lives here.
+under isc.datasrc or somewhere else, because we are reusing it with DDNS.
+But for now, it lives here.
 """
 
 import isc.dns
@@ -59,7 +60,8 @@ class Diff:
     the changes to underlying data source right away, but keeps them for
     a while.
     """
-    def __init__(self, ds_client, zone, replace=False, journaling=False):
+    def __init__(self, ds_client, zone, replace=False, journaling=False,
+                 single_update_mode=False):
         """
         Initializes the diff to a ready state. It checks the zone exists
         in the datasource and if not, NoSuchZone is raised. This also creates
@@ -76,6 +78,15 @@ class Diff:
         incoming updates but does not support journaling, the Diff object
         will still continue applying the diffs with disabling journaling.
 
+        If single_update_mode is true, the update is expected to only contain
+        1 set of changes (i.e. one set of additions, and one set of deletions).
+        If so, the additions and deletions are kept separately, and applied
+        in one go upon commit() or apply(). In this mode, additions and
+        deletions can be done in any order. The first addition and the
+        first addition still have to be the new and old SOA records,
+        respectively. Once apply() or commit() has been called, this
+        requirement is renewed (since the diff object is essentialy reset).
+
         You can also expect isc.datasrc.Error or isc.datasrc.NotImplemented
         exceptions.
         """
@@ -91,7 +102,12 @@ class Diff:
             raise NoSuchZone("Zone " + str(zone) +
                              " does not exist in the data source " +
                              str(ds_client))
-        self.__buffer = []
+        self.__single_update_mode = single_update_mode
+        if single_update_mode:
+            self.__additions = []
+            self.__deletions = []
+        else:
+            self.__buffer = []
 
     def __check_commited(self):
         """
@@ -109,6 +125,14 @@ class Diff:
 
         It does all the real work of add_data and delete_data, including
         all checks.
+
+        Raises a ValueError in several cases:
+        - if the rrset contains multiple rrs
+        - if the class of the rrset does not match that of the update
+        - in single_update_mode if the first rr is not of type SOA (both
+          for addition and deletion)
+        - in single_update_mode if any later rr is of type SOA (both for
+          addition and deletion)
         """
         self.__check_commited()
         if rr.get_rdata_count() != 1:
@@ -118,10 +142,36 @@ class Diff:
             raise ValueError("The rrset's class " + str(rr.get_class()) +
                              " does not match updater's " +
                              str(self.__updater.get_class()))
-        self.__buffer.append((operation, rr))
-        if len(self.__buffer) >= DIFF_APPLY_TRESHOLD:
-            # Time to auto-apply, so the data don't accumulate too much
-            self.apply()
+        if self.__single_update_mode:
+            if operation == 'add':
+                # First addition must be SOA
+                if len(self.__additions) == 0 and\
+                   rr.get_type() != isc.dns.RRType.SOA():
+                    raise ValueError("First addition in single update mode " +
+                                     "must be of type SOA")
+                # And later additions may not
+                elif len(self.__additions) != 0 and\
+                   rr.get_type() == isc.dns.RRType.SOA():
+                    raise ValueError("Multiple SOA records in single " +
+                                     "update mode addition")
+                self.__additions.append((operation, rr))
+            elif operation == 'delete':
+                if len(self.__deletions) == 0 and\
+                   rr.get_type() != isc.dns.RRType.SOA():
+                    raise ValueError("First deletion in single update mode " +
+                                     "must be of type SOA")
+                # And later deletions may not
+                elif len(self.__deletions) != 0 and\
+                   rr.get_type() == isc.dns.RRType.SOA():
+                    raise ValueError("Multiple SOA records in single " +
+                                     "update mode deletion")
+                self.__deletions.append((operation, rr))
+        else:
+            self.__buffer.append((operation, rr))
+            if len(self.__buffer) >= DIFF_APPLY_TRESHOLD:
+                # Time to auto-apply, so the data don't accumulate too much
+                # This is not done for DDNS type data
+                self.apply()
 
     def add_data(self, rr):
         """
@@ -149,6 +199,7 @@ class Diff:
         """
         self.__data_common(rr, 'delete')
 
+
     def compact(self):
         """
         Tries to compact the operations in buffer a little by putting some of
@@ -175,23 +226,34 @@ class Diff:
             sigdata2 = rrset2.get_rdata()[0].to_text().split()[0]
             return sigdata1 == sigdata2
 
-        buf = []
-        for (op, rrset) in self.__buffer:
-            old = buf[-1][1] if len(buf) > 0 else None
-            if old is None or op != buf[-1][0] or \
-                rrset.get_name() != old.get_name() or \
-                (not same_type(rrset, old)):
-                buf.append((op, isc.dns.RRset(rrset.get_name(),
-                                              rrset.get_class(),
-                                              rrset.get_type(),
-                                              rrset.get_ttl())))
-            if rrset.get_ttl() != buf[-1][1].get_ttl():
-                logger.warn(LIBXFRIN_DIFFERENT_TTL, rrset.get_ttl(),
-                            buf[-1][1].get_ttl(), rrset.get_name(),
-                            rrset.get_class(), rrset.get_type())
-            for rdatum in rrset.get_rdata():
-                buf[-1][1].add_rdata(rdatum)
-        self.__buffer = buf
+        def compact_buffer(buffer_to_compact):
+            '''Internal helper function for compacting buffers, compacts the
+               given buffer.
+               Returns the compacted buffer.
+            '''
+            buf = []
+            for (op, rrset) in buffer_to_compact:
+                old = buf[-1][1] if len(buf) > 0 else None
+                if old is None or op != buf[-1][0] or \
+                    rrset.get_name() != old.get_name() or \
+                    (not same_type(rrset, old)):
+                    buf.append((op, isc.dns.RRset(rrset.get_name(),
+                                                  rrset.get_class(),
+                                                  rrset.get_type(),
+                                                  rrset.get_ttl())))
+                if rrset.get_ttl() != buf[-1][1].get_ttl():
+                    logger.warn(LIBXFRIN_DIFFERENT_TTL, rrset.get_ttl(),
+                                buf[-1][1].get_ttl(), rrset.get_name(),
+                                rrset.get_class(), rrset.get_type())
+                for rdatum in rrset.get_rdata():
+                    buf[-1][1].add_rdata(rdatum)
+            return buf
+
+        if self.__single_update_mode:
+            self.__additions = compact_buffer(self.__additions)
+            self.__deletions = compact_buffer(self.__deletions)
+        else:
+            self.__buffer = compact_buffer(self.__buffer)
 
     def apply(self):
         """
@@ -209,25 +271,41 @@ class Diff:
         It also can raise isc.datasrc.Error. If that happens, you should stop
         using this object and abort the modification.
         """
-        self.__check_commited()
-        # First, compact the data
-        self.compact()
-        try:
-            # Then pass the data inside the data source
-            for (operation, rrset) in self.__buffer:
+        def apply_buffer(buf):
+            '''
+            Helper method to apply all operations in the given buffer
+            '''
+            for (operation, rrset) in buf:
                 if operation == 'add':
                     self.__updater.add_rrset(rrset)
                 elif operation == 'delete':
                     self.__updater.delete_rrset(rrset)
                 else:
                     raise ValueError('Unknown operation ' + operation)
+
+        self.__check_commited()
+        # First, compact the data
+        self.compact()
+        try:
+            # Then pass the data inside the data source
+            if self.__single_update_mode:
+                apply_buffer(self.__deletions)
+                apply_buffer(self.__additions)
+            else:
+                apply_buffer(self.__buffer)
+
             # As everything is already in, drop the buffer
         except:
             # If there's a problem, we can't continue.
             self.__updater = None
             raise
 
-        self.__buffer = []
+        # all went well, reset state of buffers
+        if self.__single_update_mode:
+            self.__additions = []
+            self.__deletions = []
+        else:
+            self.__buffer = []
 
     def commit(self):
         """
@@ -259,5 +337,29 @@ class Diff:
 
         Probably useful only for testing and introspection purposes. Don't
         modify the list.
+
+        Raises a ValueError if the buffer is in single_update_mode.
+        """
+        if self.__single_update_mode:
+            # This operation is only valid if the multi-update mode
+            raise ValueError("Compound buffer requested in single-update mode")
+        else:
+            return self.__buffer
+
+    def get_single_update_buffers(self):
+        """
+        Returns the current buffers of changes not yet passed into the data
+        source. It is a tuple of the current deletions and additions, which
+        each are in a form like [('add', rrset), ('delete', rrset),
+        ('delete', rrset), ...].
+
+        Probably useful only for testing and introspection purposes. Don't
+        modify the list.
+
+        Raises a ValueError if the buffer is not in single_update_mode.
         """
-        return self.__buffer
+        if not self.__single_update_mode:
+            # This operation is only valid if the multi-update mode
+            raise ValueError("Compound buffer requested in single-update mode")
+        else:
+            return (self.__deletions, self.__additions)

src/lib/python/isc/xfrin/tests/diff_tests.py

@@ -478,14 +478,116 @@ class DiffTest(unittest.TestCase):
         diff.compact()
         self.assertEqual(2, len(diff.get_buffer()))
 
-    def test_relpace(self):
-        """
+    def test_replace(self):
+        '''
         Test that when we want to replace the whole zone, it is propagated.
-        """
+        '''
         self.__should_replace = True
         diff = Diff(self, "example.org.", True)
         self.assertTrue(self.__updater_requested)
 
+    def test_get_buffer(self):
+        '''
+        Test that the getters raise when used in the wrong mode
+        '''
+        diff_multi = Diff(self, Name('example.org.'), single_update_mode=False)
+        self.assertRaises(ValueError, diff_multi.get_single_update_buffers)
+        self.assertEqual([], diff_multi.get_buffer())
+
+        diff_single = Diff(self, Name('example.org.'), single_update_mode=True)
+        self.assertRaises(ValueError, diff_single.get_buffer)
+        self.assertEqual(([], []), diff_single.get_single_update_buffers())
+
+    def test_single_update_mode(self):
+        '''
+        Test single-update mode. In this mode, updates and deletes can
+        be done in any order, but there may only be one changeset.
+        For both updates and deletes, exactly one SOA rr must be given,
+        and it must be the first change.
+        '''
+
+        # First create some RRsets to play with
+        soa = RRset(Name('example.org.'), self.__rrclass, RRType.SOA(),
+                    RRTTL(3600))
+        soa.add_rdata(Rdata(soa.get_type(), soa.get_class(),
+                      "ns.example.org. foo.example.org. 1234 28800 "+
+                      "7200 604800 3600"))
+
+        a = RRset(Name('www.example.org.'), self.__rrclass, RRType.A(),
+                      RRTTL(3600))
+        a.add_rdata(Rdata(a.get_type(), a.get_class(),
+                        "192.0.2.1"))
+
+        a2 = RRset(Name('www.example.org.'), self.__rrclass, RRType.A(),
+                      RRTTL(3600))
+        a2.add_rdata(Rdata(a2.get_type(), a2.get_class(),
+                        "192.0.2.2"))
+
+        # full rrset for A (to check compact())
+        a_1_2 = RRset(Name('www.example.org.'), self.__rrclass, RRType.A(),
+                      RRTTL(3600))
+        a_1_2.add_rdata(Rdata(a_1_2.get_type(), a_1_2.get_class(),
+                        "192.0.2.1"))
+        a_1_2.add_rdata(Rdata(a_1_2.get_type(), a_1_2.get_class(),
+                        "192.0.2.2"))
+
+        diff = Diff(self, Name('example.org.'), single_update_mode=True)
+
+        # adding a first should fail
+        self.assertRaises(ValueError, diff.add_data, a)
+        # But soa should work
+        diff.add_data(soa)
+        # And then A should as well
+        diff.add_data(a)
+        diff.add_data(a2)
+        # But another SOA should fail again
+        self.assertRaises(ValueError, diff.add_data, soa)
+
+        # Same for delete
+        self.assertRaises(ValueError, diff.delete_data, a)
+        diff.delete_data(soa)
+        diff.delete_data(a)
+        diff.delete_data(a2)
+        self.assertRaises(ValueError, diff.delete_data, soa)
+
+        # Not compacted yet, so the buffers should be as we
+        # filled them
+        (delbuf, addbuf) = diff.get_single_update_buffers()
+        self.assertEqual([('delete', soa), ('delete', a), ('delete', a2)], delbuf)
+        self.assertEqual([('add', soa), ('add', a), ('add', a2)], addbuf)
+
+        # Compact should compact the A records in both buffers
+        diff.compact()
+        (delbuf, addbuf) = diff.get_single_update_buffers()
+        # need rrset equality again :/
+        self.assertEqual(2, len(delbuf))
+        self.assertEqual(2, len(delbuf[0]))
+        self.assertEqual('delete', delbuf[0][0])
+        self.assertEqual(soa.to_text(), delbuf[0][1].to_text())
+        self.assertEqual(2, len(delbuf[1]))
+        self.assertEqual('delete', delbuf[1][0])
+        self.assertEqual(a_1_2.to_text(), delbuf[1][1].to_text())
+
+        self.assertEqual(2, len(addbuf))
+        self.assertEqual(2, len(addbuf[0]))
+        self.assertEqual('add', addbuf[0][0])
+        self.assertEqual(soa.to_text(), addbuf[0][1].to_text())
+        self.assertEqual(2, len(addbuf[1]))
+        self.assertEqual('add', addbuf[1][0])
+        self.assertEqual(a_1_2.to_text(), addbuf[1][1].to_text())
+
+        # Apply should reset the buffers
+        diff.apply()
+        (delbuf, addbuf) = diff.get_single_update_buffers()
+        self.assertEqual([], delbuf)
+        self.assertEqual([], addbuf)
+
+        # Now the change has been applied, and the buffers are cleared,
+        # Adding non-SOA records should fail again.
+        self.assertRaises(ValueError, diff.add_data, a)
+        self.assertRaises(ValueError, diff.delete_data, a)
+
+
 if __name__ == "__main__":
     isc.log.init("bind10")
     isc.log.resetUnitTestRootLogger()