My favorites | Sign in
Project Home Downloads Wiki Issues Source
READ-ONLY: This project has been archived. For more information see this post.
Search
for
  Advanced search   Search tips   Subscriptions
Issue 23: delete node destroys the tree
4 people starred this issue and may be notified of changes. Back to list
Status:  Fixed
Owner:  jonathan.buchanan
Closed:  Oct 2008


 
Reported by mara.dra...@pareto.nl, May 16, 2008
1. I make a tree having the following structure

test
--test1
----test1_1
------test1_1_1
----test1_2
--test2
----test2_1
--test3

2. I call the delete method on test1
3. If I display the tree after deletion I don't see test2_1

What is the expected output? What do you see instead?

I am using django mptt 0.2.1.
I also tested it with the trunk version and still the same problem.

I attached my test file.
tree.txt
1.7 KB   View   Download
Jun 2, 2008
#1 jeroen.v...@gmail.com
This patch implements the test (no fix yet) for the test system as it is used now.
complex-deletion.patch
2.5 KB   View   Download
Jun 2, 2008
#2 lniel...@spacetelescope.org
I can acknowledge this bug. The problem is that django will call pre_delete method on all objects, that are being 
deleted. In above case, this means that pre_delete will be called for:
- test1, test1_1, test1_1_1, test1_2

The pre_delete method calls the managers _close_gaps which update the lft and rght fields of all rows. This 
update should only happen *once* for the entire delete opertaion! But at the moment the lft and rght fields are 
updated for each object being deleted, thus in above case the update of lft/rght fields are updated four times.

Jun 2, 2008
#3 lniel...@spacetelescope.org
This is an example of the SQL queries being executed when you try to delete an node with children in it.

# Originating from pre_delete on node being deleted
UPDATE `menus_menuitem` SET `lft` = CASE WHEN `lft` > 6 THEN `lft` + -4 ELSE `lft` END, `rght` = CASE WHEN `rght` > 6 THEN `rght` + -4 ELSE `rght` END WHERE `tree_id` = 1 AND (`lft` > 6 OR `rght` > 6)
# Originating from pre_delete on the childs node also being deleted
UPDATE `menus_menuitem` SET `lft` = CASE WHEN `lft` > 5 THEN `lft` + -2 ELSE `lft` END, `rght` = CASE 
WHEN `rght` > 5 THEN `rght` + -2 ELSE `rght` END WHERE `tree_id` = 1 AND (`lft` > 5 OR `rght` > 5)
# Originating from Django when deleting an object with foreign keys 
UPDATE `menus_menuitem` SET `parent_id`=NULL WHERE `id` IN (215,216)
DELETE FROM `menus_menuitem` WHERE `id` IN (216,215)


it ought to look like this, to not destroy the tree:


UPDATE `menus_menuitem` SET `lft` = CASE WHEN `lft` > 6 THEN `lft` + -4 ELSE `lft` END, `rght` = CASE 
WHEN `rght` > 6 THEN `rght` + -4 ELSE `rght` END WHERE `tree_id` = 1 AND (`lft` > 6 OR `rght` > 6)
UPDATE `menus_menuitem` SET `parent_id`=NULL WHERE `id` IN (215,216)
DELETE FROM `menus_menuitem` WHERE `id` IN (216,215


Jun 2, 2008
#4 jeroen.v...@gmail.com
A fix could be implemented by overriding the save method. The attached patch is a
simple way to do this.
deletion-fix.patch
29.7 KB   View   Download
Jun 3, 2008
#5 lniel...@spacetelescope.org
I've tested above patch and it works. All tests passes including my own tests on my menu system. Thanks :-)
Jul 24, 2008
#6 lukasor...@gmail.com
Works for me pretty well! Although there surely is a more elegant way without
monkey-patching the save-method?
Sep 22, 2008
#7 jason.da...@gmail.com
Doesn't the patch just do the same thing as using a post_delete signal instead of a 
pre_delete one?  This is an important bug as it messes up the MPTT structure whenever 
we try and delete objects in the admin.
Oct 12, 2008
Project Member #8 jonathan.buchanan
Crud - for some reason I assumed pre_delete wouldn't get called for every model - the
curren
Status: Accepted
Owner: jonathan.buchanan
Labels: -Priority-Medium Priority-Critical
Oct 13, 2008
Project Member #9 jonathan.buchanan
Fixed in revision 117, with a fix similar to jeroen.vloothuis's patch but using a
simple decorator and wraps() instead. Thanks for the test cases and patches.

This includes an in-progress, but working, refactor of the tests which includes a
regression test.
Status: Fixed

Powered by Google Project Hosting