Merge pull request #604 from mozilla-services/keep-last-modified

Keep last modified

cliquet/resource/__init__.py

@@ -492,7 +492,10 @@ def delete(self):
         record = self._get_record_or_404(self.record_id)
         self._raise_412_if_modified(record)
 
-        deleted = self.model.delete_record(record)
+        # Retreive the last_modified information from a querystring if present.
+        last_modified = self.request.GET.get('last_modified')
+
+        deleted = self.model.delete_record(record, last_modified=last_modified)
         return self.postprocess(deleted, action=ACTIONS.DELETE)
 
     #
@@ -506,6 +509,7 @@ def process_record(self, new, old=None):
         .. code-block:: python
 
             def process_record(self, new, old=None):
+                new = super(MyResource, self).process_record(new, old)
                 version = old['version'] if old else 0
                 new['version'] = version + 1
                 return new
@@ -517,6 +521,7 @@ def process_record(self, new, old=None):
             from cliquet.errors import raise_invalid
 
             def process_record(self, new, old=None):
+                new = super(MyResource, self).process_record(new, old)
                 if new['browser'] not in request.headers['User-Agent']:
                     raise_invalid(self.request, name='browser', error='Wrong')
                 return new
@@ -528,6 +533,17 @@ def process_record(self, new, old=None):
         :returns: the processed record.
         :rtype: dict
         """
+        new_last_modified = new.get(self.model.modified_field)
+        not_specified = old is None or self.model.modified_field not in old
+
+        if new_last_modified is None or not_specified:
+            return new
+
+        # Drop the new last_modified if lesser or equal to the old one.
+        is_greater = new_last_modified <= old[self.model.modified_field]
+        if new_last_modified and is_greater:
+            del new[self.model.modified_field]
+
         return new
 
     def apply_changes(self, record, changes):
@@ -1073,6 +1089,7 @@ def process_record(self, new, old=None):
         """Read permissions from request body, and in the case of ``PUT`` every
         existing ACE is removed (using empty list).
         """
+        new = super(ShareableResource, self).process_record(new, old)
         permissions = self.request.validated.get('permissions', {})
 
         annotated = new.copy()

cliquet/resource/model.py

@@ -209,7 +209,7 @@ def update_record(self, record, parent_id=None,unique_fields=None):
                                    modified_field=self.modified_field,
                                    auth=self.auth)
 
-    def delete_record(self, record, parent_id=None):
+    def delete_record(self, record, parent_id=None, last_modified=None):
         """Delete a record in the collection.
 
         Override to perform actions or post-process records after deletion
@@ -237,7 +237,8 @@ def delete_record(self, record):
                                    id_field=self.id_field,
                                    modified_field=self.modified_field,
                                    deleted_field=self.deleted_field,
-                                   auth=self.auth)
+                                   auth=self.auth,
+                                   last_modified=last_modified)
 
 
 class ShareableModel(Model):
@@ -323,11 +324,11 @@ def update_record(self, record, parent_id=None, unique_fields=None):
         annotated[self.permissions_field] = permissions
         return annotated
 
-    def delete_record(self, record_id, parent_id=None):
+    def delete_record(self, record_id, parent_id=None, last_modified=None):
         """Delete record and its associated permissions.
         """
-        record = super(ShareableModel, self).delete_record(record_id,
-                                                           parent_id)
+        record = super(ShareableModel, self).delete_record(
+            record_id, parent_id, last_modified=last_modified)
         perm_object_id = self.get_permission_object_id(record_id)
         self.permission.delete_object_permissions(perm_object_id)
 

cliquet/storage/memory.py

@@ -55,8 +55,11 @@ def strip_deleted_record(self, resource, parent_id, record,
         return deleted
 
     def set_record_timestamp(self, collection_id, parent_id, record,
-                             modified_field=DEFAULT_MODIFIED_FIELD):
-        timestamp = self._bump_timestamp(collection_id, parent_id)
+                             modified_field=DEFAULT_MODIFIED_FIELD,
+                             last_modified=None):
+        timestamp = self._bump_timestamp(collection_id, parent_id, record,
+                                         modified_field,
+                                         last_modified=last_modified)
         record[modified_field] = timestamp
         return record
 
@@ -171,7 +174,8 @@ def collection_timestamp(self, collection_id, parent_id, auth=None):
             return ts
         return self._bump_timestamp(collection_id, parent_id)
 
-    def _bump_timestamp(self, collection_id, parent_id):
+    def _bump_timestamp(self, collection_id, parent_id, record=None,
+                        modified_field=None, last_modified=None):
         """Timestamp are base on current millisecond.
 
         .. note ::
@@ -180,11 +184,34 @@ def _bump_timestamp(self, collection_id, parent_id):
             the time will slide into the future. It is not problematic since
             the timestamp notion is opaque, and behaves like a revision number.
         """
+        # XXX factorize code from memory and redis backends.
+        is_specified = (record is not None
+                        and modified_field in record
+                        or last_modified is not None)
+        if is_specified:
+            # If there is a timestamp in the new record, try to use it.
+            if last_modified is not None:
+                current = last_modified
+            else:
+                current = record[modified_field]
+        else:
+            # Otherwise, use a new one.
+            current = utils.msec_time()
+
+        # Bump the timestamp only if it's more than the previous one.
         previous = self._timestamps[collection_id].get(parent_id)
-        current = utils.msec_time()
         if previous and previous >= current:
-            current = previous + 1
-        self._timestamps[collection_id][parent_id] = current
+            collection_timestamp = previous + 1
+        else:
+            collection_timestamp = current
+
+        # In case the timestamp was specified, the collection timestamp will
+        # be different from the updated timestamp. As such, we want to return
+        # the one of the record, and not the collection one.
+        if not is_specified:
+            current = collection_timestamp
+
+        self._timestamps[collection_id][parent_id] = collection_timestamp
         return current
 
     def create(self, collection_id, parent_id, record, id_generator=None,
@@ -233,10 +260,14 @@ def delete(self, collection_id, parent_id, object_id,
                id_field=DEFAULT_ID_FIELD, with_deleted=True,
                modified_field=DEFAULT_MODIFIED_FIELD,
                deleted_field=DEFAULT_DELETED_FIELD,
-               auth=None):
+               auth=None, last_modified=None):
         existing = self.get(collection_id, parent_id, object_id)
+        # Need to delete the last_modified field of the record.
+        del existing[modified_field]
+
         self.set_record_timestamp(collection_id, parent_id, existing,
-                                  modified_field=modified_field)
+                                  modified_field=modified_field,
+                                  last_modified=last_modified)
         existing = self.strip_deleted_record(collection_id,
                                              parent_id,
                                              existing)

cliquet/storage/postgresql/__init__.py

@@ -216,14 +216,16 @@ def create(self, collection_id, parent_id, record, id_generator=None,
                AND parent_id = :parent_id
                AND collection_id = :collection_id
         )
-        INSERT INTO records (id, parent_id, collection_id, data)
+        INSERT INTO records (id, parent_id, collection_id, data, last_modified)
         VALUES (:object_id, :parent_id,
-                :collection_id, (:data)::JSONB)
+                :collection_id, (:data)::JSONB,
+                from_epoch(:last_modified))
         RETURNING id, as_epoch(last_modified) AS last_modified;
         """
         placeholders = dict(object_id=record_id,
                             parent_id=parent_id,
                             collection_id=collection_id,
+                            last_modified=record.get(modified_field),
                             data=json.dumps(record))
         with self.client.connect() as conn:
             # Check that it does violate the resource unicity rules.
@@ -267,14 +269,16 @@ def update(self, collection_id, parent_id, object_id, record,
                modified_field=DEFAULT_MODIFIED_FIELD,
                auth=None):
         query_create = """
-        INSERT INTO records (id, parent_id, collection_id, data)
+        INSERT INTO records (id, parent_id, collection_id, data, last_modified)
         VALUES (:object_id, :parent_id,
-                :collection_id, (:data)::JSONB)
+                :collection_id, (:data)::JSONB,
+                from_epoch(:last_modified))
         RETURNING as_epoch(last_modified) AS last_modified;
         """
 
         query_update = """
-        UPDATE records SET data=(:data)::JSONB
+        UPDATE records SET data=(:data)::JSONB,
+                           last_modified=from_epoch(:last_modified)
         WHERE id = :object_id
            AND parent_id = :parent_id
            AND collection_id = :collection_id
@@ -283,6 +287,7 @@ def update(self, collection_id, parent_id, object_id, record,
         placeholders = dict(object_id=object_id,
                             parent_id=parent_id,
                             collection_id=collection_id,
+                            last_modified=record.get(modified_field),
                             data=json.dumps(record))
 
         record = record.copy()
@@ -312,7 +317,7 @@ def delete(self, collection_id, parent_id, object_id,
                id_field=DEFAULT_ID_FIELD, with_deleted=True,
                modified_field=DEFAULT_MODIFIED_FIELD,
                deleted_field=DEFAULT_DELETED_FIELD,
-               auth=None):
+               auth=None, last_modified=None):
         if with_deleted:
             query = """
             WITH deleted_record AS (
@@ -323,8 +328,8 @@ def delete(self, collection_id, parent_id, object_id,
                   AND collection_id = :collection_id
                 RETURNING id
             )
-            INSERT INTO deleted (id, parent_id, collection_id)
-            SELECT id, :parent_id, :collection_id
+            INSERT INTO deleted (id, parent_id, collection_id, last_modified)
+            SELECT id, :parent_id, :collection_id, from_epoch(:last_modified)
               FROM deleted_record
             RETURNING as_epoch(last_modified) AS last_modified;
             """
@@ -339,7 +344,8 @@ def delete(self, collection_id, parent_id, object_id,
             """
         placeholders = dict(object_id=object_id,
                             parent_id=parent_id,
-                            collection_id=collection_id)
+                            collection_id=collection_id,
+                            last_modified=last_modified)
 
         with self.client.connect() as conn:
             result = conn.execute(query, placeholders)

cliquet/storage/postgresql/migrations/migration_008_009.sql

@@ -0,0 +1,41 @@
+CREATE OR REPLACE FUNCTION from_epoch(epoch BIGINT) RETURNS TIMESTAMP AS $$
+BEGIN
+    RETURN TIMESTAMP WITH TIME ZONE 'epoch' + epoch * INTERVAL '1 millisecond';
+END;
+$$ LANGUAGE plpgsql
+IMMUTABLE;
+
+
+CREATE OR REPLACE FUNCTION bump_timestamp()
+RETURNS trigger AS $$
+DECLARE
+    previous TIMESTAMP;
+    current TIMESTAMP;
+
+BEGIN
+    --
+    -- This bumps the current timestamp to 1 msec in the future if the previous
+    -- timestamp is equal to the current one (or higher if was bumped already).
+    --
+    -- If a bunch of requests from the same user on the same collection
+    -- arrive in the same millisecond, the unicity constraint can raise
+    -- an error (operation is cancelled).
+    -- See https://github.com/mozilla-services/cliquet/issues/25
+    --
+    previous := collection_timestamp(NEW.parent_id, NEW.collection_id);
+
+    IF NEW.last_modified IS NULL THEN
+        current := clock_timestamp();
+        IF previous >= current THEN
+            current := previous + INTERVAL '1 milliseconds';
+        END IF;
+        NEW.last_modified := current;
+    END IF;
+
+    RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+
+
+-- Bump storage schema version.
+INSERT INTO metadata (name, value) VALUES ('storage_schema_version', '9');

cliquet/storage/postgresql/schema.sql

@@ -12,6 +12,13 @@ END;
 $$ LANGUAGE plpgsql
 IMMUTABLE;
 
+CREATE OR REPLACE FUNCTION from_epoch(epoch BIGINT) RETURNS TIMESTAMP AS $$
+BEGIN
+    RETURN TIMESTAMP WITH TIME ZONE 'epoch' + epoch * INTERVAL '1 millisecond';
+END;
+$$ LANGUAGE plpgsql
+IMMUTABLE;
+
 --
 -- Actual records
 --
@@ -161,6 +168,7 @@ RETURNS trigger AS $$
 DECLARE
     previous TIMESTAMP;
     current TIMESTAMP;
+
 BEGIN
     --
     -- This bumps the current timestamp to 1 msec in the future if the previous
@@ -172,14 +180,15 @@ BEGIN
     -- See https://github.com/mozilla-services/cliquet/issues/25
     --
     previous := collection_timestamp(NEW.parent_id, NEW.collection_id);
-    current := clock_timestamp();
 
-    IF previous >= current THEN
-        current := previous + INTERVAL '1 milliseconds';
+    IF NEW.last_modified IS NULL THEN
+        current := clock_timestamp();
+        IF previous >= current THEN
+            current := previous + INTERVAL '1 milliseconds';
+        END IF;
+        NEW.last_modified := current;
     END IF;
 
-    NEW.last_modified := current;
-
     RETURN NEW;
 END;
 $$ LANGUAGE plpgsql;

cliquet/storage/redis.py

@@ -88,19 +88,41 @@ def collection_timestamp(self, collection_id, parent_id, auth=None):
         return self._bump_timestamp(collection_id, parent_id)
 
     @wrap_redis_error
-    def _bump_timestamp(self, collection_id, parent_id):
+    def _bump_timestamp(self, collection_id, parent_id, record=None,
+                        modified_field=None, last_modified=None):
+
         key = '{0}.{1}.timestamp'.format(collection_id, parent_id)
         while 1:
             with self._client.pipeline() as pipe:
                 try:
                     pipe.watch(key)
                     previous = pipe.get(key)
                     pipe.multi()
-                    current = utils.msec_time()
+                    # XXX factorize code from memory and redis backends.
+                    is_specified = (record is not None
+                                    and modified_field in record
+                                    or last_modified is not None)
+                    if is_specified:
+                        # If there is a timestamp in the new record,
+                        # try to use it.
+                        if last_modified is not None:
+                            current = last_modified
+                        else:
+                            current = record[modified_field]
+                    else:
+                        current = utils.msec_time()
 
                     if previous and int(previous) >= current:
-                        current = int(previous) + 1
-                    pipe.set(key, current)
+                        collection_timestamp = int(previous) + 1
+                    else:
+                        collection_timestamp = current
+
+                    # Return the newly generated timestamp as the current one
+                    # only if nothing else was specified.
+                    if not is_specified:
+                        current = collection_timestamp
+
+                    pipe.set(key, collection_timestamp)
                     pipe.execute()
                     return current
                 except redis.WatchError:  # pragma: no cover
@@ -192,7 +214,7 @@ def delete(self, collection_id, parent_id, object_id,
                id_field=DEFAULT_ID_FIELD, with_deleted=True,
                modified_field=DEFAULT_MODIFIED_FIELD,
                deleted_field=DEFAULT_DELETED_FIELD,
-               auth=None):
+               auth=None, last_modified=None):
         record_key = '{0}.{1}.{2}.records'.format(collection_id,
                                                   parent_id,
                                                   object_id)
@@ -211,8 +233,12 @@ def delete(self, collection_id, parent_id, object_id,
 
         existing = self._decode(encoded_item)
 
+        # Need to delete the last_modified field.
+        del existing[modified_field]
+
         self.set_record_timestamp(collection_id, parent_id, existing,
-                                  modified_field=modified_field)
+                                  modified_field=modified_field,
+                                  last_modified=last_modified)
         existing = self.strip_deleted_record(collection_id, parent_id,
                                              existing)