Re: [Django Code] #6191: Admin delete view doesn't show all items in some circumstances

[Django Code]

#6191: Admin delete view doesn't show all items in some circumstances
--------------------------------------+-------------------------------------
Reporter: nicklane | Owner: nobody
Status: new | Milestone:
Component: Admin interface | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
--------------------------------------+-------------------------------------
Changes (by nicklane):

* version: newforms-admin => SVN
* needs_tests: 1 => 0

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:8>
Django Code <http://code.djangoproject.com/>
The web framework for perfectionists with deadlines

[Django]

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

-------------------------------------------+--------------------------------

Reporter: nicklane | Owner: nobody
Status: new | Milestone:

Component: django.contrib.admin | Version: SVN

Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0

Needs_better_patch: 1 |
-------------------------------------------+--------------------------------
Changes (by kmtracey):

* needs_better_patch: 0 => 1

Comment:

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

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:9>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: nobody
Status: new | Milestone:
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 1 |
-------------------------------------------+--------------------------------

Changes (by carljm):

* cc: carljm (added)

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:10>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: nobody
Status: new | Milestone:
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 1 |
-------------------------------------------+--------------------------------

Changes (by daemondazz):

* cc: daemondazz (added)

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:11>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: nobody
Status: new | Milestone:
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 1 |
-------------------------------------------+--------------------------------

Comment (by kmtracey):

#11821 reported this again and has a patch.

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:12>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: nobody
Status: new | Milestone:
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 1 |
-------------------------------------------+--------------------------------

Changes (by shi...@msrl.com):

* cc: mshi...@google.com (added)

Comment:

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:13>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------

Reporter: nicklane | Owner: carljm

Status: new | Milestone:
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 1 |
-------------------------------------------+--------------------------------

Changes (by carljm):

* owner: nobody => carljm

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:14>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone:
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 1 |
-------------------------------------------+--------------------------------

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

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:15>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone:
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 1 |
-------------------------------------------+--------------------------------
Comment (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?

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:16>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone:
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 1 |
-------------------------------------------+--------------------------------
Comment (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.

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:17>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone:
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 1 |
-------------------------------------------+--------------------------------
Comment (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.

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:18>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm

Status: new | Milestone: 1.2

Component: django.contrib.admin | Version: SVN
Resolution: | Keywords: nfa-someday
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0

Needs_better_patch: 0 |
-------------------------------------------+--------------------------------
Changes (by carljm):

* needs_better_patch: 1 => 0
* milestone: => 1.2

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:19>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone: 1.2
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords:

Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-------------------------------------------+--------------------------------
Changes (by carljm):

* keywords: nfa-someday =>

Comment:

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:20>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone: 1.2
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords:
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-------------------------------------------+--------------------------------

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

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:21>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone: 1.2
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords:
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-------------------------------------------+--------------------------------
Comment (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.

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:22>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone: 1.2
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords:
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-------------------------------------------+--------------------------------

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

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:23>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone: 1.2
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords:
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-------------------------------------------+--------------------------------

Comment (by carljm):

Thanks for the review, very helpful.

Replying to [comment:23 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.

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:24>

[Django]

#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone: 1.2
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords:
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-------------------------------------------+--------------------------------
Comment (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.

--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:25>