From b4ee2cf447a351bbacb5b19cf9182fb25a2e83ac Mon Sep 17 00:00:00 2001 From: Martin Hauser Date: Fri, 3 Apr 2026 00:49:42 +0200 Subject: [PATCH] fix(dcim): Refresh stale CablePath references during serialization (#21815) Cable edits can delete and recreate CablePath rows while endpoint instances remain in memory. Deferred event serialization can then encounter a stale `_path` reference and raise `CablePath.DoesNotExist`. Refresh stale `_path` references through `PathEndpoint.path` and route internal callers through that accessor. Update `EventContext` to track the latest serialization source for coalesced duplicate enqueues, while eagerly freezing delete-event payloads before row removal. Also avoid mutating `event_rule.action_data` when merging the event payload. Fixes #21498 --- netbox/dcim/api/serializers_/base.py | 10 ++- netbox/dcim/models/device_components.py | 52 +++++++++++--- netbox/dcim/tests/test_models.py | 60 ++++++++++++++++ netbox/extras/events.py | 76 ++++++++++++++++---- netbox/extras/tests/test_event_rules.py | 94 +++++++++++++++++++++++++ 5 files changed, 267 insertions(+), 25 deletions(-) diff --git a/netbox/dcim/api/serializers_/base.py b/netbox/dcim/api/serializers_/base.py index c60454937..9120ec109 100644 --- a/netbox/dcim/api/serializers_/base.py +++ b/netbox/dcim/api/serializers_/base.py @@ -38,7 +38,15 @@ class ConnectedEndpointsSerializer(serializers.ModelSerializer): @extend_schema_field(serializers.BooleanField) def get_connected_endpoints_reachable(self, obj): - return obj._path and obj._path.is_complete and obj._path.is_active + """ + Return whether the connected endpoints are reachable via a complete, active cable path. + """ + # Use the public `path` accessor rather than dereferencing `_path` + # directly. `path` already handles the stale in-memory relation case + # that can occur while CablePath rows are rebuilt during cable edits. + if path := obj.path: + return path.is_complete and path.is_active + return False class PortSerializer(serializers.ModelSerializer): diff --git a/netbox/dcim/models/device_components.py b/netbox/dcim/models/device_components.py index 09dc02ae9..75a26e25e 100644 --- a/netbox/dcim/models/device_components.py +++ b/netbox/dcim/models/device_components.py @@ -2,7 +2,7 @@ from functools import cached_property from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation from django.contrib.postgres.fields import ArrayField -from django.core.exceptions import ValidationError +from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.core.validators import MaxValueValidator, MinValueValidator from django.db import models from django.db.models import Sum @@ -307,11 +307,12 @@ class PathEndpoint(models.Model): `connected_endpoints()` is a convenience method for returning the destination of the associated CablePath, if any. """ + _path = models.ForeignKey( to='dcim.CablePath', on_delete=models.SET_NULL, null=True, - blank=True + blank=True, ) class Meta: @@ -323,11 +324,14 @@ class PathEndpoint(models.Model): # Construct the complete path (including e.g. bridged interfaces) while origin is not None: - - if origin._path is None: + # Go through the public accessor rather than dereferencing `_path` + # directly. During cable edits, CablePath rows can be deleted and + # recreated while this endpoint instance is still in memory. + cable_path = origin.path + if cable_path is None: break - path.extend(origin._path.path_objects) + path.extend(cable_path.path_objects) # If the path ends at a non-connected pass-through port, pad out the link and far-end terminations if len(path) % 3 == 1: @@ -336,8 +340,8 @@ class PathEndpoint(models.Model): elif len(path) % 3 == 2: path.insert(-1, []) - # Check for a bridged relationship to continue the trace - destinations = origin._path.destinations + # Check for a bridged relationship to continue the trace. + destinations = cable_path.destinations if len(destinations) == 1: origin = getattr(destinations[0], 'bridge', None) else: @@ -348,14 +352,42 @@ class PathEndpoint(models.Model): @property def path(self): - return self._path + """ + Return this endpoint's current CablePath, if any. + + `_path` is a denormalized reference that is updated from CablePath + save/delete handlers, including queryset.update() calls on origin + endpoints. That means an already-instantiated endpoint can briefly hold + a stale in-memory `_path` relation while the database already points to + a different CablePath (or to no path at all). + + If the cached relation points to a CablePath that has just been + deleted, refresh only the `_path` field from the database and retry. + This keeps the fix cheap and narrowly scoped to the denormalized FK. + """ + if self._path_id is None: + return None + + try: + return self._path + except ObjectDoesNotExist: + # Refresh only the denormalized FK instead of the whole model. + # The expected problem here is in-memory staleness during path + # rebuilds, not persistent database corruption. + self.refresh_from_db(fields=['_path']) + return self._path if self._path_id else None @cached_property def connected_endpoints(self): """ - Caching accessor for the attached CablePath's destination (if any) + Caching accessor for the attached CablePath's destinations (if any). + + Always route through `path` so stale in-memory `_path` references are + repaired before we cache the result for the lifetime of this instance. """ - return self._path.destinations if self._path else [] + if cable_path := self.path: + return cable_path.destinations + return [] # diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index f676ae5df..1c0aa04c3 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -5,6 +5,7 @@ from circuits.models import * from core.models import ObjectType from dcim.choices import * from dcim.models import * +from extras.events import serialize_for_event from extras.models import CustomField from ipam.models import Prefix from netbox.choices import WeightUnitChoices @@ -1345,6 +1346,65 @@ class CableTestCase(TestCase): self.assertEqual(a_terms, [interface1]) self.assertEqual(b_terms, [interface2]) + @tag('regression') # #21498 + def test_path_refreshes_replaced_cablepath_reference(self): + """ + An already-instantiated interface should refresh its denormalized + `_path` foreign key when the referenced CablePath row has been + replaced in the database. + """ + stale_interface = Interface.objects.get(device__name='TestDevice1', name='eth0') + old_path = CablePath.objects.get(pk=stale_interface._path_id) + + new_path = CablePath( + path=old_path.path, + is_active=old_path.is_active, + is_complete=old_path.is_complete, + is_split=old_path.is_split, + ) + old_path_id = old_path.pk + old_path.delete() + new_path.save() + + # The old CablePath no longer exists + self.assertFalse(CablePath.objects.filter(pk=old_path_id).exists()) + + # The already-instantiated interface still points to the deleted path + # until the accessor refreshes `_path` from the database. + self.assertEqual(stale_interface._path_id, old_path_id) + self.assertEqual(stale_interface.path.pk, new_path.pk) + + @tag('regression') # #21498 + def test_serialize_for_event_handles_stale_cablepath_reference_after_retermination(self): + """ + Serializing an interface whose previously cached `_path` row has been + deleted during cable retermination must not raise. + """ + stale_interface = Interface.objects.get(device__name='TestDevice2', name='eth0') + old_path_id = stale_interface._path_id + new_peer = Interface.objects.get(device__name='TestDevice2', name='eth1') + cable = stale_interface.cable + + self.assertIsNotNone(cable) + self.assertIsNotNone(old_path_id) + self.assertEqual(stale_interface.cable_end, 'B') + + cable.b_terminations = [new_peer] + cable.save() + + # The old CablePath was deleted during retrace. + self.assertFalse(CablePath.objects.filter(pk=old_path_id).exists()) + + # The stale in-memory instance still holds the deleted FK value. + self.assertEqual(stale_interface._path_id, old_path_id) + + # Serialization must not raise ObjectDoesNotExist. Because this interface + # was the former B-side termination, it is now disconnected. + data = serialize_for_event(stale_interface) + self.assertIsNone(data['connected_endpoints']) + self.assertIsNone(data['connected_endpoints_type']) + self.assertFalse(data['connected_endpoints_reachable']) + class VirtualDeviceContextTestCase(TestCase): diff --git a/netbox/extras/events.py b/netbox/extras/events.py index 782b29633..55e8b83e7 100644 --- a/netbox/extras/events.py +++ b/netbox/extras/events.py @@ -25,16 +25,54 @@ logger = logging.getLogger('netbox.events_processor') class EventContext(UserDict): """ - A custom dictionary that automatically serializes its associated object on demand. + Dictionary-compatible wrapper for queued events that lazily serializes + ``event['data']`` on first access. + + Backward-compatible with the plain-dict interface expected by existing + EVENTS_PIPELINE consumers. When the same object is enqueued more than once + in a single request, the serialization source is updated so consumers see + the latest state. """ - # We're emulating a dictionary here (rather than using a custom class) because prior to NetBox v4.5.2, events were - # queued as dictionaries for processing by handles in EVENTS_PIPELINE. We need to avoid introducing any breaking - # changes until a suitable minor release. + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + # Track which model instance should be serialized if/when `data` is + # requested. This may be refreshed on duplicate enqueue, while leaving + # the public `object` entry untouched for compatibility. + self._serialization_source = None + if 'object' in self: + self._serialization_source = super().__getitem__('object') + + def refresh_serialization_source(self, instance): + """ + Point lazy serialization at a fresher instance, invalidating any + already-materialized ``data``. + """ + self._serialization_source = instance + # UserDict.__contains__ checks the backing dict directly, so `in` + # does not trigger __getitem__'s lazy serialization. + if 'data' in self: + del self['data'] + + def freeze_data(self, instance): + """ + Eagerly serialize and cache the payload for delete events, where the + object may become inaccessible after deletion. + """ + super().__setitem__('data', serialize_for_event(instance)) + self._serialization_source = None + def __getitem__(self, item): if item == 'data' and 'data' not in self: - data = serialize_for_event(self['object']) - self.__setitem__('data', data) + # Materialize the payload only when an event consumer asks for it. + # + # On coalesced events, use the latest explicitly queued instance so + # webhooks/scripts/notifications observe the final queued state for + # that object within the request. + source = self._serialization_source or super().__getitem__('object') + super().__setitem__('data', serialize_for_event(source)) + return super().__getitem__(item) @@ -76,8 +114,9 @@ def get_snapshots(instance, event_type): def enqueue_event(queue, instance, request, event_type): """ - Enqueue a serialized representation of a created/updated/deleted object for the processing of - events once the request has completed. + Enqueue (or coalesce) an event for a created/updated/deleted object. + + Events are processed after the request completes. """ # Bail if this type of object does not support event rules if not has_feature(instance, 'event_rules'): @@ -88,11 +127,18 @@ def enqueue_event(queue, instance, request, event_type): assert instance.pk is not None key = f'{app_label}.{model_name}:{instance.pk}' + if key in queue: queue[key]['snapshots']['postchange'] = get_snapshots(instance, event_type)['postchange'] - # If the object is being deleted, update any prior "update" event to "delete" + + # If the object is being deleted, convert any prior update event into a + # delete event and freeze the payload before the object (or related + # rows) become inaccessible. if event_type == OBJECT_DELETED: queue[key]['event_type'] = event_type + else: + # Keep the public `object` entry stable for compatibility. + queue[key].refresh_serialization_source(instance) else: queue[key] = EventContext( object_type=ObjectType.objects.get_for_model(instance), @@ -106,9 +152,11 @@ def enqueue_event(queue, instance, request, event_type): username=request.user.username, # DEPRECATED, will be removed in NetBox v4.7.0 request_id=request.id, # DEPRECATED, will be removed in NetBox v4.7.0 ) - # Force serialization of objects prior to them actually being deleted + + # For delete events, eagerly serialize the payload before the row is gone. + # This covers both first-time enqueues and coalesced update→delete promotions. if event_type == OBJECT_DELETED: - queue[key]['data'] = serialize_for_event(instance) + queue[key].freeze_data(instance) def process_event_rules(event_rules, object_type, event): @@ -133,9 +181,9 @@ def process_event_rules(event_rules, object_type, event): if not event_rule.eval_conditions(event['data']): continue - # Compile event data - event_data = event_rule.action_data or {} - event_data.update(event['data']) + # Merge rule-specific action_data with the event payload. + # Copy to avoid mutating the rule's stored action_data dict. + event_data = {**(event_rule.action_data or {}), **event['data']} # Webhooks if event_rule.action_type == EventRuleActionChoices.WEBHOOK: diff --git a/netbox/extras/tests/test_event_rules.py b/netbox/extras/tests/test_event_rules.py index b6abf4c85..ae3f802c3 100644 --- a/netbox/extras/tests/test_event_rules.py +++ b/netbox/extras/tests/test_event_rules.py @@ -1,8 +1,10 @@ import json import uuid +from unittest import skipIf from unittest.mock import Mock, patch import django_rq +from django.conf import settings from django.http import HttpResponse from django.test import RequestFactory from django.urls import reverse @@ -343,6 +345,7 @@ class EventRuleTest(APITestCase): self.assertEqual(job.kwargs['snapshots']['prechange']['name'], sites[i].name) self.assertEqual(job.kwargs['snapshots']['prechange']['tags'], ['Bar', 'Foo']) + @skipIf('netbox.tests.dummy_plugin' not in settings.PLUGINS, 'dummy_plugin not in settings.PLUGINS') def test_send_webhook(self): request_id = uuid.uuid4() @@ -426,6 +429,97 @@ class EventRuleTest(APITestCase): self.assertEqual(job.kwargs['object_type'], script_type) self.assertEqual(job.kwargs['username'], self.user.username) + def test_duplicate_enqueue_refreshes_lazy_payload(self): + """ + When the same object is enqueued more than once in a single request, + lazy serialization should use the most recently enqueued instance while + preserving the original event['object'] reference. + """ + request = RequestFactory().get(reverse('dcim:site_add')) + request.id = uuid.uuid4() + request.user = self.user + + site = Site.objects.create(name='Site 1', slug='site-1') + stale_site = Site.objects.get(pk=site.pk) + + queue = {} + enqueue_event(queue, stale_site, request, OBJECT_UPDATED) + + event = queue[f'dcim.site:{site.pk}'] + + # Data should not be materialized yet (lazy serialization) + self.assertNotIn('data', event.data) + + fresh_site = Site.objects.get(pk=site.pk) + fresh_site.description = 'foo' + fresh_site.save() + + enqueue_event(queue, fresh_site, request, OBJECT_UPDATED) + + # The original object reference should be preserved + self.assertIs(event['object'], stale_site) + + # But serialized data should reflect the fresher instance + self.assertEqual(event['data']['description'], 'foo') + self.assertEqual(event['snapshots']['postchange']['description'], 'foo') + + def test_duplicate_enqueue_invalidates_materialized_data(self): + """ + If event['data'] has already been materialized before a second enqueue + for the same object, the stale payload should be discarded and rebuilt + from the fresher instance on next access. + """ + request = RequestFactory().get(reverse('dcim:site_add')) + request.id = uuid.uuid4() + request.user = self.user + + site = Site.objects.create(name='Site 1', slug='site-1') + + queue = {} + enqueue_event(queue, site, request, OBJECT_UPDATED) + + event = queue[f'dcim.site:{site.pk}'] + + # Force early materialization + self.assertEqual(event['data']['description'], '') + + # Now update and re-enqueue + fresh_site = Site.objects.get(pk=site.pk) + fresh_site.description = 'updated' + fresh_site.save() + + enqueue_event(queue, fresh_site, request, OBJECT_UPDATED) + + # Stale data should have been invalidated; new access should reflect update + self.assertEqual(event['data']['description'], 'updated') + + def test_update_then_delete_enqueue_freezes_payload(self): + """ + When an update event is coalesced with a subsequent delete, the event + type should be promoted to OBJECT_DELETED and the payload should be + eagerly frozen (since the object will be inaccessible after deletion). + """ + request = RequestFactory().get(reverse('dcim:site_add')) + request.id = uuid.uuid4() + request.user = self.user + + site = Site.objects.create(name='Site 1', slug='site-1') + + queue = {} + enqueue_event(queue, site, request, OBJECT_UPDATED) + + event = queue[f'dcim.site:{site.pk}'] + + enqueue_event(queue, site, request, OBJECT_DELETED) + + # Event type should have been promoted + self.assertEqual(event['event_type'], OBJECT_DELETED) + + # Data should already be materialized (frozen), not lazy + self.assertIn('data', event.data) + self.assertEqual(event['data']['name'], 'Site 1') + self.assertIsNone(event['snapshots']['postchange']) + def test_duplicate_triggers(self): """ Test for erroneous duplicate event triggers resulting from saving an object multiple times