diff --git a/docs/additional-features/change-logging.md b/docs/additional-features/change-logging.md index d580ccc6c..3eb99c94c 100644 --- a/docs/additional-features/change-logging.md +++ b/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. diff --git a/docs/release-notes/version-2.11.md b/docs/release-notes/version-2.11.md index 7c515cae3..b92dcaa13 100644 --- a/docs/release-notes/version-2.11.md +++ b/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` diff --git a/netbox/circuits/migrations/0025_standardize_models.py b/netbox/circuits/migrations/0025_standardize_models.py index 2b1d2664e..42745f35b 100644 --- a/netbox/circuits/migrations/0025_standardize_models.py +++ b/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), + ), ] diff --git a/netbox/circuits/models.py b/netbox/circuits/models.py index 693fe5ae6..26104d4d6 100644 --- a/netbox/circuits/models.py +++ b/netbox/circuits/models.py @@ -6,9 +6,8 @@ from dcim.fields import ASNField 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 @@ class Circuit(PrimaryModel): 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 @@ class CircuitTermination(BigIDModel, PathEndpoint, CableTermination): 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): diff --git a/netbox/dcim/models/device_component_templates.py b/netbox/dcim/models/device_component_templates.py index 51956f924..99de34466 100644 --- a/netbox/dcim/models/device_component_templates.py +++ b/netbox/dcim/models/device_component_templates.py @@ -75,13 +75,7 @@ class ComponentTemplateModel(ChangeLoggingMixin, BigIDModel): 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') diff --git a/netbox/dcim/models/device_components.py b/netbox/dcim/models/device_components.py index 98c04f1c4..b5b69a5f2 100644 --- a/netbox/dcim/models/device_components.py +++ b/netbox/dcim/models/device_components.py @@ -82,13 +82,7 @@ class ComponentModel(PrimaryModel): 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): diff --git a/netbox/extras/api/serializers.py b/netbox/extras/api/serializers.py index a85ca05b7..1d956bb91 100644 --- a/netbox/extras/api/serializers.py +++ b/netbox/extras/api/serializers.py @@ -338,7 +338,7 @@ class ObjectChangeSerializer(serializers.ModelSerializer): 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) diff --git a/netbox/extras/migrations/0055_objectchange_data.py b/netbox/extras/migrations/0055_objectchange_data.py new file mode 100644 index 000000000..4dc33fc1c --- /dev/null +++ b/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), + ), + ] diff --git a/netbox/extras/models/change_logging.py b/netbox/extras/models/change_logging.py index a06043e7b..fac86b641 100644 --- a/netbox/extras/models/change_logging.py +++ b/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 @@ class ObjectChange(BigIDModel): 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): diff --git a/netbox/extras/signals.py b/netbox/extras/signals.py index 0d6295e5b..ba7c725bf 100644 --- a/netbox/extras/signals.py +++ b/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() diff --git a/netbox/extras/tests/test_changelog.py b/netbox/extras/tests/test_changelog.py index c0732649b..5e44c83d1 100644 --- a/netbox/extras/tests/test_changelog.py +++ b/netbox/extras/tests/test_changelog.py @@ -40,8 +40,8 @@ class ChangeLogViewTest(ModelViewTestCase): def test_create_object(self): tags = self.create_tags('Tag 1', 'Tag 2') form_data = { - 'name': 'Test Site 1', - 'slug': 'test-site-1', + 'name': 'Site 1', + 'slug': 'site-1', 'status': SiteStatusChoices.STATUS_ACTIVE, 'cf_my_field': 'ABC', 'cf_my_field_select': 'Bar', @@ -56,7 +56,7 @@ class ChangeLogViewTest(ModelViewTestCase): response = self.client.post(**request) self.assertHttpStatus(response, 302) - site = Site.objects.get(name='Test Site 1') + site = Site.objects.get(name='Site 1') # First OC is the creation; second is the tags update oc_list = ObjectChange.objects.filter( changed_object_type=ContentType.objects.get_for_model(Site), @@ -64,20 +64,21 @@ class ChangeLogViewTest(ModelViewTestCase): ).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') + site = Site(name='Site 1', slug='site-1') site.save() tags = self.create_tags('Tag 1', 'Tag 2', 'Tag 3') site.tags.set('Tag 1', 'Tag 2') form_data = { - 'name': 'Test Site X', - 'slug': 'test-site-x', + 'name': 'Site X', + 'slug': 'site-x', 'status': SiteStatusChoices.STATUS_PLANNED, 'cf_my_field': 'DEF', 'cf_my_field_select': 'Foo', @@ -100,14 +101,16 @@ class ChangeLogViewTest(ModelViewTestCase): ).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'], '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( - name='Test Site 1', - slug='test-site-1', + name='Site 1', + slug='site-1', custom_field_data={ 'my_field': 'ABC', 'my_field_select': 'Bar' @@ -129,15 +132,83 @@ class ChangeLogViewTest(ModelViewTestCase): 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) + + def test_bulk_update_objects(self): + sites = ( + Site(name='Site 1', slug='site-1', status=SiteStatusChoices.STATUS_ACTIVE), + Site(name='Site 2', slug='site-2', status=SiteStatusChoices.STATUS_ACTIVE), + Site(name='Site 3', slug='site-3', status=SiteStatusChoices.STATUS_ACTIVE), + ) + Site.objects.bulk_create(sites) + + form_data = { + 'pk': [site.pk for site in sites], + '_apply': True, + 'status': SiteStatusChoices.STATUS_PLANNED, + 'description': 'New description', + } + + request = { + 'path': self._get_url('bulk_edit'), + 'data': post_data(form_data), + } + self.add_permissions('dcim.view_site', 'dcim.change_site') + response = self.client.post(**request) + self.assertHttpStatus(response, 302) + + objectchange = ObjectChange.objects.get( + changed_object_type=ContentType.objects.get_for_model(Site), + changed_object_id=sites[0].pk + ) + self.assertEqual(objectchange.changed_object, sites[0]) + self.assertEqual(objectchange.action, ObjectChangeActionChoices.ACTION_UPDATE) + self.assertEqual(objectchange.prechange_data['status'], SiteStatusChoices.STATUS_ACTIVE) + self.assertEqual(objectchange.prechange_data['description'], '') + self.assertEqual(objectchange.postchange_data['status'], form_data['status']) + self.assertEqual(objectchange.postchange_data['description'], form_data['description']) + + def test_bulk_delete_objects(self): + sites = ( + Site(name='Site 1', slug='site-1', status=SiteStatusChoices.STATUS_ACTIVE), + Site(name='Site 2', slug='site-2', status=SiteStatusChoices.STATUS_ACTIVE), + Site(name='Site 3', slug='site-3', status=SiteStatusChoices.STATUS_ACTIVE), + ) + Site.objects.bulk_create(sites) + + form_data = { + 'pk': [site.pk for site in sites], + 'confirm': True, + '_confirm': True, + } + + request = { + 'path': self._get_url('bulk_delete'), + 'data': post_data(form_data), + } + self.add_permissions('dcim.delete_site') + response = self.client.post(**request) + self.assertHttpStatus(response, 302) + + objectchange = ObjectChange.objects.get( + changed_object_type=ContentType.objects.get_for_model(Site), + changed_object_id=sites[0].pk + ) + self.assertEqual(objectchange.changed_object_type, ContentType.objects.get_for_model(Site)) + self.assertEqual(objectchange.changed_object_id, sites[0].pk) + self.assertEqual(objectchange.action, ObjectChangeActionChoices.ACTION_DELETE) + self.assertEqual(objectchange.prechange_data['name'], sites[0].name) + self.assertEqual(objectchange.prechange_data['slug'], sites[0].slug) + self.assertEqual(objectchange.postchange_data, None) class ChangeLogAPITest(APITestCase): - def setUp(self): - super().setUp() + @classmethod + def setUpTestData(cls): # Create a custom field on the Site model ct = ContentType.objects.get_for_model(Site) @@ -169,8 +240,8 @@ class ChangeLogAPITest(APITestCase): def test_create_object(self): data = { - 'name': 'Test Site 1', - 'slug': 'test-site-1', + 'name': 'Site 1', + 'slug': 'site-1', 'custom_fields': { 'my_field': 'ABC', 'my_field_select': 'Bar', @@ -195,17 +266,18 @@ class ChangeLogAPITest(APITestCase): ).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') + site = Site(name='Site 1', slug='site-1') site.save() data = { - 'name': 'Test Site X', - 'slug': 'test-site-x', + 'name': 'Site X', + 'slug': 'site-x', 'custom_fields': { 'my_field': 'DEF', 'my_field_select': 'Foo', @@ -229,13 +301,13 @@ class ChangeLogAPITest(APITestCase): ).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( - name='Test Site 1', - slug='test-site-1', + name='Site 1', + slug='site-1', custom_field_data={ 'my_field': 'ABC', 'my_field_select': 'Bar' @@ -255,6 +327,123 @@ class ChangeLogAPITest(APITestCase): 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) + + def test_bulk_create_objects(self): + data = ( + { + 'name': 'Site 1', + 'slug': 'site-1', + }, + { + 'name': 'Site 2', + 'slug': 'site-2', + }, + { + 'name': 'Site 3', + 'slug': 'site-3', + }, + ) + self.assertEqual(ObjectChange.objects.count(), 0) + url = reverse('dcim-api:site-list') + self.add_permissions('dcim.add_site') + + response = self.client.post(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_201_CREATED) + self.assertEqual(ObjectChange.objects.count(), 3) + + site1 = Site.objects.get(pk=response.data[0]['id']) + objectchange = ObjectChange.objects.get( + changed_object_type=ContentType.objects.get_for_model(Site), + changed_object_id=site1.pk + ) + self.assertEqual(objectchange.changed_object, site1) + self.assertEqual(objectchange.action, ObjectChangeActionChoices.ACTION_CREATE) + self.assertEqual(objectchange.prechange_data, None) + self.assertEqual(objectchange.postchange_data['name'], data[0]['name']) + self.assertEqual(objectchange.postchange_data['slug'], data[0]['slug']) + + def test_bulk_edit_objects(self): + sites = ( + Site(name='Site 1', slug='site-1'), + Site(name='Site 2', slug='site-2'), + Site(name='Site 3', slug='site-3'), + ) + Site.objects.bulk_create(sites) + + data = ( + { + 'id': sites[0].pk, + 'name': 'Site A', + 'slug': 'site-A', + }, + { + 'id': sites[1].pk, + 'name': 'Site B', + 'slug': 'site-b', + }, + { + 'id': sites[2].pk, + 'name': 'Site C', + 'slug': 'site-c', + }, + ) + self.assertEqual(ObjectChange.objects.count(), 0) + url = reverse('dcim-api:site-list') + self.add_permissions('dcim.change_site') + + response = self.client.patch(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + self.assertEqual(ObjectChange.objects.count(), 3) + + objectchange = ObjectChange.objects.get( + changed_object_type=ContentType.objects.get_for_model(Site), + changed_object_id=sites[0].pk + ) + self.assertEqual(objectchange.changed_object, sites[0]) + self.assertEqual(objectchange.action, ObjectChangeActionChoices.ACTION_UPDATE) + self.assertEqual(objectchange.prechange_data['name'], 'Site 1') + self.assertEqual(objectchange.prechange_data['slug'], 'site-1') + self.assertEqual(objectchange.postchange_data['name'], data[0]['name']) + self.assertEqual(objectchange.postchange_data['slug'], data[0]['slug']) + + def test_bulk_delete_objects(self): + sites = ( + Site(name='Site 1', slug='site-1'), + Site(name='Site 2', slug='site-2'), + Site(name='Site 3', slug='site-3'), + ) + Site.objects.bulk_create(sites) + + data = ( + { + 'id': sites[0].pk, + }, + { + 'id': sites[1].pk, + }, + { + 'id': sites[2].pk, + }, + ) + self.assertEqual(ObjectChange.objects.count(), 0) + url = reverse('dcim-api:site-list') + self.add_permissions('dcim.delete_site') + + response = self.client.delete(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_204_NO_CONTENT) + self.assertEqual(ObjectChange.objects.count(), 3) + + objectchange = ObjectChange.objects.get( + changed_object_type=ContentType.objects.get_for_model(Site), + changed_object_id=sites[0].pk + ) + self.assertEqual(objectchange.changed_object_type, ContentType.objects.get_for_model(Site)) + self.assertEqual(objectchange.changed_object_id, sites[0].pk) + self.assertEqual(objectchange.action, ObjectChangeActionChoices.ACTION_DELETE) + self.assertEqual(objectchange.prechange_data['name'], 'Site 1') + self.assertEqual(objectchange.prechange_data['slug'], 'site-1') + self.assertEqual(objectchange.postchange_data, None) diff --git a/netbox/extras/tests/test_filters.py b/netbox/extras/tests/test_filters.py index 7bcd83e81..19278ba75 100644 --- a/netbox/extras/tests/test_filters.py +++ b/netbox/extras/tests/test_filters.py @@ -327,7 +327,7 @@ class ObjectChangeTestCase(TestCase): 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 @@ class ObjectChangeTestCase(TestCase): 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 @@ class ObjectChangeTestCase(TestCase): 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 @@ class ObjectChangeTestCase(TestCase): 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 @@ class ObjectChangeTestCase(TestCase): 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 @@ class ObjectChangeTestCase(TestCase): 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) diff --git a/netbox/extras/views.py b/netbox/extras/views.py index a6315aa51..48dcb81fd 100644 --- a/netbox/extras/views.py +++ b/netbox/extras/views.py @@ -178,16 +178,18 @@ class ObjectChangeView(generic.ObjectView): 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, diff --git a/netbox/ipam/models/ip.py b/netbox/ipam/models/ip.py index d8d118a99..b37b67c79 100644 --- a/netbox/ipam/models/ip.py +++ b/netbox/ipam/models/ip.py @@ -649,13 +649,7 @@ class IPAddress(PrimaryModel): 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): diff --git a/netbox/netbox/api/views.py b/netbox/netbox/api/views.py index 991a8892d..585b75686 100644 --- a/netbox/netbox/api/views.py +++ b/netbox/netbox/api/views.py @@ -76,6 +76,8 @@ class BulkUpdateModelMixin: data_list = [] for obj in objects: data = update_data.get(obj.id) + if hasattr(obj, 'snapshot'): + obj.snapshot() serializer = self.get_serializer(obj, data=data, partial=partial) serializer.is_valid(raise_exception=True) self.perform_update(serializer) @@ -113,6 +115,8 @@ class BulkDestroyModelMixin: def perform_bulk_destroy(self, objects): with transaction.atomic(): for obj in objects: + if hasattr(obj, 'snapshot'): + obj.snapshot() self.perform_destroy(obj) @@ -127,6 +131,16 @@ class ModelViewSet(BulkUpdateModelMixin, BulkDestroyModelMixin, ModelViewSet_): brief = False brief_prefetch_fields = [] + def get_object_with_snapshot(self): + """ + Save a pre-change snapshot of the object immediately after retrieving it. This snapshot will be used to + record the "before" data in the changelog. + """ + obj = super().get_object() + if hasattr(obj, 'snapshot'): + obj.snapshot() + return obj + def get_serializer(self, *args, **kwargs): # If a list of objects has been provided, initialize the serializer with many=True @@ -221,6 +235,11 @@ class ModelViewSet(BulkUpdateModelMixin, BulkDestroyModelMixin, ModelViewSet_): except ObjectDoesNotExist: raise PermissionDenied() + def update(self, request, *args, **kwargs): + # Hotwire get_object() to ensure we save a pre-change snapshot + self.get_object = self.get_object_with_snapshot + return super().update(request, *args, **kwargs) + def perform_update(self, serializer): model = self.queryset.model logger = logging.getLogger('netbox.api.views.ModelViewSet') @@ -234,6 +253,11 @@ class ModelViewSet(BulkUpdateModelMixin, BulkDestroyModelMixin, ModelViewSet_): except ObjectDoesNotExist: raise PermissionDenied() + def destroy(self, request, *args, **kwargs): + # Hotwire get_object() to ensure we save a pre-change snapshot + self.get_object = self.get_object_with_snapshot + return super().destroy(request, *args, **kwargs) + def perform_destroy(self, instance): model = self.queryset.model logger = logging.getLogger('netbox.api.views.ModelViewSet') diff --git a/netbox/netbox/models.py b/netbox/netbox/models.py index 63965d4e6..acaa90656 100644 --- a/netbox/netbox/models.py +++ b/netbox/netbox/models.py @@ -1,3 +1,4 @@ +import logging from collections import OrderedDict from django.core.serializers.json import DjangoJSONEncoder @@ -5,6 +6,7 @@ from django.core.validators import ValidationError from django.db import models from mptt.models import MPTTModel, TreeForeignKey +from extras.choices import ObjectChangeActionChoices from utilities.mptt import TreeManager from utilities.utils import serialize_object @@ -40,18 +42,32 @@ class ChangeLoggingMixin(models.Model): class Meta: abstract = True - def to_objectchange(self, action): + def snapshot(self): + """ + Save a snapshot of the object's current state in preparation for modification. + """ + logger = logging.getLogger('netbox') + logger.debug(f"Taking a snapshot of {self}") + self._prechange_snapshot = serialize_object(self) + + def to_objectchange(self, action, related_object=None): """ Return a new ObjectChange representing a change made to this object. This will typically be called automatically by ChangeLoggingMiddleware. """ from extras.models import ObjectChange - return ObjectChange( + objectchange = ObjectChange( changed_object=self, + related_object=related_object, object_repr=str(self), - action=action, - object_data=serialize_object(self) + action=action ) + if hasattr(self, '_prechange_snapshot'): + objectchange.prechange_data = self._prechange_snapshot + if action in (ObjectChangeActionChoices.ACTION_CREATE, ObjectChangeActionChoices.ACTION_UPDATE): + objectchange.postchange_data = serialize_object(self) + + return objectchange class CustomFieldsMixin(models.Model): @@ -164,16 +180,6 @@ class NestedGroupModel(ChangeLoggingMixin, CustomFieldsMixin, BigIDModel, MPTTMo def __str__(self): return self.name - def to_objectchange(self, action): - # Remove MPTT-internal fields - from extras.models import ObjectChange - return ObjectChange( - changed_object=self, - object_repr=str(self), - action=action, - object_data=serialize_object(self, exclude=['level', 'lft', 'rght', 'tree_id']) - ) - class OrganizationalModel(ChangeLoggingMixin, CustomFieldsMixin, BigIDModel): """ diff --git a/netbox/netbox/views/generic.py b/netbox/netbox/views/generic.py index 216b50759..af09dd546 100644 --- a/netbox/netbox/views/generic.py +++ b/netbox/netbox/views/generic.py @@ -218,11 +218,18 @@ class ObjectEditView(GetReturnURLMixin, ObjectPermissionRequiredMixin, View): def get_object(self, kwargs): # Look up an existing object by slug or PK, if provided. if 'slug' in kwargs: - return get_object_or_404(self.queryset, slug=kwargs['slug']) + obj = get_object_or_404(self.queryset, slug=kwargs['slug']) elif 'pk' in kwargs: - return get_object_or_404(self.queryset, pk=kwargs['pk']) + obj = get_object_or_404(self.queryset, pk=kwargs['pk']) # Otherwise, return a new instance. - return self.queryset.model() + else: + return self.queryset.model() + + # Take a snapshot of change-logged models + if hasattr(obj, 'snapshot'): + obj.snapshot() + + return obj def alter_obj(self, obj, request, url_args, url_kwargs): # Allow views to add extra info to an object before it is processed. For example, a parent object can be defined @@ -328,9 +335,15 @@ class ObjectDeleteView(GetReturnURLMixin, ObjectPermissionRequiredMixin, View): def get_object(self, kwargs): # Look up object by slug if one has been provided. Otherwise, use PK. if 'slug' in kwargs: - return get_object_or_404(self.queryset, slug=kwargs['slug']) + obj = get_object_or_404(self.queryset, slug=kwargs['slug']) else: - return get_object_or_404(self.queryset, pk=kwargs['pk']) + obj = get_object_or_404(self.queryset, pk=kwargs['pk']) + + # Take a snapshot of change-logged models + if hasattr(obj, 'snapshot'): + obj.snapshot() + + return obj def get(self, request, **kwargs): obj = self.get_object(kwargs) @@ -771,6 +784,10 @@ class BulkEditView(GetReturnURLMixin, ObjectPermissionRequiredMixin, View): updated_objects = [] for obj in self.queryset.filter(pk__in=form.cleaned_data['pk']): + # Take a snapshot of change-logged models + if hasattr(obj, 'snapshot'): + obj.snapshot() + # Update standard fields. If a field is listed in _nullify, delete its value. for name in standard_fields: @@ -898,6 +915,11 @@ class BulkRenameView(GetReturnURLMixin, ObjectPermissionRequiredMixin, View): with transaction.atomic(): renamed_pks = [] for obj in selected_objects: + + # Take a snapshot of change-logged models + if hasattr(obj, 'snapshot'): + obj.snapshot() + find = form.cleaned_data['find'] replace = form.cleaned_data['replace'] if form.cleaned_data['use_regex']: @@ -986,14 +1008,19 @@ class BulkDeleteView(GetReturnURLMixin, ObjectPermissionRequiredMixin, View): # Delete objects queryset = self.queryset.filter(pk__in=pk_list) + deleted_count = queryset.count() try: - deleted_count = queryset.delete()[1][model._meta.label] + for obj in queryset: + # Take a snapshot of change-logged models + if hasattr(obj, 'snapshot'): + obj.snapshot() + obj.delete() except ProtectedError as e: logger.info("Caught ProtectedError while attempting to delete objects") handle_protectederror(queryset, request, e) return redirect(self.get_return_url(request)) - msg = 'Deleted {} {}'.format(deleted_count, model._meta.verbose_name_plural) + msg = f"Deleted {deleted_count} {model._meta.verbose_name_plural}" logger.info(msg) messages.success(request, msg) return redirect(self.get_return_url(request)) diff --git a/netbox/templates/extras/objectchange.html b/netbox/templates/extras/objectchange.html index 15265889e..9dcd8c2ac 100644 --- a/netbox/templates/extras/objectchange.html +++ b/netbox/templates/extras/objectchange.html @@ -83,6 +83,8 @@ + +
Difference @@ -113,13 +115,39 @@
-
+
+
+
- Object Data + Pre-Change Data
-
{{ object.object_data|render_json }}
+ {% if object.prechange_data %} +
{% for k, v in object.prechange_data.items %}{% spaceless %}
+                      {{ k }}: {{ v|render_json }}
+                    {% endspaceless %}
+{% endfor %}
+ {% else %} + None + {% endif %} +
+
+
+
+
+
+ Post-Change Data +
+
+ {% if object.postchange_data %} +
{% for k, v in object.postchange_data.items %}{% spaceless %}
+                        {{ k }}: {{ v|render_json }}
+                      {% endspaceless %}
+{% endfor %}
+ {% else %} + None + {% endif %}
diff --git a/netbox/utilities/utils.py b/netbox/utilities/utils.py index d76b469b2..f0340f094 100644 --- a/netbox/utilities/utils.py +++ b/netbox/utilities/utils.py @@ -7,6 +7,7 @@ from django.core.serializers import serialize from django.db.models import Count, OuterRef, Subquery from django.db.models.functions import Coalesce from jinja2 import Environment +from mptt.models import MPTTModel from dcim.choices import CableLengthUnitChoices from extras.utils import is_taggable @@ -83,7 +84,7 @@ def count_related(model, field): return Coalesce(subquery, 0) -def serialize_object(obj, extra=None, exclude=None): +def serialize_object(obj, extra=None): """ Return a generic JSON representation of an object using Django's built-in serializer. (This is used for things like change logging, not the REST API.) Optionally include a dictionary to supplement the object data. A list of keys @@ -93,6 +94,11 @@ def serialize_object(obj, extra=None, exclude=None): json_str = serialize('json', [obj]) data = json.loads(json_str)[0]['fields'] + # Exclude any MPTTModel fields + if issubclass(obj.__class__, MPTTModel): + for field in ['level', 'lft', 'rght', 'tree_id']: + data.pop(field) + # Include custom_field_data as "custom_fields" if hasattr(obj, 'custom_field_data'): data['custom_fields'] = data.pop('custom_field_data') @@ -112,10 +118,6 @@ def serialize_object(obj, extra=None, exclude=None): if isinstance(key, str) and key.startswith('_'): data.pop(key) - # Explicitly excluded keys - if isinstance(exclude, (list, tuple)) and key in exclude: - data.pop(key) - return data diff --git a/netbox/virtualization/migrations/0020_standardize_models.py b/netbox/virtualization/migrations/0020_standardize_models.py index 8ccb3df0d..42c44e83a 100644 --- a/netbox/virtualization/migrations/0020_standardize_models.py +++ b/netbox/virtualization/migrations/0020_standardize_models.py @@ -44,4 +44,14 @@ class Migration(migrations.Migration): name='id', field=models.BigAutoField(primary_key=True, serialize=False), ), + migrations.AddField( + model_name='vminterface', + name='created', + field=models.DateField(auto_now_add=True, null=True), + ), + migrations.AddField( + model_name='vminterface', + name='last_updated', + field=models.DateTimeField(auto_now=True, null=True), + ), ] diff --git a/netbox/virtualization/models.py b/netbox/virtualization/models.py index 364dde30b..574400359 100644 --- a/netbox/virtualization/models.py +++ b/netbox/virtualization/models.py @@ -9,12 +9,11 @@ from dcim.models import BaseInterface, Device from extras.models import ConfigContextModel, ObjectChange, TaggedItem from extras.querysets import ConfigContextModelQuerySet from extras.utils import extras_features -from netbox.models import BigIDModel, OrganizationalModel, PrimaryModel +from netbox.models import BigIDModel, ChangeLoggingMixin, OrganizationalModel, PrimaryModel from utilities.fields import NaturalOrderingField from utilities.ordering import naturalize_interface from utilities.query_functions import CollateAsChar from utilities.querysets import RestrictedQuerySet -from utilities.utils import serialize_object from .choices import * @@ -373,8 +372,9 @@ class VirtualMachine(PrimaryModel, ConfigContextModel): # Interfaces # +# TODO: Inherit from PrimaryModel @extras_features('export_templates', 'webhooks') -class VMInterface(BigIDModel, BaseInterface): +class VMInterface(ChangeLoggingMixin, BigIDModel, BaseInterface): virtual_machine = models.ForeignKey( to='virtualization.VirtualMachine', on_delete=models.CASCADE, @@ -458,13 +458,7 @@ class VMInterface(BigIDModel, BaseInterface): def to_objectchange(self, action): # Annotate the parent VirtualMachine - return ObjectChange( - changed_object=self, - object_repr=str(self), - action=action, - related_object=self.virtual_machine, - object_data=serialize_object(self) - ) + return super().to_objectchange(action, related_object=self.virtual_machine) @property def parent(self):