[Django] #27852: Admin Delete Object Block Page Doesn't Show All Related Objects Blocking Deletion
Django
Deletion
-----------------------------------------+------------------------
Reporter: Kenny Loveall | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
**STR:**
1. Create three models, two of which reference the third and the
references PROTECT on delete, like such:
{{{
class ModelA(models.Model):
pass
class ModelB(models.Model):
fk = models.ForeignKey(ModelA, on_delete=models.PROTECT)
class ModelC(models.Model):
fk = models.ForeignKey(ModelA, on_delete=models.PROTECT)
}}}
2. Register these models with the admin site
3. Create a ModelA, ModelB, & ModelC object. Make ModelB & ModelC
reference ModelA, like such:
{{{
a = ModelA()
b = ModelB(fk=a)
c = ModelC(fk=a)
}}}
4. Go to the admin site and try to delete the ModelA object.
**Expected Result:** Page listing both b & c as blocking objects
**Actual Result:** Page listing only b or c, but not both.
After investigation, I've figured out it's because the field.rel.on_delete
call in db.models.deletion.py (excerpted below) throws a
models.ProtectedError exception and therefore exits the loop early and
does not check the rest of the related fields.
{{{
def collect(self, objs, source=None, nullable=False,
collect_related=True,
source_attr=None, reverse_dependency=False):
[... omitted for brevity ...]
if collect_related:
for related in get_candidate_relations_to_delete(model._meta):
field = related.field
if field.rel.on_delete == DO_NOTHING:
continue
batches = self.get_del_batches(new_objs, field)
for batch in batches:
sub_objs = self.related_objects(related, batch)
if self.can_fast_delete(sub_objs, from_field=field):
self.fast_deletes.append(sub_objs)
elif sub_objs:
field.rel.on_delete(self, field, sub_objs,
self.using)
for field in model._meta.virtual_fields:
if hasattr(field, 'bulk_related_objects'):
# Its something like generic foreign key.
sub_objs = field.bulk_related_objects(new_objs,
self.using)
self.collect(sub_objs,
source=model,
source_attr=field.rel.related_name,
nullable=True)
}}}
This was discovered because we're implementing a soft-delete override in
the Model layer (so anytime you try to delete things it just sets a
timestamp instead of actually removing the data). This became an issue
because we take the output of this and filtered it to only objects that we
had that were deleted by our standards and found that under certain
circumstances we were allowed to delete things when we shouldn't have
been. Given this scenario being such an edge case, my hopes for getting
this fixed aren't high; although I would argue that it's still a poor user
experience for people that aren't trying to do what we're doing where it
shows them only *some* of the things blocking the deletion instead of all
of them. I'm hoping, however, that hopefully someone can at the very least
show us if we can get a list of all the delete-blocking objects from
somewhere else?
Given where this is, the bug really seems to be coming from
db.models.deletion.Collector so I'm not sure if the component field should
be admin or models but I chose admin because that's where it's easily
visible (and would impact general Django users).
--
Ticket URL: <https://code.djangoproject.com/ticket/27852>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Deletion
Reporter: Kenny Loveall | Owner: nobody
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
I haven't confirmed the issue or looked into if it's feasible to fix, but
if you provide a patch, it seems okay to change.
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:1>
Django
Deletion
Reporter: Kenny Loveall | Owner: nobody
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Anton Samarchyan):
* Attachment "djtest.zip" added.
Django
Deletion
Reporter: Kenny Loveall | Owner: nobody
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Anton Samarchyan):
I've attached an example project with the data to reproduce the issue.
Credentials: admin/Oowae5me
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:2>
Django
Deletion
Reporter: Kenny Loveall | Owner: nobody
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Anton Samarchyan):
* version: 1.8 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:3>
Django
Deletion
Reporter: Kenny Loveall | Owner: Anton
Type: | Samarchyan
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Anton Samarchyan):
* owner: nobody => Anton Samarchyan
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:4>
Django
Deletion
Reporter: Kenny Loveall | Owner: Anton
Type: | Samarchyan
Cleanup/optimization | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Anton Samarchyan):
* has_patch: 0 => 1
* component: contrib.admin => Database layer (models, ORM)
Comment:
Added [https://github.com/django/django/pull/8160 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:5>
Django
Deletion
Reporter: Kenny Loveall | Owner: Anton
Type: | Samarchyan
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:6>
Django
Deletion
Reporter: Kenny Loveall | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | 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 Hasan Ramezani):
* owner: Anton Samarchyan => Hasan Ramezani
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:7>
Django
Deletion
Reporter: Kenny Loveall | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | 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
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"ab3cbd8b9a315911248227208630a020cedca08f" ab3cbd8b]:
{{{
#!CommitTicketReference repository=""
revision="ab3cbd8b9a315911248227208630a020cedca08f"
Refs #27852 -- Fixed object deletion to show all protected related objects
rather than just the first one.
Thanks Anton Samarchyan for the initial patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:8>
Django
Deletion
Reporter: Kenny Loveall | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | 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 felixxm):
* has_patch: 1 => 0
Comment:
This should be fixed also for restricted related objects.
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:9>
Django
Deletion
Reporter: Kenny Loveall | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/12376 PR] for restricted related
objects.
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:10>
Django
Deletion
Reporter: Kenny Loveall | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"4ca5c565f4dc9e97845036e86416abc5cfde766c" 4ca5c56]:
{{{
#!CommitTicketReference repository=""
revision="4ca5c565f4dc9e97845036e86416abc5cfde766c"
Refs #27852 -- Fixed object deletion to show all restricted related
objects rather than just the first one.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:12>
Django
Deletion
Reporter: Kenny Loveall | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"2a6fc89018881c07e38f81989ff0c012e7539390" 2a6fc890]:
{{{
#!CommitTicketReference repository=""
revision="2a6fc89018881c07e38f81989ff0c012e7539390"
Refs #27852 -- Renamed a loop variable in Collector.collect() to avoid
redefinition.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:11>
Django
Deletion
Reporter: Kenny Loveall | Owner: Hasan
Type: | Ramezani
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/27852#comment:13>