Improve MPTT

- Remove the order on MPTT
- Make room with queries using left/right instead of ids
- Call _update_tree multiple times for small list instead of rebuild the all
  tree
- Improve test for with many roots
- Factorize the test to update MPTT

issue1892
review1007002

CHANGELOG

@@ -1,3 +1,4 @@
+* Remove the default order on MPTT
 * Add grouped attribute to data tag
 * Store selection tree state
 * Add order to Action Window

trytond/model/modelsql.py

@@ -381,17 +381,8 @@ class ModelSQL(ModelStorage):
         records = cls.browse(new_ids)
         cls._validate(records)
 
-        # Check for Modified Preorder Tree Traversal
-        for k in cls._fields:
-            field = cls._fields[k]
-            if (isinstance(field, fields.Many2One)
-                    and field.model_name == cls.__name__
-                    and field.left and field.right):
-                if len(new_ids) == 1:
-                    cls._update_tree(new_ids[0], k, field.left, field.right)
-                else:
-                    with Transaction().set_user(0):
-                        cls._rebuild_tree(k, False, 0)
+        field_names = cls._fields.keys()
+        cls._update_mptt(field_names, [new_ids] * len(field_names))
 
         cls.trigger_create(records)
         return records
@@ -826,21 +817,8 @@ class ModelSQL(ModelStorage):
 
         cls._validate(records)
 
-        # Check for Modified Preorder Tree Traversal
-        for k in cls._fields:
-            field = cls._fields[k]
-            if isinstance(field, fields.Many2One) \
-                    and field.model_name == cls.__name__ \
-                    and field.left and field.right:
-                if field.left in values or field.right in values:
-                    raise Exception('ValidateError',
-                            'You can not update fields: "%s", "%s"' %
-                            (field.left, field.right))
-                if len(ids) == 1:
-                    cls._update_tree(ids[0], k, field.left, field.right)
-                else:
-                    with Transaction().set_user(0):
-                        cls._rebuild_tree(k, False, 0)
+        field_names = cls._fields.keys()
+        cls._update_mptt(field_names, [ids] * len(field_names), values)
 
         cls.trigger_write(trigger_eligibles)
 
@@ -1017,13 +995,7 @@ class ModelSQL(ModelStorage):
                     '(id, write_uid, write_date) VALUES (%s, %s, %s)',
                     (obj_id, Transaction().user, datetime.datetime.now()))
 
-        for k in tree_ids.keys():
-            field = cls._fields[k]
-            if len(tree_ids[k]) == 1:
-                cls._update_tree(tree_ids[k][0], k, field.left, field.right)
-            else:
-                with Transaction().set_user(0):
-                    cls._rebuild_tree(k, False, 0)
+        cls._update_mptt(tree_ids.keys(), tree_ids.values())
 
     @classmethod
     def search(cls, domain, offset=0, limit=None, order=None, count=False,
@@ -1894,6 +1866,31 @@ class ModelSQL(ModelStorage):
         raise Exception('Error', 'Wrong field name (%s) for %s in order!' %
             (field, cls.__name__))
 
+    @classmethod
+    def _update_mptt(cls, field_names, list_ids, values=None):
+        cursor = Transaction().cursor
+        count = None
+        for field_name, ids in zip(field_names, list_ids):
+            field = cls._fields[field_name]
+            if (isinstance(field, fields.Many2One)
+                    and field.model_name == cls.__name__
+                    and field.left and field.right):
+                if (values is not None
+                        and (field.left in values or field.right in values)):
+                    raise Exception('ValidateError',
+                        'You can not update fields: "%s", "%s"' %
+                        (field.left, field.right))
+                if count is None:
+                    cursor.execute('SELECT COUNT(1) FROM "' + cls._table + '"')
+                    count, = cursor.fetchone()
+                if len(ids) < count / 4:
+                    for id_ in ids:
+                        cls._update_tree(id_, field_name,
+                            field.left, field.right)
+                else:
+                    with Transaction().set_user(0):
+                        cls._rebuild_tree(field_name, None, 0)
+
     @classmethod
     def _rebuild_tree(cls, parent, parent_id, left):
         '''
@@ -1902,10 +1899,9 @@ class ModelSQL(ModelStorage):
         cursor = Transaction().cursor
         right = left + 1
 
-        with Transaction().set_user(0):
-            childs = cls.search([
-                    (parent, '=', parent_id),
-                    ])
+        childs = cls.search([
+                (parent, '=', parent_id),
+                ], order=[])
 
         for child in childs:
             right = cls._rebuild_tree(parent, child.id, right)
@@ -1920,35 +1916,36 @@ class ModelSQL(ModelStorage):
         return right + 1
 
     @classmethod
-    def _update_tree(cls, object_id, field_name, left, right):
+    def _update_tree(cls, record_id, field_name, left, right):
         '''
         Update left, right values for the tree.
         Remarks:
             - the value (right - left - 1) / 2 will not give
                 the number of children node
-            - the order of the tree respects the default _order
         '''
         cursor = Transaction().cursor
-        cursor.execute('SELECT "' + left + '", "' + right + '" '
+        cursor.execute('SELECT "' + left + '", "' + right + '", '
+                '"' + field_name + '" '
             'FROM "' + cls._table + '" '
-            'WHERE id = %s', (object_id,))
+            'WHERE id = %s', (record_id,))
         fetchone = cursor.fetchone()
         if not fetchone:
             return
-        old_left, old_right = fetchone
-        if old_left == old_right:
+        old_left, old_right, parent_id = fetchone
+        if old_left == old_right == 0:
+            cursor.execute('SELECT MAX("' + right + '") '
+                'FROM "' + cls._table + '" '
+                'WHERE "' + field_name + '" IS NULL')
+            old_left, = cursor.fetchone()
+            old_left += 1
+            old_right = old_left + 1
             cursor.execute('UPDATE "' + cls._table + '" '
-                'SET "' + right + '" = "' + right + '" + 1 '
-                'WHERE id = %s', (object_id,))
-            old_right += 1
+                'SET "' + left + '" = %s, "' + right + '" = %s '
+                'WHERE id = %s', (old_left, old_right, record_id))
+        size = old_right - old_left + 1
 
         parent_right = 1
 
-        cursor.execute('SELECT "' + field_name + '" '
-            'FROM "' + cls._table + '" '
-            'WHERE id = %s', (object_id,))
-        parent_id = cursor.fetchone()[0] or False
-
         if parent_id:
             cursor.execute('SELECT "' + right + '" '
                 'FROM "' + cls._table + '" '
@@ -1962,85 +1959,25 @@ class ModelSQL(ModelStorage):
             if fetchone:
                 parent_right = fetchone[0] + 1
 
-        cursor.execute('SELECT id FROM "' + cls._table + '" '
-            'WHERE "' + left + '" >= %s AND "' + right + '" <= %s',
-            (old_left, old_right))
-        child_ids = [x[0] for x in cursor.fetchall()]
-
-        if len(child_ids) > cursor.IN_MAX:
-            with Transaction().set_user(0):
-                return cls._rebuild_tree(field_name, False, 0)
-
-        red_child_sql, red_child_ids = reduce_ids('id', child_ids)
-        # ids for left update
-        cursor.execute('SELECT id FROM "' + cls._table + '" '
-            'WHERE "' + left + '" >= %s '
-                'AND NOT ' + red_child_sql,
-            [parent_right] + red_child_ids)
-        left_ids = [x[0] for x in cursor.fetchall()]
-
-        # ids for right update
-        cursor.execute('SELECT id FROM "' + cls._table + '" '
-            'WHERE "' + right + '" >= %s '
-                'AND NOT ' + red_child_sql,
-            [parent_right] + red_child_ids)
-        right_ids = [x[0] for x in cursor.fetchall()]
-
-        if left_ids:
-            for i in range(0, len(left_ids), cursor.IN_MAX):
-                sub_ids = left_ids[i:i + cursor.IN_MAX]
-                red_sub_sql, red_sub_ids = reduce_ids('id', sub_ids)
-                cursor.execute('UPDATE "' + cls._table + '" '
-                    'SET "' + left + '" = "' + left + '" + '
-                        + str(old_right - old_left + 1) + ' '
-                    'WHERE ' + red_sub_sql, red_sub_ids)
-        if right_ids:
-            for i in range(0, len(right_ids), cursor.IN_MAX):
-                sub_ids = right_ids[i:i + cursor.IN_MAX]
-                red_sub_sql, red_sub_ids = reduce_ids('id', sub_ids)
-                cursor.execute('UPDATE "' + cls._table + '" '
-                    'SET "' + right + '" = "' + right + '" + '
-                        + str(old_right - old_left + 1) + ' '
-                    'WHERE ' + red_sub_sql, red_sub_ids)
-
         cursor.execute('UPDATE "' + cls._table + '" '
-            'SET "' + left + '" = "' + left + '" + '
-                    + str(parent_right - old_left) + ', '
-                '"' + right + '" = "' + right + '" + '
-                    + str(parent_right - old_left) + ' '
-            'WHERE ' + red_child_sql, red_child_ids)
-
-        # Use root user to by-pass rules
-        with contextlib.nested(Transaction().set_user(0),
-                Transaction().set_context(active_test=False)):
-            brother_ids = map(int, cls.search([
-                        (field_name, '=', parent_id),
-                        ]))
-        if brother_ids[-1] != object_id:
-            next_id = brother_ids[brother_ids.index(object_id) + 1]
-            cursor.execute('SELECT "' + left + '" '
-                'FROM "' + cls._table + '" '
-                'WHERE id = %s', (next_id,))
-            next_left = cursor.fetchone()[0]
-            cursor.execute('SELECT "' + left + '" '
-                'FROM "' + cls._table + '" '
-                'WHERE id = %s', (object_id,))
-            current_left = cursor.fetchone()[0]
-
-            cursor.execute('UPDATE "' + cls._table + '" '
-                'SET "' + left + '" = "' + left + '" + '
-                        + str(old_right - old_left + 1) + ', '
-                    '"' + right + '" = "' + right + '" + '
-                        + str(old_right - old_left + 1) + ' '
-                'WHERE "' + left + '" >= %s AND "' + right + '" <= %s',
-                (next_left, current_left))
-
-            cursor.execute('UPDATE "' + cls._table + '" '
-                'SET "' + left + '" = "' + left + '" - '
-                        + str(current_left - next_left) + ', '
-                    '"' + right + '" = "' + right + '" - '
-                        + str(current_left - next_left) + ' '
-                'WHERE ' + red_child_sql, red_child_ids)
+            'SET "' + left + '" = "' + left + '" + %s '
+            'WHERE "' + left + '" >= %s', (size, parent_right))
+        cursor.execute('UPDATE "' + cls._table + '" '
+            'SET "' + right + '" = "' + right + '" + %s '
+            'WHERE "' + right + '" >= %s', (size, parent_right))
+        if old_left < parent_right:
+            params = (parent_right - old_left,
+                parent_right - old_left,
+                old_left, old_right)
+        else:
+            params = (parent_right - old_left - size,
+                parent_right - old_left - size,
+                old_left + size, old_right + size)
+        cursor.execute('UPDATE "' + cls._table + '" '
+            'SET "' + left + '" = "' + left + '" + %s, '
+                '"' + right + '" = "' + right + '" + %s '
+            'WHERE "' + left + '" >= %s '
+                'AND "' + right + '" <= %s', params)
 
     @classmethod
     def validate(cls, records):

trytond/test/mptt.py

@@ -12,9 +12,6 @@ class MPTT(ModelSQL, ModelView):
     'Modified Preorder Tree Traversal'
     __name__ = 'test.mptt'
     name = fields.Char('Name', required=True)
-    sequence = fields.Integer('Sequence',
-        order_field='(%(table)s.sequence IS NULL) %(order)s, '
-        '%(table)s.sequence %(order)s')
     parent = fields.Many2One('test.mptt', "Parent", select=True,
             left="left", right="right")
     left = fields.Integer('Left', required=True, select=True)
@@ -22,11 +19,6 @@ class MPTT(ModelSQL, ModelView):
     childs = fields.One2Many('test.mptt', 'parent', 'Children')
     active = fields.Boolean('Active')
 
-    @classmethod
-    def __setup__(cls):
-        super(MPTT, cls).__setup__()
-        cls._order.insert(0, ('sequence', 'ASC'))
-
     @classmethod
     def validate(cls, record):
         super(MPTT, cls).validate(record)

trytond/tests/test_mptt.py

@@ -2,6 +2,7 @@
 # -*- coding: utf-8 -*-
 #This file is part of Tryton.  The COPYRIGHT file at the top level of
 #this repository contains the full copyright notices and license terms.
+import sys
 import unittest
 from trytond.tests.test_tryton import POOL, DB_NAME, USER, CONTEXT, \
         install_module
@@ -17,12 +18,10 @@ class MPTTTestCase(unittest.TestCase):
         install_module('test')
         self.mptt = POOL.get('test.mptt')
 
-    def CheckTree(self, parent_id=False, left=0, right=0):
+    def CheckTree(self, parent_id=False, left=-1, right=sys.maxint):
         childs = self.mptt.search([
                 ('parent', '=', parent_id),
-                ])
-        child_index = dict((x.id, i) for i, x in enumerate(childs))
-        childs.sort(key=lambda x: child_index[x.id])
+                ], order=[('left', 'ASC')])
         for child in childs:
             assert child.left > left, \
                 '%s: left %d <= parent left %d' % \
@@ -30,21 +29,21 @@ class MPTTTestCase(unittest.TestCase):
             assert child.left < child.right, \
                 '%s: left %d >= right %d' % \
                 (child, child.left, child.right)
-            assert right == 0 or child.right < right, \
+            assert child.right < right, \
                 '%s: right %d >= parent right %d' % \
                 (child, child.right, right)
             self.CheckTree(child.id, left=child.left,
                 right=child.right)
-        next_left = 0
+        next_left = -1
         for child in childs:
             assert child.left > next_left, \
                 '%s: left %d <= next left %d' % \
                 (child, child.left, next_left)
             next_left = child.right
         childs.reverse()
-        previous_right = 0
+        previous_right = sys.maxint
         for child in childs:
-            assert previous_right == 0 or child.right < previous_right, \
+            assert child.right < previous_right, \
                 '%s: right %d >= previous right %d' \
                 % (child, child.right, previous_right)
             previous_right = child.left
@@ -65,62 +64,28 @@ class MPTTTestCase(unittest.TestCase):
         for record in records:
             self.ReParent(record)
 
-    def ReOrder(self, parent=None):
-        records = self.mptt.search([
-                ('parent', '=', parent),
-                ])
-        if not records:
-            return
-        i = len(records)
-        for record in records:
-            record.sequence = i
-            record.save()
-            i -= 1
-        i = 0
-        for record in records:
-            record.sequence = i
-            record.save()
-            i += 1
-        for record in records:
-            self.ReOrder(record)
-
-        records = self.mptt.search([])
-        self.mptt.write(records, {
-                'sequence': 0,
-                })
-
     def test0010create(self):
         '''
         Create tree.
         '''
         with Transaction().start(DB_NAME, USER,
                 context=CONTEXT) as transaction:
-            new_records = [None]
+            new_records = [None, None, None]
             for j in range(3):
                 parent_records = new_records
                 new_records = []
                 k = 0
                 for parent_record in parent_records:
-                    for i in range(3):
-                        new_records += self.mptt.create([{
-                                    'name': 'Test %d %d %d' % (j, k, i),
-                                    'parent': (parent_record.id
-                                            if parent_record else None),
-                                    }])
+                    new_records += self.mptt.create([{
+                                'name': 'Test %d %d %d' % (j, k, i),
+                                'parent': (parent_record.id
+                                    if parent_record else None),
+                                } for i in range(3)])
                     k += 1
             self.CheckTree()
 
             transaction.cursor.commit()
 
-    def test0020reorder(self):
-        '''
-        Re-order.
-        '''
-        with Transaction().start(DB_NAME, USER,
-                context=CONTEXT) as transaction:
-            self.ReOrder()
-            transaction.cursor.rollback()
-
     def test0030reparent(self):
         '''
         Re-parent.
@@ -141,8 +106,6 @@ class MPTTTestCase(unittest.TestCase):
             self.CheckTree()
             self.ReParent()
             self.CheckTree()
-            self.ReOrder()
-            self.CheckTree()
 
             transaction.cursor.rollback()
 
@@ -164,8 +127,6 @@ class MPTTTestCase(unittest.TestCase):
                     })
             self.ReParent()
             self.CheckTree()
-            self.ReOrder()
-            self.CheckTree()
 
             transaction.cursor.rollback()