Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Fixed bug with Model.delete() which did not always delete objects in …

…the right order.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@7722 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit b1851cca3ee1494cc020f1676e2029ef138250da 1 parent 7c62153
Luke Plant authored June 21, 2008
19  django/db/models/base.py
@@ -10,7 +10,7 @@
10 10
 from django.core.exceptions import ObjectDoesNotExist, MultipleObjectsReturned, FieldError
11 11
 from django.db.models.fields import AutoField, ImageField, FieldDoesNotExist
12 12
 from django.db.models.fields.related import OneToOneRel, ManyToOneRel, OneToOneField
13  
-from django.db.models.query import delete_objects, Q
  13
+from django.db.models.query import delete_objects, Q, CollectedObjects
14 14
 from django.db.models.options import Options, AdminOptions
15 15
 from django.db import connection, transaction
16 16
 from django.db.models import signals
@@ -368,17 +368,16 @@ def validate(self):
368 368
                 error_dict[f.name] = errors
369 369
         return error_dict
370 370
 
371  
-    def _collect_sub_objects(self, seen_objs):
  371
+    def _collect_sub_objects(self, seen_objs, parent=None, nullable=False):
372 372
         """
373 373
         Recursively populates seen_objs with all objects related to this object.
374  
-        When done, seen_objs will be in the format:
375  
-            {model_class: {pk_val: obj, pk_val: obj, ...},
376  
-             model_class: {pk_val: obj, pk_val: obj, ...}, ...}
  374
+        When done, seen_objs.items() will be in the format:
  375
+            [(model_class, {pk_val: obj, pk_val: obj, ...}),
  376
+             (model_class, {pk_val: obj, pk_val: obj, ...}),...]
377 377
         """
378 378
         pk_val = self._get_pk_val()
379  
-        if pk_val in seen_objs.setdefault(self.__class__, {}):
  379
+        if seen_objs.add(self.__class__, pk_val, self, parent, nullable):
380 380
             return
381  
-        seen_objs[self.__class__][pk_val] = self
382 381
 
383 382
         for related in self._meta.get_all_related_objects():
384 383
             rel_opts_name = related.get_accessor_name()
@@ -388,16 +387,16 @@ def _collect_sub_objects(self, seen_objs):
388 387
                 except ObjectDoesNotExist:
389 388
                     pass
390 389
                 else:
391  
-                    sub_obj._collect_sub_objects(seen_objs)
  390
+                    sub_obj._collect_sub_objects(seen_objs, self.__class__, related.field.null)
392 391
             else:
393 392
                 for sub_obj in getattr(self, rel_opts_name).all():
394  
-                    sub_obj._collect_sub_objects(seen_objs)
  393
+                    sub_obj._collect_sub_objects(seen_objs, self.__class__, related.field.null)
395 394
 
396 395
     def delete(self):
397 396
         assert self._get_pk_val() is not None, "%s object can't be deleted because its %s attribute is set to None." % (self._meta.object_name, self._meta.pk.attname)
398 397
 
399 398
         # Find all the objects than need to be deleted
400  
-        seen_objs = SortedDict()
  399
+        seen_objs = CollectedObjects()
401 400
         self._collect_sub_objects(seen_objs)
402 401
 
403 402
         # Actually delete the objects
118  django/db/models/query.py
@@ -16,6 +16,92 @@
16 16
 # Pull into this namespace for backwards compatibility
17 17
 EmptyResultSet = sql.EmptyResultSet
18 18
 
  19
+class CyclicDependency(Exception):
  20
+    pass
  21
+
  22
+class CollectedObjects(object):
  23
+    """
  24
+    A container that stores keys and lists of values along with
  25
+    remembering the parent objects for all the keys.
  26
+
  27
+    This is used for the database object deletion routines so that we
  28
+    can calculate the 'leaf' objects which should be deleted first.
  29
+    """
  30
+
  31
+    def __init__(self):
  32
+        self.data = {}
  33
+        self.children = {}
  34
+
  35
+    def add(self, model, pk, obj, parent_model, nullable=False):
  36
+        """
  37
+        Adds an item.
  38
+        model is the class of the object being added,
  39
+        pk is the primary key, obj is the object itself, 
  40
+        parent_model is the model of the parent object
  41
+        that this object was reached through, nullable should
  42
+        be True if this relation is nullable.
  43
+
  44
+        If the item already existed in the structure,
  45
+        returns true, otherwise false.
  46
+        """
  47
+        d = self.data.setdefault(model, SortedDict())
  48
+        retval = pk in d
  49
+        d[pk] = obj
  50
+        # Nullable relationships can be ignored -- they
  51
+        # are nulled out before deleting, and therefore
  52
+        # do not affect the order in which objects have
  53
+        # to be deleted.
  54
+        if parent_model is not None and not nullable:
  55
+            self.children.setdefault(parent_model, []).append(model)
  56
+
  57
+        return retval
  58
+
  59
+    def __contains__(self, key):
  60
+        return self.data.__contains__(key)
  61
+
  62
+    def __getitem__(self, key):
  63
+        return self.data[key]
  64
+
  65
+    def __nonzero__(self):
  66
+        return bool(self.data)
  67
+
  68
+    def iteritems(self):
  69
+        for k in self.ordered_keys():
  70
+            yield k, self[k]
  71
+
  72
+    def items(self):
  73
+        return list(self.iteritems())
  74
+
  75
+    def keys(self):
  76
+        return self.ordered_keys()
  77
+
  78
+    def ordered_keys(self):
  79
+        """
  80
+        Returns the models in the order that they should be 
  81
+        dealth with i.e. models with no dependencies first.
  82
+        """
  83
+        dealt_with = SortedDict()
  84
+        # Start with items that have no children
  85
+        models = self.data.keys()
  86
+        while len(dealt_with) < len(models):
  87
+            found = False
  88
+            for model in models:
  89
+                children = self.children.setdefault(model, [])
  90
+                if len([c for c in children if c not in dealt_with]) == 0:
  91
+                    dealt_with[model] = None
  92
+                    found = True
  93
+            if not found:
  94
+                raise CyclicDependency("There is a cyclic dependency of items to be processed.")
  95
+            
  96
+        return dealt_with.keys()
  97
+
  98
+    def unordered_keys(self):
  99
+        """
  100
+        Fallback for the case where is a cyclic dependency but we
  101
+        don't care.
  102
+        """
  103
+        return self.data.keys()
  104
+
19 105
 class QuerySet(object):
20 106
     "Represents a lazy database lookup for a set of objects"
21 107
     def __init__(self, model=None, query=None):
@@ -275,7 +361,7 @@ def delete(self):
275 361
         while 1:
276 362
             # Collect all the objects to be deleted in this chunk, and all the
277 363
             # objects that are related to the objects that are to be deleted.
278  
-            seen_objs = SortedDict()
  364
+            seen_objs = CollectedObjects()
279 365
             for object in del_query[:CHUNK_SIZE]:
280 366
                 object._collect_sub_objects(seen_objs)
281 367
 
@@ -682,19 +768,27 @@ def delete_objects(seen_objs):
682 768
     Iterate through a list of seen classes, and remove any instances that are
683 769
     referred to.
684 770
     """
685  
-    ordered_classes = seen_objs.keys()
686  
-    ordered_classes.reverse()
687  
-
  771
+    try:
  772
+        ordered_classes = seen_objs.keys()
  773
+    except CyclicDependency:
  774
+        # if there is a cyclic dependency, we cannot in general delete
  775
+        # the objects.  However, if an appropriate transaction is set
  776
+        # up, or if the database is lax enough, it will succeed. 
  777
+        # So for now, we go ahead and try anway.
  778
+        ordered_classes = seen_objs.unordered_keys()
  779
+
  780
+    obj_pairs = {}
688 781
     for cls in ordered_classes:
689  
-        seen_objs[cls] = seen_objs[cls].items()
690  
-        seen_objs[cls].sort()
  782
+        items = seen_objs[cls].items()
  783
+        items.sort()
  784
+        obj_pairs[cls] = items
691 785
 
692 786
         # Pre notify all instances to be deleted
693  
-        for pk_val, instance in seen_objs[cls]:
  787
+        for pk_val, instance in items:
694 788
             dispatcher.send(signal=signals.pre_delete, sender=cls,
695 789
                     instance=instance)
696 790
 
697  
-        pk_list = [pk for pk,instance in seen_objs[cls]]
  791
+        pk_list = [pk for pk,instance in items]
698 792
         del_query = sql.DeleteQuery(cls, connection)
699 793
         del_query.delete_batch_related(pk_list)
700 794
 
@@ -705,15 +799,17 @@ def delete_objects(seen_objs):
705 799
 
706 800
     # Now delete the actual data
707 801
     for cls in ordered_classes:
708  
-        seen_objs[cls].reverse()
709  
-        pk_list = [pk for pk,instance in seen_objs[cls]]
  802
+        items = obj_pairs[cls]
  803
+        items.reverse()
  804
+
  805
+        pk_list = [pk for pk,instance in items]
710 806
         del_query = sql.DeleteQuery(cls, connection)
711 807
         del_query.delete_batch(pk_list)
712 808
 
713 809
         # Last cleanup; set NULLs where there once was a reference to the
714 810
         # object, NULL the primary key of the found objects, and perform
715 811
         # post-notification.
716  
-        for pk_val, instance in seen_objs[cls]:
  812
+        for pk_val, instance in items:
717 813
             for field in cls._meta.fields:
718 814
                 if field.rel and field.null and field.rel.to in seen_objs:
719 815
                     setattr(instance, field.attname, None)
105  tests/modeltests/delete/models.py
@@ -33,8 +33,46 @@ class D(DefaultRepr, models.Model):
33 33
 # However, if we start at As, we might find Bs first (in which 
34 34
 # case things will be nice), or find Ds first.
35 35
 
  36
+# Some mutually dependent models, but nullable
  37
+class E(DefaultRepr, models.Model):
  38
+    f = models.ForeignKey('F', null=True, related_name='e_rel')
  39
+
  40
+class F(DefaultRepr, models.Model):
  41
+    e = models.ForeignKey(E, related_name='f_rel')
  42
+
36 43
 
37 44
 __test__ = {'API_TESTS': """
  45
+# First, some tests for the datastructure we use
  46
+
  47
+>>> from django.db.models.query import CollectedObjects
  48
+
  49
+>>> g = CollectedObjects()
  50
+>>> g.add("key1", 1, "item1", None)
  51
+False
  52
+>>> g["key1"]
  53
+{1: 'item1'}
  54
+>>> g.add("key2", 1, "item1", "key1")
  55
+False
  56
+>>> g.add("key2", 2, "item2", "key1")
  57
+False
  58
+>>> g["key2"]
  59
+{1: 'item1', 2: 'item2'}
  60
+>>> g.add("key3", 1, "item1", "key1")
  61
+False
  62
+>>> g.add("key3", 1, "item1", "key2")
  63
+True
  64
+>>> g.ordered_keys()
  65
+['key3', 'key2', 'key1']
  66
+
  67
+>>> g.add("key2", 1, "item1", "key3")
  68
+True
  69
+>>> g.ordered_keys()
  70
+Traceback (most recent call last):
  71
+    ...
  72
+CyclicDependency: There is a cyclic dependency of items to be processed.
  73
+
  74
+
  75
+
38 76
 # Due to the way that transactions work in the test harness,
39 77
 # doing m.delete() here can work but fail in a real situation,
40 78
 # since it may delete all objects, but not in the right order.
@@ -42,11 +80,10 @@ class D(DefaultRepr, models.Model):
42 80
 
43 81
 # Also, it is possible that the order is correct 'accidentally', due
44 82
 # solely to order of imports etc.  To check this, we set the order
45  
-# that 'get_models()' will retrieve to a known 'tricky' order, and
46  
-# then try again with the reverse and try again.  Slightly naughty
47  
-# access to internals here.
  83
+# that 'get_models()' will retrieve to a known 'nice' order, and
  84
+# then try again with a known 'tricky' order.  Slightly naughty
  85
+# access to internals here :-)
48 86
 
49  
->>> from django.utils.datastructures import SortedDict
50 87
 >>> from django.db.models.loading import cache
51 88
 
52 89
 # Nice order
@@ -56,8 +93,6 @@ class D(DefaultRepr, models.Model):
56 93
 >>> del C._meta._related_objects_cache
57 94
 >>> del D._meta._related_objects_cache
58 95
 
59  
-
60  
-
61 96
 >>> a1 = A()
62 97
 >>> a1.save()
63 98
 >>> b1 = B(a=a1)
@@ -67,9 +102,9 @@ class D(DefaultRepr, models.Model):
67 102
 >>> d1 = D(c=c1, a=a1)
68 103
 >>> d1.save()
69 104
 
70  
->>> sd = SortedDict()
71  
->>> a1._collect_sub_objects(sd)
72  
->>> list(reversed(sd.keys()))
  105
+>>> o = CollectedObjects()
  106
+>>> a1._collect_sub_objects(o)
  107
+>>> o.keys()
73 108
 [<class 'modeltests.delete.models.D'>, <class 'modeltests.delete.models.C'>, <class 'modeltests.delete.models.B'>, <class 'modeltests.delete.models.A'>]
74 109
 >>> a1.delete()
75 110
 
@@ -80,7 +115,6 @@ class D(DefaultRepr, models.Model):
80 115
 >>> del C._meta._related_objects_cache
81 116
 >>> del D._meta._related_objects_cache
82 117
 
83  
-
84 118
 >>> a2 = A()
85 119
 >>> a2.save()
86 120
 >>> b2 = B(a=a2)
@@ -90,13 +124,56 @@ class D(DefaultRepr, models.Model):
90 124
 >>> d2 = D(c=c2, a=a2)
91 125
 >>> d2.save()
92 126
 
93  
->>> sd2 = SortedDict()
94  
->>> a2._collect_sub_objects(sd2)
95  
->>> list(reversed(sd2.keys()))
  127
+>>> o = CollectedObjects()
  128
+>>> a2._collect_sub_objects(o)
  129
+>>> o.keys()
96 130
 [<class 'modeltests.delete.models.D'>, <class 'modeltests.delete.models.C'>, <class 'modeltests.delete.models.B'>, <class 'modeltests.delete.models.A'>]
97 131
 >>> a2.delete()
98 132
 
99  
-
  133
+# Tests for nullable related fields
  134
+
  135
+>>> g = CollectedObjects()
  136
+>>> g.add("key1", 1, "item1", None)
  137
+False
  138
+>>> g.add("key2", 1, "item1", "key1", nullable=True)
  139
+False
  140
+>>> g.add("key1", 1, "item1", "key2")
  141
+True
  142
+>>> g.ordered_keys()
  143
+['key1', 'key2']
  144
+
  145
+>>> e1 = E()
  146
+>>> e1.save()
  147
+>>> f1 = F(e=e1)
  148
+>>> f1.save()
  149
+>>> e1.f = f1
  150
+>>> e1.save()
  151
+
  152
+# Since E.f is nullable, we should delete F first (after nulling out
  153
+# the E.f field), then E.
  154
+
  155
+>>> o = CollectedObjects()
  156
+>>> e1._collect_sub_objects(o)
  157
+>>> o.keys()
  158
+[<class 'modeltests.delete.models.F'>, <class 'modeltests.delete.models.E'>]
  159
+
  160
+>>> e1.delete()
  161
+
  162
+>>> e2 = E()
  163
+>>> e2.save()
  164
+>>> f2 = F(e=e2)
  165
+>>> f2.save()
  166
+>>> e2.f = f2
  167
+>>> e2.save()
  168
+
  169
+# Same deal as before, though we are starting from the other object.
  170
+
  171
+>>> o = CollectedObjects()
  172
+>>> f2._collect_sub_objects(o)
  173
+>>> o.keys()
  174
+[<class 'modeltests.delete.models.F'>, <class 'modeltests.delete.models.E'>]
  175
+
  176
+>>> f2.delete()
100 177
 
101 178
 """
102 179
 }

0 notes on commit b1851cc

Please sign in to comment.
Something went wrong with that request. Please try again.