Opened 7 years ago

Closed 4 years ago

#27852 closed Cleanup/optimization (fixed)

Admin Delete Object Block Page Doesn't Show All Related Objects Blocking Deletion

Reported by: Kenny Loveall Owned by: Hasan Ramezani
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: 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

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)
  1. Register these models with the admin site
  1. Create a ModelA, ModelB, & ModelC object. Make ModelB & ModelC reference ModelA, like such:
    a = ModelA()
    b = ModelB(fk=a)
    c = ModelC(fk=a)
    
  1. 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).

Attachments (1)

djtest.zip (14.3 KB ) - added by Anton Samarchyan 7 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

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.

by Anton Samarchyan, 7 years ago

Attachment: djtest.zip added

comment:2 by Anton Samarchyan, 7 years ago

I've attached an example project with the data to reproduce the issue. Credentials: admin/Oowae5me

comment:3 by Anton Samarchyan, 7 years ago

Version: 1.8master

comment:4 by Anton Samarchyan, 7 years ago

Owner: changed from nobody to Anton Samarchyan
Status: newassigned

comment:5 by Anton Samarchyan, 7 years ago

Component: contrib.adminDatabase layer (models, ORM)
Has patch: set

Added PR

comment:6 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:7 by Hasan Ramezani, 4 years ago

Owner: changed from Anton Samarchyan to Hasan Ramezani
Patch needs improvement: unset
Last edited 4 years ago by Mariusz Felisiak (previous) (diff)

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In ab3cbd8b:

Refs #27852 -- Fixed object deletion to show all protected related objects rather than just the first one.

Thanks Anton Samarchyan for the initial patch.

comment:9 by Mariusz Felisiak, 4 years ago

Has patch: unset

This should be fixed also for restricted related objects.

comment:10 by Mariusz Felisiak, 4 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

PR for restricted related objects.

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 2a6fc890:

Refs #27852 -- Renamed a loop variable in Collector.collect() to avoid redefinition.

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 4ca5c56:

Refs #27852 -- Fixed object deletion to show all restricted related objects rather than just the first one.

comment:13 by Mariusz Felisiak, 4 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top