New test checkRestoreWithMultipleObjectsInUndoRedo from ZODB 3.2.2.

3.3 didn't have the bug, and this confirms it.

trunk/src/ZODB/tests/RecoveryStorage.py

@@ -183,3 +183,106 @@ class RecoveryStorage(IteratorDeepCompare):
         data, serial = self._dst.load(root._p_oid, '')
         raises(KeyError, self._dst.load, obj1._p_oid, '')
         raises(KeyError, self._dst.load, obj2._p_oid, '')
+
+    def checkRestoreWithMultipleObjectsInUndoRedo(self):
+        from ZODB.FileStorage import FileStorage
+
+        # Undo creates backpointers in (at least) FileStorage.  ZODB 3.2.1
+        # FileStorage._data_find() had an off-by-8 error, neglecting to
+        # account for the size of the backpointer when searching a
+        # transaction with multiple data records.  The results were
+        # unpredictable.  For example, it could raise a Python exception
+        # due to passing a negative offset to file.seek(), or could
+        # claim that a transaction didn't have data for an oid despite
+        # that it actually did.
+        #
+        # The former failure mode was seen in real life, in a ZRS secondary
+        # doing recovery.  On my box today, the second failure mode is
+        # what happens in this test (with an unpatched _data_find, of
+        # course).  Note that the error can only "bite" if more than one
+        # data record is in a transaction, and the oid we're looking for
+        # follows at least one data record with a backpointer.
+        #
+        # Unfortunately, _data_find() is a low-level implementation detail,
+        # and this test does some horrid white-box abuse to test it.
+
+        is_filestorage = isinstance(self._storage, FileStorage)
+
+        db = DB(self._storage)
+        c = db.open()
+        r = c.root()
+
+        # Create some objects.
+        r["obj1"] = MinPO(1)
+        r["obj2"] = MinPO(1)
+        transaction.commit()
+
+        # Add x attributes to them.
+        r["obj1"].x = 'x1'
+        r["obj2"].x = 'x2'
+        transaction.commit()
+
+        r = db.open().root()
+        self.assertEquals(r["obj1"].x, 'x1')
+        self.assertEquals(r["obj2"].x, 'x2')
+
+        # Dirty tricks.
+        if is_filestorage:
+            obj1_oid = r["obj1"]._p_oid
+            obj2_oid = r["obj2"]._p_oid
+            # This will be the offset of the next transaction, which
+            # will contain two backpointers.
+            pos = self._storage.getSize()
+
+        # Undo the attribute creation.
+        info = self._storage.undoInfo()
+        tid = info[0]['id']
+        t = Transaction()
+        self._storage.tpc_begin(t)
+        oids = self._storage.undo(tid, t)
+        self._storage.tpc_vote(t)
+        self._storage.tpc_finish(t)
+
+        r = db.open().root()
+        self.assertRaises(AttributeError, getattr, r["obj1"], 'x')
+        self.assertRaises(AttributeError, getattr, r["obj2"], 'x')
+
+        if is_filestorage:
+            # _data_find should find data records for both objects in that
+            # transaction.  Without the patch, the second assert failed
+            # (it claimed it couldn't find a data record for obj2) on my
+            # box, but other failure modes were possible.
+            self.assert_(self._storage._data_find(pos, obj1_oid, '') > 0)
+            self.assert_(self._storage._data_find(pos, obj2_oid, '') > 0)
+
+            # The offset of the next ("redo") transaction.
+            pos = self._storage.getSize()
+
+        # Undo the undo (restore the attributes).
+        info = self._storage.undoInfo()
+        tid = info[0]['id']
+        t = Transaction()
+        self._storage.tpc_begin(t)
+        oids = self._storage.undo(tid, t)
+        self._storage.tpc_vote(t)
+        self._storage.tpc_finish(t)
+
+        r = db.open().root()
+        self.assertEquals(r["obj1"].x, 'x1')
+        self.assertEquals(r["obj2"].x, 'x2')
+
+        if is_filestorage:
+            # Again _data_find should find both objects in this txn, and
+            # again the second assert failed on my box.
+            self.assert_(self._storage._data_find(pos, obj1_oid, '') > 0)
+            self.assert_(self._storage._data_find(pos, obj2_oid, '') > 0)
+
+        # Indirectly provoke .restore().  .restore in turn indirectly
+        # provokes _data_find too, but not usefully for the purposes of
+        # the specific bug this test aims at:  copyTransactionsFrom() uses
+        # storage iterators that chase backpointers themselves, and
+        # return the data they point at instead.  The result is that
+        # _data_find didn't actually see anything dangerous in this
+        # part of the test.
+        self._dst.copyTransactionsFrom(self._storage)
+        self.compare(self._storage, self._dst)