Opened 15 months ago

Last modified 2 weeks ago

#27272 assigned New feature

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: master
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Accepted
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 15 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 15 months ago by Tim Graham

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 Changed 15 months ago by Simon Charette

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.

Changed 15 months ago by Daniel Izquierdo

Attachment: deletion.diff added

comment:3 Changed 15 months ago by Daniel Izquierdo

@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 Changed 15 months ago by Simon Charette

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 Changed 15 months ago by Simon Charette

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 Changed 15 months ago by Daniel Izquierdo

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 Changed 14 months ago by Tim Graham

Has patch: set

comment:8 Changed 14 months ago by Tim Graham

Patch needs improvement: set

Comments for improvement are on the PR.

comment:9 Changed 4 months ago by Daniel Izquierdo

Patch needs improvement: unset

comment:10 Changed 2 weeks ago by Tim Martin

Patch needs improvement: set

The patch failed tests on Windows. Unfortunately the build is old enough that the logs have expired, so I can't see whether this looks like a significant problem or just a transient error.

comment:11 Changed 2 weeks ago by Tim Graham

Patch needs improvement: unset

I wouldn't consider that a blocker to reviewing the patch. Most likely it's a transient error.

comment:12 Changed 2 weeks ago by Daniel Izquierdo

Will rebase right now to trigger a new build and look into the error.

comment:13 Changed 2 weeks ago by Daniel Izquierdo

The issue appears to have been transient as the new build passed.

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