diff --git a/base_requirements.txt b/base_requirements.txt index 673006aa466..da7695f5ad7 100644 --- a/base_requirements.txt +++ b/base_requirements.txt @@ -27,6 +27,10 @@ django-graphiql-debug-toolbar django-htmx # Modified Preorder Tree Traversal (recursive nesting of objects) +# Retained primarily for plugin backward compatibility: the deprecated +# NestedGroupModel base remains MPTT-backed for plugins still using it. Also +# required by historical migrations that pre-date the switch to PostgreSQL ltree. +# NetBox core runtime uses netbox.models.ltree.LtreeModel instead. django-mptt # Context managers for PostgreSQL advisory locks diff --git a/netbox/core/models/change_logging.py b/netbox/core/models/change_logging.py index 4409994da75..5a9c474fe50 100644 --- a/netbox/core/models/change_logging.py +++ b/netbox/core/models/change_logging.py @@ -6,7 +6,6 @@ from django.db import models from django.urls import reverse from django.utils.translation import gettext_lazy as _ -from mptt.models import MPTTModel from core.choices import ObjectChangeActionChoices from core.querysets import ObjectChangeQuerySet @@ -160,13 +159,26 @@ def diff_exclude_fields(self): model = self.changed_object_type.model_class() attrs = set() + # model_class() returns None when the model's app is no longer installed + # (e.g. a removed plugin); there are no fields to exclude, and the + # issubclass() checks below would raise TypeError on None. + if model is None: + return attrs + # Exclude auto-populated change tracking fields if issubclass(model, ChangeLoggingMixin): attrs.update({'created', 'last_updated'}) - # Exclude MPTT-internal fields + # Exclude trigger-maintained ltree columns (path and the optional sort_path) + from netbox.models.ltree import LtreeModel + if issubclass(model, LtreeModel): + attrs.update({'path', 'sort_path'}) + + # Exclude MPTT bookkeeping columns for the deprecated MPTT-backed + # NestedGroupModel still shipped for plugin compatibility. + from mptt.models import MPTTModel if issubclass(model, MPTTModel): - attrs.update({'level', 'lft', 'rght', 'tree_id'}) + attrs.update({'lft', 'rght', 'tree_id', 'level'}) return attrs diff --git a/netbox/dcim/api/views.py b/netbox/dcim/api/views.py index 3efa63acb07..cf29238d9d4 100644 --- a/netbox/dcim/api/views.py +++ b/netbox/dcim/api/views.py @@ -16,7 +16,7 @@ from netbox.api.authentication import IsAuthenticatedOrLoginNotRequired from netbox.api.metadata import ContentTypeMetadata from netbox.api.pagination import StripCountAnnotationsPaginator -from netbox.api.viewsets import MPTTLockedMixin, NetBoxModelViewSet, NetBoxReadOnlyModelViewSet +from netbox.api.viewsets import NetBoxModelViewSet, NetBoxReadOnlyModelViewSet from netbox.api.viewsets.mixins import SequentialBulkCreatesMixin from utilities.api import get_serializer_for_model from utilities.query import count_related @@ -95,7 +95,7 @@ def paths(self, request, pk): # Regions # -class RegionViewSet(MPTTLockedMixin, NetBoxModelViewSet): +class RegionViewSet(NetBoxModelViewSet): queryset = Region.objects.add_related_count( Region.objects.all(), Site, @@ -111,7 +111,7 @@ class RegionViewSet(MPTTLockedMixin, NetBoxModelViewSet): # Site groups # -class SiteGroupViewSet(MPTTLockedMixin, NetBoxModelViewSet): +class SiteGroupViewSet(NetBoxModelViewSet): queryset = SiteGroup.objects.add_related_count( SiteGroup.objects.all(), Site, @@ -137,7 +137,7 @@ class SiteViewSet(NetBoxModelViewSet): # Locations # -class LocationViewSet(MPTTLockedMixin, NetBoxModelViewSet): +class LocationViewSet(NetBoxModelViewSet): queryset = Location.objects.add_related_count( Location.objects.add_related_count( Location.objects.all(), @@ -356,7 +356,7 @@ class DeviceBayTemplateViewSet(NetBoxModelViewSet): filterset_class = filtersets.DeviceBayTemplateFilterSet -class InventoryItemTemplateViewSet(MPTTLockedMixin, NetBoxModelViewSet): +class InventoryItemTemplateViewSet(NetBoxModelViewSet): queryset = InventoryItemTemplate.objects.all() serializer_class = serializers.InventoryItemTemplateSerializer filterset_class = filtersets.InventoryItemTemplateFilterSet @@ -388,7 +388,7 @@ class DeviceRoleViewSet(NetBoxModelViewSet): # Platforms # -class PlatformViewSet(MPTTLockedMixin, NetBoxModelViewSet): +class PlatformViewSet(NetBoxModelViewSet): queryset = Platform.objects.add_related_count( Platform.objects.add_related_count( Platform.objects.all(), @@ -543,7 +543,7 @@ class DeviceBayViewSet(NetBoxModelViewSet): filterset_class = filtersets.DeviceBayFilterSet -class InventoryItemViewSet(MPTTLockedMixin, NetBoxModelViewSet): +class InventoryItemViewSet(NetBoxModelViewSet): queryset = InventoryItem.objects.all() serializer_class = serializers.InventoryItemSerializer filterset_class = filtersets.InventoryItemFilterSet diff --git a/netbox/dcim/graphql/types.py b/netbox/dcim/graphql/types.py index 50aa4cb2219..903296d03bd 100644 --- a/netbox/dcim/graphql/types.py +++ b/netbox/dcim/graphql/types.py @@ -12,7 +12,8 @@ from netbox.graphql.scalars import BigInt from netbox.graphql.types import ( BaseObjectType, - NestedGroupObjectType, + LtreeNodeMixin, + NestedLtreeGroupObjectType, NetBoxObjectType, OrganizationalObjectType, PrimaryObjectType, @@ -322,11 +323,11 @@ class DeviceBayTemplateType(ComponentTemplateType): @strawberry_django.type( models.InventoryItemTemplate, - exclude=['component_type', 'component_id', 'parent'], + exclude=['component_type', 'component_id', 'parent', 'path'], filters=InventoryItemTemplateFilter, pagination=True ) -class InventoryItemTemplateType(ComponentTemplateType): +class InventoryItemTemplateType(LtreeNodeMixin, ComponentTemplateType): role: Annotated['InventoryItemRoleType', strawberry.lazy('dcim.graphql.types')] | None manufacturer: Annotated['ManufacturerType', strawberry.lazy('dcim.graphql.types')] @@ -350,11 +351,11 @@ def parent(self) -> Annotated['InventoryItemTemplateType', strawberry.lazy('dcim @strawberry_django.type( models.DeviceRole, - fields='__all__', + exclude=['path', 'sort_path'], filters=DeviceRoleFilter, pagination=True ) -class DeviceRoleType(NestedGroupObjectType): +class DeviceRoleType(NestedLtreeGroupObjectType): parent: Annotated['DeviceRoleType', strawberry.lazy('dcim.graphql.types')] | None children: list[Annotated['DeviceRoleType', strawberry.lazy('dcim.graphql.types')]] color: str @@ -487,11 +488,11 @@ class InterfaceTemplateType(ModularComponentTemplateType): @strawberry_django.type( models.InventoryItem, - exclude=['component_type', 'component_id', 'parent'], + exclude=['component_type', 'component_id', 'parent', 'path'], filters=InventoryItemFilter, pagination=True ) -class InventoryItemType(ComponentType): +class InventoryItemType(LtreeNodeMixin, ComponentType): role: Annotated['InventoryItemRoleType', strawberry.lazy('dcim.graphql.types')] | None manufacturer: Annotated['ManufacturerType', strawberry.lazy('dcim.graphql.types')] | None @@ -529,11 +530,11 @@ class InventoryItemRoleType(OrganizationalObjectType): @strawberry_django.type( models.Location, # fields='__all__', - exclude=['parent'], # bug - temp + exclude=['parent', 'path', 'sort_path'], # bug - temp filters=LocationFilter, pagination=True ) -class LocationType(VLANGroupsMixin, ImageAttachmentsMixin, ContactsMixin, NestedGroupObjectType): +class LocationType(VLANGroupsMixin, ImageAttachmentsMixin, ContactsMixin, NestedLtreeGroupObjectType): site: Annotated["SiteType", strawberry.lazy('dcim.graphql.types')] tenant: Annotated["TenantType", strawberry.lazy('tenancy.graphql.types')] | None parent: Annotated["LocationType", strawberry.lazy('dcim.graphql.types')] | None @@ -602,11 +603,11 @@ class ModuleType(PrimaryObjectType): @strawberry_django.type( models.ModuleBay, # fields='__all__', - exclude=['parent'], + exclude=['parent', 'path', 'sort_path'], filters=ModuleBayFilter, pagination=True ) -class ModuleBayType(ModularComponentType): +class ModuleBayType(LtreeNodeMixin, ModularComponentType): installed_module: Annotated["ModuleType", strawberry.lazy('dcim.graphql.types')] | None children: list[Annotated["ModuleBayType", strawberry.lazy('dcim.graphql.types')]] @@ -667,11 +668,11 @@ class ModuleTypeType(PrimaryObjectType): @strawberry_django.type( models.Platform, - fields='__all__', + exclude=['path', 'sort_path'], filters=PlatformFilter, pagination=True ) -class PlatformType(NestedGroupObjectType): +class PlatformType(NestedLtreeGroupObjectType): parent: Annotated['PlatformType', strawberry.lazy('dcim.graphql.types')] | None children: list[Annotated['PlatformType', strawberry.lazy('dcim.graphql.types')]] manufacturer: Annotated["ManufacturerType", strawberry.lazy('dcim.graphql.types')] | None @@ -875,11 +876,11 @@ class RearPortTemplateType(ModularComponentTemplateType): @strawberry_django.type( models.Region, - exclude=['parent'], + exclude=['parent', 'path', 'sort_path'], filters=RegionFilter, pagination=True ) -class RegionType(VLANGroupsMixin, ContactsMixin, NestedGroupObjectType): +class RegionType(VLANGroupsMixin, ContactsMixin, NestedLtreeGroupObjectType): sites: list[Annotated["SiteType", strawberry.lazy('dcim.graphql.types')]] children: list[Annotated["RegionType", strawberry.lazy('dcim.graphql.types')]] @@ -952,11 +953,11 @@ def circuit_terminations(self) -> list[ @strawberry_django.type( models.SiteGroup, - exclude=['parent'], # bug - temp + exclude=['parent', 'path', 'sort_path'], # bug - temp filters=SiteGroupFilter, pagination=True ) -class SiteGroupType(VLANGroupsMixin, ContactsMixin, NestedGroupObjectType): +class SiteGroupType(VLANGroupsMixin, ContactsMixin, NestedLtreeGroupObjectType): sites: list[Annotated["SiteType", strawberry.lazy('dcim.graphql.types')]] children: list[Annotated["SiteGroupType", strawberry.lazy('dcim.graphql.types')]] diff --git a/netbox/dcim/migrations/0238_ltree_paths.py b/netbox/dcim/migrations/0238_ltree_paths.py new file mode 100644 index 00000000000..67c69e77476 --- /dev/null +++ b/netbox/dcim/migrations/0238_ltree_paths.py @@ -0,0 +1,283 @@ +""" +Replace django-mptt with PostgreSQL ltree for dcim's hierarchical models. + +For each of (Region, SiteGroup, Location, DeviceRole, Platform, ModuleBay, +InventoryItem, InventoryItemTemplate) this migration: + +1. Enables the PostgreSQL ltree extension (idempotent). +2. Adds a nullable `path` LTreeField. For models that previously had + `MPTTMeta.order_insertion_by = ('name',)` — Region, SiteGroup, Location, + DeviceRole, Platform, ModuleBay — also adds a `sort_path` text column. +3. Installs per-table BEFORE/AFTER triggers. For models with sort_path, the + trigger maintains both columns. +4. Populates path (and sort_path where applicable) for existing rows via a + single recursive CTE per table. +5. Tightens path to NOT NULL. +6. Drops the legacy MPTT columns (lft, rght, tree_id, level). +7. Adds a GiST index on path (descendant/ancestor lookups via `<@` / `@>`). + For sort_path models, also adds a btree index for ORDER BY listing. + +!!! OPERATOR WARNING !!! + +Step 4 runs ONE recursive-CTE UPDATE per table over every row, taking a +row-exclusive lock on the entire table for the duration of the statement. +On large deployments — particularly dcim_inventoryitem, which can contain +millions of rows — this can block writes for minutes. Plan a maintenance +window and budget accordingly. The other tables (region, sitegroup, +location, devicerole, platform, modulebay, inventoryitemtemplate) are +typically far smaller. + +Notes: +- The reverse migration is lossy: it re-adds the MPTT columns (lft/rght/tree_id/ + level) empty and does NOT rebuild the tree. Forward migration is the supported + direction. +""" +import django.db.models.deletion +from django.contrib.postgres.indexes import GistIndex +from django.contrib.postgres.operations import CreateExtension +from django.db import migrations, models + +import netbox.models.ltree +from utilities.ltree import InstallLtreeTriggers +from utilities.mptt_to_ltree import assert_paths_populated_sql, populate_paths_sql + +# All models getting an ltree `path` column. +ALL_MODELS = ( + 'region', 'sitegroup', 'location', 'devicerole', 'platform', + 'inventoryitem', 'inventoryitemtemplate', 'modulebay', +) + +ALL_TABLES = ( + 'dcim_region', + 'dcim_sitegroup', + 'dcim_location', + 'dcim_devicerole', + 'dcim_platform', + 'dcim_inventoryitem', + 'dcim_inventoryitemtemplate', + 'dcim_modulebay', +) + +# Subset that previously declared `MPTTMeta.order_insertion_by = ('name',)` and +# therefore needs a `sort_path` text column maintained alongside `path`. +SORT_MODELS = ('region', 'sitegroup', 'location', 'devicerole', 'platform', 'modulebay') + +SORT_TABLES = ( + 'dcim_region', + 'dcim_sitegroup', + 'dcim_location', + 'dcim_devicerole', + 'dcim_platform', + 'dcim_modulebay', +) + +LEGACY_FIELDS = ('lft', 'rght', 'tree_id', 'level') + + +class Migration(migrations.Migration): + + dependencies = [ + ('dcim', '0237_module_remove_local_context_data'), + ] + + operations = [ + # Enable the ltree extension first so the migration fails fast if it is missing. + CreateExtension('ltree'), + + # Switch parent from mptt.fields.TreeForeignKey to django.db.models.ForeignKey + # (no-op at the SQL level; reconciles migration state with model definitions). + migrations.AlterField( + model_name='devicerole', name='parent', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, + related_name='children', to='dcim.devicerole'), + ), + migrations.AlterField( + model_name='inventoryitem', name='parent', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, + related_name='child_items', to='dcim.inventoryitem'), + ), + migrations.AlterField( + model_name='inventoryitemtemplate', name='parent', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, + related_name='child_items', to='dcim.inventoryitemtemplate'), + ), + migrations.AlterField( + model_name='location', name='parent', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, + related_name='children', to='dcim.location'), + ), + migrations.AlterField( + model_name='modulebay', name='parent', + field=models.ForeignKey(blank=True, editable=False, null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name='children', to='dcim.modulebay'), + ), + migrations.AlterField( + model_name='platform', name='parent', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, + related_name='children', to='dcim.platform'), + ), + migrations.AlterField( + model_name='region', name='parent', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, + related_name='children', to='dcim.region'), + ), + migrations.AlterField( + model_name='sitegroup', name='parent', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, + related_name='children', to='dcim.sitegroup'), + ), + + # 2. Add nullable path column on all tree models. + *[ + migrations.AddField( + model_name=m, name='path', + field=netbox.models.ltree.LtreeField(blank=True, editable=False, null=True), + ) + for m in ALL_MODELS + ], + # 2b. Add sort_path column (with default '') on the 6 models with order_insertion_by. + # ModuleBay's `name` uses the natural_sort collation, so its sort_path must + # match it (Slot 0..Slot 13, not lexicographic Slot 0, 1, 10, 2...). The other + # five use a plain-collation `name`, so their sort_path stays plain. + *[ + migrations.AddField( + model_name=m, name='sort_path', + field=models.TextField(blank=True, default='', editable=False), + ) + for m in SORT_MODELS if m != 'modulebay' + ], + migrations.AddField( + model_name='modulebay', name='sort_path', + field=models.TextField(blank=True, default='', editable=False, db_collation='natural_sort'), + ), + + # 3. Install path-maintenance triggers. Models with sort_path get triggers + # that maintain both columns; the other two get path-only triggers. + *[InstallLtreeTriggers(t, name_column='name') for t in SORT_TABLES], + InstallLtreeTriggers('dcim_inventoryitem'), + InstallLtreeTriggers('dcim_inventoryitemtemplate'), + + # 4. Populate existing rows via per-table recursive CTE (sort_path only + # for the models that carry it). + migrations.RunSQL( + '\n'.join(populate_paths_sql(t, sort_path=t in SORT_TABLES) for t in ALL_TABLES), + reverse_sql=migrations.RunSQL.noop, + ), + + # 4b. Fail fast (with a useful message) if any row still has NULL path — + # otherwise the AlterField below aborts opaquely inside ALTER COLUMN. + migrations.RunSQL( + '\n'.join(assert_paths_populated_sql(t) for t in ALL_TABLES), + reverse_sql=migrations.RunSQL.noop, + ), + + # 5. Tighten path to NOT NULL with empty-string default. + *[ + migrations.AlterField( + model_name=m, name='path', + field=netbox.models.ltree.LtreeField(blank=True, default='', editable=False), + ) + for m in ALL_MODELS + ], + + # 6. Update Meta.ordering on the SORT_MODELS to reflect sort_path-based ordering. + migrations.AlterModelOptions( + name='devicerole', options={'ordering': ('sort_path',)}, + ), + # InventoryItem has no sort_path; its global list is flat + alphabetical + # (the per-device tab renders the hierarchy via get_children().order_by('path')). + migrations.AlterModelOptions( + name='inventoryitem', options={'ordering': ('name', 'pk')}, + ), + migrations.AlterModelOptions( + name='location', options={'ordering': ('site', 'sort_path')}, + ), + migrations.AlterModelOptions( + name='modulebay', options={'ordering': ('sort_path', 'pk')}, + ), + migrations.AlterModelOptions( + name='platform', options={'ordering': ('sort_path',)}, + ), + migrations.AlterModelOptions( + name='region', options={'ordering': ('sort_path',)}, + ), + migrations.AlterModelOptions( + name='sitegroup', options={'ordering': ('sort_path',)}, + ), + + # 7. Drop legacy (tree_id, lft) indexes added in 0226_add_mptt_tree_indexes, + # then drop the legacy MPTT columns. + migrations.RemoveIndex(model_name='devicerole', name='dcim_devicerole_tree_id_lfbf11'), + migrations.RemoveIndex(model_name='inventoryitem', name='dcim_inventoryitem_tree_id975c'), + migrations.RemoveIndex(model_name='inventoryitemtemplate', name='dcim_inventoryitemtemplatedee0'), + migrations.RemoveIndex(model_name='location', name='dcim_location_tree_id_lft_idx'), + migrations.RemoveIndex(model_name='modulebay', name='dcim_modulebay_tree_id_lft_idx'), + migrations.RemoveIndex(model_name='platform', name='dcim_platform_tree_id_lft_idx'), + migrations.RemoveIndex(model_name='region', name='dcim_region_tree_id_lft_idx'), + migrations.RemoveIndex(model_name='sitegroup', name='dcim_sitegroup_tree_id_lft_idx'), + *[ + migrations.RemoveField(model_name=m, name=f) + for m in ALL_MODELS for f in LEGACY_FIELDS + ], + + # 8. Add GiST indexes on path (descendant/ancestor containment). + migrations.AddIndex( + model_name='region', + index=GistIndex(fields=['path'], name='dcim_region_path_gist'), + ), + migrations.AddIndex( + model_name='sitegroup', + index=GistIndex(fields=['path'], name='dcim_sitegroup_path_gist'), + ), + migrations.AddIndex( + model_name='location', + index=GistIndex(fields=['path'], name='dcim_location_path_gist'), + ), + migrations.AddIndex( + model_name='devicerole', + index=GistIndex(fields=['path'], name='dcim_devicerole_path_gist'), + ), + migrations.AddIndex( + model_name='platform', + index=GistIndex(fields=['path'], name='dcim_platform_path_gist'), + ), + migrations.AddIndex( + model_name='inventoryitem', + index=GistIndex(fields=['path'], name='dcim_inventoryitem_path_gist'), + ), + migrations.AddIndex( + model_name='inventoryitemtemplate', + index=GistIndex(fields=['path'], name='dcim_inv_item_tmpl_path_gist'), + ), + migrations.AddIndex( + model_name='modulebay', + index=GistIndex(fields=['path'], name='dcim_modulebay_path_gist'), + ), + + # 9. Add btree indexes on sort_path (tree-flatten ORDER BY listing). + migrations.AddIndex( + model_name='region', + index=models.Index(fields=['sort_path'], name='dcim_region_sort_path_idx'), + ), + migrations.AddIndex( + model_name='sitegroup', + index=models.Index(fields=['sort_path'], name='dcim_sitegroup_sort_path_idx'), + ), + migrations.AddIndex( + model_name='location', + index=models.Index(fields=['sort_path'], name='dcim_location_sort_path_idx'), + ), + migrations.AddIndex( + model_name='devicerole', + index=models.Index(fields=['sort_path'], name='dcim_devicerole_sort_path_idx'), + ), + migrations.AddIndex( + model_name='platform', + index=models.Index(fields=['sort_path'], name='dcim_platform_sort_path_idx'), + ), + migrations.AddIndex( + model_name='modulebay', + index=models.Index(fields=['sort_path'], name='dcim_modulebay_sort_path_idx'), + ), + ] diff --git a/netbox/dcim/models/device_component_templates.py b/netbox/dcim/models/device_component_templates.py index dec8758a9c7..728bd15a976 100644 --- a/netbox/dcim/models/device_component_templates.py +++ b/netbox/dcim/models/device_component_templates.py @@ -1,9 +1,9 @@ from django.contrib.contenttypes.fields import GenericForeignKey +from django.contrib.postgres.indexes import GistIndex from django.core.exceptions import ValidationError from django.core.validators import MaxValueValidator, MinValueValidator from django.db import models from django.utils.translation import gettext_lazy as _ -from mptt.models import MPTTModel, TreeForeignKey from dcim.choices import * from dcim.constants import * @@ -11,8 +11,8 @@ from dcim.models.mixins import InterfaceValidationMixin from dcim.utils import get_module_bay_positions, resolve_module_placeholder from netbox.models import ChangeLoggedModel +from netbox.models.ltree import LtreeManager, LtreeModel from utilities.fields import ColorField, NaturalOrderingField -from utilities.mptt import TreeManager from utilities.ordering import naturalize_interface from utilities.tracking import TrackingModelMixin from wireless.choices import WirelessRoleChoices @@ -752,11 +752,18 @@ class Meta(ModularComponentTemplateModel.Meta): verbose_name_plural = _('module bay templates') def instantiate(self, **kwargs): + module = kwargs.get('module') return self.component_model( - name=self.resolve_name(kwargs.get('module'), kwargs.get('device')), - label=self.resolve_label(kwargs.get('module'), kwargs.get('device')), - position=self.resolve_position(kwargs.get('module'), kwargs.get('device')), + name=self.resolve_name(module, kwargs.get('device')), + label=self.resolve_label(module, kwargs.get('device')), + position=self.resolve_position(module, kwargs.get('device')), enabled=self.enabled, + # A module bay created for an installed module nests under that module's + # bay. bulk_create() bypasses ModuleBay.save() (which would otherwise set + # this), so the parent must be assigned here for the path trigger to nest + # it correctly. Device-level bays are instantiated without a module and + # remain roots (parent=None). + parent=module.module_bay if module else None, **kwargs ) instantiate.do_not_call_in_templates = True @@ -812,11 +819,11 @@ def to_yaml(self): } -class InventoryItemTemplate(MPTTModel, ComponentTemplateModel): +class InventoryItemTemplate(LtreeModel, ComponentTemplateModel): """ A template for an InventoryItem to be created for a new parent Device. """ - parent = TreeForeignKey( + parent = models.ForeignKey( to='self', on_delete=models.CASCADE, related_name='child_items', @@ -860,13 +867,14 @@ class InventoryItemTemplate(MPTTModel, ComponentTemplateModel): help_text=_('Manufacturer-assigned part identifier') ) - objects = TreeManager() + objects = LtreeManager() component_model = InventoryItem class Meta: ordering = ('device_type__id', 'parent__id', 'name') indexes = ( models.Index(fields=('component_type', 'component_id')), + GistIndex(fields=['path'], name='dcim_inv_item_tmpl_path_gist'), ) constraints = ( models.UniqueConstraint( diff --git a/netbox/dcim/models/device_components.py b/netbox/dcim/models/device_components.py index cf80b94c4d2..ce2e376bf24 100644 --- a/netbox/dcim/models/device_components.py +++ b/netbox/dcim/models/device_components.py @@ -2,11 +2,11 @@ from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation from django.contrib.postgres.fields import ArrayField +from django.contrib.postgres.indexes import GistIndex from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.core.validators import MaxValueValidator, MinValueValidator from django.db import models from django.utils.translation import gettext_lazy as _ -from mptt.models import MPTTModel, TreeForeignKey from dcim.choices import * from dcim.constants import * @@ -15,9 +15,9 @@ from dcim.models.mixins import InterfaceValidationMixin from netbox.choices import ColorChoices from netbox.models import NetBoxModel, OrganizationalModel +from netbox.models.ltree import LtreeManager, LtreeModel, SortPathField from netbox.models.mixins import OwnerMixin from utilities.fields import ColorField, NaturalOrderingField -from utilities.mptt import TreeManager from utilities.ordering import naturalize_interface from utilities.query_functions import CollateAsChar from utilities.tracking import TrackingModelMixin @@ -1313,34 +1313,11 @@ def clean(self): # Bays # -class ModuleBayManager(TreeManager): - """ - Order ModuleBays by the natural-sort name of each tree's root, then by MPTT - left value within the tree. Combined with the root-insert bypass in - ModuleBay.save(), this lets us keep MPTTMeta.order_insertion_by for cheap - intra-tree sibling placement while skipping the global tree_id renumbering - it would otherwise perform on every root insert. - """ - def get_queryset(self, *args, **kwargs): - # Use the raw manager to avoid recursing through this get_queryset() when - # building the annotation subquery. - root_name = self.model._objects_raw.filter( - tree_id=models.OuterRef('tree_id'), - level=0, - ).values('name')[:1] - return super().get_queryset(*args, **kwargs).annotate( - _root_name=models.Subquery( - root_name, - output_field=models.CharField(db_collation='natural_sort'), - ) - ).order_by('_root_name', 'lft') - - -class ModuleBay(ModularComponentModel, TrackingModelMixin, MPTTModel): +class ModuleBay(ModularComponentModel, TrackingModelMixin, LtreeModel): """ An empty space within a Device which can house a child device """ - parent = TreeForeignKey( + parent = models.ForeignKey( to='self', on_delete=models.CASCADE, related_name='children', @@ -1359,18 +1336,31 @@ class ModuleBay(ModularComponentModel, TrackingModelMixin, MPTTModel): verbose_name=_('enabled'), default=True, ) - - objects = ModuleBayManager() - # Plain TreeManager used by ModuleBayManager to build the _root_name subquery - # without recursing through our annotated get_queryset(). - _objects_raw = TreeManager() + # sort_path inherits `name`'s natural_sort collation automatically (LtreeModelBase), + # so ORDER BY sort_path sorts siblings naturally (Slot 0..Slot 13) — as MPTT's + # order_insertion_by=('name',) did — rather than lexicographically. + sort_path = SortPathField( + editable=False, + blank=True, + default='', + ) clone_fields = ('device', 'enabled') + objects = LtreeManager() + class Meta(ModularComponentModel.Meta): - # Empty tuple triggers Django migration detection for MPTT indexes - # (see #21016, django-mptt/django-mptt#682) - indexes = () + # Order by sort_path alone (not device-first), reproducing the MPTT + # ModuleBayManager's ('_root_name', 'lft'): sort_path begins with the tree's + # root-bay name (natural_sort collation), so the global list groups by + # root-bay name across devices, descendants following their root. `pk` + # gives a deterministic tie-break among same-named roots on different devices + # (MPTT's lft=1 left this order arbitrary). + ordering = ('sort_path', 'pk') + indexes = ( + GistIndex(fields=['path'], name='dcim_modulebay_path_gist'), + models.Index(fields=['sort_path'], name='dcim_modulebay_sort_path_idx'), + ) constraints = ( models.UniqueConstraint( fields=('device', 'module', 'name'), @@ -1380,12 +1370,6 @@ class Meta(ModularComponentModel.Meta): verbose_name = _('module bay') verbose_name_plural = _('module bays') - class MPTTMeta: - # Used for placing siblings within a single tree at insert time. Costs - # are bounded to that tree's rows. Cross-tree (root) insertion is - # handled in save() to avoid the O(N) tree_id shift this would trigger. - order_insertion_by = ('name',) - def clean(self): super().clean() @@ -1405,38 +1389,14 @@ def save(self, *args, **kwargs): self.parent = self.module.module_bay else: self.parent = None - opts = self._mptt_meta - # For new root nodes, allocate the next tree_id and pre-set MPTT fields - # so super().save() skips MPTT's order_insertion_by-driven insertion - # path. That path would otherwise UPDATE every later tree_id on each - # root insert (NB-2800). Children still go through MPTT, which keeps - # siblings in name order via the same order_insertion_by setting. - if self._state.adding and self.parent_id is None and not self.lft and not self.rght: - max_tree_id = ModuleBay._objects_raw.aggregate( - models.Max('tree_id') - )['tree_id__max'] or 0 - self.tree_id = max_tree_id + 1 - self.lft = 1 - self.rght = 2 - self.level = 0 - elif ( - not self._state.adding - and self.parent_id is None - and self._mptt_cached_fields.get(opts.parent_attr) is None - ): - # Existing root being updated. Spoof the cached order_insertion_by - # values so MPTT's same_order check passes and it skips its reorder - # path, which would UPDATE every later tree_id on each root rename. - # ModuleBayManager._root_name recovers display order at query time, - # so the tree_id reshuffling would be wasted work. Child renames - # and root<->child transitions intentionally fall through to MPTT. - for field_name in opts.order_insertion_by: - field_name = field_name.lstrip('-') - self._mptt_cached_fields[field_name] = opts.get_raw_field_value( - self, field_name - ) super().save(*args, **kwargs) + def _parent_creates_cycle(self): + # A ModuleBay's parent is system-derived from its module (see save()), not + # user-assigned, and module/bay recursion is validated in clean(); skip the + # generic ltree cycle guard. + return False + @property def _occupied(self): """ @@ -1528,12 +1488,12 @@ class Meta: verbose_name_plural = _('inventory item roles') -class InventoryItem(MPTTModel, ComponentModel, TrackingModelMixin): +class InventoryItem(LtreeModel, ComponentModel, TrackingModelMixin): """ An InventoryItem represents a serialized piece of hardware within a Device, such as a line card or power supply. InventoryItems are used only for inventory purposes. """ - parent = TreeForeignKey( + parent = models.ForeignKey( to='self', on_delete=models.CASCADE, related_name='child_items', @@ -1601,14 +1561,19 @@ class InventoryItem(MPTTModel, ComponentModel, TrackingModelMixin): help_text=_('This item was automatically discovered') ) - objects = TreeManager() - clone_fields = ('device', 'parent', 'role', 'manufacturer', 'status', 'part_id') + objects = LtreeManager() + class Meta: - ordering = ('device__id', 'parent__id', 'name') + # Global list is flat + alphabetical by name (natural_sort collation). The + # per-device Inventory tab renders the hierarchy instead — DeviceInventoryView + # .get_children() orders that by `path`. `pk` is a deterministic tie-break for + # same-named items on different devices. + ordering = ('name', 'pk') indexes = ( models.Index(fields=('component_type', 'component_id')), + GistIndex(fields=['path'], name='dcim_inventoryitem_path_gist'), ) constraints = ( models.UniqueConstraint( @@ -1622,12 +1587,6 @@ class Meta: def clean(self): super().clean() - # An InventoryItem cannot be its own parent - if self.pk and self.parent_id == self.pk: - raise ValidationError({ - "parent": _("Cannot assign self as parent.") - }) - # Validation for moving InventoryItems if not self._state.adding: # Cannot move an InventoryItem to another device if it has a parent diff --git a/netbox/dcim/models/devices.py b/netbox/dcim/models/devices.py index 88d094b978b..9e4db98134b 100644 --- a/netbox/dcim/models/devices.py +++ b/netbox/dcim/models/devices.py @@ -4,6 +4,7 @@ import yaml from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation from django.contrib.contenttypes.models import ContentType +from django.contrib.postgres.indexes import GistIndex from django.core.exceptions import ValidationError from django.core.files.storage import default_storage from django.core.validators import MaxValueValidator, MinValueValidator @@ -23,7 +24,7 @@ from extras.querysets import ConfigContextModelQuerySet from netbox.choices import ColorChoices from netbox.config import ConfigItem -from netbox.models import NestedGroupModel, OrganizationalModel, PrimaryModel +from netbox.models import NestedLtreeGroupModel, OrganizationalModel, PrimaryModel from netbox.models.features import ContactsMixin, ImageAttachmentsMixin from netbox.models.mixins import WeightMixin from utilities.exceptions import AbortRequest @@ -385,7 +386,7 @@ def is_child_device(self): # Devices # -class DeviceRole(NestedGroupModel): +class DeviceRole(NestedLtreeGroupModel): """ Devices are organized by functional role; for example, "Core Switch" or "File Server". Each DeviceRole is assigned a color to be used when displaying rack elevations. The vm_role field determines whether the role is applicable to @@ -411,10 +412,11 @@ class DeviceRole(NestedGroupModel): clone_fields = ('parent', 'description') class Meta: - ordering = ('name',) - # Empty tuple triggers Django migration detection for MPTT indexes - # (see #21016, django-mptt/django-mptt#682) - indexes = () + ordering = ('sort_path',) + indexes = ( + GistIndex(fields=['path'], name='dcim_devicerole_path_gist'), + models.Index(fields=['sort_path'], name='dcim_devicerole_sort_path_idx'), + ) constraints = ( models.UniqueConstraint( fields=('parent', 'name'), @@ -441,7 +443,7 @@ class Meta: verbose_name_plural = _('device roles') -class Platform(NestedGroupModel): +class Platform(NestedLtreeGroupModel): """ Platform refers to the software or firmware running on a Device. For example, "Cisco IOS-XR" or "Juniper Junos". A Platform may optionally be associated with a particular Manufacturer. @@ -465,10 +467,11 @@ class Platform(NestedGroupModel): clone_fields = ('parent', 'description') class Meta: - ordering = ('name',) - # Empty tuple triggers Django migration detection for MPTT indexes - # (see #21016, django-mptt/django-mptt#682) - indexes = () + ordering = ('sort_path',) + indexes = ( + GistIndex(fields=['path'], name='dcim_platform_path_gist'), + models.Index(fields=['sort_path'], name='dcim_platform_sort_path_idx'), + ) verbose_name = _('platform') verbose_name_plural = _('platforms') constraints = ( diff --git a/netbox/dcim/models/modules.py b/netbox/dcim/models/modules.py index e6baa34174f..100293bd622 100644 --- a/netbox/dcim/models/modules.py +++ b/netbox/dcim/models/modules.py @@ -5,7 +5,6 @@ from django.db.models.signals import post_save from django.utils.translation import gettext_lazy as _ from jsonschema.exceptions import ValidationError as JSONValidationError -from mptt.models import MPTTModel from dcim.choices import * from dcim.utils import create_port_mappings, update_interface_bridges @@ -386,29 +385,32 @@ def save(self, *args, **kwargs): component._location = self.device.location component._rack = self.device.rack - # we handle create and update separately - this is for create - if not issubclass(component_model, MPTTModel): - component_model.objects.bulk_create(create_instances) - # Emit the post_save signal for each newly created object - for component in create_instances: - post_save.send( - sender=component_model, - instance=component, - created=True, - raw=False, - using='default', - update_fields=None - ) - else: - # MPTT models must be saved individually to maintain tree structure - for instance in create_instances: - instance.save() + # Bulk-create new instances. ModuleBay is ltree-backed: its parent is set + # in ModuleBayTemplate.instantiate() (bulk_create bypasses ModuleBay.save()), + # and the BEFORE INSERT trigger derives path/sort_path from parent_id per row. + component_model.objects.bulk_create(create_instances) + for component in create_instances: + post_save.send( + sender=component_model, + instance=component, + created=True, + raw=False, + using='default', + update_fields=None + ) update_fields = ['module'] + # ModuleBay.parent is derived from .module in ModuleBay.save(), and the + # path/sort_path trigger only fires on parent_id/name changes. A bare + # bulk_update(['module']) bypasses both, leaving the adopted bay rooted + # at its pre-adoption location. Set parent in lockstep so the BEFORE + # trigger recomputes path/sort_path. + if component_model is ModuleBay: + for instance in update_instances: + instance.parent = self.module_bay + update_fields = ['module', 'parent'] - # we handle create and update separately - this is for update component_model.objects.bulk_update(update_instances, update_fields) - # Emit the post_save signal for each updated object for component in update_instances: post_save.send( sender=component_model, @@ -419,10 +421,6 @@ def save(self, *args, **kwargs): update_fields=update_fields ) - # Rebuild MPTT tree if needed (bulk_update bypasses model save) - if issubclass(component_model, MPTTModel) and update_instances: - component_model.objects.rebuild() - # Replicate any front/rear port mappings from the ModuleType create_port_mappings(self.device, self.module_type, self) diff --git a/netbox/dcim/models/sites.py b/netbox/dcim/models/sites.py index 00bc9a240a3..16877d6da73 100644 --- a/netbox/dcim/models/sites.py +++ b/netbox/dcim/models/sites.py @@ -1,6 +1,7 @@ import decimal from django.contrib.contenttypes.fields import GenericRelation +from django.contrib.postgres.indexes import GistIndex from django.core.exceptions import ValidationError from django.core.validators import MaxValueValidator, MinValueValidator from django.db import models @@ -9,7 +10,7 @@ from dcim.choices import * from dcim.constants import * -from netbox.models import NestedGroupModel, PrimaryModel +from netbox.models import NestedLtreeGroupModel, PrimaryModel from netbox.models.features import ContactsMixin, ImageAttachmentsMixin __all__ = ( @@ -24,7 +25,7 @@ # Regions # -class Region(ContactsMixin, NestedGroupModel): +class Region(ContactsMixin, NestedLtreeGroupModel): """ A region represents a geographic collection of sites. For example, you might create regions representing countries, states, and/or cities. Regions are recursively nested into a hierarchy: all sites belonging to a child region are @@ -44,9 +45,11 @@ class Region(ContactsMixin, NestedGroupModel): ) class Meta: - # Empty tuple triggers Django migration detection for MPTT indexes - # (see #21016, django-mptt/django-mptt#682) - indexes = () + ordering = ('sort_path',) + indexes = ( + GistIndex(fields=['path'], name='dcim_region_path_gist'), + models.Index(fields=['sort_path'], name='dcim_region_sort_path_idx'), + ) constraints = ( models.UniqueConstraint( fields=('parent', 'name'), @@ -83,7 +86,7 @@ def get_site_count(self): # Site groups # -class SiteGroup(ContactsMixin, NestedGroupModel): +class SiteGroup(ContactsMixin, NestedLtreeGroupModel): """ A site group is an arbitrary grouping of sites. For example, you might have corporate sites and customer sites; and within corporate sites you might distinguish between offices and data centers. Like regions, site groups can be @@ -103,9 +106,11 @@ class SiteGroup(ContactsMixin, NestedGroupModel): ) class Meta: - # Empty tuple triggers Django migration detection for MPTT indexes - # (see #21016, django-mptt/django-mptt#682) - indexes = () + ordering = ('sort_path',) + indexes = ( + GistIndex(fields=['path'], name='dcim_sitegroup_path_gist'), + models.Index(fields=['sort_path'], name='dcim_sitegroup_sort_path_idx'), + ) constraints = ( models.UniqueConstraint( fields=('parent', 'name'), @@ -273,7 +278,7 @@ def get_status_color(self): # Locations # -class Location(ContactsMixin, ImageAttachmentsMixin, NestedGroupModel): +class Location(ContactsMixin, ImageAttachmentsMixin, NestedLtreeGroupModel): """ A Location represents a subgroup of Racks and/or Devices within a Site. A Location may represent a building within a site, or a room within a building, for example. @@ -323,10 +328,16 @@ class Location(ContactsMixin, ImageAttachmentsMixin, NestedGroupModel): ) class Meta: - ordering = ['site', 'name'] - # Empty tuple triggers Django migration detection for MPTT indexes - # (see #21016, django-mptt/django-mptt#682) - indexes = () + # Group by site, then tree-flatten within each site. This mirrors the prior + # MPTT behavior (Meta.ordering = ['site', 'name']) while upgrading the + # within-site ordering to sort_path, so descendants follow their parent in + # name order. sort_path is unique within a site (root names are unique per + # site, child names unique per parent), so no further tie-break is needed. + ordering = ('site', 'sort_path') + indexes = ( + GistIndex(fields=['path'], name='dcim_location_path_gist'), + models.Index(fields=['sort_path'], name='dcim_location_sort_path_idx'), + ) constraints = ( models.UniqueConstraint( fields=('site', 'parent', 'name'), diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index 260615b4ea3..aa7c43b15a1 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -1057,18 +1057,17 @@ def test_child_module_bay_ordering(self): self.assertEqual(names, ['Bay 1', 'Bay 1.1', 'Bay 1.2', 'Bay 1.3']) @tag('regression') # #22146 - def test_root_module_bay_rename_preserves_tree_ids(self): + def test_root_module_bay_rename_preserves_paths(self): """ - Renaming a root module bay must not renumber any other root tree's - tree_id. The renamed bay's own tree_id is also expected to remain - stable, but the load-bearing assertion is that the *other* bays are - not shifted. + Renaming a root module bay must not rewrite any tree's path. Renaming + touches only sort_path (the display-ordering column), so every bay's + path — including the renamed bay's own — must be unchanged afterward. """ device_type = DeviceType.objects.first() device_role = DeviceRole.objects.first() site = Site.objects.first() device = Device.objects.create( - name='Rename TreeID Device', + name='Rename Path Device', device_type=device_type, role=device_role, site=site, @@ -1076,8 +1075,8 @@ def test_root_module_bay_rename_preserves_tree_ids(self): for name in ('Bay 1', 'Bay 2', 'Bay 3', 'Bay 4'): ModuleBay.objects.create(device=device, name=name) - tree_ids_before = { - bay.name: bay.tree_id + paths_before = { + bay.pk: str(bay.path) for bay in ModuleBay.objects.filter(device=device) } @@ -1085,18 +1084,16 @@ def test_root_module_bay_rename_preserves_tree_ids(self): bay.name = 'Bay 99' bay.save() - tree_ids_after = { - bay.name: bay.tree_id + paths_after = { + bay.pk: str(bay.path) for bay in ModuleBay.objects.filter(device=device) } - for name in ('Bay 1', 'Bay 3', 'Bay 4'): - self.assertEqual(tree_ids_after[name], tree_ids_before[name]) - self.assertEqual(tree_ids_after['Bay 99'], tree_ids_before['Bay 2']) + self.assertEqual(paths_after, paths_before) @tag('regression') # #22146 def test_root_module_bay_rename_updates_display_order(self): """ - Even though renaming a root module bay does not renumber tree_ids, + Even though renaming a root module bay does not rewrite its path, the manager's _root_name annotation must reflect the new name so the display ordering is correct. """ @@ -1186,7 +1183,9 @@ def test_root_to_child_transition_still_relocates(self): movable_bay.refresh_from_db() host_bay.refresh_from_db() self.assertEqual(movable_bay.parent_id, host_bay.pk) - self.assertEqual(movable_bay.tree_id, host_bay.tree_id) + # The trigger cascade must have re-rooted the moved bay into host_bay's + # tree: its path is now a strict descendant of host_bay's path. + self.assertTrue(str(movable_bay.path).startswith(f'{host_bay.path}.')) def test_single_module_token(self): device_type = DeviceType.objects.first() @@ -1265,6 +1264,38 @@ def test_nested_module_bay_label_resolution(self): nested_bay = module.modulebays.get(name='SFP A-21') self.assertEqual(nested_bay.label, 'A-21') + @tag('regression') # #21418 + def test_module_install_nests_module_bay_parent(self): + """ + A module bay instantiated when a module is installed must be nested under the + installing module's bay. bulk_create() bypasses ModuleBay.save(), so the parent + is assigned in ModuleBayTemplate.instantiate(); without it the bay would be left + a root with a top-level ltree path. + """ + manufacturer = Manufacturer.objects.first() + site = Site.objects.first() + device_role = DeviceRole.objects.first() + + device_type = DeviceType.objects.create( + manufacturer=manufacturer, model='Chassis with Bay', slug='chassis-with-bay' + ) + ModuleBayTemplate.objects.create(device_type=device_type, name='Bay A') + + module_type = ModuleType.objects.create(manufacturer=manufacturer, model='Module with Sub-bay') + ModuleBayTemplate.objects.create(module_type=module_type, name='Sub-bay 1') + + device = Device.objects.create( + name='Nested Bay Parent Device', device_type=device_type, role=device_role, site=site + ) + parent_bay = device.modulebays.get(name='Bay A') + module = Module.objects.create(device=device, module_bay=parent_bay, module_type=module_type) + + nested_bay = module.modulebays.get(name='Sub-bay 1') + self.assertEqual(nested_bay.parent, parent_bay) + # The ltree path/level must reflect the nesting, not a root placement. + self.assertEqual(nested_bay.level, parent_bay.level + 1) + self.assertTrue(str(nested_bay.path).startswith(f'{parent_bay.path}.')) + @tag('regression') # #20467 def test_nested_module_bay_position_resolution(self): """Test that {module} in a module bay template's position field is resolved when the module is installed.""" @@ -2529,3 +2560,65 @@ def test_three_phase_per_leg_recursive_aggregation(self): self.assertEqual(legs_by_name['A']['maximum'], 200) self.assertEqual(legs_by_name['B']['allocated'], 0) self.assertEqual(legs_by_name['C']['allocated'], 0) + + +class InventoryItemCycleTestCase(TestCase): + """ + InventoryItem (ltree-backed, not the nested-group base) must reject assigning + self or a descendant as parent — behavior django-mptt previously enforced via + InvalidMove on save(). + """ + @classmethod + def setUpTestData(cls): + site = Site.objects.create(name='Site 1', slug='inv-site-1') + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='inv-mfr-1') + device_type = DeviceType.objects.create( + manufacturer=manufacturer, model='Device Type 1', slug='inv-dt-1' + ) + role = DeviceRole.objects.create(name='Role 1', slug='inv-role-1') + cls.device = Device.objects.create( + name='Device 1', device_type=device_type, role=role, site=site + ) + + def test_cannot_assign_descendant_as_parent(self): + a = InventoryItem.objects.create(device=self.device, name='A') + b = InventoryItem.objects.create(device=self.device, name='B', parent=a) + c = InventoryItem.objects.create(device=self.device, name='C', parent=b) + a.parent = c + with self.assertRaises(ValidationError): + a.full_clean() + # The save()-level guard also rejects the cycle when clean() is bypassed. + with self.assertRaises(ValidationError): + a.save() + + def test_cannot_assign_self_as_parent(self): + a = InventoryItem.objects.create(device=self.device, name='A') + a.parent = a + with self.assertRaises(ValidationError): + a.full_clean() + + +class InventoryItemTemplateCycleTestCase(TestCase): + """InventoryItemTemplate must likewise reject self/descendant as parent.""" + + @classmethod + def setUpTestData(cls): + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='iit-mfr-1') + cls.device_type = DeviceType.objects.create( + manufacturer=manufacturer, model='Device Type 1', slug='iit-dt-1' + ) + + def test_cannot_assign_descendant_as_parent(self): + a = InventoryItemTemplate.objects.create(device_type=self.device_type, name='A') + b = InventoryItemTemplate.objects.create(device_type=self.device_type, name='B', parent=a) + a.parent = b + with self.assertRaises(ValidationError): + a.full_clean() + with self.assertRaises(ValidationError): + a.save() + + def test_cannot_assign_self_as_parent(self): + a = InventoryItemTemplate.objects.create(device_type=self.device_type, name='A') + a.parent = a + with self.assertRaises(ValidationError): + a.full_clean() diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index cc888597d75..f463fe4e989 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -2849,6 +2849,14 @@ class DeviceInventoryView(DeviceComponentsView): hide_if_empty=True ) + def get_children(self, request, parent): + # DeviceInventoryItemTable indents rows by record.level; under MPTT, + # TreeManager forced tree-flatten (tree_id, lft) ordering so descendants + # were contiguous with their parent. InventoryItem.Meta.ordering is a + # flat sort suited to the global list; for the indented device tab, + # order by ltree path so the rendered hierarchy is correct. + return super().get_children(request, parent).order_by('path') + @register_model_view(Device, 'configcontext', path='config-context') class DeviceConfigContextView(ObjectConfigContextView): diff --git a/netbox/extras/querysets.py b/netbox/extras/querysets.py index 7602aee530a..dfdd03608a5 100644 --- a/netbox/extras/querysets.py +++ b/netbox/extras/querysets.py @@ -130,10 +130,7 @@ def _get_config_context_filters(self): if self.model._meta.model_name == 'device': base_query.add( (Q( - locations__tree_id=OuterRef('location__tree_id'), - locations__level__lte=OuterRef('location__level'), - locations__lft__lte=OuterRef('location__lft'), - locations__rght__gte=OuterRef('location__rght'), + locations__path__ancestor_or_equal=OuterRef('location__path'), ) | Q(locations=None)), Q.AND ) @@ -142,40 +139,29 @@ def _get_config_context_filters(self): base_query.add(Q(locations=None), Q.AND) base_query.add(Q(device_types=None), Q.AND) - # MPTT-based filters + # Ltree-based filters: the ConfigContext-side tree node must be an ancestor + # (or equal to) the device/VM-side tree node, i.e. `cc_node.path @> obj_node.path`. base_query.add( (Q( - regions__tree_id=OuterRef('site__region__tree_id'), - regions__level__lte=OuterRef('site__region__level'), - regions__lft__lte=OuterRef('site__region__lft'), - regions__rght__gte=OuterRef('site__region__rght'), + regions__path__ancestor_or_equal=OuterRef('site__region__path'), ) | Q(regions=None)), Q.AND ) base_query.add( (Q( - site_groups__tree_id=OuterRef('site__group__tree_id'), - site_groups__level__lte=OuterRef('site__group__level'), - site_groups__lft__lte=OuterRef('site__group__lft'), - site_groups__rght__gte=OuterRef('site__group__rght'), + site_groups__path__ancestor_or_equal=OuterRef('site__group__path'), ) | Q(site_groups=None)), Q.AND ) base_query.add( (Q( - roles__tree_id=OuterRef('role__tree_id'), - roles__level__lte=OuterRef('role__level'), - roles__lft__lte=OuterRef('role__lft'), - roles__rght__gte=OuterRef('role__rght'), + roles__path__ancestor_or_equal=OuterRef('role__path'), ) | Q(roles=None)), Q.AND ) base_query.add( (Q( - platforms__tree_id=OuterRef('platform__tree_id'), - platforms__level__lte=OuterRef('platform__level'), - platforms__lft__lte=OuterRef('platform__lft'), - platforms__rght__gte=OuterRef('platform__rght'), + platforms__path__ancestor_or_equal=OuterRef('platform__path'), ) | Q(platforms=None)), Q.AND ) diff --git a/netbox/netbox/api/viewsets/__init__.py b/netbox/netbox/api/viewsets/__init__.py index 95b2351d3c8..6b7aa654a61 100644 --- a/netbox/netbox/api/viewsets/__init__.py +++ b/netbox/netbox/api/viewsets/__init__.py @@ -1,17 +1,16 @@ import logging +import warnings from functools import cached_property from django.core.exceptions import ObjectDoesNotExist, PermissionDenied from django.db import router, transaction from django.db.models import ProtectedError, RestrictedError -from django_pglocks import advisory_lock from rest_framework import mixins as drf_mixins from rest_framework import status from rest_framework.response import Response from rest_framework.viewsets import GenericViewSet from netbox.api.serializers.features import ChangeLogMessageSerializer -from netbox.constants import ADVISORY_LOCK_KEYS from utilities.api import get_annotations_for_serializer, get_prefetches_for_serializer from utilities.exceptions import AbortRequest, PreconditionFailed from utilities.query import reapply_model_ordering @@ -19,6 +18,7 @@ from . import mixins __all__ = ( + 'MPTTLockedMixin', 'NetBoxModelViewSet', 'NetBoxReadOnlyModelViewSet', ) @@ -337,20 +337,25 @@ def perform_destroy(self, instance): raise PermissionDenied() +# TODO: Remove this in NetBox v5.0 class MPTTLockedMixin: """ - Puts pglock on objects that derive from MPTTModel for parallel API calling. - Note: If adding this to a view, must add the model name to ADVISORY_LOCK_KEYS + Deprecated no-op mixin retained for backward compatibility. + + Historically this acquired a pglock around create/update/destroy to serialize + concurrent writes to MPTT-based tree models. NetBox no longer uses MPTT: tree + integrity is now maintained by the PostgreSQL ltree triggers (see + `utilities.ltree`), which take per-tree advisory locks at the database level. + This mixin is therefore now a transparent pass-through and may be removed in a + future release. Plugins should stop inheriting from it. """ - def create(self, request, *args, **kwargs): - with advisory_lock(ADVISORY_LOCK_KEYS[self.queryset.model._meta.model_name]): - return super().create(request, *args, **kwargs) - - def update(self, request, *args, **kwargs): - with advisory_lock(ADVISORY_LOCK_KEYS[self.queryset.model._meta.model_name]): - return super().update(request, *args, **kwargs) - - def destroy(self, request, *args, **kwargs): - with advisory_lock(ADVISORY_LOCK_KEYS[self.queryset.model._meta.model_name]): - return super().destroy(request, *args, **kwargs) + def __init_subclass__(cls, **kwargs): + super().__init_subclass__(**kwargs) + warnings.warn( + "MPTTLockedMixin is deprecated and no longer does anything; tree write " + "concurrency is now handled by ltree database triggers. Remove it from " + f"{cls.__name__}.", + DeprecationWarning, + stacklevel=2, + ) diff --git a/netbox/netbox/constants.py b/netbox/netbox/constants.py index b226c606cb0..ab741d2f94c 100644 --- a/netbox/netbox/constants.py +++ b/netbox/netbox/constants.py @@ -29,17 +29,6 @@ 'available-vlans': 100300, 'available-asns': 100400, - # MPTT locks - 'region': 105100, - 'sitegroup': 105200, - 'location': 105300, - 'tenantgroup': 105400, - 'contactgroup': 105500, - 'wirelesslangroup': 105600, - 'inventoryitem': 105700, - 'inventoryitemtemplate': 105800, - 'platform': 105900, - # Jobs 'job-schedules': 110100, } diff --git a/netbox/netbox/graphql/filter_lookups.py b/netbox/netbox/graphql/filter_lookups.py index 243dfb4e8ef..752e0e9661f 100644 --- a/netbox/netbox/graphql/filter_lookups.py +++ b/netbox/netbox/graphql/filter_lookups.py @@ -126,7 +126,7 @@ def filter(self, info: Info, queryset: QuerySet, prefix: DirectiveValue[str] = ' @strawberry.enum class TreeNodeMatch(Enum): EXACT = 'exact' # Just the node itself - DESCENDANTS = 'descendants' # Node and all descendants + DESCENDANTS = 'descendants' # All descendants, excluding the node itself SELF_AND_DESCENDANTS = 'self_and_descendants' # Node and all descendants CHILDREN = 'children' # Just immediate children SIBLINGS = 'siblings' # Nodes with same parent @@ -159,37 +159,39 @@ def filter(self, info: Info, queryset: QuerySet, prefix: DirectiveValue[str] = ' # Generate base Q filter for the related model without prefix q_filter = generate_tree_node_q_filter(related_model, self) - # Handle different relationship types - if isinstance(model_field, (ManyToManyField, ManyToManyRel)): - return queryset, Q(**{f'{model_field_name}__in': related_model.objects.filter(q_filter)}) - if isinstance(model_field, ForeignKey): - return queryset, Q(**{f'{model_field_name}__{k}': v for k, v in q_filter.children}) - if isinstance(model_field, ManyToOneRel): + # Handle different relationship types. All variants resolve the related + # rows against the q_filter (which may be a compound Q for DESCENDANTS, + # ANCESTORS, SIBLINGS, SELF_AND_DESCENDANTS) and join via __in. Destructuring + # q_filter.children into kwargs would crash on compound match types. + if isinstance(model_field, (ManyToManyField, ManyToManyRel, ForeignKey, ManyToOneRel)): return queryset, Q(**{f'{model_field_name}__in': related_model.objects.filter(q_filter)}) return queryset, Q(**{f'{model_field_name}__{k}': v for k, v in q_filter.children}) def generate_tree_node_q_filter(model_class, filter_value: TreeNodeFilter) -> Q: """ - Generate appropriate Q filter for MPTT tree filtering based on match type + Generate Q filter for ltree-backed hierarchical models based on match type. """ try: node = model_class.objects.get(id=filter_value.id) except model_class.DoesNotExist: return Q(pk__in=[]) + if not getattr(node, 'path', None): + return Q(id=filter_value.id) + if filter_value.match_type == TreeNodeMatch.EXACT: return Q(id=filter_value.id) if filter_value.match_type == TreeNodeMatch.DESCENDANTS: - return Q(tree_id=node.tree_id, lft__gt=node.lft, rght__lt=node.rght) + return Q(path__descendant=node.path) & ~Q(id=node.id) if filter_value.match_type == TreeNodeMatch.SELF_AND_DESCENDANTS: - return Q(tree_id=node.tree_id, lft__gte=node.lft, rght__lte=node.rght) + return Q(path__descendant_or_equal=node.path) if filter_value.match_type == TreeNodeMatch.CHILDREN: - return Q(tree_id=node.tree_id, level=node.level + 1, lft__gt=node.lft, rght__lt=node.rght) + return Q(parent_id=node.id) if filter_value.match_type == TreeNodeMatch.SIBLINGS: - return Q(tree_id=node.tree_id, level=node.level, parent=node.parent) & ~Q(id=node.id) + return Q(parent_id=node.parent_id) & ~Q(id=node.id) if filter_value.match_type == TreeNodeMatch.ANCESTORS: - return Q(tree_id=node.tree_id, lft__lt=node.lft, rght__gt=node.rght) + return Q(path__ancestor=node.path) & ~Q(id=node.id) if filter_value.match_type == TreeNodeMatch.PARENT: return Q(id=node.parent_id) if node.parent_id else Q(pk__in=[]) return Q() diff --git a/netbox/netbox/graphql/types.py b/netbox/netbox/graphql/types.py index 10975385331..96ac3e73a1d 100644 --- a/netbox/netbox/graphql/types.py +++ b/netbox/netbox/graphql/types.py @@ -1,6 +1,7 @@ import strawberry import strawberry_django from django.contrib.contenttypes.models import ContentType +from django.db.models import ExpressionWrapper, F, Func, IntegerField, Value from strawberry.types import Info from core.graphql.mixins import ChangelogMixin @@ -11,7 +12,9 @@ __all__ = ( 'BaseObjectType', 'ContentTypeType', + 'LtreeNodeMixin', 'NestedGroupObjectType', + 'NestedLtreeGroupObjectType', 'NetBoxObjectType', 'ObjectType', 'OrganizationalObjectType', @@ -83,6 +86,33 @@ class OrganizationalObjectType( pass +@strawberry.type +class LtreeNodeMixin: + """ + Exposes the ltree-backed tree depth as a `level` field, preserving the `level` + field MPTT-based types previously surfaced automatically as a real column. + + The depth is computed in the database as `nlevel(path) - 1` (root = 0) and + annotated onto the queryset. We prefer the annotation over the `path` column, + which is excluded from the schema. When a resolution path does not apply the + annotation (e.g. a nested relation), `ltree_level` is absent; fall back to the + loaded `path` string (the same depth the LtreeModel.level property computes) + so the field never raises AttributeError. + """ + @strawberry_django.field(annotate={ + 'ltree_level': ExpressionWrapper( + Func(F('path'), function='nlevel', output_field=IntegerField()) - Value(1), + output_field=IntegerField(), + ) + }) + def level(self) -> int: + ltree_level = getattr(self, 'ltree_level', None) + if ltree_level is not None: + return ltree_level + path = getattr(self, 'path', '') or '' + return str(path).count('.') + + class NestedGroupObjectType( ChangelogMixin, CustomFieldsMixin, @@ -92,7 +122,25 @@ class NestedGroupObjectType( BaseObjectType ): """ - Base GraphQL type for models which inherit from NestedGroupModel. + Base GraphQL type for the deprecated MPTT-backed NestedGroupModel, kept for + plugin compatibility. MPTT exposes `level` as a real column, so no annotation + mixin is needed. New code should use NestedLtreeGroupObjectType. + """ + pass + + +class NestedLtreeGroupObjectType( + LtreeNodeMixin, + ChangelogMixin, + CustomFieldsMixin, + JournalEntriesMixin, + TagsMixin, + OwnerMixin, + BaseObjectType +): + """ + Base GraphQL type for models which inherit from NestedLtreeGroupModel. + Adds a `level` field annotated via `nlevel(path)`. """ pass diff --git a/netbox/netbox/models/__init__.py b/netbox/netbox/models/__init__.py index a01472bb6e4..3b56b943efc 100644 --- a/netbox/netbox/models/__init__.py +++ b/netbox/netbox/models/__init__.py @@ -8,6 +8,7 @@ from mptt.models import MPTTModel, TreeForeignKey from netbox.models.features import * +from netbox.models.ltree import LtreeManager, LtreeModel, SortPathField from netbox.models.mixins import OwnerMixin from utilities.mptt import TreeManager from utilities.querysets import RestrictedQuerySet @@ -16,6 +17,8 @@ 'AdminModel', 'ChangeLoggedModel', 'NestedGroupModel', + 'NestedGroupModelMixin', + 'NestedLtreeGroupModel', 'NetBoxModel', 'OrganizationalModel', 'PrimaryModel', @@ -159,23 +162,11 @@ class Meta: abstract = True -class NestedGroupModel(OwnerMixin, NetBoxModel, MPTTModel): +class NestedGroupModelMixin(OwnerMixin, NetBoxModel): """ - Base model for objects which are used to form a hierarchy (regions, locations, etc.). These models nest - recursively using MPTT. Within each parent, each child instance must have a unique name. - - Note: django-mptt injects the (tree_id, lft) index dynamically, but Django's migration autodetector won't - detect it unless concrete subclasses explicitly declare Meta.indexes (even as an empty tuple). See #21016 - and django-mptt/django-mptt#682. + Shared field set and behavior for hierarchical group models. Concrete bases supply the + tree backend (MPTT or ltree) and the corresponding `parent` ForeignKey / manager. """ - parent = TreeForeignKey( - to='self', - on_delete=models.CASCADE, - related_name='children', - blank=True, - null=True, - db_index=True - ) name = models.CharField( verbose_name=_('name'), max_length=100 @@ -194,6 +185,29 @@ class NestedGroupModel(OwnerMixin, NetBoxModel, MPTTModel): blank=True ) + class Meta: + abstract = True + + def __str__(self): + return self.name + + +class NestedGroupModel(NestedGroupModelMixin, MPTTModel): + """ + Deprecated MPTT-backed nested group base, retained for backwards compatibility with plugins. + + New code (in NetBox core and in plugins) should use `NestedLtreeGroupModel` instead. This + class will be removed in a future release once the deprecation period has elapsed. + """ + parent = TreeForeignKey( + to='self', + on_delete=models.CASCADE, + related_name='children', + blank=True, + null=True, + db_index=True + ) + objects = TreeManager() class Meta: @@ -202,19 +216,52 @@ class Meta: class MPTTMeta: order_insertion_by = ('name',) - def __str__(self): - return self.name - def clean(self): super().clean() - # An MPTT model cannot be its own parent + # A nested group cannot be its own parent or a descendant of itself. The ltree + # base (NestedLtreeGroupModel) enforces this via LtreeModel.clean(); this MPTT + # variant keeps the original get_descendants()-based check. if not self._state.adding and self.parent and self.parent in self.get_descendants(include_self=True): raise ValidationError({ - "parent": "Cannot assign self or child {type} as parent.".format(type=self._meta.verbose_name) + "parent": _("Cannot assign self or a descendant as parent.") }) +class NestedLtreeGroupModel(NestedGroupModelMixin, LtreeModel): + """ + Base model for objects which are used to form a hierarchy (regions, locations, etc.). These models nest + recursively using PostgreSQL ltree. Within each parent, each child instance must have a unique name. + + `sort_path` is a trigger-maintained text column holding a chr(9)-separated chain of ancestor + names; ordering by it yields tree-flatten output with siblings in name (collation) order. + Inserts, reparents, AND renames all update `sort_path` (a rename cascades to descendants), so + list ordering reflects renames immediately — unlike django-mptt's `order_insertion_by`, which + left descendants stale until a manual rebuild. + """ + parent = models.ForeignKey( + to='self', + on_delete=models.CASCADE, + related_name='children', + blank=True, + null=True, + db_index=True + ) + sort_path = SortPathField( + editable=False, + blank=True, + default='', + ) + + # Re-declare so the LtreeManager wins over BaseModel's RestrictedQuerySet + # default manager via MRO resolution. + objects = LtreeManager() + + class Meta: + abstract = True + ordering = ('sort_path',) + + class OrganizationalModel(OwnerMixin, NetBoxModel): """ Organizational models are those which are used solely to categorize and qualify other objects, and do not convey diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index 9a084ffe117..ef5d530a2dd 100644 --- a/netbox/netbox/models/features.py +++ b/netbox/netbox/models/features.py @@ -407,13 +407,13 @@ def get_contacts(self, inherited=True): """ from tenancy.models import ContactAssignment - from . import NestedGroupModel + from . import NestedGroupModel, NestedLtreeGroupModel filter = Q( object_type=ObjectType.objects.get_for_model(self), object_id__in=( self.get_ancestors(include_self=True) - if (isinstance(self, NestedGroupModel) and inherited) + if (isinstance(self, (NestedGroupModel, NestedLtreeGroupModel)) and inherited) else [self.pk] ), ) diff --git a/netbox/netbox/models/lookups.py b/netbox/netbox/models/lookups.py new file mode 100644 index 00000000000..b1541167b5c --- /dev/null +++ b/netbox/netbox/models/lookups.py @@ -0,0 +1,56 @@ +from django.db.models import Lookup + +__all__ = ( + 'Ancestor', + 'AncestorOrEqual', + 'Descendant', + 'DescendantOrEqual', +) + + +class Ancestor(Lookup): + """ + `path` is a strict ancestor of the queried path: path @> rhs AND path <> rhs. + + Use `ancestor_or_equal` for the inclusive form (`@>`). + """ + lookup_name = 'ancestor' + + def as_sql(self, compiler, connection): + lhs, lhs_params = self.process_lhs(compiler, connection) + rhs, rhs_params = self.process_rhs(compiler, connection) + return f'({lhs} @> {rhs} AND {lhs} <> {rhs})', lhs_params + rhs_params + lhs_params + rhs_params + + +class AncestorOrEqual(Lookup): + """`path` is an ancestor of (or equal to) the queried path: path @> rhs""" + lookup_name = 'ancestor_or_equal' + + def as_sql(self, compiler, connection): + lhs, lhs_params = self.process_lhs(compiler, connection) + rhs, rhs_params = self.process_rhs(compiler, connection) + return f'{lhs} @> {rhs}', lhs_params + rhs_params + + +class Descendant(Lookup): + """ + `path` is a strict descendant of the queried path: path <@ rhs AND path <> rhs. + + Use `descendant_or_equal` for the inclusive form (`<@`). + """ + lookup_name = 'descendant' + + def as_sql(self, compiler, connection): + lhs, lhs_params = self.process_lhs(compiler, connection) + rhs, rhs_params = self.process_rhs(compiler, connection) + return f'({lhs} <@ {rhs} AND {lhs} <> {rhs})', lhs_params + rhs_params + lhs_params + rhs_params + + +class DescendantOrEqual(Lookup): + """`path` is a descendant of (or equal to) the queried path: path <@ rhs""" + lookup_name = 'descendant_or_equal' + + def as_sql(self, compiler, connection): + lhs, lhs_params = self.process_lhs(compiler, connection) + rhs, rhs_params = self.process_rhs(compiler, connection) + return f'{lhs} <@ {rhs}', lhs_params + rhs_params diff --git a/netbox/netbox/models/ltree.py b/netbox/netbox/models/ltree.py new file mode 100644 index 00000000000..a5ff18206ee --- /dev/null +++ b/netbox/netbox/models/ltree.py @@ -0,0 +1,536 @@ +""" +Ltree-based hierarchical model support - a replacement for django-mptt backed by +a PostgreSQL ltree column. + +LtreeModel covers the subset of django-mptt's MPTTModel API that NetBox actually +uses. It is deliberately NOT a full reimplementation of MPTT's surface — methods +NetBox does not rely on (e.g. get_leafnodes(), get_next_sibling(), +get_previous_sibling()) are intentionally omitted. + +Paths are maintained entirely by PostgreSQL triggers installed via the +InstallLtreeTriggers migration operation. The Python layer never computes or +mutates paths directly; it only reads `path` back from the database after +inserts and parent_id changes via refresh_from_db(fields=['path']). +""" +from django.core.exceptions import FieldDoesNotExist, ValidationError +from django.db import IntegrityError, OperationalError, connection, models +from django.db.models import ForeignKey, ManyToManyField +from django.db.models.expressions import RawSQL +from django.utils.translation import gettext_lazy as _ + +# Import lookup classes by their fully-qualified path rather than `from . import +# lookups`: this module is imported by netbox/models/__init__.py before that package +# finishes initializing, so a package-relative import would fail the attribute lookup +# on the partially-initialized `netbox.models` package (circular import). +from netbox.models.lookups import Ancestor, AncestorOrEqual, Descendant, DescendantOrEqual +from utilities.querysets import RestrictedQuerySet + +__all__ = ( + 'LtreeField', + 'LtreeManager', + 'LtreeModel', + 'LtreeQuerySet', + 'SortPathField', +) + + +# +# Field +# + +class LtreeField(models.TextField): + """ + Custom field backed by PostgreSQL's ltree type. Stores hierarchical paths + such as "1.4.27" (each label is the integer PK of an ancestor). + """ + description = "PostgreSQL ltree field" + + # `path` is computed by a BEFORE INSERT trigger, so its final value isn't known + # until the row is written. Marking the field db_returning lets the + # INSERT ... RETURNING clause fetch the trigger-computed value in the same + # round-trip (PostgreSQL evaluates RETURNING after BEFORE triggers fire), + # avoiding a follow-up SELECT in LtreeModel.save(). Mirrors AutoFieldMixin. + db_returning = True + + def db_type(self, connection): + return 'ltree' + + def get_prep_value(self, value): + if value is None: + return value + return str(value) + + +LtreeField.register_lookup(Ancestor) +LtreeField.register_lookup(AncestorOrEqual) +LtreeField.register_lookup(Descendant) +LtreeField.register_lookup(DescendantOrEqual) + + +class SortPathField(models.TextField): + """ + Text column holding the chr(9)-separated chain of ancestor names that drives + tree-flatten ordering. Like `path`, its value is maintained by triggers, so it + is marked db_returning to be populated via INSERT ... RETURNING without an + extra SELECT. It deconstructs as a plain TextField so existing migrations + (which created the column as TextField) require no schema change. + """ + db_returning = True + + def deconstruct(self): + name, path, args, kwargs = super().deconstruct() + return name, 'django.db.models.TextField', args, kwargs + + +# +# QuerySet / Manager +# + +class LtreeQuerySet(RestrictedQuerySet): + """QuerySet for ltree-based hierarchies, layered on RestrictedQuerySet.""" + + def bulk_create(self, objs, *args, **kwargs): + """ + Same as the standard `bulk_create` but rejects any row whose parent is an + unsaved instance. Django's bulk_create builds the multi-row INSERT VALUES + up front from each instance's `parent_id`; an unsaved parent's pk is not + assigned until the INSERT's RETURNING clause executes, so a child + referencing an unsaved parent (even one earlier in the same batch) goes + in with parent_id=NULL and the BEFORE trigger stores it as a root. Save + the parents first, then bulk_create their children. + """ + objs_list = list(objs) + for idx, obj in enumerate(objs_list): + parent = getattr(obj, 'parent', None) + if parent is not None and parent.pk is None: + raise ValueError( + "bulk_create: child at index {idx} references an unsaved parent. " + "Django cannot propagate the parent's RETURNING-assigned pk into " + "the child's parent_id before the INSERT executes, so the child " + "would be persisted with parent_id=NULL and stored as a root. " + "Save the parent first, then bulk_create the children.".format(idx=idx) + ) + return super().bulk_create(objs_list, *args, **kwargs) + + def add_related_count(self, queryset, model, rel_field, count_attr, cumulative=False): + """ + Annotate `queryset` with the count of `model` instances related via + `rel_field`, mirroring django-mptt's `TreeManager.add_related_count`. + + When `cumulative=True`, counts include rows pointing to any descendant + (using the ltree `<@` operator against the parent's `path`). Handles + ForeignKey, ManyToManyField, and the NetBox GenericForeignKey "scope" + pattern (scope_type / scope_id). + + The six historical variants (3 relation kinds × cumulative/not) are + assembled from two fragments: how a related row links to a tree node + (`link_expr` + any join/scope filter), and which node is counted — the + parent row itself (non-cumulative) or any node in its subtree via `<@` + (cumulative). + """ + try: + field = model._meta.get_field(rel_field) + except Exception: + field = None + is_many_to_many = isinstance(field, ManyToManyField) + has_direct_fk = isinstance(field, ForeignKey) + has_generic_fk = ( + hasattr(model, 'scope_type') and hasattr(model, 'scope_id') + and not has_direct_fk and not is_many_to_many + ) + + qn = connection.ops.quote_name + parent_table = qn(queryset.model._meta.db_table) + related_table = qn(model._meta.db_table) + + # `from_join` is the FROM (+ m2m join); `link_expr` is the column that points + # at a tree node's id; `scope_filter` constrains the generic-FK content type. + params = [] + from_join = f'FROM {related_table}' + scope_filter = '' + if is_many_to_many: + # m2m_column_name() points at the declaring model (`model`); + # m2m_reverse_name() points at the related (tree) model. + m2m_table = qn(field.remote_field.through._meta.db_table) + from_join += ( + f' INNER JOIN {m2m_table}' + f' ON {related_table}."id" = {m2m_table}.{qn(field.m2m_column_name())}' + ) + link_expr = f'{m2m_table}.{qn(field.m2m_reverse_name())}' + elif has_generic_fk: + link_expr = f'{related_table}."scope_id"' + # Resolve scope_type_id via subquery so the annotation can be built at + # import time (e.g. in a view class body) before contenttypes migrate. + scope_filter = ( + f' AND {related_table}."scope_type_id" = (' + 'SELECT id FROM django_content_type WHERE app_label = %s AND model = %s)' + ) + params = [queryset.model._meta.app_label, queryset.model._meta.model_name] + else: + # field.column honors a custom db_column; fall back to Django's default + # `{rel_field}_id` if the field was not resolved (a renamed unrelated + # field must not break import-time annotation construction). + rel_field_col = qn(field.column if field is not None else f'{rel_field}_id') + link_expr = f'{related_table}.{rel_field_col}' + + if cumulative: + node_join = f' INNER JOIN {parent_table} AS subtree ON {link_expr} = subtree."id"' + where = f'WHERE subtree."path" <@ {parent_table}."path"{scope_filter}' + else: + node_join = '' + where = f'WHERE {link_expr} = {parent_table}."id"{scope_filter}' + + sql = f'(SELECT COUNT(DISTINCT {related_table}."id") {from_join}{node_join} {where})' + return queryset.annotate(**{ + count_attr: RawSQL(sql, params, output_field=models.IntegerField()) + }) + + +class LtreeManager(models.Manager.from_queryset(LtreeQuerySet)): + """Drop-in replacement for django-mptt's TreeManager.""" + + +# +# Abstract model +# + +class LtreeModelBase(models.base.ModelBase): + """ + Metaclass that keeps a model's `sort_path` collation in sync with its `name`. + + `sort_path` holds a chr(9)-joined chain of ancestor names, so to flatten siblings + in the same order the database sorts `name`, the two columns must share a collation. + Deriving it here means a subclass that gives `name` a custom db_collation (e.g. + `natural_sort`) automatically gets a matching `sort_path` — no need to redeclare the + field just to repeat the collation. An explicit db_collation on `sort_path` is left + untouched. + """ + def __new__(mcs, name, bases, namespace, **kwargs): + cls = super().__new__(mcs, name, bases, namespace, **kwargs) + if cls._meta.abstract: + return cls + try: + name_field = cls._meta.get_field('name') + sort_path_field = cls._meta.get_field('sort_path') + except FieldDoesNotExist: + return cls + name_collation = getattr(name_field, 'db_collation', None) + if name_collation and not getattr(sort_path_field, 'db_collation', None): + sort_path_field.db_collation = name_collation + return cls + + +class LtreeModel(models.Model, metaclass=LtreeModelBase): + """ + Abstract base for hierarchical models backed by PostgreSQL ltree. + + Subclasses must declare a `parent = models.ForeignKey('self', ...)`. The + `path` column is maintained by per-table triggers installed via + InstallLtreeTriggers; do not write to it from Python. + + Bulk creates: + The BEFORE INSERT trigger resolves a row's parent by SELECTing `path` + from the same table by parent_id. LtreeQuerySet.bulk_create() rejects any + row whose parent is an *unsaved* instance, so in normal use — parents + saved before their children are bulk-created — batch order does not matter + (each parent row already exists for the lookup). The lone exception is + manually pre-assigned PKs: if a child references a same-batch parent by a + hand-set pk, that parent must appear earlier in the batch (the BEFORE + trigger fires per row in list order), or the child gets a root-level path. + + Sort-path on rename: + For subclasses with the optional `sort_path` column (see + InstallLtreeTriggers' `name_column` arg), renaming a row updates its + own sort_path AND cascades into descendants' sort_paths via the AFTER + trigger. This diverges from django-mptt's `order_insertion_by` (which + leaves both the renamed row and its descendants stale until a manual + rebuild) because list views are expected to reflect renames promptly. + `rebuild_sort_paths()` is still available for bulk repair after raw + SQL writes that bypass the triggers. + + Concurrency: + Path maintenance takes no table-wide lock (unlike django-mptt, which + acquired a per-model advisory lock on *every* write to protect its global + lft/rght/tree_id numbering). Instead, the BEFORE trigger serializes per + tree: it takes a transaction-level advisory lock keyed on the root of the + tree being written (and, for a cross-tree move, on both the source and + destination roots, acquired in ascending key order to avoid deadlocks). + + Every child insert, move, and reparent of a node in a tree takes the same + key, so an insert deep in a subtree and a concurrent reparent of one of its + ancestors are serialized — the loser blocks until the winner commits, and + the winner's AFTER cascade can never miss a row inserted concurrently + (which a row-level `FOR SHARE` on the parent could not prevent: a set-based + cascade's snapshot would skip a row inserted after it began). Writes to + *different* trees use different keys and proceed fully in parallel — e.g. + inventory ingestion across different devices, each its own tree. + + Inserting a *new root* (parent_id IS NULL) takes no lock: the uncommitted + row is invisible to other transactions and has no descendants, so nothing + can contend with it. This keeps a bulk import of many top-level objects + (the dominant import pattern) lock-free instead of taking one advisory lock + per root. The residual case that still scales with volume is inserting + children into many *distinct existing* trees in one transaction (one lock + per distinct tree touched); if a very large such import hits "out of shared + memory", raising the server's `max_locks_per_transaction` is the remedy. + + Two residual, retryable cases remain (PostgreSQL aborts one transaction + with a deadlock error rather than persisting a stale path): crossing + reparents (moving A under B while moving B under A), and two concurrent + *moves* in an ancestor/descendant relationship (a move locks the moved + row before its BEFORE trigger can take the advisory lock). Plain inserts + — the high-volume path — never hit this. + """ + # `default=''` here is a Django-side placeholder that the BEFORE INSERT + # trigger always overwrites with a valid path before the row reaches + # storage. Empty ltree (`''`) is itself a valid PostgreSQL ltree value + # (nlevel = 0) in supported PostgreSQL versions (15+), so even harnesses + # that bypass the trigger will not fail at INSERT — they will simply + # store a zero-level path. + path = LtreeField(editable=False, null=False, blank=True, default='') + + objects = LtreeManager() + + class Meta: + abstract = True + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + # Read from __dict__ rather than via attribute access: a deferred + # `parent_id`/`name` (e.g. a GraphQL query selecting only a subset of + # fields) must not be lazily loaded here, since that triggers + # refresh_from_db() which rebuilds the instance and recurses into __init__. + self._loaded_parent_id = self.__dict__.get('parent_id') + self._loaded_name = self.__dict__.get('name') + + @classmethod + def from_db(cls, db, field_names, values): + instance = super().from_db(db, field_names, values) + instance._loaded_parent_id = instance.__dict__.get('parent_id') + instance._loaded_name = instance.__dict__.get('name') + return instance + + def _parent_creates_cycle(self): + """ + Return True if the current `parent` assignment would make this node its + own ancestor (the new parent is self or one of its descendants), mirroring + django-mptt's save-time InvalidMove guard. + + Subclasses whose `parent` is system-managed (e.g. ModuleBay, whose parent + is derived from its module) may override this to disable the check. + """ + if self.parent_id is None: + return False + # Self-as-parent is always a cycle and must be caught even if self.path + # is empty or deferred (path would otherwise short-circuit below). + if self.parent_id == self.pk: + return True + if not self.path: + return False + # The new parent lies inside this node's current subtree iff its path is a + # descendant of (or equal to) self.path. + return type(self)._default_manager.filter( + pk=self.parent_id, path__descendant_or_equal=self.path + ).exists() + + @classmethod + def _has_sort_path(cls): + """ + Whether this model carries the optional trigger-maintained `sort_path` + column (the MPTT `order_insertion_by=('name',)` equivalent). Single source + of truth for clean(), save(), and _tree_order_field(). + """ + try: + cls._meta.get_field('sort_path') + return True + except FieldDoesNotExist: + return False + + def clean(self): + """ + Reject assigning self or a descendant as parent, surfacing it as a field + error for forms/serializers. This mirrors the save()-time guard; the two + share _parent_creates_cycle() so the rule lives in exactly one place. + + Subclasses whose `parent` is system-managed (e.g. ModuleBay) disable the + check by overriding _parent_creates_cycle() to return False. + + For sort_path-backed models, also reject a tab in the name column: sort_path + joins ancestor names with chr(9) (TAB), so a literal tab in a name would + corrupt sibling ordering for the node and its descendants. + """ + super().clean() + + if self.pk and self._parent_creates_cycle(): + raise ValidationError({ + "parent": _("Cannot assign self or a descendant as parent.") + }) + + if self._has_sort_path() and '\t' in (getattr(self, 'name', None) or ''): + raise ValidationError({ + "name": _("Name cannot contain tab characters.") + }) + + def save(self, *args, **kwargs): + """ + Triggers compute `path` (and `sort_path`, where present) server-side. + + On INSERT the trigger-maintained columns are db_returning, so they are + populated in-place by the INSERT ... RETURNING clause without an extra + query. On an UPDATE that changes `parent` or the name column the triggers + rewrite those columns server-side, so refresh them afterward to keep the + in-memory instance consistent (e.g. so change logging snapshots the value + the triggers actually wrote, not a stale one). + """ + is_insert = self._state.adding + # When update_fields is supplied and excludes parent, the DB does not see + # the new parent_id, so the trigger does not fire and _loaded_parent_id + # must not advance — otherwise a subsequent full save() would mis-detect + # the (real) parent change as already-applied and leave path stale. + update_fields = kwargs.get('update_fields') + parent_written = update_fields is None or 'parent' in update_fields or 'parent_id' in update_fields + parent_changed = (not is_insert) and parent_written and self.parent_id != self._loaded_parent_id + + # Reject cyclic moves before writing, mirroring django-mptt's save-time + # guard so scripts / bulk callers (which bypass form & serializer clean()) + # cannot silently corrupt the tree. + if parent_changed and self._parent_creates_cycle(): + raise ValidationError(_("Cannot assign self or a descendant as parent.")) + + # The sort_path trigger also fires on a name change; detect that so the + # cascaded sort_path can be refreshed below (path-only models have no + # sort_path and are unaffected by renames). + has_sort_path = self._has_sort_path() + name_written = update_fields is None or 'name' in update_fields + name_changed = ( + (not is_insert) and has_sort_path and name_written + and self.__dict__.get('name') != self._loaded_name + ) + + try: + super().save(*args, **kwargs) + except IntegrityError as exc: + # A concurrent reparent that races the Python-level _parent_creates_cycle + # check is caught by the BEFORE trigger, which RAISEs 'cycle detected ...' + # with ERRCODE = check_violation (SQLSTATE 23514). Gate on the SQLSTATE + # (the primary, stable signal) AND the message marker, so an unrelated + # CHECK constraint on a subclass (also 23514) is not misreported as a + # cycle. Surface it as a ValidationError so the API/UI returns 400 instead + # of the IntegrityError → 500 the trigger would otherwise produce. + if ( + getattr(exc.__cause__, 'sqlstate', None) == '23514' + and 'cycle detected' in str(exc) + ): + raise ValidationError( + _("Cannot assign self or a descendant as parent.") + ) from None + # The BEFORE trigger likewise rejects a tab in the name (it would corrupt + # sort_path); translate it for direct save() calls that skip clean(). + if ( + getattr(exc.__cause__, 'sqlstate', None) == '23514' + and 'tab character' in str(exc) + ): + raise ValidationError({ + "name": _("Name cannot contain tab characters.") + }) from None + raise + except OperationalError as exc: + # The per-tree advisory locks can deadlock on crossing reparents or two + # concurrent ancestor/descendant moves; PostgreSQL aborts one with + # SQLSTATE 40P01. Surface a clear, retryable message instead of the + # opaque 500 the bare OperationalError would produce. + if getattr(exc.__cause__, 'sqlstate', None) == '40P01': + raise ValidationError( + _("The hierarchy was modified concurrently; please retry.") + ) from None + raise + + if (parent_changed or name_changed) and not is_insert: + # The triggers rewrote path/sort_path on this UPDATE; fetch them back so + # the in-memory instance matches storage (e.g. so change logging snapshots + # the value the triggers wrote, not a stale one). This costs one extra + # SELECT per reparent/rename; INSERT ... RETURNING covers the insert case, + # so only updates reach here. + refresh_fields = ['path'] + (['sort_path'] if has_sort_path else []) + self.refresh_from_db(fields=refresh_fields) + + if is_insert or parent_written: + self._loaded_parent_id = self.parent_id + if is_insert or name_written: + self._loaded_name = self.__dict__.get('name') + + # -- MPTT-compatible API ------------------------------------------------ + + @property + def level(self): + """Zero-based depth (root = 0). Mirrors django-mptt's `level`.""" + if not self.path: + return 0 + return str(self.path).count('.') + + def get_level(self): + return self.level + + @classmethod + def _tree_order_field(cls): + """ + Field name to order hierarchical queries by. Models that carry a + `sort_path` column (the MPTT `order_insertion_by=('name',)` equivalent) + order siblings by name to match the prior MPTT behavior; models + without it fall back to `path` (PK-padded, insertion order). + """ + return 'sort_path' if cls._has_sort_path() else 'path' + + def get_ancestors(self, ascending=False, include_self=False): + if not self.path: + return type(self)._default_manager.none() + lookup = 'ancestor_or_equal' if include_self else 'ancestor' + qs = type(self)._default_manager.filter(**{f'path__{lookup}': self.path}) + order_field = self._tree_order_field() + return qs.order_by(f'-{order_field}' if ascending else order_field) + + def get_descendants(self, include_self=False): + if not self.path: + return type(self)._default_manager.none() + lookup = 'descendant_or_equal' if include_self else 'descendant' + return type(self)._default_manager.filter( + **{f'path__{lookup}': self.path} + ).order_by(self._tree_order_field()) + + def get_children(self): + return type(self)._default_manager.filter(parent_id=self.pk).order_by(self._tree_order_field()) + + @classmethod + def rebuild_sort_paths(cls, name_column='name'): + """ + Recompute `sort_path` for every row from current values of `name_column`. + + Inserts, reparents, AND renames are all maintained automatically by the + BEFORE/AFTER triggers (the BEFORE trigger fires on INSERT and on updates to + parent_id or the name column). Use this only to repair `sort_path` after a + raw SQL write (e.g. a bulk COPY or a direct UPDATE) that bypassed those + triggers. + + Raises if the table does not have a `sort_path` column. + """ + from django.db import connection + + if not cls._has_sort_path(): + raise NotImplementedError( + f"{cls.__name__} does not have a sort_path column" + ) + qn = connection.ops.quote_name + table = qn(cls._meta.db_table) + name_col = qn(name_column) + sql = f''' + WITH RECURSIVE t(id, parent_id, sort_path) AS ( + SELECT id, parent_id, {name_col}::text + FROM {table} WHERE parent_id IS NULL + UNION ALL + SELECT r.id, r.parent_id, t.sort_path || chr(9) || r.{name_col} + FROM {table} r INNER JOIN t ON r.parent_id = t.id + ) + UPDATE {table} SET sort_path = t.sort_path FROM t WHERE {table}.id = t.id; + ''' + with connection.cursor() as cursor: + cursor.execute(sql) diff --git a/netbox/netbox/tables/columns.py b/netbox/netbox/tables/columns.py index e0d16c664b7..5620d7cb538 100644 --- a/netbox/netbox/tables/columns.py +++ b/netbox/netbox/tables/columns.py @@ -45,6 +45,7 @@ 'TagColumn', 'TemplateColumn', 'ToggleColumn', + 'TreeColumn', 'UtilizationColumn', ) @@ -604,9 +605,9 @@ def value(self, record, table, **kwargs): return None -class MPTTColumn(tables.TemplateColumn): +class TreeColumn(tables.TemplateColumn): """ - Display a nested hierarchy for MPTT-enabled models. + Display a nested hierarchy for tree-enabled models (Region, Location, etc.). """ template_code = """ {% load helpers %} @@ -628,6 +629,11 @@ def value(self, value): return value +# Deprecated alias for plugin compatibility; use TreeColumn going forward. +# TODO: Remove this in NetBox v5.0 +MPTTColumn = TreeColumn + + class UtilizationColumn(tables.TemplateColumn): """ Display a colored utilization bar graph. diff --git a/netbox/netbox/tables/tables.py b/netbox/netbox/tables/tables.py index 7c7275f21ab..fcfa6d7e8c4 100644 --- a/netbox/netbox/tables/tables.py +++ b/netbox/netbox/tables/tables.py @@ -340,7 +340,7 @@ class NestedGroupModelTable(NetBoxTable): linkify=True, verbose_name=_('Owner'), ) - name = columns.MPTTColumn( + name = columns.TreeColumn( verbose_name=_('Name'), linkify=True ) diff --git a/netbox/netbox/tests/test_base_classes.py b/netbox/netbox/tests/test_base_classes.py index cc1ab1de511..b8754a2e80e 100644 --- a/netbox/netbox/tests/test_base_classes.py +++ b/netbox/netbox/tests/test_base_classes.py @@ -40,11 +40,12 @@ ) from netbox.graphql.types import ( NestedGroupObjectType, + NestedLtreeGroupObjectType, NetBoxObjectType, OrganizationalObjectType, PrimaryObjectType, ) -from netbox.models import NestedGroupModel, NetBoxModel, OrganizationalModel, PrimaryModel +from netbox.models import NestedGroupModelMixin, NestedLtreeGroupModel, NetBoxModel, OrganizationalModel, PrimaryModel from netbox.registry import registry from netbox.tables import ( NestedGroupModelTable, @@ -77,7 +78,7 @@ def get_model_form_base_class(model): return PrimaryModelForm if issubclass(model, OrganizationalModel): return OrganizationalModelForm - if issubclass(model, NestedGroupModel): + if issubclass(model, NestedGroupModelMixin): return NestedGroupModelForm if issubclass(model, NetBoxModel): return NetBoxModelForm @@ -94,7 +95,7 @@ def get_bulk_edit_form_base_class(model): return PrimaryModelBulkEditForm if issubclass(model, OrganizationalModel): return OrganizationalModelBulkEditForm - if issubclass(model, NestedGroupModel): + if issubclass(model, NestedGroupModelMixin): return NestedGroupModelBulkEditForm if issubclass(model, NetBoxModel): return NetBoxModelBulkEditForm @@ -111,7 +112,7 @@ def get_import_form_base_class(model): return PrimaryModelImportForm if issubclass(model, OrganizationalModel): return OrganizationalModelImportForm - if issubclass(model, NestedGroupModel): + if issubclass(model, NestedGroupModelMixin): return NestedGroupModelImportForm if issubclass(model, NetBoxModel): return NetBoxModelImportForm @@ -128,7 +129,7 @@ def get_filterset_form_base_class(model): return PrimaryModelFilterSetForm if issubclass(model, OrganizationalModel): return OrganizationalModelFilterSetForm - if issubclass(model, NestedGroupModel): + if issubclass(model, NestedGroupModelMixin): return NestedGroupModelFilterSetForm if issubclass(model, NetBoxModel): return NetBoxModelFilterSetForm @@ -192,7 +193,7 @@ def get_model_filterset_base_class(model): return PrimaryModelFilterSet if issubclass(model, OrganizationalModel): return OrganizationalModelFilterSet - if issubclass(model, NestedGroupModel): + if issubclass(model, NestedGroupModelMixin): return NestedGroupModelFilterSet if issubclass(model, NetBoxModel): return NetBoxModelFilterSet @@ -243,7 +244,7 @@ def get_model_table_base_class(model): return PrimaryModelTable if issubclass(model, OrganizationalModel): return OrganizationalModelTable - if issubclass(model, NestedGroupModel): + if issubclass(model, NestedGroupModelMixin): return NestedGroupModelTable if issubclass(model, NetBoxModel): return NetBoxTable @@ -324,7 +325,7 @@ def get_model_serializer_base_class(model): return PrimaryModelSerializer if issubclass(model, OrganizationalModel): return OrganizationalModelSerializer - if issubclass(model, NestedGroupModel): + if issubclass(model, NestedGroupModelMixin): return NestedGroupModelSerializer if issubclass(model, NetBoxModel): return NetBoxModelSerializer @@ -365,7 +366,9 @@ def get_model_type_base_class(model): return PrimaryObjectType if issubclass(model, OrganizationalModel): return OrganizationalObjectType - if issubclass(model, NestedGroupModel): + if issubclass(model, NestedLtreeGroupModel): + return NestedLtreeGroupObjectType + if issubclass(model, NestedGroupModelMixin): return NestedGroupObjectType if issubclass(model, NetBoxModel): return NetBoxObjectType diff --git a/netbox/netbox/views/generic/bulk_views.py b/netbox/netbox/views/generic/bulk_views.py index bdd7528d07f..6672ab5a0f7 100644 --- a/netbox/netbox/views/generic/bulk_views.py +++ b/netbox/netbox/views/generic/bulk_views.py @@ -1,6 +1,7 @@ import logging import re from collections import Counter +from contextlib import contextmanager from copy import deepcopy from types import SimpleNamespace @@ -16,7 +17,6 @@ from django.shortcuts import get_object_or_404, redirect, render from django.utils.safestring import mark_safe from django.utils.translation import gettext as _ -from mptt.models import MPTTModel from core.exceptions import JobFailed from core.models import ObjectType @@ -55,6 +55,35 @@ ) +# TODO: Remove in NetBox v5.0. +# MPTT support is retained only for plugins whose tree models still derive from the +# deprecated MPTT-backed bases. NetBox core uses netbox.models.ltree.LtreeModel, whose +# database triggers maintain the tree on every write, so ltree (and non-tree) models +# need no special bulk handling. These two helpers confine all MPTT-specific bulk +# bookkeeping so it can be deleted in one place; for non-MPTT models they are no-ops. +@contextmanager +def _delay_mptt_updates(model): + """ + Defer tree (lft/rght/tree_id) recomputation until the end of a bulk write for + legacy MPTT models. A no-op context manager for ltree and non-tree models. + """ + from mptt.models import MPTTModel + + if issubclass(model, MPTTModel): + with model.objects.delay_mptt_updates(): + yield + else: + yield + + +def _rebuild_mptt_tree(model): + """Rebuild the MPTT tree after a bulk edit for legacy MPTT models; else a no-op.""" + from mptt.models import MPTTModel + + if issubclass(model, MPTTModel): + model.objects.rebuild() + + class ObjectListView(BaseMultiObjectView, ActionsMixin, TableMixin): """ Display multiple objects, all the same type, as a table. @@ -563,11 +592,9 @@ def create_and_update_objects(self, form, request): for obj in self.queryset.model.objects.filter(id__in=prefetch_ids) } if prefetch_ids else {} - # For MPTT models, delay tree updates until all saves are complete - if issubclass(self.queryset.model, MPTTModel): - with self.queryset.model.objects.delay_mptt_updates(): - saved_objects = self._process_import_records(form, request, records, prefetched_objects) - else: + # Delay tree updates until all saves are complete (MPTT plugin models only; + # no-op for ltree). TODO: Remove the wrapper in v5.0 (see _delay_mptt_updates). + with _delay_mptt_updates(self.queryset.model): saved_objects = self._process_import_records(form, request, records, prefetched_objects) return saved_objects @@ -762,9 +789,9 @@ def _update_objects(self, form, request): if is_background_request(request): request.job.logger.info(f"Updated {obj}") - # Rebuild the tree for MPTT models - if issubclass(self.queryset.model, MPTTModel): - self.queryset.model.objects.rebuild() + # Rebuild the tree for MPTT plugin models (no-op for ltree; its triggers keep + # the tree current). TODO: Remove in v5.0 (see _rebuild_mptt_tree). + _rebuild_mptt_tree(self.queryset.model) return updated_objects @@ -974,15 +1001,10 @@ def post(self, request): renamed_pks = self._rename_objects(form, selected_objects, field_names) if '_apply' in request.POST: - # For MPTT models, delay tree updates until all saves are complete - if issubclass(self.queryset.model, MPTTModel): - with self.queryset.model.objects.delay_mptt_updates(): - for obj in selected_objects: - for field in field_names: - setattr(obj, field, getattr(obj.new_names, field)) - obj._changelog_message = form.cleaned_data.get('changelog_message', '') - obj.save() - else: + # Delay tree updates until all saves are complete (MPTT + # plugin models only; no-op for ltree). + # TODO: Remove the wrapper in v5.0 (see _delay_mptt_updates). + with _delay_mptt_updates(self.queryset.model): for obj in selected_objects: for field in field_names: setattr(obj, field, getattr(obj.new_names, field)) diff --git a/netbox/templates/dcim/module.html b/netbox/templates/dcim/module.html index 2df2be2521d..e14940ace5c 100644 --- a/netbox/templates/dcim/module.html +++ b/netbox/templates/dcim/module.html @@ -2,7 +2,6 @@ {% load helpers %} {% load plugins %} {% load i18n %} -{% load mptt %} {% block breadcrumbs %} {{ block.super }} diff --git a/netbox/tenancy/api/views.py b/netbox/tenancy/api/views.py index bf729d10673..9068c585a8e 100644 --- a/netbox/tenancy/api/views.py +++ b/netbox/tenancy/api/views.py @@ -1,6 +1,6 @@ from rest_framework.routers import APIRootView -from netbox.api.viewsets import MPTTLockedMixin, NetBoxModelViewSet +from netbox.api.viewsets import NetBoxModelViewSet from tenancy import filtersets from tenancy.models import * @@ -19,7 +19,7 @@ def get_view_name(self): # Tenants # -class TenantGroupViewSet(MPTTLockedMixin, NetBoxModelViewSet): +class TenantGroupViewSet(NetBoxModelViewSet): queryset = TenantGroup.objects.add_related_count( TenantGroup.objects.all(), Tenant, @@ -41,7 +41,7 @@ class TenantViewSet(NetBoxModelViewSet): # Contacts # -class ContactGroupViewSet(MPTTLockedMixin, NetBoxModelViewSet): +class ContactGroupViewSet(NetBoxModelViewSet): queryset = ContactGroup.objects.annotate_contacts() serializer_class = serializers.ContactGroupSerializer filterset_class = filtersets.ContactGroupFilterSet diff --git a/netbox/tenancy/graphql/types.py b/netbox/tenancy/graphql/types.py index ebc0f2e2b2b..aa9c3323440 100644 --- a/netbox/tenancy/graphql/types.py +++ b/netbox/tenancy/graphql/types.py @@ -4,7 +4,7 @@ import strawberry_django from extras.graphql.mixins import ContactsMixin, CustomFieldsMixin, TagsMixin -from netbox.graphql.types import BaseObjectType, NestedGroupObjectType, OrganizationalObjectType, PrimaryObjectType +from netbox.graphql.types import BaseObjectType, NestedLtreeGroupObjectType, OrganizationalObjectType, PrimaryObjectType from tenancy import models from .filters import * @@ -88,11 +88,11 @@ class TenantType(ContactsMixin, PrimaryObjectType): @strawberry_django.type( models.TenantGroup, - fields='__all__', + exclude=['path', 'sort_path'], filters=TenantGroupFilter, pagination=True ) -class TenantGroupType(NestedGroupObjectType): +class TenantGroupType(NestedLtreeGroupObjectType): parent: Annotated['TenantGroupType', strawberry.lazy('tenancy.graphql.types')] | None tenants: list[TenantType] @@ -125,11 +125,11 @@ class ContactRoleType(ContactAssignmentsMixin, OrganizationalObjectType): @strawberry_django.type( models.ContactGroup, - fields='__all__', + exclude=['path', 'sort_path'], filters=ContactGroupFilter, pagination=True ) -class ContactGroupType(NestedGroupObjectType): +class ContactGroupType(NestedLtreeGroupObjectType): parent: Annotated['ContactGroupType', strawberry.lazy('tenancy.graphql.types')] | None contacts: list[ContactType] diff --git a/netbox/tenancy/migrations/0025_ltree_paths.py b/netbox/tenancy/migrations/0025_ltree_paths.py new file mode 100644 index 00000000000..3e49059084c --- /dev/null +++ b/netbox/tenancy/migrations/0025_ltree_paths.py @@ -0,0 +1,124 @@ +"""Replace django-mptt with PostgreSQL ltree for tenancy's hierarchical models. + +The reverse migration is lossy: it re-adds the MPTT columns empty and does not +rebuild the tree. Forward migration is the supported direction. +""" +import django.db.models.deletion +from django.contrib.postgres.indexes import GistIndex +from django.contrib.postgres.operations import CreateExtension +from django.db import migrations, models + +import netbox.models.ltree +from utilities.ltree import InstallLtreeTriggers +from utilities.mptt_to_ltree import assert_paths_populated_sql, populate_paths_sql + +MODELS = ('tenantgroup', 'contactgroup') +TABLES = ('tenancy_tenantgroup', 'tenancy_contactgroup') +LEGACY_FIELDS = ('lft', 'rght', 'tree_id', 'level') + + +class Migration(migrations.Migration): + + dependencies = [ + ('tenancy', '0024_default_ordering_indexes'), + ] + + operations = [ + # Enable the ltree extension first so the migration fails fast if it is missing. + CreateExtension('ltree'), + + # Switch parent from mptt.fields.TreeForeignKey to django.db.models.ForeignKey. + migrations.AlterField( + model_name='contactgroup', name='parent', + field=models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, + related_name='children', to='tenancy.contactgroup', + ), + ), + migrations.AlterField( + model_name='tenantgroup', name='parent', + field=models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, + related_name='children', to='tenancy.tenantgroup', + ), + ), + + # Add path (nullable initially) on both models. + *[ + migrations.AddField( + model_name=m, name='path', + field=netbox.models.ltree.LtreeField(blank=True, editable=False, null=True), + ) + for m in MODELS + ], + # Add sort_path. TenantGroup gets natural_sort collation (matching its name field). + migrations.AddField( + model_name='contactgroup', name='sort_path', + field=models.TextField(blank=True, default='', editable=False), + ), + migrations.AddField( + model_name='tenantgroup', name='sort_path', + field=models.TextField( + blank=True, default='', editable=False, db_collation='natural_sort', + ), + ), + + # Install triggers maintaining both path and sort_path. + *[InstallLtreeTriggers(t, name_column='name') for t in TABLES], + + # Populate path and sort_path for existing rows via per-table recursive CTE. + migrations.RunSQL( + '\n'.join(populate_paths_sql(t, sort_path=True) for t in TABLES), + reverse_sql=migrations.RunSQL.noop, + ), + + # Fail fast if any row still has NULL path (orphan FKs) before the + # AlterField below tries to set NOT NULL inside ALTER COLUMN. + migrations.RunSQL( + '\n'.join(assert_paths_populated_sql(t) for t in TABLES), + reverse_sql=migrations.RunSQL.noop, + ), + + *[ + migrations.AlterField( + model_name=m, name='path', + field=netbox.models.ltree.LtreeField(blank=True, default='', editable=False), + ) + for m in MODELS + ], + + migrations.AlterModelOptions( + name='contactgroup', options={'ordering': ('sort_path',)}, + ), + migrations.AlterModelOptions( + name='tenantgroup', options={'ordering': ('sort_path',)}, + ), + + # Drop legacy (tree_id, lft) indexes and the MPTT columns. + migrations.RemoveIndex(model_name='contactgroup', name='tenancy_contactgroup_tree_d2ce'), + migrations.RemoveIndex(model_name='tenantgroup', name='tenancy_tenantgroup_tree_ifebc'), + *[ + migrations.RemoveField(model_name=m, name=f) + for m in MODELS for f in LEGACY_FIELDS + ], + + # GiST indexes on path. + migrations.AddIndex( + model_name='tenantgroup', + index=GistIndex(fields=['path'], name='tenancy_tenantgroup_path_gist'), + ), + migrations.AddIndex( + model_name='contactgroup', + index=GistIndex(fields=['path'], name='tenancy_contactgroup_path_gist'), + ), + + # Btree indexes on sort_path for ORDER BY listing. + migrations.AddIndex( + model_name='tenantgroup', + index=models.Index(fields=['sort_path'], name='tenancy_tg_sort_path_idx'), + ), + migrations.AddIndex( + model_name='contactgroup', + index=models.Index(fields=['sort_path'], name='tenancy_cg_sort_path_idx'), + ), + ] diff --git a/netbox/tenancy/models/contacts.py b/netbox/tenancy/models/contacts.py index 0e0360cbabc..2f62f2e0800 100644 --- a/netbox/tenancy/models/contacts.py +++ b/netbox/tenancy/models/contacts.py @@ -1,14 +1,15 @@ from django.contrib.contenttypes.fields import GenericForeignKey +from django.contrib.postgres.indexes import GistIndex from django.core.exceptions import ValidationError from django.db import models from django.db.models.expressions import RawSQL from django.urls import reverse from django.utils.translation import gettext_lazy as _ -from netbox.models import ChangeLoggedModel, NestedGroupModel, OrganizationalModel, PrimaryModel +from netbox.models import ChangeLoggedModel, NestedLtreeGroupModel, OrganizationalModel, PrimaryModel from netbox.models.features import CustomFieldsMixin, ExportTemplatesMixin, TagsMixin, has_feature +from netbox.models.ltree import LtreeManager from tenancy.choices import * -from utilities.mptt import TreeManager __all__ = ( 'Contact', @@ -18,39 +19,39 @@ ) -class ContactGroupManager(TreeManager): +class ContactGroupManager(LtreeManager): def annotate_contacts(self): """ Annotate the total number of Contacts belonging to each ContactGroup. - This returns both direct children and children of child groups. Raw SQL is used here to avoid double-counting - contacts which are assigned to multiple child groups of the parent. + Counts contacts assigned to the group itself or any descendant group (via the + ltree `<@` operator on the path column). DISTINCT avoids double-counting + contacts which are assigned to multiple groups in the subtree. """ return self.annotate( contact_count=RawSQL( "SELECT COUNT(DISTINCT m2m.contact_id)" " FROM tenancy_contact_groups m2m" " INNER JOIN tenancy_contactgroup cg ON m2m.contactgroup_id = cg.id" - " WHERE cg.tree_id = tenancy_contactgroup.tree_id" - " AND cg.lft >= tenancy_contactgroup.lft" - " AND cg.lft <= tenancy_contactgroup.rght", + " WHERE cg.path <@ tenancy_contactgroup.path", () ) ) -class ContactGroup(NestedGroupModel): +class ContactGroup(NestedLtreeGroupModel): """ An arbitrary collection of Contacts. """ objects = ContactGroupManager() class Meta: - ordering = ['name'] - # Empty tuple triggers Django migration detection for MPTT indexes - # (see #21016, django-mptt/django-mptt#682) - indexes = () + ordering = ('sort_path',) + indexes = ( + GistIndex(fields=['path'], name='tenancy_contactgroup_path_gist'), + models.Index(fields=['sort_path'], name='tenancy_cg_sort_path_idx'), + ) constraints = ( models.UniqueConstraint( fields=('parent', 'name'), diff --git a/netbox/tenancy/models/tenants.py b/netbox/tenancy/models/tenants.py index 923c9a9ec83..95cdfafb8f1 100644 --- a/netbox/tenancy/models/tenants.py +++ b/netbox/tenancy/models/tenants.py @@ -1,8 +1,9 @@ +from django.contrib.postgres.indexes import GistIndex from django.db import models from django.db.models import Q from django.utils.translation import gettext_lazy as _ -from netbox.models import NestedGroupModel, PrimaryModel +from netbox.models import NestedLtreeGroupModel, PrimaryModel from netbox.models.features import ContactsMixin __all__ = ( @@ -11,7 +12,7 @@ ) -class TenantGroup(NestedGroupModel): +class TenantGroup(NestedLtreeGroupModel): """ An arbitrary collection of Tenants. """ @@ -26,12 +27,14 @@ class TenantGroup(NestedGroupModel): max_length=100, unique=True ) + # sort_path inherits natural_sort collation from `name` automatically (LtreeModelBase). class Meta: - ordering = ['name'] - # Empty tuple triggers Django migration detection for MPTT indexes - # (see #21016, django-mptt/django-mptt#682) - indexes = () + ordering = ('sort_path',) + indexes = ( + GistIndex(fields=['path'], name='tenancy_tenantgroup_path_gist'), + models.Index(fields=['sort_path'], name='tenancy_tg_sort_path_idx'), + ) verbose_name = _('tenant group') verbose_name_plural = _('tenant groups') diff --git a/netbox/utilities/ltree.py b/netbox/utilities/ltree.py new file mode 100644 index 00000000000..5dc495c91d1 --- /dev/null +++ b/netbox/utilities/ltree.py @@ -0,0 +1,293 @@ +""" +Migration support for ltree-based hierarchical models. + +This module holds the schema-level machinery that backs `netbox.models.ltree`: +the PostgreSQL trigger function / trigger SQL and the `InstallLtreeTriggers` +migration operation that installs them. It is kept separate from the model layer +(`netbox.models.ltree`) so that migrations depend only on this DB-level code and +not on the model definitions. + +The paths maintained by these triggers are never computed or mutated from Python; +the model layer only reads `path`/`sort_path` back from the database. +""" +from django.db import migrations + +__all__ = ( + 'InstallLtreeTriggers', +) + + +# Path label is the row's PK zero-padded to 19 chars (max bigint width) so that +# lexicographic ordering of ltree labels matches numeric PK ordering across digit +# boundaries (e.g. "0...09" sorts before "0...10"). +# Per-tree advisory locking (see _lock_tree_roots_sql / LtreeModel "Concurrency"): +# every child insert / move / reparent of a node takes a transaction-level +# advisory lock keyed on the root(s) of the tree(s) it touches, BEFORE reading the +# parent path. A concurrent reparent of an ancestor takes the same key, so the two +# serialize and the AFTER cascade can never miss a row inserted concurrently; +# writes in different trees use different keys and run in parallel. Inserting a new +# root (parent_id IS NULL) takes no lock at all -- it is a race-free singleton tree +# -- so a bulk import of many top-level objects does not accumulate one lock per +# root and cannot exhaust the shared lock table. +_LOCK_TREE_ROOTS_SQL = ''' + -- A brand-new root (INSERT with parent_id IS NULL) starts its own singleton + -- tree that no concurrent transaction can yet see (MVCC: the uncommitted row + -- is invisible) or reference (no other transaction has its PK). For such a + -- row this BEFORE function reads no other row (the parent lookup below is + -- gated on parent_id) and the AFTER cascade fires only on UPDATE, so the + -- insert touches solely its own NEW row -- there is nothing to serialize + -- against, and the advisory lock it would otherwise take can never contend. + -- Skipping it here is what stops a bulk import of many top-level objects from + -- taking one xact-lock per root and exhausting the shared lock table + -- (sized by max_locks_per_transaction). Every other case still locks below: + -- a child insert, any reparent, and a reparent-to-root (TG_OP = UPDATE, where + -- the existing row has a real subtree the AFTER cascade must rewrite). + IF NOT (TG_OP = 'INSERT' AND NEW.parent_id IS NULL) THEN + -- Destination tree root: the parent's root label, or this row's own label + -- when it is (or becomes) a root. The CASE guards against a parent whose path + -- is the empty ltree '' (reachable only via a trigger-bypassing raw write): + -- subltree('', 0, 1) would raise 'invalid positions', so fall back to the + -- child's own label as the lock key rather than aborting the insert/move. + IF NEW.parent_id IS NOT NULL THEN + EXECUTE format( + 'SELECT CASE WHEN nlevel(path) > 0 THEN subltree(path, 0, 1)::text END' + ' FROM %%I WHERE id = $1', + TG_TABLE_NAME + ) INTO dest_root USING NEW.parent_id; + END IF; + dest_root := COALESCE(dest_root, lpad(NEW.id::text, 19, '0')); + -- Source tree root (moves only): this row's current root, which the AFTER + -- cascade will rewrite. + IF TG_OP = 'UPDATE' AND OLD.path IS NOT NULL AND nlevel(OLD.path) > 0 THEN + old_root := subltree(OLD.path, 0, 1)::text; + END IF; + key_dest := hashtextextended(TG_TABLE_NAME || ':' || dest_root, 0); + IF old_root IS NOT NULL AND old_root <> dest_root THEN + -- Cross-tree move: lock both roots, ascending, to avoid deadlock between + -- two concurrent moves that touch the same pair. + key_old := hashtextextended(TG_TABLE_NAME || ':' || old_root, 0); + PERFORM pg_advisory_xact_lock(LEAST(key_dest, key_old)); + PERFORM pg_advisory_xact_lock(GREATEST(key_dest, key_old)); + ELSE + PERFORM pg_advisory_xact_lock(key_dest); + END IF; + END IF; +''' + +_COMPUTE_PATH_ONLY_FN = ''' +CREATE OR REPLACE FUNCTION "{table}_ltree_compute_path_fn"() RETURNS TRIGGER AS $$ +DECLARE + parent_path ltree; + dest_root text; + old_root text; + key_dest bigint; + key_old bigint; +BEGIN +''' + _LOCK_TREE_ROOTS_SQL + ''' + IF NEW.parent_id IS NOT NULL THEN + EXECUTE format('SELECT path FROM %%I WHERE id = $1', TG_TABLE_NAME) + INTO parent_path USING NEW.parent_id; + -- Cycle guard. The Python LtreeModel.save() also rejects cyclic moves, + -- but a QuerySet.update() / bulk_update() bypasses save() entirely, so + -- catch the case here as a last line of defense. A cycle exists iff + -- this row's own label appears anywhere in parent_path (the row would + -- become its own ancestor). Match the label as any segment via lquery. + IF parent_path ~ ('*.' || lpad(NEW.id::text, 19, '0') || '.*')::lquery + OR parent_path = lpad(NEW.id::text, 19, '0')::ltree THEN + RAISE EXCEPTION 'cycle detected: %% cannot be its own ancestor', TG_TABLE_NAME + USING ERRCODE = 'check_violation'; + END IF; + NEW.path := parent_path || lpad(NEW.id::text, 19, '0')::ltree; + ELSE + NEW.path := lpad(NEW.id::text, 19, '0')::ltree; + END IF; + RETURN NEW; +END +$$ LANGUAGE plpgsql; +''' + +_CASCADE_PATH_ONLY_FN = ''' +CREATE OR REPLACE FUNCTION "{table}_ltree_cascade_path_fn"() RETURNS TRIGGER AS $$ +BEGIN + -- `nlevel($2) > 0` guards against an empty OLD.path ('', reachable only via a + -- trigger-bypassing raw write): `path <@ ''` is true for EVERY row, so without + -- this the cascade would rewrite the entire table on one reparent. + EXECUTE format( + 'UPDATE %%I SET path = $1 || subpath(path, nlevel($2))' + ' WHERE nlevel($2) > 0 AND path <@ $2 AND id != $3', + TG_TABLE_NAME + ) USING NEW.path, OLD.path, NEW.id; + RETURN NULL; +END +$$ LANGUAGE plpgsql; +''' + +# For models with order_insertion_by=(name,) — maintain a second text column +# `sort_path` whose value is the chain of ancestor names joined by chr(9) (TAB). +# TAB sorts strictly below any printable character under both the default text +# collation and the ICU `natural_sort` collation (which is `und-u-kn-true`). +# ICU collations with default variable weighting treat U+0001..U+0008 as +# variable-ignorable, so a chr(1) separator under natural_sort would interleave +# children with unrelated roots; TAB is given a primary weight and orders +# deterministically. ORDER BY sort_path then gives MPTT-equivalent +# tree-flatten ordering with siblings in name (collation) order. +# +# The BEFORE trigger fires on INSERT, parent_id changes, and name changes, so +# a rename updates the row's own sort_path; the AFTER trigger then cascades +# the new sort_path into descendants. (django-mptt's `order_insertion_by` +# stops at the renamed node and leaves descendants stale until a manual +# rebuild — NetBox auto-cascades because operators expect renames to flow +# through. `rebuild_sort_paths()` is still available for bulk repair.) +_COMPUTE_PATH_AND_SORT_FN = ''' +CREATE OR REPLACE FUNCTION "{table}_ltree_compute_path_fn"() RETURNS TRIGGER AS $$ +DECLARE + parent_path ltree; + parent_sort_path text; + dest_root text; + old_root text; + key_dest bigint; + key_old bigint; +BEGIN +''' + _LOCK_TREE_ROOTS_SQL + ''' + -- sort_path joins ancestor names with chr(9) (TAB); a literal tab in a name + -- would inject a spurious separator and corrupt sibling ordering for the node + -- and its descendants. LtreeModel.clean() rejects this for forms/serializers; + -- this is the backstop for bulk_create / scripts / raw writes that bypass clean(). + IF position(chr(9) in COALESCE(NEW."{name_col}", '')) > 0 THEN + RAISE EXCEPTION 'name contains a tab character, which is not allowed' + USING ERRCODE = 'check_violation'; + END IF; + IF NEW.parent_id IS NOT NULL THEN + EXECUTE format('SELECT path, sort_path FROM %%I WHERE id = $1', TG_TABLE_NAME) + INTO parent_path, parent_sort_path USING NEW.parent_id; + -- Cycle guard. See _COMPUTE_PATH_ONLY_FN for the rationale; this catches + -- raw UPDATE / bulk_update paths that bypass LtreeModel.save(). + IF parent_path ~ ('*.' || lpad(NEW.id::text, 19, '0') || '.*')::lquery + OR parent_path = lpad(NEW.id::text, 19, '0')::ltree THEN + RAISE EXCEPTION 'cycle detected: %% cannot be its own ancestor', TG_TABLE_NAME + USING ERRCODE = 'check_violation'; + END IF; + NEW.path := parent_path || lpad(NEW.id::text, 19, '0')::ltree; + NEW.sort_path := parent_sort_path || chr(9) || NEW."{name_col}"; + ELSE + NEW.path := lpad(NEW.id::text, 19, '0')::ltree; + NEW.sort_path := NEW."{name_col}"; + END IF; + RETURN NEW; +END +$$ LANGUAGE plpgsql; +''' + +_CASCADE_PATH_AND_SORT_FN = """ +CREATE OR REPLACE FUNCTION "{table}_ltree_cascade_path_fn"() RETURNS TRIGGER AS $$ +BEGIN + -- COALESCE guards against a NULL sort_path slipping in via a raw write that + -- bypassed the BEFORE trigger: without it, length(NULL)/substring(... FROM NULL) + -- would cascade NULL to every descendant's sort_path in one shot. + -- `nlevel($2) > 0` guards against an empty OLD.path ('', reachable only via a + -- trigger-bypassing raw write): `path <@ ''` is true for EVERY row, so without + -- this the cascade would rewrite the entire table on one reparent. + EXECUTE format( + 'UPDATE %%I SET ' + ' path = $1 || subpath(path, nlevel($2)), ' + ' sort_path = COALESCE($4, '''') || substring(COALESCE(sort_path, '''') FROM length(COALESCE($5, '''')) + 1) ' + 'WHERE nlevel($2) > 0 AND path <@ $2 AND id != $3', + TG_TABLE_NAME + ) USING NEW.path, OLD.path, NEW.id, NEW.sort_path, OLD.sort_path; + RETURN NULL; +END +$$ LANGUAGE plpgsql; +""" + +_BEFORE_TRIGGER_PATH_ONLY = ''' +CREATE TRIGGER "{table}_ltree_compute_path" + BEFORE INSERT OR UPDATE OF parent_id ON "{table}" + FOR EACH ROW EXECUTE FUNCTION "{table}_ltree_compute_path_fn"(); +''' + +# For path+sort tables, also fire on UPDATE OF {name_col} so that renaming a +# node recomputes its sort_path. The cascade trigger then propagates the new +# sort_path to descendants. +_BEFORE_TRIGGER_PATH_AND_SORT = ''' +CREATE TRIGGER "{table}_ltree_compute_path" + BEFORE INSERT OR UPDATE OF parent_id, "{name_col}" ON "{table}" + FOR EACH ROW EXECUTE FUNCTION "{table}_ltree_compute_path_fn"(); +''' + +# AFTER trigger fires on the columns that operators / Django write directly +# (parent_id and the name column) — NOT on path or sort_path. The cascade +# function rewrites path/sort_path on descendants in a single statement, and +# because that statement does not touch parent_id or {name_col}, the AFTER +# trigger does not re-fire on those descendant rows. This prevents the +# quadratic re-cascade that would otherwise occur for any deep subtree. +_AFTER_TRIGGER_PATH_ONLY = ''' +CREATE TRIGGER "{table}_ltree_cascade_path" + AFTER UPDATE OF parent_id ON "{table}" + FOR EACH ROW WHEN (OLD.path IS DISTINCT FROM NEW.path) + EXECUTE FUNCTION "{table}_ltree_cascade_path_fn"(); +''' + +_AFTER_TRIGGER_PATH_AND_SORT = ''' +CREATE TRIGGER "{table}_ltree_cascade_path" + AFTER UPDATE OF parent_id, "{name_col}" ON "{table}" + FOR EACH ROW WHEN ( + OLD.path IS DISTINCT FROM NEW.path + OR OLD.sort_path IS DISTINCT FROM NEW.sort_path + ) + EXECUTE FUNCTION "{table}_ltree_cascade_path_fn"(); +''' + + +class InstallLtreeTriggers(migrations.operations.base.Operation): + """ + Install per-table ltree path-maintenance triggers. + + Two row-level triggers are installed on each target table: + + BEFORE INSERT OR UPDATE OF parent_id -> compute NEW.path (and sort_path if applicable) + AFTER UPDATE OF parent_id -> cascade path/sort_path change to descendants + + If `name_column` is provided, the model is expected to have a `sort_path` + text column whose value will be maintained as a chr(9)-separated chain of + ancestor names. This implements MPTT's `order_insertion_by=(name,)` + semantics: insert, reparent, and rename all honor the current value of + `name_column`, with renames cascaded into descendants' sort_paths. + """ + reversible = True + + def __init__(self, table_name, name_column=None): + self.table_name = table_name + self.name_column = name_column + + def state_forwards(self, app_label, state): + pass + + def database_forwards(self, app_label, schema_editor, from_state, to_state): + if self.name_column: + schema_editor.execute(_COMPUTE_PATH_AND_SORT_FN.format( + table=self.table_name, name_col=self.name_column, + )) + schema_editor.execute(_CASCADE_PATH_AND_SORT_FN.format( + table=self.table_name, + )) + schema_editor.execute(_BEFORE_TRIGGER_PATH_AND_SORT.format( + table=self.table_name, name_col=self.name_column, + )) + schema_editor.execute(_AFTER_TRIGGER_PATH_AND_SORT.format( + table=self.table_name, name_col=self.name_column, + )) + else: + schema_editor.execute(_COMPUTE_PATH_ONLY_FN.format(table=self.table_name)) + schema_editor.execute(_CASCADE_PATH_ONLY_FN.format(table=self.table_name)) + schema_editor.execute(_BEFORE_TRIGGER_PATH_ONLY.format(table=self.table_name)) + schema_editor.execute(_AFTER_TRIGGER_PATH_ONLY.format(table=self.table_name)) + + def database_backwards(self, app_label, schema_editor, from_state, to_state): + t = self.table_name + schema_editor.execute(f'DROP TRIGGER IF EXISTS "{t}_ltree_cascade_path" ON "{t}";') + schema_editor.execute(f'DROP TRIGGER IF EXISTS "{t}_ltree_compute_path" ON "{t}";') + schema_editor.execute(f'DROP FUNCTION IF EXISTS "{t}_ltree_cascade_path_fn"();') + schema_editor.execute(f'DROP FUNCTION IF EXISTS "{t}_ltree_compute_path_fn"();') + + def describe(self): + return f"Install ltree path triggers on {self.table_name}" diff --git a/netbox/utilities/mptt_to_ltree.py b/netbox/utilities/mptt_to_ltree.py new file mode 100644 index 00000000000..c669d4b30d1 --- /dev/null +++ b/netbox/utilities/mptt_to_ltree.py @@ -0,0 +1,115 @@ +""" +Reusable SQL builders for migrating a django-mptt tree to a PostgreSQL ltree +`path` (and optional `sort_path`) column. + +NetBox's core hierarchical models moved from django-mptt to ltree in v4.6. The +per-table data backfill that migration performs is identical in shape for every +tree, so the SQL is centralized here rather than copied into each app's +migration. This also gives plugin maintainers a supported path for migrating +their own MPTT models to `netbox.models.ltree.LtreeModel`; from a data migration: + + from django.contrib.postgres.operations import CreateExtension + from django.db import migrations + from utilities.ltree import InstallLtreeTriggers + from utilities.mptt_to_ltree import assert_paths_populated_sql, populate_paths_sql + + operations = [ + CreateExtension('ltree'), + # ... AddField('path', nullable), [AddField('sort_path')], InstallLtreeTriggers(...) ... + migrations.RunSQL( + populate_paths_sql('myplugin_mymodel', sort_path=True), + reverse_sql=migrations.RunSQL.noop, + ), + migrations.RunSQL( + assert_paths_populated_sql('myplugin_mymodel'), + reverse_sql=migrations.RunSQL.noop, + ), + # ... AlterField('path' -> NOT NULL) ... + ] + +The values produced here must stay byte-identical to what the runtime triggers +in `utilities.ltree` maintain: each path label is the row PK zero-padded to +`_PATH_LABEL_WIDTH` chars, and `sort_path` is the chr(9) (TAB) separated chain of +ancestor `name` values. Keep the two modules in sync if either changes. +""" + +__all__ = ( + 'assert_paths_populated_sql', + 'populate_paths_sql', +) + +# Width to which each PK is zero-padded when used as an ltree label. Must match +# the lpad() width used by the trigger functions in utilities.ltree (19 = max +# bigint digit width) so that backfilled paths and trigger-maintained paths sort +# and compare identically. +_PATH_LABEL_WIDTH = 19 + + +def populate_paths_sql(table, *, sort_path=False): + """ + Return SQL that backfills `path` (and `sort_path` when `sort_path=True`) for + every existing row in `table`, walking the tree from its roots + (parent_id IS NULL) downward via a single recursive CTE. + + `path` is the chain of PK labels, each zero-padded to `_PATH_LABEL_WIDTH` + chars. `sort_path` is the chr(9) (TAB) separated chain of ancestor `name` + values, matching the `order_insertion_by=('name',)` semantics the triggers + maintain at runtime. + + !!! warning + The UPDATE takes a row-exclusive lock on the entire table for the + duration of the statement. On large tables this can block writes for + minutes — plan a maintenance window accordingly. + """ + if sort_path: + return f""" +WITH RECURSIVE t(id, parent_id, path, sort_path) AS ( + SELECT id, parent_id, + lpad(id::text, {_PATH_LABEL_WIDTH}, '0')::ltree, + name::text + FROM "{table}" WHERE parent_id IS NULL + UNION ALL + SELECT r.id, r.parent_id, + t.path || lpad(r.id::text, {_PATH_LABEL_WIDTH}, '0')::ltree, + t.sort_path || chr(9) || r.name + FROM "{table}" r JOIN t ON r.parent_id = t.id +) +UPDATE "{table}" SET path = t.path, sort_path = t.sort_path +FROM t WHERE "{table}".id = t.id; +""" + return f""" +WITH RECURSIVE t(id, parent_id, path) AS ( + SELECT id, parent_id, lpad(id::text, {_PATH_LABEL_WIDTH}, '0')::ltree + FROM "{table}" WHERE parent_id IS NULL + UNION ALL + SELECT r.id, r.parent_id, t.path || lpad(r.id::text, {_PATH_LABEL_WIDTH}, '0')::ltree + FROM "{table}" r JOIN t ON r.parent_id = t.id +) +UPDATE "{table}" SET path = t.path FROM t WHERE "{table}".id = t.id; +""" + + +def assert_paths_populated_sql(table): + """ + Return SQL that raises if any row in `table` still has a NULL `path` after + `populate_paths_sql()` runs. + + The recursive CTE only reaches rows whose ancestry chains back to a + `parent_id IS NULL` root, so any row left with a NULL path points (directly + or transitively) at an orphan or cyclic parent_id. Catch that here, naming + the table and row count, rather than letting the subsequent + `AlterField(path -> NOT NULL)` abort opaquely inside ALTER COLUMN. + """ + return f""" +DO $$ +DECLARE missing bigint; +BEGIN + SELECT count(*) INTO missing FROM "{table}" WHERE path IS NULL; + IF missing > 0 THEN + RAISE EXCEPTION + 'ltree backfill left % rows in "{table}" with NULL path; ' + 'likely orphan parent_id references — resolve before re-running ' + 'this migration', missing; + END IF; +END $$; +""" diff --git a/netbox/utilities/query.py b/netbox/utilities/query.py index e672984c0f5..6c26021bf39 100644 --- a/netbox/utilities/query.py +++ b/netbox/utilities/query.py @@ -1,8 +1,6 @@ from django.db.models import Count, OuterRef, QuerySet, Subquery from django.db.models.functions import Coalesce -from utilities.mptt import TreeManager - __all__ = ( 'count_related', 'dict_to_filter_params', @@ -59,14 +57,40 @@ def dict_to_filter_params(d, prefix=''): return params +# TODO: Remove in NetBox v5.0. MPTT support is retained only for plugins that have +# not yet migrated their tree models to netbox.models.ltree.LtreeModel; NetBox core +# no longer uses django-mptt. When MPTT support is dropped, delete this helper and +# its call in reapply_model_ordering(). +def _is_mptt_model(model) -> bool: + """ + Whether `model` is backed by django-mptt (deprecated). MPTT applies its own + tree ordering via the TreeManager, so such querysets must NOT have plain + model-level ordering reapplied after .annotate(). + """ + from mptt.managers import TreeManager + + return any(isinstance(manager, TreeManager) for manager in model._meta.local_managers) + + def reapply_model_ordering(queryset: QuerySet) -> QuerySet: """ Reapply model-level ordering in case it has been lost through .annotate(). https://code.djangoproject.com/ticket/32811 """ - # MPTT-based models are exempt from this; use caution when annotating querysets of these models - if any(isinstance(manager, TreeManager) for manager in queryset.model._meta.local_managers): + # Models ordered by a trigger-maintained ltree column (`sort_path`/`path`) are + # exempt. Key the check on the ordering itself, NOT on LtreeManager presence: + # InventoryItem/InventoryItemTemplate use an LtreeManager only for path + # maintenance but order by a regular column (name), so they DO need their + # ordering reapplied after .annotate() strips it (Django #32811). + ordering = queryset.model._meta.ordering or () + if any(isinstance(f, str) and f.lstrip('-') in ('sort_path', 'path') for f in ordering): + return queryset + + # TODO: Remove in NetBox v5.0 (see _is_mptt_model). Plugins may still use MPTT + # via the generic bulk views, so keep exempting MPTT-based models for now. + if _is_mptt_model(queryset.model): return queryset + if queryset.ordered: return queryset diff --git a/netbox/utilities/templatetags/mptt.py b/netbox/utilities/templatetags/mptt.py deleted file mode 100644 index 96326e9a2e2..00000000000 --- a/netbox/utilities/templatetags/mptt.py +++ /dev/null @@ -1,21 +0,0 @@ -from django import template -from django.utils.html import escape -from django.utils.safestring import mark_safe - -register = template.Library() - - -@register.simple_tag() -def nested_tree(obj): - """ - Renders the entire hierarchy of a recursively-nested object (such as Region or SiteGroup). - """ - if not obj: - return mark_safe('—') - - nodes = obj.get_ancestors(include_self=True) - return mark_safe( - ' / '.join( - f'{escape(node)}' for node in nodes - ) - ) diff --git a/netbox/utilities/testing/filtersets.py b/netbox/utilities/testing/filtersets.py index 7b1a7d03877..46640f94b58 100644 --- a/netbox/utilities/testing/filtersets.py +++ b/netbox/utilities/testing/filtersets.py @@ -6,10 +6,10 @@ from django.contrib.contenttypes.models import ContentType from django.db.models import ForeignKey, ManyToManyField, ManyToManyRel, ManyToOneRel, OneToOneRel from django.utils.module_loading import import_string -from mptt.models import MPTTModel from taggit.managers import TaggableManager from extras.filters import TagFilter +from netbox.models.ltree import LtreeModel from utilities.filters import MultiValueContentTypeFilter, TreeNodeMultipleChoiceFilter __all__ = ( @@ -20,10 +20,8 @@ EXEMPT_MODEL_FIELDS = ( 'comments', 'custom_field_data', - 'level', # MPTT - 'lft', # MPTT - 'rght', # MPTT - 'tree_id', # MPTT + 'path', # ltree, trigger-maintained + 'sort_path', # ltree, trigger-maintained ) @@ -59,8 +57,8 @@ def get_filters_for_model_field(self, field): if field.related_model is ContentType: return [(None, None)] - # ForeignKey to an MPTT-enabled model - if issubclass(field.related_model, MPTTModel) and field.model is not field.related_model: + # ForeignKey to an ltree-backed hierarchical model + if issubclass(field.related_model, LtreeModel) and field.model is not field.related_model: return [(f'{filter_name}_id', TreeNodeMultipleChoiceFilter)] return [(f'{filter_name}_id', django_filters.ModelMultipleChoiceFilter)] diff --git a/netbox/utilities/tests/test_filters.py b/netbox/utilities/tests/test_filters.py index 591934aee89..ea30aac6752 100644 --- a/netbox/utilities/tests/test_filters.py +++ b/netbox/utilities/tests/test_filters.py @@ -2,7 +2,6 @@ from django.conf import settings from django.db import models from django.test import TestCase -from mptt.fields import TreeForeignKey from taggit.managers import TaggableManager from dcim.choices import * @@ -113,9 +112,11 @@ class DummyModel(models.Model): integerfield = models.IntegerField() macaddressfield = MACAddressField() timefield = models.TimeField() - treeforeignkeyfield = TreeForeignKey( + treeforeignkeyfield = models.ForeignKey( to='self', - on_delete=models.CASCADE + on_delete=models.CASCADE, + null=True, + blank=True, ) tags = TaggableManager(through=TaggedItem) diff --git a/netbox/utilities/tests/test_ltree.py b/netbox/utilities/tests/test_ltree.py new file mode 100644 index 00000000000..441bdd04e7e --- /dev/null +++ b/netbox/utilities/tests/test_ltree.py @@ -0,0 +1,911 @@ +"""Tests for the ltree-based hierarchical model infrastructure.""" +from django.contrib.contenttypes.models import ContentType +from django.db import connection +from django.test import TestCase + +from core.models import ObjectChange +from dcim.models import Region, Site +from tenancy.models import Contact, ContactGroup + + +def _path(*pks): + """Construct the expected ltree path value from a sequence of PKs. + + Mirrors the trigger's zero-padded label scheme (19 chars per label). + """ + return '.'.join(str(pk).zfill(19) for pk in pks) + + +class LtreeTriggerTests(TestCase): + """Verify per-row PostgreSQL triggers maintain `path` correctly.""" + + def test_insert_root_path(self): + r = Region.objects.create(name='Root', slug='root') + self.assertEqual(r.path, _path(r.pk)) + + def test_insert_child_path(self): + r = Region.objects.create(name='Root', slug='root') + c = Region.objects.create(parent=r, name='Child', slug='child') + self.assertEqual(c.path, _path(r.pk, c.pk)) + + def test_grandchild_path(self): + r = Region.objects.create(name='R', slug='r') + c = Region.objects.create(parent=r, name='C', slug='c') + g = Region.objects.create(parent=c, name='G', slug='g') + self.assertEqual(g.path, _path(r.pk, c.pk, g.pk)) + + def test_move_cascades_to_descendants(self): + r = Region.objects.create(name='R', slug='r') + c = Region.objects.create(parent=r, name='C', slug='c') + g = Region.objects.create(parent=c, name='G', slug='g') + c.parent = None + c.save() + c.refresh_from_db() + g.refresh_from_db() + self.assertEqual(c.path, _path(c.pk)) + self.assertEqual(g.path, _path(c.pk, g.pk)) + + def test_bulk_create_populates_paths(self): + """BEFORE INSERT trigger fires on bulk_create, populating path.""" + root = Region.objects.create(name='R', slug='r-bulk') + children = Region.objects.bulk_create([ + Region(parent=root, name=f'C{i}', slug=f'c{i}-bulk') for i in range(3) + ]) + for child in children: + child.refresh_from_db() + self.assertEqual(child.path, _path(root.pk, child.pk)) + + def test_queryset_update_with_parent_id_cascades(self): + """Raw .update() that changes parent_id still fires triggers.""" + r1 = Region.objects.create(name='R1', slug='r1-up') + r2 = Region.objects.create(name='R2', slug='r2-up') + c = Region.objects.create(parent=r1, name='C', slug='c-up') + g = Region.objects.create(parent=c, name='G', slug='g-up') + + Region.objects.filter(pk=c.pk).update(parent=r2) + c.refresh_from_db() + g.refresh_from_db() + self.assertEqual(c.path, _path(r2.pk, c.pk)) + self.assertEqual(g.path, _path(r2.pk, c.pk, g.pk)) + + def test_gist_index_exists(self): + """Every ltree-backed table has a GiST index on path.""" + expected = { + 'dcim_region_path_gist', + 'dcim_sitegroup_path_gist', + 'dcim_location_path_gist', + 'dcim_devicerole_path_gist', + 'dcim_platform_path_gist', + 'dcim_inventoryitem_path_gist', + 'dcim_inv_item_tmpl_path_gist', + 'dcim_modulebay_path_gist', + 'tenancy_tenantgroup_path_gist', + 'tenancy_contactgroup_path_gist', + 'wireless_lan_grp_path_gist', + } + with connection.cursor() as cursor: + cursor.execute(""" + SELECT indexname FROM pg_indexes + WHERE indexname = ANY(%s) AND indexdef LIKE '%%USING gist%%' + """, [list(expected)]) + found = {row[0] for row in cursor.fetchall()} + self.assertSetEqual(found, expected) + + +class AdvisoryLockScopeTests(TestCase): + """ + Pin where the BEFORE trigger takes its per-tree advisory lock. + + A new root (INSERT with parent_id IS NULL) is a race-free singleton tree and + must take NO lock, so a bulk import of many top-level objects cannot exhaust + the shared lock table. Every other write (child insert, reparent-to-root) + must still lock per tree — the companion assertions keep the optimization from + silently over-broadening to cases that need serialization. + + `pg_locks` is cluster-wide and advisory *xact* locks are held until the + TestCase transaction ends, so each test measures the DELTA in this backend's + advisory-lock count across a single operation rather than an absolute count. + """ + + @staticmethod + def _advisory_lock_count(): + with connection.cursor() as cursor: + cursor.execute( + "SELECT count(*) FROM pg_locks " + "WHERE locktype = 'advisory' AND pid = pg_backend_pid()" + ) + return cursor.fetchone()[0] + + def test_root_insert_takes_no_lock(self): + before = self._advisory_lock_count() + Region.objects.create(name='Root', slug='root-lock') + self.assertEqual(self._advisory_lock_count() - before, 0) + + def test_child_insert_takes_one_lock(self): + root = Region.objects.create(name='Root', slug='root-lock2') + before = self._advisory_lock_count() + Region.objects.create(parent=root, name='Child', slug='child-lock2') + self.assertEqual(self._advisory_lock_count() - before, 1) + + def test_reparent_to_root_takes_lock(self): + root = Region.objects.create(name='Root', slug='root-lock3') + child = Region.objects.create(parent=root, name='Child', slug='child-lock3') + before = self._advisory_lock_count() + child.parent = None + child.save() + # An existing row promoted to root still has a real subtree to rewrite, so + # it must lock (its own new root key, plus the old tree's root for the move). + self.assertGreaterEqual(self._advisory_lock_count() - before, 1) + + +class LtreeAPIParityTests(TestCase): + """Verify the MPTTModel-compatible API surface.""" + + @classmethod + def setUpTestData(cls): + # Build: root -> mid -> leaf + # -> leaf2 (sibling of mid's child) + cls.root = Region.objects.create(name='Root', slug='root-api') + cls.mid = Region.objects.create(parent=cls.root, name='Mid', slug='mid-api') + cls.leaf = Region.objects.create(parent=cls.mid, name='Leaf', slug='leaf-api') + cls.leaf2 = Region.objects.create(parent=cls.mid, name='Leaf2', slug='leaf2-api') + + def test_level(self): + self.assertEqual(self.root.level, 0) + self.assertEqual(self.mid.level, 1) + self.assertEqual(self.leaf.level, 2) + self.assertEqual(self.leaf.get_level(), 2) + + def test_get_ancestors(self): + ancestors = list(self.leaf.get_ancestors().values_list('name', flat=True)) + self.assertEqual(ancestors, ['Root', 'Mid']) + with_self = list(self.leaf.get_ancestors(include_self=True).values_list('name', flat=True)) + self.assertEqual(with_self, ['Root', 'Mid', 'Leaf']) + + def test_get_descendants(self): + descendants = sorted(self.root.get_descendants().values_list('name', flat=True)) + self.assertEqual(descendants, ['Leaf', 'Leaf2', 'Mid']) + + def test_get_children(self): + children = sorted(self.root.get_children().values_list('name', flat=True)) + self.assertEqual(children, ['Mid']) + + +class CycleValidationTests(TestCase): + """clean() and save() must refuse to assign self or a descendant as parent.""" + + def test_cycle_raises(self): + from django.core.exceptions import ValidationError + a = Region.objects.create(name='A', slug='a-cyc') + b = Region.objects.create(parent=a, name='B', slug='b-cyc') + Region.objects.create(parent=b, name='C', slug='c-cyc') + a.parent = b + with self.assertRaises(ValidationError): + a.full_clean() + + def test_cycle_raises_on_save_without_clean(self): + # The save()-level guard mirrors django-mptt's InvalidMove: a cyclic move + # is rejected even when full_clean() is bypassed (scripts, bulk callers). + from django.core.exceptions import ValidationError + a = Region.objects.create(name='A', slug='a-cyc2') + b = Region.objects.create(parent=a, name='B', slug='b-cyc2') + Region.objects.create(parent=b, name='C', slug='c-cyc2') + a.parent = b + with self.assertRaises(ValidationError): + a.save() + # The rejected move must leave the tree unchanged. + a.refresh_from_db() + self.assertIsNone(a.parent_id) + + def test_self_parent_raises_on_save(self): + from django.core.exceptions import ValidationError + a = Region.objects.create(name='A', slug='a-self') + a.parent = a + with self.assertRaises(ValidationError): + a.save() + + def test_move_to_unrelated_parent_is_allowed(self): + # A legitimate move (target is neither self nor a descendant) must succeed. + a = Region.objects.create(name='A', slug='a-ok') + b = Region.objects.create(name='B', slug='b-ok') + child = Region.objects.create(parent=a, name='Child', slug='child-ok') + child.parent = b + child.save() + child.refresh_from_db() + self.assertEqual(child.parent_id, b.pk) + self.assertEqual(child.path, _path(b.pk, child.pk)) + + +class SortPathTests(TestCase): + """ + Verify that sort_path produces tree-flatten output with siblings in name + order, mirroring MPTT's `order_insertion_by=('name',)` behavior. + """ + + def test_siblings_in_name_order_regardless_of_insertion_order(self): + # Create siblings out of name order + Region.objects.create(name='Zebra', slug='zebra-sp') + Region.objects.create(name='Aardvark', slug='aardvark-sp') + buffalo = Region.objects.create(name='Buffalo', slug='buffalo-sp') + + # Children of Buffalo also out of order + Region.objects.create(parent=buffalo, name='Zoo', slug='b-zoo-sp') + Region.objects.create(parent=buffalo, name='Apex', slug='b-apex-sp') + + ordered = list( + Region.objects.filter(slug__endswith='-sp') + .order_by('sort_path') + .values_list('name', flat=True) + ) + # Tree-flatten with siblings in name order: + # Aardvark, Buffalo (parent), Apex (child), Zoo (child), Zebra + self.assertEqual(ordered, ['Aardvark', 'Buffalo', 'Apex', 'Zoo', 'Zebra']) + + def test_default_ordering_is_sort_path(self): + """Region.objects.all() uses sort_path-based ordering by default.""" + Region.objects.create(name='B', slug='b-default') + Region.objects.create(name='A', slug='a-default') + names = list( + Region.objects.filter(slug__endswith='-default').values_list('name', flat=True) + ) + self.assertEqual(names, ['A', 'B']) + + def test_get_descendants_returns_siblings_in_name_order(self): + """ + For models with a sort_path column, get_descendants() must return + descendants with siblings in name order (matching MPTT's + order_insertion_by behavior), not insertion/PK order. + """ + root = Region.objects.create(name='Root', slug='root-gdord') + # Insert siblings out of name order + Region.objects.create(parent=root, name='Zebra', slug='z-gdord') + Region.objects.create(parent=root, name='Aardvark', slug='a-gdord') + Region.objects.create(parent=root, name='Buffalo', slug='b-gdord') + names = list(root.get_descendants().values_list('name', flat=True)) + self.assertEqual(names, ['Aardvark', 'Buffalo', 'Zebra']) + + def test_get_children_returns_in_name_order(self): + root = Region.objects.create(name='Root', slug='root-gcord') + Region.objects.create(parent=root, name='Zebra', slug='z-gcord') + Region.objects.create(parent=root, name='Aardvark', slug='a-gcord') + names = list(root.get_children().values_list('name', flat=True)) + self.assertEqual(names, ['Aardvark', 'Zebra']) + + +class AddRelatedCountTests(TestCase): + """add_related_count must cumulate across subtrees via path <@.""" + + def test_cumulative_fk_count(self): + root = Region.objects.create(name='R', slug='r-arc') + child = Region.objects.create(parent=root, name='C', slug='c-arc') + Site.objects.create(name='S1', slug='s1-arc', region=child) + Site.objects.create(name='S2', slug='s2-arc', region=root) + + qs = Region.objects.add_related_count( + Region.objects.filter(slug__endswith='-arc'), + Site, 'region', 'site_count', cumulative=True, + ) + counts = {r.name: r.site_count for r in qs} + # root sees both sites (direct + via child) + self.assertEqual(counts['R'], 2) + self.assertEqual(counts['C'], 1) + + def test_cumulative_m2m_count(self): + """ + Cumulative count over an M2M relation walks the subtree via + path <@, joining the through table by the correct columns. + + Regression: previously the JOINs swapped m2m_column_name() and + m2m_reverse_name(), producing wrong counts on M2M relations. + """ + root = ContactGroup.objects.create(name='Root', slug='root-m2m') + child = ContactGroup.objects.create(parent=root, name='Child', slug='child-m2m') + leaf = ContactGroup.objects.create(parent=child, name='Leaf', slug='leaf-m2m') + # 3 contacts spread across the subtree (one per node) + c_root = Contact.objects.create(name='CR') + c_child = Contact.objects.create(name='CC') + c_leaf = Contact.objects.create(name='CL') + c_root.groups.add(root) + c_child.groups.add(child) + c_leaf.groups.add(leaf) + + qs = ContactGroup.objects.add_related_count( + ContactGroup.objects.filter(slug__endswith='-m2m'), + Contact, 'groups', 'contact_count', cumulative=True, + ) + counts = {g.name: g.contact_count for g in qs} + self.assertEqual(counts['Root'], 3) + self.assertEqual(counts['Child'], 2) + self.assertEqual(counts['Leaf'], 1) + + def test_noncumulative_fk_count(self): + """Non-cumulative FK count includes only rows pointing directly at the node.""" + root = Region.objects.create(name='R', slug='r-ncfk') + child = Region.objects.create(parent=root, name='C', slug='c-ncfk') + Site.objects.create(name='S1', slug='s1-ncfk', region=child) + Site.objects.create(name='S2', slug='s2-ncfk', region=root) + + qs = Region.objects.add_related_count( + Region.objects.filter(slug__endswith='-ncfk'), + Site, 'region', 'site_count', cumulative=False, + ) + counts = {r.name: r.site_count for r in qs} + # Each node counts only its own directly-assigned sites (no subtree rollup). + self.assertEqual(counts['R'], 1) + self.assertEqual(counts['C'], 1) + + def test_noncumulative_m2m_count(self): + """Non-cumulative M2M count includes only directly-assigned rows, not the subtree.""" + root = ContactGroup.objects.create(name='Root', slug='root-ncm2m') + child = ContactGroup.objects.create(parent=root, name='Child', slug='child-ncm2m') + c_root = Contact.objects.create(name='CR-nc') + c_child = Contact.objects.create(name='CC-nc') + c_root.groups.add(root) + c_child.groups.add(child) + + qs = ContactGroup.objects.add_related_count( + ContactGroup.objects.filter(slug__endswith='-ncm2m'), + Contact, 'groups', 'contact_count', cumulative=False, + ) + counts = {g.name: g.contact_count for g in qs} + self.assertEqual(counts['Root'], 1) + self.assertEqual(counts['Child'], 1) + + +class SaveUpdateFieldsTests(TestCase): + """ + Regression: when save(update_fields=...) excludes parent, _loaded_parent_id + must not advance, otherwise a subsequent full save() will mis-detect the + parent change as already-applied and leave path stale in memory. + """ + + def test_partial_save_then_full_save_refreshes_path(self): + r1 = Region.objects.create(name='R1', slug='r1-uf') + r2 = Region.objects.create(name='R2', slug='r2-uf') + obj = Region.objects.create(name='Obj', slug='obj-uf') + original_path = obj.path + + # Reparent in memory but persist a different field only: + obj.parent = r1 + obj.name = 'Obj-renamed' + obj.save(update_fields=['name']) + + # DB parent_id is still NULL — confirm: + db_parent = Region.objects.values_list('parent_id', flat=True).get(pk=obj.pk) + self.assertIsNone(db_parent) + self.assertEqual( + Region.objects.values_list('path', flat=True).get(pk=obj.pk), + original_path, + ) + + # Now a full save persists the new parent — path must refresh: + obj.parent = r2 + obj.save() + db_path = Region.objects.values_list('path', flat=True).get(pk=obj.pk) + self.assertEqual(db_path, _path(r2.pk, obj.pk)) + self.assertEqual(obj.path, db_path, "in-memory path is stale after full save") + + +class BulkCreateOrderingTests(TestCase): + """ + The BEFORE INSERT trigger looks up parent.path via subquery per row. + In bulk_create the trigger fires per row in list order, so a parent + placed in the same batch must precede its children. + """ + + def test_parent_before_child_in_same_batch(self): + root = Region.objects.create(name='R', slug='r-bcord') + # Parent BEFORE child in the list — both get correct paths + parent_pending = Region(parent=root, name='Mid', slug='mid-bcord') + child_pending = Region(parent=parent_pending, name='Leaf', slug='leaf-bcord') + # parent_pending isn't yet saved, so child_pending.parent_id is None; + # set the parent reference after the parent is saved. + Region.objects.bulk_create([parent_pending]) + parent_pending.refresh_from_db() + child_pending.parent = parent_pending + Region.objects.bulk_create([child_pending]) + child_pending.refresh_from_db() + self.assertEqual(child_pending.path, _path(root.pk, parent_pending.pk, child_pending.pk)) + + def test_bulk_create_rejects_child_before_parent_in_same_batch(self): + """The guard refuses misordered batches instead of writing bad paths.""" + unsaved_parent = Region(name='P', slug='p-bcrej') + unsaved_child = Region(parent=unsaved_parent, name='C', slug='c-bcrej') + with self.assertRaises(ValueError) as ctx: + Region.objects.bulk_create([unsaved_child, unsaved_parent]) + self.assertIn('unsaved parent', str(ctx.exception)) + + def test_bulk_create_rejects_unsaved_parent_earlier_in_batch(self): + """ + An unsaved parent placed earlier in the batch must also be rejected: + Django binds VALUES from the child's parent_id BEFORE the parent's + RETURNING-assigned pk lands, so the child would be inserted with + parent_id=NULL and silently stored as a root. + """ + unsaved_parent = Region(name='P', slug='p-bcearly') + unsaved_child = Region(parent=unsaved_parent, name='C', slug='c-bcearly') + with self.assertRaises(ValueError) as ctx: + Region.objects.bulk_create([unsaved_parent, unsaved_child]) + self.assertIn('unsaved parent', str(ctx.exception)) + # Nothing should have been written. + self.assertFalse(Region.objects.filter(slug__in=('p-bcearly', 'c-bcearly')).exists()) + + def test_bulk_create_rejects_unsaved_parent_not_in_batch(self): + """ + An unsaved parent that isn't in the batch must be rejected with a clear + ValueError; otherwise the child's parent_id is None at INSERT time and + the BEFORE trigger silently stores the row as a root. + """ + external_parent = Region(name='X', slug='x-bcok') # not in the batch + with self.assertRaises(ValueError) as ctx: + Region.objects.bulk_create([Region(parent=external_parent, name='Y', slug='y-bcok')]) + self.assertIn('unsaved parent', str(ctx.exception)) + # Nothing should have been written. + self.assertFalse(Region.objects.filter(slug='y-bcok').exists()) + + +class TreeNodeFilterTests(TestCase): + """ + Regression: the FK branch of TreeNodeFilter.filter() previously destructured + q_filter.children as (str, value) tuples, which crashed for compound Q + match types (DESCENDANTS, ANCESTORS, SIBLINGS, SELF_AND_DESCENDANTS). The + FK branch must resolve via __in like the M2M / M2O branches. + """ + + @classmethod + def setUpTestData(cls): + from tenancy.models import Tenant, TenantGroup + cls.Tenant = Tenant + cls.TenantGroup = TenantGroup + cls.root = TenantGroup.objects.create(name='Root', slug='root-tnf') + cls.mid = TenantGroup.objects.create(parent=cls.root, name='Mid', slug='mid-tnf') + cls.leaf = TenantGroup.objects.create(parent=cls.mid, name='Leaf', slug='leaf-tnf') + cls.sibling = TenantGroup.objects.create(parent=cls.root, name='Sibling', slug='sibling-tnf') + + cls.t_root = Tenant.objects.create(name='TRoot', slug='troot-tnf', group=cls.root) + cls.t_mid = Tenant.objects.create(name='TMid', slug='tmid-tnf', group=cls.mid) + cls.t_leaf = Tenant.objects.create(name='TLeaf', slug='tleaf-tnf', group=cls.leaf) + cls.t_sibling = Tenant.objects.create(name='TSib', slug='tsib-tnf', group=cls.sibling) + + def _filter(self, match_type): + from netbox.graphql.filter_lookups import TreeNodeFilter, TreeNodeMatch + tnf = TreeNodeFilter(id=self.mid.pk, match_type=getattr(TreeNodeMatch, match_type)) + # `filter` is decorated by @strawberry_django.filter_field; its wrapper + # asserts info is not None. The undecorated body is on `_unbound_wrapped_func`. + inner = TreeNodeFilter.filter._unbound_wrapped_func + qs, q = inner(tnf, info=None, queryset=self.Tenant.objects.all(), prefix='group__') + return list(qs.filter(q).values_list('name', flat=True)) + + def test_descendants_strict(self): + # DESCENDANTS of `mid` = leaf only (Mid itself excluded) + names = self._filter('DESCENDANTS') + self.assertEqual(sorted(names), ['TLeaf']) + + def test_self_and_descendants(self): + names = self._filter('SELF_AND_DESCENDANTS') + self.assertEqual(sorted(names), ['TLeaf', 'TMid']) + + def test_ancestors(self): + names = self._filter('ANCESTORS') + # Ancestors of Mid = Root only (Mid itself excluded) + self.assertEqual(sorted(names), ['TRoot']) + + def test_siblings(self): + # Siblings of Mid (within Root) = Sibling + names = self._filter('SIBLINGS') + self.assertEqual(sorted(names), ['TSib']) + + +class DescendantLookupSemanticsTests(TestCase): + """ + path__descendant is strict (path <@ rhs AND path != rhs); the inclusive + form is path__descendant_or_equal. Previously both were inclusive. + """ + + def test_strict_descendant_excludes_self(self): + root = Region.objects.create(name='Root', slug='root-dls') + Region.objects.create(parent=root, name='Kid', slug='kid-dls') + strict = list( + Region.objects.filter(path__descendant=root.path) + .values_list('name', flat=True) + ) + self.assertEqual(sorted(strict), ['Kid']) + inclusive = list( + Region.objects.filter(path__descendant_or_equal=root.path) + .values_list('name', flat=True) + ) + self.assertEqual(sorted(inclusive), ['Kid', 'Root']) + + +class AncestorLookupSemanticsTests(TestCase): + """ + path__ancestor is strict (path @> rhs AND path != rhs); the inclusive form + is path__ancestor_or_equal. + """ + + def test_strict_ancestor_excludes_self(self): + root = Region.objects.create(name='Root', slug='root-als') + kid = Region.objects.create(parent=root, name='Kid', slug='kid-als') + strict = list( + Region.objects.filter(path__ancestor=kid.path) + .values_list('name', flat=True) + ) + self.assertEqual(sorted(strict), ['Root']) + inclusive = list( + Region.objects.filter(path__ancestor_or_equal=kid.path) + .values_list('name', flat=True) + ) + self.assertEqual(sorted(inclusive), ['Kid', 'Root']) + + +class RenameCascadesSortPathTests(TestCase): + """ + Renaming a node updates its own sort_path AND cascades into descendants' + sort_paths via the AFTER trigger. (Diverges from MPTT order_insertion_by, + which leaves descendants stale until manual rebuild.) + """ + + def test_rename_cascades_into_descendants(self): + parent = Region.objects.create(name='Bravo', slug='bravo-rcsp') + mid = Region.objects.create(parent=parent, name='Mid', slug='mid-rcsp') + leaf = Region.objects.create(parent=mid, name='Leaf', slug='leaf-rcsp') + + parent.name = 'Zulu' + parent.save() + + parent.refresh_from_db() + mid.refresh_from_db() + leaf.refresh_from_db() + self.assertEqual(parent.sort_path, 'Zulu') + self.assertEqual(mid.sort_path, f'Zulu{chr(9)}Mid') + self.assertEqual(leaf.sort_path, f'Zulu{chr(9)}Mid{chr(9)}Leaf') + # Paths unchanged — only sort_path moved. + self.assertEqual(mid.path, _path(parent.pk, mid.pk)) + self.assertEqual(leaf.path, _path(parent.pk, mid.pk, leaf.pk)) + + def test_rename_does_not_affect_unrelated_subtree(self): + # Two roots; renaming one must not touch the other's sort_path. + a = Region.objects.create(name='AA', slug='aa-iso') + Region.objects.create(parent=a, name='AKid', slug='akid-iso') + b = Region.objects.create(name='BB', slug='bb-iso') + b_kid = Region.objects.create(parent=b, name='BKid', slug='bkid-iso') + + a.name = 'AAren' + a.save() + + b_kid.refresh_from_db() + self.assertEqual(b_kid.sort_path, f'BB{chr(9)}BKid') + + +class RebuildSortPathsTests(TestCase): + """rebuild_sort_paths() is still available for repair after raw SQL writes.""" + + def test_rebuild_after_raw_update(self): + parent = Region.objects.create(name='Bravo', slug='bravo-rsp') + mid = Region.objects.create(parent=parent, name='Mid', slug='mid-rsp') + # Raw .update() with only the name column bypasses the BEFORE trigger + # (its column list is parent_id + name, but the trigger is keyed on + # name in the SET clause). Actually update() on `name` DOES fire the + # trigger now — so to simulate a bypass we corrupt sort_path directly. + Region.objects.filter(pk=parent.pk).update(sort_path='garbage') + Region.objects.filter(pk=mid.pk).update(sort_path='also-garbage') + + Region.rebuild_sort_paths() + parent.refresh_from_db() + mid.refresh_from_db() + self.assertEqual(parent.sort_path, 'Bravo') + self.assertEqual(mid.sort_path, f'Bravo{chr(9)}Mid') + + def test_raises_without_sort_path(self): + # InventoryItem uses LtreeModel but doesn't have a sort_path column. + from dcim.models import InventoryItem + with self.assertRaises(NotImplementedError): + InventoryItem.rebuild_sort_paths() + + +class SortPathRefreshTests(TestCase): + """ + save() must refresh the in-memory `sort_path` (not just `path`) after insert + and reparent, so callers (notably change logging) never snapshot a stale value. + """ + + def test_create_refreshes_sort_path(self): + root = Region.objects.create(name='Root', slug='root-spr') + child = Region.objects.create(name='Kid', slug='kid-spr', parent=root) + db_sort_path = Region.objects.values_list('sort_path', flat=True).get(pk=child.pk) + self.assertEqual(child.sort_path, db_sort_path) + self.assertEqual(child.sort_path, f'Root{chr(9)}Kid') + + def test_reparent_refreshes_sort_path(self): + a = Region.objects.create(name='Alpha', slug='alpha-spr') + b = Region.objects.create(name='Bravo', slug='bravo-spr') + child = Region.objects.create(name='Kid', slug='kid2-spr', parent=a) + child.parent = b + child.save() + db_sort_path = Region.objects.values_list('sort_path', flat=True).get(pk=child.pk) + self.assertEqual(child.sort_path, db_sort_path) + self.assertEqual(child.sort_path, f'Bravo{chr(9)}Kid') + + def test_rename_refreshes_sort_path(self): + # The trigger rewrites sort_path on a name change; the in-memory instance + # must reflect it without a manual refresh_from_db(). + root = Region.objects.create(name='Root', slug='root-rn') + root.name = 'Renamed' + root.save() + db_sort_path = Region.objects.values_list('sort_path', flat=True).get(pk=root.pk) + self.assertEqual(root.sort_path, db_sort_path) + self.assertEqual(root.sort_path, 'Renamed') + + +class ReapplyModelOrderingTests(TestCase): + """Ltree-backed models must be exempt from reapply_model_ordering().""" + + def test_ltree_model_is_exempt(self): + from utilities.query import reapply_model_ordering + # Clear ordering so a non-exempt model would be re-ordered by Meta.ordering. + qs = Region.objects.all().order_by() + result = reapply_model_ordering(qs) + # The LtreeManager is inherited from the abstract base (not in + # local_managers), so the exemption must still apply and return qs as-is. + self.assertIs(result, qs) + + def test_mptt_model_detection(self): + # MPTT support is retained for plugins (NestedGroupModel stays MPTT-backed); + # reapply_model_ordering() must keep exempting such models. See _is_mptt_model. + # TODO: Remove in NetBox v5.0 alongside MPTT support. + from netbox.models import NestedGroupModel + from utilities.query import _is_mptt_model + + self.assertTrue(_is_mptt_model(NestedGroupModel)) + # An ltree-backed model must NOT be misdetected as MPTT. + self.assertFalse(_is_mptt_model(Region)) + + +class AddRelatedCountErrorTests(TestCase): + """ + add_related_count() must not raise at queryset-build time for unresolvable + rel_field — many call sites bind the annotation as a class attribute at + module import. The annotation is still attached using the Django default + column naming, and any error surfaces at evaluation time. + """ + + def test_unknown_field_does_not_raise_at_build(self): + qs = Region.objects.add_related_count( + Region.objects.all(), Region, 'not_a_field', 'bogus_count', cumulative=True + ) + self.assertIn('bogus_count', qs.query.annotations) + + +class ChangeLogExclusionTests(TestCase): + """ + Trigger-maintained columns (`path`, `sort_path`) must be excluded from change + log diffs, and the postchange snapshot must capture the refreshed values. + """ + + def test_sort_path_excluded_from_diff(self): + oc = ObjectChange() + oc.changed_object_type = ContentType.objects.get_for_model(Region) + self.assertIn('path', oc.diff_exclude_fields) + self.assertIn('sort_path', oc.diff_exclude_fields) + + def test_reparent_postchange_snapshot_matches_db(self): + a = Region.objects.create(name='Alpha', slug='alpha-cl') + b = Region.objects.create(name='Bravo', slug='bravo-cl') + child = Region.objects.create(name='Kid', slug='kid-cl', parent=a) + # Reload so the prechange snapshot reflects the persisted state. + child = Region.objects.get(pk=child.pk) + child.snapshot() + child.parent = b + child.save() + oc = child.to_objectchange('update') + db = Region.objects.values('path', 'sort_path').get(pk=child.pk) + self.assertEqual(oc.postchange_data['path'], db['path']) + self.assertEqual(oc.postchange_data['sort_path'], db['sort_path']) + # path/sort_path are excluded, so they must not surface in the cleaned diff data. + self.assertNotIn('sort_path', oc.postchange_data_clean) + self.assertNotIn('path', oc.postchange_data_clean) + + +class MPTTChangeLogExclusionTests(TestCase): + """ + ObjectChange.diff_exclude_fields must hide MPTT bookkeeping columns + (lft/rght/tree_id/level) for plugin models still using the deprecated + MPTT-backed NestedGroupModel, in addition to ltree's path/sort_path. + """ + + def test_diff_exclude_fields_for_mptt_subclass(self): + from unittest.mock import MagicMock + + from mptt.models import MPTTModel + + class _FakeMPTT(MPTTModel): + class Meta: + abstract = True + app_label = 'tests' + + oc = ObjectChange() + fake_ct = MagicMock() + fake_ct.model_class.return_value = _FakeMPTT + # Prime the FK descriptor's cache so accessing changed_object_type + # returns our mock without hitting the DB or invoking type checks. + ObjectChange._meta.get_field('changed_object_type').set_cached_value(oc, fake_ct) + + excluded = oc.diff_exclude_fields + self.assertIn('lft', excluded) + self.assertIn('rght', excluded) + self.assertIn('tree_id', excluded) + self.assertIn('level', excluded) + + +class AddRelatedCountResilienceTests(TestCase): + """ + add_related_count() must not raise FieldDoesNotExist at queryset-build + time so that view modules (which bind it as a class attribute) can be + imported even if a referenced field has been renamed. + """ + + def test_unknown_field_does_not_raise(self): + # Bare manager call equivalent to a view class body using a stale name. + qs = Region.objects.add_related_count( + Region.objects.all(), + Region, # any model; the field name is what matters + 'this_field_does_not_exist', + 'noop_count', + cumulative=True, + ) + # The annotation was attached; evaluating it would fail at the DB + # (column doesn't exist) but importing the view module must succeed. + self.assertIn('noop_count', qs.query.annotations) + + +class CascadeTriggerScopeTests(TestCase): + """ + The AFTER cascade trigger fires on parent_id or name changes. A rename + leaves `path` untouched but pushes the new sort_path into descendants. + """ + + def test_rename_preserves_descendant_path_but_updates_sort_path(self): + parent = Region.objects.create(name='Bravo', slug='bravo-ct') + child = Region.objects.create(parent=parent, name='Mid', slug='mid-ct') + original_child_path = child.path + parent.name = 'Zulu' + parent.save() + child.refresh_from_db() + # Path is unaffected by a rename; sort_path follows the new name. + self.assertEqual(child.path, original_child_path) + self.assertNotIn('Bravo', child.sort_path) + self.assertIn('Zulu', child.sort_path) + + +class CycleGuardWithEmptyPathTests(TestCase): + """ + _parent_creates_cycle must catch self-as-parent even when self.path is + empty or deferred — otherwise an instance constructed without a loaded + path can pass the Python guard and corrupt the tree. + """ + + def test_self_parent_rejected_when_path_is_empty(self): + from django.core.exceptions import ValidationError + a = Region.objects.create(name='A', slug='a-empty-cyc') + # Simulate a caller (script, plugin) that holds an instance whose `path` + # attribute was never loaded — e.g. via .only('id', 'parent_id'). + # Empty-string path is the LtreeField default, so this mirrors what a + # deferred-or-unset path looks like at the Python layer. + a.path = '' + a.parent_id = a.pk + with self.assertRaises(ValidationError): + a.save() + + +class TriggerCycleGuardTests(TestCase): + """ + The BEFORE INSERT/UPDATE trigger must reject a parent_id assignment that + would form a cycle. The Python save()-time guard already catches this for + ordinary save(); the trigger backstops QuerySet.update / bulk_update / + raw UPDATEs that bypass save(). + """ + + def test_queryset_update_to_self_parent_blocked_by_trigger(self): + from django.db import IntegrityError, transaction + a = Region.objects.create(name='A', slug='a-tgcyc') + # IntegrityError leaves Django's transaction marked-for-rollback; wrap + # the failing UPDATE in a savepoint so the outer test transaction can + # continue to issue queries (refresh_from_db). + with self.assertRaises(IntegrityError) as ctx: + with transaction.atomic(): + Region.objects.filter(pk=a.pk).update(parent_id=a.pk) + self.assertIn('cycle detected', str(ctx.exception)) + a.refresh_from_db() + self.assertIsNone(a.parent_id) + + def test_queryset_update_to_descendant_blocked_by_trigger(self): + from django.db import IntegrityError, transaction + a = Region.objects.create(name='A', slug='a-tgcyc2') + b = Region.objects.create(parent=a, name='B', slug='b-tgcyc2') + # Try to reparent A under B (B is A's descendant) via raw UPDATE. + with self.assertRaises(IntegrityError) as ctx: + with transaction.atomic(): + Region.objects.filter(pk=a.pk).update(parent_id=b.pk) + self.assertIn('cycle detected', str(ctx.exception)) + a.refresh_from_db() + self.assertIsNone(a.parent_id) + + def test_legitimate_reparent_via_update_still_works(self): + a = Region.objects.create(name='A', slug='a-tgok') + b = Region.objects.create(name='B', slug='b-tgok') + child = Region.objects.create(parent=a, name='C', slug='c-tgok') + Region.objects.filter(pk=child.pk).update(parent_id=b.pk) + child.refresh_from_db() + self.assertEqual(child.parent_id, b.pk) + self.assertEqual(child.path, _path(b.pk, child.pk)) + + def test_queryset_update_mid_tree_to_own_descendant_blocked(self): + """ + Reparenting a non-root node under one of its own (non-immediate) + descendants must raise. The earlier `lpad(NEW.id) @> parent_path` + check only fired when NEW was the root of parent_path, missing + mid-tree cycles entirely. + """ + from django.db import IntegrityError, transaction + a = Region.objects.create(name='A', slug='a-midcyc') + b = Region.objects.create(parent=a, name='B', slug='b-midcyc') + c = Region.objects.create(parent=b, name='C', slug='c-midcyc') + # Try to make B a child of C — B is mid-tree (path A.B), parent_path is A.B.C. + with self.assertRaises(IntegrityError) as ctx: + with transaction.atomic(): + Region.objects.filter(pk=b.pk).update(parent_id=c.pk) + self.assertIn('cycle detected', str(ctx.exception)) + b.refresh_from_db() + self.assertEqual(b.parent_id, a.pk) + + def test_queryset_update_mid_tree_self_loop_blocked(self): + """A non-root node assigning itself as parent must also raise.""" + from django.db import IntegrityError, transaction + a = Region.objects.create(name='A', slug='a-midself') + b = Region.objects.create(parent=a, name='B', slug='b-midself') + with self.assertRaises(IntegrityError) as ctx: + with transaction.atomic(): + Region.objects.filter(pk=b.pk).update(parent_id=b.pk) + self.assertIn('cycle detected', str(ctx.exception)) + b.refresh_from_db() + self.assertEqual(b.parent_id, a.pk) + + +class NaturalSortSortPathTests(TestCase): + """ + Sort-path separator must produce correct tree-flatten ordering under the + `natural_sort` (ICU `und-u-kn-true`) collation used by ModuleBay, + TenantGroup, and WirelessLANGroup. chr(1) was variable-ignorable under + that collation and would interleave children with unrelated roots; + chr(9) (TAB) is treated non-variably and orders deterministically. + """ + + def test_chr9_separator_collates_below_letters_under_natural_sort(self): + # Direct collation probe — independent of any model schema. + with connection.cursor() as cur: + cur.execute(""" + SELECT + ('A' || chr(9) || 'Z') COLLATE "natural_sort" + < 'AA' COLLATE "natural_sort" + """) + self.assertTrue(cur.fetchone()[0]) + + def test_tree_flatten_ordering_under_natural_sort(self): + # TenantGroup.sort_path uses natural_sort collation; build a small tree + # where chr(1) would have produced wrong sibling ordering. TenantGroup + # names are globally unique, so each row gets a distinct name. Under + # the old chr(1) separator the child's sort_path was variable-ignorable + # and collated AFTER the unrelated root 'nsP1'; chr(9) sorts strictly + # below digits/letters and keeps children clustered under their parent. + from tenancy.models import TenantGroup + parent = TenantGroup.objects.create(name='nsP', slug='nsp-ns') + TenantGroup.objects.create(parent=parent, name='nsPchild', slug='nspc-ns') + TenantGroup.objects.create(name='nsP1', slug='nsp1-ns') # unrelated root + + names = list( + TenantGroup.objects.filter(slug__endswith='-ns') + .order_by('sort_path') + .values_list('name', flat=True) + ) + # Expected tree-flatten: parent, its child, then the unrelated root. + self.assertEqual(names, ['nsP', 'nsPchild', 'nsP1']) diff --git a/netbox/wireless/api/views.py b/netbox/wireless/api/views.py index f5883e930d2..6d48c0be796 100644 --- a/netbox/wireless/api/views.py +++ b/netbox/wireless/api/views.py @@ -1,6 +1,6 @@ from rest_framework.routers import APIRootView -from netbox.api.viewsets import MPTTLockedMixin, NetBoxModelViewSet +from netbox.api.viewsets import NetBoxModelViewSet from wireless import filtersets from wireless.models import * @@ -15,7 +15,7 @@ def get_view_name(self): return 'Wireless' -class WirelessLANGroupViewSet(MPTTLockedMixin, NetBoxModelViewSet): +class WirelessLANGroupViewSet(NetBoxModelViewSet): queryset = WirelessLANGroup.objects.add_related_count( WirelessLANGroup.objects.all(), WirelessLAN, diff --git a/netbox/wireless/graphql/types.py b/netbox/wireless/graphql/types.py index 557f32ce476..060175ff089 100644 --- a/netbox/wireless/graphql/types.py +++ b/netbox/wireless/graphql/types.py @@ -3,7 +3,7 @@ import strawberry import strawberry_django -from netbox.graphql.types import NestedGroupObjectType, PrimaryObjectType +from netbox.graphql.types import NestedLtreeGroupObjectType, PrimaryObjectType from wireless import models from .filters import * @@ -22,11 +22,11 @@ @strawberry_django.type( models.WirelessLANGroup, - fields='__all__', + exclude=['path', 'sort_path'], filters=WirelessLANGroupFilter, pagination=True ) -class WirelessLANGroupType(NestedGroupObjectType): +class WirelessLANGroupType(NestedLtreeGroupObjectType): parent: Annotated["WirelessLANGroupType", strawberry.lazy('wireless.graphql.types')] | None wireless_lans: list[Annotated["WirelessLANType", strawberry.lazy('wireless.graphql.types')]] diff --git a/netbox/wireless/migrations/0020_ltree_paths.py b/netbox/wireless/migrations/0020_ltree_paths.py new file mode 100644 index 00000000000..539357bdca2 --- /dev/null +++ b/netbox/wireless/migrations/0020_ltree_paths.py @@ -0,0 +1,86 @@ +"""Replace django-mptt with PostgreSQL ltree for wireless's hierarchical models. + +The reverse migration is lossy: it re-adds the MPTT columns empty and does not +rebuild the tree. Forward migration is the supported direction. +""" +import django.db.models.deletion +from django.contrib.postgres.indexes import GistIndex +from django.contrib.postgres.operations import CreateExtension +from django.db import migrations, models + +import netbox.models.ltree +from utilities.ltree import InstallLtreeTriggers +from utilities.mptt_to_ltree import assert_paths_populated_sql, populate_paths_sql + +MODEL = 'wirelesslangroup' +TABLE = 'wireless_wirelesslangroup' +LEGACY_FIELDS = ('lft', 'rght', 'tree_id', 'level') + + +class Migration(migrations.Migration): + + dependencies = [ + ('wireless', '0019_default_ordering_indexes'), + ] + + operations = [ + # Enable the ltree extension first so the migration fails fast if it is missing. + CreateExtension('ltree'), + + migrations.AlterField( + model_name='wirelesslangroup', name='parent', + field=models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, + related_name='children', to='wireless.wirelesslangroup', + ), + ), + + migrations.AddField( + model_name=MODEL, name='path', + field=netbox.models.ltree.LtreeField(blank=True, editable=False, null=True), + ), + # sort_path uses natural_sort to match the WirelessLANGroup.name collation. + migrations.AddField( + model_name=MODEL, name='sort_path', + field=models.TextField( + blank=True, default='', editable=False, db_collation='natural_sort', + ), + ), + + InstallLtreeTriggers(TABLE, name_column='name'), + + # Populate path and sort_path for existing rows by walking the tree from + # the roots (parent_id IS NULL) downward via a single recursive CTE. + migrations.RunSQL( + populate_paths_sql(TABLE, sort_path=True), + reverse_sql=migrations.RunSQL.noop, + ), + + # Fail fast if any row still has NULL path (orphan FKs) before the + # AlterField below tries to set NOT NULL inside ALTER COLUMN. + migrations.RunSQL( + assert_paths_populated_sql(TABLE), + reverse_sql=migrations.RunSQL.noop, + ), + + migrations.AlterField( + model_name=MODEL, name='path', + field=netbox.models.ltree.LtreeField(blank=True, default='', editable=False), + ), + + migrations.AlterModelOptions( + name=MODEL, options={'ordering': ('sort_path',)}, + ), + + migrations.RemoveIndex(model_name=MODEL, name='wireless_wirelesslangroup_fbcd'), + *[migrations.RemoveField(model_name=MODEL, name=f) for f in LEGACY_FIELDS], + + migrations.AddIndex( + model_name=MODEL, + index=GistIndex(fields=['path'], name='wireless_lan_grp_path_gist'), + ), + migrations.AddIndex( + model_name=MODEL, + index=models.Index(fields=['sort_path'], name='wireless_lan_grp_sort_idx'), + ), + ] diff --git a/netbox/wireless/models.py b/netbox/wireless/models.py index f212fb6dcd2..d3618d513bc 100644 --- a/netbox/wireless/models.py +++ b/netbox/wireless/models.py @@ -1,3 +1,4 @@ +from django.contrib.postgres.indexes import GistIndex from django.core.exceptions import ValidationError from django.db import models from django.utils.translation import gettext_lazy as _ @@ -5,7 +6,7 @@ from dcim.choices import LinkStatusChoices from dcim.constants import WIRELESS_IFACE_TYPES from dcim.models.mixins import CachedScopeMixin -from netbox.models import NestedGroupModel, PrimaryModel +from netbox.models import NestedLtreeGroupModel, PrimaryModel from netbox.models.mixins import DistanceMixin from .choices import * @@ -46,7 +47,7 @@ class Meta: abstract = True -class WirelessLANGroup(NestedGroupModel): +class WirelessLANGroup(NestedLtreeGroupModel): """ A nested grouping of WirelessLANs """ @@ -61,12 +62,14 @@ class WirelessLANGroup(NestedGroupModel): max_length=100, unique=True ) + # sort_path inherits natural_sort collation from `name` automatically (LtreeModelBase). class Meta: - ordering = ('name', 'pk') - # Empty tuple triggers Django migration detection for MPTT indexes - # (see #21016, django-mptt/django-mptt#682) - indexes = () + ordering = ('sort_path',) + indexes = ( + GistIndex(fields=['path'], name='wireless_lan_grp_path_gist'), + models.Index(fields=['sort_path'], name='wireless_lan_grp_sort_idx'), + ) constraints = ( models.UniqueConstraint( fields=('parent', 'name'),