Proposal: Prevent data loss in the admin

[Rune Kaagaard]

Hi

I've made a branch that adds optimistic concurrency control to the admin: https://github.com/runekaagaard/django-lock-the-admin/compare/adminlock. It works for the main change object and inlines and has tests. All tests passes with postgres and sqlite.

This prevents users from accidentally overwriting each others changes. To confirm this is is the case today [1]:

1. Open the same change page in the admin in two tabs.

2. Make different modifications to them. 

3. Press save in both tabs. 

4. The changes in the last saved tab silently overwrites the first ones.

In my proposal an edit conflict looks like:

It works by:

- Adding a "admin.Lock" model with version number, pk and content type of the objects being edited.

- Adding "lock_version" hidden fields to the main object form and inline forms.

- Wrapping the save part of "changeform_view" in a transaction that rolls back if a any object on the page has a stale version.

- Showing an error message if the user wasn't allowed to save.

What do you think, is this something I should open a ticket for and start working on?

Scope

This proposal doesn't cover editable inlines [2], but they are kind of unusable as is, unless you are a single admin, that really knows what you are doing. I would love to work on that for v1.8, but prefers getting a locking mechanism in place first without it!?

The same goes for locking of admin actions and the cryptic error you get when you are deleting an already deleted inline [3].

Rails supports both pessimistic and optimistic locking [4] [5]. It would be cool to have a "contrib.locking" module, but I think it's out of scope for this proposal. Plus, it will be easy to change the locking method when/if such a module happens.

Implementation

1. Should there be a settings.ADMIN_USE_LOCKING?

2. Should there be a ModelAdmin.use_locking property?

3. Instead of having an "admin.Lock" model you could have a "version" field on each table and make a LockingModel available. I didn't do that because i wanted to prevent data loss in the admin out of the box, also for existing projects. What's your take on that?

Should it be a third party app?

I think the "killer app" for Django should be safe by default. Third party solutions already exists, though [6] [7] [8].

Test project

To test the UI of this feature, use the adminlock-test branch:

git clone https://github.com/runekaagaard/django-lock-the-admin.git

cd django-lock-the-admin

git checkout adminlock-test

cd test_project

python manage.py migrate

python manage.py createsuperuser

python manage.py runserver

References:

- [1] https://code.djangoproject.com/ticket/11652

- [2] https://code.djangoproject.com/ticket/11313

- [3] https://code.djangoproject.com/ticket/15574

- [4] http://edgeapi.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html

- [5] http://api.rubyonrails.org/classes/ActiveRecord/Locking/Pessimistic.html

- [6] https://github.com/saxix/django-concurrency

- [7] https://github.com/RobCombs/django-locking

- [8] https://github.com/runekaagaard/django-admin-locking

Best,

Rune Kaagaard

[Daniele Procida]

On Mon, Dec 8, 2014, Rune Kaagaard <rum...@gmail.com> wrote:

>I've made a branch that adds optimistic concurrency control to the admin:
>https://github.com/runekaagaard/django-lock-the-admin/compare/adminlock. It
>works for the main change object and inlines and has tests. All tests
>passes with postgres and sqlite.

I like the concept.

I would prefer however to see something like:

3 items on this page have been updated by another user while you were editing them.

You can:

* see the other user's changes[link] in a new browser window, and apply your changes there, or
* refresh this pages[link] (Warning! your changes here will be lost!) and reapply your changes here

This way the user is invited to compare the differences and to copy their changes over, rather than simply lose them.

Daniele

[Rune Kaagaard]

@Daniele

I struggled a bit with how to display the error message, and your solution makes perfect sense.

Best,

Rune Kaagaard

[Tim Graham]

Hi Rune,

It's not clear to me that a fix that's coupled to the admin is the way to go when this problem could exist for any model form, etc.

One ticket you didn't mention that seems like it could be a good place to start is https://code.djangoproject.com/ticket/16549

Tim

[Russell Keith-Magee]

Hi Rune,

I agree with Tim. This is clearly a problem that exists, and I agree it would be good to fix it. However, I wouldn't want to see this as an "Admin only" fix.

I'd rather see a generic hook that Admin then leverages as the "first customer". A similar approach was taken with object-level permissions - a generic framework for object-level permissions was added, and admin was modified to support that API as part of the proof that the API was sufficient.

From an implementation perspective, I also have a mild negative reaction to the idea of using a separate table in the database for locks. Using a database row to marshal this behavior sounds like adding load to a database when all you need is a transient marker. A backend API that lets you use memcache, redis, or even a file-based backend might be called for. And that's assuming we take the "separate lock table" approach - I can think of other approaches that involve version indicators on the model itself. These are just initial impressions, however; I haven't given the problem a whole lot of thought, so these might not be viable ideas given a little more consideration.

Yours,

Russ Magee %-) 

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/f1a56260-89d7-4439-aed1-9407d799f62f%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Steven Cummings]

FWIW, this was on my mind a few years ago and I created this [1] ticket. It spidered out into one or two more, but the basic ideas were:

* optimistic locking could be implemented by enhancing saves with conditions (i.e., save_if)

* a row-modified count could help that function also tell you if the save condition passed or failed

* rows modified *may* be used now to update data via QuerySet.update, but allegedly not all of the DB implementations provide the count in the same way (e.g., matched vs. updated)

* the modified count discrepancy was worth spinning up another ticket to discuss and investigate

I agree with Russell, I think the need for this is much broader than the admin.

[1] https://code.djangoproject.com/ticket/16549

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAJxq84_Kz_6ZCTzxEjaZm61hdfYcKnQNHw-xyPr%3DzX%3Dtc8GYvw%40mail.gmail.com.

[Rune Kaagaard]

Hi Tim, Russell and Steven

I agree it would be more useful to be able to work with locking from all places, not only the admin. The question then is how? :)

From an implementation perspective, I also have a mild negative reaction to the idea of using a separate table in the database for locks. Using a database row to marshal this behavior sounds like adding load to a database when all you need is a transient marker. A backend API that lets you use memcache, redis, or even a file-based backend might be called for. And that's assuming we take the "separate lock table" approach - I can think of other approaches that involve version indicators on the model itself.

I think having a separate lock table can and should be avoided with a general locking solution and that it's solvable without adding to the query count. For my use cases a version field on each table would fit nicely, and a transient datastore would not be safe enough, but more opinions are needed on this of course.

I first tried to look into the "save with conditions" idea, but couldn't make it fit. I did not see a way to pass information about what happened (modification count/update count) from save_base, to save, to an overriding save method, without breaking existing apps heavily. That's also mentioned as a challenge in the ticket.

Secondly, i tried an approach that adds an OptimisticLockingModel model that extends the _do_update() method. A very naive POC can be found at:

https://github.com/runekaagaard/django-lock-the-admin/compare/optimistic-locking?expand=1#diff-c000f88621a9da867b462e1cf3e7cb36R13

That works for the admin in the sense that exceptions are thrown, and all the data is safe. I think the "matched vs. updated" issue goes away because a matched row is also always updated by the version field being incremented. Which drawbacks and edge cases can you see in such an solution?

A whole other issue is how to handle the deletion of already deleted objects.

Best,

Rune Kaagaard

[Rune Kaagaard]

A whole other issue is how to handle the deletion of already deleted objects.

I misspoke here. I meant how to handle the deletion of an object with an outdated version number. Should it raise the same exception? 

[Carl Meyer]

Yes, I think that deletion is like any other update to an outdated model
- it should raise a concurrent modification exception.

FWIW, I also wrote an implementation of this a few years ago; I don't
know if there's much in it that would be useful to an implementation
built in to Django:
https://github.com/mozilla/moztrap/blob/master/moztrap/model/mtmodel.py

It's mixed in with a number of other things there (soft-deletion with
cascade, tracking of modifying user) but it's pretty easy to pick out
the concurrency-control bits by just searching for "cc_version".

I took the approach of adding an incrementing field to the model itself.
I think this is by far the best combination of simplicity and
performance, but it would definitely need to be an optional base class
if added to Django.

Carl