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

[Daniel Izquierdo]

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

[Tim Graham]

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.

[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.

[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()

[Simon Charette]

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.

[Daniel Izquierdo]

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.

[Tim Graham]

​PR

[Tim Graham]

Comments for improvement are on the PR.

[Tim Graham]

More comments for improvement are on the PR.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4e1d809:

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 89abecc:

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