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.

2 files changed, 311 insertions, 27 deletions

diff --git a/tests/custom_managers/models.py b/tests/custom_managers/models.py
index c6a99620e9..726ece7dab 100644
--- a/tests/custom_managers/models.py
+++ b/tests/custom_managers/models.py
@@ -102,16 +102,36 @@ class Person(models.Model):
 
 
 @python_2_unicode_compatible
+class FunPerson(models.Model):
+    first_name = models.CharField(max_length=30)
+    last_name = models.CharField(max_length=30)
+    fun = models.BooleanField(default=True)
+
+    favorite_book = models.ForeignKey('Book', null=True, related_name='fun_people_favorite_books')
+    favorite_thing_type = models.ForeignKey('contenttypes.ContentType', null=True)
+    favorite_thing_id = models.IntegerField(null=True)
+    favorite_thing = generic.GenericForeignKey('favorite_thing_type', 'favorite_thing_id')
+
+    objects = FunPeopleManager()
+
+    def __str__(self):
+        return "%s %s" % (self.first_name, self.last_name)
+
+@python_2_unicode_compatible
 class Book(models.Model):
     title = models.CharField(max_length=50)
     author = models.CharField(max_length=30)
     is_published = models.BooleanField(default=False)
     published_objects = PublishedBookManager()
     authors = models.ManyToManyField(Person, related_name='books')
+    fun_authors = models.ManyToManyField(FunPerson, related_name='books')
 
     favorite_things = generic.GenericRelation(Person,
         content_type_field='favorite_thing_type', object_id_field='favorite_thing_id')
 
+    fun_people_favorite_things = generic.GenericRelation(FunPerson,
+        content_type_field='favorite_thing_type', object_id_field='favorite_thing_id')
+
     def __str__(self):
         return self.title
 
diff --git a/tests/custom_managers/tests.py b/tests/custom_managers/tests.py
index 021decc455..af36110eca 100644
--- a/tests/custom_managers/tests.py
+++ b/tests/custom_managers/tests.py
@@ -3,7 +3,7 @@ from __future__ import unicode_literals
 from django.test import TestCase
 from django.utils import six
 
-from .models import Person, Book, Car, PersonManager, PublishedBookManager
+from .models import Person, FunPerson, Book, Car, PersonManager, PublishedBookManager
 
 
 class CustomManagerTests(TestCase):
@@ -12,10 +12,11 @@ class CustomManagerTests(TestCase):
             title="How to program", author="Rodney Dangerfield", is_published=True)
         self.b2 = Book.published_objects.create(
             title="How to be smart", author="Albert Einstein", is_published=False)
-        self.p1 = Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
-        self.p2 = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False)
 
     def test_manager(self):
+        Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
+        droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False)
+
         # Test a custom `Manager` method.
         self.assertQuerysetEqual(
             Person.objects.get_fun_people(), [
@@ -66,7 +67,7 @@ class CustomManagerTests(TestCase):
 
         # The RelatedManager used on the 'books' descriptor extends the default
         # manager
-        self.assertIsInstance(self.p2.books, PublishedBookManager)
+        self.assertIsInstance(droopy.books, PublishedBookManager)
 
         # The default manager, "objects", doesn't exist, because a custom one
         # was provided.
@@ -113,78 +114,341 @@ class CustomManagerTests(TestCase):
             lambda c: c.name
         )
 
-    def test_related_manager_fk(self):
-        self.p1.favorite_book = self.b1
-        self.p1.save()
-        self.p2.favorite_book = self.b1
-        self.p2.save()
+    def test_fk_related_manager(self):
+        Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1)
+        Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1)
+        FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1)
+        FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1)
 
         self.assertQuerysetEqual(
             self.b1.favorite_books.order_by('first_name').all(), [
                 "Bugs",
                 "Droopy",
             ],
-            lambda c: c.first_name
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        self.assertQuerysetEqual(
+            self.b1.fun_people_favorite_books.all(), [
+                "Bugs",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
         )
         self.assertQuerysetEqual(
             self.b1.favorite_books(manager='boring_people').all(), [
                 "Droopy",
             ],
-            lambda c: c.first_name
+            lambda c: c.first_name,
+            ordered=False,
         )
         self.assertQuerysetEqual(
             self.b1.favorite_books(manager='fun_people').all(), [
                 "Bugs",
             ],
-            lambda c: c.first_name
+            lambda c: c.first_name,
+            ordered=False,
         )
 
-    def test_related_manager_gfk(self):
-        self.p1.favorite_thing = self.b1
-        self.p1.save()
-        self.p2.favorite_thing = self.b1
-        self.p2.save()
+    def test_gfk_related_manager(self):
+        Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1)
+        Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1)
+        FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1)
+        FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1)
 
         self.assertQuerysetEqual(
-            self.b1.favorite_things.order_by('first_name').all(), [
+            self.b1.favorite_things.all(), [
                 "Bugs",
                 "Droopy",
             ],
-            lambda c: c.first_name
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        self.assertQuerysetEqual(
+            self.b1.fun_people_favorite_things.all(), [
+                "Bugs",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
         )
         self.assertQuerysetEqual(
             self.b1.favorite_things(manager='boring_people').all(), [
                 "Droopy",
             ],
-            lambda c: c.first_name
+            lambda c: c.first_name,
+            ordered=False,
         )
         self.assertQuerysetEqual(
             self.b1.favorite_things(manager='fun_people').all(), [
                 "Bugs",
             ],
-            lambda c: c.first_name
+            lambda c: c.first_name,
+            ordered=False,
         )
 
-    def test_related_manager_m2m(self):
-        self.b1.authors.add(self.p1)
-        self.b1.authors.add(self.p2)
+    def test_m2m_related_manager(self):
+        bugs = Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
+        self.b1.authors.add(bugs)
+        droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False)
+        self.b1.authors.add(droopy)
+        bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
+        self.b1.fun_authors.add(bugs)
+        droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False)
+        self.b1.fun_authors.add(droopy)
 
         self.assertQuerysetEqual(
             self.b1.authors.order_by('first_name').all(), [
                 "Bugs",
                 "Droopy",
             ],
-            lambda c: c.first_name
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        self.assertQuerysetEqual(
+            self.b1.fun_authors.order_by('first_name').all(), [
+                "Bugs",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
         )
         self.assertQuerysetEqual(
             self.b1.authors(manager='boring_people').all(), [
                 "Droopy",
             ],
-            lambda c: c.first_name
+            lambda c: c.first_name,
+            ordered=False,
         )
         self.assertQuerysetEqual(
             self.b1.authors(manager='fun_people').all(), [
                 "Bugs",
             ],
-            lambda c: c.first_name
+            lambda c: c.first_name,
+            ordered=False,
+        )
+
+    def test_removal_through_default_fk_related_manager(self):
+        bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1)
+        droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1)
+
+        self.b1.fun_people_favorite_books.remove(droopy)
+        self.assertQuerysetEqual(
+            FunPerson._base_manager.filter(favorite_book=self.b1), [
+                "Bugs",
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+
+        self.b1.fun_people_favorite_books.remove(bugs)
+        self.assertQuerysetEqual(
+            FunPerson._base_manager.filter(favorite_book=self.b1), [
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        bugs.favorite_book = self.b1
+        bugs.save()
+
+        self.b1.fun_people_favorite_books.clear()
+        self.assertQuerysetEqual(
+            FunPerson._base_manager.filter(favorite_book=self.b1), [
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+
+    def test_removal_through_specified_fk_related_manager(self):
+        Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1)
+        droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1)
+
+        # Check that the fun manager DOESN'T remove boring people.
+        self.b1.favorite_books(manager='fun_people').remove(droopy)
+        self.assertQuerysetEqual(
+            self.b1.favorite_books(manager='boring_people').all(), [
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        # Check that the boring manager DOES remove boring people.
+        self.b1.favorite_books(manager='boring_people').remove(droopy)
+        self.assertQuerysetEqual(
+            self.b1.favorite_books(manager='boring_people').all(), [
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        droopy.favorite_book = self.b1
+        droopy.save()
+
+        # Check that the fun manager ONLY clears fun people.
+        self.b1.favorite_books(manager='fun_people').clear()
+        self.assertQuerysetEqual(
+            self.b1.favorite_books(manager='boring_people').all(), [
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        self.assertQuerysetEqual(
+            self.b1.favorite_books(manager='fun_people').all(), [
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+
+    def test_removal_through_default_gfk_related_manager(self):
+        bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1)
+        droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1)
+
+        self.b1.fun_people_favorite_things.remove(droopy)
+        self.assertQuerysetEqual(
+            FunPerson._base_manager.order_by('first_name').filter(favorite_thing_id=self.b1.pk), [
+                "Bugs",
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+
+        self.b1.fun_people_favorite_things.remove(bugs)
+        self.assertQuerysetEqual(
+            FunPerson._base_manager.order_by('first_name').filter(favorite_thing_id=self.b1.pk), [
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        bugs.favorite_book = self.b1
+        bugs.save()
+
+        self.b1.fun_people_favorite_things.clear()
+        self.assertQuerysetEqual(
+            FunPerson._base_manager.order_by('first_name').filter(favorite_thing_id=self.b1.pk), [
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+
+    def test_removal_through_specified_gfk_related_manager(self):
+        Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1)
+        droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1)
+
+        # Check that the fun manager DOESN'T remove boring people.
+        self.b1.favorite_things(manager='fun_people').remove(droopy)
+        self.assertQuerysetEqual(
+            self.b1.favorite_things(manager='boring_people').all(), [
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+
+        # Check that the boring manager DOES remove boring people.
+        self.b1.favorite_things(manager='boring_people').remove(droopy)
+        self.assertQuerysetEqual(
+            self.b1.favorite_things(manager='boring_people').all(), [
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        droopy.favorite_thing = self.b1
+        droopy.save()
+
+        # Check that the fun manager ONLY clears fun people.
+        self.b1.favorite_things(manager='fun_people').clear()
+        self.assertQuerysetEqual(
+            self.b1.favorite_things(manager='boring_people').all(), [
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        self.assertQuerysetEqual(
+            self.b1.favorite_things(manager='fun_people').all(), [
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+
+    def test_removal_through_default_m2m_related_manager(self):
+        bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
+        self.b1.fun_authors.add(bugs)
+        droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False)
+        self.b1.fun_authors.add(droopy)
+
+        self.b1.fun_authors.remove(droopy)
+        self.assertQuerysetEqual(
+            self.b1.fun_authors.through._default_manager.all(), [
+                "Bugs",
+                "Droopy",
+            ],
+            lambda c: c.funperson.first_name,
+            ordered=False,
+        )
+
+        self.b1.fun_authors.remove(bugs)
+        self.assertQuerysetEqual(
+            self.b1.fun_authors.through._default_manager.all(), [
+                "Droopy",
+            ],
+            lambda c: c.funperson.first_name,
+            ordered=False,
+        )
+        self.b1.fun_authors.add(bugs)
+
+        self.b1.fun_authors.clear()
+        self.assertQuerysetEqual(
+            self.b1.fun_authors.through._default_manager.all(), [
+                "Droopy",
+            ],
+            lambda c: c.funperson.first_name,
+            ordered=False,
+        )
+
+    def test_removal_through_specified_m2m_related_manager(self):
+        bugs = Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True)
+        self.b1.authors.add(bugs)
+        droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False)
+        self.b1.authors.add(droopy)
+
+        # Check that the fun manager DOESN'T remove boring people.
+        self.b1.authors(manager='fun_people').remove(droopy)
+        self.assertQuerysetEqual(
+            self.b1.authors(manager='boring_people').all(), [
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+
+        # Check that the boring manager DOES remove boring people.
+        self.b1.authors(manager='boring_people').remove(droopy)
+        self.assertQuerysetEqual(
+            self.b1.authors(manager='boring_people').all(), [
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        self.b1.authors.add(droopy)
+
+
+        # Check that the fun manager ONLY clears fun people.
+        self.b1.authors(manager='fun_people').clear()
+        self.assertQuerysetEqual(
+            self.b1.authors(manager='boring_people').all(), [
+                "Droopy",
+            ],
+            lambda c: c.first_name,
+            ordered=False,
+        )
+        self.assertQuerysetEqual(
+            self.b1.authors(manager='fun_people').all(), [
+            ],
+            lambda c: c.first_name,
+            ordered=False,
         )