Merge pull request #21691 from netbox-community/14329-cf

#14329 Improve diffs for custom_fields

netbox/core/models/change_logging.py

@@ -11,7 +11,7 @@ from mptt.models import MPTTModel
 from core.choices import ObjectChangeActionChoices
 from core.querysets import ObjectChangeQuerySet
 from netbox.models.features import ChangeLoggingMixin, has_feature
-from utilities.data import shallow_compare_dict
+from utilities.data import deep_compare_dict
 
 __all__ = (
     'ObjectChange',
@@ -199,18 +199,18 @@ class ObjectChange(models.Model):
         # Determine which attributes have changed
         if self.action == ObjectChangeActionChoices.ACTION_CREATE:
             changed_attrs = sorted(postchange_data.keys())
-        elif self.action == ObjectChangeActionChoices.ACTION_DELETE:
+            return {
+                'pre': {k: prechange_data.get(k) for k in changed_attrs},
+                'post': {k: postchange_data.get(k) for k in changed_attrs},
+            }
+        if self.action == ObjectChangeActionChoices.ACTION_DELETE:
             changed_attrs = sorted(prechange_data.keys())
-        else:
-            # TODO: Support deep (recursive) comparison
-            changed_data = shallow_compare_dict(prechange_data, postchange_data)
-            changed_attrs = sorted(changed_data.keys())
-
+            return {
+                'pre': {k: prechange_data.get(k) for k in changed_attrs},
+                'post': {k: postchange_data.get(k) for k in changed_attrs},
+            }
+        diff_added, diff_removed = deep_compare_dict(prechange_data, postchange_data)
         return {
-            'pre': {
-                k: prechange_data.get(k) for k in changed_attrs
-            },
-            'post': {
-                k: postchange_data.get(k) for k in changed_attrs
-            },
+            'pre': dict(sorted(diff_removed.items())),
+            'post': dict(sorted(diff_added.items())),
         }

netbox/core/views.py

@@ -30,7 +30,7 @@ from netbox.views import generic
 from netbox.views.generic.base import BaseObjectView
 from netbox.views.generic.mixins import TableMixin
 from utilities.apps import get_installed_apps
-from utilities.data import shallow_compare_dict
+from utilities.data import deep_compare_dict
 from utilities.forms import ConfirmationForm
 from utilities.htmx import htmx_partial
 from utilities.json import ConfigJSONEncoder
@@ -273,14 +273,11 @@ class ObjectChangeView(generic.ObjectView):
             prechange_data = instance.prechange_data_clean
 
         if prechange_data and instance.postchange_data:
-            diff_added = shallow_compare_dict(
-                prechange_data or dict(),
-                instance.postchange_data_clean or dict(),
+            diff_added, diff_removed = deep_compare_dict(
+                prechange_data,
+                instance.postchange_data_clean,
                 exclude=['last_updated'],
             )
-            diff_removed = {
-                x: prechange_data.get(x) for x in diff_added
-            } if prechange_data else {}
         else:
             diff_added = None
             diff_removed = None

netbox/templates/core/objectchange.html

@@ -120,7 +120,17 @@
               {% spaceless %}
                 <pre class="change-data">
                   {% for k, v in object.prechange_data_clean.items %}
-                    <span{% if k in diff_removed %} class="removed"{% endif %}>{{ k }}: {{ v|json }}</span>
+                    {% with subdiff=diff_removed|get_key:k %}
+                      {% if subdiff.items %}
+                        <span>{{ k }}: {</span>
+                        {% for sub_k, sub_v in v.items %}
+                          <span class="ps-4{% if sub_k in subdiff %} removed{% endif %}">{{ sub_k }}: {{ sub_v|json }}</span>
+                        {% endfor %}
+                        <span>}</span>
+                      {% else %}
+                        <span{% if k in diff_removed %} class="removed"{% endif %}>{{ k }}: {{ v|json }}</span>
+                      {% endif %}
+                    {% endwith %}
                   {% endfor %}
                 </pre>
               {% endspaceless %}
@@ -140,7 +150,17 @@
                   {% spaceless %}
                     <pre class="change-data">
                       {% for k, v in object.postchange_data_clean.items %}
-                        <span{% if k in diff_added %} class="added"{% endif %}>{{ k }}: {{ v|json }}</span>
+                        {% with subdiff=diff_added|get_key:k %}
+                          {% if subdiff.items %}
+                            <span>{{ k }}: {</span>
+                            {% for sub_k, sub_v in v.items %}
+                              <span class="ps-4{% if sub_k in subdiff %} added{% endif %}">{{ sub_k }}: {{ sub_v|json }}</span>
+                            {% endfor %}
+                            <span>}</span>
+                          {% else %}
+                            <span{% if k in diff_added %} class="added"{% endif %}>{{ k }}: {{ v|json }}</span>
+                          {% endif %}
+                        {% endwith %}
                       {% endfor %}
                     </pre>
                   {% endspaceless %}

netbox/utilities/data.py

@@ -7,6 +7,7 @@ __all__ = (
     'array_to_ranges',
     'array_to_string',
     'check_ranges_overlap',
+    'deep_compare_dict',
     'deepmerge',
     'drange',
     'flatten_dict',
@@ -83,6 +84,35 @@ def shallow_compare_dict(source_dict, destination_dict, exclude=tuple()):
     return difference
 
 
+def deep_compare_dict(source_dict, destination_dict, exclude=tuple()):
+    """
+    Return a two-tuple of dictionaries (added, removed) representing the differences between source_dict and
+    destination_dict. For values which are themselves dicts, the comparison is performed recursively such that only
+    the changed keys within the nested dict are included. `exclude` is a list or tuple of keys to be ignored.
+    """
+    added = {}
+    removed = {}
+
+    all_keys = set(source_dict) | set(destination_dict)
+    for key in all_keys:
+        if key in exclude:
+            continue
+        src_val = source_dict.get(key)
+        dst_val = destination_dict.get(key)
+        if src_val == dst_val:
+            continue
+        if isinstance(src_val, dict) and isinstance(dst_val, dict):
+            sub_added, sub_removed = deep_compare_dict(src_val, dst_val)
+            if sub_added or sub_removed:
+                added[key] = sub_added
+                removed[key] = sub_removed
+        else:
+            added[key] = dst_val
+            removed[key] = src_val
+
+    return added, removed
+
+
 #
 # Array utilities
 #

netbox/utilities/tests/test_data.py

@@ -3,6 +3,7 @@ from django.test import TestCase
 
 from utilities.data import (
     check_ranges_overlap,
+    deep_compare_dict,
     get_config_value_ci,
     ranges_to_string,
     ranges_to_string_list,
@@ -100,6 +101,64 @@ class RangeFunctionsTestCase(TestCase):
         )
 
 
+class DeepCompareDictTestCase(TestCase):
+
+    def test_no_changes(self):
+        source = {'a': 1, 'b': 'foo', 'c': {'x': 1, 'y': 2}}
+        added, removed = deep_compare_dict(source, source)
+        self.assertEqual(added, {})
+        self.assertEqual(removed, {})
+
+    def test_scalar_change(self):
+        source = {'a': 1, 'b': 'foo'}
+        dest = {'a': 2, 'b': 'foo'}
+        added, removed = deep_compare_dict(source, dest)
+        self.assertEqual(added, {'a': 2})
+        self.assertEqual(removed, {'a': 1})
+
+    def test_key_added(self):
+        source = {'a': 1}
+        dest = {'a': 1, 'b': 'new'}
+        added, removed = deep_compare_dict(source, dest)
+        self.assertEqual(added, {'b': 'new'})
+        self.assertEqual(removed, {'b': None})
+
+    def test_key_removed(self):
+        source = {'a': 1, 'b': 'old'}
+        dest = {'a': 1}
+        added, removed = deep_compare_dict(source, dest)
+        self.assertEqual(added, {'b': None})
+        self.assertEqual(removed, {'b': 'old'})
+
+    def test_nested_dict_partial_change(self):
+        """Only changed sub-keys of a nested dict are included."""
+        source = {'custom_fields': {'cf1': 'old', 'cf2': 'unchanged'}}
+        dest = {'custom_fields': {'cf1': 'new', 'cf2': 'unchanged'}}
+        added, removed = deep_compare_dict(source, dest)
+        self.assertEqual(added, {'custom_fields': {'cf1': 'new'}})
+        self.assertEqual(removed, {'custom_fields': {'cf1': 'old'}})
+
+    def test_nested_dict_no_change(self):
+        source = {'name': 'test', 'custom_fields': {'cf1': 'same'}}
+        added, removed = deep_compare_dict(source, source)
+        self.assertEqual(added, {})
+        self.assertEqual(removed, {})
+
+    def test_mixed_flat_and_nested(self):
+        source = {'name': 'old', 'custom_fields': {'cf1': 'old', 'cf2': 'same'}}
+        dest = {'name': 'new', 'custom_fields': {'cf1': 'new', 'cf2': 'same'}}
+        added, removed = deep_compare_dict(source, dest)
+        self.assertEqual(added, {'name': 'new', 'custom_fields': {'cf1': 'new'}})
+        self.assertEqual(removed, {'name': 'old', 'custom_fields': {'cf1': 'old'}})
+
+    def test_exclude(self):
+        source = {'a': 1, 'last_updated': '2024-01-01'}
+        dest = {'a': 2, 'last_updated': '2024-06-01'}
+        added, removed = deep_compare_dict(source, dest, exclude=['last_updated'])
+        self.assertEqual(added, {'a': 2})
+        self.assertEqual(removed, {'a': 1})
+
+
 class GetConfigValueCITestCase(TestCase):
 
     def test_exact_match(self):