delete node destroys the tree

[GoogleCodeExporter]

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.

Original issue reported on code.google.com by mara.dra...@pareto.nl on 16 May 2008 at 2:03

Attachments:

• tree.txt

[GoogleCodeExporter]

This patch implements the test (no fix yet) for the test system as it is used 
now.

Original comment by jeroen.v...@gmail.com on 2 Jun 2008 at 2:43

Attachments:

• complex-deletion.patch

[GoogleCodeExporter]

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.

Original comment by lniel...@spacetelescope.org on 2 Jun 2008 at 5:15

[GoogleCodeExporter]

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

Original comment by lniel...@spacetelescope.org on 2 Jun 2008 at 5:20

[GoogleCodeExporter]

A fix could be implemented by overriding the save method. The attached patch is 
a
simple way to do this.

Original comment by jeroen.v...@gmail.com on 2 Jun 2008 at 8:48

Attachments:

• deletion-fix.patch

[GoogleCodeExporter]

I've tested above patch and it works. All tests passes including my own tests 
on my menu system. Thanks :-)

Original comment by lniel...@spacetelescope.org on 3 Jun 2008 at 9:39

[GoogleCodeExporter]

Works for me pretty well! Although there surely is a more elegant way without
monkey-patching the save-method?

Original comment by lukasor...@gmail.com on 24 Jul 2008 at 10:26

[GoogleCodeExporter]

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.

Original comment by jason.da...@gmail.com on 22 Sep 2008 at 2:34

[GoogleCodeExporter]

Crud - for some reason I assumed pre_delete wouldn't get called for every model 
- the
curren

Original comment by jonathan.buchanan on 12 Oct 2008 at 9:59

• Changed state: Accepted
• Added labels: Priority-Critical
• Removed labels: Priority-Medium

[GoogleCodeExporter]

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.

Original comment by jonathan.buchanan on 13 Oct 2008 at 10:35

• Changed state: Fixed