From f64a5ef404cb6fd28e008a01039a3beea2fa8e1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anssi=20K=C3=A4=C3=A4ri=C3=A4inen?= Date: Wed, 10 Oct 2012 15:58:39 +0300 Subject: [PATCH] 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 " 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 | 19 ++-- django/db/models/sql/subqueries.py | 8 +- .../regressiontests/delete_regress/models.py | 8 +- tests/regressiontests/delete_regress/tests.py | 86 ++++++++++++++++++- 4 files changed, 108 insertions(+), 13 deletions(-) diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index ad82b167a6..264e29eaf3 100644 --- a/django/db/models/sql/query.py +++ b/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.select_fields = [] - q.default_cols = False - q.select_related = False - q.set_extra_mask(('a',)) - q.set_aggregate_mask(()) + q.set_extra_mask(['a']) 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). diff --git a/django/db/models/sql/subqueries.py b/django/db/models/sql/subqueries.py index 9f3fb8ac22..1371ef58e9 100644 --- a/django/db/models/sql/subqueries.py +++ b/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] diff --git a/tests/regressiontests/delete_regress/models.py b/tests/regressiontests/delete_regress/models.py index 5db253f713..dbe383fb41 100644 --- a/tests/regressiontests/delete_regress/models.py +++ b/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) diff --git a/tests/regressiontests/delete_regress/tests.py b/tests/regressiontests/delete_regress/tests.py index ebe59bffd7..ec0d460f17 100644 --- a/tests/regressiontests/delete_regress/tests.py +++ b/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()) +