From 2a7fc23b29c6d785599af50e90cafeb8dc510271 Mon Sep 17 00:00:00 2001 From: August Raack <60975983+sum-rock@users.noreply.github.com> Date: Wed, 28 Dec 2022 02:50:35 -0600 Subject: [PATCH] Modify ``change`` field to be a json field. (#407) * Modify ``change`` field to be a json field. Storing the object changes as a json is preferred because it allows SQL queries to access the change values. This work moves the burden of handling json objects from an implementation of python's json library in this package and puts it instead onto the ORM. Ultimately, having the text field store the changes was leaving them less accessible to external systems and code that is written outside the scope of the django auditlog. This change was accomplished by updating the field type on the model and then removing the JSON dumps invocations on write and JSON loads invocations on read. Test were updated to assert equality of dictionaries rather than equality of JSON parsable text. Separately, it was asserted that postgres will make these changes to existing data. Therefore, existing postgres installations should update the type of existing field values without issue. * Add test coverage for messages exceeding char len The "Modify change field to be a json field" commit reduced test coverage on the mixins.py file by 0.03%. The reduction in coverage was the result of reducing the number of operations required to achieve the desired state. An additional test was added to increase previously uncovered code. The net effect is an increase in test case coverage. * Add line to changelog Better markdown formatting Co-authored-by: Hasan Ramezani * Update CHANGELOG text format More specific language in the improvement section regarding `LogEntry.change` Co-authored-by: Hasan Ramezani * Update migration to show Django version 4.0 Co-authored-by: Hasan Ramezani * Update CHANGELOG to show breaking change Running the migration to update the field type of `LogEntry.change` is a breaking change. Co-authored-by: Hasan Ramezani * Update serial order of migrations * Adjust manager method for compatibility The create log method on the LogEntry manager required an additional kwarg for a call to create an instance regardless of a change or not. This felt brittle anyway. The reason it had worked prior to these changes was that the `change` kwarg was sending a string "null" and not a None when there were no changes. Co-authored-by: Hasan Ramezani --- CHANGELOG.md | 4 ++ .../migrations/0015_alter_logentry_changes.py | 18 ++++++++ auditlog/models.py | 13 +++--- auditlog/receivers.py | 4 +- auditlog_tests/tests.py | 44 ++++++++++++++----- 5 files changed, 62 insertions(+), 21 deletions(-) create mode 100644 auditlog/migrations/0015_alter_logentry_changes.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a20307..c63e855 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changes +#### Breaking Changes + +- feat: Change `LogEntry.change` field type to `JSONField` rather than `TextField`. This change include a migration that may take time to run depending on the number of records on your `LogEntry` table ([#407](https://github.com/jazzband/django-auditlog/pull/407)) + ## Next Release #### Improvements diff --git a/auditlog/migrations/0015_alter_logentry_changes.py b/auditlog/migrations/0015_alter_logentry_changes.py new file mode 100644 index 0000000..6743b89 --- /dev/null +++ b/auditlog/migrations/0015_alter_logentry_changes.py @@ -0,0 +1,18 @@ +# Generated by Django 4.0 on 2022-08-04 15:41 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("auditlog", "0014_logentry_cid"), + ] + + operations = [ + migrations.AlterField( + model_name="logentry", + name="changes", + field=models.JSONField(null=True, verbose_name="change message"), + ), + ] diff --git a/auditlog/models.py b/auditlog/models.py index d5b04ff..8f98fce 100644 --- a/auditlog/models.py +++ b/auditlog/models.py @@ -26,13 +26,15 @@ class LogEntryManager(models.Manager): Custom manager for the :py:class:`LogEntry` model. """ - def log_create(self, instance, **kwargs): + def log_create(self, instance, force_log: bool = False, **kwargs): """ Helper method to create a new log entry. This method automatically populates some fields when no explicit value is given. :param instance: The model instance to log a change for. :type instance: Model + :param force_log: Create a LogEntry even if no changes exist. + :type force_log: bool :param kwargs: Field overrides for the :py:class:`LogEntry` object. :return: The new log entry or `None` if there were no changes. :rtype: LogEntry @@ -42,7 +44,7 @@ class LogEntryManager(models.Manager): changes = kwargs.get("changes", None) pk = self._get_pk_value(instance) - if changes is not None: + if changes is not None or force_log: kwargs.setdefault( "content_type", ContentType.objects.get_for_model(instance) ) @@ -350,7 +352,7 @@ class LogEntry(models.Model): action = models.PositiveSmallIntegerField( choices=Action.choices, verbose_name=_("action"), db_index=True ) - changes = models.TextField(blank=True, verbose_name=_("change message")) + changes = models.JSONField(null=True, verbose_name=_("change message")) actor = models.ForeignKey( to=settings.AUTH_USER_MODEL, on_delete=models.SET_NULL, @@ -399,10 +401,7 @@ class LogEntry(models.Model): """ :return: The changes recorded in this log entry as a dictionary object. """ - try: - return json.loads(self.changes) or {} - except ValueError: - return {} + return self.changes or {} @property def changes_str(self, colon=": ", arrow=" \u2192 ", separator="; "): diff --git a/auditlog/receivers.py b/auditlog/receivers.py index 561013e..fefa2cd 100644 --- a/auditlog/receivers.py +++ b/auditlog/receivers.py @@ -1,4 +1,3 @@ -import json from functools import wraps from django.conf import settings @@ -115,7 +114,8 @@ def _create_log_entry( LogEntry.objects.log_create( instance, action=action, - changes=json.dumps(changes), + changes=changes, + force_log=force_log, ) except BaseException as e: error = e diff --git a/auditlog_tests/tests.py b/auditlog_tests/tests.py index c747582..04f3975 100644 --- a/auditlog_tests/tests.py +++ b/auditlog_tests/tests.py @@ -105,9 +105,9 @@ class SimpleModelTest(TestCase): obj.save() def check_update_log_entry(self, obj, history): - self.assertJSONEqual( + self.assertDictEqual( history.changes, - '{"boolean": ["False", "True"]}', + {"boolean": ["False", "True"]}, msg="The change is correctly logged", ) @@ -120,9 +120,9 @@ class SimpleModelTest(TestCase): obj.save(update_fields=["boolean"]) # This implicitly asserts there is only one UPDATE change since the `.get` would fail otherwise. - self.assertJSONEqual( + self.assertDictEqual( obj.history.get(action=LogEntry.Action.UPDATE).changes, - '{"boolean": ["False", "True"]}', + {"boolean": ["False", "True"]}, msg=( "Object modifications that are not saved to DB are not logged " "when using the `update_fields`." @@ -153,9 +153,9 @@ class SimpleModelTest(TestCase): obj.integer = 1 obj.boolean = True obj.save(update_fields=None) - self.assertJSONEqual( + self.assertDictEqual( obj.history.get(action=LogEntry.Action.UPDATE).changes, - '{"boolean": ["False", "True"], "integer": ["None", "1"]}', + {"boolean": ["False", "True"], "integer": ["None", "1"]}, msg="The 2 fields changed are correctly logged", ) @@ -537,9 +537,9 @@ class SimpleIncludeModelTest(TestCase): obj.text = "Newer text" obj.save(update_fields=["text", "label"]) - self.assertJSONEqual( + self.assertDictEqual( obj.history.get(action=LogEntry.Action.UPDATE).changes, - '{"label": ["Initial label", "New label"]}', + {"label": ["Initial label", "New label"]}, msg="Only the label was logged, regardless of multiple entries in `update_fields`", ) @@ -1395,7 +1395,27 @@ class DiffMsgTest(TestCase): return LogEntry.objects.log_create( SimpleModel.objects.create(), # doesn't affect anything action=action, - changes=json.dumps(changes), + changes=changes, + ) + + def test_change_msg_create_when_exceeds_max_len(self): + log_entry = self._create_log_entry( + LogEntry.Action.CREATE, + { + "Camelopardalis": [None, "Giraffe"], + "Capricornus": [None, "Sea goat"], + "Equuleus": [None, "Little horse"], + "Horologium": [None, "Clock"], + "Microscopium": [None, "Microscope"], + "Reticulum": [None, "Net"], + "Telescopium": [None, "Telescope"], + }, + ) + + self.assertEqual( + self.admin.msg_short(log_entry), + "7 changes: Camelopardalis, Capricornus, Equuleus, Horologium, " + "Microscopium, ..", ) def test_changes_msg_delete(self): @@ -1569,9 +1589,9 @@ class JSONModelTest(TestCase): history = obj.history.get(action=LogEntry.Action.UPDATE) - self.assertJSONEqual( + self.assertDictEqual( history.changes, - '{"json": ["{}", "{\'quantity\': \'1\'}"]}', + {"json": ["{}", "{'quantity': '1'}"]}, msg="The change is correctly logged", ) @@ -1910,7 +1930,7 @@ class TestAccessLog(TestCase): self.assertEqual( log_entry.action, LogEntry.Action.ACCESS, msg="Action is 'ACCESS'" ) - self.assertEqual(log_entry.changes, "null") + self.assertIsNone(log_entry.changes) self.assertEqual(log_entry.changes_dict, {})