[bug] Tree parent fix (#12401)

* Refactor

* row locking on bulk-update

* Rebuild tree in background task

* Regression testing

* Fix unit test

* Allow more generous timeout

src/backend/InvenTree/InvenTree/api.py

@@ -6,6 +6,7 @@ from pathlib import Path
 
 from django.conf import settings
 from django.contrib.contenttypes.models import ContentType
+from django.core.exceptions import ObjectDoesNotExist
 from django.db import transaction
 from django.http import JsonResponse
 from django.urls import path, reverse
@@ -534,8 +535,12 @@ class BulkUpdateMixin(BulkOperationMixin):
 
     Bulk update allows for multiple items to be updated in a single API query,
     rather than using multiple API calls to the various detail endpoints.
+
+    Each instance is validated and saved individually, so that any custom save methods are triggered.
     """
 
+    BULK_ID_FIELD: str = 'pk'
+
     def validate_update(self, queryset, request) -> None:
         """Perform validation right before updating.
 
@@ -579,7 +584,14 @@ class BulkUpdateMixin(BulkOperationMixin):
         # Perform the update operation
         data = request.data
 
-        n = queryset.count()
+        # Extract the primary key values up-front:
+        # Each instance is re-fetched from the database immediately before it is
+        # updated, as saving one instance may alter database state which other
+        # instances in the queryset depend on (e.g. MPTT tree structure fields).
+        # Saving a stale instance can result in database corruption (and it must
+        # be the *instance* that is fresh - refresh_from_db is not sufficient here,
+        # as MPTT caches original field values when the instance is loaded).
+        pk_values = sorted(queryset.values_list(self.BULK_ID_FIELD, flat=True))
 
         instance_data = []
 
@@ -589,7 +601,18 @@ class BulkUpdateMixin(BulkOperationMixin):
             # as we want to trigger any custom post_save methods on the model
 
             # Run validation first
-            for instance in queryset:
+            for pk in pk_values:
+                try:
+                    instance = queryset.select_for_update(of=('self',)).get(**{
+                        self.BULK_ID_FIELD: pk
+                    })
+                except ObjectDoesNotExist:
+                    raise ValidationError({
+                        'non_field_errors': _(
+                            'Item no longer matches the provided criteria'
+                        )
+                    })
+
                 serializer = self.get_serializer(instance, data=data, partial=True)
                 serializer.is_valid(raise_exception=True)
                 serializer.save()
@@ -597,7 +620,7 @@ class BulkUpdateMixin(BulkOperationMixin):
                 instance_data.append(serializer.data)
 
         return Response(
-            {'success': f'Updated {n} items', 'items': instance_data}, status=200
+            {'success': 'Updated multiple items', 'items': instance_data}, status=200
         )
 
 

src/backend/InvenTree/InvenTree/models.py

@@ -744,6 +744,7 @@ class InvenTreeTree(ContentTypeMixin, MPTTModel):
 
         order_insertion_by = ['name']
 
+    @transaction.atomic
     def delete(self, *args, **kwargs):
         """Handle the deletion of a tree node.
 
@@ -814,18 +815,7 @@ class InvenTreeTree(ContentTypeMixin, MPTTModel):
             next_tree_id += 1
 
         # 3. Rebuild the model tree(s) as required
-        #  - If any partial rebuilds fail, we will rebuild the entire tree
+        self.__class__.rebuild_trees(trees)
-
-        result = True
-
-        for tree_id in trees:
-            if tree_id:
-                if not self.partial_rebuild(tree_id):
-                    result = False
-
-        if not result:
-            # Rebuild the entire tree (expensive!!!)
-            self.__class__.objects.rebuild()
 
     def handle_tree_delete(self, delete_children=False, delete_items=False):
         """Delete a single instance of the tree, based on provided kwargs.
@@ -942,12 +932,18 @@ class InvenTreeTree(ContentTypeMixin, MPTTModel):
             # New instance, so we need to rebuild the tree (if it has a parent)
             trees.add(self.tree_id)
 
-        for tree_id in trees:
+        # Flag to indicate that a tree rebuild task was triggered by this save
-            if tree_id:
+        self._tree_rebuild_offloaded = False
-                self.partial_rebuild(tree_id)
 
         if len(trees) > 0:
-            # A tree update was performed, so we need to refresh the instance
+            # Offload the tree rebuild(s) to the background worker.
+            # Note that repeated calls are de-duplicated (per tree),
+            # so a bulk operation results in a single rebuild per affected tree.
+            ran_sync = self.__class__.offload_tree_rebuild(trees)
+            self._tree_rebuild_offloaded = True
+
+            if ran_sync:
+                # The tree was rebuilt synchronously, so refresh the instance
                 try:
                     self.refresh_from_db()
                 except TransactionManagementError:
@@ -958,24 +954,67 @@ class InvenTreeTree(ContentTypeMixin, MPTTModel):
                     InvenTree.sentry.report_exception(e)
                     InvenTree.exceptions.log_error(f'{self.__class__.__name__}.save')
 
-    def partial_rebuild(self, tree_id: int) -> bool:
+    @classmethod
+    def offload_tree_rebuild(cls, tree_ids) -> bool:
+        """Offload a rebuild of the specified trees to the background worker.
+
+        - The tree structure (and pathstring values, where applicable) are rebuilt for each tree
+        - If the background worker is not running, the rebuild is performed synchronously
+        - Identical pending tasks are skipped, so repeated calls (e.g. during a bulk
+          operation) result in (at most) a single queued rebuild per affected tree
+
+        Returns:
+            bool: True if any rebuild was performed synchronously (in the calling thread)
+        """
+        from InvenTree.tasks import offload_task
+
+        ran_sync = False
+
+        for tree_id in tree_ids:
+            if tree_id:
+                result = offload_task(
+                    'InvenTree.tasks.rebuild_model_tree', cls._meta.label_lower, tree_id
+                )
+
+                if result is True:
+                    # offload_task returns True if the task ran synchronously
+                    ran_sync = True
+
+        return ran_sync
+
+    @classmethod
+    def rebuild_trees(cls, tree_ids) -> None:
+        """Rebuild the specified trees, with fallback to a full rebuild.
+
+        - Perform a partial rebuild for each provided tree_id
+        - If any partial rebuild fails, rebuild the entire tree (expensive!!!)
+        """
+        result = True
+
+        for tree_id in tree_ids:
+            if tree_id and not cls.partial_rebuild(tree_id):
+                result = False
+
+        if not result:
+            # Rebuild the entire tree (expensive!!!)
+            cls.objects.rebuild()
+
+    @classmethod
+    def partial_rebuild(cls, tree_id: int) -> bool:
         """Perform a partial rebuild of the tree structure.
 
         If a failure occurs, log the error and return False.
         """
         try:
-            self.__class__.objects.partial_rebuild(tree_id)
+            cls.objects.partial_rebuild(tree_id)
             return True
         except Exception as e:
             # This is a critical error, explicitly report to sentry
             InvenTree.sentry.report_exception(e)
 
-            InvenTree.exceptions.log_error(f'{self.__class__.__name__}.partial_rebuild')
+            InvenTree.exceptions.log_error(f'{cls.__name__}.partial_rebuild')
             logger.exception(
-                'Failed to rebuild tree for %s <%s>: %s',
+                'Failed to rebuild tree <%s> for %s: %s', tree_id, cls.__name__, e
-                self.__class__.__name__,
-                self.pk,
-                e,
             )
             return False
 
@@ -1078,6 +1117,11 @@ class PathStringMixin(models.Model):
         # Rebuild upper first, to ensure the lower nodes are updated correctly
         super().save(*args, **kwargs)
 
+        # Determine if a tree rebuild task was already triggered by this save
+        # (e.g. if the node was re-parented) - if so, the pathstring values
+        # for any lower nodes are updated by that task
+        rebuild_offloaded = getattr(self, '_tree_rebuild_offloaded', False)
+
         # Ensure that the pathstring is correctly constructed
         pathstring = self.construct_pathstring(refresh=True)
 
@@ -1088,12 +1132,10 @@ class PathStringMixin(models.Model):
             self.pathstring = pathstring
             super().save(*args, **kwargs)
 
-            # Bulk-update any child nodes, if applicable
+            # Update the pathstring values for any lower nodes,
-            lower_nodes = list(
+            # by offloading the update to the background worker
-                self.get_descendants(include_self=False).values_list('pk', flat=True)
+            if not rebuild_offloaded and self.get_descendant_count() > 0:
-            )
+                self.__class__.offload_tree_rebuild([self.tree_id])
-
-            self.rebuild_lower_nodes(lower_nodes)
 
     def delete(self, *args, **kwargs):
         """Custom delete method for PathStringMixin.
@@ -1111,9 +1153,7 @@ class PathStringMixin(models.Model):
             )
 
         # Store the node ID values for lower nodes, before we delete this one
-        lower_nodes = list(
+        lower_nodes = self.get_lower_nodes()
-            self.get_descendants(include_self=False).values_list('pk', flat=True)
-        )
 
         # Delete this node - after which we expect the tree structure will be updated
         super().delete(*args, **kwargs)
@@ -1125,6 +1165,12 @@ class PathStringMixin(models.Model):
         """String representation of a category is the full path to that category."""
         return f'{self.pathstring} - {self.description}'
 
+    def get_lower_nodes(self) -> list[int]:
+        """Return a list of all lower nodes in the tree."""
+        return list(
+            self.get_descendants(include_self=False).values_list('pk', flat=True)
+        )
+
     def rebuild_lower_nodes(self, lower_nodes: list[int]):
         """Rebuild the pathstring for lower nodes in the tree.
 
@@ -1145,6 +1191,52 @@ class PathStringMixin(models.Model):
         if len(nodes_to_update) > 0:
             self.__class__.objects.bulk_update(nodes_to_update, ['pathstring'])
 
+    @classmethod
+    def rebuild_tree_pathstring_values(cls, tree_ids) -> None:
+        """Rebuild the 'pathstring' values for all nodes in the specified trees.
+
+        Each tree is processed in a single pass:
+        the pathstring for each node is constructed from its parent node,
+        and any changed values are written back in a single bulk update.
+        """
+        tree_nodes = list(cls.objects.filter(tree_id__in=tree_ids))
+        node_map = {node.pk: node for node in tree_nodes}
+
+        # Cache of node ID -> list of path elements (from the top level down)
+        path_cache: dict[int, list[str]] = {}
+
+        def path_names(node) -> list[str]:
+            """Construct the path (list of names) for a node, via its parent chain."""
+            if node.pk in path_cache:
+                return path_cache[node.pk]
+
+            names = [str(getattr(node, cls.PATH_FIELD, node.pk))]
+
+            if node.parent_id:
+                parent = node_map.get(node.parent_id)
+
+                if parent is None:
+                    # Parent node exists outside the selected trees
+                    parent = cls.objects.get(pk=node.parent_id)
+                    node_map[node.parent_id] = parent
+
+                names = [*path_names(parent), *names]
+
+            path_cache[node.pk] = names
+            return names
+
+        nodes_to_update = []
+
+        for node in tree_nodes:
+            pathstring = InvenTree.helpers.constructPathString(path_names(node))
+
+            if pathstring != node.pathstring:
+                node.pathstring = pathstring
+                nodes_to_update.append(node)
+
+        if len(nodes_to_update) > 0:
+            cls.objects.bulk_update(nodes_to_update, ['pathstring'], batch_size=250)
+
     def construct_pathstring(self, refresh: bool = False) -> str:
         """Construct the pathstring for this tree node.
 

src/backend/InvenTree/InvenTree/tasks.py

@@ -617,6 +617,40 @@ def scheduled_task(
     return _task_wrapper
 
 
+@tracer.start_as_current_span('rebuild_model_tree')
+def rebuild_model_tree(model: str, tree_id: int) -> None:
+    """Rebuild the tree structure (and pathstring values) for a tree model.
+
+    This task is offloaded to the background worker whenever nodes are
+    restructured (e.g. re-parented), to avoid expensive tree rebuild
+    operations blocking the calling thread.
+
+    Arguments:
+        model: Label of the model class to rebuild, e.g. 'stock.stocklocation'
+        tree_id: ID of the tree to rebuild
+    """
+    from django.apps import apps
+
+    import InvenTree.models
+
+    try:
+        model_class = apps.get_model(model)
+    except (LookupError, ValueError):
+        logger.warning("rebuild_model_tree: Model '%s' does not exist", model)
+        return
+
+    if not issubclass(model_class, InvenTree.models.InvenTreeTree):
+        logger.warning("rebuild_model_tree: Model '%s' is not a tree model", model)
+        return
+
+    # Rebuild the tree structure, based on the parent-child relationships
+    model_class.rebuild_trees([tree_id])
+
+    # Rebuild the 'pathstring' values for the entire tree (if applicable)
+    if issubclass(model_class, InvenTree.models.PathStringMixin):
+        model_class.rebuild_tree_pathstring_values([tree_id])
+
+
 @tracer.start_as_current_span('heartbeat')
 @scheduled_task(ScheduledTask.MINUTES, 1)
 def heartbeat():

src/backend/InvenTree/part/test_api.py

@@ -104,6 +104,72 @@ class PartCategoryAPITest(InvenTreeAPITestCase):
         'part_category.delete',
     ]
 
+    def test_bulk_set_parent(self):
+        """Test that bulk re-parenting of categories correctly rebuilds the tree.
+
+        Re-parenting multiple categories in a single 'bulk update' API call must
+        leave the tree structure and the 'pathstring' values fully consistent.
+
+        Ref: https://github.com/inventree/InvenTree/issues/12394
+        """
+        url = reverse('api-part-category-list')
+
+        parent_a = PartCategory.objects.create(name='Parent A')
+        parent_b = PartCategory.objects.create(name='Parent B')
+
+        # Create a number of subcategories under 'Parent A',
+        # some of which have their own child categories
+        categories = []
+
+        for i in range(50):
+            category = PartCategory.objects.create(name=f'Cat{i:02d}', parent=parent_a)
+            categories.append(category)
+
+            if i % 5 == 0:
+                child = PartCategory.objects.create(
+                    name=f'Cat{i:02d}-child', parent=category
+                )
+                PartCategory.objects.create(name=f'Cat{i:02d}-grandchild', parent=child)
+
+        # Move all subcategories to 'Parent B' in a single bulk update.
+        # The query count must scale linearly with the number of items.
+        # Note: when the background worker is not running (e.g. in tests), each
+        # item save runs the (de-duplicated) tree rebuild task synchronously
+        self.patch(
+            url,
+            {'items': [category.pk for category in categories], 'parent': parent_b.pk},
+            expected_code=200,
+            max_query_count=60 * len(categories),
+            max_query_time=10,  # Note: in production this is offloaded to the background worker
+        )
+
+        for category in categories:
+            category.refresh_from_db()
+            self.assertEqual(category.parent, parent_b)
+
+        parent_a.refresh_from_db()
+        parent_b.refresh_from_db()
+
+        # Check *all* categories in the affected trees
+        # (fixture data outside these trees does not have pathstring values set)
+        affected_trees = PartCategory.objects.filter(
+            tree_id__in=[parent_a.tree_id, parent_b.tree_id]
+        )
+
+        for category in affected_trees:
+            # The stored pathstring must match the 'parent' chain for the node
+            chain = []
+            node = category
+
+            while node is not None:
+                chain.insert(0, node)
+                node = node.parent
+
+            self.assertEqual(category.pathstring, '/'.join(node.name for node in chain))
+
+            # The MPTT tree data must match the 'parent' chain, too
+            self.assertEqual(list(category.get_ancestors()), chain[:-1])
+
     def test_category_list(self):
         """Test the PartCategoryList API endpoint."""
         url = reverse('api-part-category-list')

src/backend/InvenTree/stock/test_api.py

@@ -73,6 +73,71 @@ class StockLocationTest(StockAPITestCase):
         for ordering in ['name', 'pathstring', 'level', 'tree_id']:
             self.run_ordering_test(self.list_url, ordering)
 
+    def test_bulk_set_parent(self):
+        """Test that bulk re-parenting of locations correctly rebuilds the tree.
+
+        Re-parenting multiple locations in a single 'bulk update' API call must
+        leave the tree structure and the 'pathstring' values fully consistent.
+
+        Ref: https://github.com/inventree/InvenTree/issues/12394
+        """
+        parent_a = StockLocation.objects.create(name='Parent A')
+        parent_b = StockLocation.objects.create(name='Parent B')
+
+        # Create a number of sub-locations under 'Parent A',
+        # some of which have their own child locations
+        locations = []
+
+        for i in range(25):
+            location = StockLocation.objects.create(name=f'Sub{i:02d}', parent=parent_a)
+            locations.append(location)
+
+            if i % 5 == 0:
+                child = StockLocation.objects.create(
+                    name=f'Sub{i:02d}-child', parent=location
+                )
+                StockLocation.objects.create(
+                    name=f'Sub{i:02d}-grandchild', parent=child
+                )
+
+        # Move all sub-locations to 'Parent B' in a single bulk update.
+        # The query count must scale linearly with the number of items.
+        # Note: when the background worker is not running (e.g. in tests), each
+        # item save runs the (de-duplicated) tree rebuild task synchronously
+        self.patch(
+            self.list_url,
+            {'items': [location.pk for location in locations], 'parent': parent_b.pk},
+            expected_code=200,
+            max_query_count=60 * len(locations),
+        )
+
+        for location in locations:
+            location.refresh_from_db()
+            self.assertEqual(location.parent, parent_b)
+
+        parent_a.refresh_from_db()
+        parent_b.refresh_from_db()
+
+        # Check *all* locations in the affected trees
+        # (fixture data outside these trees does not have pathstring values set)
+        affected_trees = StockLocation.objects.filter(
+            tree_id__in=[parent_a.tree_id, parent_b.tree_id]
+        )
+
+        for location in affected_trees:
+            # The stored pathstring must match the 'parent' chain for the node
+            chain = []
+            node = location
+
+            while node is not None:
+                chain.insert(0, node)
+                node = node.parent
+
+            self.assertEqual(location.pathstring, '/'.join(node.name for node in chain))
+
+            # The MPTT tree data must match the 'parent' chain, too
+            self.assertEqual(list(location.get_ancestors()), chain[:-1])
+
     def test_list(self):
         """Test the StockLocationList API endpoint."""
         test_cases = [
@@ -2227,7 +2292,7 @@ class StockItemTest(StockAPITestCase):
 
         data = response.data
 
-        self.assertEqual(data['success'], 'Updated 10 items')
+        self.assertEqual(data['success'], 'Updated multiple items')
         self.assertEqual(len(data['items']), 10)
 
         for item in data['items']: