Fixes #22934: Clear queued events when a UI view rolls back a write
BulkCreateView, BulkDeleteView, and ObjectDeleteView each catch an exception, roll back the transaction, and return a normal response — but without clearing the in-memory events queue. The queue is flushed after the view returns, so webhooks and event rules fired for creations and deletions that were never committed. Send the clear_events signal from each of the affected handlers, matching the idiom already used by the sibling handlers in these views. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Jeremy Stretch
parent
a6f8848e3c
commit
f72072128a
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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())
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue