etianen / django-reversion

django-reversion is an extension to the Django web framework that provides version control for model instances.
https://django-reversion.readthedocs.io
BSD 3-Clause "New" or "Revised" License
3.04k stars 489 forks source link

4.0.0 changelog breaking change is not clear #970

Closed GeyseR closed 3 months ago

GeyseR commented 5 months ago

The 4.0.0 docs section says:

Breaking: The create_revision view decorator and RevisionMiddleware no longer roll back the revision and database transaction on response status code >= 400. It's the responsibility of the view to use transaction.atomic() to roll back any invalid data. This can be enabled globally by setting ATOMIC_REQUESTS=True. (@etianen) https://docs.djangoproject.com/en/3.1/ref/settings/#std:setting-DATABASE-ATOMIC_REQUESTS

From the description I initially thought that by using the ATOMIC_REQUESTS setting the original reversion middleware will be restored.

But after checking the source code of the reversion package and django itself I think there are several options for the developer: 1) use create_revision decorator or ReversionMiddleware - create revisions automatically for all models changes made in the view. Optionally wrap view(s) in transaction 2) use transaction.atomic or ATOMIC_REQUESTS - wrap view(s) in atomic, handle revisions creation manually 3) write own decorator/middleware to restore the original behavior that rollbacks model changes on status >= 400

Did I get the current state of things right? Should we make the section more clear?

etianen commented 3 months ago

It's been a very long time since the change, so I've had to look back over the commit history to answer your question.

You're correct in that setting ATOMIC_REQUESTS=True doesn't quite restore the original behaviour. ATOMIC_REQUESTS=True only rolls back the revision and database transaction if the view raises an exception, whereas pre-4.0.0 django-reversion rolled it back on and status code >= 400.

Using ATOMIC_REQUESTS=True and RevisionMiddleware is pretty close to the original behaviour. If you want the exact behavior you'll need to go with your option (3).

In general, the advice django gives with ATOMIC_REQUEST about it being "inefficient when traffic increases" applies even more strongly to django-reversion. The best thing to do is to use transaction.atomic() and reversion.create_revision() manually where needed.