MPTT fields not properly updated on target after instance.move_to(...) #188

Closed
orokusaki opened this Issue May 20, 2012 · 8 comments

3 participants

@orokusaki

Given the tree::

Sports
    Fishing
        Fishing Poles
        Fishing Lures

The following code is problematic::

>>> poles = Node.objects.get(title=u'Fishing Poles')
>>> lures = Node.objects.get(title=u'Fishing Lures')
>>> # Check the original values
>>> poles.mptt_left, poles.mptt_right
(3, 4)
>>> lures.mptt_left, lures.mptt_right
(5, 6)
>>> # Move the "Lures" node to the left of the "Poles" node
>>> lures.move_to(poles, 'left')
>>> poles.mptt_left, poles.mptt_right
(3, 4)
>>> lures.mptt_left, lures.mptt_right
(3, 4)
>>> # Fetch them from the DB to check the *real* values
>>> poles = Node.objects.get(title=u'Fishing Poles')
>>> lures = Node.objects.get(title=u'Fishing Lures')
>>> poles.mptt_left, poles.mptt_right
(5, 6)
>>> lures.mptt_left, lures.mptt_right
(3, 4)

In my example, I'm using move_to(target, "left"), but I noticed the same result with "right", and possibly in one other circumstance.

I noticed this because a post_save handler in my application was using instance.get_ancestors(include_self=True) (which returned the wrong self, since the mptt fields were not properly updated on instance), and I was sending the post_save signal for both nodes after move_to was called.

@orokusaki

It turns out the target is never updated inside of any of the move/insert methods :( which is unfortunate, because I can't think of many cases where target shouldn't be changed. node.move_to(target, 'left|right|first-child|last-child') will/can affect the left and right attrs of target.

With some guidance from craigds (e.g. "It's ok to update target inside of methods A, B, and C, so go ahead and provide a patch", etc.), I can provide a patch.

@craigds
Collaborator

This is difficult/impossible to fix, since Django's ORM doesn't maintain a registry of the instances. That is, once you've fetched an object from the database, it's completely independent of the other objects you fetched.

So there's no way that poles can update lures, because there's no reference between them.

We hack-fixed this issue for the specific case of parent-child relationships, where there is a direct reference between them. That is, this code will work as expected:

>>> a = Node.objects.create()
>>> b = Node.objects.create(parent=a)
>>> (a.lft, a.rght)
(0, 3)

The only way to fix this in the general case would be to maintain our own thread-local global index of model instances. I don't think that's practical - it would need to use weakrefs and do garbage collection to avoid memory leaks, and that's opening a whole can of worms.

Related is https://code.djangoproject.com/ticket/17 in which adding this kind of capability to the ORM was considered

@orokusaki

I'm not sure I follow. I'm not suggesting updating all the nodes that are effected, only the ones being operated on.

Take TreeManager._move_child_within_tree for instance, which is called with both the node being moved and the target node. If the target's attributes were updated at the bottom of this method, would this cause a problem? If not, it would be a nice improvement.

@craigds
Collaborator

Right, I see. The discrepancy arises when you have something like this:

nodes = []
for i in range(10):
    last = nodes[-1] if nodes else None
    new_node = Node.objects.create()
    new_node.move_to(last, 'left')

I've seen this sort of code before, seems pretty common.

Unfortunately even if we make move_to() set the correct lft and rght on both new_node and last, the tree fields on the other 8 nodes are still going to be wrong. (and it's less obvious why they're wrong than with the current behaviour)

@orokusaki

Ah, makes sense now. I feared there might have been some case that precluded my suggestion. Thanks much for the explanation, Craig.

@craigds
Collaborator
@matthiask
Collaborator

The comment on issue #257 also applies here. You should make sure that all nodes reflect the current state of the database before calling move_to:
http://django-mptt.github.io/django-mptt/models.html#move-to-target-position-first-child

We should at least add a big warning to the documentation if we can't fix this in code...

@craigds
Collaborator

see #257 for updates. closing this one

@craigds craigds closed this Nov 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment