Browsing history entries in admin fires pre/post save signals potentially leading to nasty side effects

[romanek-adam-b2c2]

As described in the subject - opening a specific history entry using django admin causes the underlying model pre/post save signals to be fired which may potentially lead to nasty side effects.

In our case we have a post_save signal handler attached to one of our models. The signal handler takes an instance of a model, extracts some fields from it and publishes them to Redis. The expectation is that Redis would always keep the latest "view" on a subset of the model instance fields.

However, I realised that when browsing the history of a specific model instance via django admin, historical instances are de-serialised from JSON and temporarily saved inside a database transaction (using savepoints) BUT this also leads to firing normal pre/post save signals. So in our case our signal handler publishes to Redis the history entry which was opened last. It totally breaks our business logic and thus turned out to be a critical bug.

I'm fairly sure this side effect is not obvious to most users of this library but it's not documented anywhere. Am I missing something?

EDIT: when browsing history using django admin the pre/post signals are fired with raw param set to True. This param can be used to distinguish them from the regular signals fired when saving a model instance - they get raw=False.

[etianen]

That is correct, and definitely counts as a gotcha for the admin integration. 😢

[etianen]

I've added a note to the top of the admin docs about this 🙇 https://django-reversion.readthedocs.io/en/latest/admin.html

[romanek-adam-b2c2]

Thank you @etianen !

[dbischof]

raw=False does not come through in the post_delete signals. Rather than rely on it, wouldn't it be better to check on the transaction used by the history browsing and only perform work if it succeeds?

from functools import partial

from django.db import transaction
from django.db.models.signals import post_delete

@receiver(post_delete, dispatch_uid='instance-deleted')
def model_deleted(sender, instance, using, **kwargs):
    transaction.on_commit(partial(sync_delete, obj=instance))

[etianen]

That's a good idea 🙇