Merge pull request Kinto#665 from mozilla-services/fix-behaviour-spec…

…ified-timestamps

Fix PG behaviour when specifying timestamps in the past

CHANGELOG.rst

@@ -27,6 +27,7 @@ This document describes changes between each past release.
 
 - Resource events are not emitted if the transaction is rolledback (e.g. a batch
   subrequest fails) (#634)
+- Fix a migration of PostgreSQL schema introduced in #604 that was never executed
 - Fix PostgreSQL backend timestamps when collection is empty (ref Kinto/kinto#433)
 
 **Internal changes**

cliquet/resource/__init__.py

@@ -511,6 +511,19 @@ def delete(self):
 
         # Retreive the last_modified information from a querystring if present.
         last_modified = self.request.GET.get('last_modified')
+        if last_modified:
+            last_modified = native_value(last_modified.strip('"'))
+            if not isinstance(last_modified, six.integer_types):
+                error_details = {
+                    'name': 'last_modified',
+                    'location': 'querystring',
+                    'description': 'Invalid value for %s' % last_modified
+                }
+                raise_invalid(self.request, **error_details)
+
+            # If less or equal than current record. Ignore it.
+            if last_modified <= record[self.model.modified_field]:
+                last_modified = None
 
         deleted = self.model.delete_record(record, last_modified=last_modified)
         return self.postprocess(deleted, action=ACTIONS.DELETE)
@@ -557,8 +570,8 @@ def process_record(self, new, old=None):
             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:
+        is_less_or_equal = new_last_modified <= old[self.model.modified_field]
+        if new_last_modified and is_less_or_equal:
             del new[self.model.modified_field]
 
         return new

cliquet/storage/postgresql/__init__.py

@@ -66,7 +66,7 @@ class Storage(StorageBase):
 
     """  # NOQA
 
-    schema_version = 9
+    schema_version = 10
 
     def __init__(self, client, max_fetch_size, *args, **kwargs):
         super(Storage, self).__init__(*args, **kwargs)
@@ -158,7 +158,7 @@ def _get_installed_version(self):
         SELECT value AS version
           FROM metadata
          WHERE name = 'storage_schema_version'
-         ORDER BY value DESC;
+         ORDER BY LPAD(value, 3, '0') DESC;
         """
         with self.client.connect() as conn:
             result = conn.execute(query)
@@ -184,6 +184,7 @@ def flush(self, auth=None):
         query = """
         DELETE FROM deleted;
         DELETE FROM records;
+        DELETE FROM timestamps;
         DELETE FROM metadata;
         """
         with self.client.connect(force_commit=True) as conn:

cliquet/storage/postgresql/migrations/migration_008_009.sql

@@ -1,32 +1,9 @@
-CREATE TABLE IF NOT EXISTS timestamps (
-  parent_id TEXT NOT NULL,
-  collection_id TEXT NOT NULL,
-  last_modified TIMESTAMP NOT NULL,
-  PRIMARY KEY (parent_id, collection_id)
-);
-
-
-CREATE OR REPLACE FUNCTION collection_timestamp(uid VARCHAR, resource VARCHAR)
-RETURNS TIMESTAMP AS $$
-DECLARE
-    ts TIMESTAMP;
+CREATE OR REPLACE FUNCTION from_epoch(epoch BIGINT) RETURNS TIMESTAMP AS $$
 BEGIN
-    ts := NULL;
-
-    SELECT last_modified INTO ts
-      FROM timestamps
-     WHERE parent_id = uid
-       AND collection_id = resource;
-
-    IF ts IS NULL THEN
-      ts := clock_timestamp();
-      INSERT INTO timestamps (parent_id, collection_id, last_modified)
-      VALUES (uid, resource, ts);
-    END IF;
-
-    RETURN ts;
+    RETURN TIMESTAMP WITH TIME ZONE 'epoch' + epoch * INTERVAL '1 millisecond';
 END;
-$$ LANGUAGE plpgsql;
+$$ LANGUAGE plpgsql
+IMMUTABLE;
 
 
 CREATE OR REPLACE FUNCTION bump_timestamp()
@@ -45,27 +22,16 @@ BEGIN
     -- 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
-        previous := collection_timestamp(NEW.parent_id, NEW.collection_id);
         current := clock_timestamp();
         IF previous >= current THEN
             current := previous + INTERVAL '1 milliseconds';
         END IF;
         NEW.last_modified := current;
     END IF;
 
-    --
-    -- Upsert current collection timestamp.
-    --
-    WITH upsert AS (
-        UPDATE timestamps SET last_modified = NEW.last_modified
-         WHERE parent_id = NEW.parent_id AND collection_id = NEW.collection_id
-        RETURNING *
-    )
-    INSERT INTO timestamps (parent_id, collection_id, last_modified)
-    SELECT NEW.parent_id, NEW.collection_id, NEW.last_modified
-    WHERE NOT EXISTS (SELECT * FROM upsert);
-
     RETURN NEW;
 END;
 $$ LANGUAGE plpgsql;

cliquet/storage/postgresql/migrations/migration_009_010.sql

@@ -0,0 +1,88 @@
+CREATE TABLE IF NOT EXISTS timestamps (
+  parent_id TEXT NOT NULL,
+  collection_id TEXT NOT NULL,
+  last_modified TIMESTAMP NOT NULL,
+  PRIMARY KEY (parent_id, collection_id)
+);
+
+
+CREATE OR REPLACE FUNCTION collection_timestamp(uid VARCHAR, resource VARCHAR)
+RETURNS TIMESTAMP AS $$
+DECLARE
+    ts TIMESTAMP;
+BEGIN
+    ts := NULL;
+
+    SELECT last_modified INTO ts
+      FROM timestamps
+     WHERE parent_id = uid
+       AND collection_id = resource;
+
+    IF ts IS NULL THEN
+      ts := clock_timestamp();
+      INSERT INTO timestamps (parent_id, collection_id, last_modified)
+      VALUES (uid, resource, ts);
+    END IF;
+
+    RETURN ts;
+END;
+$$ LANGUAGE plpgsql;
+
+
+CREATE OR REPLACE FUNCTION bump_timestamp()
+RETURNS trigger AS $$
+DECLARE
+    previous TIMESTAMP;
+    current TIMESTAMP;
+
+BEGIN
+    previous := NULL;
+    SELECT last_modified INTO previous
+      FROM timestamps
+     WHERE parent_id = NEW.parent_id
+       AND collection_id = NEW.collection_id;
+
+    --
+    -- 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
+    --
+    current := clock_timestamp();
+    IF previous IS NOT NULL AND previous >= current THEN
+        current := previous + INTERVAL '1 milliseconds';
+    END IF;
+
+
+    IF NEW.last_modified IS NULL THEN
+        -- If record does not carry last-modified, assign it to current.
+        NEW.last_modified := current;
+    ELSE
+        -- Use record last-modified as collection timestamp.
+        IF previous IS NULL OR NEW.last_modified > previous THEN
+            current := NEW.last_modified;
+        END IF;
+    END IF;
+
+    --
+    -- Upsert current collection timestamp.
+    --
+    WITH upsert AS (
+        UPDATE timestamps SET last_modified = current
+         WHERE parent_id = NEW.parent_id AND collection_id = NEW.collection_id
+        RETURNING *
+    )
+    INSERT INTO timestamps (parent_id, collection_id, last_modified)
+    SELECT NEW.parent_id, NEW.collection_id, current
+    WHERE NOT EXISTS (SELECT * FROM upsert);
+
+    RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+
+
+-- Bump storage schema version.
+INSERT INTO metadata (name, value) VALUES ('storage_schema_version', '10');

cliquet/storage/postgresql/schema.sql

@@ -174,6 +174,12 @@ DECLARE
     current TIMESTAMP;
 
 BEGIN
+    previous := NULL;
+    SELECT last_modified INTO previous
+      FROM timestamps
+     WHERE parent_id = NEW.parent_id
+       AND collection_id = NEW.collection_id;
+
     --
     -- 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).
@@ -183,25 +189,32 @@ BEGIN
     -- an error (operation is cancelled).
     -- See https://github.com/mozilla-services/cliquet/issues/25
     --
+    current := clock_timestamp();
+    IF previous IS NOT NULL AND previous >= current THEN
+        current := previous + INTERVAL '1 milliseconds';
+    END IF;
+
+
     IF NEW.last_modified IS NULL THEN
-        previous := collection_timestamp(NEW.parent_id, NEW.collection_id);
-        current := clock_timestamp();
-        IF previous >= current THEN
-            current := previous + INTERVAL '1 milliseconds';
-        END IF;
+        -- If record does not carry last-modified, assign it to current.
         NEW.last_modified := current;
+    ELSE
+        -- Use record last-modified as collection timestamp.
+        IF previous IS NULL OR NEW.last_modified > previous THEN
+            current := NEW.last_modified;
+        END IF;
     END IF;
 
     --
     -- Upsert current collection timestamp.
     --
     WITH upsert AS (
-        UPDATE timestamps SET last_modified = NEW.last_modified
+        UPDATE timestamps SET last_modified = current
          WHERE parent_id = NEW.parent_id AND collection_id = NEW.collection_id
         RETURNING *
     )
     INSERT INTO timestamps (parent_id, collection_id, last_modified)
-    SELECT NEW.parent_id, NEW.collection_id, NEW.last_modified
+    SELECT NEW.parent_id, NEW.collection_id, current
     WHERE NOT EXISTS (SELECT * FROM upsert);
 
     RETURN NEW;
@@ -228,4 +241,4 @@ INSERT INTO metadata (name, value) VALUES ('created_at', NOW()::TEXT);
 
 -- Set storage schema version.
 -- Should match ``cliquet.storage.postgresql.PostgreSQL.schema_version``
-INSERT INTO metadata (name, value) VALUES ('storage_schema_version', '9');
+INSERT INTO metadata (name, value) VALUES ('storage_schema_version', '10');

cliquet/tests/resource/test_record.py

@@ -147,7 +147,7 @@ def test_delete_uses_last_modified_from_querystring(self):
         last_modified = record[self.model.modified_field] + 20
         self.resource.record_id = record['id']
         self.resource.request.GET = {
-            'last_modified': last_modified
+            'last_modified': '%s' % last_modified
         }
 
         result = self.resource.delete()['data']
@@ -159,6 +159,36 @@ def test_delete_uses_last_modified_from_querystring(self):
         retrieved = result['data'][0]
         self.assertEqual(retrieved[self.model.modified_field], last_modified)
 
+    def test_delete_validates_the_last_modified_in_querystring(self):
+        record = self.model.create_record({'field': 'value'})
+        self.resource.record_id = record['id']
+        self.resource.request.GET = {'last_modified': 'abc'}
+        self.assertRaises(httpexceptions.HTTPBadRequest, self.resource.delete)
+
+    def test_delete_accepts_the_last_modified_between_quotes(self):
+        record = self.model.create_record({'field': 'value'})
+        last_modified = record[self.model.modified_field] + 20
+        self.resource.record_id = record['id']
+        self.resource.request.GET = {'last_modified': '"%s"' % last_modified}
+        result = self.resource.delete()['data']
+        self.assertEqual(result[self.model.modified_field], last_modified)
+
+    def test_delete_ignores_last_modified_if_equal(self):
+        record = self.model.create_record({'field': 'value'})
+        last_modified = record[self.model.modified_field]
+        self.resource.record_id = record['id']
+        self.resource.request.GET = {'last_modified': '%s' % last_modified}
+        result = self.resource.delete()['data']
+        self.assertGreater(result[self.model.modified_field], last_modified)
+
+    def test_delete_ignores_last_modified_if_less(self):
+        record = self.model.create_record({'field': 'value'})
+        last_modified = record[self.model.modified_field] - 20
+        self.resource.record_id = record['id']
+        self.resource.request.GET = {'last_modified': '%s' % last_modified}
+        result = self.resource.delete()['data']
+        self.assertGreater(result[self.model.modified_field], last_modified)
+
 
 class PatchTest(BaseTest):
     def setUp(self):