Fixes #23013: Avoid denormalization refreshes for location assignment when not needed

netbox/dcim/signals.py

@@ -102,9 +102,23 @@ def handle_location_site_change(instance, created, using=None, **kwargs):
     transaction opened here. For the same reason the new Site is assigned by ID: reading
     instance.site would fetch the related object over a router-selected connection whenever
     the save left it uncached (a rename, say).
+
+    When the values read from the database immediately before this save show that the Site
+    assignment is unchanged, the propagation is skipped: every value written below is
+    derived from it, so there is nothing for the descendants to pick up.
     """
     if created:
         return
+
+    # Skip the propagation when this save left the Site assignment untouched. The pre-save
+    # value is read from the database by cache_presave_scope_fields() immediately before the
+    # write, with the row locked, so the comparison holds even when overlapping saves race on
+    # the same object. The stash exists only for saves made inside a transaction; when it's
+    # absent (autocommit saves), propagate unconditionally.
+    prev = getattr(instance, '_presave_scope_fields', None)
+    if prev is not None and prev['site_id'] == instance.site_id:
+        return
+
     with transaction.atomic(using=using, savepoint=False):
         instance.get_descendants().using(using).update(site_id=instance.site_id)
         # Materialized once so every statement below sees the same membership, even if a
@@ -119,56 +133,54 @@ def handle_location_site_change(instance, created, using=None, **kwargs):
             model.objects.using(using).filter(device__location__in=locations).update(_site_id=instance.site_id)
 
         # Objects scoped to descendant Locations receive no post_save of their own from the
-        # queryset updates above, so their cached scope fields are updated here whenever the
-        # Site assignment has actually changed. (Objects scoped to this Location itself are
-        # recomputed by sync_cached_scope_fields on this same save.) Values are read fresh
-        # from the database rather than taken from the saved instance, whose cached site
-        # relation may be stale.
-        prev = getattr(instance, '_presave_scope_fields', None)
-        if prev is None or prev['site_id'] != instance.site_id:
-            # Lock the destination Site (without blocking FK inserts that reference it) so
-            # a concurrent scope change on that Site serializes against this move; an
-            # unlocked read could stamp region/group values from before that change.
-            site = (
-                Site.objects.using(using)
-                .filter(pk=instance.site_id)
-                .select_for_update(no_key=True)
-                .values('region_id', 'group_id')
-                .first()
-            )
-            if site is not None:
-                # Select rows through the authoritative scope rather than the cached
-                # _location, which may itself be stale; scope_id doubles as the correct
-                # _location value for Location-scoped rows.
-                # The content type is read on the saving connection as well, since its ID is
-                # fed straight into the pinned filter below; a router-selected read could
-                # return an ID which means something else on that connection.
-                location_ct = ContentType.objects.db_manager(using).get_for_model(Location)
-                for model in (Prefix, Cluster, WirelessLAN):
-                    model.objects.using(using).filter(scope_type=location_ct, scope_id__in=locations).update(
-                        _location_id=F('scope_id'),
-                        _site_id=instance.site_id,
-                        _region_id=site['region_id'],
-                        _site_group_id=site['group_id'],
-                    )
-
-                # CircuitTermination caches the same ancestry under its own generic
-                # termination field rather than CachedScopeMixin.scope, so it is invisible to
-                # both the loop above and sync_cached_scope_fields(). Its own rows are
-                # refreshed by the denormalized-field registry, but only their _site: _region
-                # and _site_group are mapped off the separate _site registration, which fires
-                # on a Site save. Rows scoped to descendant Locations get nothing at all, as
-                # the get_descendants() update above fires no post_save — which is why the
-                # include_self=True membership is load-bearing here.
-                CircuitTermination.objects.using(using).filter(
-                    termination_type=location_ct, termination_id__in=locations
-                ).update(
-                    _location_id=F('termination_id'),
+        # queryset updates above, so their cached scope fields are updated here. (Objects
+        # scoped to this Location itself are recomputed by sync_cached_scope_fields on this
+        # same save.) Values are read fresh from the database rather than taken from the
+        # saved instance, whose cached site relation may be stale.
+        #
+        # Lock the destination Site (without blocking FK inserts that reference it) so a
+        # concurrent scope change on that Site serializes against this move; an unlocked
+        # read could stamp region/group values from before that change.
+        site = (
+            Site.objects.using(using)
+            .filter(pk=instance.site_id)
+            .select_for_update(no_key=True)
+            .values('region_id', 'group_id')
+            .first()
+        )
+        if site is not None:
+            # Select rows through the authoritative scope rather than the cached
+            # _location, which may itself be stale; scope_id doubles as the correct
+            # _location value for Location-scoped rows.
+            # The content type is read on the saving connection as well, since its ID is
+            # fed straight into the pinned filter below; a router-selected read could
+            # return an ID which means something else on that connection.
+            location_ct = ContentType.objects.db_manager(using).get_for_model(Location)
+            for model in (Prefix, Cluster, WirelessLAN):
+                model.objects.using(using).filter(scope_type=location_ct, scope_id__in=locations).update(
+                    _location_id=F('scope_id'),
                     _site_id=instance.site_id,
                     _region_id=site['region_id'],
                     _site_group_id=site['group_id'],
                 )
 
+            # CircuitTermination caches the same ancestry under its own generic
+            # termination field rather than CachedScopeMixin.scope, so it is invisible to
+            # both the loop above and sync_cached_scope_fields(). Its own rows are
+            # refreshed by the denormalized-field registry, but only their _site: _region
+            # and _site_group are mapped off the separate _site registration, which fires
+            # on a Site save. Rows scoped to descendant Locations get nothing at all, as
+            # the get_descendants() update above fires no post_save — which is why the
+            # include_self=True membership is load-bearing here.
+            CircuitTermination.objects.using(using).filter(
+                termination_type=location_ct, termination_id__in=locations
+            ).update(
+                _location_id=F('termination_id'),
+                _site_id=instance.site_id,
+                _region_id=site['region_id'],
+                _site_group_id=site['group_id'],
+            )
+
 
 @receiver(post_save, sender=Rack)
 def handle_rack_site_change(instance, created, using=None, **kwargs):

netbox/dcim/tests/test_signals.py

@@ -125,6 +125,103 @@ class LocationSiteChangeSignalTestCase(TestCase):
         # Should not raise — newly-created locations have no descendants.
         Location.objects.create(name='New', slug='new', site=self.site_a)
 
+    def _capture_propagation_updates(self, location):
+        """
+        Save the Location and return the UPDATE statements it issued against the tables this
+        handler propagates to. dcim_cabletermination and dcim_location are excluded: the
+        denormalized-field registry (netbox.denormalized) rewrites the former's _location on
+        every Location save, and the latter carries the saved row's own UPDATE, so neither
+        distinguishes a propagation from a plain save.
+        """
+        tables = ('dcim_rack', 'dcim_device', 'dcim_powerpanel', *(
+            model._meta.db_table for model in signals.COMPONENT_MODELS
+        ))
+
+        with CaptureQueriesContext(connection) as ctx:
+            location.save()
+
+        return {
+            table for table in tables
+            for q in ctx.captured_queries
+            if q['sql'].startswith(f'UPDATE "{table}"')
+        }
+
+    def _seed_location_with_children(self):
+        location = Location.objects.create(name='Parent', slug='parent', site=self.site_a)
+        device = Device.objects.create(
+            name='Device',
+            site=self.site_a,
+            location=location,
+            device_type=self.device_type,
+            role=self.device_role,
+        )
+        Interface.objects.create(device=device, name='Interface 1')
+        Rack.objects.create(name='Rack', site=self.site_a, location=location)
+        PowerPanel.objects.create(name='Panel', site=self.site_a, location=location)
+        return location
+
+    def test_unchanged_site_skips_propagation(self):
+        # Every value the handler writes is derived from the Location's site assignment, so a
+        # save which leaves it alone has nothing to propagate and must not rewrite a single
+        # descendant row. Rewriting them is not merely wasted work: PostgreSQL writes a new
+        # tuple version for every row an UPDATE matches, and holds a row lock on each for the
+        # remainder of the transaction.
+        location = self._seed_location_with_children()
+        location.description = 'updated'
+
+        self.assertEqual(self._capture_propagation_updates(location), set())
+
+    def test_changed_site_propagates(self):
+        # Counterpart to the test above, which would pass vacuously if these UPDATEs stopped
+        # being issued (or their tables were renamed) rather than merely being skipped.
+        location = self._seed_location_with_children()
+        location.site = self.site_b
+
+        updated_tables = self._capture_propagation_updates(location)
+
+        self.assertEqual(updated_tables, {'dcim_rack', 'dcim_device', 'dcim_powerpanel', *(
+            model._meta.db_table for model in signals.COMPONENT_MODELS
+        )})
+
+
+class LocationSiteChangeAutocommitTestCase(TransactionTestCase):
+    """
+    Exercise the autocommit save path, which TestCase cannot reach (it wraps every test in a
+    transaction). Outside an atomic block the pre-save read and the save's UPDATE run in
+    separate transactions, so the skip guard is disabled there: the stash is cleared and the
+    propagation runs unconditionally.
+
+    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 test_autocommit_noop_save_always_propagates(self):
+        site = Site.objects.create(name='Site', slug='site')
+        other_site = Site.objects.create(name='Other Site', slug='other-site')
+        manufacturer = Manufacturer.objects.create(name='Manufacturer', slug='manufacturer')
+        device_type = DeviceType.objects.create(manufacturer=manufacturer, model='Device Type')
+        device_role = DeviceRole.objects.create(name='Device Role', slug='device-role')
+        location = Location.objects.create(name='Loc', slug='loc', site=site)
+        device = Device.objects.create(
+            name='Device', site=site, location=location, device_type=device_type, role=device_role
+        )
+        interface = Interface.objects.create(device=device, name='Interface 1')
+
+        # A transactional save first, so the instance carries a stash. The subsequent
+        # autocommit save must clear it rather than compare against a previous save's values.
+        with transaction.atomic():
+            location.save()
+
+        # Poison a cached column via a signal-less update; an unconditional propagation
+        # repairs it.
+        Interface.objects.filter(pk=interface.pk).update(_site=other_site)
+
+        location.save()  # Autocommit: no stash, unconditional propagation
+
+        interface.refresh_from_db()
+        self.assertEqual(interface._site, site)
+
 
 class RackSiteChangeSignalTestCase(TestCase):
     """