Stop deleting log entries in log_create #559
Conversation
aleh-rymasheuski
force-pushed
the
stop-deleting-logs
branch
from
d167a50 to
2b51e8d
Compare
Codecov Report
@@ Coverage Diff @@
## master #559 +/- ##
==========================================
- Coverage 94.96% 94.94% -0.03%
==========================================
Files 31 31
Lines 994 990 -4
==========================================
- Hits 944 940 -4
Misses 50 50
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
auditlog/receivers.py
Outdated
| @@ -52,6 +52,9 @@ def log_update(sender, instance, **kwargs): | |||
| if instance.pk is not None: | |||
| update_fields = kwargs.get("update_fields", None) | |||
| old = sender.objects.filter(pk=instance.pk).first() | |||
| if not old: | |||
There was a problem hiding this comment.
@hramezani, should I replace the check on line 52 (if instance.pk is not None) to if instance._state.adding? https://docs.djangoproject.com/en/4.2/ref/models/instances/#django.db.models.Model._state
It will spare us an extra query to the database.
There was a problem hiding this comment.
I thought about this for a short while and decided to use the adding flag (it exists for a reason).
aleh-rymasheuski
force-pushed
the
stop-deleting-logs
branch
from
2b51e8d to
9a45bc5
Compare
|
Thanks @aleh-rymasheuski @aqeelat it would be great to take a look at this PR if you have time. |
| if instance.pk is not None: | ||
| if not instance._state.adding: |
| # Delete log entries with the same pk as a newly created model. | ||
| # This should only be necessary when a pk is used twice. | ||
| if kwargs.get("action", None) is LogEntry.Action.CREATE: | ||
| if ( | ||
| kwargs.get("object_id", None) is not None | ||
| and self.filter( | ||
| content_type=kwargs.get("content_type"), | ||
| object_id=kwargs.get("object_id"), | ||
| ).exists() | ||
| ): | ||
| self.filter( | ||
| content_type=kwargs.get("content_type"), | ||
| object_id=kwargs.get("object_id"), | ||
| ).delete() | ||
| else: | ||
| self.filter( | ||
| content_type=kwargs.get("content_type"), | ||
| object_pk=kwargs.get("object_pk", ""), | ||
| ).delete() | ||
|
|
There was a problem hiding this comment.
Why was this added in the first place?
https://thoughtbot.com/blog/chestertons-fence
There was a problem hiding this comment.
@aqeelat, I tried to find the reason for adding it and couldn't find any solid reason. The first revision of this block of code was added in a commit 81098cd labeled "Big commit: testproject, bugfixes, changed directory structure".
One possible reason is that the bug I fixed in receivers.py in this pull request was adding an invalid "updated" log entry before logging the "created" log entry.
Another possible reason is that @jjkester was using auditlog with MySQL which was known to reuse the values of primary keys at least at the time when auditlog was first implemented.
The comment in the removed code says that primary key reuse "should only happen when all records were deleted / the table was truncated" which is the third possible reason: @jjkester had to truncate the table of an auditlogged model at least once (which shouldn't be viewed as a common use case therefore it shouldn't be the behavior by default).
There was a problem hiding this comment.
The primary key reuse is still a thing in mysql (link). Should we handle this by checking the db engine? Or maybe by a configuration var?
There was a problem hiding this comment.
@aqeelat, I don't think we should ever delete auditlog entries by default. The audit log exists to be immutable.
There was a problem hiding this comment.
@aleh-rymasheuski I agree. However, people who are serious about auditlogs will also be careful to avoid primary-key reuse.
The main issue is that when accessing the logs for a instance, we shouldn't get entries for older instances with the same object id.
I know this is an edge case, which is why I'm trying to find the most inclusive solution.
There was a problem hiding this comment.
@hramezani @aleh-rymasheuski
I don't want to stand in the way here. I trust you're judgment. I just wanted to give my 2 cents.
There was a problem hiding this comment.
Thanks @aqeelat for your input here.
IMHO, we can merge it.
This pull request addresses two issues:
log_createspends 22% of its run time callingdelete()method in a test run of a medium-sized data loading job on my machine (7% of the total run time of the job).I assume this is technically a breaking change so I'd like it included in 3.0.0 release too.
Notice the change in the
log_updatehandler. Before my changes, every object creation was producing two log entries:updatedlog entry, thencreatedlog entry; and while creating this second log entry, auditlog was deleting the first one, so it was not noticeable.