Issue: Optimistic locking
Author: tuukkamustonen
State: closed
Created: 2015-07-15 07:42:37

I'm not sure what is the correct way to implement support [optimistic locking](http://martinfowler.com/eaaCatalog/optimisticOfflineLock.html). As usual, I want to hold a version column that I increment on updates, while also ensuring that I am updating the row with the current version.

I assume I could implement this with something like:

``` python
Layer.update(name=self.name, label=self.label, version=self.version + 1).where((id=self.id) & (version=self.version))
```

But here, I need to explicitly list all the fields/values in `update(...)`. Is there some shortcut?


The typical way to handle this is to select the record you're modifying `FOR UPDATE` (see [docs](http://docs.peewee-orm.com/en/latest/peewee/api.html#SelectQuery.for_update)). Here's a [blog post](http://blog.2ndquadrant.com/postgresql-anti-patterns-read-modify-write-cycles/) you might also find useful. Note that this only works with Postgresql.


@coleifer `FOR UPDATE` is for pessimistic locking, while I explicitly need optimistic locking here. A user who opens up data for modification might think for a long time (while modifying a form) and even vanish without never submitting the changes. So, optimistic locking suits my use case better.

It sounds like there's no builtin support for this. Would you implement it as I sketched above or do you have a more sophisticated approach?


I don't have any plans on adding this to Peewee itself, so you can choose the implementation that best suits your use-case.


@coleifer I finally have to implement this. If I made a PR for it, would consider merging it into peewee? It could work like:

```
class MyModel(peewee.Model):
    version = IntegerVersionField()

m = MyModel()
m.save()
m1 = MyModel.get()
m2 = MyModel.get()
m1.save()
m2.save()  # would throw error

m2.save(optimistic=False)  # temporarily disable locking
```

Or it could be:

```
class MyModel(peewee.Model):
    version = IntegerField(default=1)

    class Meta:
        version_field = 'version'
```

I don't have strong opinion on the API, I just wish it was included in peewee and would not break during version upgrades. This touches part of core functionalities (`save()`) so I'm not sure if this could go into playhouse?

It shouldn't need too many modifications to `Model.save()`, the core idea would be something like:

```
        if self._meta.version_field:
            field_dict[self._meta.version_field] += 1
            rows = self.update(**field_dict).where(self._pk_expr() & (self._meta.fields[self._meta.version_field] == getattr(self, self._meta.version_field))).execute()
            if rows != 1:
                raise Exception("Something to indicate that update did not succeed. The cause is probably in version mismatch, but it could be that object with PK was no longer there :|")
        else:
            rows = self.update(**field_dict).where(self._pk_expr()).execute()
```

Maybe something more sophisticated than `getattr` etc.

I am personally doing locking based on integer version field, but it could be timestamp, hash, or anything.

What do you think?


That's pretty cool, but honestly I think that is just enough beyond the
scope that I'd rather it live as its own project. By all means you can
create your own base middle class that overrides save.

There is enough complexity there that I think I could get bogged down
supporting exotic edge cases or who knows, and really I want to focus on
peewee.
On Dec 7, 2015 2:39 AM, "Tuukka Mustonen" notifications@github.com wrote:

> @coleifer https://github.com/coleifer I finally have to implement this.
> If I made a PR for it, would consider merging it into peewee? It could work
> like:
> 
> class MyModel(peewee.Model):
>     version = IntegerVersionField()
> 
> m = MyModel()
> m.save()
> m1 = MyModel.get()
> m2 = MyModel.get()
> m1.save()
> m2.save()  # would throw error
> 
> m2.save(optimistic=False)  # temporarily disable locking
> 
> Or it could be:
> 
> class MyModel(peewee.Model):
>     version = IntegerField(default=1)
> 
> ```
> class Meta:
>     version_field = 'version'
> ```
> 
> I don't have strong opinion on the API, I just wish it was included in
> peewee and would not break during version upgrades. This touches part of
> core functionalities (save()) so I'm not sure if this could go into
> playhouse?
> 
> It shouldn't need too many modifications to Model.save(), the core idea
> would be something like:
> 
> ```
>     if self._meta.version_field:
>         field_dict[self._meta.version_field] += 1
>         rows = self.update(**field_dict).where(self._pk_expr() & (self._meta.fields[self._meta.version_field] == getattr(self, self._meta.version_field))).execute()
>         if rows != 1:
>             raise Exception("Something to indicate that update did not succeed. The cause is probably in version mismatch, but it could be that object with PK was no longer there :|")
>     else:
>         rows = self.update(**field_dict).where(self._pk_expr()).execute()
> ```
> 
> Maybe something more sophisticated than getattr etc.
> 
> I am personally doing locking based on integer version field, but it could
> be timestamp, hash, or anything.
> 
> What do you think?
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/coleifer/peewee/issues/658#issuecomment-162448760.


Personally I would love to see this as part of peewee, as it's something I found useful when I was using Ruby's ActiveRecord. Until now I wasn't doing optimistic locking in my peewee based projects, just because I was lazy to implement it myself...

@tuukkamustonen if you end up implementing it in a separate lib, please post the link here. 
And about your suggested implementation - I suggest using a dedicated exception, so it can be properly handled by using code.


@tuukkamustonen: just throwing a +1 here - if this is something you've already written, I'd love to see the code!


@arikfr @tconkling Well, I am just pasting here the code I'm running atm. Maybe @coleifer wants to sanity check it, too.

I am not running it in production, nor do I have extensive test suite for it. It's just something I threw in for now, and I actually had it back in my head to ask for comments at some point.

The big drawback is that this code is pretty much bound to a specific peewee version, because it re-writes the whole `save` method. Of course, one could rename it differently, but, well.

A model is defined by:

```
class MyModel(LockableModel):

    class Meta(object):
        version_field = 'version'

    version = IntegerField(default=1)
```

It only supports numeric locking field.

And the actual implementation (my only additions are the two blocks with `# Comments`):

```
class ConcurrentModification(PeeweeException):
    """Indicates that entity has been changed in the database between loading and saving."""

class LockableModel(peewee.Model):

    # The base was copied from peewee 2.7.4
    def save(self, force_insert=False, only=None, optimistic=True):
        field_dict = dict(self._data)
        if self._meta.primary_key is not False:
            pk_field = self._meta.primary_key
            pk_value = self._get_pk_value()
        else:
            pk_field = pk_value = None
        if only:
            field_dict = self._prune_fields(field_dict, only)
        elif getattr(self._meta, 'only_save_dirty', False) and not force_insert:
            field_dict = self._prune_fields(
                field_dict,
                self.dirty_fields)
            # Empty dict is ok with optimistic locking (to simply increment version)
            # XXX: Is this really needed? There is no use case for this!
            if not field_dict and (
                not getattr(self._meta, 'version_field', None) or (
                    getattr(self._meta, 'version_field', None) and not optimistic
                )
            ):
                self._dirty.clear()
                return False

        self._populate_unsaved_relations(field_dict)
        if pk_value is not None and not force_insert:
            if self._meta.composite_key:
                for pk_part_name in pk_field.field_names:
                    field_dict.pop(pk_part_name, None)
            else:
                field_dict.pop(pk_field.name, None)

            # Locking
            if optimistic and getattr(self._meta, 'version_field', None):
                # Add and increment version field, if it was not manually given. This is the normal behavior
                if self._meta.version_field in field_dict:
                    raise ValueError('You must not modify version field manually while using locking!')
                field_dict[self._meta.version_field] = self._data[self._meta.version_field] + 1
                rows = self.update(**field_dict).where(
                    self._pk_expr() &
                    (
                        self._meta.fields[self._meta.version_field] ==
                        getattr(self, self._meta.version_field)
                    )
                ).execute()
                if rows == 0:
                    entity = self.select().where(self._pk_expr()).get()  # throws DoesNotExist if not found, but it should...
                    raise ConcurrentModification(
                        'Tried to save entity with version of `{}` but database contains entry with version of `{}`!'
                        .format(field_dict[self._meta.version_field], getattr(entity, self._meta.version_field))
                    )
                # Also set correct version on this instance, if update succeeded
                setattr(self, self._meta.version_field, field_dict[self._meta.version_field])

            else:
                rows = self.update(**field_dict).where(self._pk_expr()).execute()
        elif pk_field is None:
            self.insert(**field_dict).execute()
            rows = 1
        else:
            pk_from_cursor = self.insert(**field_dict).execute()
            if pk_from_cursor is not None:
                pk_value = pk_from_cursor
            self._set_pk_value(pk_value)
            rows = 1
        self._dirty.clear()
        return rows
```

So, calling `save()` does the usual optimistic locking stuff, and calling `save(optimistic=False)` disables the feature.


Cool, I'll play with this myself!

It seems like something @coleifer could possibly support by breaking out some of the `save()` functionality out into several other functions that custom subclasses, like your optimistic-locking model, could override?