Ver Fonte

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
Martin Hauser há 2 dias atrás
pai
commit
0f22d67617

+ 2 - 1
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

+ 22 - 0
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

+ 4 - 4
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):

+ 3 - 2
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)
 

+ 54 - 0
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

+ 14 - 0
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
 #

+ 31 - 0
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'}))