Opened 7 years ago

Closed 5 years ago

Last modified 4 years ago

#6191 closed (fixed)

Admin delete view doesn't show all items in some circumstances

Reported by: nicklane Owned by: carljm
Component: contrib.admin Version: master
Severity: Keywords:
Cc: tom@…, carljm, daemondazz, mshields@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The admin delete view, which lists all of the objects to be deleted, sometimes doesn't include all of the objects that actually end up getting deleted. This is quite concerning when you delete an object and later realise that a whole stack of other objects were also deleted that you were not expecting :-)

The situation that caused this to happen was when there was more than one related field to the same model (e.g. with different related_names).

The code adds each related field's options to a list of "seen" opts and ignores and subsequent opts for the same model, which breaks if you have more than one field related to the same model.

Attachments (16)

_get_deleted_objects.patch (1.4 KB) - added by nicklane 7 years ago.
6191.patch
admin_delete.patch (4.4 KB) - added by nicklane 7 years ago.
Patch with initial tests
admin_delete.2.patch (4.1 KB) - added by nicklane 7 years ago.
Patch with initial tests
admin_delete.3.patch (4.0 KB) - added by nicklane 7 years ago.
admin_delete.4.patch (5.5 KB) - added by nicklane 7 years ago.
Patch with initial tests
admin_deleted_objects.patch (3.4 KB) - added by nicklane 7 years ago.
Simplified the tests
newforms-admin_deleted_objects.patch (3.5 KB) - added by nicklane 7 years ago.
Patch agains newforms-admin
6191.patch (4.5 KB) - added by nicklane 7 years ago.
Updated patch to work with trunk
multiple-fk-delete-view.diff (4.1 KB) - added by shields@… 6 years ago.
6191_r12203.diff (9.5 KB) - added by carljm 5 years ago.
updated patch with increased test coverage
6191_r12203.2.diff (28.9 KB) - added by carljm 5 years ago.
new patch that removes the algorithm duplication entirely
6191_r12203.3.diff (29.1 KB) - added by carljm 5 years ago.
updated patch: fixes nested list layout issue with siblings
6191_r12299.diff (29.0 KB) - added by carljm 5 years ago.
updated patch; removes unused added import
6191_r12556.diff (29.7 KB) - added by carljm 5 years ago.
updated patch, applies cleanly
6191_r12557.diff (31.8 KB) - added by carljm 5 years ago.
updated patch with (hopefully temporary) workaround fix for #12025 (generics)
6191_r12578_incorporated_minor_review_items.diff (34.3 KB) - added by carljm 5 years ago.
incorporates all of Russ' suggestions except for merging/API changes for NestedObjects? and CollectedObjects? - also removes unneeded parens to make Alex Gaynor happy

Download all attachments as: .zip

Change History (44)

Changed 7 years ago by nicklane

6191.patch

comment:1 Changed 7 years ago by nicklane

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The attached patch solves the problem by storing the related field's accessor name (related_name) instead of the opts, which seemed like a sensible approach to me... any thoughts?

comment:2 Changed 7 years ago by Karen Tracey <kmtracey@…>

  • Keywords nfa-someday added
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from SVN to newforms-admin

From a quick look at the code, the behavior is the same in newforms-admin (the code in question lives in django/contrib/admin/util.py over there). Changing version to newforms-admin since that's where the admin development focus is now, and where the fix will likely be made. Tagging as not blocking merge, though, since it's a problem shared with old admin.

comment:3 Changed 7 years ago by jkocherhans

  • Needs tests set

It may not be fun, but this is the kind of thing that really needs regression tests. A good start would be to post some minimal models and describe how to create a set of instances will trigger this bug.

comment:4 Changed 7 years ago by nicklane

Oops, tests are on my list... but I forgot to tick "needs tests".

Changed 7 years ago by nicklane

Patch with initial tests

Changed 7 years ago by nicklane

Patch with initial tests

Changed 7 years ago by nicklane

Changed 7 years ago by nicklane

Patch with initial tests

comment:5 Changed 7 years ago by nicklane

Sorry about the bad patches, I couldn't get Mercurial queues to export a patch which played nice with trac. I'll start a django-developers thread when I have a chance, to discuss the issues I have with the tests.

Changed 7 years ago by nicklane

Simplified the tests

comment:6 Changed 7 years ago by nicklane

To clarify (from the tests in the latest patch):

class Staff(models.Model):
    name = models.CharField(max_length=100)

class Project(models.Model):
    name = models.CharField(max_length=100)
    team_leader = models.ForeignKey(Staff, related_name='lead_projects')
    contact_person = models.ForeignKey(Staff, related_name='contact_projects')

>>> adam = Staff.objects.create(name=u'Adam')
>>> sue = Staff.objects.create(name=u'Sue')
>>> project = Project.objects.create(name=u'World Domination', team_leader=adam, contact_person=sue)

>>> print_deleted_objects(adam)
[[u'Project: World Domination', []]]

>>> print_deleted_objects(sue)
[[u'Project: World Domination', []]]

The 2nd test fails without the patch applied.

Changed 7 years ago by nicklane

Patch agains newforms-admin

comment:7 Changed 7 years ago by Thomas Steinacher <tom@…>

  • Cc tom@… added

Changed 7 years ago by nicklane

Updated patch to work with trunk

comment:8 Changed 7 years ago by nicklane

  • Needs tests unset
  • Version changed from newforms-admin to SVN

comment:9 Changed 6 years ago by kmtracey

  • Patch needs improvement set

As of the fix for #7276, there's at least one more case where the admin delete confirmation page doesn't show all the objects that are actually going to be deleted, which highlights the perils of having two separate code paths: one for calculating the to-be-deleted objects for display and the 2nd that does the actual deletion. It would really be better if the admin for-display part could somehow make use of the underlying ORM routines that are going to govern what actually gets deleted, but that's not an easy fix.

Also the current patch will display an object twice -- if for example A is both the team_leader and contact_person for a project, that project will get displayed twice on the confirmation page for deleting A. That's a bit confusing (though I'm not entirely sure it can't also happen with current code -- I find that admin routine a bit hard to get my head around).

comment:10 Changed 6 years ago by carljm

  • Cc carljm added

comment:11 Changed 6 years ago by daemondazz

  • Cc daemondazz added

comment:12 Changed 6 years ago by kmtracey

#11821 reported this again and has a patch.

Changed 6 years ago by shields@…

comment:13 Changed 6 years ago by shields@…

  • Cc mshields@… added

I've attached the patch I wrote when reporting this as #11821.

I think this problem is fairly serious, so if this patch can't be accepted, let me know what needs to change.

comment:14 Changed 5 years ago by carljm

  • Owner changed from nobody to carljm

Changed 5 years ago by carljm

updated patch with increased test coverage

comment:15 Changed 5 years ago by carljm

The attached patch is based on nicklane's 6191.patch, updated to apply to trunk, and with a number of new tests. Just uploading for the tests; I don't think the fix is ready for trunk.

Notably (among other things), the new tests test that:

  1. If a model has two foreign keys to it from a single related model, both are followed (the original issue here, passes after the patch).
  1. A given object is only displayed once in the list, even if it can be found through two different paths (passes before the patch, fails after it).
  1. A cyclic dependency still only lists each object once, not repeatedly until an arbitrary max recursion is hit (this is #11296, currently fails both before and after).

IMO, kmtracey is right: the real fix here is to re-use the same code that is actually used to collect the objects for deletion (that's Model._collect_sub_objects). Otherwise we're likely to continue to see recurring problems with mismatches between the two duplicate algorithms. It's tricky, because get_deleted_objects generates a nested data structure for display, where Model._collect_sub_objects generates a flat one. I think it's possible by adding a callback hook to Model._collect_sub_objects, so get_deleted_objects can be notified each time _collect_sub_objects recurses. This is the route I'm planning to pursue, and I think I should be able to get all three of the above cases to pass (fixing both this and #11296), and eliminate the duplicate logic. Comments welcome.

Changed 5 years ago by carljm

new patch that removes the algorithm duplication entirely

comment:16 Changed 5 years ago by carljm

I ended up taking a slightly different approach thanks to feedback from Alex Gaynor on my above comment. I built a collection object that can be passed into Model._collect_sub_objects (implementing the interface it expects from its collector), but then can spit out the collected objects in the nested list form that the delete confirmation view needs. The patch now fixes both this bug and #11296 (passes all three tests I mentioned previously), and removes the algorithm duplication so there shouldn't be able to be discrepancies between the displayed objects and the deleted ones.

Some areas where I'm aware this changes behavior:

  • If an inherited model is deleted, the parent model is now explicitly displayed as well, whereas before it wasn't.
  • The previous code appeared to try to use url reversing for the links to changeform for each object, but was broken in such a way that the reversing always failed, causing a fallback to the old method. Reversing now works if the admin urls are included in the new style. This doesn't cause a user-visible change, but does cause the generated links to now go through the iri_to_uri encoding that reverse() performs. This required one small test change.

I know the admin is already coupled to internal details of various things in Django, but I'm slightly uncomfortable introducing yet another one with the use of Model._collect_sub_objects, which is technically marked as a private API. I do think it's reasonable that the public model API would provide some way for client code to discover in advance what related objects would be affected by a delete() call. Should this patch should also promote _collect_sub_objects to a semi-public (but not documented) API, presumably just by removing the underscore? Or even document it?

comment:17 Changed 5 years ago by carljm

One additional note: there was a minor change I had to make to the CollectedObject.add() signature; no way this approach could work without it. The change is minimally intrusive: there's test coverage of CollectedObject and _collect_sub_objects in modeltests/delete, and all those tests pass with no change.

Changed 5 years ago by carljm

updated patch: fixes nested list layout issue with siblings

comment:18 Changed 5 years ago by carljm

Discovered two more differences between this patch and current behavior.

  1. Currently the list of "related objects to be deleted" includes entries for "one or more foo in bar: my_bar", where foo is a M2M field on the bar model pointing to the object about to be deleted. With the patch, m2m intermediary relationship objects are only listed if you have an explicit "through" model, in which case each "through" object is listed separately. IMO, an m2m without an explicit through model is conceptually just a relationship; the fact that an entry in a table will be deleted is just a relationship implementation detail that doesn't require notification. If the previous behavior is preferred, however, I am willing to add it to the patch.
  1. In general, this patch only ever lists a given object once, even if there are multiple relationship paths to it. Current behavior can potentially list the same object many times if there are several paths to it.

comment:19 Changed 5 years ago by carljm

  • milestone set to 1.2
  • Patch needs improvement unset

Changed 5 years ago by carljm

updated patch; removes unused added import

comment:20 Changed 5 years ago by carljm

  • Keywords nfa-someday removed

Just realized from working on #12721 that this patch would not notify the deletion of related objects via a GenericRelation. Those objects are not collected as part of _collect_sub_objects, they are deleted far deeper in the internals, in DeleteQuery.delete_batch_related. I'm not sure why that needs to be the case (seems a bit ugly), but haven't looked at it closely; in any case I guess changing that is too invasive for the scope of this bug? So given that, the only reasonable solution I can see is to duplicate the generic-related-finding code from delete_batch_related in the admin routine. Reintroduces a bit of duplication, but only for GenericRelation, so much less than was there previously.

I'll work on that unless anyone pops up with a better approach.

comment:21 Changed 5 years ago by carljm

Oh, now I see #12025: apparently the previous code didn't notify on GenericRelations either. Seems like it wouldn't hurt to knock that off at the same time, but maybe it's better to keep the issues separate.

Changed 5 years ago by carljm

updated patch, applies cleanly

Changed 5 years ago by carljm

updated patch with (hopefully temporary) workaround fix for #12025 (generics)

comment:22 Changed 5 years ago by carljm

The latest patch here includes special-case code to fix #12025. If the fix for #12953 moves generic-relation handling up to Model._collect_sub_objects(), it should be possible to get rid of the special-case here.

comment:23 follow-up: Changed 5 years ago by russellm

The general approach here looks really good. Some review comments:

  • I got a test failure running the admin_util.NestedObjectsTests.test_siblings test (got [0, [2, 1]], expected [0, [1, 2]]). The problem appears to be that NestedObjects uses a set to collect objects, which therefore doesn't give a guaranteed result order. One fix is to use a list instead if a set; the other fix is to modify the test so it isn't order dependent. I'm fairly certain just using a list is all that is required - I couldn't see anything obvious that was relying on the fact that sets were unique or easy to look up.
  • I'm not wild about the inner _format_callback function in get_deleted_objects(). admin_site, perms_needed, and levels_to_root are all variables from a scope outside the function itself, which is a bit of a code smell. I'd be much happier if _format_callback was a standalone function in its own right that took these extra values as kwargs that were passed in by nested (i.e., nested takes any extra kwargs and passes them down the the callback)
  • The one test case that I can see that is obviously missing is inheritance. On paper, this is probably caught by deleting OneToOneFields, but there is enough special handling for inheritance that it's worth having a specific test case for it (suggestion: SuperVillain subclassing Villain, create a SuperVillain; what happens if you delete the villain? if you delete the supervillain?)
  • The template change makes me mildly nervous. Ideally, I'd prefer that this change wasn't necessary, as it will break any existing templates in the field that are constructed against the context that has historically existed - even if that just means introducing a dummy top-level list entry so the old template can iterate over a single element.
  • The comments on the add() method for NestedObjects makes a point of highlighting that the model, pk and parent_model arguments aren't actually used. I accept that they are completely redundant as they can be derived from the obj and parent_obj arguments. However, I'm a little nervous
  • Is there any reason that NestedObjects can't be merged into CollectedObjects? It seems to me like there is a lot of duplication between the two classes, and we would be better served by extending CollectedObjects to provide the nested() functionality rather than have a second collection class.

comment:24 in reply to: ↑ 23 Changed 5 years ago by carljm

Thanks for the review, very helpful.

Replying to russellm:

  • I got a test failure running the admin_util.NestedObjectsTests.test_siblings test (got [0, [2, 1]], expected [0, [1, 2]]). The problem appears to be that NestedObjects uses a set to collect objects, which therefore doesn't give a guaranteed result order. One fix is to use a list instead if a set; the other fix is to modify the test so it isn't order dependent. I'm fairly certain just using a list is all that is required - I couldn't see anything obvious that was relying on the fact that sets were unique or easy to look up.

Right, I'll just use a list. Somehow I thought the set was to prevent duplicates, but I check for already-seen first, so that's not an issue.

  • I'm not wild about the inner _format_callback function in get_deleted_objects(). admin_site, perms_needed, and levels_to_root are all variables from a scope outside the function itself, which is a bit of a code smell. I'd be much happier if _format_callback was a standalone function in its own right that took these extra values as kwargs that were passed in by nested (i.e., nested takes any extra kwargs and passes them down the the callback)

One man's closure is another's code smell, I guess ;-) Just don't tell the Lispers & Javascripters. I think the closure style is simpler and more elegant here than adding kwargs all over the place, but I'm not tied to it; I'll use your suggestion instead.

  • The one test case that I can see that is obviously missing is inheritance. On paper, this is probably caught by deleting OneToOneFields, but there is enough special handling for inheritance that it's worth having a specific test case for it (suggestion: SuperVillain subclassing Villain, create a SuperVillain; what happens if you delete the villain? if you delete the supervillain?)

I was trying to strike a balance of thoroughly testing the admin functionality without redundantly testing Model._collect_sub_objects, which is tested elsewhere; since my code has no special handling for inheritance I thought this fell into the "redundant" category. But I guess that same argument could apply to the multiple-fkey tests; I'll add inheritance tests.

  • The template change makes me mildly nervous. Ideally, I'd prefer that this change wasn't necessary, as it will break any existing templates in the field that are constructed against the context that has historically existed - even if that just means introducing a dummy top-level list entry so the old template can iterate over a single element.

Ok. I'll use a dummy top-level list wrapper to keep the template context backwards-compatible, but I'll at least fix the template misspelling of "deletable," since that's internal to the short loop ;)

  • The comments on the add() method for NestedObjects makes a point of highlighting that the model, pk and parent_model arguments aren't actually used. I accept that they are completely redundant as they can be derived from the obj and parent_obj arguments. However, I'm a little nervous

Was there more here that got cut off regarding the nature of your nervousness? If I do merge CollectedObjects and NestedObjects, I would at least be tempted to remove redundancy from the .add() API, but this would require giving CollectedObjects knowledge about model internals (specifically model.pk), which currently it avoids. Do you have a clear preference between the simpler API (object, parent), which requires the collection class to know it's collecting model instances, vs. the current redundant API (obj_class, obj_pk, obj, parent_class, parent_pk), where the redundancy is needed because NestedObjects ultimately needs access to the full instance?

  • Is there any reason that NestedObjects can't be merged into CollectedObjects? It seems to me like there is a lot of duplication between the two classes, and we would be better served by extending CollectedObjects to provide the nested() functionality rather than have a second collection class.

These last two issues are a result of me trying to touch django.db.* as little as possible. I certainly thought of merging CollectedObjects and NestedObjects; if you like the idea, I'll give it a shot. I'm a little concerned that the internals look more similar than they actually are, and the combined code may be more complex and harder to read than the separated versions; but I'll see how it works out.

Changed 5 years ago by carljm

incorporates all of Russ' suggestions except for merging/API changes for NestedObjects? and CollectedObjects? - also removes unneeded parens to make Alex Gaynor happy

comment:25 Changed 5 years ago by carljm

I spent some time at the sprints today working on merging NestedObjects and CollectedObjects. I don't have working merged code yet, but my feeling so far is that it's not worth it. The two collections accept the same .add() API, but the needed semantics in each case are quite specific and quite different.

CollectedObjects cares only about dependencies between model classes, not actual model objects, and it doesn't care about nullable relations (the delete is still cascaded across the nullable FK, but because its nullable it doesn't affect the order of deletions). Aside from the nullable FKs, it needs to build a complete depgraph (leaving open the possibility of cyclic deps), and then just bail in the cyclic case.

NestedObjects, OTOH, needs to build a depgraph of individual objects, with nullable relations treated no differently from non-nullable (either way it's a parent relationship for the nested list). It doesn't even care about having a complete depgraph, just "as much as possible without including a given object twice," which also ensures no cyclic deps.

Given these differing semantics, the merged collection class ends up looking very much like two separate collections internally anyway, and I think there's a net loss in clarity of purpose.

If you feel strongly about at least seeing a working merged version for comparison before moving ahead, comment to that effect and I'll continue working on it.

comment:26 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [12598]) Fixed #6191, #11296 -- Modified the admin deletion confirmation page to use the same object collection scheme as the actual deletion. This ensures that all objects that may be deleted are actually deleted, and that cyclic display problems are avoided. Thanks to carljm for the patch.

comment:27 Changed 5 years ago by russellm

(In [12600]) [1.1.X] Fixed #6191, #11296 -- Modified the admin deletion confirmation page to use the same object collection scheme as the actual deletion. This ensures that all objects that may be deleted are actually deleted, and that cyclic display problems are avoided. Thanks to carljm for the patch.

Backport of r12598 from trunk.

comment:28 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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