Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

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.
  • Loading branch information...
commit f64a5ef404cb6fd28e008a01039a3beea2fa8e1b 1 parent da56e1b
Anssi Kääriäinen authored October 10, 2012
19  django/db/models/sql/query.py
@@ -431,13 +431,9 @@ def get_count(self, using):
431 431
 
432 432
     def has_results(self, using):
433 433
         q = self.clone()
  434
+        q.clear_select_clause()
434 435
         q.add_extra({'a': 1}, None, None, None, None, None)
435  
-        q.select = []
436  
-        q.select_fields = []
437  
-        q.default_cols = False
438  
-        q.select_related = False
439  
-        q.set_extra_mask(('a',))
440  
-        q.set_aggregate_mask(())
  436
+        q.set_extra_mask(['a'])
441 437
         q.clear_ordering(True)
442 438
         q.set_limits(high=1)
443 439
         compiler = q.get_compiler(using=using)
@@ -1626,6 +1622,17 @@ def can_filter(self):
1626 1622
         """
1627 1623
         return not self.low_mark and self.high_mark is None
1628 1624
 
  1625
+    def clear_select_clause(self):
  1626
+        """
  1627
+        Removes all fields from SELECT clause.
  1628
+        """
  1629
+        self.select = []
  1630
+        self.select_fields = []
  1631
+        self.default_cols = False
  1632
+        self.select_related = False
  1633
+        self.set_extra_mask(())
  1634
+        self.set_aggregate_mask(())
  1635
+
1629 1636
     def clear_select_fields(self):
1630 1637
         """
1631 1638
         Clears the list of fields to select (but not extra_select columns).
8  django/db/models/sql/subqueries.py
@@ -70,8 +70,9 @@ def delete_qs(self, query, using):
70 70
                 self.delete_batch(values, using)
71 71
                 return
72 72
             else:
73  
-                values = innerq
  73
+                innerq.clear_select_clause()
74 74
                 innerq.select = [(self.get_initial_alias(), pk.column)]
  75
+                values = innerq
75 76
             where = self.where_class()
76 77
             where.add((Constraint(None, pk.column, pk), 'in', values), AND)
77 78
             self.where = where
@@ -237,11 +238,8 @@ def add_date_select(self, field_name, lookup_type, order='ASC'):
237 238
                 % field.name
238 239
         alias = result[3][-1]
239 240
         select = Date((alias, field.column), lookup_type)
  241
+        self.clear_select_clause()
240 242
         self.select = [select]
241  
-        self.select_fields = [None]
242  
-        self.select_related = False # See #7097.
243  
-        self.aggregates = SortedDict() # See 18056.
244  
-        self.set_extra_mask([])
245 243
         self.distinct = True
246 244
         self.order_by = order == 'ASC' and [1] or [-1]
247 245
 
8  tests/regressiontests/delete_regress/models.py
@@ -2,7 +2,6 @@
2 2
 from django.contrib.contenttypes.models import ContentType
3 3
 from django.db import models
4 4
 
5  
-
6 5
 class Award(models.Model):
7 6
     name = models.CharField(max_length=25)
8 7
     object_id = models.PositiveIntegerField()
@@ -93,3 +92,10 @@ class FooPhoto(models.Model):
93 92
 class FooFileProxy(FooFile):
94 93
     class Meta:
95 94
         proxy = True
  95
+
  96
+class OrgUnit(models.Model):
  97
+    name = models.CharField(max_length=64, unique=True)
  98
+
  99
+class Login(models.Model):
  100
+    description = models.CharField(max_length=32)
  101
+    orgunit = models.ForeignKey(OrgUnit)
86  tests/regressiontests/delete_regress/tests.py
@@ -8,7 +8,8 @@
8 8
 
9 9
 from .models import (Book, Award, AwardNote, Person, Child, Toy, PlayedWith,
10 10
     PlayedWithNote, Email, Researcher, Food, Eaten, Policy, Version, Location,
11  
-    Item, Image, File, Photo, FooFile, FooImage, FooPhoto, FooFileProxy)
  11
+    Item, Image, File, Photo, FooFile, FooImage, FooPhoto, FooFileProxy, Login,
  12
+    OrgUnit)
12 13
 
13 14
 
14 15
 # Can't run this test under SQLite, because you can't
@@ -265,3 +266,86 @@ def test_delete_proxy_pair(self):
265 266
         Image.objects.all().delete()
266 267
 
267 268
         self.assertEqual(len(FooFileProxy.objects.all()), 0)
  269
+
  270
+class Ticket19102Tests(TestCase):
  271
+    """
  272
+    Test different queries which alter the SELECT clause of the query. We
  273
+    also must be using a subquery for the deletion (that is, the original
  274
+    query has a join in it). The deletion should be done as "fast-path"
  275
+    deletion (that is, just one query for the .delete() call).
  276
+
  277
+    Note that .values() is not tested here on purpose. .values().delete()
  278
+    doesn't work for non fast-path deletes at all.
  279
+    """
  280
+    def setUp(self):
  281
+        self.o1 = OrgUnit.objects.create(name='o1')
  282
+        self.o2 = OrgUnit.objects.create(name='o2')
  283
+        self.l1 = Login.objects.create(description='l1', orgunit=self.o1)
  284
+        self.l2 = Login.objects.create(description='l2', orgunit=self.o2)
  285
+
  286
+    @skipUnlessDBFeature("update_can_self_select")
  287
+    def test_ticket_19102_annotate(self):
  288
+        with self.assertNumQueries(1):
  289
+            Login.objects.order_by('description').filter(
  290
+                orgunit__name__isnull=False
  291
+            ).annotate(
  292
+                n=models.Count('description')
  293
+            ).filter(
  294
+                n=1, pk=self.l1.pk
  295
+            ).delete()
  296
+        self.assertFalse(Login.objects.filter(pk=self.l1.pk).exists())
  297
+        self.assertTrue(Login.objects.filter(pk=self.l2.pk).exists())
  298
+
  299
+    @skipUnlessDBFeature("update_can_self_select")
  300
+    def test_ticket_19102_extra(self):
  301
+        with self.assertNumQueries(1):
  302
+            Login.objects.order_by('description').filter(
  303
+                orgunit__name__isnull=False
  304
+            ).extra(
  305
+                select={'extraf':'1'}
  306
+            ).filter(
  307
+                pk=self.l1.pk
  308
+            ).delete()
  309
+        self.assertFalse(Login.objects.filter(pk=self.l1.pk).exists())
  310
+        self.assertTrue(Login.objects.filter(pk=self.l2.pk).exists())
  311
+
  312
+    @skipUnlessDBFeature("update_can_self_select")
  313
+    @skipUnlessDBFeature('can_distinct_on_fields')
  314
+    def test_ticket_19102_distinct_on(self):
  315
+        # Both Login objs should have same description so that only the one
  316
+        # having smaller PK will be deleted.
  317
+        Login.objects.update(description='description')
  318
+        with self.assertNumQueries(1):
  319
+            Login.objects.distinct('description').order_by('pk').filter(
  320
+                orgunit__name__isnull=False
  321
+            ).delete()
  322
+        # Assumed that l1 which is created first has smaller PK.
  323
+        self.assertFalse(Login.objects.filter(pk=self.l1.pk).exists())
  324
+        self.assertTrue(Login.objects.filter(pk=self.l2.pk).exists())
  325
+
  326
+    @skipUnlessDBFeature("update_can_self_select")
  327
+    def test_ticket_19102_select_related(self):
  328
+        with self.assertNumQueries(1):
  329
+            Login.objects.filter(
  330
+                pk=self.l1.pk
  331
+            ).filter(
  332
+                orgunit__name__isnull=False
  333
+            ).order_by(
  334
+                'description'
  335
+            ).select_related('orgunit').delete()
  336
+        self.assertFalse(Login.objects.filter(pk=self.l1.pk).exists())
  337
+        self.assertTrue(Login.objects.filter(pk=self.l2.pk).exists())
  338
+    
  339
+    @skipUnlessDBFeature("update_can_self_select")
  340
+    def test_ticket_19102_defer(self):
  341
+        with self.assertNumQueries(1):
  342
+            Login.objects.filter(
  343
+                pk=self.l1.pk
  344
+            ).filter(
  345
+                orgunit__name__isnull=False
  346
+            ).order_by(
  347
+                'description'
  348
+            ).only('id').delete()
  349
+        self.assertFalse(Login.objects.filter(pk=self.l1.pk).exists())
  350
+        self.assertTrue(Login.objects.filter(pk=self.l2.pk).exists())
  351
+

0 notes on commit f64a5ef

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