Skip to content

Commit 2d57300

Browse files
freakboy3742freakboy3742
committed
Fixed #12953 -- Ensure that deletion cascades through generic relations. Also cleans up the special-casing of generic relations in the deleted object discovery process. Thanks to carljm for the report and patch.
git-svn-id: http://code.djangoproject.com/svn/django/trunk@12790 bcc190cf-cafb-0310-a4f2-bffc1f526a37
1 parent d8c9d21 commit 2d57300

File tree

6 files changed

+171
-128
lines changed

6 files changed

+171
-128
lines changed

‎django/contrib/admin/util.py

Lines changed: 4 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -108,29 +108,6 @@ def get_deleted_objects(objs, opts, user, admin_site, levels_to_root=4):
108108
# TODO using a private model API!
109109
obj._collect_sub_objects(collector)
110110

111-
# TODO This next bit is needed only because GenericRelations are
112-
# cascade-deleted way down in the internals in
113-
# DeleteQuery.delete_batch_related, instead of being found by
114-
# _collect_sub_objects. Refs #12593.
115-
from django.contrib.contenttypes import generic
116-
for f in obj._meta.many_to_many:
117-
if isinstance(f, generic.GenericRelation):
118-
rel_manager = f.value_from_object(obj)
119-
for related in rel_manager.all():
120-
# There's a wierdness here in the case that the
121-
# generic-related object also has FKs pointing to it
122-
# from elsewhere. DeleteQuery does not follow those
123-
# FKs or delete any such objects explicitly (which is
124-
# probably a bug). Some databases may cascade those
125-
# deletes themselves, and some won't. So do we report
126-
# those objects as to-be-deleted? No right answer; for
127-
# now we opt to report only on objects that Django
128-
# will explicitly delete, at risk that some further
129-
# objects will be silently deleted by a
130-
# referential-integrity-maintaining database.
131-
collector.add(related.__class__, related.pk, related,
132-
obj.__class__, obj)
133-
134111
perms_needed = set()
135112

136113
to_delete = collector.nested(_format_callback,
@@ -188,6 +165,10 @@ def add(self, model, pk, obj,
188165
"""
189166
model, pk = type(obj), obj._get_pk_val()
190167

168+
# auto-created M2M models don't interest us
169+
if model._meta.auto_created:
170+
return True
171+
191172
key = model, pk
192173

193174
if key in self.seen:

‎django/db/models/base.py

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -556,7 +556,7 @@ def _collect_sub_objects(self, seen_objs, parent=None, nullable=False):
556556

557557
for related in self._meta.get_all_related_objects():
558558
rel_opts_name = related.get_accessor_name()
559-
if isinstance(related.field.rel, OneToOneRel):
559+
if not related.field.rel.multiple:
560560
try:
561561
sub_obj = getattr(self, rel_opts_name)
562562
except ObjectDoesNotExist:
@@ -582,6 +582,30 @@ def _collect_sub_objects(self, seen_objs, parent=None, nullable=False):
582582
for sub_obj in delete_qs:
583583
sub_obj._collect_sub_objects(seen_objs, self, related.field.null)
584584

585+
for related in self._meta.get_all_related_many_to_many_objects():
586+
if related.field.rel.through:
587+
opts = related.field.rel.through._meta
588+
reverse_field_name = related.field.m2m_reverse_field_name()
589+
nullable = opts.get_field(reverse_field_name).null
590+
filters = {reverse_field_name: self}
591+
for sub_obj in related.field.rel.through._base_manager.filter(**filters):
592+
sub_obj._collect_sub_objects(seen_objs, self, nullable)
593+
594+
for f in self._meta.many_to_many:
595+
if f.rel.through:
596+
opts = f.rel.through._meta
597+
field_name = f.m2m_field_name()
598+
nullable = opts.get_field(field_name).null
599+
filters = {field_name: self}
600+
for sub_obj in f.rel.through._base_manager.filter(**filters):
601+
sub_obj._collect_sub_objects(seen_objs, self, nullable)
602+
else:
603+
# m2m-ish but with no through table? GenericRelation: cascade delete
604+
for sub_obj in f.value_from_object(self).all():
605+
# Generic relations not enforced by db constraints, thus we can set
606+
# nullable=True, order does not matter
607+
sub_obj._collect_sub_objects(seen_objs, self, True)
608+
585609
# Handle any ancestors (for the model-inheritance case). We do this by
586610
# traversing to the most remote parent classes -- those with no parents
587611
# themselves -- and then adding those instances to the collection. That

‎django/db/models/query.py

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1278,8 +1278,6 @@ def delete_objects(seen_objs, using):
12781278
signals.pre_delete.send(sender=cls, instance=instance)
12791279

12801280
pk_list = [pk for pk,instance in items]
1281-
del_query = sql.DeleteQuery(cls)
1282-
del_query.delete_batch_related(pk_list, using=using)
12831281

12841282
update_query = sql.UpdateQuery(cls)
12851283
for field, model in cls._meta.get_fields_with_model():

‎django/db/models/sql/subqueries.py

Lines changed: 1 addition & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -26,52 +26,9 @@ def do_query(self, table, where, using):
2626
self.where = where
2727
self.get_compiler(using).execute_sql(None)
2828

29-
def delete_batch_related(self, pk_list, using):
30-
"""
31-
Set up and execute delete queries for all the objects related to the
32-
primary key values in pk_list. To delete the objects themselves, use
33-
the delete_batch() method.
34-
35-
More than one physical query may be executed if there are a
36-
lot of values in pk_list.
37-
"""
38-
from django.contrib.contenttypes import generic
39-
cls = self.model
40-
for related in cls._meta.get_all_related_many_to_many_objects():
41-
if not isinstance(related.field, generic.GenericRelation):
42-
for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE):
43-
where = self.where_class()
44-
where.add((Constraint(None,
45-
related.field.m2m_reverse_name(), related.field),
46-
'in',
47-
pk_list[offset : offset+GET_ITERATOR_CHUNK_SIZE]),
48-
AND)
49-
self.do_query(related.field.m2m_db_table(), where, using=using)
50-
51-
for f in cls._meta.many_to_many:
52-
w1 = self.where_class()
53-
db_prep_value = None
54-
if isinstance(f, generic.GenericRelation):
55-
from django.contrib.contenttypes.models import ContentType
56-
ct_field = f.rel.to._meta.get_field(f.content_type_field_name)
57-
w1.add((Constraint(None, ct_field.column, ct_field), 'exact',
58-
ContentType.objects.get_for_model(cls).id), AND)
59-
id_field = f.rel.to._meta.get_field(f.object_id_field_name)
60-
db_prep_value = id_field.get_db_prep_value
61-
for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE):
62-
where = self.where_class()
63-
where.add((Constraint(None, f.m2m_column_name(), f), 'in',
64-
map(db_prep_value,
65-
pk_list[offset : offset + GET_ITERATOR_CHUNK_SIZE])),
66-
AND)
67-
if w1:
68-
where.add(w1, AND)
69-
self.do_query(f.m2m_db_table(), where, using=using)
70-
7129
def delete_batch(self, pk_list, using):
7230
"""
73-
Set up and execute delete queries for all the objects in pk_list. This
74-
should be called after delete_batch_related(), if necessary.
31+
Set up and execute delete queries for all the objects in pk_list.
7532
7633
More than one physical query may be executed if there are a
7734
lot of values in pk_list.

‎tests/regressiontests/delete_regress/models.py

Lines changed: 33 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1,62 +1,37 @@
1-
from django.conf import settings
2-
from django.db import models, backend, connection, transaction, DEFAULT_DB_ALIAS
3-
from django.db.models import sql, query
4-
from django.test import TransactionTestCase
1+
from django.db import models
2+
3+
from django.contrib.contenttypes import generic
4+
from django.contrib.contenttypes.models import ContentType
5+
6+
class Award(models.Model):
7+
name = models.CharField(max_length=25)
8+
object_id = models.PositiveIntegerField()
9+
content_type = models.ForeignKey(ContentType)
10+
content_object = generic.GenericForeignKey()
11+
12+
class AwardNote(models.Model):
13+
award = models.ForeignKey(Award)
14+
note = models.CharField(max_length=100)
15+
16+
class Person(models.Model):
17+
name = models.CharField(max_length=25)
18+
awards = generic.GenericRelation(Award)
519

620
class Book(models.Model):
721
pagecount = models.IntegerField()
822

9-
# Can't run this test under SQLite, because you can't
10-
# get two connections to an in-memory database.
11-
if settings.DATABASES[DEFAULT_DB_ALIAS]['ENGINE'] != 'django.db.backends.sqlite3':
12-
class DeleteLockingTest(TransactionTestCase):
13-
def setUp(self):
14-
# Create a second connection to the database
15-
conn_settings = settings.DATABASES[DEFAULT_DB_ALIAS]
16-
self.conn2 = backend.DatabaseWrapper({
17-
'HOST': conn_settings['HOST'],
18-
'NAME': conn_settings['NAME'],
19-
'OPTIONS': conn_settings['OPTIONS'],
20-
'PASSWORD': conn_settings['PASSWORD'],
21-
'PORT': conn_settings['PORT'],
22-
'USER': conn_settings['USER'],
23-
'TIME_ZONE': settings.TIME_ZONE,
24-
})
25-
26-
# Put both DB connections into managed transaction mode
27-
transaction.enter_transaction_management()
28-
transaction.managed(True)
29-
self.conn2._enter_transaction_management(True)
30-
31-
def tearDown(self):
32-
# Close down the second connection.
33-
transaction.leave_transaction_management()
34-
self.conn2.close()
35-
36-
def test_concurrent_delete(self):
37-
"Deletes on concurrent transactions don't collide and lock the database. Regression for #9479"
38-
39-
# Create some dummy data
40-
b1 = Book(id=1, pagecount=100)
41-
b2 = Book(id=2, pagecount=200)
42-
b3 = Book(id=3, pagecount=300)
43-
b1.save()
44-
b2.save()
45-
b3.save()
46-
47-
transaction.commit()
48-
49-
self.assertEquals(3, Book.objects.count())
50-
51-
# Delete something using connection 2.
52-
cursor2 = self.conn2.cursor()
53-
cursor2.execute('DELETE from delete_regress_book WHERE id=1')
54-
self.conn2._commit();
55-
56-
# Now perform a queryset delete that covers the object
57-
# deleted in connection 2. This causes an infinite loop
58-
# under MySQL InnoDB unless we keep track of already
59-
# deleted objects.
60-
Book.objects.filter(pagecount__lt=250).delete()
61-
transaction.commit()
62-
self.assertEquals(1, Book.objects.count())
23+
class Toy(models.Model):
24+
name = models.CharField(max_length=50)
25+
26+
class Child(models.Model):
27+
name = models.CharField(max_length=50)
28+
toys = models.ManyToManyField(Toy, through='PlayedWith')
29+
30+
class PlayedWith(models.Model):
31+
child = models.ForeignKey(Child)
32+
toy = models.ForeignKey(Toy)
33+
date = models.DateField()
34+
35+
class PlayedWithNote(models.Model):
36+
played = models.ForeignKey(PlayedWith)
37+
note = models.TextField()

‎tests/regressiontests/delete_regress/tests.py

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
import datetime
2+
3+
from django.conf import settings
4+
from django.db import backend, connection, transaction, DEFAULT_DB_ALIAS
5+
from django.test import TestCase, TransactionTestCase
6+
7+
from models import Book, Award, AwardNote, Person, Child, Toy, PlayedWith, PlayedWithNote
8+
9+
# Can't run this test under SQLite, because you can't
10+
# get two connections to an in-memory database.
11+
if settings.DATABASES[DEFAULT_DB_ALIAS]['ENGINE'] != 'django.db.backends.sqlite3':
12+
class DeleteLockingTest(TransactionTestCase):
13+
def setUp(self):
14+
# Create a second connection to the default database
15+
conn_settings = settings.DATABASES[DEFAULT_DB_ALIAS]
16+
self.conn2 = backend.DatabaseWrapper({
17+
'HOST': conn_settings['HOST'],
18+
'NAME': conn_settings['NAME'],
19+
'OPTIONS': conn_settings['OPTIONS'],
20+
'PASSWORD': conn_settings['PASSWORD'],
21+
'PORT': conn_settings['PORT'],
22+
'USER': conn_settings['USER'],
23+
'TIME_ZONE': settings.TIME_ZONE,
24+
})
25+
26+
# Put both DB connections into managed transaction mode
27+
transaction.enter_transaction_management()
28+
transaction.managed(True)
29+
self.conn2._enter_transaction_management(True)
30+
31+
def tearDown(self):
32+
# Close down the second connection.
33+
transaction.leave_transaction_management()
34+
self.conn2.close()
35+
36+
def test_concurrent_delete(self):
37+
"Deletes on concurrent transactions don't collide and lock the database. Regression for #9479"
38+
39+
# Create some dummy data
40+
b1 = Book(id=1, pagecount=100)
41+
b2 = Book(id=2, pagecount=200)
42+
b3 = Book(id=3, pagecount=300)
43+
b1.save()
44+
b2.save()
45+
b3.save()
46+
47+
transaction.commit()
48+
49+
self.assertEquals(3, Book.objects.count())
50+
51+
# Delete something using connection 2.
52+
cursor2 = self.conn2.cursor()
53+
cursor2.execute('DELETE from delete_regress_book WHERE id=1')
54+
self.conn2._commit();
55+
56+
# Now perform a queryset delete that covers the object
57+
# deleted in connection 2. This causes an infinite loop
58+
# under MySQL InnoDB unless we keep track of already
59+
# deleted objects.
60+
Book.objects.filter(pagecount__lt=250).delete()
61+
transaction.commit()
62+
self.assertEquals(1, Book.objects.count())
63+
64+
class DeleteCascadeTests(TestCase):
65+
def test_generic_relation_cascade(self):
66+
"""
67+
Test that Django cascades deletes through generic-related
68+
objects to their reverse relations.
69+
70+
This might falsely succeed if the database cascades deletes
71+
itself immediately; the postgresql_psycopg2 backend does not
72+
give such a false success because ForeignKeys are created with
73+
DEFERRABLE INITIALLY DEFERRED, so its internal cascade is
74+
delayed until transaction commit.
75+
76+
"""
77+
person = Person.objects.create(name='Nelson Mandela')
78+
award = Award.objects.create(name='Nobel', content_object=person)
79+
note = AwardNote.objects.create(note='a peace prize',
80+
award=award)
81+
self.assertEquals(AwardNote.objects.count(), 1)
82+
person.delete()
83+
self.assertEquals(Award.objects.count(), 0)
84+
# first two asserts are just sanity checks, this is the kicker:
85+
self.assertEquals(AwardNote.objects.count(), 0)
86+
87+
def test_fk_to_m2m_through(self):
88+
"""
89+
Test that if a M2M relationship has an explicitly-specified
90+
through model, and some other model has an FK to that through
91+
model, deletion is cascaded from one of the participants in
92+
the M2M, to the through model, to its related model.
93+
94+
Like the above test, this could in theory falsely succeed if
95+
the DB cascades deletes itself immediately.
96+
97+
"""
98+
juan = Child.objects.create(name='Juan')
99+
paints = Toy.objects.create(name='Paints')
100+
played = PlayedWith.objects.create(child=juan, toy=paints,
101+
date=datetime.date.today())
102+
note = PlayedWithNote.objects.create(played=played,
103+
note='the next Jackson Pollock')
104+
self.assertEquals(PlayedWithNote.objects.count(), 1)
105+
paints.delete()
106+
self.assertEquals(PlayedWith.objects.count(), 0)
107+
# first two asserts just sanity checks, this is the kicker:
108+
self.assertEquals(PlayedWithNote.objects.count(), 0)

0 commit comments

Comments
 (0)