From d954511c2c49a722f78a31bce32d5f767e78ecec Mon Sep 17 00:00:00 2001 From: Tom Gamull Date: Fri, 7 Nov 2025 07:08:54 -0500 Subject: [PATCH] Fixes #355: Handle None values in ChangeDiff._update_conflicts() This fixes an AttributeError that occurs when creating objects with custom fields in a branch. PR #350 partially addressed this by checking for None in self.original and self.current, but missed checking self.modified. Changes: - Add None check for self.modified in addition to original and current - Add safety check 'k in self.modified' before accessing modified[k] - Explicitly set self.conflicts = None before returning for clarity This prevents AttributeError when: - Creating circuit terminations with custom fields - Creating devices with module bays and custom fields - Any object creation where self.modified is None Added comprehensive test coverage in test_changediff.py to verify the fix handles all None value scenarios correctly. --- netbox_branching/models/changes.py | 9 +- netbox_branching/tests/test_changediff.py | 171 ++++++++++++++++++++++ 2 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 netbox_branching/tests/test_changediff.py diff --git a/netbox_branching/models/changes.py b/netbox_branching/models/changes.py index 9e2fcc5..f82b7d7 100644 --- a/netbox_branching/models/changes.py +++ b/netbox_branching/models/changes.py @@ -196,14 +196,17 @@ def _update_conflicts(self): """ Record any conflicting changes between the modified and current object data. """ - if self.original is None or self.current is None: - # Both the original and current states must be available to compare + # All three states (original, modified, current) must be available to detect conflicts + if self.original is None or self.modified is None or self.current is None: + self.conflicts = None return + conflicts = None if self.action == ObjectChangeActionChoices.ACTION_UPDATE: conflicts = [ k for k, v in self.original.items() - if v != self.modified[k] and v != self.current.get(k) and self.modified[k] != self.current.get(k) + if k in self.modified and v != self.modified[k] and v != self.current.get(k) + and self.modified[k] != self.current.get(k) ] elif self.action == ObjectChangeActionChoices.ACTION_DELETE: conflicts = [ diff --git a/netbox_branching/tests/test_changediff.py b/netbox_branching/tests/test_changediff.py new file mode 100644 index 0000000..f155178 --- /dev/null +++ b/netbox_branching/tests/test_changediff.py @@ -0,0 +1,171 @@ +from django.test import TestCase +from django.contrib.contenttypes.models import ContentType + +from core.choices import ObjectChangeActionChoices +from netbox_branching.models import Branch, ChangeDiff + + +class ChangeDiffTestCase(TestCase): + """ + Test cases for ChangeDiff model, specifically the _update_conflicts() method. + """ + + def setUp(self): + """Set up test branch and content type.""" + self.branch = Branch.objects.create(name='Test Branch') + # Use a generic content type for testing + self.content_type = ContentType.objects.get_for_model(Branch) + + def test_update_conflicts_with_none_modified(self): + """ + Test that _update_conflicts() handles None modified data gracefully. + Fixes issue #355: AttributeError when self.modified is None. + """ + change_diff = ChangeDiff( + branch=self.branch, + object_type=self.content_type, + object_id=1, + object_repr='Test Object', + action=ObjectChangeActionChoices.ACTION_UPDATE, + original={'field1': 'value1', 'field2': 'value2'}, + modified=None, # This causes the AttributeError in issue #355 + current={'field1': 'value1', 'field2': 'value3'} + ) + + # Should not raise AttributeError + try: + change_diff.save() + # Conflicts should be None when modified is None + self.assertIsNone(change_diff.conflicts) + except AttributeError as e: + self.fail(f'_update_conflicts() raised AttributeError with None modified: {e}') + + def test_update_conflicts_with_none_original(self): + """ + Test that _update_conflicts() handles None original data gracefully. + This was partially fixed in PR #350. + """ + change_diff = ChangeDiff( + branch=self.branch, + object_type=self.content_type, + object_id=1, + object_repr='Test Object', + action=ObjectChangeActionChoices.ACTION_UPDATE, + original=None, + modified={'field1': 'value1', 'field2': 'value2'}, + current={'field1': 'value1', 'field2': 'value3'} + ) + + # Should not raise AttributeError + try: + change_diff.save() + self.assertIsNone(change_diff.conflicts) + except AttributeError as e: + self.fail(f'_update_conflicts() raised AttributeError with None original: {e}') + + def test_update_conflicts_with_none_current(self): + """ + Test that _update_conflicts() handles None current data gracefully. + This was partially fixed in PR #350. + """ + change_diff = ChangeDiff( + branch=self.branch, + object_type=self.content_type, + object_id=1, + object_repr='Test Object', + action=ObjectChangeActionChoices.ACTION_UPDATE, + original={'field1': 'value1', 'field2': 'value2'}, + modified={'field1': 'value1', 'field2': 'value3'}, + current=None + ) + + # Should not raise AttributeError + try: + change_diff.save() + self.assertIsNone(change_diff.conflicts) + except AttributeError as e: + self.fail(f'_update_conflicts() raised AttributeError with None current: {e}') + + def test_update_conflicts_with_valid_data_no_conflicts(self): + """ + Test that _update_conflicts() correctly identifies no conflicts. + """ + change_diff = ChangeDiff( + branch=self.branch, + object_type=self.content_type, + object_id=1, + object_repr='Test Object', + action=ObjectChangeActionChoices.ACTION_UPDATE, + original={'field1': 'value1', 'field2': 'value2'}, + modified={'field1': 'value1', 'field2': 'value3'}, + current={'field1': 'value1', 'field2': 'value2'} + ) + + change_diff.save() + # No conflicts: field2 changed in modified but not in current + self.assertIsNone(change_diff.conflicts) + + def test_update_conflicts_with_valid_data_has_conflicts(self): + """ + Test that _update_conflicts() correctly identifies conflicts. + """ + change_diff = ChangeDiff( + branch=self.branch, + object_type=self.content_type, + object_id=1, + object_repr='Test Object', + action=ObjectChangeActionChoices.ACTION_UPDATE, + original={'field1': 'value1', 'field2': 'value2'}, + modified={'field1': 'value_branch', 'field2': 'value2'}, + current={'field1': 'value_main', 'field2': 'value2'} + ) + + change_diff.save() + # Conflict: field1 changed differently in both branch and main + self.assertIsNotNone(change_diff.conflicts) + self.assertIn('field1', change_diff.conflicts) + + def test_update_conflicts_with_missing_key_in_modified(self): + """ + Test that _update_conflicts() handles missing keys in modified data. + Ensures 'k in self.modified' check prevents KeyError. + """ + change_diff = ChangeDiff( + branch=self.branch, + object_type=self.content_type, + object_id=1, + object_repr='Test Object', + action=ObjectChangeActionChoices.ACTION_UPDATE, + original={'field1': 'value1', 'field2': 'value2'}, + modified={'field1': 'value1'}, # field2 missing + current={'field1': 'value1', 'field2': 'value3'} + ) + + # Should not raise KeyError + try: + change_diff.save() + # Should complete without error + self.assertIsNone(change_diff.conflicts) + except KeyError as e: + self.fail(f'_update_conflicts() raised KeyError with missing key: {e}') + + def test_delete_conflicts_with_none_original(self): + """ + Test DELETE action with None original data. + """ + change_diff = ChangeDiff( + branch=self.branch, + object_type=self.content_type, + object_id=1, + object_repr='Test Object', + action=ObjectChangeActionChoices.ACTION_DELETE, + original=None, + modified=None, + current={'field1': 'value1'} + ) + + try: + change_diff.save() + self.assertIsNone(change_diff.conflicts) + except AttributeError as e: + self.fail(f'_update_conflicts() raised AttributeError on DELETE with None original: {e}')