Merge lp:~therve/storm/flush-order-673666 into lp:storm

[Thomas Herve]

The test case was fairly easy to create, and the fix as well. I'm not convinced this is the best fix, but it seems to be reasonable enough.

[Gustavo Niemeyer (niemeyer)]

[1]

Yeah, this looks quite simple, but it seems to be hiding a bug somewhere else.

The obvious question is why did the pair survive for that long elsewhere, rather than
getting cleaned up when the transaction was flushed?

review: Needs Information

[lp:~therve/storm/flush-order-673666]

385. By Thomas Herve on 2010-12-11

Try another approach

[Thomas Herve (therve)]

I tried something else: I added an unlink on remove flush. I tried to think hard about the consequences:
 * Can a change go undetected because of that?
 * Can we have a leak because remote_info references local_info?

But I've answered no to both questions, unless I'm missing something.

Unmerged revisions

385. By Thomas Herve on 2010-12-11

Try another approach

384. By Thomas Herve on 2010-11-13

Add a test, and one possible fix.

=== modified file 'storm/references.py'
--- storm/references.py	2010-06-01 08:33:33 +0000
+++ storm/references.py	2010-12-11 11:58:13 +0000
@@ -645,6 +645,8 @@
                 #                      remote_info)
                 remote_info.event.hook("removed", self._break_on_remote_removed,
                                        weakref.ref(local_info))
+                remote_info.event.hook("flushed", self._break_on_remote_flushed,
+                                       local_info)
             else:
                 remote_has_changed = False
                 for local_column, remote_column in pairs:

=== modified file 'tests/store/base.py'
--- tests/store/base.py	2010-10-19 13:36:26 +0000
+++ tests/store/base.py	2010-12-11 11:58:13 +0000
@@ -3629,6 +3629,31 @@
         ref1.selfref = None
         self.assertRaises(OrderLoopError, self.store.flush)
 
+    def test_reference_set_clean_flush_order(self):
+        """
+        When a L{ReferenceSet} target is removed, it doesn't force the removal
+        of the flush order if this one is not present in the current
+        transaction.
+        """
+        class MyFoo(Foo):
+            bars = ReferenceSet(Foo.title, Bar.title)
+
+        foo = MyFoo()
+        foo.title = u"Title 40"
+        self.store.add(foo)
+        self.store.flush()
+
+        bar = Bar()
+        bar.id = 400
+        bar.foo_id = 4000
+        bar.title = u"Title 40"
+
+        foo.bars.add(bar)
+
+        self.store.commit()
+        self.store.flush()
+        self.store.remove(bar)
+
     def add_reference_set_bar_400(self):
         bar = Bar()
         bar.id = 400