Opened 8 years ago

Closed 5 years ago

#27272 closed New feature (fixed)

Add a on_delete RESTRICT handler to allow cascading deletions while protecting direct ones

Reported by: Daniel Izquierdo Owned by: Daniel Izquierdo
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Consider this set of models:

class Artist(models.Model):
    name = models.CharField(max_length=10)

class Album(models.Model):
    artist = models.ForeignKey(Artist, on_delete=models.CASCADE)

class Song(models.Model):
    artist = models.ForeignKey(Artist, on_delete=models.CASCADE)
    album = models.ForeignKey(Album, on_delete=models.PROTECT)

And the following test:

class TestDeletion(TestCase):
    def test_delete(self):
        artist = Artist.objects.create(name='test')
        album = Album.objects.create(artist=artist)
        Song.objects.create(artist=artist, album=album)

        artist.delete()

What is the expected result of the test?

Currently, this test fails with ProtectedError raised, because trying to delete the artist results in trying to delete the album, which is protected because of the related song. But, the related song was going to be deleted anyway, via the cascading relationship to the artist itself. So I believe that deletion should succeed in this particular case.

If there's agreement that the current behavior is a bug, I'll work on a fix.

Mailing list discussion (no replies so far): https://groups.google.com/forum/#!topic/django-users/xEe4D5VHRnY

Attachments (1)

deletion.diff (2.0 KB ) - added by Daniel Izquierdo 8 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Tim Graham, 8 years ago

Component: UncategorizedDatabase layer (models, ORM)
Type: UncategorizedBug

I'm not sure. In the case of a conflict between CASCADE and PROTECT, what general rule would you use? I guess a patch would help evaluate the idea. The relevant code is django/db/models/deletion.py.

comment:2 by Simon Charette, 8 years ago

I"m pretty sure this is the expected behavior here. PROTECT should prevent deletion of a referenced row even it's attempted through cascading deletion just like ON DELETE RESTRICT does.

I'm afraid this cannot be changed now without breaking backward compatibility.

by Daniel Izquierdo, 8 years ago

Attachment: deletion.diff added

comment:3 by Daniel Izquierdo, 8 years ago

@Simon it may be that this cannot be changed in Django without breaking backwards compatibility, but there's an argument to be made for the other proposed behavior: given multiple cascade paths, depending on which one we take first, the object preventing deletion of the referenced row may be not even exist anymore before the protected path is visited.

Regarding ON DELETE RESTRICT, checked on Postgres 9.5 and it does delete the row:

psql (9.5.4)
Type "help" for help.

deltest=> CREATE TABLE artist ( id integer PRIMARY KEY, name text );
CREATE TABLE
deltest=> CREATE TABLE album ( id integer PRIMARY KEY, artist_id integer REFERENCES artist ON DELETE CASCADE);
CREATE TABLE
deltest=> CREATE TABLE song ( id integer PRIMARY KEY, artist_id integer REFERENCES artist ON DELETE CASCADE, album_id integer REFERENCES album ON DELETE RESTRICT);
CREATE TABLE
deltest=> 
deltest=> INSERT INTO artist VALUES (1, 'test');
INSERT 0 1
deltest=> INSERT INTO album (id, artist_id) VALUES (2, 1);
INSERT 0 1
deltest=> INSERT INTO song (id, artist_id, album_id) VALUES (3, 1, 2);
INSERT 0 1
deltest=> 
deltest=> -- This will fail
deltest=> DELETE FROM album WHERE id = 2;
ERROR:  update or delete on table "album" violates foreign key constraint "song_album_id_fkey" on table "song"
DETAIL:  Key (id)=(2) is still referenced from table "song".
deltest=> 
deltest=> -- This will succeed
deltest=> DELETE FROM artist WHERE id = 1;
DELETE 1
deltest=> 
deltest=> -- Check it cascaded
deltest=> SELECT * FROM song;
 id | artist_id | album_id 
----+-----------+----------
(0 rows)

deltest=> SELECT * FROM album;
 id | artist_id 
----+-----------
(0 rows)
deltest=> 

Note that trying to delete the album directly fails as expected, but deleting the artist deletes everything because the song is reachable from the artist and supposed to cascade. I would argue the intention of the user doing this query (or doing artist.delete()) is to delete everything.

@Tim I've attached a sample patch that makes the following tests pass.

class TestDeletion(TestCase):
    def test_delete(self):
        artist = Artist.objects.create(name='test')
        album = Album.objects.create(artist=artist)
        Song.objects.create(artist=artist, album=album)

        with self.assertRaises(ProtectedError):
            album.delete()

        artist.delete()

    def test_delete_no_cascade(self):
        artist = Artist.objects.create(name='test')
        another_artist = Artist.objects.create(name='another test')
        album = Album.objects.create(artist=artist)
        Song.objects.create(artist=another_artist, album=album)

        with self.assertRaises(ProtectedError):
            album.delete()

        with self.assertRaises(ProtectedError):
            artist.delete()

comment:4 by Simon Charette, 8 years ago

Triage Stage: UnreviewedAccepted

Thanks for the detailed analysis Daniele!

The on_delete argument was introduced in 616b30227d901a7452810a9ffaa55eaf186dc9e1 to fix #7539 where there was some discussion about RESTRICT but I cannot find a mention of why PROTECT work this way.

I would argue that both PROTECT and RESTRICT can be useful and that PROTECT should be kept this way given some users might rely on its current behavior. What I would suggest we do here is introduce a new on_delete=RESTRICT handler that mimics what the SQL one does.

comment:5 by Simon Charette, 8 years ago

Cc: Simon Charette added
Summary: Protected objects not deleted even if they would have been deleted via cascade anywayAdd a on_delete RESTRICT handler to allow cascading deletions while protecting direct ones
Type: BugNew feature
Version: 1.10master

comment:6 by Daniel Izquierdo, 8 years ago

Owner: changed from nobody to Daniel Izquierdo
Status: newassigned

Thanks Simon, that does seem like a good idea to preserve backwards compatibility while also supporting the new behavior. If it's okay I'll assign this to myself and work on a patch.

comment:7 by Tim Graham, 8 years ago

Has patch: set

comment:8 by Tim Graham, 8 years ago

Patch needs improvement: set

Comments for improvement are on the PR.

comment:9 by Daniel Izquierdo, 7 years ago

Patch needs improvement: unset

comment:10 by Tim Graham, 6 years ago

Patch needs improvement: set

More comments for improvement are on the PR.

comment:11 by Daniel Izquierdo, 5 years ago

Patch needs improvement: unset

comment:12 by Mariusz Felisiak, 5 years ago

Needs tests: set
Patch needs improvement: set

comment:13 by Daniel Izquierdo, 5 years ago

Needs tests: unset
Patch needs improvement: unset

comment:14 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:15 by Mariusz Felisiak, 5 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 4e1d809:

Refs #27272 -- Added Collector.add_dependency().

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 89abecc:

Fixed #27272 -- Added an on_delete RESTRICT handler to allow cascading deletions while protecting direct ones.

Note: See TracTickets for help on using tickets.
Back to Top