From a23a0f2708cd3c62b729c4caf4fe72e9906984a1 Mon Sep 17 00:00:00 2001 From: Arthur Date: Fri, 6 Jun 2025 15:18:55 -0700 Subject: [PATCH 1/9] #19680 fix deletion dependency order for GenericRelations --- netbox/dcim/models/devices.py | 4 +- netbox/netbox/models/deletion.py | 117 +++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 netbox/netbox/models/deletion.py diff --git a/netbox/dcim/models/devices.py b/netbox/dcim/models/devices.py index 5988f82410e..ae0b9fffe74 100644 --- a/netbox/dcim/models/devices.py +++ b/netbox/dcim/models/devices.py @@ -25,6 +25,7 @@ from netbox.choices import ColorChoices from netbox.config import ConfigItem from netbox.models import NestedGroupModel, OrganizationalModel, PrimaryModel +from netbox.models.deletion import DeleteMixin from netbox.models.mixins import WeightMixin from netbox.models.features import ContactsMixin, ImageAttachmentsMixin from utilities.fields import ColorField, CounterCacheField @@ -430,12 +431,13 @@ class Meta: class Device( + DeleteMixin, ContactsMixin, ImageAttachmentsMixin, RenderConfigMixin, ConfigContextModel, TrackingModelMixin, - PrimaryModel + PrimaryModel, ): """ A Device represents a piece of physical hardware mounted within a Rack. Each Device is assigned a DeviceType, diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py new file mode 100644 index 00000000000..006c49c1e07 --- /dev/null +++ b/netbox/netbox/models/deletion.py @@ -0,0 +1,117 @@ +import logging + +from django.contrib.contenttypes.fields import GenericRelation +from django.db import router +from django.db.models.deletion import Collector + +logger = logging.getLogger("netbox.models.deletion") + + +class CustomCollector(Collector): + """ + Custom collector that handles GenericRelations correctly. + """ + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._processed_objects = set() + + def collect( + self, + objs, + source=None, + nullable=False, + collect_related=True, + source_attr=None, + reverse_dependency=False, + keep_parents=False, + fail_on_restricted=True, + ): + """ + Override collect to first collect standard dependencies, + then add GenericRelations to the dependency graph. + """ + # Filter out objects we've already processed + new_objs = [] + for obj in objs: + obj_key = f"{obj._meta.model_name}.{obj.pk}" + if obj_key not in self._processed_objects: + self._processed_objects.add(obj_key) + new_objs.append(obj) + + if not new_objs: + return + + # Call parent collect first to get all standard dependencies + super().collect( + new_objs, + source=source, + nullable=nullable, + collect_related=collect_related, + source_attr=source_attr, + reverse_dependency=reverse_dependency, + keep_parents=keep_parents, + fail_on_restricted=fail_on_restricted, + ) + + # Track which GenericRelations we've already processed to prevent infinite recursion + processed_relations = set() + + # Now add GenericRelations to the dependency graph + for _, instances in list(self.data.items()): + for instance in instances: + # Get all GenericRelations for this model + for field in instance._meta.private_fields: + if isinstance(field, GenericRelation): + # Create a unique key for this relation + relation_key = f"{instance._meta.model_name}.{field.name}" + if relation_key in processed_relations: + continue + processed_relations.add(relation_key) + + # Get the related objects + related_objs = getattr(instance, field.name).all() + if related_objs: + # Add them to the dependency graph + self.collect( + related_objs, + source=instance, + nullable=True, + collect_related=True, + source_attr=field.name, + reverse_dependency=True, + keep_parents=keep_parents, + fail_on_restricted=False, + ) + + +class DeleteMixin: + """ + Mixin to override the model delete function to use our custom collector. + """ + + def delete(self, using=None, keep_parents=False): + """ + Override delete to use our custom collector. + """ + using = using or router.db_for_write(self.__class__, instance=self) + assert self._get_pk_val() is not None, "%s object can't be deleted because its %s attribute is set to None." % ( + self._meta.object_name, + self._meta.pk.attname, + ) + + collector = CustomCollector(using=using) + collector.collect([self], keep_parents=keep_parents) + + return collector.delete() + + delete.alters_data = True + + @classmethod + def verify_mro(cls, instance): + """ + Verify that this mixin is first in the MRO. + """ + mro = instance.__class__.__mro__ + if mro.index(cls) != 0: + raise RuntimeError(f"{cls.__name__} must be first in the MRO. Current MRO: {mro}") From cf4f0982c5d8d265a927c1d544c0abf329cec542 Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 9 Jun 2025 11:10:10 -0700 Subject: [PATCH 2/9] 19680 add test --- netbox/core/tests/test_changelog.py | 76 ++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/netbox/core/tests/test_changelog.py b/netbox/core/tests/test_changelog.py index 4914dbaf37a..1e61c1d3e9c 100644 --- a/netbox/core/tests/test_changelog.py +++ b/netbox/core/tests/test_changelog.py @@ -6,12 +6,13 @@ from core.choices import ObjectChangeActionChoices from core.models import ObjectChange, ObjectType from dcim.choices import SiteStatusChoices -from dcim.models import Site +from dcim.models import Site, CableTermination, Device, DeviceType, DeviceRole, Interface, Cable from extras.choices import * from extras.models import CustomField, CustomFieldChoiceSet, Tag from utilities.testing import APITestCase from utilities.testing.utils import create_tags, post_data from utilities.testing.views import ModelViewTestCase +from dcim.models import Manufacturer class ChangeLogViewTest(ModelViewTestCase): @@ -270,6 +271,79 @@ def test_update_object_nochange(self): # Check that no ObjectChange records have been created self.assertEqual(ObjectChange.objects.count(), 0) + def test_ordering_genericrelation(self): + # Create required objects first + manufacturer = Manufacturer.objects.create(name='Manufacturer 1') + device_type = DeviceType.objects.create( + manufacturer=manufacturer, + model='Model 1', + slug='model-1' + ) + device_role = DeviceRole.objects.create( + name='Role 1', + slug='role-1' + ) + site = Site.objects.create( + name='Site 1', + slug='site-1' + ) + + # Create two devices + device1 = Device.objects.create( + name='Device 1', + device_type=device_type, + role=device_role, + site=site + ) + device2 = Device.objects.create( + name='Device 2', + device_type=device_type, + role=device_role, + site=site + ) + + # Create interfaces on both devices + interface1 = Interface.objects.create( + device=device1, + name='eth0', + type='1000base-t' + ) + interface2 = Interface.objects.create( + device=device2, + name='eth0', + type='1000base-t' + ) + + # Create a cable between the interfaces + _ = Cable.objects.create( + a_terminations=[interface1], + b_terminations=[interface2], + status='connected' + ) + + # Delete device1 + request = { + 'path': reverse('dcim:device_delete', kwargs={'pk': device1.pk}), + 'data': post_data({'confirm': True}), + } + self.add_permissions( + 'dcim.delete_device', + 'dcim.delete_interface', + 'dcim.delete_cable', + 'dcim.delete_cabletermination' + ) + response = self.client.post(**request) + self.assertHttpStatus(response, 302) + + # Get the last 3 ObjectChange records ordered by time + changes = ObjectChange.objects.order_by('-time')[:3] + + # Verify the order of deletion + self.assertEqual(len(changes), 3) + self.assertEqual(changes[0].changed_object_type, ContentType.objects.get_for_model(Device)) + self.assertEqual(changes[1].changed_object_type, ContentType.objects.get_for_model(Interface)) + self.assertEqual(changes[2].changed_object_type, ContentType.objects.get_for_model(CableTermination)) + class ChangeLogAPITest(APITestCase): From 21f30070e6403f3eb4542fb242098093b162d8f4 Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 9 Jun 2025 14:39:51 -0700 Subject: [PATCH 3/9] 19680 fix Collector and test --- netbox/core/tests/test_changelog.py | 10 ++++++---- netbox/netbox/models/deletion.py | 16 ++-------------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/netbox/core/tests/test_changelog.py b/netbox/core/tests/test_changelog.py index 1e61c1d3e9c..df84610763a 100644 --- a/netbox/core/tests/test_changelog.py +++ b/netbox/core/tests/test_changelog.py @@ -335,14 +335,16 @@ def test_ordering_genericrelation(self): response = self.client.post(**request) self.assertHttpStatus(response, 302) - # Get the last 3 ObjectChange records ordered by time - changes = ObjectChange.objects.order_by('-time')[:3] + # Get the ObjectChange records for delete actions ordered by time + changes = ObjectChange.objects.filter( + action=ObjectChangeActionChoices.ACTION_DELETE + ).order_by('time')[:3] # Verify the order of deletion self.assertEqual(len(changes), 3) - self.assertEqual(changes[0].changed_object_type, ContentType.objects.get_for_model(Device)) + self.assertEqual(changes[0].changed_object_type, ContentType.objects.get_for_model(CableTermination)) self.assertEqual(changes[1].changed_object_type, ContentType.objects.get_for_model(Interface)) - self.assertEqual(changes[2].changed_object_type, ContentType.objects.get_for_model(CableTermination)) + self.assertEqual(changes[2].changed_object_type, ContentType.objects.get_for_model(Device)) class ChangeLogAPITest(APITestCase): diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index 006c49c1e07..6d0fc18ba5e 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -69,20 +69,8 @@ def collect( continue processed_relations.add(relation_key) - # Get the related objects - related_objs = getattr(instance, field.name).all() - if related_objs: - # Add them to the dependency graph - self.collect( - related_objs, - source=instance, - nullable=True, - collect_related=True, - source_attr=field.name, - reverse_dependency=True, - keep_parents=keep_parents, - fail_on_restricted=False, - ) + # Add the model that the generic relation points to as a dependency + self.add_dependency(field.related_model, instance, reverse_dependency=True) class DeleteMixin: From fd9decce04b4d1729d6e4072369101921ef02d4e Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 9 Jun 2025 14:48:33 -0700 Subject: [PATCH 4/9] 19680 put on changeloggingmixin --- netbox/dcim/models/devices.py | 2 -- netbox/netbox/models/features.py | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/netbox/dcim/models/devices.py b/netbox/dcim/models/devices.py index ae0b9fffe74..f0978c2f030 100644 --- a/netbox/dcim/models/devices.py +++ b/netbox/dcim/models/devices.py @@ -25,7 +25,6 @@ from netbox.choices import ColorChoices from netbox.config import ConfigItem from netbox.models import NestedGroupModel, OrganizationalModel, PrimaryModel -from netbox.models.deletion import DeleteMixin from netbox.models.mixins import WeightMixin from netbox.models.features import ContactsMixin, ImageAttachmentsMixin from utilities.fields import ColorField, CounterCacheField @@ -431,7 +430,6 @@ class Meta: class Device( - DeleteMixin, ContactsMixin, ImageAttachmentsMixin, RenderConfigMixin, diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index 25f23c9d324..79145ce7020 100644 --- a/netbox/netbox/models/features.py +++ b/netbox/netbox/models/features.py @@ -16,6 +16,7 @@ from extras.constants import CUSTOMFIELD_EMPTY_VALUES from extras.utils import is_taggable from netbox.config import get_config +from netbox.models.deletion import DeleteMixin from netbox.registry import registry from netbox.signals import post_clean from utilities.json import CustomFieldJSONEncoder @@ -45,7 +46,7 @@ # Feature mixins # -class ChangeLoggingMixin(models.Model): +class ChangeLoggingMixin(DeleteMixin, models.Model): """ Provides change logging support for a model. Adds the `created` and `last_updated` fields. """ From ffae0df80817898ee09a2ae07d9c3058f713bd16 Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 9 Jun 2025 14:55:08 -0700 Subject: [PATCH 5/9] 19680 cleanup --- netbox/netbox/models/deletion.py | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index 6d0fc18ba5e..d81bd27f81c 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -31,20 +31,9 @@ def collect( Override collect to first collect standard dependencies, then add GenericRelations to the dependency graph. """ - # Filter out objects we've already processed - new_objs = [] - for obj in objs: - obj_key = f"{obj._meta.model_name}.{obj.pk}" - if obj_key not in self._processed_objects: - self._processed_objects.add(obj_key) - new_objs.append(obj) - - if not new_objs: - return - # Call parent collect first to get all standard dependencies super().collect( - new_objs, + objs, source=source, nullable=nullable, collect_related=collect_related, From aa21e0ff558a712bcbea65a9e1ece41f7f006e55 Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 9 Jun 2025 14:58:04 -0700 Subject: [PATCH 6/9] 19680 cleanup --- netbox/netbox/models/deletion.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index d81bd27f81c..10416b748aa 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -12,10 +12,6 @@ class CustomCollector(Collector): Custom collector that handles GenericRelations correctly. """ - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - self._processed_objects = set() - def collect( self, objs, From 5334fd78493496587010f2f7e16be984f2a73642 Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 9 Jun 2025 14:59:00 -0700 Subject: [PATCH 7/9] 19680 cleanup --- netbox/dcim/models/devices.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netbox/dcim/models/devices.py b/netbox/dcim/models/devices.py index f0978c2f030..5988f82410e 100644 --- a/netbox/dcim/models/devices.py +++ b/netbox/dcim/models/devices.py @@ -435,7 +435,7 @@ class Device( RenderConfigMixin, ConfigContextModel, TrackingModelMixin, - PrimaryModel, + PrimaryModel ): """ A Device represents a piece of physical hardware mounted within a Rack. Each Device is assigned a DeviceType, From 1c71107a44b9d2f6ef335fd8109806b0e067b9c7 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 10 Jun 2025 08:31:04 -0700 Subject: [PATCH 8/9] 19680 skip changelog update for deleted objects --- netbox/core/signals.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/netbox/core/signals.py b/netbox/core/signals.py index 4b537b2d47a..6206421f9a0 100644 --- a/netbox/core/signals.py +++ b/netbox/core/signals.py @@ -117,6 +117,7 @@ def handle_deleted_object(sender, instance, **kwargs): # to queueing any events for the object being deleted, in case a validation error is # raised, causing the deletion to fail. model_name = f'{sender._meta.app_label}.{sender._meta.model_name}' + print(f"handle_deleted_object: {model_name}") validators = get_config().PROTECTION_RULES.get(model_name, []) try: run_validators(instance, validators) @@ -162,6 +163,12 @@ def handle_deleted_object(sender, instance, **kwargs): getattr(obj, related_field_name).remove(instance) elif type(relation) is ManyToOneRel and relation.field.null is True: setattr(obj, related_field_name, None) + # make sure the object hasn't been deleted - in case of + # deletion chaining of related objects + try: + obj.refresh_from_db() + except DoesNotExist: + continue obj.save() # Enqueue the object for event processing From 289d11c6bbf868301044b76f158a1949d968be5c Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 11 Jun 2025 13:22:33 -0700 Subject: [PATCH 9/9] 19680 remove print --- netbox/core/signals.py | 1 - 1 file changed, 1 deletion(-) diff --git a/netbox/core/signals.py b/netbox/core/signals.py index 6206421f9a0..8ba8cc24484 100644 --- a/netbox/core/signals.py +++ b/netbox/core/signals.py @@ -117,7 +117,6 @@ def handle_deleted_object(sender, instance, **kwargs): # to queueing any events for the object being deleted, in case a validation error is # raised, causing the deletion to fail. model_name = f'{sender._meta.app_label}.{sender._meta.model_name}' - print(f"handle_deleted_object: {model_name}") validators = get_config().PROTECTION_RULES.get(model_name, []) try: run_validators(instance, validators)