From 0f22d676176c40ef37c39c9772b12b32303529c7 Mon Sep 17 00:00:00 2001 From: Martin Hauser Date: Mon, 31 Aug 2026 17:35:10 +0200 Subject: [PATCH] fix(models): Normalize update_fields to prevent generator consumption Introduce `normalize_update_fields()` utility to convert update_fields to frozenset, preventing one-shot iterables from being consumed during membership tests. Update Service, VLANGroup, and CircuitTermination save methods to use normalized fields. Add comprehensive test coverage. Fixes generator exhaustion when save() overrides check field membership before persisting denormalized caches alongside their source fields. Fixes #23078 --- netbox/circuits/models/circuits.py | 3 +- netbox/circuits/tests/test_models.py | 22 ++++++++++++ netbox/ipam/models/services.py | 8 ++--- netbox/ipam/models/vlans.py | 5 +-- netbox/ipam/tests/test_models.py | 54 ++++++++++++++++++++++++++++ netbox/utilities/data.py | 14 ++++++++ netbox/utilities/tests/test_data.py | 31 ++++++++++++++++ 7 files changed, 130 insertions(+), 7 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 10d621092..2b194e4c3 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -17,6 +17,7 @@ from netbox.models.features import ( TagsMixin, ) from netbox.models.mixins import DistanceMixin +from utilities.data import normalize_update_fields from utilities.string import title from .base import BaseCircuitType @@ -382,7 +383,7 @@ class CircuitTermination( def save(self, *args, **kwargs): is_new = self._state.adding - update_fields = kwargs.get('update_fields') + update_fields = normalize_update_fields(kwargs) # Only consider circuit/term_side changes if those fields # are actually being persisted diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index a3c4a54ca..b7d078ca2 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -76,6 +76,28 @@ class CircuitTerminationTestCase(TestCase): # New circuit's cache should be populated self.assertEqual(self.circuits[1].termination_a, termination) + def test_circuit_termination_circuit_change_with_generator_update_fields(self): + """ + A one-shot iterable passed as update_fields must still reach the database, so the + circuit change is persisted and both caches are updated. + """ + termination = CircuitTermination.objects.create( + circuit=self.circuits[0], + term_side='A', + termination=self.sites[0], + ) + + termination.circuit = self.circuits[1] + termination.save(update_fields=(field for field in ('circuit',))) + + termination.refresh_from_db() + self.circuits[0].refresh_from_db() + self.circuits[1].refresh_from_db() + + self.assertEqual(termination.circuit, self.circuits[1]) + self.assertIsNone(self.circuits[0].termination_a) + self.assertEqual(self.circuits[1].termination_a, termination) + def test_circuit_termination_term_side_change_clears_old_cache(self): """ When a CircuitTermination's term_side is changed, the old side's cache should be cleared diff --git a/netbox/ipam/models/services.py b/netbox/ipam/models/services.py index ee12e3b04..5642adf25 100644 --- a/netbox/ipam/models/services.py +++ b/netbox/ipam/models/services.py @@ -8,7 +8,7 @@ from ipam.choices import * from ipam.constants import * from netbox.models import PrimaryModel from netbox.models.features import ContactsMixin -from utilities.data import array_to_string +from utilities.data import array_to_string, normalize_update_fields __all__ = ( 'Service', @@ -42,9 +42,9 @@ class ServiceBase(models.Model): def save(self, *args, **kwargs): # On saving find the smallest port and save for default ordering self._ports_lowest = min(self.ports) if self.ports else None - update_fields = kwargs.get('update_fields') - if update_fields is not None and '_ports_lowest' not in update_fields: - kwargs['update_fields'] = list(update_fields) + ['_ports_lowest'] + update_fields = normalize_update_fields(kwargs) + if update_fields is not None and 'ports' in update_fields: + kwargs['update_fields'] = update_fields | {'_ports_lowest'} super().save(*args, **kwargs) def __str__(self): diff --git a/netbox/ipam/models/vlans.py b/netbox/ipam/models/vlans.py index c294ef39e..538c45298 100644 --- a/netbox/ipam/models/vlans.py +++ b/netbox/ipam/models/vlans.py @@ -16,6 +16,7 @@ from utilities.data import ( check_ranges_overlap, get_inclusive_integer_range_bounds, normalize_integer_range, + normalize_update_fields, ranges_to_string, ranges_to_string_list, ) @@ -148,10 +149,10 @@ class VLANGroup(OrganizationalModel): self.total_vlan_ids += vid_range.upper - vid_range.lower self.vid_ranges = vid_ranges - update_fields = kwargs.get('update_fields') + update_fields = normalize_update_fields(kwargs) if update_fields is not None and 'vid_ranges' in update_fields: # total_vlan_ids is a denormalized cache of vid_ranges; persist them together. - kwargs['update_fields'] = list(set(update_fields) | {'total_vlan_ids'}) + kwargs['update_fields'] = update_fields | {'total_vlan_ids'} super().save(*args, **kwargs) diff --git a/netbox/ipam/tests/test_models.py b/netbox/ipam/tests/test_models.py index c20a99bc6..20a3bab33 100644 --- a/netbox/ipam/tests/test_models.py +++ b/netbox/ipam/tests/test_models.py @@ -1799,6 +1799,20 @@ class VLANGroupTestCase(TestCase): self.assertEqual(vlangroup.vid_ranges, [NumericRange(100, 101, bounds='[)')]) self.assertEqual(vlangroup.total_vlan_ids, 1) + def test_total_vlan_ids_with_generator_update_fields(self): + vlangroup = VLANGroup.objects.create( + name='VLAN Group Generator Update Fields', + slug='vlan-group-generator-update-fields', + vid_ranges=[NumericRange(100, 200, bounds='[)')], + ) + + vlangroup.vid_ranges = [NumericRange(100, 100, bounds='[]')] + vlangroup.save(update_fields=(field for field in ('vid_ranges',))) + vlangroup.refresh_from_db() + + self.assertEqual(vlangroup.vid_ranges, [NumericRange(100, 101, bounds='[)')]) + self.assertEqual(vlangroup.total_vlan_ids, 1) + def test_annotate_utilization_with_zero_total_vlan_ids(self): vlangroup = VLANGroup.objects.create( name='VLAN Group Zero Total', @@ -1957,6 +1971,46 @@ class ServiceTemplateTestCase(TestCase): template.save() self.assertEqual(template._ports_lowest, 53) + def test_servicetemplate_lowest_port_with_generator_update_fields(self): + """ + A one-shot iterable in update_fields must still persist the ports change + alongside the derived _ports_lowest. + """ + template = ServiceTemplate( + name='Template 4', + protocol=ServiceProtocolChoices.PROTOCOL_TCP, + ports=[80, 443], + ) + template.full_clean() + template.save() + + template.ports = [22, 8080] + template.save(update_fields=(field for field in ('ports',))) + template.refresh_from_db() + + self.assertEqual(template.ports, [22, 8080]) + self.assertEqual(template._ports_lowest, 22) + + def test_servicetemplate_unrelated_update_fields_leaves_ports_alone(self): + """ + A save naming an unrelated field must not persist _ports_lowest derived from an + in-memory ports change that is not itself being written. + """ + template = ServiceTemplate.objects.create( + name='Template 5', + protocol=ServiceProtocolChoices.PROTOCOL_TCP, + ports=[80, 443], + ) + + template.ports = [22] + template.name = 'Template 5 renamed' + template.save(update_fields=['name']) + template.refresh_from_db() + + self.assertEqual(template.name, 'Template 5 renamed') + self.assertEqual(template.ports, [80, 443]) + self.assertEqual(template._ports_lowest, 80) + def test_servicetemplate_empty_ports(self): """ Test with empty ports list diff --git a/netbox/utilities/data.py b/netbox/utilities/data.py index 4cc486be8..fe9cf668c 100644 --- a/netbox/utilities/data.py +++ b/netbox/utilities/data.py @@ -14,6 +14,7 @@ __all__ = ( 'get_config_value_ci', 'get_inclusive_integer_range_bounds', 'normalize_integer_range', + 'normalize_update_fields', 'ranges_to_string', 'ranges_to_string_list', 'resolve_attr_path', @@ -115,6 +116,19 @@ def deep_compare_dict(source_dict, destination_dict, exclude=tuple()): return added, removed +def normalize_update_fields(kwargs): + """ + Replace `kwargs['update_fields']` with a frozenset and return it, so a save() override can + run membership tests without consuming a one-shot iterable. `None` and an absent key are + left alone. + """ + update_fields = kwargs.get('update_fields') + if update_fields is not None: + update_fields = frozenset(update_fields) + kwargs['update_fields'] = update_fields + return update_fields + + # # Array utilities # diff --git a/netbox/utilities/tests/test_data.py b/netbox/utilities/tests/test_data.py index f557bc79e..b969fd717 100644 --- a/netbox/utilities/tests/test_data.py +++ b/netbox/utilities/tests/test_data.py @@ -7,6 +7,7 @@ from utilities.data import ( get_config_value_ci, get_inclusive_integer_range_bounds, normalize_integer_range, + normalize_update_fields, ranges_to_string, ranges_to_string_list, string_to_ranges, @@ -229,3 +230,33 @@ class GetConfigValueCITestCase(TestCase): def test_empty_dict(self): self.assertIsNone(get_config_value_ci({}, 'any.key')) self.assertEqual(get_config_value_ci({}, 'any.key', default=[]), []) + + +class NormalizeUpdateFieldsTestCase(TestCase): + + def test_none_is_passed_through(self): + kwargs = {'update_fields': None} + self.assertIsNone(normalize_update_fields(kwargs)) + self.assertIsNone(kwargs['update_fields']) + + def test_absent_key_is_not_added(self): + kwargs = {} + self.assertIsNone(normalize_update_fields(kwargs)) + self.assertNotIn('update_fields', kwargs) + + def test_generator_is_materialized_in_place(self): + kwargs = {'update_fields': (field for field in ('name', 'description'))} + update_fields = normalize_update_fields(kwargs) + + self.assertEqual(update_fields, frozenset({'name', 'description'})) + self.assertEqual(kwargs['update_fields'], frozenset({'name', 'description'})) + + def test_empty_generator_normalizes_to_empty_frozenset(self): + kwargs = {'update_fields': (field for field in ())} + self.assertEqual(normalize_update_fields(kwargs), frozenset()) + self.assertEqual(kwargs['update_fields'], frozenset()) + + def test_list_is_normalized(self): + kwargs = {'update_fields': ['name']} + self.assertEqual(normalize_update_fields(kwargs), frozenset({'name'})) + self.assertEqual(kwargs['update_fields'], frozenset({'name'}))