Merge pull request #22974 from netbox-community/22934-fix-event-queue-leaks

Fixes #22934: Clear queued events when a UI view rolls back a write

netbox/netbox/tests/test_views.py

@@ -1,16 +1,24 @@
 import urllib.parse
+from contextlib import contextmanager
 from unittest.mock import patch
 
 from django.contrib.contenttypes.models import ContentType
 from django.http import HttpResponse
-from django.test import Client, override_settings
+from django.test import Client, TransactionTestCase, override_settings
 from django.urls import reverse
 
-from dcim.models import DeviceType, Manufacturer, Site
+from dcim.choices import DeviceStatusChoices, InterfaceTypeChoices, SiteStatusChoices
+from dcim.models import Device, DeviceRole, DeviceType, Interface, Manufacturer, Site, VirtualChassis
+from extras.events import enqueue_event
 from extras.models import ImageAttachment
+from extras.validators import CustomValidator
+from ipam.choices import VLANStatusChoices
+from ipam.models import VLAN, VLANGroup
 from netbox.constants import EMPTY_TABLE_TEXT
 from netbox.search.backends import search_backend
+from users.models import User
 from utilities.testing import TestCase
+from utilities.views import get_action_url
 
 
 class HomeViewTestCase(TestCase):
@@ -159,3 +167,226 @@ class MediaViewTestCase(TestCase):
         url = reverse('media', kwargs={'path': traversal_path})
         response = self.client.get(url)
         self.assertHttpStatus(response, 404)
+
+
+class DeleteAtomicityMixin:
+    """
+    Scenario shared by the transactional and autocommit variants of the ObjectDeleteView
+    atomicity test (Ref: #22934).
+
+    VirtualChassis.delete() clears vc_position/vc_priority on each member device and saves them
+    before calling super().delete(), so an aborted deletion must roll those writes back. Otherwise
+    they commit while their queued events are discarded, leaving a committed change which
+    dispatches no events.
+    """
+    PROTECTION_RULES = {'dcim.virtualchassis': [CustomValidator({'name': {'eq': 'Nonexistent'}})]}
+
+    def create_virtual_chassis(self):
+        site = Site.objects.create(name='Site 1', slug='site-1')
+        role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1')
+        manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer, model='Device Type 1', slug='device-type-1'
+        )
+        virtual_chassis = VirtualChassis.objects.create(name='Virtual Chassis 1')
+        device = Device.objects.create(
+            name='Device 1', site=site, role=role, device_type=device_type,
+            virtual_chassis=virtual_chassis, vc_position=1, vc_priority=10
+        )
+
+        return virtual_chassis, device
+
+    def delete_virtual_chassis(self, virtual_chassis):
+        return self.client.post(
+            get_action_url(VirtualChassis, action='delete', kwargs={'pk': virtual_chassis.pk}),
+            data={'confirm': True}
+        )
+
+    def assertDeleteRolledBack(self, virtual_chassis, device):
+        self.assertTrue(VirtualChassis.objects.filter(pk=virtual_chassis.pk).exists())
+        device.refresh_from_db()
+        self.assertEqual(device.vc_position, 1)
+        self.assertEqual(device.vc_priority, 10)
+
+
+class RolledBackWriteEventQueueTestCase(DeleteAtomicityMixin, TestCase):
+    """
+    Verify that UI views which roll back a write transaction and return a normal response
+    discard any events queued during the aborted transaction. (Ref: #22934)
+    """
+    @contextmanager
+    def assertNoEventsDispatched(self, expect_queued=True):
+        """
+        Assert that no queued events are flushed to the events pipeline while within this block.
+
+        Unless expect_queued is disabled, also assert that at least one event *was* queued, so a
+        test cannot pass merely because nothing ever reached the queue (e.g. if the order in which
+        objects are processed changes, and the abort comes to precede the first event).
+        """
+        with patch('core.signals.enqueue_event', side_effect=enqueue_event) as enqueue:
+            with patch('netbox.context_managers.flush_events') as flush_events:
+                yield
+        flush_events.assert_not_called()
+        if expect_queued:
+            self.assertGreater(enqueue.call_count, 0, "No events were queued; nothing was cleared")
+
+    def test_bulk_create_view_rollback(self):
+        # A bulk create which trips a uniqueness constraint partway through must not dispatch
+        # created events for the objects written before the abort.
+        group = VLANGroup.objects.create(name='Test Group', slug='test-group')
+        VLAN.objects.create(group=group, vid=101, name='VLAN-101')
+        self.add_permissions('ipam.add_vlan', 'ipam.view_vlan')
+
+        with self.assertNoEventsDispatched():
+            response = self.client.post(reverse('ipam:vlan_bulk_add'), data={
+                'pattern': '100,101',
+                'group': group.pk,
+                'name': 'VLAN-{vid}',
+                'status': VLANStatusChoices.STATUS_ACTIVE,
+            })
+        self.assertHttpStatus(response, 200)
+
+        # The transaction was rolled back, so VID 100 must not exist
+        self.assertFalse(VLAN.objects.filter(group=group, vid=100).exists())
+
+    @override_settings(
+        PROTECTION_RULES={'dcim.site': [CustomValidator({'status': {'eq': SiteStatusChoices.STATUS_DECOMMISSIONING}})]}
+    )
+    def test_bulk_delete_view_rollback_abortrequest(self):
+        # A bulk delete aborted by a protection rule must not dispatch deleted events for the
+        # objects deleted before the abort.
+        site_a = Site.objects.create(
+            name='Site A', slug='site-a', status=SiteStatusChoices.STATUS_DECOMMISSIONING
+        )
+        site_b = Site.objects.create(name='Site B', slug='site-b', status=SiteStatusChoices.STATUS_ACTIVE)
+        self.add_permissions('dcim.delete_site', 'dcim.view_site')
+
+        with self.assertNoEventsDispatched():
+            self.client.post(reverse('dcim:site_bulk_delete'), data={
+                'pk': [site_a.pk, site_b.pk],
+                'confirm': True,
+                '_confirm': True,
+            })
+
+        # Both sites must still exist
+        self.assertEqual(Site.objects.filter(pk__in=[site_a.pk, site_b.pk]).count(), 2)
+
+    def test_bulk_delete_view_rollback_protectederror(self):
+        # A bulk delete aborted by a dependent object must not dispatch deleted events for the
+        # objects deleted before the abort.
+        manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
+        device_types = (
+            DeviceType(manufacturer=manufacturer, model='Device Type 1', slug='device-type-1'),
+            DeviceType(manufacturer=manufacturer, model='Device Type 2', slug='device-type-2'),
+        )
+        DeviceType.objects.bulk_create(device_types)
+        site = Site.objects.create(name='Site 1', slug='site-1')
+        role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1')
+        Device.objects.create(name='Device 1', site=site, role=role, device_type=device_types[1])
+        self.add_permissions('dcim.delete_devicetype', 'dcim.view_devicetype')
+
+        with self.assertNoEventsDispatched():
+            self.client.post(reverse('dcim:devicetype_bulk_delete'), data={
+                'pk': [dt.pk for dt in device_types],
+                'confirm': True,
+                '_confirm': True,
+            })
+
+        # Both device types must still exist
+        self.assertEqual(DeviceType.objects.count(), 2)
+
+    @override_settings(
+        PROTECTION_RULES={'dcim.device': [CustomValidator({'status': {'eq': DeviceStatusChoices.STATUS_OFFLINE}})]}
+    )
+    def test_object_delete_view_rollback_abortrequest(self):
+        # A cascading delete aborted by a protection rule on the parent must not dispatch deleted
+        # events for the children collected before the abort. Django's collector deletes dependent
+        # objects first, so the interface's pre_delete handler queues an event before the device's
+        # protection rule trips.
+        site = Site.objects.create(name='Site 1', slug='site-1')
+        role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1')
+        manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer, model='Device Type 1', slug='device-type-1'
+        )
+        device = Device.objects.create(
+            name='Device 1', site=site, role=role, device_type=device_type,
+            status=DeviceStatusChoices.STATUS_ACTIVE
+        )
+        interface = Interface.objects.create(device=device, name='eth0', type=InterfaceTypeChoices.TYPE_VIRTUAL)
+        self.add_permissions('dcim.delete_device', 'dcim.view_device')
+
+        with self.assertNoEventsDispatched():
+            self.client.post(get_action_url(Device, action='delete', kwargs={'pk': device.pk}), data={
+                'confirm': True,
+            })
+
+        # Both the device and its interface must still exist
+        self.assertTrue(Device.objects.filter(pk=device.pk).exists())
+        self.assertTrue(Interface.objects.filter(pk=interface.pk).exists())
+
+    @override_settings(PROTECTION_RULES=DeleteAtomicityMixin.PROTECTION_RULES)
+    def test_object_delete_view_rollback_is_atomic(self):
+        # An aborted delete must not leave behind writes performed by the model's delete() before
+        # super().delete(). Note what this detects: every test here runs inside the harness'
+        # transaction, so an escaped write is rolled back with the test regardless, and the
+        # assertions below cannot see it. Without the view's atomic block, AbortRequest instead
+        # escapes the collector's atomic(savepoint=False) and marks the harness' transaction as
+        # needing rollback, so this test fails with TransactionManagementError. The committed
+        # symptom itself is covered by DeleteAtomicityTestCase below.
+        virtual_chassis, device = self.create_virtual_chassis()
+        self.add_permissions('dcim.delete_virtualchassis', 'dcim.view_virtualchassis')
+
+        with self.assertNoEventsDispatched():
+            self.delete_virtual_chassis(virtual_chassis)
+
+        self.assertDeleteRolledBack(virtual_chassis, device)
+
+    def test_object_delete_view_rollback_protectederror(self):
+        # A delete aborted by a dependent object must not dispatch any events. Nothing is queued
+        # today, as the collector raises before any pre_delete signal fires; assert the queue is
+        # clear regardless, so correctness does not rest on Django's collection order.
+        manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer, model='Device Type 1', slug='device-type-1'
+        )
+        site = Site.objects.create(name='Site 1', slug='site-1')
+        role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1')
+        Device.objects.create(name='Device 1', site=site, role=role, device_type=device_type)
+        self.add_permissions('dcim.delete_devicetype', 'dcim.view_devicetype')
+
+        with self.assertNoEventsDispatched(expect_queued=False):
+            self.client.post(get_action_url(DeviceType, action='delete', kwargs={'pk': device_type.pk}), data={
+                'confirm': True,
+            })
+
+        self.assertTrue(DeviceType.objects.filter(pk=device_type.pk).exists())
+
+
+class DeleteAtomicityTestCase(DeleteAtomicityMixin, TransactionTestCase):
+    """
+    Exercise the autocommit path, which TestCase cannot reach: it wraps every test in a
+    transaction, so a write which escapes the view's rollback is rolled back with the test and
+    stays invisible to the assertions. Outside a transaction, a write performed by delete() before
+    super().delete() genuinely commits unless the view opens its own atomic block. (Ref: #22934)
+
+    Note: TransactionTestCase teardown flushes all tables, which removes rows seeded by data
+    migrations from a --keepdb database (e.g. the dcim.0206 ModuleTypeProfiles). A fresh test
+    database restores them.
+    """
+    def setUp(self):
+        # A superuser, rather than the object permissions granted by utilities.testing.TestCase:
+        # this case covers transaction handling, not permission enforcement.
+        self.user = User.objects.create_user(username='testuser', is_superuser=True)
+        self.client = Client()
+        self.client.force_login(self.user)
+
+    @override_settings(PROTECTION_RULES=DeleteAtomicityMixin.PROTECTION_RULES)
+    def test_object_delete_view_rollback_is_atomic(self):
+        virtual_chassis, device = self.create_virtual_chassis()
+
+        self.delete_virtual_chassis(virtual_chassis)
+
+        # Without the view's atomic block, the member device's cleared position and priority commit
+        # here, leaving a change which dispatches no events
+        self.assertDeleteRolledBack(virtual_chassis, device)

netbox/netbox/views/generic/bulk_views.py

@@ -427,7 +427,7 @@ class BulkCreateView(GetReturnURLMixin, BaseMultiObjectView):
                 return redirect(self.get_return_url(request))
 
             except (AbortTransaction, IntegrityError):
-                pass
+                clear_events.send(sender=self)
 
             except (AbortRequest, PermissionsViolation) as e:
                 logger.debug(e.message)
@@ -1218,6 +1218,7 @@ class BulkDeleteView(GetReturnURLMixin, BaseMultiObjectView):
 
                 except (ProtectedError, RestrictedError) as e:
                     logger.warning(f"Caught {type(e)} while attempting to delete objects")
+                    clear_events.send(sender=self)
                     if is_background_request(request):
                         request.job.logger.error(
                             _("Deletion failed due to the presence of one or more dependent objects.")
@@ -1227,6 +1228,7 @@ class BulkDeleteView(GetReturnURLMixin, BaseMultiObjectView):
 
                 except AbortRequest as e:
                     logger.debug(e.message)
+                    clear_events.send(sender=self)
                     if is_background_request(request):
                         request.job.logger.error(e.message)
                         raise JobFailed

netbox/netbox/views/generic/object_views.py

@@ -495,13 +495,16 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView):
 
             # Delete the object
             try:
-                obj.delete()
+                with transaction.atomic(using=router.db_for_write(self.queryset.model)):
+                    obj.delete()
             except (ProtectedError, RestrictedError) as e:
                 logger.info(f"Caught {type(e)} while attempting to delete objects")
+                clear_events.send(sender=self)
                 handle_protectederror([obj], request, e)
                 return redirect(obj.get_absolute_url())
             except AbortRequest as e:
                 logger.debug(e.message)
+                clear_events.send(sender=self)
                 messages.error(request, mark_safe(e.message))
                 return redirect(obj.get_absolute_url())