Merge pull request #20708 from netbox-community/20699-changelog-ordering

Fixes #20699: Ensure proper ordering of changelog entries resulting from cascading deletions

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

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.choices import InterfaceTypeChoices, ModuleStatusChoices, SiteStatusChoices
-from dcim.models import Site, CableTermination, Device, DeviceType, DeviceRole, Interface, Cable
+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)

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,
     ):
-        """
+        # By default, Django will force the deletion of dependent objects before the parent only if the ForeignKey field
-        Override collect to first collect standard dependencies,
+        # is not nullable. We want to ensure proper ordering regardless, so if the ForeignKey has `on_delete=CASCADE`
-        then add GenericRelations to the dependency graph.
+        # applied, we set `nullable` to False when calling `collect()`.
-        """
+        if objs and source and source_attr:
-        # Call parent collect first to get all standard dependencies
+            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