Fix stale and incorrect pre-change snapshots

- Snapshot a stored Cable before update_terminations() rewrites its terminations
- Discard the creation snapshot of a new Cable with a preset pk
- Discard snapshots taken by the wireless link & MAC address handlers and sync() after saving
- Keep a caller's existing snapshot in the wireless link & MAC address handlers
- Discard a snapshot read from the database only after its event has been enqueued

netbox/core/signals.py

@@ -155,8 +155,6 @@ def handle_changed_object(sender, instance, **kwargs):
         OBJECT_DELETED: ObjectChangeActionChoices.ACTION_DELETE,
     }[event_type]
     objectchange = instance.to_objectchange(action)
-    if instance.__dict__.pop('_prechange_snapshot_from_db', False):
-        del instance._prechange_snapshot
     object_change_id = None
     # If this is a many-to-many field change, check for a previous ObjectChange instance recorded
     # for this object by this request and update it
@@ -185,6 +183,10 @@ def handle_changed_object(sender, instance, **kwargs):
     enqueue_event(queue, instance, request, event_type, object_change_id=object_change_id)
     events_queue.set(queue)
 
+    # Discard a snapshot read from the database only once the event has captured it
+    if instance.__dict__.pop('_prechange_snapshot_from_db', False):
+        instance.__dict__.pop('_prechange_snapshot', None)
+
     # Increment metric counters
     if event_type == OBJECT_CREATED:
         model_inserts.labels(instance._meta.model_name).inc()

netbox/core/tests/test_signals.py

@@ -107,6 +107,20 @@ class HandleChangedObjectSignalTestCase(TestCase):
         self.assertEqual([oc.postchange_data['description'] for oc in ocs], ['first', 'second'])
         self.assertFalse(hasattr(site, '_prechange_snapshot'))
 
+    def test_update_without_snapshot_enqueues_prechange_from_database(self):
+        """The event for an update saved without a snapshot carries its pre-change data. Ref: #23617"""
+        site = Site.objects.create(name='Site 1', slug='site-1')
+        request = _build_request(self.user)
+
+        with self.assertLogs('netbox.core.signals', level='WARNING'), event_tracking(request):
+            site.description = 'updated'
+            site.save()
+            snapshots = events_queue.get()[f'dcim.site:{site.pk}']['snapshots']
+
+        self.assertEqual(snapshots['prechange']['description'], '')
+        self.assertEqual(snapshots['postchange']['description'], 'updated')
+        self.assertFalse(hasattr(site, '_prechange_snapshot'))
+
     def test_update_keeps_existing_snapshot(self):
         site = Site.objects.create(name='Site 1', slug='site-1')
         request = _build_request(self.user)

netbox/dcim/models/cables.py

@@ -21,6 +21,7 @@ from dcim.exceptions import UnsupportedCablePath
 from dcim.fields import PathField
 from dcim.utils import decompile_path_node, object_to_path_node, rebuild_cable_paths
 from netbox.choices import ColorChoices
+from netbox.context import current_request
 from netbox.models import ChangeLoggedModel, PrimaryModel
 from utilities.conversion import to_meters
 from utilities.data import normalize_update_fields
@@ -353,6 +354,8 @@ class Cable(PrimaryModel):
 
     def save(self, *args, force_insert=False, force_update=False, using=None, update_fields=None):
         _created = self.pk is None
+        # Unlike _created, also true for a new Cable with a preset pk (e.g. one restored by deserialize_object())
+        adding = self._state.adding
         save_kwargs = {
             'using': using,
             'update_fields': update_fields,
@@ -391,21 +394,28 @@ class Cable(PrimaryModel):
         profile_written = update_fields is None or 'profile' in update_fields
 
         # If this is a new Cable, save it before attempting to create its CableTerminations
-        if self._state.adding:
+        if adding:
             super().save(*args, force_insert=True, **save_kwargs)
             # Update the private PK used in __str__()
             self._pk = self.pk
             # Record the pre-termination state for the update made by the save below
             self.snapshot()
 
-        if profile_written and self._orig_profile != self.profile:
-            self.update_terminations(force=True)
-        elif self._terminations_modified:
-            self.update_terminations()
+        force_terminations = profile_written and self._orig_profile != self.profile
+        if force_terminations or self._terminations_modified:
+            # update_terminations() writes before the pre_save fallback runs, so snapshot the stored Cable now
+            if current_request.get() is not None and not hasattr(self, '_prechange_snapshot'):
+                db = using or router.db_for_write(Cable, instance=self)
+                original = Cable._base_manager.using(db).get(pk=self.pk)
+                original.snapshot()
+                self._prechange_snapshot = original._prechange_snapshot
+                # Discarded by handle_changed_object() after this save, like the fallback's own snapshot
+                self._prechange_snapshot_from_db = True
+            self.update_terminations(force=force_terminations)
 
         super().save(*args, force_update=True, **save_kwargs)
         # Discard the pre-termination snapshot so that a later save of this instance takes a fresh one
-        if _created:
+        if adding:
             self.__dict__.pop('_prechange_snapshot', None)
 
         try:

netbox/dcim/signals.py

@@ -498,6 +498,10 @@ def update_mac_address_interface(instance, created, raw, **kwargs):
     assign the MACAddress to the interface.
     """
     if created and not raw and instance.primary_mac_address:
-        instance.primary_mac_address.snapshot()
+        # Keep any snapshot the caller took before making unsaved changes to the MAC address
+        if not hasattr(instance.primary_mac_address, '_prechange_snapshot'):
+            instance.primary_mac_address.snapshot()
         instance.primary_mac_address.assigned_object = instance
         instance.primary_mac_address.save()
+        # Discard the snapshot so that a later save of the MAC address takes a fresh one
+        instance.primary_mac_address.__dict__.pop('_prechange_snapshot', None)

netbox/dcim/tests/test_models.py

@@ -73,6 +73,29 @@ class MACAddressTestCase(TestCase):
         )
         self.assertIsNone(change.prechange_data['assigned_object_id'])
         self.assertEqual(change.postchange_data['assigned_object_id'], interface.pk)
+        self.assertFalse(hasattr(mac, '_prechange_snapshot'))
+
+    @tag('regression')  # Ref: #23617
+    def test_primary_mac_assignment_keeps_existing_snapshot(self):
+        mac = MACAddress.objects.create(mac_address='1234567890ce')
+        request = RequestFactory().get('/')
+        request.id = uuid.uuid4()
+        request.user = User.objects.create_user(username='testuser')
+
+        with event_tracking(request):
+            mac.snapshot()
+            mac.description = 'updated'
+            Interface.objects.create(
+                device=self.interface.device, name='Interface 2', type=InterfaceTypeChoices.TYPE_1GE_FIXED,
+                primary_mac_address=mac
+            )
+
+        change = ObjectChange.objects.get(
+            changed_object_type=ObjectType.objects.get_for_model(MACAddress),
+            changed_object_id=mac.pk,
+        )
+        self.assertEqual(change.prechange_data['description'], '')
+        self.assertEqual(change.postchange_data['description'], 'updated')
 
     @tag('regression')
     def test_clean_will_not_allow_removal_of_assigned_object_if_primary(self):
@@ -3471,6 +3494,39 @@ class CableDisconnectChangeLoggingTestCase(TestCase):
         self.assertEqual(len(change.prechange_data['b_terminations']), 1)
         self.assertEqual(change.postchange_data['label'], 'x')
 
+    @tag('regression')  # Ref: #23617
+    def test_resaving_new_cable_with_preset_pk_records_current_prechange(self):
+        # A new Cable with a preset pk (e.g. restored by deserialize_object()) must also discard its creation snapshot
+        with self.assertLogs('netbox.core.signals', level='WARNING'), event_tracking(self._request()):
+            cable = Cable(pk=1000000, a_terminations=[self.interface1], b_terminations=[self.interface2])
+            cable.save()
+            cable.label = 'x'
+            cable.save()
+
+        change = self._updates(cable).last()
+        self.assertEqual(len(change.prechange_data['a_terminations']), 1)
+        self.assertEqual(len(change.prechange_data['b_terminations']), 1)
+        self.assertEqual(change.postchange_data['label'], 'x')
+
+    @tag('regression')  # Ref: #23617
+    def test_termination_update_without_snapshot_records_prechange(self):
+        cable = self._connect(self.interface1, self.interface2)
+        old_b_termination = CableTermination.objects.get(cable=cable, cable_end=CableEndChoices.SIDE_B)
+
+        request = self._request()
+        with event_tracking(request):
+            cable = Cable.objects.get(pk=cable.pk)
+            cable.label = 'x'
+            cable.b_terminations = [self.interface3]
+            cable.save()
+
+        change = self._updates(cable).get(request_id=request.id)
+        self.assertEqual(change.prechange_data['b_terminations'], [old_b_termination.pk])
+        self.assertEqual(change.prechange_data['label'], '')
+        self.assertNotEqual(change.postchange_data['b_terminations'], [old_b_termination.pk])
+        self.assertEqual(change.postchange_data['label'], 'x')
+        self.assertFalse(hasattr(cable, '_prechange_snapshot'))
+
     @tag('regression')  # Ref: netbox-branching#631
     def test_cable_deletion_records_disconnect(self):
         cable = self._connect(self.interface1, self.interface2)

netbox/netbox/models/features.py

@@ -703,12 +703,16 @@ class SyncedDataMixin(models.Model):
         :param save: If true, save() will be called after data has been synchronized
         """
         # ManagedFile is not change-logged and so has no snapshot(); keep any snapshot already taken
-        if save and hasattr(self, 'snapshot') and not hasattr(self, '_prechange_snapshot'):
+        take_snapshot = save and hasattr(self, 'snapshot') and not hasattr(self, '_prechange_snapshot')
+        if take_snapshot:
             self.snapshot()
         self.sync_data()
         self.data_synced = timezone.now()
         if save:
             self.save()
+        # Discard the snapshot taken above so that a later save of this object takes a fresh one
+        if take_snapshot:
+            self.__dict__.pop('_prechange_snapshot', None)
     sync.alters_data = True
 
     def sync_data(self):

netbox/netbox/tests/test_model_features.py

@@ -164,8 +164,9 @@ class ModelFeaturesTestCase(TestCase):
         request.id = uuid.uuid4()
         request.user = User.objects.create_user(username='testuser')
 
+        template = ConfigTemplate.objects.get(pk=obj.pk)
         with event_tracking(request):
-            ConfigTemplate.objects.get(pk=obj.pk).sync(save=True)
+            template.sync(save=True)
 
         change = ObjectChange.objects.get(
             changed_object_type=ContentType.objects.get_for_model(ConfigTemplate),
@@ -173,6 +174,7 @@ class ModelFeaturesTestCase(TestCase):
         )
         self.assertEqual(change.prechange_data['template_code'], 'original\n')
         self.assertEqual(change.postchange_data['template_code'], 'updated\n')
+        self.assertFalse(hasattr(template, '_prechange_snapshot'))
 
     @patch.object(ManagedFile, 'sync_data')
     def test_synceddatamixin_sync_without_change_logging(self, mock_sync_data):

netbox/wireless/signals.py

@@ -27,14 +27,20 @@ def update_connected_interfaces(instance, created, raw=False, **kwargs):
 
     if instance.interface_a.wireless_link != instance:
         logger.debug(f"Updating interface A for wireless link {instance}")
-        instance.interface_a.snapshot()
+        # Keep any snapshot the caller took before making unsaved changes to the interface
+        if not hasattr(instance.interface_a, '_prechange_snapshot'):
+            instance.interface_a.snapshot()
         instance.interface_a.wireless_link = instance
         instance.interface_a.save()
+        # Discard the snapshot so that a later save of the interface takes a fresh one
+        instance.interface_a.__dict__.pop('_prechange_snapshot', None)
     if instance.interface_b.wireless_link != instance:
         logger.debug(f"Updating interface B for wireless link {instance}")
-        instance.interface_b.snapshot()
+        if not hasattr(instance.interface_b, '_prechange_snapshot'):
+            instance.interface_b.snapshot()
         instance.interface_b.wireless_link = instance
         instance.interface_b.save()
+        instance.interface_b.__dict__.pop('_prechange_snapshot', None)
 
     # Create/update cable paths
     if created:

netbox/wireless/tests/test_models.py

@@ -90,6 +90,29 @@ class WirelessLinkTestCase(TestCase):
             )
             self.assertIsNone(change.prechange_data['wireless_link'])
             self.assertEqual(change.postchange_data['wireless_link'], wireless_link.pk)
+            self.assertFalse(hasattr(interface, '_prechange_snapshot'))
+
+    # Regression test for #23617
+    def test_creating_wireless_link_keeps_existing_interface_snapshot(self):
+        interfaces = [
+            Interface.objects.create(device=self.device, name=name, type=InterfaceTypeChoices.TYPE_80211AC)
+            for name in ('radio0', 'radio1')
+        ]
+        request = RequestFactory().post('/')
+        request.id = uuid4()
+        request.user = self.user
+
+        with event_tracking(request):
+            interfaces[0].snapshot()
+            interfaces[0].description = 'updated'
+            WirelessLink.objects.create(interface_a=interfaces[0], interface_b=interfaces[1], ssid='LINK1')
+
+        change = ObjectChange.objects.get(
+            changed_object_type=ContentType.objects.get_for_model(Interface),
+            changed_object_id=interfaces[0].pk,
+        )
+        self.assertEqual(change.prechange_data['description'], '')
+        self.assertEqual(change.postchange_data['description'], 'updated')
 
     def test_resaving_wireless_link_does_not_log_connected_interfaces(self):
         interface_a = Interface.objects.create(