Initial work on #5913

docs/additional-features/change-logging.md

@@ -1,6 +1,6 @@
 # Change Logging
 
-Every time an object in NetBox is created, updated, or deleted, a serialized copy of that object is saved to the database, along with meta data including the current time and the user associated with the change. These records form a persistent record of changes both for each individual object as well as NetBox as a whole. The global change log can be viewed by navigating to Other > Change Log.
+Every time an object in NetBox is created, updated, or deleted, a serialized copy of that object taken both before and after the change is saved to the database, along with meta data including the current time and the user associated with the change. These records form a persistent record of changes both for each individual object as well as NetBox as a whole. The global change log can be viewed by navigating to Other > Change Log.
 
 A serialized representation of the instance being modified is included in JSON format. This is similar to how objects are conveyed within the REST API, but does not include any nested representations. For instance, the `tenant` field of a site will record only the tenant's ID, not a representation of the tenant.
 

docs/release-notes/version-2.11.md

@@ -16,6 +16,10 @@ In addition to the new `mark_connected` boolean field, the REST API representati
 
 Devices can now be assigned to locations (formerly known as rack groups) within a site without needing to be assigned to a particular rack. This is handy for assigning devices to rooms or floors within a building where racks are not used. The `location` foreign key field has been added to the Device model to support this.
 
+#### Improved Change Logging ([#5913](https://github.com/netbox-community/netbox/issues/5913))
+
+The ObjectChange model (which is used to record the creation, modification, and deletion of NetBox objects) now explicitly records the pre-change and post-change state of each object, rather than only the post-change state. This was done to present a more clear depiction of each change being made, and to prevent the erroneous association of a previous unlogged change with its successor.
+
 ### Enhancements
 
 * [#5370](https://github.com/netbox-community/netbox/issues/5370) - Extend custom field support to organizational models
@@ -54,3 +58,6 @@ Devices can now be assigned to locations (formerly known as rack groups) within
   * Renamed `group` field to `location`
 * extras.CustomField
   * Added new custom field type: `multi-select`
+* extras.ObjectChange
+  * Added the `prechange_data` field
+  * Renamed `object_data` to `postchange_data`

netbox/circuits/migrations/0025_standardize_models.py

@@ -34,4 +34,14 @@ class Migration(migrations.Migration):
             name='id',
             field=models.BigAutoField(primary_key=True, serialize=False),
         ),
+        migrations.AddField(
+            model_name='circuittermination',
+            name='created',
+            field=models.DateField(auto_now_add=True, null=True),
+        ),
+        migrations.AddField(
+            model_name='circuittermination',
+            name='last_updated',
+            field=models.DateTimeField(auto_now=True, null=True),
+        ),
     ]

netbox/circuits/models.py

@@ -6,9 +6,8 @@
 from dcim.models import CableTermination, PathEndpoint
 from extras.models import ObjectChange, TaggedItem
 from extras.utils import extras_features
-from netbox.models import BigIDModel, OrganizationalModel, PrimaryModel
+from netbox.models import BigIDModel, ChangeLoggingMixin, OrganizationalModel, PrimaryModel
 from utilities.querysets import RestrictedQuerySet
-from utilities.utils import serialize_object
 from .choices import *
 from .querysets import CircuitQuerySet
 
@@ -235,7 +234,7 @@ def termination_z(self):
         return self._get_termination('Z')
 
 
-class CircuitTermination(BigIDModel, PathEndpoint, CableTermination):
+class CircuitTermination(ChangeLoggingMixin, BigIDModel, PathEndpoint, CableTermination):
     circuit = models.ForeignKey(
         to='circuits.Circuit',
         on_delete=models.CASCADE,
@@ -289,18 +288,11 @@ def __str__(self):
     def to_objectchange(self, action):
         # Annotate the parent Circuit
         try:
-            related_object = self.circuit
+            circuit = self.circuit
         except Circuit.DoesNotExist:
             # Parent circuit has been deleted
-            related_object = None
-
-        return ObjectChange(
-            changed_object=self,
-            object_repr=str(self),
-            action=action,
-            related_object=related_object,
-            object_data=serialize_object(self)
-        )
+            circuit = None
+        return super().to_objectchange(action, related_object=circuit)
 
     @property
     def parent(self):

netbox/dcim/models/device_component_templates.py

@@ -75,13 +75,7 @@ def to_objectchange(self, action):
         except ObjectDoesNotExist:
             # The parent DeviceType has already been deleted
             device_type = None
-        return ObjectChange(
-            changed_object=self,
-            object_repr=str(self),
-            action=action,
-            related_object=device_type,
-            object_data=serialize_object(self)
-        )
+        return super().to_objectchange(action, related_object=device_type)
 
 
 @extras_features('custom_fields', 'export_templates', 'webhooks')

netbox/dcim/models/device_components.py

@@ -82,13 +82,7 @@ def to_objectchange(self, action):
         except ObjectDoesNotExist:
             # The parent Device has already been deleted
             device = None
-        return ObjectChange(
-            changed_object=self,
-            object_repr=str(self),
-            action=action,
-            related_object=device,
-            object_data=serialize_object(self)
-        )
+        return super().to_objectchange(action, related_object=device)
 
     @property
     def parent(self):

netbox/extras/api/serializers.py

@@ -338,7 +338,7 @@ class Meta:
         model = ObjectChange
         fields = [
             'id', 'url', 'time', 'user', 'user_name', 'request_id', 'action', 'changed_object_type',
-            'changed_object_id', 'changed_object', 'object_data',
+            'changed_object_id', 'changed_object', 'prechange_data', 'postchange_data',
         ]
 
     @swagger_serializer_method(serializer_or_field=serializers.DictField)

netbox/extras/migrations/0055_objectchange_data.py

@@ -0,0 +1,28 @@
+# Generated by Django 3.2b1 on 2021-03-03 20:30
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('extras', '0054_standardize_models'),
+    ]
+
+    operations = [
+        migrations.RenameField(
+            model_name='objectchange',
+            old_name='object_data',
+            new_name='postchange_data',
+        ),
+        migrations.AlterField(
+            model_name='objectchange',
+            name='postchange_data',
+            field=models.JSONField(blank=True, editable=False, null=True),
+        ),
+        migrations.AddField(
+            model_name='objectchange',
+            name='prechange_data',
+            field=models.JSONField(blank=True, editable=False, null=True),
+        ),
+    ]

netbox/extras/models/change_logging.py

@@ -67,15 +67,22 @@ class ObjectChange(BigIDModel):
         max_length=200,
         editable=False
     )
-    object_data = models.JSONField(
-        editable=False
+    prechange_data = models.JSONField(
+        editable=False,
+        blank=True,
+        null=True
+    )
+    postchange_data = models.JSONField(
+        editable=False,
+        blank=True,
+        null=True
     )
 
     objects = RestrictedQuerySet.as_manager()
 
     csv_headers = [
         'time', 'user', 'user_name', 'request_id', 'action', 'changed_object_type', 'changed_object_id',
-        'related_object_type', 'related_object_id', 'object_repr', 'object_data',
+        'related_object_type', 'related_object_id', 'object_repr', 'prechange_data', 'postchange_data',
     ]
 
     class Meta:
@@ -114,7 +121,8 @@ def to_csv(self):
             self.related_object_type,
             self.related_object_id,
             self.object_repr,
-            self.object_data,
+            self.prechange_data,
+            self.postchange_data,
         )
 
     def get_action_class(self):

netbox/extras/signals.py

@@ -36,6 +36,9 @@ def _handle_changed_object(request, sender, instance, **kwargs):
     # Record an ObjectChange if applicable
     if hasattr(instance, 'to_objectchange'):
         objectchange = instance.to_objectchange(action)
+        # TODO: Move this to to_objectchange()
+        if hasattr(instance, '_prechange_snapshot'):
+            objectchange.prechange_data = instance._prechange_snapshot
         objectchange.user = request.user
         objectchange.request_id = request.id
         objectchange.save()
@@ -62,6 +65,9 @@ def _handle_deleted_object(request, sender, instance, **kwargs):
     # Record an ObjectChange if applicable
     if hasattr(instance, 'to_objectchange'):
         objectchange = instance.to_objectchange(ObjectChangeActionChoices.ACTION_DELETE)
+        # TODO: Move this to to_objectchange()
+        if hasattr(instance, '_prechange_snapshot'):
+            objectchange.prechange_data = instance._prechange_snapshot
         objectchange.user = request.user
         objectchange.request_id = request.id
         objectchange.save()

netbox/extras/tests/test_changelog.py

@@ -64,10 +64,11 @@ def test_create_object(self):
         ).order_by('pk')
         self.assertEqual(oc_list[0].changed_object, site)
         self.assertEqual(oc_list[0].action, ObjectChangeActionChoices.ACTION_CREATE)
-        self.assertEqual(oc_list[0].object_data['custom_fields']['my_field'], form_data['cf_my_field'])
-        self.assertEqual(oc_list[0].object_data['custom_fields']['my_field_select'], form_data['cf_my_field_select'])
+        self.assertEqual(oc_list[0].prechange_data, None)
+        self.assertEqual(oc_list[0].postchange_data['custom_fields']['my_field'], form_data['cf_my_field'])
+        self.assertEqual(oc_list[0].postchange_data['custom_fields']['my_field_select'], form_data['cf_my_field_select'])
         self.assertEqual(oc_list[1].action, ObjectChangeActionChoices.ACTION_UPDATE)
-        self.assertEqual(oc_list[1].object_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc_list[1].postchange_data['tags'], ['Tag 1', 'Tag 2'])
 
     def test_update_object(self):
         site = Site(name='Test Site 1', slug='test-site-1')
@@ -100,9 +101,11 @@ def test_update_object(self):
         ).first()
         self.assertEqual(oc.changed_object, site)
         self.assertEqual(oc.action, ObjectChangeActionChoices.ACTION_UPDATE)
-        self.assertEqual(oc.object_data['custom_fields']['my_field'], form_data['cf_my_field'])
-        self.assertEqual(oc.object_data['custom_fields']['my_field_select'], form_data['cf_my_field_select'])
-        self.assertEqual(oc.object_data['tags'], ['Tag 3'])
+        self.assertEqual(oc.prechange_data['name'], 'Test Site 1')
+        self.assertEqual(oc.prechange_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc.postchange_data['custom_fields']['my_field'], form_data['cf_my_field'])
+        self.assertEqual(oc.postchange_data['custom_fields']['my_field_select'], form_data['cf_my_field_select'])
+        self.assertEqual(oc.postchange_data['tags'], ['Tag 3'])
 
     def test_delete_object(self):
         site = Site(
@@ -129,9 +132,12 @@ def test_delete_object(self):
         self.assertEqual(oc.changed_object, None)
         self.assertEqual(oc.object_repr, site.name)
         self.assertEqual(oc.action, ObjectChangeActionChoices.ACTION_DELETE)
-        self.assertEqual(oc.object_data['custom_fields']['my_field'], 'ABC')
-        self.assertEqual(oc.object_data['custom_fields']['my_field_select'], 'Bar')
-        self.assertEqual(oc.object_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc.prechange_data['custom_fields']['my_field'], 'ABC')
+        self.assertEqual(oc.prechange_data['custom_fields']['my_field_select'], 'Bar')
+        self.assertEqual(oc.prechange_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc.postchange_data, None)
+
+    # TODO: Add tests for bulk edit, bulk delete views
 
 
 class ChangeLogAPITest(APITestCase):
@@ -195,9 +201,10 @@ def test_create_object(self):
         ).order_by('pk')
         self.assertEqual(oc_list[0].changed_object, site)
         self.assertEqual(oc_list[0].action, ObjectChangeActionChoices.ACTION_CREATE)
-        self.assertEqual(oc_list[0].object_data['custom_fields'], data['custom_fields'])
+        self.assertEqual(oc_list[0].prechange_data, None)
+        self.assertEqual(oc_list[0].postchange_data['custom_fields'], data['custom_fields'])
         self.assertEqual(oc_list[1].action, ObjectChangeActionChoices.ACTION_UPDATE)
-        self.assertEqual(oc_list[1].object_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc_list[1].postchange_data['tags'], ['Tag 1', 'Tag 2'])
 
     def test_update_object(self):
         site = Site(name='Test Site 1', slug='test-site-1')
@@ -229,8 +236,8 @@ def test_update_object(self):
         ).first()
         self.assertEqual(oc.changed_object, site)
         self.assertEqual(oc.action, ObjectChangeActionChoices.ACTION_UPDATE)
-        self.assertEqual(oc.object_data['custom_fields'], data['custom_fields'])
-        self.assertEqual(oc.object_data['tags'], ['Tag 3'])
+        self.assertEqual(oc.postchange_data['custom_fields'], data['custom_fields'])
+        self.assertEqual(oc.postchange_data['tags'], ['Tag 3'])
 
     def test_delete_object(self):
         site = Site(
@@ -255,6 +262,9 @@ def test_delete_object(self):
         self.assertEqual(oc.changed_object, None)
         self.assertEqual(oc.object_repr, site.name)
         self.assertEqual(oc.action, ObjectChangeActionChoices.ACTION_DELETE)
-        self.assertEqual(oc.object_data['custom_fields']['my_field'], 'ABC')
-        self.assertEqual(oc.object_data['custom_fields']['my_field_select'], 'Bar')
-        self.assertEqual(oc.object_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc.prechange_data['custom_fields']['my_field'], 'ABC')
+        self.assertEqual(oc.prechange_data['custom_fields']['my_field_select'], 'Bar')
+        self.assertEqual(oc.prechange_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc.postchange_data, None)
+
+    # TODO: Add tests for bulk edit, bulk delete views

netbox/extras/tests/test_filters.py

@@ -327,7 +327,7 @@ def setUpTestData(cls):
                 action=ObjectChangeActionChoices.ACTION_CREATE,
                 changed_object=site,
                 object_repr=str(site),
-                object_data={'name': site.name, 'slug': site.slug}
+                postchange_data={'name': site.name, 'slug': site.slug}
             ),
             ObjectChange(
                 user=users[0],
@@ -336,7 +336,7 @@ def setUpTestData(cls):
                 action=ObjectChangeActionChoices.ACTION_UPDATE,
                 changed_object=site,
                 object_repr=str(site),
-                object_data={'name': site.name, 'slug': site.slug}
+                postchange_data={'name': site.name, 'slug': site.slug}
             ),
             ObjectChange(
                 user=users[1],
@@ -345,7 +345,7 @@ def setUpTestData(cls):
                 action=ObjectChangeActionChoices.ACTION_DELETE,
                 changed_object=site,
                 object_repr=str(site),
-                object_data={'name': site.name, 'slug': site.slug}
+                postchange_data={'name': site.name, 'slug': site.slug}
             ),
             ObjectChange(
                 user=users[1],
@@ -354,7 +354,7 @@ def setUpTestData(cls):
                 action=ObjectChangeActionChoices.ACTION_CREATE,
                 changed_object=ipaddress,
                 object_repr=str(ipaddress),
-                object_data={'address': ipaddress.address, 'status': ipaddress.status}
+                postchange_data={'address': ipaddress.address, 'status': ipaddress.status}
             ),
             ObjectChange(
                 user=users[2],
@@ -363,7 +363,7 @@ def setUpTestData(cls):
                 action=ObjectChangeActionChoices.ACTION_UPDATE,
                 changed_object=ipaddress,
                 object_repr=str(ipaddress),
-                object_data={'address': ipaddress.address, 'status': ipaddress.status}
+                postchange_data={'address': ipaddress.address, 'status': ipaddress.status}
             ),
             ObjectChange(
                 user=users[2],
@@ -372,7 +372,7 @@ def setUpTestData(cls):
                 action=ObjectChangeActionChoices.ACTION_DELETE,
                 changed_object=ipaddress,
                 object_repr=str(ipaddress),
-                object_data={'address': ipaddress.address, 'status': ipaddress.status}
+                postchange_data={'address': ipaddress.address, 'status': ipaddress.status}
             ),
         )
         ObjectChange.objects.bulk_create(object_changes)

netbox/extras/views.py

@@ -178,16 +178,18 @@ def get_extra_context(self, request, instance):
         next_change = objectchanges.filter(time__gt=instance.time).order_by('time').first()
         prev_change = objectchanges.filter(time__lt=instance.time).order_by('-time').first()
 
-        if prev_change:
+        if instance.prechange_data and instance.postchange_data:
             diff_added = shallow_compare_dict(
-                prev_change.object_data,
-                instance.object_data,
+                instance.prechange_data or dict(),
+                instance.postchange_data or dict(),
                 exclude=['last_updated'],
             )
-            diff_removed = {x: prev_change.object_data.get(x) for x in diff_added}
+            diff_removed = {
+                x: instance.prechange_data.get(x) for x in diff_added
+            } if instance.prechange_data else {}
         else:
-            # No previous change; this is the initial change that added the object
-            diff_added = diff_removed = instance.object_data
+            diff_added = None
+            diff_removed = None
 
         return {
             'diff_added': diff_added,

netbox/ipam/models/ip.py

@@ -649,13 +649,7 @@ def save(self, *args, **kwargs):
 
     def to_objectchange(self, action):
         # Annotate the assigned object, if any
-        return ObjectChange(
-            changed_object=self,
-            object_repr=str(self),
-            action=action,
-            related_object=self.assigned_object,
-            object_data=serialize_object(self)
-        )
+        return super().to_objectchange(action, related_object=self.assigned_object)
 
     def to_csv(self):