From 90675dbbabac16d7ac02106f6df8e077b6efa68f Mon Sep 17 00:00:00 2001 From: Martin Hauser Date: Mon, 7 Sep 2026 20:14:10 +0200 Subject: [PATCH] fix(dcim): Persist normalized cable length on partial saves Cable.save() recomputed _abs_length in memory but never added it to update_fields, so a save naming length or length_unit left the stored normalized value stale. Derive it from the values the row will hold after the save and persist it alongside its source fields. Fixes #23096 --- netbox/dcim/models/cables.py | 35 ++++-- netbox/dcim/tests/test_models.py | 209 +++++++++++++++++++++++++++++++ 2 files changed, 235 insertions(+), 9 deletions(-) diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index f26cf1e97..af39946f1 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -9,7 +9,7 @@ from django.contrib.postgres.fields import ArrayField from django.contrib.postgres.indexes import GinIndex from django.core.exceptions import ValidationError from django.core.validators import MaxValueValidator, MinValueValidator -from django.db import models +from django.db import models, router from django.dispatch import Signal from django.urls import reverse from django.utils.translation import gettext_lazy as _ @@ -328,15 +328,32 @@ class Cable(PrimaryModel): } update_fields = normalize_update_fields(save_kwargs) - # Store the given length (if any) in meters for use in database ordering - if self.length is not None and self.length_unit: - self._abs_length = to_meters(self.length, self.length_unit) - else: - self._abs_length = None + length_written = update_fields is None or 'length' in update_fields + length_unit_written = update_fields is None or 'length_unit' in update_fields - # Clear length_unit if no length is defined - if self.length is None: - self.length_unit = None + if length_written or length_unit_written: + if length_written and length_unit_written: + stored = {} + else: + # Read from the database this save will write, so a router cannot split the two + db = using or router.db_for_write(Cable, instance=self) + stored = Cable.objects.using(db).filter(pk=self.pk).values('length', 'length_unit').first() or {} + length = self.length if length_written else stored.get('length') + length_unit = self.length_unit if length_unit_written else stored.get('length_unit') + + # Clear length_unit if no length is defined + if length is None and length_unit_written: + self.length_unit = None + + # Store the given length (if any) in meters for use in database ordering + if length is not None and length_unit: + self._abs_length = to_meters(length, length_unit) + else: + self._abs_length = None + + # _abs_length is a denormalized cache of length and length_unit, so persist them together + if update_fields is not None: + save_kwargs['update_fields'] = update_fields | {'_abs_length'} # A field counts as changed only when this save actually writes it status_written = update_fields is None or 'status' in update_fields diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index 3c870f4ec..036426665 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -1,9 +1,11 @@ from decimal import Decimal from django.core.exceptions import ValidationError +from django.db import connection from django.db.models import ProtectedError from django.db.models.signals import post_save from django.test import TestCase, tag +from django.test.utils import CaptureQueriesContext from circuits.models import * from core.models import ObjectType @@ -2700,6 +2702,213 @@ class CableTestCase(TestCase): self.assertEqual(cable._abs_length, Decimal('1609343983.9066')) + def test_partial_save_persists_normalized_length(self): + """ + A save naming only length must persist the normalized length alongside it. + """ + cable = Cable.objects.first() + cable.length = Decimal('1') + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save() + + cable.length = Decimal('2') + cable.save(update_fields=['length']) + cable.refresh_from_db() + + self.assertEqual(cable.length, Decimal('2.00')) + self.assertEqual(cable.length_unit, CableLengthUnitChoices.UNIT_METER) + self.assertEqual(cable._abs_length, Decimal('2.0000')) + + def test_partial_save_persists_normalized_length_for_a_unit_change(self): + """ + A save naming only length_unit must renormalize against the stored length. + """ + cable = Cable.objects.first() + cable.length = Decimal('10') + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save() + + cable.length_unit = CableLengthUnitChoices.UNIT_FOOT + cable.save(update_fields=['length_unit']) + cable.refresh_from_db() + + self.assertEqual(cable.length, Decimal('10.00')) + self.assertEqual(cable.length_unit, CableLengthUnitChoices.UNIT_FOOT) + self.assertEqual(cable._abs_length, Decimal('3.0480')) + + def test_partial_save_persists_normalized_length_for_both_source_fields(self): + """ + A save naming both source fields must normalize from the values being written. + """ + cable = Cable.objects.first() + cable.length = Decimal('2') + cable.length_unit = CableLengthUnitChoices.UNIT_KILOMETER + cable.save(update_fields=['length', 'length_unit']) + cable.refresh_from_db() + + self.assertEqual(cable._abs_length, Decimal('2000.0000')) + + def test_partial_save_normalizes_against_an_unwritten_length_unit(self): + """ + A save naming only length must normalize against the stored unit, not an unwritten one. + """ + cable = Cable.objects.first() + cable.length = Decimal('1') + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save() + + cable.length = Decimal('2') + cable.length_unit = CableLengthUnitChoices.UNIT_KILOMETER + cable.save(update_fields=['length']) + cable.refresh_from_db() + + self.assertEqual(cable.length_unit, CableLengthUnitChoices.UNIT_METER) + self.assertEqual(cable._abs_length, Decimal('2.0000')) + + def test_partial_save_normalizes_against_an_unwritten_length(self): + """ + A save naming only length_unit must normalize against the stored length, not an unwritten one. + """ + cable = Cable.objects.first() + cable.length = Decimal('1') + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save() + + cable.length = Decimal('2') + cable.length_unit = CableLengthUnitChoices.UNIT_CENTIMETER + cable.save(update_fields=['length_unit']) + cable.refresh_from_db() + + self.assertEqual(cable.length, Decimal('1.00')) + self.assertEqual(cable.length_unit, CableLengthUnitChoices.UNIT_CENTIMETER) + self.assertEqual(cable._abs_length, Decimal('0.0100')) + + def test_partial_save_normalizes_against_an_unwritten_cleared_length(self): + """ + A save naming only length_unit must keep the unit when the excluded length is cleared in memory. + """ + cable = Cable.objects.first() + cable.length = Decimal('1') + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save() + + cable.length = None + cable.length_unit = CableLengthUnitChoices.UNIT_CENTIMETER + cable.save(update_fields=['length_unit']) + cable.refresh_from_db() + + self.assertEqual(cable.length, Decimal('1.00')) + self.assertEqual(cable.length_unit, CableLengthUnitChoices.UNIT_CENTIMETER) + self.assertEqual(cable._abs_length, Decimal('0.0100')) + + def test_partial_save_clearing_length_keeps_the_stored_unit(self): + """ + A save naming only length must clear the normalized length without writing length_unit. + """ + cable = Cable.objects.first() + cable.length = Decimal('1') + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save() + + cable.length = None + cable.save(update_fields=['length']) + + # The unit was not written, so the instance must still agree with the row + self.assertEqual(cable.length_unit, CableLengthUnitChoices.UNIT_METER) + + cable.refresh_from_db() + + self.assertIsNone(cable.length) + self.assertEqual(cable.length_unit, CableLengthUnitChoices.UNIT_METER) + self.assertIsNone(cable._abs_length) + + def test_partial_save_leaves_an_unwritten_length_alone(self): + """ + A save naming an unrelated field must not persist an in-memory length change. + """ + cable = Cable.objects.first() + cable.length = Decimal('1') + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save() + + cable.length = Decimal('99') + cable.label = 'Renamed' + cable.save(update_fields=['label']) + cable.refresh_from_db() + + self.assertEqual(cable.label, 'Renamed') + self.assertEqual(cable.length, Decimal('1.00')) + self.assertEqual(cable._abs_length, Decimal('1.0000')) + + def test_partial_save_normalizes_against_an_out_of_band_length(self): + """ + A save naming only length_unit must read the stored length, not one cached on the instance. + """ + cable = Cable.objects.first() + cable.length = Decimal('1') + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save() + + Cable.objects.filter(pk=cable.pk).update(length=Decimal('7')) + + cable.length_unit = CableLengthUnitChoices.UNIT_FOOT + cable.save(update_fields=['length_unit']) + cable.refresh_from_db() + + self.assertEqual(cable.length, Decimal('7.00')) + self.assertEqual(cable._abs_length, Decimal('2.1336')) + + def test_partial_save_clears_a_unit_written_without_a_stored_length(self): + """ + A save naming only length_unit must drop the unit when the row holds no length. + """ + cable = Cable.objects.first() + + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save(update_fields=['length_unit']) + cable.refresh_from_db() + + self.assertIsNone(cable.length) + self.assertIsNone(cable.length_unit) + self.assertIsNone(cable._abs_length) + + def test_full_save_clears_the_unit_when_the_length_is_removed(self): + """ + A full save with no length must clear the stored unit. + """ + cable = Cable.objects.first() + cable.length = Decimal('1') + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save() + + cable.length = None + cable.save() + cable.refresh_from_db() + + self.assertIsNone(cable.length) + self.assertIsNone(cable.length_unit) + self.assertIsNone(cable._abs_length) + + def test_partial_save_reads_the_stored_pair_only_for_an_excluded_field(self): + """ + A save writing both source fields must normalize without reading the row back. + """ + cable = Cable.objects.first() + cable.length = Decimal('1') + cable.length_unit = CableLengthUnitChoices.UNIT_METER + cable.save() + + with CaptureQueriesContext(connection) as both_written: + cable.save(update_fields=['length', 'length_unit']) + with CaptureQueriesContext(connection) as one_written: + cable.save(update_fields=['length']) + + def cable_reads(queries): + return [q for q in queries if q['sql'].startswith('SELECT') and 'FROM "dcim_cable"' in q['sql']] + + self.assertEqual(cable_reads(both_written), []) + self.assertEqual(len(cable_reads(one_written)), 1) + class CableTerminationTestCase(TestCase):