[Django] #27272: Protected objects not deleted even if they would have been deleted via cascade anyway
Django
cascade anyway
----------------------------------+--------------------
Reporter: Daniel Izquierdo | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------
Consider this set of models:
{{{
#!python
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:
{{{
#!python
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
--
Ticket URL: <https://code.djangoproject.com/ticket/27272>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
cascade anyway
Reporter: Daniel Izquierdo | Owner: nobody
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* component: Uncategorized => Database layer (models, ORM)
* needs_better_patch: => 0
* type: Uncategorized => Bug
* needs_tests: => 0
* needs_docs: => 0
Comment:
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
[https://github.com/django/django/blob/master/django/db/models/deletion.py
django/db/models/deletion.py].
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:1>
Django
cascade anyway
Reporter: Daniel Izquierdo | Owner: nobody
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:2>
Django
cascade anyway
Reporter: Daniel Izquierdo | Owner: nobody
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Daniel Izquierdo):
* Attachment "deletion.diff" added.
Django
cascade anyway
Reporter: Daniel Izquierdo | Owner: nobody
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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:
{{{
#!sql
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.
{{{
#!python
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()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:3>
Django
cascade anyway
Reporter: Daniel Izquierdo | Owner: nobody
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:4>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* cc: Simon Charette (added)
* version: 1.10 => master
* type: Bug => New feature
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:5>
Django
protecting direct ones
-------------------------------------+-------------------------------------
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Daniel Izquierdo):
* owner: nobody => Daniel Izquierdo
* status: new => assigned
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:6>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/7364 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:7>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
Comment:
Comments for improvement are on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:8>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Daniel Izquierdo):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:9>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Martin):
* needs_better_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:10>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 1 => 0
Comment:
I wouldn't consider that a blocker to reviewing the patch. Most likely
it's a transient error.
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:11>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Daniel Izquierdo):
Will rebase right now to trigger a new build and look into the error.
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:12>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Daniel Izquierdo):
The issue appears to have been transient as the new build passed.
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:13>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
Comment:
More comments for improvement are on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:10>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Daniel Izquierdo):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:11>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:12>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Daniel Izquierdo):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:13>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:14>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:15>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"4e1d809aa570c0e0736587672607f9d6c22e42c9" 4e1d809]:
{{{
#!CommitTicketReference repository=""
revision="4e1d809aa570c0e0736587672607f9d6c22e42c9"
Refs #27272 -- Added Collector.add_dependency().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:16>
Django
protecting direct ones
-------------------------------------+-------------------------------------
Reporter: Daniel Izquierdo | Owner: Daniel
| Izquierdo
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"89abecc75d7eadab681f29be5237972c6c2f997b" 89abecc]:
{{{
#!CommitTicketReference repository=""
revision="89abecc75d7eadab681f29be5237972c6c2f997b"
Fixed #27272 -- Added an on_delete RESTRICT handler to allow cascading
deletions while protecting direct ones.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27272#comment:17>