mirror of
https://github.com/jazzband/django-auditlog.git
synced 2026-03-16 22:20:26 +00:00
Change diff to evaluate PKs for FK relationships (#420)
* Change diff to evaluate PKs for FK relationships The diff method now evaluates the primary keys for changes to determine if a new LogEntry should be created. Previously, the diff method was evaluating the string representation of the object. This was flawed because cases can occur when a parent object has in memory changes to its string string representation and the child related object is saved prior to these in memory changes being persisted. In these cases a new LogEntry object would be created erroneously. This cases is asserted with a test and a regression test will verify the bug. The consequence of these updates is that the ``LogEntry.changes`` field now stores primary keys rather than string representations for related objects. To keep the changes dictionary display unaffected by this update, a method was added to the ``LogEntry`` model. This method looks up the object display string from the stored foreign key. Exceptions were written to handle backwards compatibility. * Added test case to cover another bug Because the string representation is not unique for every object, relying on it to determine FK diffs may not capture all changes. This test case shows another type of scenario that is fixed by comparing primary keys rather than object string representations. This is likely occurring fairly regularly but is hard to spot because it is an error of omission. * Update to docstring and added changelog
This commit is contained in:
August Raack
GitHub
parent
2a7fc23b29
commit
c649629225
5 changed files with 178 additions and 2 deletions
|
|
@ -36,6 +36,7 @@
|
|||
|
||||
#### Fixes
|
||||
|
||||
- fix: Foreign key values are used to check for changes in related fields instead of object representations. When changes are detected, the foreign key value is persisted in `LogEntry.changes` field instead of object representations. ([#420](https://github.com/jazzband/django-auditlog/pull/420))
|
||||
- fix: Display `created` timestamp in server timezone ([#404](https://github.com/jazzband/django-auditlog/pull/404))
|
||||
- fix: Handle port in `remote_addr` ([#417](https://github.com/jazzband/django-auditlog/pull/417))
|
||||
- fix: Handle the error with AttributeError: 'OneToOneRel' error occur during a `PolymorphicModel` has relation with other models ([#429](https://github.com/jazzband/django-auditlog/pull/429))
|
||||
|
|
|
|||
|
|
@ -74,6 +74,10 @@ def get_field_value(obj, field):
|
|||
value = django_timezone.make_naive(value, timezone=timezone.utc)
|
||||
elif isinstance(field, JSONField):
|
||||
value = field.to_python(getattr(obj, field.name, None))
|
||||
elif (field.one_to_one or field.many_to_one) and hasattr(field, "rel_class"):
|
||||
value = smart_str(
|
||||
getattr(obj, field.get_attname(), None), strings_only=True
|
||||
)
|
||||
else:
|
||||
value = smart_str(getattr(obj, field.name, None))
|
||||
except ObjectDoesNotExist:
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ import ast
|
|||
import json
|
||||
from copy import deepcopy
|
||||
from datetime import timezone
|
||||
from typing import Any, Dict, List
|
||||
from typing import Any, Dict, List, Union
|
||||
|
||||
from dateutil import parser
|
||||
from dateutil.tz import gettz
|
||||
|
|
@ -10,7 +10,11 @@ from django.conf import settings
|
|||
from django.contrib.contenttypes.fields import GenericRelation
|
||||
from django.contrib.contenttypes.models import ContentType
|
||||
from django.core import serializers
|
||||
from django.core.exceptions import FieldDoesNotExist
|
||||
from django.core.exceptions import (
|
||||
FieldDoesNotExist,
|
||||
ObjectDoesNotExist,
|
||||
ValidationError,
|
||||
)
|
||||
from django.db import DEFAULT_DB_ALIAS, models
|
||||
from django.db.models import Q, QuerySet
|
||||
from django.utils import formats
|
||||
|
|
@ -491,6 +495,9 @@ class LogEntry(models.Model):
|
|||
value = formats.localize(value)
|
||||
except ValueError:
|
||||
pass
|
||||
elif field_type in ["ForeignKey", "OneToOneField"]:
|
||||
value = self._get_changes_display_for_fk_field(field, value)
|
||||
|
||||
# check if length is longer than 140 and truncate with ellipsis
|
||||
if len(value) > 140:
|
||||
value = f"{value[:140]}..."
|
||||
|
|
@ -502,6 +509,31 @@ class LogEntry(models.Model):
|
|||
changes_display_dict[verbose_name] = values_display
|
||||
return changes_display_dict
|
||||
|
||||
def _get_changes_display_for_fk_field(
|
||||
self, field: Union[models.ForeignKey, models.OneToOneField], value: Any
|
||||
) -> str:
|
||||
"""
|
||||
:return: A string representing a given FK value and the field to which it belongs
|
||||
"""
|
||||
# Return "None" if the FK value is "None".
|
||||
if value == "None":
|
||||
return value
|
||||
|
||||
# Attempt to convert given value to the PK type for the related model
|
||||
try:
|
||||
pk_value = field.related_model._meta.pk.to_python(value)
|
||||
# ValidationError will handle legacy values where string representations were
|
||||
# stored rather than PKs. This will also handle cases where the PK type is
|
||||
# changed between the time the LogEntry is created and this method is called.
|
||||
except ValidationError:
|
||||
return value
|
||||
# Attempt to return the string representation of the object
|
||||
try:
|
||||
return smart_str(field.related_model.objects.get(pk=pk_value))
|
||||
# ObjectDoesNotExist will be raised if the object was deleted.
|
||||
except ObjectDoesNotExist:
|
||||
return f"Deleted '{field.related_model.__name__}' ({value})"
|
||||
|
||||
|
||||
class AuditlogHistoryField(GenericRelation):
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -22,6 +22,9 @@ class SimpleModel(models.Model):
|
|||
|
||||
history = AuditlogHistoryField()
|
||||
|
||||
def __str__(self):
|
||||
return self.text
|
||||
|
||||
|
||||
class AltPrimaryKeyModel(models.Model):
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -1684,6 +1684,142 @@ class ModelInstanceDiffTest(TestCase):
|
|||
)
|
||||
|
||||
|
||||
class TestRelatedDiffs(TestCase):
|
||||
def setUp(self):
|
||||
self.test_date = datetime.datetime(2022, 1, 1, 12, tzinfo=datetime.timezone.utc)
|
||||
|
||||
def test_log_entry_changes_on_fk_object_update(self):
|
||||
t1 = self.test_date
|
||||
with freezegun.freeze_time(t1):
|
||||
simple = SimpleModel.objects.create()
|
||||
one_simple = SimpleModel.objects.create()
|
||||
two_simple = SimpleModel.objects.create()
|
||||
instance = RelatedModel.objects.create(
|
||||
one_to_one=simple, related=one_simple
|
||||
)
|
||||
|
||||
t2 = self.test_date + datetime.timedelta(days=20)
|
||||
with freezegun.freeze_time(t2):
|
||||
instance.related = two_simple
|
||||
instance.save()
|
||||
|
||||
log_one = instance.history.filter(timestamp=t1).first()
|
||||
log_two = instance.history.filter(timestamp=t2).first()
|
||||
self.assertTrue(isinstance(log_one, LogEntry))
|
||||
self.assertTrue(isinstance(log_two, LogEntry))
|
||||
|
||||
self.assertEqual(int(log_one.changes_dict["related"][1]), one_simple.id)
|
||||
self.assertEqual(int(log_one.changes_dict["one_to_one"][1]), simple.id)
|
||||
self.assertEqual(int(log_two.changes_dict["related"][1]), two_simple.id)
|
||||
|
||||
def test_log_entry_changes_on_fk_id_update(self):
|
||||
t1 = self.test_date
|
||||
with freezegun.freeze_time(t1):
|
||||
simple = SimpleModel.objects.create()
|
||||
one_simple = SimpleModel.objects.create()
|
||||
two_simple = SimpleModel.objects.create()
|
||||
instance = RelatedModel.objects.create(
|
||||
one_to_one_id=int(simple.id), related_id=int(one_simple.id)
|
||||
)
|
||||
|
||||
t2 = self.test_date + datetime.timedelta(days=20)
|
||||
with freezegun.freeze_time(t2):
|
||||
instance.related_id = int(two_simple.id)
|
||||
instance.save()
|
||||
|
||||
log_one = instance.history.filter(timestamp=t1).first()
|
||||
log_two = instance.history.filter(timestamp=t2).first()
|
||||
self.assertTrue(isinstance(log_one, LogEntry))
|
||||
self.assertTrue(isinstance(log_two, LogEntry))
|
||||
|
||||
self.assertEqual(int(log_one.changes_dict["related"][1]), one_simple.id)
|
||||
self.assertEqual(int(log_one.changes_dict["one_to_one"][1]), simple.id)
|
||||
self.assertEqual(int(log_two.changes_dict["related"][1]), two_simple.id)
|
||||
|
||||
def test_log_entry_create_fk_changes_to_string_objects_in_display_dict(self):
|
||||
t1 = self.test_date
|
||||
with freezegun.freeze_time(t1):
|
||||
simple = SimpleModel.objects.create(text="Test Foo")
|
||||
one_simple = SimpleModel.objects.create(text="Test Bar")
|
||||
instance = RelatedModel.objects.create(
|
||||
one_to_one=simple, related=one_simple
|
||||
)
|
||||
|
||||
log_one = instance.history.filter(timestamp=t1).first()
|
||||
self.assertTrue(isinstance(log_one, LogEntry))
|
||||
display_dict = log_one.changes_display_dict
|
||||
self.assertEqual(display_dict["related"][1], "Test Bar")
|
||||
self.assertEqual(display_dict["related"][0], "None")
|
||||
self.assertEqual(display_dict["one to one"][1], "Test Foo")
|
||||
|
||||
def test_log_entry_deleted_fk_changes_to_string_objects_in_display_dict(self):
|
||||
t1 = self.test_date
|
||||
with freezegun.freeze_time(t1):
|
||||
simple = SimpleModel.objects.create(text="Test Foo")
|
||||
one_simple = SimpleModel.objects.create(text="Test Bar")
|
||||
one_simple_id = int(one_simple.id)
|
||||
instance = RelatedModel.objects.create(
|
||||
one_to_one=simple, related=one_simple
|
||||
)
|
||||
|
||||
t2 = self.test_date + datetime.timedelta(days=20)
|
||||
with freezegun.freeze_time(t2):
|
||||
one_simple.delete()
|
||||
|
||||
log_two = LogEntry.objects.filter(object_id=instance.id, timestamp=t2).first()
|
||||
self.assertTrue(isinstance(log_two, LogEntry))
|
||||
display_dict = log_two.changes_display_dict
|
||||
self.assertEqual(
|
||||
display_dict["related"][0], f"Deleted 'SimpleModel' ({one_simple_id})"
|
||||
)
|
||||
self.assertEqual(display_dict["related"][1], "None")
|
||||
|
||||
def test_no_log_entry_created_on_related_object_string_update(self):
|
||||
t1 = self.test_date
|
||||
with freezegun.freeze_time(t1):
|
||||
simple = SimpleModel.objects.create(text="Test Foo")
|
||||
one_simple = SimpleModel.objects.create(text="Test Bar")
|
||||
instance = RelatedModel.objects.create(
|
||||
one_to_one=simple, related=one_simple
|
||||
)
|
||||
|
||||
t2 = self.test_date + datetime.timedelta(days=20)
|
||||
with freezegun.freeze_time(t2):
|
||||
# Order is important. Without special FK handling, the arbitrary in memory
|
||||
# changes to the (same) related object's signature result in a perceived
|
||||
# update where no update has occurred.
|
||||
one_simple.text = "Test Baz"
|
||||
instance.save()
|
||||
one_simple.save()
|
||||
|
||||
# Assert that only one log for the instance was created
|
||||
self.assertEqual(instance.history.all().count(), 1)
|
||||
# Assert that two logs were created for the parent object
|
||||
self.assertEqual(one_simple.history.all().count(), 2)
|
||||
|
||||
def test_log_entry_created_if_obj_strings_are_same_for_two_objs(self):
|
||||
"""FK changes trigger update when the string representation is the same."""
|
||||
t1 = self.test_date
|
||||
with freezegun.freeze_time(t1):
|
||||
simple = SimpleModel.objects.create(text="Test Foo")
|
||||
one_simple = SimpleModel.objects.create(text="Twinsies", boolean=True)
|
||||
two_simple = SimpleModel.objects.create(text="Twinsies", boolean=False)
|
||||
instance = RelatedModel.objects.create(
|
||||
one_to_one=simple, related=one_simple
|
||||
)
|
||||
|
||||
t2 = self.test_date + datetime.timedelta(days=20)
|
||||
with freezegun.freeze_time(t2):
|
||||
instance.related = two_simple
|
||||
instance.save()
|
||||
|
||||
self.assertEqual(instance.history.all().count(), 2)
|
||||
log_create = instance.history.filter(timestamp=t1).first()
|
||||
log_update = instance.history.filter(timestamp=t2).first()
|
||||
self.assertEqual(int(log_create.changes_dict["related"][1]), one_simple.id)
|
||||
self.assertEqual(int(log_update.changes_dict["related"][1]), two_simple.id)
|
||||
|
||||
|
||||
class TestModelSerialization(TestCase):
|
||||
def setUp(self):
|
||||
super().setUp()
|
||||
|
|
|
|||
Loading…
Reference in a new issue