Fixed #19102 -- Fixed fast-path delete for modified SELECT clause cases

There was a bug introduced in #18676 which caused fast-path deletes
implemented as "DELETE WHERE pk IN <subquery>" to fail if the SELECT
clause contained additional stuff (for example extra() and annotate()).

Thanks to Trac alias pressureman for spotting this regression.

django/db/models/sql/query.py

@@ -431,13 +431,9 @@ class Query(object):
 
     def has_results(self, using):
         q = self.clone()
+        q.clear_select_clause()
         q.add_extra({'a': 1}, None, None, None, None, None)
-        q.select = []
+        q.set_extra_mask(['a'])
-        q.select_fields = []
-        q.default_cols = False
-        q.select_related = False
-        q.set_extra_mask(('a',))
-        q.set_aggregate_mask(())
         q.clear_ordering(True)
         q.set_limits(high=1)
         compiler = q.get_compiler(using=using)
@@ -1626,6 +1622,17 @@ class Query(object):
         """
         return not self.low_mark and self.high_mark is None
 
+    def clear_select_clause(self):
+        """
+        Removes all fields from SELECT clause.
+        """
+        self.select = []
+        self.select_fields = []
+        self.default_cols = False
+        self.select_related = False
+        self.set_extra_mask(())
+        self.set_aggregate_mask(())
+
     def clear_select_fields(self):
         """
         Clears the list of fields to select (but not extra_select columns).

django/db/models/sql/subqueries.py

@@ -70,8 +70,9 @@ class DeleteQuery(Query):
                 self.delete_batch(values, using)
                 return
             else:
-                values = innerq
+                innerq.clear_select_clause()
                 innerq.select = [(self.get_initial_alias(), pk.column)]
+                values = innerq
             where = self.where_class()
             where.add((Constraint(None, pk.column, pk), 'in', values), AND)
             self.where = where
@@ -237,11 +238,8 @@ class DateQuery(Query):
                 % field.name
         alias = result[3][-1]
         select = Date((alias, field.column), lookup_type)
+        self.clear_select_clause()
         self.select = [select]
-        self.select_fields = [None]
-        self.select_related = False # See #7097.
-        self.aggregates = SortedDict() # See 18056.
-        self.set_extra_mask([])
         self.distinct = True
         self.order_by = order == 'ASC' and [1] or [-1]
 

tests/regressiontests/delete_regress/models.py

@@ -2,7 +2,6 @@ from django.contrib.contenttypes import generic
 from django.contrib.contenttypes.models import ContentType
 from django.db import models
 
-
 class Award(models.Model):
     name = models.CharField(max_length=25)
     object_id = models.PositiveIntegerField()
@@ -93,3 +92,10 @@ class FooPhoto(models.Model):
 class FooFileProxy(FooFile):
     class Meta:
         proxy = True
+
+class OrgUnit(models.Model):
+    name = models.CharField(max_length=64, unique=True)
+
+class Login(models.Model):
+    description = models.CharField(max_length=32)
+    orgunit = models.ForeignKey(OrgUnit)

tests/regressiontests/delete_regress/tests.py

@@ -8,7 +8,8 @@ from django.test import TestCase, TransactionTestCase, skipUnlessDBFeature
 
 from .models import (Book, Award, AwardNote, Person, Child, Toy, PlayedWith,
     PlayedWithNote, Email, Researcher, Food, Eaten, Policy, Version, Location,
-    Item, Image, File, Photo, FooFile, FooImage, FooPhoto, FooFileProxy)
+    Item, Image, File, Photo, FooFile, FooImage, FooPhoto, FooFileProxy, Login,
+    OrgUnit)
 
 
 # Can't run this test under SQLite, because you can't
@@ -265,3 +266,86 @@ class ProxyDeleteTest(TestCase):
         Image.objects.all().delete()
 
         self.assertEqual(len(FooFileProxy.objects.all()), 0)
+
+class Ticket19102Tests(TestCase):
+    """
+    Test different queries which alter the SELECT clause of the query. We
+    also must be using a subquery for the deletion (that is, the original
+    query has a join in it). The deletion should be done as "fast-path"
+    deletion (that is, just one query for the .delete() call).
+
+    Note that .values() is not tested here on purpose. .values().delete()
+    doesn't work for non fast-path deletes at all.
+    """
+    def setUp(self):
+        self.o1 = OrgUnit.objects.create(name='o1')
+        self.o2 = OrgUnit.objects.create(name='o2')
+        self.l1 = Login.objects.create(description='l1', orgunit=self.o1)
+        self.l2 = Login.objects.create(description='l2', orgunit=self.o2)
+
+    @skipUnlessDBFeature("update_can_self_select")
+    def test_ticket_19102_annotate(self):
+        with self.assertNumQueries(1):
+            Login.objects.order_by('description').filter(
+                orgunit__name__isnull=False
+            ).annotate(
+                n=models.Count('description')
+            ).filter(
+                n=1, pk=self.l1.pk
+            ).delete()
+        self.assertFalse(Login.objects.filter(pk=self.l1.pk).exists())
+        self.assertTrue(Login.objects.filter(pk=self.l2.pk).exists())
+
+    @skipUnlessDBFeature("update_can_self_select")
+    def test_ticket_19102_extra(self):
+        with self.assertNumQueries(1):
+            Login.objects.order_by('description').filter(
+                orgunit__name__isnull=False
+            ).extra(
+                select={'extraf':'1'}
+            ).filter(
+                pk=self.l1.pk
+            ).delete()
+        self.assertFalse(Login.objects.filter(pk=self.l1.pk).exists())
+        self.assertTrue(Login.objects.filter(pk=self.l2.pk).exists())
+
+    @skipUnlessDBFeature("update_can_self_select")
+    @skipUnlessDBFeature('can_distinct_on_fields')
+    def test_ticket_19102_distinct_on(self):
+        # Both Login objs should have same description so that only the one
+        # having smaller PK will be deleted.
+        Login.objects.update(description='description')
+        with self.assertNumQueries(1):
+            Login.objects.distinct('description').order_by('pk').filter(
+                orgunit__name__isnull=False
+            ).delete()
+        # Assumed that l1 which is created first has smaller PK.
+        self.assertFalse(Login.objects.filter(pk=self.l1.pk).exists())
+        self.assertTrue(Login.objects.filter(pk=self.l2.pk).exists())
+
+    @skipUnlessDBFeature("update_can_self_select")
+    def test_ticket_19102_select_related(self):
+        with self.assertNumQueries(1):
+            Login.objects.filter(
+                pk=self.l1.pk
+            ).filter(
+                orgunit__name__isnull=False
+            ).order_by(
+                'description'
+            ).select_related('orgunit').delete()
+        self.assertFalse(Login.objects.filter(pk=self.l1.pk).exists())
+        self.assertTrue(Login.objects.filter(pk=self.l2.pk).exists())
+    
+    @skipUnlessDBFeature("update_can_self_select")
+    def test_ticket_19102_defer(self):
+        with self.assertNumQueries(1):
+            Login.objects.filter(
+                pk=self.l1.pk
+            ).filter(
+                orgunit__name__isnull=False
+            ).order_by(
+                'description'
+            ).only('id').delete()
+        self.assertFalse(Login.objects.filter(pk=self.l1.pk).exists())
+        self.assertTrue(Login.objects.filter(pk=self.l2.pk).exists())
+