diff --git a/src/backend/InvenTree/InvenTree/api.py b/src/backend/InvenTree/InvenTree/api.py index 1dfc53fa5c..82ef505529 100644 --- a/src/backend/InvenTree/InvenTree/api.py +++ b/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 ) diff --git a/src/backend/InvenTree/InvenTree/models.py b/src/backend/InvenTree/InvenTree/models.py index de211a9d02..769ad0bdd5 100644 --- a/src/backend/InvenTree/InvenTree/models.py +++ b/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 - - 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() + self.__class__.rebuild_trees(trees) def handle_tree_delete(self, delete_children=False, delete_items=False): """Delete a single instance of the tree, based on provided kwargs. @@ -942,40 +932,89 @@ 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: - if tree_id: - self.partial_rebuild(tree_id) + # Flag to indicate that a tree rebuild task was triggered by this save + self._tree_rebuild_offloaded = False if len(trees) > 0: - # A tree update was performed, so we need to refresh the instance - try: - self.refresh_from_db() - except TransactionManagementError: - # If we are inside a transaction block, we cannot refresh from db - pass - except Exception as e: - # Any other error is unexpected - InvenTree.sentry.report_exception(e) - InvenTree.exceptions.log_error(f'{self.__class__.__name__}.save') + # 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 - def partial_rebuild(self, tree_id: int) -> bool: + if ran_sync: + # The tree was rebuilt synchronously, so refresh the instance + try: + self.refresh_from_db() + except TransactionManagementError: + # If we are inside a transaction block, we cannot refresh from db + pass + except Exception as e: + # Any other error is unexpected + InvenTree.sentry.report_exception(e) + InvenTree.exceptions.log_error(f'{self.__class__.__name__}.save') + + @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', - self.__class__.__name__, - self.pk, - e, + 'Failed to rebuild tree <%s> for %s: %s', tree_id, cls.__name__, 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 - lower_nodes = list( - self.get_descendants(include_self=False).values_list('pk', flat=True) - ) - - self.rebuild_lower_nodes(lower_nodes) + # Update the pathstring values for any lower nodes, + # by offloading the update to the background worker + if not rebuild_offloaded and self.get_descendant_count() > 0: + self.__class__.offload_tree_rebuild([self.tree_id]) 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( - self.get_descendants(include_self=False).values_list('pk', flat=True) - ) + lower_nodes = self.get_lower_nodes() # 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. diff --git a/src/backend/InvenTree/InvenTree/tasks.py b/src/backend/InvenTree/InvenTree/tasks.py index 0b868920b8..b876b15e38 100644 --- a/src/backend/InvenTree/InvenTree/tasks.py +++ b/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(): diff --git a/src/backend/InvenTree/part/test_api.py b/src/backend/InvenTree/part/test_api.py index e340b41e79..1a4b27c7d9 100644 --- a/src/backend/InvenTree/part/test_api.py +++ b/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') diff --git a/src/backend/InvenTree/stock/test_api.py b/src/backend/InvenTree/stock/test_api.py index 2cad462ff4..3fdadfa306 100644 --- a/src/backend/InvenTree/stock/test_api.py +++ b/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']: