Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Fixed #21169 -- Reworked RelatedManager methods use default filtering

The `remove()` and `clear()` methods of the related managers created by
`ForeignKey`, `GenericForeignKey`, and `ManyToManyField` suffered from a
number of issues. Some operations ran multiple data modifying queries without
wrapping them in a transaction, and some operations didn't respect default
filtering when it was present (i.e. when the default manager on the related
model implemented a custom `get_queryset()`).

Fixing the issues introduced some backward incompatible changes:

- The implementation of `remove()` for `ForeignKey` related managers changed
  from a series of `Model.save()` calls to a single `QuerySet.update()` call.
  The change means that `pre_save` and `post_save` signals aren't called anymore.

- The `remove()` and `clear()` methods for `GenericForeignKey` related
  managers now perform bulk delete so `Model.delete()` isn't called anymore.

- The `remove()` and `clear()` methods for `ManyToManyField` related
  managers perform nested queries when filtering is involved, which may
  or may not be an issue depending on the database and the data itself.

Refs. #3871, #21174.

Thanks Anssi Kääriäinen and Tim Graham for the reviews.
  • Loading branch information...
commit 17c3997f6828e88e4646071a8187c1318b65597d 1 parent 0b3c8fc
Loic Bistuer authored September 27, 2013 akaariai committed November 27, 2013
117  django/db/models/fields/related.py
@@ -2,7 +2,7 @@
@@ -464,14 +464,21 @@ def get_or_create(self, **kwargs):
@@ -536,6 +543,7 @@ def __init__(self, model=None, query_field_name=None, instance=None, symmetrical
@@ -572,6 +580,19 @@ def __call__(self, **kwargs):
@@ -625,18 +646,20 @@ def add(self, *objs):
@@ -722,55 +745,33 @@ def _remove_items(self, source_field_name, target_field_name, *objs):
2  docs/ref/models/querysets.txt
@@ -2132,6 +2132,8 @@ extract two field values, where only one is expected::
2132 2132
     inner_qs = Blog.objects.filter(name__contains='Ch').values('name', 'id')
2133 2133
     entries = Entry.objects.filter(blog__name__in=inner_qs)
2134 2134
 
  2135
+.. _nested-queries-performance:
  2136
+
2135 2137
 .. admonition:: Performance considerations
2136 2138
 
2137 2139
     Be cautious about using nested queries and understand your database
26  docs/releases/1.7.txt
@@ -574,6 +574,32 @@ a :exc:`~exceptions.ValueError` when encountering them, you will have to
574 574
 install pytz_. You may be affected by this problem if you use Django's time
575 575
 zone-related date formats or :mod:`django.contrib.syndication`.
576 576
 
  577
+``remove()`` and ``clear()`` methods of related managers
  578
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  579
+
  580
+The ``remove()`` and ``clear()`` methods of the related managers created by
  581
+``ForeignKey``, ``GenericForeignKey``, and ``ManyToManyField`` suffered from a
  582
+number of issues. Some operations ran multiple data modifying queries without
  583
+wrapping them in a transaction, and some operations didn't respect default
  584
+filtering when it was present (i.e. when the default manager on the related
  585
+model implemented a custom ``get_queryset()``).
  586
+
  587
+Fixing the issues introduced some backward incompatible changes:
  588
+
  589
+- The default implementation of ``remove()`` for ``ForeignKey`` related managers
  590
+  changed from a series of ``Model.save()`` calls to a single
  591
+  ``QuerySet.update()`` call. The change means that ``pre_save`` and
  592
+  ``post_save`` signals aren't sent anymore.
  593
+
  594
+- The ``remove()`` and ``clear()`` methods for ``GenericForeignKey`` related
  595
+  managers now perform bulk delete. The ``Model.delete()`` method isn't called
  596
+  on each instance anymore.
  597
+
  598
+- The ``remove()`` and ``clear()`` methods for ``ManyToManyField`` related
  599
+  managers perform nested queries when filtering is involved, which may or
  600
+  may not be an issue depending on your database and your data itself.
  601
+  See :ref:`this note <nested-queries-performance>` for more details.
  602
+
577 603
 .. _pytz: https://pypi.python.org/pypi/pytz/
578 604
 
579 605
 Miscellaneous
20  tests/custom_managers/models.py
@@ -102,16 +102,36 @@ def __str__(self):
102 102
 
103 103
 
104 104
 @python_2_unicode_compatible
  105
+class FunPerson(models.Model):
  106
+    first_name = models.CharField(max_length=30)
  107
+    last_name = models.CharField(max_length=30)
  108
+    fun = models.BooleanField(default=True)
  109
+
  110
+    favorite_book = models.ForeignKey('Book', null=True, related_name='fun_people_favorite_books')
  111
+    favorite_thing_type = models.ForeignKey('contenttypes.ContentType', null=True)
  112
+    favorite_thing_id = models.IntegerField(null=True)
  113
+    favorite_thing = generic.GenericForeignKey('favorite_thing_type', 'favorite_thing_id')
  114
+
  115
+    objects = FunPeopleManager()
  116
+
  117
+    def __str__(self):
  118
+        return "%s %s" % (self.first_name, self.last_name)
  119
+
  120
+@python_2_unicode_compatible
105 121
 class Book(models.Model):
106 122
     title = models.CharField(max_length=50)
107 123
     author = models.CharField(max_length=30)
108 124
     is_published = models.BooleanField(default=False)
109 125
     published_objects = PublishedBookManager()
110 126
     authors = models.ManyToManyField(Person, related_name='books')
  127
+    fun_authors = models.ManyToManyField(FunPerson, related_name='books')
111 128
 
112 129
     favorite_things = generic.GenericRelation(Person,
113 130
         content_type_field='favorite_thing_type', object_id_field='favorite_thing_id')
114 131
 
  132
+    fun_people_favorite_things = generic.GenericRelation(FunPerson,
  133
+        content_type_field='favorite_thing_type', object_id_field='favorite_thing_id')
  134
+
115 135
     def __str__(self):
116 136
         return self.title
117 137
 
318  tests/custom_managers/tests.py
@@ -3,7 +3,7 @@
3 3
 from django.test import TestCase
4 4
 from django.utils import six
5 5
 
6  
-from .models import Person, Book, Car, PersonManager, PublishedBookManager
  6
+from .models import Person, FunPerson, Book, Car, PersonManager, PublishedBookManager
7 7
 
8 8
 
9 9
 class CustomManagerTests(TestCase):
@@ -12,10 +12,11 @@ def setUp(self):
12 12
             title="How to program", author="Rodney Dangerfield", is_published=True)
13 13
         self.b2 = Book.published_objects.create(
14 14
             title="How to be smart", author="Albert Einstein", is_published=False)
15  
-        self.p1 = Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
16  
-        self.p2 = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False)
17 15
 
18 16
     def test_manager(self):
  17
+        Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
  18
+        droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False)
  19
+
19 20
         # Test a custom `Manager` method.
20 21
         self.assertQuerysetEqual(
21 22
             Person.objects.get_fun_people(), [
@@ -66,7 +67,7 @@ def test_manager(self):
66 67
 
67 68
         # The RelatedManager used on the 'books' descriptor extends the default
68 69
         # manager
69  
-        self.assertIsInstance(self.p2.books, PublishedBookManager)
  70
+        self.assertIsInstance(droopy.books, PublishedBookManager)
70 71
 
71 72
         # The default manager, "objects", doesn't exist, because a custom one
72 73
         # was provided.
@@ -113,78 +114,341 @@ def test_manager(self):
113 114
             lambda c: c.name
114 115
         )
115 116
 
116  
-    def test_related_manager_fk(self):
117  
-        self.p1.favorite_book = self.b1
118  
-        self.p1.save()
119  
-        self.p2.favorite_book = self.b1
120  
-        self.p2.save()
  117
+    def test_fk_related_manager(self):
  118
+        Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1)
  119
+        Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1)
  120
+        FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1)
  121
+        FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1)
121 122
 
122 123
         self.assertQuerysetEqual(
123 124
             self.b1.favorite_books.order_by('first_name').all(), [
124 125
                 "Bugs",
125 126
                 "Droopy",
126 127
             ],
127  
-            lambda c: c.first_name
  128
+            lambda c: c.first_name,
  129
+            ordered=False,
  130
+        )
  131
+        self.assertQuerysetEqual(
  132
+            self.b1.fun_people_favorite_books.all(), [
  133
+                "Bugs",
  134
+            ],
  135
+            lambda c: c.first_name,
  136
+            ordered=False,
128 137
         )
129 138
         self.assertQuerysetEqual(
130 139
             self.b1.favorite_books(manager='boring_people').all(), [
131 140
                 "Droopy",
132 141
             ],
133  
-            lambda c: c.first_name
  142
+            lambda c: c.first_name,
  143
+            ordered=False,
134 144
         )
135 145
         self.assertQuerysetEqual(
136 146
             self.b1.favorite_books(manager='fun_people').all(), [
137 147
                 "Bugs",
138 148
             ],
139  
-            lambda c: c.first_name
  149
+            lambda c: c.first_name,
  150
+            ordered=False,
140 151
         )
141 152
 
142  
-    def test_related_manager_gfk(self):
143  
-        self.p1.favorite_thing = self.b1
144  
-        self.p1.save()
145  
-        self.p2.favorite_thing = self.b1
146  
-        self.p2.save()
  153
+    def test_gfk_related_manager(self):
  154
+        Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1)
  155
+        Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1)
  156
+        FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1)
  157
+        FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1)
147 158
 
148 159
         self.assertQuerysetEqual(
149  
-            self.b1.favorite_things.order_by('first_name').all(), [
  160
+            self.b1.favorite_things.all(), [
150 161
                 "Bugs",
151 162
                 "Droopy",
152 163
             ],
153  
-            lambda c: c.first_name
  164
+            lambda c: c.first_name,
  165
+            ordered=False,
  166
+        )
  167
+        self.assertQuerysetEqual(
  168
+            self.b1.fun_people_favorite_things.all(), [
  169
+                "Bugs",
  170
+            ],
  171
+            lambda c: c.first_name,
  172
+            ordered=False,
154 173
         )
155 174
         self.assertQuerysetEqual(
156 175
             self.b1.favorite_things(manager='boring_people').all(), [
157 176
                 "Droopy",
158 177
             ],
159  
-            lambda c: c.first_name
  178
+            lambda c: c.first_name,
  179
+            ordered=False,
160 180
         )
161 181
         self.assertQuerysetEqual(
162 182
             self.b1.favorite_things(manager='fun_people').all(), [
163 183
                 "Bugs",
164 184
             ],
165  
-            lambda c: c.first_name
  185
+            lambda c: c.first_name,
  186
+            ordered=False,
166 187
         )
167 188
 
168  
-    def test_related_manager_m2m(self):
169  
-        self.b1.authors.add(self.p1)
170  
-        self.b1.authors.add(self.p2)
  189
+    def test_m2m_related_manager(self):
  190
+        bugs = Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
  191
+        self.b1.authors.add(bugs)
  192
+        droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False)
  193
+        self.b1.authors.add(droopy)
  194
+        bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
  195
+        self.b1.fun_authors.add(bugs)
  196
+        droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False)
  197
+        self.b1.fun_authors.add(droopy)
171 198
 
172 199
         self.assertQuerysetEqual(
173 200
             self.b1.authors.order_by('first_name').all(), [
174 201
                 "Bugs",
175 202
                 "Droopy",
176 203
             ],
177  
-            lambda c: c.first_name
  204
+            lambda c: c.first_name,
  205
+            ordered=False,
  206
+        )
  207
+        self.assertQuerysetEqual(
  208
+            self.b1.fun_authors.order_by('first_name').all(), [
  209
+                "Bugs",
  210
+            ],
  211
+            lambda c: c.first_name,
  212
+            ordered=False,
178 213
         )
179 214
         self.assertQuerysetEqual(
180 215
             self.b1.authors(manager='boring_people').all(), [
181 216
                 "Droopy",
182 217
             ],
183  
-            lambda c: c.first_name
  218
+            lambda c: c.first_name,
  219
+            ordered=False,
184 220
         )
185 221
         self.assertQuerysetEqual(
186 222
             self.b1.authors(manager='fun_people').all(), [
187 223
                 "Bugs",
188 224
             ],
189  
-            lambda c: c.first_name
  225
+            lambda c: c.first_name,
  226
+            ordered=False,
  227
+        )
  228
+
  229
+    def test_removal_through_default_fk_related_manager(self):
  230
+        bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1)
  231
+        droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1)
  232
+
  233
+        self.b1.fun_people_favorite_books.remove(droopy)
  234
+        self.assertQuerysetEqual(
  235
+            FunPerson._base_manager.filter(favorite_book=self.b1), [
  236
+                "Bugs",
  237
+                "Droopy",
  238
+            ],
  239
+            lambda c: c.first_name,
  240
+            ordered=False,
  241
+        )
  242
+
  243
+        self.b1.fun_people_favorite_books.remove(bugs)
  244
+        self.assertQuerysetEqual(
  245
+            FunPerson._base_manager.filter(favorite_book=self.b1), [
  246
+                "Droopy",
  247
+            ],
  248
+            lambda c: c.first_name,
  249
+            ordered=False,
  250
+        )
  251
+        bugs.favorite_book = self.b1
  252
+        bugs.save()
  253
+
  254
+        self.b1.fun_people_favorite_books.clear()
  255
+        self.assertQuerysetEqual(
  256
+            FunPerson._base_manager.filter(favorite_book=self.b1), [
  257
+                "Droopy",
  258
+            ],
  259
+            lambda c: c.first_name,
  260
+            ordered=False,
  261
+        )
  262
+
  263
+    def test_removal_through_specified_fk_related_manager(self):
  264
+        Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1)
  265
+        droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1)
  266
+
  267
+        # Check that the fun manager DOESN'T remove boring people.
  268
+        self.b1.favorite_books(manager='fun_people').remove(droopy)
  269
+        self.assertQuerysetEqual(
  270
+            self.b1.favorite_books(manager='boring_people').all(), [
  271
+                "Droopy",
  272
+            ],
  273
+            lambda c: c.first_name,
  274
+            ordered=False,
  275
+        )
  276
+        # Check that the boring manager DOES remove boring people.
  277
+        self.b1.favorite_books(manager='boring_people').remove(droopy)
  278
+        self.assertQuerysetEqual(
  279
+            self.b1.favorite_books(manager='boring_people').all(), [
  280
+            ],
  281
+            lambda c: c.first_name,
  282
+            ordered=False,
  283
+        )
  284
+        droopy.favorite_book = self.b1
  285
+        droopy.save()
  286
+
  287
+        # Check that the fun manager ONLY clears fun people.
  288
+        self.b1.favorite_books(manager='fun_people').clear()
  289
+        self.assertQuerysetEqual(
  290
+            self.b1.favorite_books(manager='boring_people').all(), [
  291
+                "Droopy",
  292
+            ],
  293
+            lambda c: c.first_name,
  294
+            ordered=False,
  295
+        )
  296
+        self.assertQuerysetEqual(
  297
+            self.b1.favorite_books(manager='fun_people').all(), [
  298
+            ],
  299
+            lambda c: c.first_name,
  300
+            ordered=False,
  301
+        )
  302
+
  303
+    def test_removal_through_default_gfk_related_manager(self):
  304
+        bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1)
  305
+        droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1)
  306
+
  307
+        self.b1.fun_people_favorite_things.remove(droopy)
  308
+        self.assertQuerysetEqual(
  309
+            FunPerson._base_manager.order_by('first_name').filter(favorite_thing_id=self.b1.pk), [
  310
+                "Bugs",
  311
+                "Droopy",
  312
+            ],
  313
+            lambda c: c.first_name,
  314
+            ordered=False,
  315
+        )
  316
+
  317
+        self.b1.fun_people_favorite_things.remove(bugs)
  318
+        self.assertQuerysetEqual(
  319
+            FunPerson._base_manager.order_by('first_name').filter(favorite_thing_id=self.b1.pk), [
  320
+                "Droopy",
  321
+            ],
  322
+            lambda c: c.first_name,
  323
+            ordered=False,
  324
+        )
  325
+        bugs.favorite_book = self.b1
  326
+        bugs.save()
  327
+
  328
+        self.b1.fun_people_favorite_things.clear()
  329
+        self.assertQuerysetEqual(
  330
+            FunPerson._base_manager.order_by('first_name').filter(favorite_thing_id=self.b1.pk), [
  331
+                "Droopy",
  332
+            ],
  333
+            lambda c: c.first_name,
  334
+            ordered=False,
  335
+        )
  336
+
  337
+    def test_removal_through_specified_gfk_related_manager(self):
  338
+        Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1)
  339
+        droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1)
  340
+
  341
+        # Check that the fun manager DOESN'T remove boring people.
  342
+        self.b1.favorite_things(manager='fun_people').remove(droopy)
  343
+        self.assertQuerysetEqual(
  344
+            self.b1.favorite_things(manager='boring_people').all(), [
  345
+                "Droopy",
  346
+            ],
  347
+            lambda c: c.first_name,
  348
+            ordered=False,
  349
+        )
  350
+
  351
+        # Check that the boring manager DOES remove boring people.
  352
+        self.b1.favorite_things(manager='boring_people').remove(droopy)
  353
+        self.assertQuerysetEqual(
  354
+            self.b1.favorite_things(manager='boring_people').all(), [
  355
+            ],
  356
+            lambda c: c.first_name,
  357
+            ordered=False,
  358
+        )
  359
+        droopy.favorite_thing = self.b1
  360
+        droopy.save()
  361
+
  362
+        # Check that the fun manager ONLY clears fun people.
  363
+        self.b1.favorite_things(manager='fun_people').clear()
  364
+        self.assertQuerysetEqual(
  365
+            self.b1.favorite_things(manager='boring_people').all(), [
  366
+                "Droopy",
  367
+            ],
  368
+            lambda c: c.first_name,
  369
+            ordered=False,
  370
+        )
  371
+        self.assertQuerysetEqual(
  372
+            self.b1.favorite_things(manager='fun_people').all(), [
  373
+            ],
  374
+            lambda c: c.first_name,
  375
+            ordered=False,
  376
+        )
  377
+
  378
+    def test_removal_through_default_m2m_related_manager(self):
  379
+        bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
  380
+        self.b1.fun_authors.add(bugs)
  381
+        droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False)
  382
+        self.b1.fun_authors.add(droopy)
  383
+
  384
+        self.b1.fun_authors.remove(droopy)
  385
+        self.assertQuerysetEqual(
  386
+            self.b1.fun_authors.through._default_manager.all(), [
  387
+                "Bugs",
  388
+                "Droopy",
  389
+            ],
  390
+            lambda c: c.funperson.first_name,
  391
+            ordered=False,
  392
+        )
  393
+
  394
+        self.b1.fun_authors.remove(bugs)
  395
+        self.assertQuerysetEqual(
  396
+            self.b1.fun_authors.through._default_manager.all(), [
  397
+                "Droopy",
  398
+            ],
  399
+            lambda c: c.funperson.first_name,
  400
+            ordered=False,
  401
+        )
  402
+        self.b1.fun_authors.add(bugs)
  403
+
  404
+        self.b1.fun_authors.clear()
  405
+        self.assertQuerysetEqual(
  406
+            self.b1.fun_authors.through._default_manager.all(), [
  407
+                "Droopy",
  408
+            ],
  409
+            lambda c: c.funperson.first_name,
  410
+            ordered=False,
  411
+        )
  412
+
  413
+    def test_removal_through_specified_m2m_related_manager(self):
  414
+        bugs = Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
  415
+        self.b1.authors.add(bugs)
  416
+        droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False)
  417
+        self.b1.authors.add(droopy)
  418
+
  419
+        # Check that the fun manager DOESN'T remove boring people.
  420
+        self.b1.authors(manager='fun_people').remove(droopy)
  421
+        self.assertQuerysetEqual(
  422
+            self.b1.authors(manager='boring_people').all(), [
  423
+                "Droopy",
  424
+            ],
  425
+            lambda c: c.first_name,
  426
+            ordered=False,
  427
+        )
  428
+
  429
+        # Check that the boring manager DOES remove boring people.
  430
+        self.b1.authors(manager='boring_people').remove(droopy)
  431
+        self.assertQuerysetEqual(
  432
+            self.b1.authors(manager='boring_people').all(), [
  433
+            ],
  434
+            lambda c: c.first_name,
  435
+            ordered=False,
  436
+        )
  437
+        self.b1.authors.add(droopy)
  438
+
  439
+
  440
+        # Check that the fun manager ONLY clears fun people.
  441
+        self.b1.authors(manager='fun_people').clear()
  442
+        self.assertQuerysetEqual(
  443
+            self.b1.authors(manager='boring_people').all(), [
  444
+                "Droopy",
  445
+            ],
  446
+            lambda c: c.first_name,
  447
+            ordered=False,
  448
+        )
  449
+        self.assertQuerysetEqual(
  450
+            self.b1.authors(manager='fun_people').all(), [
  451
+            ],
  452
+            lambda c: c.first_name,
  453
+            ordered=False,
190 454
         )

0 notes on commit 17c3997

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