diff --git a/netbox/core/signals.py b/netbox/core/signals.py index 46a0fe0fd..2994aaa41 100644 --- a/netbox/core/signals.py +++ b/netbox/core/signals.py @@ -3,6 +3,7 @@ from threading import local from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.db.models import CASCADE from django.db.models.fields.reverse_related import ManyToManyRel, ManyToOneRel from django.db.models.signals import m2m_changed, post_migrate, post_save, pre_delete from django.dispatch import receiver, Signal @@ -220,14 +221,8 @@ def handle_deleted_object(sender, instance, **kwargs): obj.snapshot() # Ensure the change record includes the "before" state if type(relation) is ManyToManyRel: getattr(obj, related_field_name).remove(instance) - elif type(relation) is ManyToOneRel and relation.field.null is True: + elif type(relation) is ManyToOneRel and relation.null and relation.on_delete is not CASCADE: 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 diff --git a/netbox/core/tests/test_changelog.py b/netbox/core/tests/test_changelog.py index 4a00e4a25..b9242605f 100644 --- a/netbox/core/tests/test_changelog.py +++ b/netbox/core/tests/test_changelog.py @@ -5,14 +5,16 @@ from rest_framework import status from core.choices import ObjectChangeActionChoices from core.models import ObjectChange, ObjectType -from dcim.choices import SiteStatusChoices -from dcim.models import Site, CableTermination, Device, DeviceType, DeviceRole, Interface, Cable +from dcim.choices import InterfaceTypeChoices, ModuleStatusChoices, SiteStatusChoices +from dcim.models import ( + Cable, CableTermination, Device, DeviceRole, DeviceType, Manufacturer, Module, ModuleBay, ModuleType, Interface, + Site, +) 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.utils import create_tags, create_test_device, post_data from utilities.testing.views import ModelViewTestCase -from dcim.models import Manufacturer class ChangeLogViewTest(ModelViewTestCase): @@ -622,3 +624,64 @@ class ChangeLogAPITest(APITestCase): self.assertEqual(objectchange.prechange_data['name'], 'Site 1') self.assertEqual(objectchange.prechange_data['slug'], 'site-1') self.assertEqual(objectchange.postchange_data, None) + + def test_deletion_ordering(self): + """ + Check that the cascading deletion of dependent objects is recorded in the correct order. + """ + device = create_test_device('device1') + module_bay = ModuleBay.objects.create(device=device, name='Module Bay 1') + module_type = ModuleType.objects.create(manufacturer=Manufacturer.objects.first(), model='Module Type 1') + self.add_permissions('dcim.add_module', 'dcim.add_interface', 'dcim.delete_module') + self.assertEqual(ObjectChange.objects.count(), 0) # Sanity check + + # Create a new Module + data = { + 'device': device.pk, + 'module_bay': module_bay.pk, + 'module_type': module_type.pk, + 'status': ModuleStatusChoices.STATUS_ACTIVE, + } + url = reverse('dcim-api:module-list') + response = self.client.post(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_201_CREATED) + module = device.modules.first() + + # Create an Interface on the Module + data = { + 'device': device.pk, + 'module': module.pk, + 'name': 'Interface 1', + 'type': InterfaceTypeChoices.TYPE_1GE_FIXED, + } + url = reverse('dcim-api:interface-list') + response = self.client.post(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_201_CREATED) + interface = device.interfaces.first() + + # Delete the Module + url = reverse('dcim-api:module-detail', kwargs={'pk': module.pk}) + response = self.client.delete(url, **self.header) + self.assertHttpStatus(response, status.HTTP_204_NO_CONTENT) + self.assertEqual(Module.objects.count(), 0) + self.assertEqual(Interface.objects.count(), 0) + + # Verify the creation of the expected ObjectChange records. We should see four total records, in this order: + # 1. Module created + # 2. Interface created + # 3. Interface deleted + # 4. Module deleted + changes = ObjectChange.objects.order_by('time') + self.assertEqual(len(changes), 4) + self.assertEqual(changes[0].changed_object_type, ContentType.objects.get_for_model(Module)) + self.assertEqual(changes[0].changed_object_id, module.pk) + self.assertEqual(changes[0].action, ObjectChangeActionChoices.ACTION_CREATE) + self.assertEqual(changes[1].changed_object_type, ContentType.objects.get_for_model(Interface)) + self.assertEqual(changes[1].changed_object_id, interface.pk) + self.assertEqual(changes[1].action, ObjectChangeActionChoices.ACTION_CREATE) + self.assertEqual(changes[2].changed_object_type, ContentType.objects.get_for_model(Interface)) + self.assertEqual(changes[2].changed_object_id, interface.pk) + self.assertEqual(changes[2].action, ObjectChangeActionChoices.ACTION_DELETE) + self.assertEqual(changes[3].changed_object_type, ContentType.objects.get_for_model(Module)) + self.assertEqual(changes[3].changed_object_id, module.pk) + self.assertEqual(changes[3].action, ObjectChangeActionChoices.ACTION_DELETE) diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index 10416b748..911bc2007 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -2,14 +2,14 @@ import logging from django.contrib.contenttypes.fields import GenericRelation from django.db import router -from django.db.models.deletion import Collector +from django.db.models.deletion import CASCADE, Collector logger = logging.getLogger("netbox.models.deletion") class CustomCollector(Collector): """ - Custom collector that handles GenericRelations correctly. + Override Django's stock Collector to handle GenericRelations and ensure proper ordering of cascading deletions. """ def collect( @@ -23,11 +23,15 @@ class CustomCollector(Collector): keep_parents=False, fail_on_restricted=True, ): - """ - Override collect to first collect standard dependencies, - then add GenericRelations to the dependency graph. - """ - # Call parent collect first to get all standard dependencies + # By default, Django will force the deletion of dependent objects before the parent only if the ForeignKey field + # is not nullable. We want to ensure proper ordering regardless, so if the ForeignKey has `on_delete=CASCADE` + # applied, we set `nullable` to False when calling `collect()`. + if objs and source and source_attr: + model = objs[0].__class__ + field = model._meta.get_field(source_attr) + if field.remote_field.on_delete == CASCADE: + nullable = False + super().collect( objs, source=source, @@ -39,10 +43,8 @@ class CustomCollector(Collector): fail_on_restricted=fail_on_restricted, ) - # Track which GenericRelations we've already processed to prevent infinite recursion + # Add GenericRelations to the dependency graph 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