MPTT cache_tree_children error on filtered pages queryset #978

Closed
brente opened this Issue Sep 1, 2011 · 5 comments

Comments

Projects
None yet
3 participants
Contributor

brente commented Sep 1, 2011

CMSChangeList querysets now get sorted by tree_id and lft. This solves issues like #835 (sorting with NULL parents was inconsistent across database engines), but the same MPTT error occurs if the queryset filters out pages from the higher levels, especially from the first trees.

For instance, this can happen when the user applies filters on templates or status, or if the first visible page according to her page permissions is on a higher level than any of the others.

This is because mptt.templatetags.mptt_tags.cache_tree_children assumes the first node in the queryset to be the root.

I fixed this by trapping the ValueError exception MPTT raises in CMSChangeList.set_items to give up caching:

    try:
        mptt_tags.cache_tree_children(pages)
    except ValueError:
        #restore initial list
        pages = all_pages[:]

I also had to change the subsequent call to page.get_children to

        children = list(page.get_children())

because negative indexing on children (a queryset, without caching) was not permitted.

I'm forking 2.2RC2 and will be submitting this patch soon.

Collaborator

ojii commented Sep 1, 2011

I was able to reproduce this issue. kind of worrying...

Not entirely sure I agree with the proposed fix though, investigating...

Collaborator

ojii commented Sep 1, 2011

I see three possible solutions (please contribute others if you can think of them):

  1. Use your proposed fix (which is suboptimal for performance reasons)
  2. Disable the ability to filter pages on changelist
  3. Find a way to do 'cache_tree_children' regardless of tree state.

Not 100% sure what way to go yet.

1 is problematic due to perfomance reasons, 2 will probably get some negative reaction from people using that feature and 3 is the hardest to implement.

Contributor

brente commented Sep 1, 2011

I agree: it's more a workaround than a fix, but I was in a hurry because I needed people to go on with the tests, and it looked like disabling the caching would only impact performance.
(By the way, I haven't submitted anything yet because I didn't have time to write proper test.)

I'm afraid using cache_tree_children with filtered querysets could also produce unexpected results without actually causing any errors.

Consider a simple structure with two trees with one subnode each, like this:

1
    1.1
2
    2.1

If node 2 is excluded from the queryset (permissions, changelist filters, whatever) node 2.1 ends up as a child of node 1. This also makes my workaround worthless.

So it looks like cache_tree_children is perfectly safe only if you're dealing with complete subtrees.

I might have missed something. I will try to look into this tomorrow, and hope to come up with something useful.

Collaborator

ojii commented Sep 1, 2011

well i'd say it's not cache_tree_chidrens fault, but how the hell should we display a partial tree?

That's what makes me kinda favor route 2...

Contributor

brente commented Sep 1, 2011

I always assumed users with partial permissions would only see the hierarchy whenever possible. In fact, they already do: if a user is only allowed to edit the home page and a subtree starting from a subnode of a different tree, she will see both nodes as roots in the changelist.

Actually, I don't think there's anything wrong with flattening the structure whenever a changelist filter is applied, as the application already does: if I'm looking for pages awaiting approval, or using a specific template, I sort of expect a list of pages, not a tree. The only problem in this case is that mptt does expect a proper tree. (As a side note, if the result is a simple list of pages, with no hierarchy, we can probably skip all the children processing)

The real issue, here, is tree disruption caused by user permissions, and IMO route 2 is not a solution, because it only covers changelist filters.

One possibility would be always showing full subtrees and only allow users to enter the pages they can actually edit, i.e. always retrieving and displaying the full ancestry of editable nodes, leaving unmodifiable parents inactive and hiding completely inaccessible subtrees. This would probably be a nightmare performance-wise, unless we find a way to leverage the cache globally.

Or we could leave it as it is (i.e. tree fragments and isolated pages on the same level as the root), but we need to fix the way parent-children relationships are cached to prevent a misrepresentation.

ojii closed this in 9a0684c Sep 2, 2011

chrisglass closed this in 6ef4ed6 Sep 2, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment