Merge #1262

Conflicts:
	src/bin/xfrin/tests/xfrin_test.py
	src/lib/python/isc/xfrin/diff.py
	src/lib/python/isc/xfrin/tests/diff_tests.py

diff --cc src/bin/xfrin/tests/xfrin_test.py
index baf83a36b9e73fd7ecff722fb37f21447f759c6e,0699b127bc10cb1a0e9306875f94ff9e9dde35eb..68a312427eb874386c22ba7f1b99b637a97b5202
--- 1/src/bin/xfrin/tests/xfrin_test.py
--- 2/src/bin/xfrin/tests/xfrin_test.py
+++ b/src/bin/xfrin/tests/xfrin_test.py
@@@ -274,18 -288,13 +298,22 @@@ class TestXfrinState(unittest.TestCase)
                                RRTTL(3600))
          self.ns_rrset.add_rdata(Rdata(RRType.NS(), TEST_RRCLASS,
                                        'ns.example.com'))
+         self.a_rrset = RRset(TEST_ZONE_NAME, TEST_RRCLASS, RRType.A(),
+                              RRTTL(3600))
+         self.a_rrset.add_rdata(Rdata(RRType.A(), TEST_RRCLASS, '192.0.2.1'))
+ 
          self.conn._datasrc_client = MockDataSourceClient()
-         self.conn._diff = Diff(MockDataSourceClient(), TEST_ZONE_NAME)
+         self.conn._diff = Diff(self.conn._datasrc_client, TEST_ZONE_NAME)
  
 +class TestXfrinStateBase(TestXfrinState):
 +    def setUp(self):
 +        super().setUp()
 +
 +    def test_handle_rr_on_base(self):
 +        # The base version of handle_rr() isn't supposed to be called
 +        # directly (the argument doesn't matter in this test)
 +        self.assertRaises(XfrinException, XfrinState().handle_rr, None)
 +
  class TestXfrinInitialSOA(TestXfrinState):
      def setUp(self):
          super().setUp()
@@@ -331,13 -341,18 +360,16 @@@ class TestXfrinFirstData(TestXfrinState
          # the initial SOA.  Should switch to AXFR.
          self.assertFalse(self.state.handle_rr(self.conn, self.ns_rrset))
          self.assertEqual(type(XfrinAXFR()), type(self.conn.get_xfrstate()))
+         # The Diff for AXFR should be created at this point
+         self.assertNotEqual(None, self.conn._diff)
  
      def test_handle_ixfr_to_axfr_by_different_soa(self):
 -        # Response contains two consecutive SOA but the serial of the second
 -        # does not match the requested one.  The only possible interpretation
 -        # at this point is that it's an AXFR-compatible IXFR that only
 -        # consists of the SOA RR.  It will result in broken zone and should
 -        # be rejected anyway, but at this point we should switch to AXFR.
 +        # An unusual case: Response contains two consecutive SOA but the
 +        # serial of the second does not match the requested one.  See
 +        # the documentation for XfrinFirstData.handle_rr().
          self.assertFalse(self.state.handle_rr(self.conn, soa_rrset))
          self.assertEqual(type(XfrinAXFR()), type(self.conn.get_xfrstate()))
+         self.assertNotEqual(None, self.conn._diff)
  
      def test_finish_message(self):
          self.assertTrue(self.state.finish_message(self.conn))
@@@ -1081,8 -1116,9 +1133,9 @@@ class TestIXFRResponse(TestXfrinConnect
          self.conn._handle_xfrin_responses()
          self.assertEqual(type(XfrinIXFREnd()), type(self.conn.get_xfrstate()))
          self.assertEqual([], self.conn._datasrc_client.diffs)
-         self.check_diffs([[('delete', begin_soa_rrset), ('add', soa_rrset)]],
-                          self.conn._datasrc_client.committed_diffs)
+         check_diffs(self.assertEqual,
 -                    [[('remove', begin_soa_rrset), ('add', soa_rrset)]],
++                    [[('delete', begin_soa_rrset), ('add', soa_rrset)]],
+                     self.conn._datasrc_client.committed_diffs)
  
      def test_ixfr_response_multi_sequences(self):
          '''Similar to the previous case, but with multiple diff seqs.
@@@ -1107,19 -1143,20 +1160,20 @@@
          self.conn._handle_xfrin_responses()
          self.assertEqual(type(XfrinIXFREnd()), type(self.conn.get_xfrstate()))
          self.assertEqual([], self.conn._datasrc_client.diffs)
-         self.check_diffs([[('delete', begin_soa_rrset),
-                            ('delete', self._create_a('192.0.2.1')),
-                            ('add', self._create_soa('1231')),
-                            ('add', self._create_a('192.0.2.2'))],
-                           [('delete', self._create_soa('1231')),
-                            ('delete', self._create_a('192.0.2.3')),
-                            ('add', self._create_soa('1232')),
-                            ('add', self._create_a('192.0.2.4'))],
-                           [('delete', self._create_soa('1232')),
-                            ('delete', self._create_a('192.0.2.5')),
-                            ('add', soa_rrset),
-                            ('add', self._create_a('192.0.2.6'))]],
-                          self.conn._datasrc_client.committed_diffs)
+         check_diffs(self.assertEqual,
 -                    [[('remove', begin_soa_rrset),
 -                      ('remove', self._create_a('192.0.2.1')),
++                    [[('delete', begin_soa_rrset),
++                      ('delete', self._create_a('192.0.2.1')),
+                       ('add', self._create_soa('1231')),
+                       ('add', self._create_a('192.0.2.2'))],
 -                     [('remove', self._create_soa('1231')),
 -                      ('remove', self._create_a('192.0.2.3')),
++                     [('delete', self._create_soa('1231')),
++                      ('delete', self._create_a('192.0.2.3')),
+                       ('add', self._create_soa('1232')),
+                       ('add', self._create_a('192.0.2.4'))],
 -                     [('remove', self._create_soa('1232')),
 -                      ('remove', self._create_a('192.0.2.5')),
++                     [('delete', self._create_soa('1232')),
++                      ('delete', self._create_a('192.0.2.5')),
+                       ('add', soa_rrset),
+                       ('add', self._create_a('192.0.2.6'))]],
+                     self.conn._datasrc_client.committed_diffs)
  
      def test_ixfr_response_multi_messages(self):
          '''Similar to the first case, but RRs span over multiple messages.
@@@ -1133,8 -1170,9 +1187,9 @@@
              answers=[soa_rrset])
          self.conn._handle_xfrin_responses()
          self.assertEqual(type(XfrinIXFREnd()), type(self.conn.get_xfrstate()))
-         self.check_diffs([[('delete', begin_soa_rrset), ('add', soa_rrset)]],
-                          self.conn._datasrc_client.committed_diffs)
+         check_diffs(self.assertEqual,
 -                    [[('remove', begin_soa_rrset), ('add', soa_rrset)]],
++                    [[('delete', begin_soa_rrset), ('add', soa_rrset)]],
+                     self.conn._datasrc_client.committed_diffs)
  
      def test_ixfr_response_broken(self):
          '''Test with a broken response.
@@@ -1163,8 -1202,64 +1219,64 @@@
                       self._create_a('192.0.2.1')])
          self.assertRaises(XfrinProtocolError,
                            self.conn._handle_xfrin_responses)
-         self.check_diffs([[('delete', begin_soa_rrset), ('add', soa_rrset)]],
-                          self.conn._datasrc_client.committed_diffs)
+         check_diffs(self.assertEqual,
 -                    [[('remove', begin_soa_rrset), ('add', soa_rrset)]],
++                    [[('delete', begin_soa_rrset), ('add', soa_rrset)]],
+                     self.conn._datasrc_client.committed_diffs)
+ 
+     def test_ixfr_to_axfr_response(self):
+         '''AXFR-style IXFR response.
+ 
+         It simply updates the zone's SOA one time.
+ 
+         '''
+         ns_rr = self._create_ns()
+         a_rr = self._create_a('192.0.2.1')
+         self.conn.reply_data = self.conn.create_response_data(
+             questions=[Question(TEST_ZONE_NAME, TEST_RRCLASS, RRType.IXFR())],
+             answers=[soa_rrset, ns_rr, a_rr, soa_rrset])
+         self.conn._handle_xfrin_responses()
+         self.assertEqual(type(XfrinAXFREnd()), type(self.conn.get_xfrstate()))
+         self.assertEqual([], self.conn._datasrc_client.diffs)
+         # The SOA should be added exactly once, and in our implementation
+         # it should be added at the end of the sequence.
+         check_diffs(self.assertEqual,
+                     [[('add', ns_rr), ('add', a_rr), ('add', soa_rrset)]],
+                     self.conn._datasrc_client.committed_diffs)
+ 
+     def test_ixfr_to_axfr_response_mismatch_soa(self):
+         '''AXFR-style IXFR response, but the two SOA are not the same.
+ 
+         In the current implementation, we accept it and use the second SOA.
+ 
+         '''
+         ns_rr = self._create_ns()
+         a_rr = self._create_a('192.0.2.1')
+         self.conn.reply_data = self.conn.create_response_data(
+             questions=[Question(TEST_ZONE_NAME, TEST_RRCLASS, RRType.IXFR())],
+             answers=[soa_rrset, ns_rr, a_rr, begin_soa_rrset])
+         self.conn._handle_xfrin_responses()
+         self.assertEqual(type(XfrinAXFREnd()), type(self.conn.get_xfrstate()))
+         self.assertEqual([], self.conn._datasrc_client.diffs)
+         check_diffs(self.assertEqual,
+                     [[('add', ns_rr), ('add', a_rr),
+                       ('add', begin_soa_rrset)]],
+                     self.conn._datasrc_client.committed_diffs)
+ 
+     def test_ixfr_to_axfr_response_extra(self):
+         '''Test with an extra RR after the end of AXFR-style IXFR session.
+ 
+         The session should be rejected, and nothing should be committed.
+ 
+         '''
+         ns_rr = self._create_ns()
+         a_rr = self._create_a('192.0.2.1')
+         self.conn.reply_data = self.conn.create_response_data(
+             questions=[Question(TEST_ZONE_NAME, TEST_RRCLASS, RRType.IXFR())],
+             answers=[soa_rrset, ns_rr, a_rr, soa_rrset, a_rr])
+         self.assertRaises(XfrinProtocolError,
+                           self.conn._handle_xfrin_responses)
+         self.assertEqual(type(XfrinAXFREnd()), type(self.conn.get_xfrstate()))
+         self.assertEqual([], self.conn._datasrc_client.committed_diffs)
  
  class TestIXFRSession(TestXfrinConnection):
      '''Tests for a full IXFR session (query and response).
@@@ -1187,8 -1282,9 +1299,9 @@@
  
          # Check some details of the IXFR protocol processing
          self.assertEqual(type(XfrinIXFREnd()), type(self.conn.get_xfrstate()))
-         self.check_diffs([[('delete', begin_soa_rrset), ('add', soa_rrset)]],
-                          self.conn._datasrc_client.committed_diffs)
+         check_diffs(self.assertEqual,
 -                    [[('remove', begin_soa_rrset), ('add', soa_rrset)]],
++                    [[('delete', begin_soa_rrset), ('add', soa_rrset)]],
+                     self.conn._datasrc_client.committed_diffs)
  
          # Check if the query was IXFR.
          qdata = self.conn.query_data[2:]

diff --cc src/bin/xfrin/xfrin.py.in
index f8b310ab08688e0e0f99f0d3d28a9478607c9524,acc747ba93153ee6981311e07885b9a407142045..58878015cd1925286badc87b2cbe2043a8546956
--- 1/src/bin/xfrin/xfrin.py.in
--- 2/src/bin/xfrin/xfrin.py.in
+++ b/src/bin/xfrin/xfrin.py.in
@@@ -318,8 -258,11 +321,11 @@@ class XfrinFirstData(XfrinState)
          else:
              logger.debug(DBG_XFRIN_TRACE, XFRIN_GOT_NONINCREMENTAL_RESP,
                   conn.zone_str())
+             # We are now goint to add RRs to the new zone.  We need create
+             # a Diff object.  It will be used throughtout the XFR session.
+             conn._diff = Diff(conn._datasrc_client, conn._zone_name, True)
              self.set_xfrstate(conn, XfrinAXFR())
 -        return False    # need to revisit this RR in an update context
 +        return False
  
  class XfrinIXFRDeleteSOA(XfrinState):
      def handle_rr(self, conn, rr):

diff --cc src/lib/python/isc/xfrin/diff.py
index 29ac31e08249b4e19caa331a18957a5c4de308ea,8842fdb335c903733d0f925dc37f943a4b311b54..a2d9a7d759976f4cb92aa78b1431c901afe15896
--- 1/src/lib/python/isc/xfrin/diff.py
--- 2/src/lib/python/isc/xfrin/diff.py
+++ b/src/lib/python/isc/xfrin/diff.py
@@@ -59,19 -40,20 +59,21 @@@ class Diff
      the changes to underlying data source right away, but keeps them for
      a while.
      """
-     def __init__(self, ds_client, zone):
 -    def __init__(self, datasource, zone, replace=False):
++    def __init__(self, ds_client, zone, replace=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
          a transaction in the data source.
  
 -        The datasource is the one containing the zone. Zone is isc.dns.Name
 -        object representing the name of the zone (its apex). If replace is true,
 -        the content of the whole zone is wiped out before applying the diff.
 +        The ds_client is the datasource client containing the zone. Zone is
 +        isc.dns.Name object representing the name of the zone (its apex).
++        If replace is true, the content of the whole zone is wiped out before
++        applying the diff.
  
          You can also expect isc.datasrc.Error or isc.datasrc.NotImplemented
          exceptions.
          """
-         self.__updater = ds_client.get_updater(zone, False)
 -        self.__updater = datasource.get_updater(zone, replace)
++        self.__updater = ds_client.get_updater(zone, replace)
          if self.__updater is None:
              # The no such zone case
              raise NoSuchZone("Zone " + str(zone) +

diff --cc src/lib/python/isc/xfrin/tests/diff_tests.py
index c9cc588030dc91f99f1ea070223a8a3a0225d809,e1ba5619398a6f60cb45963cb5e95c10d95fe616..9fab89007f32f4bf847d4f8eefe0ff68137ec5bd
--- 1/src/lib/python/isc/xfrin/tests/diff_tests.py
--- 2/src/lib/python/isc/xfrin/tests/diff_tests.py
+++ b/src/lib/python/isc/xfrin/tests/diff_tests.py
@@@ -44,8 -37,7 +44,9 @@@ class DiffTest(unittest.TestCase)
          self.__data_operations = []
          self.__apply_called = False
          self.__commit_called = False
 +        self.__broken_called = False
 +        self.__warn_called = False
+         self.__should_replace = False
          # Some common values
          self.__rrclass = RRClass.IN()
          self.__type = RRType.A()
@@@ -354,84 -323,14 +355,92 @@@
          diff.compact()
          check()
  
 +    def test_wrong_class(self):
 +        """
 +        Test a wrong class of rrset is rejected.
 +        """
 +        diff = Diff(self, Name('example.org.'))
 +        rrset = RRset(Name('a.example.org.'), RRClass.CH(), RRType.NS(),
 +                      self.__ttl)
 +        rrset.add_rdata(Rdata(RRType.NS(), RRClass.CH(), 'ns.example.org.'))
 +        self.assertRaises(ValueError, diff.add_data, rrset)
 +        self.assertRaises(ValueError, diff.delete_data, rrset)
 +
 +    def __do_raise_test(self):
 +        """
 +        Do a raise test. Expects that one of the operations is exchanged for
 +        broken version.
 +        """
 +        diff = Diff(self, Name('example.org.'))
 +        diff.add_data(self.__rrset1)
 +        diff.delete_data(self.__rrset2)
 +        self.assertRaises(TestError, diff.commit)
 +        self.assertTrue(self.__broken_called)
 +        self.assertRaises(ValueError, diff.add_data, self.__rrset1)
 +        self.assertRaises(ValueError, diff.delete_data, self.__rrset2)
 +        self.assertRaises(ValueError, diff.commit)
 +        self.assertRaises(ValueError, diff.apply)
 +
 +    def test_raise_add(self):
 +        """
 +        Test the exception from add_rrset is propagated and the diff can't be
 +        used afterwards.
 +        """
 +        self.add_rrset = self.__broken_operation
 +        self.__do_raise_test()
 +
 +    def test_raise_delete(self):
 +        """
 +        Test the exception from delete_rrset is propagated and the diff can't be
 +        used afterwards.
 +        """
 +        self.delete_rrset = self.__broken_operation
 +        self.__do_raise_test()
 +
 +    def test_raise_commit(self):
 +        """
 +        Test the exception from updater's commit gets propagated and it can't be
 +        used afterwards.
 +        """
 +        self.commit = self.__broken_operation
 +        self.__do_raise_test()
 +
 +    def test_ttl(self):
 +        """
 +        Test the TTL handling. A warn function should have been called if they
 +        differ, but that's all, it should not crash or raise.
 +        """
 +        orig_logger = isc.xfrin.diff.logger
 +        try:
 +            isc.xfrin.diff.logger = self
 +            diff = Diff(self, Name('example.org.'))
 +            diff.add_data(self.__rrset1)
 +            rrset2 = RRset(Name('a.example.org.'), self.__rrclass,
 +                                  self.__type, RRTTL(120))
 +            rrset2.add_rdata(Rdata(self.__type, self.__rrclass, '192.10.2.2'))
 +            diff.add_data(rrset2)
 +            rrset2 = RRset(Name('a.example.org.'), self.__rrclass,
 +                                  self.__type, RRTTL(6000))
 +            rrset2.add_rdata(Rdata(self.__type, self.__rrclass, '192.10.2.3'))
 +            diff.add_data(rrset2)
 +            # They should get compacted together and complain.
 +            diff.compact()
 +            self.assertEqual(1, len(diff.get_buffer()))
 +            # The TTL stays on the first value, no matter if smaller or bigger
 +            # ones come later.
 +            self.assertEqual(self.__ttl, diff.get_buffer()[0][1].get_ttl())
 +            self.assertTrue(self.__warn_called)
 +        finally:
 +            isc.xfrin.diff.logger = orig_logger
 +
+     def test_relpace(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)
+ 
  if __name__ == "__main__":
      isc.log.init("bind10")
      unittest.main()