Django Admin adding new revisions

[Rob Wheeler]

Hello,

I have been running django-reversion 1.6.6 on django 1.4.x for a few years.  I recently upgraded to django-reversion 1.9.3 on django 1.8.4 and I noticed an odd behavior.  In the django admin interface, when I click on a revision in the history list to view it and then click back (not 'save'). I find that a new revision with blank comment has been created.  Is this a new feature or a bug?

Thanks,

-Rob

[Dave Hall]

That's very odd. The latest django-reversion uses database transactions to handle revision preview and rollback, and it sounds like a dirty revision is being left over.

Are you using a customised VersionAdmin, or just the standard interface?

Are you using RevisionMiddleware?

--
You received this message because you are subscribed to the Google Groups "django-reversion discussion group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-reversi...@googlegroups.com.
To post to this group, send email to django-r...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-reversion.
For more options, visit https://groups.google.com/d/optout.

[hele...@gmail.com]

Hi Rob,

We're seeing a similar issue. Were you able to figure out what was going on?

Thanks.

Helen.

[Dave Hall]

I'm afraid Rob never got back to me.

Same questions apply: 

Are you using a customised VersionAdmin, or just the standard interface?

Are you using RevisionMiddleware?

--

You received this message because you are subscribed to the Google Groups "django-reversion discussion group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-reversi...@googlegroups.com.
To post to this group, send email to django-r...@googlegroups.com.

Visit this group at https://groups.google.com/group/django-reversion.

[hele...@gmail.com]

Thanks for replying so quickly.

1. Standard interface

2. Yes

Helen

[Dave Hall]

Can you disable the revision middleware and see if the problem goes away?

[plin...@gmail.com]

Dave,

Thanks for that suggestion. Disabling the middleware fixes the issue, but after digging into it more, my feeling is that the way that page works has some unexpected side effects that maybe should be changed.

When the old revision is being temporarily reverted by the version admin's revision view, the model save signals for the old versions fire, which adds them to the revision context when revision middleware is enabled. The transaction for the revert then gets rolled back, but when the request ends, the middleware saves the old versions that were added to the revision context in a new revision. It looks like this revert/rollback behavior was introduced in reversion 1.9. Does that make sense?

Firing model save signals for old versions of a model from a page that seems like it shouldn't have any side effects seems unexpected and dangerous depending on how you are using save signals in the rest of your application. In this case, it add them to the middleware's reversion context, but there is another place in my application where I've used save signals that now has to deal with them being fired when the model isn't actually changing. What are your thoughts?

Thanks,

Peter

[Dave Hall]

Thanks for your follow-up! So there are a couple of issues here:

A bug: Use of RevisionMiddleware with admin rollback creates empty duplicate revisions: https://github.com/etianen/django-reversion/issues/496

The signals is more difficult. If all the signals do is database-level stuff, then the transaction will roll back the changes, no worries. If they perform other side-effects, however, we do indeed have a problem.

There's a couple of conflicting needs here. Users which use the signals to perform database actions would expect them to be fired during rollback. Users which use signals to perform other side-effects, however, will have issues.

Which means we need a new feature: Configuration setting to disable signals during admin version rollback: https://github.com/etianen/django-reversion/issues/497

Shall we move the discussion to these two new issues?

[Dave Hall]

Needless to say, I'm a little busy at the moment, so these will be addressed faster if I receive some good pull requests. :)

[Peter Linnartz]

I'm seeing slightly different behavior than you're describing, using Django 1.8.4 and reversion 1.9.3. Loading the revision creates a new revision that is not empty: it contains all versions as they were in the revision that is being reverted and then rolled back by the admin view. When version.revision.revert(delete=True) is called, each serialized object gets saved and added to the middleware's revision context and then saved as versions in the new revision in the middleware's process_repsonse. If we didn't fire the save signals on the revert, those old versions wouldn't be added to the middleware's revision context. Does that make sense? Let me know if I'm missing something, or if this behavior is different in the latest Django/reversion.

You received this message because you are subscribed to a topic in the Google Groups "django-reversion discussion group" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-reversion/8jmNzei5w08/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-reversi...@googlegroups.com.

[Dave Hall]

Ignoring the bug in RevisionMiddleware for now...

So the way the admin site currently handles rollback is:

1. Create a new DB savepoint.
   
2. Create a new revision context
   
3. Roll back the revision in the DB, including all model signals
   
4. If the user chooses to roll back the revision, the savepoint created in (1) is committed. Else, the savepoint created in (1) is rolled back.

In order for stage 3 to work, all the DB signals HAVE to fire. Otherwise, when the savepoint is committed, the app will be in an inconsistent state. However, if the signals cause side-effects that can't be rolled back in the transaction, rolling back the savepoint will cause inconsistencies in the app.

Prior to 1.9, the admin preview view was a hacky patch-up which loaded the previous revision into the admin interface without touching the DB. Unfortunately, it means a lot of common-use cases such as foreign keys and computed properties ended up inconsistent in the admin preview.

So our options are:

1. Don't use database transactions - (inconsistent previews in common usage, consistent app always.)
   
2. Use database transactions with signals - (full consistency in previews and app in common usage, but inconsistent app if signals have non-db side-effects).

A way to fix signals which can't be rolled back in a DB transaction is to use the new on_commit Django hook, and only apply then when a transaction is completed. That's pretty good practice for non-db side-effects anyway.

[Peter Linnartz]

That makes sense, thanks for the explanation. Sorry I haven't been able to follow up until now. I'm definitely interested in digging in more and helping out with the middleware bug, but haven't been able to because of other priorities at work. If I'm able to work on it, I'll shoot a pull request out, but it may be a while.