diff --git a/django/contrib/admin/util.py b/django/contrib/admin/util.py index fc940ad908..f727626a03 100644 --- a/django/contrib/admin/util.py +++ b/django/contrib/admin/util.py @@ -108,29 +108,6 @@ def get_deleted_objects(objs, opts, user, admin_site, levels_to_root=4): # TODO using a private model API! obj._collect_sub_objects(collector) - # TODO This next bit is needed only because GenericRelations are - # cascade-deleted way down in the internals in - # DeleteQuery.delete_batch_related, instead of being found by - # _collect_sub_objects. Refs #12593. - from django.contrib.contenttypes import generic - for f in obj._meta.many_to_many: - if isinstance(f, generic.GenericRelation): - rel_manager = f.value_from_object(obj) - for related in rel_manager.all(): - # There's a wierdness here in the case that the - # generic-related object also has FKs pointing to it - # from elsewhere. DeleteQuery does not follow those - # FKs or delete any such objects explicitly (which is - # probably a bug). Some databases may cascade those - # deletes themselves, and some won't. So do we report - # those objects as to-be-deleted? No right answer; for - # now we opt to report only on objects that Django - # will explicitly delete, at risk that some further - # objects will be silently deleted by a - # referential-integrity-maintaining database. - collector.add(related.__class__, related.pk, related, - obj.__class__, obj) - perms_needed = set() to_delete = collector.nested(_format_callback, @@ -188,6 +165,10 @@ class NestedObjects(object): """ model, pk = type(obj), obj._get_pk_val() + # auto-created M2M models don't interest us + if model._meta.auto_created: + return True + key = model, pk if key in self.seen: diff --git a/django/db/models/base.py b/django/db/models/base.py index 807137fb57..51707b82ce 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -556,7 +556,7 @@ class Model(object): for related in self._meta.get_all_related_objects(): rel_opts_name = related.get_accessor_name() - if isinstance(related.field.rel, OneToOneRel): + if not related.field.rel.multiple: try: sub_obj = getattr(self, rel_opts_name) except ObjectDoesNotExist: @@ -582,6 +582,30 @@ class Model(object): for sub_obj in delete_qs: sub_obj._collect_sub_objects(seen_objs, self, related.field.null) + for related in self._meta.get_all_related_many_to_many_objects(): + if related.field.rel.through: + opts = related.field.rel.through._meta + reverse_field_name = related.field.m2m_reverse_field_name() + nullable = opts.get_field(reverse_field_name).null + filters = {reverse_field_name: self} + for sub_obj in related.field.rel.through._base_manager.filter(**filters): + sub_obj._collect_sub_objects(seen_objs, self, nullable) + + for f in self._meta.many_to_many: + if f.rel.through: + opts = f.rel.through._meta + field_name = f.m2m_field_name() + nullable = opts.get_field(field_name).null + filters = {field_name: self} + for sub_obj in f.rel.through._base_manager.filter(**filters): + sub_obj._collect_sub_objects(seen_objs, self, nullable) + else: + # m2m-ish but with no through table? GenericRelation: cascade delete + for sub_obj in f.value_from_object(self).all(): + # Generic relations not enforced by db constraints, thus we can set + # nullable=True, order does not matter + sub_obj._collect_sub_objects(seen_objs, self, True) + # Handle any ancestors (for the model-inheritance case). We do this by # traversing to the most remote parent classes -- those with no parents # themselves -- and then adding those instances to the collection. That diff --git a/django/db/models/query.py b/django/db/models/query.py index 5c7d08fb39..f6b4419d27 100644 --- a/django/db/models/query.py +++ b/django/db/models/query.py @@ -1278,8 +1278,6 @@ def delete_objects(seen_objs, using): signals.pre_delete.send(sender=cls, instance=instance) pk_list = [pk for pk,instance in items] - del_query = sql.DeleteQuery(cls) - del_query.delete_batch_related(pk_list, using=using) update_query = sql.UpdateQuery(cls) for field, model in cls._meta.get_fields_with_model(): diff --git a/django/db/models/sql/subqueries.py b/django/db/models/sql/subqueries.py index ef6cec553e..6b52729f68 100644 --- a/django/db/models/sql/subqueries.py +++ b/django/db/models/sql/subqueries.py @@ -26,52 +26,9 @@ class DeleteQuery(Query): self.where = where self.get_compiler(using).execute_sql(None) - def delete_batch_related(self, pk_list, using): - """ - Set up and execute delete queries for all the objects related to the - primary key values in pk_list. To delete the objects themselves, use - the delete_batch() method. - - More than one physical query may be executed if there are a - lot of values in pk_list. - """ - from django.contrib.contenttypes import generic - cls = self.model - for related in cls._meta.get_all_related_many_to_many_objects(): - if not isinstance(related.field, generic.GenericRelation): - for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE): - where = self.where_class() - where.add((Constraint(None, - related.field.m2m_reverse_name(), related.field), - 'in', - pk_list[offset : offset+GET_ITERATOR_CHUNK_SIZE]), - AND) - self.do_query(related.field.m2m_db_table(), where, using=using) - - for f in cls._meta.many_to_many: - w1 = self.where_class() - db_prep_value = None - if isinstance(f, generic.GenericRelation): - from django.contrib.contenttypes.models import ContentType - ct_field = f.rel.to._meta.get_field(f.content_type_field_name) - w1.add((Constraint(None, ct_field.column, ct_field), 'exact', - ContentType.objects.get_for_model(cls).id), AND) - id_field = f.rel.to._meta.get_field(f.object_id_field_name) - db_prep_value = id_field.get_db_prep_value - for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE): - where = self.where_class() - where.add((Constraint(None, f.m2m_column_name(), f), 'in', - map(db_prep_value, - pk_list[offset : offset + GET_ITERATOR_CHUNK_SIZE])), - AND) - if w1: - where.add(w1, AND) - self.do_query(f.m2m_db_table(), where, using=using) - def delete_batch(self, pk_list, using): """ - Set up and execute delete queries for all the objects in pk_list. This - should be called after delete_batch_related(), if necessary. + Set up and execute delete queries for all the objects in pk_list. More than one physical query may be executed if there are a lot of values in pk_list. diff --git a/tests/regressiontests/delete_regress/models.py b/tests/regressiontests/delete_regress/models.py index ff561c9d20..4f61e78513 100644 --- a/tests/regressiontests/delete_regress/models.py +++ b/tests/regressiontests/delete_regress/models.py @@ -1,62 +1,37 @@ -from django.conf import settings -from django.db import models, backend, connection, transaction, DEFAULT_DB_ALIAS -from django.db.models import sql, query -from django.test import TransactionTestCase +from django.db import models + +from django.contrib.contenttypes import generic +from django.contrib.contenttypes.models import ContentType + +class Award(models.Model): + name = models.CharField(max_length=25) + object_id = models.PositiveIntegerField() + content_type = models.ForeignKey(ContentType) + content_object = generic.GenericForeignKey() + +class AwardNote(models.Model): + award = models.ForeignKey(Award) + note = models.CharField(max_length=100) + +class Person(models.Model): + name = models.CharField(max_length=25) + awards = generic.GenericRelation(Award) class Book(models.Model): pagecount = models.IntegerField() -# Can't run this test under SQLite, because you can't -# get two connections to an in-memory database. -if settings.DATABASES[DEFAULT_DB_ALIAS]['ENGINE'] != 'django.db.backends.sqlite3': - class DeleteLockingTest(TransactionTestCase): - def setUp(self): - # Create a second connection to the database - conn_settings = settings.DATABASES[DEFAULT_DB_ALIAS] - self.conn2 = backend.DatabaseWrapper({ - 'HOST': conn_settings['HOST'], - 'NAME': conn_settings['NAME'], - 'OPTIONS': conn_settings['OPTIONS'], - 'PASSWORD': conn_settings['PASSWORD'], - 'PORT': conn_settings['PORT'], - 'USER': conn_settings['USER'], - 'TIME_ZONE': settings.TIME_ZONE, - }) +class Toy(models.Model): + name = models.CharField(max_length=50) - # Put both DB connections into managed transaction mode - transaction.enter_transaction_management() - transaction.managed(True) - self.conn2._enter_transaction_management(True) +class Child(models.Model): + name = models.CharField(max_length=50) + toys = models.ManyToManyField(Toy, through='PlayedWith') - def tearDown(self): - # Close down the second connection. - transaction.leave_transaction_management() - self.conn2.close() +class PlayedWith(models.Model): + child = models.ForeignKey(Child) + toy = models.ForeignKey(Toy) + date = models.DateField() - def test_concurrent_delete(self): - "Deletes on concurrent transactions don't collide and lock the database. Regression for #9479" - - # Create some dummy data - b1 = Book(id=1, pagecount=100) - b2 = Book(id=2, pagecount=200) - b3 = Book(id=3, pagecount=300) - b1.save() - b2.save() - b3.save() - - transaction.commit() - - self.assertEquals(3, Book.objects.count()) - - # Delete something using connection 2. - cursor2 = self.conn2.cursor() - cursor2.execute('DELETE from delete_regress_book WHERE id=1') - self.conn2._commit(); - - # Now perform a queryset delete that covers the object - # deleted in connection 2. This causes an infinite loop - # under MySQL InnoDB unless we keep track of already - # deleted objects. - Book.objects.filter(pagecount__lt=250).delete() - transaction.commit() - self.assertEquals(1, Book.objects.count()) +class PlayedWithNote(models.Model): + played = models.ForeignKey(PlayedWith) + note = models.TextField() diff --git a/tests/regressiontests/delete_regress/tests.py b/tests/regressiontests/delete_regress/tests.py new file mode 100644 index 0000000000..7ebe5e86a4 --- /dev/null +++ b/tests/regressiontests/delete_regress/tests.py @@ -0,0 +1,108 @@ +import datetime + +from django.conf import settings +from django.db import backend, connection, transaction, DEFAULT_DB_ALIAS +from django.test import TestCase, TransactionTestCase + +from models import Book, Award, AwardNote, Person, Child, Toy, PlayedWith, PlayedWithNote + +# Can't run this test under SQLite, because you can't +# get two connections to an in-memory database. +if settings.DATABASES[DEFAULT_DB_ALIAS]['ENGINE'] != 'django.db.backends.sqlite3': + class DeleteLockingTest(TransactionTestCase): + def setUp(self): + # Create a second connection to the default database + conn_settings = settings.DATABASES[DEFAULT_DB_ALIAS] + self.conn2 = backend.DatabaseWrapper({ + 'HOST': conn_settings['HOST'], + 'NAME': conn_settings['NAME'], + 'OPTIONS': conn_settings['OPTIONS'], + 'PASSWORD': conn_settings['PASSWORD'], + 'PORT': conn_settings['PORT'], + 'USER': conn_settings['USER'], + 'TIME_ZONE': settings.TIME_ZONE, + }) + + # Put both DB connections into managed transaction mode + transaction.enter_transaction_management() + transaction.managed(True) + self.conn2._enter_transaction_management(True) + + def tearDown(self): + # Close down the second connection. + transaction.leave_transaction_management() + self.conn2.close() + + def test_concurrent_delete(self): + "Deletes on concurrent transactions don't collide and lock the database. Regression for #9479" + + # Create some dummy data + b1 = Book(id=1, pagecount=100) + b2 = Book(id=2, pagecount=200) + b3 = Book(id=3, pagecount=300) + b1.save() + b2.save() + b3.save() + + transaction.commit() + + self.assertEquals(3, Book.objects.count()) + + # Delete something using connection 2. + cursor2 = self.conn2.cursor() + cursor2.execute('DELETE from delete_regress_book WHERE id=1') + self.conn2._commit(); + + # Now perform a queryset delete that covers the object + # deleted in connection 2. This causes an infinite loop + # under MySQL InnoDB unless we keep track of already + # deleted objects. + Book.objects.filter(pagecount__lt=250).delete() + transaction.commit() + self.assertEquals(1, Book.objects.count()) + +class DeleteCascadeTests(TestCase): + def test_generic_relation_cascade(self): + """ + Test that Django cascades deletes through generic-related + objects to their reverse relations. + + This might falsely succeed if the database cascades deletes + itself immediately; the postgresql_psycopg2 backend does not + give such a false success because ForeignKeys are created with + DEFERRABLE INITIALLY DEFERRED, so its internal cascade is + delayed until transaction commit. + + """ + person = Person.objects.create(name='Nelson Mandela') + award = Award.objects.create(name='Nobel', content_object=person) + note = AwardNote.objects.create(note='a peace prize', + award=award) + self.assertEquals(AwardNote.objects.count(), 1) + person.delete() + self.assertEquals(Award.objects.count(), 0) + # first two asserts are just sanity checks, this is the kicker: + self.assertEquals(AwardNote.objects.count(), 0) + + def test_fk_to_m2m_through(self): + """ + Test that if a M2M relationship has an explicitly-specified + through model, and some other model has an FK to that through + model, deletion is cascaded from one of the participants in + the M2M, to the through model, to its related model. + + Like the above test, this could in theory falsely succeed if + the DB cascades deletes itself immediately. + + """ + juan = Child.objects.create(name='Juan') + paints = Toy.objects.create(name='Paints') + played = PlayedWith.objects.create(child=juan, toy=paints, + date=datetime.date.today()) + note = PlayedWithNote.objects.create(played=played, + note='the next Jackson Pollock') + self.assertEquals(PlayedWithNote.objects.count(), 1) + paints.delete() + self.assertEquals(PlayedWith.objects.count(), 0) + # first two asserts just sanity checks, this is the kicker: + self.assertEquals(PlayedWithNote.objects.count(), 0)