Fix a bug in cascade deletion/nullification

sqlobject · Sep 29, 2019 · 9c313c0 · 9c313c0
1 parent f9822e7
commit 9c313c0
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 24 deletions.
diff --git a/docs/News.rst b/docs/News.rst
@@ -17,6 +17,8 @@ Features
 Bug fixes
 ---------
 
+* Fixed a bug in cascade deletion/nullification.
+
 * Fixed a bug working with microseconds in Time columns.
 
 SQLObject 3.7.3

diff --git a/sqlobject/main.py b/sqlobject/main.py
@@ -1639,36 +1639,41 @@ def destroySelf(self):
                 continue
 
             query = []
-            delete = setnull = restrict = False
+            restrict = False
             for _col in cols:
+                query.append(getattr(k.q, _col.name) == self.id)
                 if _col.cascade is False:
                     # Found a restriction
                     restrict = True
-                query.append(getattr(k.q, _col.name) == self.id)
+            query = sqlbuilder.OR(*query)
+            results = k.select(query, connection=self._connection)
+            if restrict and results.count():
+                # Restrictions only apply if there are
+                # matching records on the related table
+                raise SQLObjectIntegrityError(
+                    "Tried to delete %s::%s but "
+                    "table %s has a restriction against it" %
+                    (klass.__name__, self.id, k.__name__))
+
+            setnull = {}
+            for _col in cols:
                 if _col.cascade == 'null':
-                    setnull = _col.name
-                elif _col.cascade:
+                    setnull[_col.name] = None
+            if setnull:
+                for row in results:
+                    row.set(**setnull)
+
+            delete = False
+            for _col in cols:
+                if _col.cascade and _col.cascade != 'null':
                     delete = True
             assert delete or setnull or restrict, (
                 "Class %s depends on %s accoriding to "
                 "findDependantColumns, but this seems inaccurate"
                 % (k, klass))
-            query = sqlbuilder.OR(*query)
-            results = k.select(query, connection=self._connection)
-            if restrict:
-                if results.count():
-                    # Restrictions only apply if there are
-                    # matching records on the related table
-                    raise SQLObjectIntegrityError(
-                        "Tried to delete %s::%s but "
-                        "table %s has a restriction against it" %
-                        (klass.__name__, self.id, k.__name__))
-            else:
+            if delete:
                 for row in results:
-                    if delete:
-                        row.destroySelf()
-                    else:
-                        row.set(**{setnull: None})
+                    row.destroySelf()
 
         self.sqlmeta._obsolete = True
         self._connection._SO_delete(self)

diff --git a/sqlobject/tests/test_ForeignKey_cascade.py b/sqlobject/tests/test_ForeignKey_cascade.py
@@ -91,9 +91,7 @@ def test3():
     raises(SQLObjectNotFound, emily.sync)
 
     assert message.recipient is None
-
-    # This looks like a bug; `message.sender` here should be None
-    # assert message.sender is None
+    assert message.sender is None
 
 
 class SOTestPerson4(SQLObject):
@@ -120,6 +118,24 @@ def test4():
     message.expire()
 
     john.sync()
+    raises(SQLObjectNotFound, message.sync)
+
+
+def test5():
+    setupClass([SOTestPerson4, SOTestMessageCascadeMixed])
 
-    # This is even a nastier bug; `message` was deleted from the DB
-    # message.sync()
+    john = SOTestPerson4(name='john')
+    emily = SOTestPerson4(name='emily')
+    message = SOTestMessageCascadeMixed(
+        sender=emily, recipient=john, body='test4'
+    )
+
+    john.destroySelf()
+    emily.expire()
+    message.expire()
+
+    emily.sync()
+    message.sync()
+
+    assert message.recipient is None
+    assert message.sender == emily