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

[nicklane]

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.

[nicklane]

6191.patch

[nicklane]

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?

[Karen Tracey <kmtracey@…>]

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.

[jkocherhans]

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.

[nicklane]

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

[nicklane]

Patch with initial tests

[nicklane]

Patch with initial tests

[nicklane]

Patch with initial tests

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

[nicklane]

Simplified the tests

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

[nicklane]

Patch agains newforms-admin

[nicklane]

Updated patch to work with trunk

[Karen Tracey]

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

[Karen Tracey]

#11821 reported this again and has a patch.

[shields@…]

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.

[Carl Meyer]

updated patch with increased test coverage

[Carl Meyer]

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

2. 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).

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

[Carl Meyer]

new patch that removes the algorithm duplication entirely

[Carl Meyer]

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?

[Carl Meyer]

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.

[Carl Meyer]

updated patch: fixes nested list layout issue with siblings

[Carl Meyer]

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.

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

[Carl Meyer]

updated patch; removes unused added import

[Carl Meyer]

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.

[Carl Meyer]

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.

[Carl Meyer]

updated patch, applies cleanly

[Carl Meyer]

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

[Carl Meyer]

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.

[Russell Keith-Magee]

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.

[Carl Meyer]

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.

[Carl Meyer]

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

[Carl Meyer]

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.

[Russell Keith-Magee]

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

[Russell Keith-Magee]

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

[Jacob]

Milestone 1.2 deleted