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'}))