From 57094ffdfd34313980e4f98bc641b9d92f91fc72 Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 15 Jun 2026 09:18:17 -0700 Subject: [PATCH 1/4] #21355 - Handle updates to denormalized data via PostgreSQL triggers --- docs/development/application-registry.md | 4 - netbox/circuits/apps.py | 12 --- .../0058_denormalization_triggers.py | 33 ++++++++ netbox/circuits/tests/test_models.py | 77 ++++++++++++++++++- netbox/dcim/apps.py | 18 +---- .../0239_denormalization_triggers.py | 38 +++++++++ netbox/dcim/tests/test_signals.py | 61 +++++++++++++++ netbox/ipam/apps.py | 12 --- .../0091_denormalization_triggers.py | 33 ++++++++ netbox/ipam/tests/test_signals.py | 70 +++++++++++++++++ netbox/netbox/denormalized.py | 57 -------------- netbox/netbox/registry.py | 1 - netbox/utilities/migration.py | 75 +++++++++++++++++- 13 files changed, 386 insertions(+), 105 deletions(-) create mode 100644 netbox/circuits/migrations/0058_denormalization_triggers.py create mode 100644 netbox/dcim/migrations/0239_denormalization_triggers.py create mode 100644 netbox/ipam/migrations/0091_denormalization_triggers.py delete mode 100644 netbox/netbox/denormalized.py diff --git a/docs/development/application-registry.md b/docs/development/application-registry.md index 7946d9df4..5a7057d43 100644 --- a/docs/development/application-registry.md +++ b/docs/development/application-registry.md @@ -16,10 +16,6 @@ A dictionary mapping of models to foreign keys with which cached counter fields A dictionary mapping data backend types to their respective classes. These are used to interact with [remote data sources](../models/core/datasource.md). -### `denormalized_fields` - -Stores registration made using `netbox.denormalized.register()`. For each model, a list of related models and their field mappings is maintained to facilitate automatic updates. - ### `filtersets` A dictionary mapping each model (identified by its app and label) to its filterset class, if one has been registered for it. Filtersets are registered using the `@register_filterset` decorator. diff --git a/netbox/circuits/apps.py b/netbox/circuits/apps.py index 001470b2e..d1d4fe9e7 100644 --- a/netbox/circuits/apps.py +++ b/netbox/circuits/apps.py @@ -1,7 +1,5 @@ from django.apps import AppConfig -from netbox import denormalized - class CircuitsConfig(AppConfig): name = "circuits" @@ -11,16 +9,6 @@ class CircuitsConfig(AppConfig): from netbox.models.features import register_models from . import search, signals # noqa: F401 - from .models import CircuitTermination # Register models register_models(*self.get_models()) - - denormalized.register(CircuitTermination, '_site', { - '_region': 'region', - '_site_group': 'group', - }) - - denormalized.register(CircuitTermination, '_location', { - '_site': 'site', - }) diff --git a/netbox/circuits/migrations/0058_denormalization_triggers.py b/netbox/circuits/migrations/0058_denormalization_triggers.py new file mode 100644 index 000000000..21bce5ec3 --- /dev/null +++ b/netbox/circuits/migrations/0058_denormalization_triggers.py @@ -0,0 +1,33 @@ +""" +Maintain CircuitTermination's denormalized site/region/site-group columns via PostgreSQL triggers instead +of the Python `post_save` handler formerly registered in netbox.denormalized. +""" +from django.db import migrations + +from utilities.migration import InstallDenormalizationTrigger + + +class Migration(migrations.Migration): + + dependencies = [ + ('circuits', '0057_default_ordering_indexes'), + # Source tables (dcim_site, dcim_location) must already exist. + ('dcim', '0238_ltree_paths'), + ] + + operations = [ + # When a Site's region/group changes, propagate to terminations assigned to it. + InstallDenormalizationTrigger( + dependent_table='circuits_circuittermination', + source_table='dcim_site', + fk_column='_site_id', + mappings={'_region_id': 'region_id', '_site_group_id': 'group_id'}, + ), + # When a Location's site changes, propagate to terminations assigned to it. + InstallDenormalizationTrigger( + dependent_table='circuits_circuittermination', + source_table='dcim_location', + fk_column='_location_id', + mappings={'_site_id': 'site_id'}, + ), + ] diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index a3c4a54ca..6030e9eb8 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -3,7 +3,7 @@ from django.core.exceptions import NON_FIELD_ERRORS, ValidationError from django.test import TestCase from circuits.models import Circuit, CircuitTermination, CircuitType, Provider, ProviderNetwork -from dcim.models import Site +from dcim.models import Location, Region, Site, SiteGroup class CircuitTerminationTestCase(TestCase): @@ -166,3 +166,78 @@ class CircuitTerminationTestCase(TestCase): self.assertIn(NON_FIELD_ERRORS, errors) self.assertIn('Please select a Provider Network.', errors[NON_FIELD_ERRORS]) self.assertNotIn('termination_id', errors) + + +class CircuitTerminationDenormalizationTriggerTestCase(TestCase): + """ + Verify the PostgreSQL triggers (installed by circuits migration 0058) that keep a + CircuitTermination's denormalized scope columns in sync with its Site/Location. + + These replace the former Python `post_save` handler in netbox.denormalized. Unlike that + handler, the triggers also fire for bulk QuerySet.update() writes (exercised below). + """ + + @classmethod + def setUpTestData(cls): + provider = Provider.objects.create(name='Provider 1', slug='provider-1') + circuit_type = CircuitType.objects.create(name='Circuit Type 1', slug='circuit-type-1') + cls.circuit = Circuit.objects.create(cid='Circuit 1', provider=provider, type=circuit_type) + + def test_site_region_group_change_propagates_to_termination(self): + region_a = Region.objects.create(name='Region A', slug='region-a') + region_b = Region.objects.create(name='Region B', slug='region-b') + group_a = SiteGroup.objects.create(name='Group A', slug='group-a') + group_b = SiteGroup.objects.create(name='Group B', slug='group-b') + site = Site.objects.create(name='Site', slug='site', region=region_a, group=group_a) + + termination = CircuitTermination.objects.create( + circuit=self.circuit, term_side='A', termination=site, + ) + self.assertEqual(termination._region, region_a) + self.assertEqual(termination._site_group, group_a) + + # Reassign the Site's region/group; the trigger should update the termination. + site.region = region_b + site.group = group_b + site.save() + + termination.refresh_from_db() + self.assertEqual(termination._region, region_b) + self.assertEqual(termination._site_group, group_b) + + def test_location_site_change_propagates_to_termination(self): + site_a = Site.objects.create(name='Site A', slug='site-a') + site_b = Site.objects.create(name='Site B', slug='site-b') + location = Location.objects.create(name='Loc', slug='loc', site=site_a) + + termination = CircuitTermination.objects.create( + circuit=self.circuit, term_side='A', termination=location, + ) + self.assertEqual(termination._site, site_a) + self.assertEqual(termination._location, location) + + # Move the Location to a different Site; the trigger should update _site. + location.site = site_b + location.save() + + termination.refresh_from_db() + self.assertEqual(termination._site, site_b) + + def test_bulk_update_of_site_propagates_to_termination(self): + """ + A QuerySet.update() bypasses post_save (the old handler never fired for it); the + DB trigger fires regardless, which is the behavior this change introduces. + """ + region_a = Region.objects.create(name='Region A', slug='region-a') + region_b = Region.objects.create(name='Region B', slug='region-b') + site = Site.objects.create(name='Site', slug='site', region=region_a) + + termination = CircuitTermination.objects.create( + circuit=self.circuit, term_side='A', termination=site, + ) + self.assertEqual(termination._region, region_a) + + Site.objects.filter(pk=site.pk).update(region=region_b) + + termination.refresh_from_db() + self.assertEqual(termination._region, region_b) diff --git a/netbox/dcim/apps.py b/netbox/dcim/apps.py index 0aad86fc8..803ad95cc 100644 --- a/netbox/dcim/apps.py +++ b/netbox/dcim/apps.py @@ -1,7 +1,5 @@ from django.apps import AppConfig -from netbox import denormalized - class DCIMConfig(AppConfig): name = "dcim" @@ -12,24 +10,10 @@ class DCIMConfig(AppConfig): from utilities.counters import connect_counters from . import search, signals # noqa: F401 - from .models import CableTermination, Device, DeviceType, ModuleType, RackType, VirtualChassis + from .models import Device, DeviceType, ModuleType, RackType, VirtualChassis # Register models register_models(*self.get_models()) - # Register denormalized fields - denormalized.register(CableTermination, '_device', { - '_rack': 'rack', - '_location': 'location', - '_site': 'site', - }) - denormalized.register(CableTermination, '_rack', { - '_location': 'location', - '_site': 'site', - }) - denormalized.register(CableTermination, '_location', { - '_site': 'site', - }) - # Register counters connect_counters(Device, DeviceType, ModuleType, RackType, VirtualChassis) diff --git a/netbox/dcim/migrations/0239_denormalization_triggers.py b/netbox/dcim/migrations/0239_denormalization_triggers.py new file mode 100644 index 000000000..6d548ad6d --- /dev/null +++ b/netbox/dcim/migrations/0239_denormalization_triggers.py @@ -0,0 +1,38 @@ +""" +Maintain CableTermination's denormalized device/rack/location/site columns via PostgreSQL triggers instead +of the Python `post_save` handler formerly registered in netbox.denormalized. +""" +from django.db import migrations + +from utilities.migration import InstallDenormalizationTrigger + + +class Migration(migrations.Migration): + + dependencies = [ + ('dcim', '0238_ltree_paths'), + ] + + operations = [ + # When a Device's rack/location/site changes, propagate to its cable terminations. + InstallDenormalizationTrigger( + dependent_table='dcim_cabletermination', + source_table='dcim_device', + fk_column='_device_id', + mappings={'_rack_id': 'rack_id', '_location_id': 'location_id', '_site_id': 'site_id'}, + ), + # When a Rack's location/site changes, propagate to cable terminations assigned to it. + InstallDenormalizationTrigger( + dependent_table='dcim_cabletermination', + source_table='dcim_rack', + fk_column='_rack_id', + mappings={'_location_id': 'location_id', '_site_id': 'site_id'}, + ), + # When a Location's site changes, propagate to cable terminations assigned to it. + InstallDenormalizationTrigger( + dependent_table='dcim_cabletermination', + source_table='dcim_location', + fk_column='_location_id', + mappings={'_site_id': 'site_id'}, + ), + ] diff --git a/netbox/dcim/tests/test_signals.py b/netbox/dcim/tests/test_signals.py index 9edcfbd46..c2abfc221 100644 --- a/netbox/dcim/tests/test_signals.py +++ b/netbox/dcim/tests/test_signals.py @@ -9,6 +9,7 @@ from dcim.choices import CableEndChoices, LinkStatusChoices from dcim.models import ( Cable, CablePath, + CableTermination, Device, DeviceRole, DeviceType, @@ -486,3 +487,63 @@ class CableSignalDirectHandlerTestCase(SimpleTestCase): signals.update_mac_address_interface(instance=interface, created=True, raw=True) primary_mac.save.assert_not_called() + + +class CableTerminationDenormalizationTriggerTestCase(TestCase): + """ + Verify the PostgreSQL triggers (installed by dcim migration 0239) that keep a + CableTermination's denormalized _device/_rack/_location/_site columns in sync with the + parent Device/Rack/Location. + + These replace the former Python `post_save` handler in netbox.denormalized. Crucially, + the triggers also fire for bulk QuerySet.update() writes — which the handler (a post_save + receiver) never saw — so this exercises that path explicitly. + """ + + @classmethod + def setUpTestData(cls): + cls.site_a = Site.objects.create(name='Site A', slug='site-a') + cls.site_b = Site.objects.create(name='Site B', slug='site-b') + cls.location_b = Location.objects.create(name='Loc B', slug='loc-b', site=cls.site_b) + cls.rack_b = Rack.objects.create(name='Rack B', site=cls.site_b, location=cls.location_b) + manufacturer = Manufacturer.objects.create(name='Manufacturer', slug='manufacturer') + cls.device_type = DeviceType.objects.create(manufacturer=manufacturer, model='Device Type') + cls.device_role = DeviceRole.objects.create(name='Device Role', slug='device-role') + + def _connected_termination(self): + device = Device.objects.create( + name='Device', site=self.site_a, device_type=self.device_type, role=self.device_role, + ) + interface_a = Interface.objects.create(device=device, name='Interface A') + interface_b = Interface.objects.create(device=device, name='Interface B') + cable = Cable(a_terminations=[interface_a], b_terminations=[interface_b]) + cable.save() + termination = CableTermination.objects.filter(_device=device).first() + self.assertIsNotNone(termination) + self.assertEqual(termination._site, self.site_a) + return device, termination + + def test_device_move_propagates_to_cable_termination(self): + device, termination = self._connected_termination() + + device.site = self.site_b + device.location = self.location_b + device.rack = self.rack_b + device.save() + + termination.refresh_from_db() + self.assertEqual(termination._site, self.site_b) + self.assertEqual(termination._location, self.location_b) + self.assertEqual(termination._rack, self.rack_b) + + def test_bulk_update_of_device_propagates_to_cable_termination(self): + """ + A bulk QuerySet.update() bypasses post_save (the old handler never fired for it); + the DB trigger fires regardless. + """ + device, termination = self._connected_termination() + + Device.objects.filter(pk=device.pk).update(site=self.site_b) + + termination.refresh_from_db() + self.assertEqual(termination._site, self.site_b) diff --git a/netbox/ipam/apps.py b/netbox/ipam/apps.py index f9310af8d..bfd79dad2 100644 --- a/netbox/ipam/apps.py +++ b/netbox/ipam/apps.py @@ -1,7 +1,5 @@ from django.apps import AppConfig -from netbox import denormalized - class IPAMConfig(AppConfig): name = "ipam" @@ -11,16 +9,6 @@ class IPAMConfig(AppConfig): from netbox.models.features import register_models from . import search, signals # noqa: F401 - from .models import Prefix # Register models register_models(*self.get_models()) - - # Register denormalized fields - denormalized.register(Prefix, '_site', { - '_region': 'region', - '_site_group': 'group', - }) - denormalized.register(Prefix, '_location', { - '_site': 'site', - }) diff --git a/netbox/ipam/migrations/0091_denormalization_triggers.py b/netbox/ipam/migrations/0091_denormalization_triggers.py new file mode 100644 index 000000000..c72efaf2c --- /dev/null +++ b/netbox/ipam/migrations/0091_denormalization_triggers.py @@ -0,0 +1,33 @@ +""" +Maintain Prefix's denormalized site/region/site-group columns via PostgreSQL triggers instead of the +Python `post_save` handler formerly registered in netbox.denormalized. +""" +from django.db import migrations + +from utilities.migration import InstallDenormalizationTrigger + + +class Migration(migrations.Migration): + + dependencies = [ + ('ipam', '0090_vlangroup_recompute_total_vlan_ids'), + # Source tables (dcim_site, dcim_location) must already exist. + ('dcim', '0238_ltree_paths'), + ] + + operations = [ + # When a Site's region/group changes, propagate to prefixes assigned to it. + InstallDenormalizationTrigger( + dependent_table='ipam_prefix', + source_table='dcim_site', + fk_column='_site_id', + mappings={'_region_id': 'region_id', '_site_group_id': 'group_id'}, + ), + # When a Location's site changes, propagate to prefixes assigned to it. + InstallDenormalizationTrigger( + dependent_table='ipam_prefix', + source_table='dcim_location', + fk_column='_location_id', + mappings={'_site_id': 'site_id'}, + ), + ] diff --git a/netbox/ipam/tests/test_signals.py b/netbox/ipam/tests/test_signals.py index 0db43c624..747e8db56 100644 --- a/netbox/ipam/tests/test_signals.py +++ b/netbox/ipam/tests/test_signals.py @@ -5,6 +5,7 @@ from django.test import RequestFactory, TestCase from core.choices import ObjectChangeActionChoices from core.models import ObjectChange +from dcim.models import Location, Region, Site, SiteGroup from ipam.models import IPAddress, Prefix from netbox.context_managers import event_tracking from users.models import User @@ -229,3 +230,72 @@ class ClearOOBIPSignalTestCase(TestCase): action=ObjectChangeActionChoices.ACTION_UPDATE, ).exists() ) + + +class PrefixDenormalizationTriggerTestCase(TestCase): + """ + Verify the PostgreSQL triggers (installed by ipam migration 0091) that keep a Prefix's + denormalized scope columns in sync with its Site/Location. + + These replace the former Python `post_save` handler in netbox.denormalized. Unlike that + handler, the triggers also fire for bulk QuerySet.update() writes (exercised below). + """ + + def test_site_region_group_change_propagates_to_prefix(self): + region_a = Region.objects.create(name='Region A', slug='region-a') + region_b = Region.objects.create(name='Region B', slug='region-b') + group_a = SiteGroup.objects.create(name='Group A', slug='group-a') + group_b = SiteGroup.objects.create(name='Group B', slug='group-b') + site = Site.objects.create(name='Site', slug='site', region=region_a, group=group_a) + prefix = Prefix.objects.create( + prefix='10.0.0.0/24', + scope_type=ContentType.objects.get_for_model(Site), + scope_id=site.pk, + ) + self.assertEqual(prefix._region, region_a) + self.assertEqual(prefix._site_group, group_a) + + site.region = region_b + site.group = group_b + site.save() + + prefix.refresh_from_db() + self.assertEqual(prefix._region, region_b) + self.assertEqual(prefix._site_group, group_b) + + def test_location_site_change_propagates_to_prefix(self): + site_a = Site.objects.create(name='Site A', slug='site-a') + site_b = Site.objects.create(name='Site B', slug='site-b') + location = Location.objects.create(name='Loc', slug='loc', site=site_a) + prefix = Prefix.objects.create( + prefix='10.0.0.0/24', + scope_type=ContentType.objects.get_for_model(Location), + scope_id=location.pk, + ) + self.assertEqual(prefix._site, site_a) + + location.site = site_b + location.save() + + prefix.refresh_from_db() + self.assertEqual(prefix._site, site_b) + + def test_bulk_update_of_site_propagates_to_prefix(self): + """ + A bulk QuerySet.update() bypasses post_save (the old handler never fired for it); + the DB trigger fires regardless. + """ + region_a = Region.objects.create(name='Region A', slug='region-a') + region_b = Region.objects.create(name='Region B', slug='region-b') + site = Site.objects.create(name='Site', slug='site', region=region_a) + prefix = Prefix.objects.create( + prefix='10.0.0.0/24', + scope_type=ContentType.objects.get_for_model(Site), + scope_id=site.pk, + ) + self.assertEqual(prefix._region, region_a) + + Site.objects.filter(pk=site.pk).update(region=region_b) + + prefix.refresh_from_db() + self.assertEqual(prefix._region, region_b) diff --git a/netbox/netbox/denormalized.py b/netbox/netbox/denormalized.py deleted file mode 100644 index 3c2492a9c..000000000 --- a/netbox/netbox/denormalized.py +++ /dev/null @@ -1,57 +0,0 @@ -import logging - -from django.db.models.signals import post_save -from django.dispatch import receiver - -from netbox.registry import registry - -logger = logging.getLogger('netbox.denormalized') - - -def register(model, field_name, mappings): - """ - Register a denormalized model field to ensure that it is kept up-to-date with the related object. - - Args: - model: The class being updated - field_name: The name of the field related to the triggering instance - mappings: Dictionary mapping of local to remote fields - """ - logger.debug(f'Registering denormalized field {model}.{field_name}') - - field = model._meta.get_field(field_name) - rel_model = field.related_model - - registry['denormalized_fields'][rel_model].append( - (model, field_name, mappings) - ) - - -@receiver(post_save) -def update_denormalized_fields(sender, instance, created, raw, **kwargs): - """ - Check if the sender has denormalized fields registered, and update them as necessary. - """ - def _get_field_value(instance, field_name): - field = instance._meta.get_field(field_name) - return field.value_from_object(instance) - - # Skip for new objects or those being populated from raw data - if created or raw: - return - - # Look up any denormalized fields referencing this model from the application registry - for model, field_name, mappings in registry['denormalized_fields'].get(sender, []): - logger.debug(f'Updating denormalized values for {model}.{field_name}') - filter_params = { - field_name: instance.pk, - } - update_params = { - # Map the denormalized field names to the instance's values - denorm: _get_field_value(instance, origin) for denorm, origin in mappings.items() - } - - # TODO: Improve efficiency here by placing conditions on the query? - # Update all the denormalized fields with the triggering object's new values - count = model.objects.filter(**filter_params).update(**update_params) - logger.debug(f'Updated {count} rows') diff --git a/netbox/netbox/registry.py b/netbox/netbox/registry.py index f8186b4e0..35ce3c976 100644 --- a/netbox/netbox/registry.py +++ b/netbox/netbox/registry.py @@ -25,7 +25,6 @@ class Registry(dict): registry = Registry({ 'counter_fields': collections.defaultdict(dict), 'data_backends': dict(), - 'denormalized_fields': collections.defaultdict(list), 'event_types': dict(), 'filtersets': dict(), 'model_actions': collections.defaultdict(set), diff --git a/netbox/utilities/migration.py b/netbox/utilities/migration.py index 97036fc06..ffaf42c1c 100644 --- a/netbox/utilities/migration.py +++ b/netbox/utilities/migration.py @@ -1,8 +1,9 @@ -from django.db import models +from django.db import migrations, models from netbox.config import ConfigItem __all__ = ( + 'InstallDenormalizationTrigger', 'custom_deconstruct', ) @@ -32,3 +33,75 @@ def custom_deconstruct(field): } return name, path, args, kwargs + + +class InstallDenormalizationTrigger(migrations.operations.base.Operation): + """ + Install a PostgreSQL trigger that keeps denormalized columns on a dependent table in sync with their + source object. + + When a row in `source_table` is updated, the trigger copies the values of the mapped source columns into + the corresponding denormalized columns on every `dependent_table` row that references it via `fk_column`. + This replaces the Python `post_save` handler formerly defined in `netbox.denormalized`. + + Args: + dependent_table: The table carrying the denormalized columns (e.g. 'ipam_prefix'). + source_table: The table whose changes are propagated (e.g. 'dcim_site'). + fk_column: The column on `dependent_table` referencing `source_table` (e.g. '_site_id'). + mappings: A mapping of {dependent_column: source_column}, using actual database column names + (e.g. {'_region_id': 'region_id', '_site_group_id': 'group_id'}). + + The trigger fires AFTER UPDATE of the source columns, and only when at least one of them actually changed. + Like the handler it replaces, it does not fire on INSERT (a newly created source row has no dependents + yet) and it does not cascade: updating the denormalized columns does not itself trigger further + denormalization. + """ + reversible = True + + def __init__(self, dependent_table, source_table, fk_column, mappings): + self.dependent_table = dependent_table + self.source_table = source_table + self.fk_column = fk_column + self.mappings = mappings + + @property + def function_name(self): + return f'{self.dependent_table}_denorm_from_{self.source_table}_fn' + + @property + def trigger_name(self): + return f'{self.dependent_table}_denorm_from_{self.source_table}' + + def state_forwards(self, app_label, state): + # Triggers are not part of Django's model state. + pass + + def database_forwards(self, app_label, schema_editor, from_state, to_state): + source_columns = list(self.mappings.values()) + set_clause = ', '.join(f'"{dest}" = NEW."{src}"' for dest, src in self.mappings.items()) + update_of = ', '.join(f'"{col}"' for col in source_columns) + when_clause = ' OR '.join(f'OLD."{col}" IS DISTINCT FROM NEW."{col}"' for col in source_columns) + + schema_editor.execute(f''' + CREATE OR REPLACE FUNCTION "{self.function_name}"() RETURNS TRIGGER AS $$ + BEGIN + UPDATE "{self.dependent_table}" + SET {set_clause} + WHERE "{self.fk_column}" = NEW.id; + RETURN NULL; + END + $$ LANGUAGE plpgsql; + ''') + schema_editor.execute(f''' + CREATE TRIGGER "{self.trigger_name}" + AFTER UPDATE OF {update_of} ON "{self.source_table}" + FOR EACH ROW WHEN ({when_clause}) + EXECUTE FUNCTION "{self.function_name}"(); + ''') + + def database_backwards(self, app_label, schema_editor, from_state, to_state): + schema_editor.execute(f'DROP TRIGGER IF EXISTS "{self.trigger_name}" ON "{self.source_table}";') + schema_editor.execute(f'DROP FUNCTION IF EXISTS "{self.function_name}"();') + + def describe(self): + return f'Install denormalization trigger on {self.source_table} updating {self.dependent_table}' From 041e749996422ac63a1f3c12f8a8506066b98341 Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 15 Jun 2026 10:38:11 -0700 Subject: [PATCH 2/4] cleanup --- .../0058_denormalization_triggers.py | 19 +--- netbox/circuits/tests/test_models.py | 13 ++- .../0239_denormalization_triggers.py | 36 +++++- netbox/dcim/signals.py | 103 ++--------------- netbox/dcim/tests/test_signals.py | 90 +++++++++++++-- .../0091_denormalization_triggers.py | 21 +--- netbox/ipam/tests/test_signals.py | 12 +- netbox/utilities/migration.py | 106 +++++++++++++++--- .../0057_denormalization_triggers.py | 18 +++ .../0021_denormalization_triggers.py | 19 ++++ 10 files changed, 272 insertions(+), 165 deletions(-) create mode 100644 netbox/virtualization/migrations/0057_denormalization_triggers.py create mode 100644 netbox/wireless/migrations/0021_denormalization_triggers.py diff --git a/netbox/circuits/migrations/0058_denormalization_triggers.py b/netbox/circuits/migrations/0058_denormalization_triggers.py index 21bce5ec3..db0b9ffb3 100644 --- a/netbox/circuits/migrations/0058_denormalization_triggers.py +++ b/netbox/circuits/migrations/0058_denormalization_triggers.py @@ -4,7 +4,7 @@ of the Python `post_save` handler formerly registered in netbox.denormalized. """ from django.db import migrations -from utilities.migration import InstallDenormalizationTrigger +from utilities.migration import cached_scope_triggers class Migration(migrations.Migration): @@ -15,19 +15,4 @@ class Migration(migrations.Migration): ('dcim', '0238_ltree_paths'), ] - operations = [ - # When a Site's region/group changes, propagate to terminations assigned to it. - InstallDenormalizationTrigger( - dependent_table='circuits_circuittermination', - source_table='dcim_site', - fk_column='_site_id', - mappings={'_region_id': 'region_id', '_site_group_id': 'group_id'}, - ), - # When a Location's site changes, propagate to terminations assigned to it. - InstallDenormalizationTrigger( - dependent_table='circuits_circuittermination', - source_table='dcim_location', - fk_column='_location_id', - mappings={'_site_id': 'site_id'}, - ), - ] + operations = cached_scope_triggers('circuits_circuittermination') diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index 6030e9eb8..0b139e764 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -206,8 +206,12 @@ class CircuitTerminationDenormalizationTriggerTestCase(TestCase): self.assertEqual(termination._site_group, group_b) def test_location_site_change_propagates_to_termination(self): - site_a = Site.objects.create(name='Site A', slug='site-a') - site_b = Site.objects.create(name='Site B', slug='site-b') + region_a = Region.objects.create(name='Region A', slug='region-a') + region_b = Region.objects.create(name='Region B', slug='region-b') + group_a = SiteGroup.objects.create(name='Group A', slug='group-a') + group_b = SiteGroup.objects.create(name='Group B', slug='group-b') + site_a = Site.objects.create(name='Site A', slug='site-a', region=region_a, group=group_a) + site_b = Site.objects.create(name='Site B', slug='site-b', region=region_b, group=group_b) location = Location.objects.create(name='Loc', slug='loc', site=site_a) termination = CircuitTermination.objects.create( @@ -216,12 +220,15 @@ class CircuitTerminationDenormalizationTriggerTestCase(TestCase): self.assertEqual(termination._site, site_a) self.assertEqual(termination._location, location) - # Move the Location to a different Site; the trigger should update _site. + # Move the Location to a different Site; the trigger updates _site and pulls the new + # site's region/group through in the same statement. location.site = site_b location.save() termination.refresh_from_db() self.assertEqual(termination._site, site_b) + self.assertEqual(termination._region, region_b) + self.assertEqual(termination._site_group, group_b) def test_bulk_update_of_site_propagates_to_termination(self): """ diff --git a/netbox/dcim/migrations/0239_denormalization_triggers.py b/netbox/dcim/migrations/0239_denormalization_triggers.py index 6d548ad6d..1d4275721 100644 --- a/netbox/dcim/migrations/0239_denormalization_triggers.py +++ b/netbox/dcim/migrations/0239_denormalization_triggers.py @@ -1,11 +1,31 @@ """ -Maintain CableTermination's denormalized device/rack/location/site columns via PostgreSQL triggers instead -of the Python `post_save` handler formerly registered in netbox.denormalized. +Maintain denormalized device/rack/location/site columns via PostgreSQL triggers instead of Python +`post_save` handlers: + +- CableTermination's _device/_rack/_location/_site (formerly netbox.denormalized). +- Each device component's _site/_location/_rack (formerly dcim.signals.handle_device_site_change / + handle_rack_site_change / handle_location_site_change). These are derived solely from the parent + Device, so a single Device-sourced trigger per component table covers direct device edits as well as + the Rack/Location cascades (which write Device.site/location and thus fire this trigger). """ from django.db import migrations from utilities.migration import InstallDenormalizationTrigger +# Device component tables carrying _site/_location/_rack denormalized from their parent Device. +COMPONENT_TABLES = ( + 'dcim_consoleport', + 'dcim_consoleserverport', + 'dcim_devicebay', + 'dcim_frontport', + 'dcim_interface', + 'dcim_inventoryitem', + 'dcim_modulebay', + 'dcim_poweroutlet', + 'dcim_powerport', + 'dcim_rearport', +) + class Migration(migrations.Migration): @@ -35,4 +55,16 @@ class Migration(migrations.Migration): fk_column='_location_id', mappings={'_site_id': 'site_id'}, ), + # Device components: mirror the parent Device's site/location/rack onto each component. + # The Rack/Location → Device cascades (dcim.signals) write Device.site/location, which fires + # this same trigger, so no separate Rack/Location-sourced component triggers are needed. + *[ + InstallDenormalizationTrigger( + dependent_table=table, + source_table='dcim_device', + fk_column='device_id', + mappings={'_site_id': 'site_id', '_location_id': 'location_id', '_rack_id': 'rack_id'}, + ) + for table in COMPONENT_TABLES + ], ] diff --git a/netbox/dcim/signals.py b/netbox/dcim/signals.py index 8d5baf6d1..6c1f7f4ff 100644 --- a/netbox/dcim/signals.py +++ b/netbox/dcim/signals.py @@ -5,59 +5,36 @@ from django.db.models.signals import post_delete, post_save from django.dispatch import receiver from dcim.choices import CableEndChoices, LinkStatusChoices -from ipam.models import Prefix -from virtualization.models import Cluster, VMInterface -from wireless.models import WirelessLAN +from virtualization.models import VMInterface from .models import ( Cable, CablePath, CableTermination, - ConsolePort, - ConsoleServerPort, Device, - DeviceBay, - FrontPort, Interface, - InventoryItem, Location, - ModuleBay, PathEndpoint, PortMapping, - PowerOutlet, PowerPanel, - PowerPort, Rack, - RearPort, - Site, VirtualChassis, ) from .models.cables import trace_paths from .utils import create_cablepaths, rebuild_paths -COMPONENT_MODELS = ( - ConsolePort, - ConsoleServerPort, - DeviceBay, - FrontPort, - Interface, - InventoryItem, - ModuleBay, - PowerOutlet, - PowerPort, - RearPort, -) - - # # Location/rack/device assignment # + @receiver(post_save, sender=Location) def handle_location_site_change(instance, created, **kwargs): """ - Update child objects if Site assignment has changed. We intentionally recurse through each child - object instead of calling update() on the QuerySet to ensure the proper change records get created for each. + Cascade a Location's Site assignment down to the Racks, Devices, and PowerPanels it contains + (and to descendant Locations). The denormalized cache columns on cable terminations and device + components are maintained by PostgreSQL triggers, which fire on these Site/Location/Rack/Device + column writes. """ if not created: instance.get_descendants().update(site=instance.site) @@ -65,39 +42,16 @@ def handle_location_site_change(instance, created, **kwargs): Rack.objects.filter(location__in=locations).update(site=instance.site) Device.objects.filter(location__in=locations).update(site=instance.site) PowerPanel.objects.filter(location__in=locations).update(site=instance.site) - CableTermination.objects.filter(_location__in=locations).update(_site=instance.site) - # Update component models for devices in these locations - for model in COMPONENT_MODELS: - model.objects.filter(device__location__in=locations).update(_site=instance.site) @receiver(post_save, sender=Rack) def handle_rack_site_change(instance, created, **kwargs): """ - Update child Devices if Site or Location assignment has changed. + Cascade a Rack's Site/Location assignment down to the Devices it contains. The denormalized cache + columns on those devices' components are maintained by PostgreSQL triggers. """ if not created: Device.objects.filter(rack=instance).update(site=instance.site, location=instance.location) - # Update component models for devices in this rack - for model in COMPONENT_MODELS: - model.objects.filter(device__rack=instance).update( - _site=instance.site, - _location=instance.location, - ) - - -@receiver(post_save, sender=Device) -def handle_device_site_change(instance, created, **kwargs): - """ - Update child components to update the parent Site, Location, and Rack when a Device is saved. - """ - if not created: - for model in COMPONENT_MODELS: - model.objects.filter(device=instance).update( - _site=instance.site, - _location=instance.location, - _rack=instance.rack, - ) # @@ -210,44 +164,3 @@ def update_mac_address_interface(instance, created, raw, **kwargs): if created and not raw and instance.primary_mac_address: instance.primary_mac_address.assigned_object = instance instance.primary_mac_address.save() - - -@receiver(post_save, sender=Location) -@receiver(post_save, sender=Site) -def sync_cached_scope_fields(instance, created, **kwargs): - """ - Rebuild cached scope fields for all CachedScopeMixin-based models - affected by a change in a Region, SiteGroup, Site, or Location. - - This method is safe to run for objects created in the past and does - not rely on incremental updates. Cached fields are recomputed from - authoritative relationships. - """ - if created: - return - - if isinstance(instance, Location): - filters = {'_location': instance} - elif isinstance(instance, Site): - filters = {'_site': instance} - else: - return - - # These models are explicitly listed because they all subclass CachedScopeMixin - # and therefore require their cached scope fields to be recomputed. - for model in (Prefix, Cluster, WirelessLAN): - qs = model.objects.filter(**filters) - - # Bulk update cached fields to avoid O(N) performance issues with large datasets. - # This does not trigger post_save signals, avoiding spurious change log entries. - objects_to_update = [] - for obj in qs: - # Recompute cache using the same logic as save() - obj.cache_related_objects() - objects_to_update.append(obj) - - if objects_to_update: - model.objects.bulk_update( - objects_to_update, - ['_location', '_site', '_site_group', '_region'] - ) diff --git a/netbox/dcim/tests/test_signals.py b/netbox/dcim/tests/test_signals.py index c2abfc221..1b83f892d 100644 --- a/netbox/dcim/tests/test_signals.py +++ b/netbox/dcim/tests/test_signals.py @@ -2,6 +2,7 @@ from types import SimpleNamespace from unittest.mock import MagicMock, patch from django.contrib.contenttypes.models import ContentType +from django.db import connection from django.test import SimpleTestCase, TestCase from dcim import signals @@ -26,6 +27,8 @@ from dcim.models import ( SiteGroup, VirtualChassis, ) +from dcim.models.device_components import ComponentModel +from dcim.models.mixins import CachedScopeMixin from ipam.models import Prefix from virtualization.models import Cluster, ClusterType from wireless.models import WirelessLAN @@ -117,10 +120,11 @@ class RackSiteChangeSignalTestCase(TestCase): self.assertEqual(interface._location, self.location_b) -class DeviceSiteChangeSignalTestCase(TestCase): +class DeviceComponentScopeTriggerTestCase(TestCase): """ - Verify dcim.signals.handle_device_site_change propagates a Device's site/location/rack - to its components on save. + Verify the PostgreSQL trigger (dcim migration 0239) that propagates a Device's site/location/rack + onto its components' denormalized _site/_location/_rack columns. This replaces the former + dcim.signals.handle_device_site_change handler. """ @classmethod @@ -147,6 +151,25 @@ class DeviceSiteChangeSignalTestCase(TestCase): interface.refresh_from_db() self.assertEqual(interface._site, self.site_b) + def test_bulk_update_of_device_updates_components_cached_scope(self): + """ + A bulk QuerySet.update() bypasses post_save (the old handler never fired for it); the DB + trigger fires regardless. This is also the path the Rack/Location cascades take. + """ + device = Device.objects.create( + name='Device', + site=self.site_a, + device_type=self.device_type, + role=self.device_role, + ) + interface = Interface.objects.create(device=device, name='Interface 1') + self.assertEqual(interface._site, self.site_a) + + Device.objects.filter(pk=device.pk).update(site=self.site_b) + + interface.refresh_from_db() + self.assertEqual(interface._site, self.site_b) + class VirtualChassisMasterSignalTestCase(TestCase): """ @@ -386,10 +409,12 @@ class MACAddressInterfaceSignalTestCase(TestCase): self.assertIsNone(mac.assigned_object) -class SyncCachedScopeFieldsSignalTestCase(TestCase): +class CachedScopeFieldTriggerTestCase(TestCase): """ - Verify dcim.signals.sync_cached_scope_fields recomputes cached scope fields on - Prefix, Cluster, and WirelessLAN when a Site or Location is modified. + Verify the PostgreSQL triggers (ipam/virtualization/wireless denormalization migrations) that keep + the CachedScopeMixin scope columns (_site/_location/_region/_site_group) on Prefix, Cluster, and + WirelessLAN in sync when a scoped Site or Location is modified. These replace the former + dcim.signals.sync_cached_scope_fields handler. """ def test_site_group_change_updates_prefix_cached_scope(self): @@ -429,11 +454,9 @@ class SyncCachedScopeFieldsSignalTestCase(TestCase): self.assertEqual(prefix._location, location) self.assertEqual(prefix._site, site_b) - def test_signal_updates_cluster_and_wirelesslan_cached_scope(self): - # Lock down the explicit (Prefix, Cluster, WirelessLAN) tuple in the - # signal by exercising Cluster and WirelessLAN alongside Prefix. If a - # future change drops Cluster or WirelessLAN from that tuple, this test - # will catch it. + def test_triggers_update_cluster_and_wirelesslan_cached_scope(self): + # Cluster and WirelessLAN each carry their own Site/Location triggers (installed by the + # virtualization and wireless denormalization migrations); exercise both alongside Prefix. group_a = SiteGroup.objects.create(name='Group A', slug='group-a') group_b = SiteGroup.objects.create(name='Group B', slug='group-b') site = Site.objects.create(name='Site', slug='site', group=group_a) @@ -547,3 +570,48 @@ class CableTerminationDenormalizationTriggerTestCase(TestCase): termination.refresh_from_db() self.assertEqual(termination._site, self.site_b) + + +def _concrete_subclasses(base): + """Yield every non-abstract model descending from an abstract base model.""" + for subclass in base.__subclasses__(): + if subclass._meta.abstract: + yield from _concrete_subclasses(subclass) + else: + yield subclass + + +def _installed_triggers(): + with connection.cursor() as cursor: + cursor.execute('SELECT tgname FROM pg_trigger WHERE NOT tgisinternal') + return {row[0] for row in cursor.fetchall()} + + +class DenormalizationTriggerCoverageTestCase(TestCase): + """ + Guard against a new model silently shipping without its denormalization triggers. The set of + device-component tables and CachedScopeMixin dependents is hand-listed in migrations; this test + derives those sets from the model layer and asserts the expected triggers are installed, so adding + a new component / scoped model without a matching trigger migration fails CI. + """ + + def test_device_components_have_device_trigger(self): + triggers = _installed_triggers() + for model in _concrete_subclasses(ComponentModel): + table = model._meta.db_table + self.assertIn( + f'{table}_denorm_from_dcim_device', triggers, + msg=f'{model.__name__} has no dcim_device denormalization trigger (add it to ' + f'dcim migration 0239 COMPONENT_TABLES)', + ) + + def test_cached_scope_models_have_site_and_location_triggers(self): + triggers = _installed_triggers() + for model in _concrete_subclasses(CachedScopeMixin): + table = model._meta.db_table + for source in ('dcim_site', 'dcim_location'): + self.assertIn( + f'{table}_denorm_from_{source}', triggers, + msg=f'{model.__name__} (CachedScopeMixin) has no {source} denormalization trigger; ' + f'add cached_scope_triggers({table!r}) in a migration for its app', + ) diff --git a/netbox/ipam/migrations/0091_denormalization_triggers.py b/netbox/ipam/migrations/0091_denormalization_triggers.py index c72efaf2c..e92b2a41c 100644 --- a/netbox/ipam/migrations/0091_denormalization_triggers.py +++ b/netbox/ipam/migrations/0091_denormalization_triggers.py @@ -1,10 +1,10 @@ """ Maintain Prefix's denormalized site/region/site-group columns via PostgreSQL triggers instead of the -Python `post_save` handler formerly registered in netbox.denormalized. +Python `post_save` handler formerly registered in netbox.denormalized (and dcim.signals.sync_cached_scope_fields). """ from django.db import migrations -from utilities.migration import InstallDenormalizationTrigger +from utilities.migration import cached_scope_triggers class Migration(migrations.Migration): @@ -15,19 +15,4 @@ class Migration(migrations.Migration): ('dcim', '0238_ltree_paths'), ] - operations = [ - # When a Site's region/group changes, propagate to prefixes assigned to it. - InstallDenormalizationTrigger( - dependent_table='ipam_prefix', - source_table='dcim_site', - fk_column='_site_id', - mappings={'_region_id': 'region_id', '_site_group_id': 'group_id'}, - ), - # When a Location's site changes, propagate to prefixes assigned to it. - InstallDenormalizationTrigger( - dependent_table='ipam_prefix', - source_table='dcim_location', - fk_column='_location_id', - mappings={'_site_id': 'site_id'}, - ), - ] + operations = cached_scope_triggers('ipam_prefix') diff --git a/netbox/ipam/tests/test_signals.py b/netbox/ipam/tests/test_signals.py index 747e8db56..885144fa7 100644 --- a/netbox/ipam/tests/test_signals.py +++ b/netbox/ipam/tests/test_signals.py @@ -264,8 +264,12 @@ class PrefixDenormalizationTriggerTestCase(TestCase): self.assertEqual(prefix._site_group, group_b) def test_location_site_change_propagates_to_prefix(self): - site_a = Site.objects.create(name='Site A', slug='site-a') - site_b = Site.objects.create(name='Site B', slug='site-b') + region_a = Region.objects.create(name='Region A', slug='region-a') + region_b = Region.objects.create(name='Region B', slug='region-b') + group_a = SiteGroup.objects.create(name='Group A', slug='group-a') + group_b = SiteGroup.objects.create(name='Group B', slug='group-b') + site_a = Site.objects.create(name='Site A', slug='site-a', region=region_a, group=group_a) + site_b = Site.objects.create(name='Site B', slug='site-b', region=region_b, group=group_b) location = Location.objects.create(name='Loc', slug='loc', site=site_a) prefix = Prefix.objects.create( prefix='10.0.0.0/24', @@ -274,11 +278,15 @@ class PrefixDenormalizationTriggerTestCase(TestCase): ) self.assertEqual(prefix._site, site_a) + # Move the Location to a different Site; the trigger updates _site and pulls the new + # site's region/group through in the same statement. location.site = site_b location.save() prefix.refresh_from_db() self.assertEqual(prefix._site, site_b) + self.assertEqual(prefix._region, region_b) + self.assertEqual(prefix._site_group, group_b) def test_bulk_update_of_site_propagates_to_prefix(self): """ diff --git a/netbox/utilities/migration.py b/netbox/utilities/migration.py index ffaf42c1c..ca0d793cd 100644 --- a/netbox/utilities/migration.py +++ b/netbox/utilities/migration.py @@ -4,6 +4,7 @@ from netbox.config import ConfigItem __all__ = ( 'InstallDenormalizationTrigger', + 'cached_scope_triggers', 'custom_deconstruct', ) @@ -40,29 +41,43 @@ class InstallDenormalizationTrigger(migrations.operations.base.Operation): Install a PostgreSQL trigger that keeps denormalized columns on a dependent table in sync with their source object. - When a row in `source_table` is updated, the trigger copies the values of the mapped source columns into - the corresponding denormalized columns on every `dependent_table` row that references it via `fk_column`. - This replaces the Python `post_save` handler formerly defined in `netbox.denormalized`. + When rows in `source_table` are updated, the trigger copies the values of the mapped source columns into + the corresponding denormalized columns on every `dependent_table` row that references them via + `fk_column`. This replaces the Python `post_save` handlers formerly defined in `netbox.denormalized` and + `dcim.signals`. + + The trigger is statement-level (`FOR EACH STATEMENT`) and uses transition tables: a single bulk source + update (`UPDATE ... WHERE ...`, `QuerySet.update()`, `bulk_update()`) fires the trigger once and is + propagated with a single set-based UPDATE joining the changed rows, rather than once per affected row. Args: dependent_table: The table carrying the denormalized columns (e.g. 'ipam_prefix'). source_table: The table whose changes are propagated (e.g. 'dcim_site'). fk_column: The column on `dependent_table` referencing `source_table` (e.g. '_site_id'). mappings: A mapping of {dependent_column: source_column}, using actual database column names - (e.g. {'_region_id': 'region_id', '_site_group_id': 'group_id'}). + (e.g. {'_region_id': 'region_id', '_site_group_id': 'group_id'}). Each is copied directly + from the changed source row. + related_mappings: An optional iterable of related-table lookups for columns that live one hop + beyond `source_table`. Each entry is a dict with keys `table` (the related table), `source_fk` + (a column on `source_table` referencing `related_table.id`), and `mappings` + ({dependent_column: related_column}). Each is resolved by joining the related table once. + This closes the chain gap when a denormalized column is derived through an intermediate object + (e.g. a Location's Site change must refresh the dependent's region/site-group, not just its site). - The trigger fires AFTER UPDATE of the source columns, and only when at least one of them actually changed. - Like the handler it replaces, it does not fire on INSERT (a newly created source row has no dependents - yet) and it does not cascade: updating the denormalized columns does not itself trigger further - denormalization. + The trigger fires AFTER UPDATE of the watched source columns (the direct `mappings` sources plus each + related `source_fk`), and only propagates to rows whose watched column(s) actually changed (the body + joins the OLD/NEW transition tables and filters with IS DISTINCT FROM). It does not fire on INSERT (a + newly created source row has no dependents yet) and it does not recurse: the dependent tables carry no + triggers of their own. """ reversible = True - def __init__(self, dependent_table, source_table, fk_column, mappings): + def __init__(self, dependent_table, source_table, fk_column, mappings, related_mappings=()): self.dependent_table = dependent_table self.source_table = source_table self.fk_column = fk_column self.mappings = mappings + self.related_mappings = list(related_mappings) @property def function_name(self): @@ -77,17 +92,36 @@ class InstallDenormalizationTrigger(migrations.operations.base.Operation): pass def database_forwards(self, app_label, schema_editor, from_state, to_state): - source_columns = list(self.mappings.values()) - set_clause = ', '.join(f'"{dest}" = NEW."{src}"' for dest, src in self.mappings.items()) - update_of = ', '.join(f'"{col}"' for col in source_columns) - when_clause = ' OR '.join(f'OLD."{col}" IS DISTINCT FROM NEW."{col}"' for col in source_columns) + # `n`/`o` are the NEW/OLD transition tables (all rows changed by the source statement). Direct + # mappings copy from the new source row; related mappings join the related table once. + set_parts = [f'"{dest}" = n."{src}"' for dest, src in self.mappings.items()] + watched = list(self.mappings.values()) + related_joins = [] + for i, rel in enumerate(self.related_mappings): + alias = f'r{i}' + related_joins.append(f'LEFT JOIN "{rel["table"]}" AS {alias} ON {alias}.id = n."{rel["source_fk"]}"') + for dest, rel_col in rel['mappings'].items(): + set_parts.append(f'"{dest}" = {alias}."{rel_col}"') + watched.append(rel['source_fk']) + + # Deduplicate watched columns while preserving order (a direct mapping and a related lookup may + # both key off the same source column, e.g. site_id). + watched_columns = list(dict.fromkeys(watched)) + + set_clause = ', '.join(set_parts) + update_of = ', '.join(f'"{col}"' for col in watched_columns) + change_filter = ' OR '.join(f'o."{col}" IS DISTINCT FROM n."{col}"' for col in watched_columns) + joins = ('\n ' + '\n '.join(related_joins)) if related_joins else '' schema_editor.execute(f''' CREATE OR REPLACE FUNCTION "{self.function_name}"() RETURNS TRIGGER AS $$ BEGIN - UPDATE "{self.dependent_table}" + UPDATE "{self.dependent_table}" AS dep SET {set_clause} - WHERE "{self.fk_column}" = NEW.id; + FROM new_rows AS n + JOIN old_rows AS o ON o.id = n.id{joins} + WHERE dep."{self.fk_column}" = n.id + AND ({change_filter}); RETURN NULL; END $$ LANGUAGE plpgsql; @@ -95,8 +129,8 @@ class InstallDenormalizationTrigger(migrations.operations.base.Operation): schema_editor.execute(f''' CREATE TRIGGER "{self.trigger_name}" AFTER UPDATE OF {update_of} ON "{self.source_table}" - FOR EACH ROW WHEN ({when_clause}) - EXECUTE FUNCTION "{self.function_name}"(); + REFERENCING OLD TABLE AS old_rows NEW TABLE AS new_rows + FOR EACH STATEMENT EXECUTE FUNCTION "{self.function_name}"(); ''') def database_backwards(self, app_label, schema_editor, from_state, to_state): @@ -105,3 +139,41 @@ class InstallDenormalizationTrigger(migrations.operations.base.Operation): def describe(self): return f'Install denormalization trigger on {self.source_table} updating {self.dependent_table}' + + +# Site/region/site-group lookup shared by every CachedScopeMixin-style dependent (see cached_scope_triggers). +SITE_SCOPE_RELATED_MAPPINGS = ( + { + 'table': 'dcim_site', + 'source_fk': 'site_id', + 'mappings': {'_region_id': 'region_id', '_site_group_id': 'group_id'}, + }, +) + + +def cached_scope_triggers(dependent_table): + """ + Return the Site + Location `InstallDenormalizationTrigger` pair for a dependent table carrying the + standard cached-scope columns (_site/_location/_region/_site_group) — i.e. any CachedScopeMixin model + (Prefix, Cluster, WirelessLAN) plus CircuitTermination, which share the same denormalization shape. + + Region- and SiteGroup-scoped rows need no trigger: their cached FK is the scoped object itself and + never changes underneath them. So two triggers fully cover the cache: + - dcim_site: region/group changed -> refresh _region/_site_group on rows scoped to that site + - dcim_location: site changed -> refresh _site (and the new site's region/group) + """ + return [ + InstallDenormalizationTrigger( + dependent_table=dependent_table, + source_table='dcim_site', + fk_column='_site_id', + mappings={'_region_id': 'region_id', '_site_group_id': 'group_id'}, + ), + InstallDenormalizationTrigger( + dependent_table=dependent_table, + source_table='dcim_location', + fk_column='_location_id', + mappings={'_site_id': 'site_id'}, + related_mappings=SITE_SCOPE_RELATED_MAPPINGS, + ), + ] diff --git a/netbox/virtualization/migrations/0057_denormalization_triggers.py b/netbox/virtualization/migrations/0057_denormalization_triggers.py new file mode 100644 index 000000000..d40003a75 --- /dev/null +++ b/netbox/virtualization/migrations/0057_denormalization_triggers.py @@ -0,0 +1,18 @@ +""" +Maintain Cluster's denormalized scope columns (CachedScopeMixin: _site/_location/_region/_site_group) +via PostgreSQL triggers instead of the Python `dcim.signals.sync_cached_scope_fields` handler. +""" +from django.db import migrations + +from utilities.migration import cached_scope_triggers + + +class Migration(migrations.Migration): + + dependencies = [ + ('virtualization', '0056_virtualmachine_render_config_permission'), + # Source tables (dcim_site, dcim_location) must already exist. + ('dcim', '0238_ltree_paths'), + ] + + operations = cached_scope_triggers('virtualization_cluster') diff --git a/netbox/wireless/migrations/0021_denormalization_triggers.py b/netbox/wireless/migrations/0021_denormalization_triggers.py new file mode 100644 index 000000000..97dd56379 --- /dev/null +++ b/netbox/wireless/migrations/0021_denormalization_triggers.py @@ -0,0 +1,19 @@ +""" +Maintain WirelessLAN's denormalized scope columns (CachedScopeMixin: _site/_location/_region/ +_site_group) via PostgreSQL triggers instead of the Python `dcim.signals.sync_cached_scope_fields` +handler. +""" +from django.db import migrations + +from utilities.migration import cached_scope_triggers + + +class Migration(migrations.Migration): + + dependencies = [ + ('wireless', '0020_ltree_paths'), + # Source tables (dcim_site, dcim_location) must already exist. + ('dcim', '0238_ltree_paths'), + ] + + operations = cached_scope_triggers('wireless_wirelesslan') From 0bd5909cf03fe97c1aac905805836b361215789b Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 15 Jun 2026 10:39:06 -0700 Subject: [PATCH 3/4] cleanup --- netbox/utilities/migration.py | 56 ++++++++++++++++------------------- 1 file changed, 25 insertions(+), 31 deletions(-) diff --git a/netbox/utilities/migration.py b/netbox/utilities/migration.py index ca0d793cd..aaf2b3977 100644 --- a/netbox/utilities/migration.py +++ b/netbox/utilities/migration.py @@ -41,34 +41,34 @@ class InstallDenormalizationTrigger(migrations.operations.base.Operation): Install a PostgreSQL trigger that keeps denormalized columns on a dependent table in sync with their source object. - When rows in `source_table` are updated, the trigger copies the values of the mapped source columns into - the corresponding denormalized columns on every `dependent_table` row that references them via - `fk_column`. This replaces the Python `post_save` handlers formerly defined in `netbox.denormalized` and - `dcim.signals`. - - The trigger is statement-level (`FOR EACH STATEMENT`) and uses transition tables: a single bulk source - update (`UPDATE ... WHERE ...`, `QuerySet.update()`, `bulk_update()`) fires the trigger once and is - propagated with a single set-based UPDATE joining the changed rows, rather than once per affected row. + When a row in `source_table` is updated, the trigger copies the values of the mapped source columns into + the corresponding denormalized columns on every `dependent_table` row that references it via `fk_column`. + This replaces the Python `post_save` handlers formerly defined in `netbox.denormalized` and `dcim.signals`. Args: dependent_table: The table carrying the denormalized columns (e.g. 'ipam_prefix'). source_table: The table whose changes are propagated (e.g. 'dcim_site'). fk_column: The column on `dependent_table` referencing `source_table` (e.g. '_site_id'). mappings: A mapping of {dependent_column: source_column}, using actual database column names - (e.g. {'_region_id': 'region_id', '_site_group_id': 'group_id'}). Each is copied directly - from the changed source row. + (e.g. {'_region_id': 'region_id', '_site_group_id': 'group_id'}). Each is copied directly: + `dependent_column = NEW.source_column`. related_mappings: An optional iterable of related-table lookups for columns that live one hop beyond `source_table`. Each entry is a dict with keys `table` (the related table), `source_fk` (a column on `source_table` referencing `related_table.id`), and `mappings` - ({dependent_column: related_column}). Each is resolved by joining the related table once. + ({dependent_column: related_column}). Each is resolved with a single multi-column subquery + (`(cols) = (SELECT cols FROM table WHERE id = NEW.source_fk)`), so the related row is read once. This closes the chain gap when a denormalized column is derived through an intermediate object (e.g. a Location's Site change must refresh the dependent's region/site-group, not just its site). The trigger fires AFTER UPDATE of the watched source columns (the direct `mappings` sources plus each - related `source_fk`), and only propagates to rows whose watched column(s) actually changed (the body - joins the OLD/NEW transition tables and filters with IS DISTINCT FROM). It does not fire on INSERT (a + related `source_fk`), and only when at least one of them actually changed. It does not fire on INSERT (a newly created source row has no dependents yet) and it does not recurse: the dependent tables carry no triggers of their own. + + Note: this is a row-level trigger, so a bulk source update of N rows fires it N times. A statement-level + trigger with transition tables would batch this, but PostgreSQL forbids transition tables on a trigger + with an `UPDATE OF ` list, and dropping that column list would fire the trigger on every source + update (including unrelated columns) — a worse trade on hot-write tables like dcim_device. """ reversible = True @@ -92,16 +92,14 @@ class InstallDenormalizationTrigger(migrations.operations.base.Operation): pass def database_forwards(self, app_label, schema_editor, from_state, to_state): - # `n`/`o` are the NEW/OLD transition tables (all rows changed by the source statement). Direct - # mappings copy from the new source row; related mappings join the related table once. - set_parts = [f'"{dest}" = n."{src}"' for dest, src in self.mappings.items()] + # Direct column copies from the changed source row. + set_parts = [f'"{dest}" = NEW."{src}"' for dest, src in self.mappings.items()] watched = list(self.mappings.values()) - related_joins = [] - for i, rel in enumerate(self.related_mappings): - alias = f'r{i}' - related_joins.append(f'LEFT JOIN "{rel["table"]}" AS {alias} ON {alias}.id = n."{rel["source_fk"]}"') - for dest, rel_col in rel['mappings'].items(): - set_parts.append(f'"{dest}" = {alias}."{rel_col}"') + # One-hop lookups: a single multi-column subquery per related table reads its row only once. + for rel in self.related_mappings: + dests = ', '.join(f'"{d}"' for d in rel['mappings'].keys()) + cols = ', '.join(f'"{c}"' for c in rel['mappings'].values()) + set_parts.append(f'({dests}) = (SELECT {cols} FROM "{rel["table"]}" WHERE id = NEW."{rel["source_fk"]}")') watched.append(rel['source_fk']) # Deduplicate watched columns while preserving order (a direct mapping and a related lookup may @@ -110,18 +108,14 @@ class InstallDenormalizationTrigger(migrations.operations.base.Operation): set_clause = ', '.join(set_parts) update_of = ', '.join(f'"{col}"' for col in watched_columns) - change_filter = ' OR '.join(f'o."{col}" IS DISTINCT FROM n."{col}"' for col in watched_columns) - joins = ('\n ' + '\n '.join(related_joins)) if related_joins else '' + when_clause = ' OR '.join(f'OLD."{col}" IS DISTINCT FROM NEW."{col}"' for col in watched_columns) schema_editor.execute(f''' CREATE OR REPLACE FUNCTION "{self.function_name}"() RETURNS TRIGGER AS $$ BEGIN - UPDATE "{self.dependent_table}" AS dep + UPDATE "{self.dependent_table}" SET {set_clause} - FROM new_rows AS n - JOIN old_rows AS o ON o.id = n.id{joins} - WHERE dep."{self.fk_column}" = n.id - AND ({change_filter}); + WHERE "{self.fk_column}" = NEW.id; RETURN NULL; END $$ LANGUAGE plpgsql; @@ -129,8 +123,8 @@ class InstallDenormalizationTrigger(migrations.operations.base.Operation): schema_editor.execute(f''' CREATE TRIGGER "{self.trigger_name}" AFTER UPDATE OF {update_of} ON "{self.source_table}" - REFERENCING OLD TABLE AS old_rows NEW TABLE AS new_rows - FOR EACH STATEMENT EXECUTE FUNCTION "{self.function_name}"(); + FOR EACH ROW WHEN ({when_clause}) + EXECUTE FUNCTION "{self.function_name}"(); ''') def database_backwards(self, app_label, schema_editor, from_state, to_state): From 614eb7c6c124bff9ba20161a3bcd08b4eb2bffe1 Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 15 Jun 2026 10:54:23 -0700 Subject: [PATCH 4/4] fix review comments --- netbox/dcim/tests/test_signals.py | 15 +++++++++++---- netbox/utilities/migration.py | 6 ++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/netbox/dcim/tests/test_signals.py b/netbox/dcim/tests/test_signals.py index 1b83f892d..31d11e74c 100644 --- a/netbox/dcim/tests/test_signals.py +++ b/netbox/dcim/tests/test_signals.py @@ -1,6 +1,7 @@ from types import SimpleNamespace from unittest.mock import MagicMock, patch +from django.apps import apps from django.contrib.contenttypes.models import ContentType from django.db import connection from django.test import SimpleTestCase, TestCase @@ -30,6 +31,7 @@ from dcim.models import ( from dcim.models.device_components import ComponentModel from dcim.models.mixins import CachedScopeMixin from ipam.models import Prefix +from netbox.plugins import PluginConfig from virtualization.models import Cluster, ClusterType from wireless.models import WirelessLAN @@ -573,11 +575,15 @@ class CableTerminationDenormalizationTriggerTestCase(TestCase): def _concrete_subclasses(base): - """Yield every non-abstract model descending from an abstract base model.""" + """ + Yield every non-abstract, non-plugin model descending from an abstract base model. Plugin-contributed + models are skipped: a plugin that adds a ComponentModel/CachedScopeMixin subclass is responsible for + its own trigger migration, and must not fail core's coverage check just by being installed. + """ for subclass in base.__subclasses__(): if subclass._meta.abstract: yield from _concrete_subclasses(subclass) - else: + elif not isinstance(apps.get_app_config(subclass._meta.app_label), PluginConfig): yield subclass @@ -589,10 +595,11 @@ def _installed_triggers(): class DenormalizationTriggerCoverageTestCase(TestCase): """ - Guard against a new model silently shipping without its denormalization triggers. The set of + Guard against a new core model silently shipping without its denormalization triggers. The set of device-component tables and CachedScopeMixin dependents is hand-listed in migrations; this test derives those sets from the model layer and asserts the expected triggers are installed, so adding - a new component / scoped model without a matching trigger migration fails CI. + a new component / scoped model without a matching trigger migration fails CI. Plugin-contributed + models are excluded (see _concrete_subclasses). """ def test_device_components_have_device_trigger(self): diff --git a/netbox/utilities/migration.py b/netbox/utilities/migration.py index aaf2b3977..296655930 100644 --- a/netbox/utilities/migration.py +++ b/netbox/utilities/migration.py @@ -59,6 +59,9 @@ class InstallDenormalizationTrigger(migrations.operations.base.Operation): (`(cols) = (SELECT cols FROM table WHERE id = NEW.source_fk)`), so the related row is read once. This closes the chain gap when a denormalized column is derived through an intermediate object (e.g. a Location's Site change must refresh the dependent's region/site-group, not just its site). + If `source_fk` is NULL the subquery returns no row and all its target columns are set to NULL, + which is the correct result (the source object has no related object); current callers use a + non-nullable `source_fk` (Location.site), so this does not arise in practice. The trigger fires AFTER UPDATE of the watched source columns (the direct `mappings` sources plus each related `source_fk`), and only when at least one of them actually changed. It does not fire on INSERT (a @@ -120,6 +123,9 @@ class InstallDenormalizationTrigger(migrations.operations.base.Operation): END $$ LANGUAGE plpgsql; ''') + # Drop first so the operation is idempotent (re-run / partially-applied migration, or a + # trigger pre-installed during testing); CREATE TRIGGER alone errors if one already exists. + schema_editor.execute(f'DROP TRIGGER IF EXISTS "{self.trigger_name}" ON "{self.source_table}";') schema_editor.execute(f''' CREATE TRIGGER "{self.trigger_name}" AFTER UPDATE OF {update_of} ON "{self.source_table}"