From 7a23c8a72e19b988bf79b26965cd80302bafb22d Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 23 Feb 2016 10:39:21 +0100 Subject: [PATCH 1/7] Fix migrations of PR #604 In PR #604, we updated the schema, but didn't bump its version. As a consequence, the migrations were never executed. In PR #649, we replaced the orphan migration with a new one about collection timestamps. This commit restores the migration of #604 and puts the one of #649 into a new one. --- cliquet/storage/postgresql/__init__.py | 2 +- .../migrations/migration_008_009.sql | 46 ++---------- .../migrations/migration_009_010.sql | 75 +++++++++++++++++++ cliquet/storage/postgresql/schema.sql | 2 +- 4 files changed, 83 insertions(+), 42 deletions(-) create mode 100644 cliquet/storage/postgresql/migrations/migration_009_010.sql diff --git a/cliquet/storage/postgresql/__init__.py b/cliquet/storage/postgresql/__init__.py index 83b49318a..ec56d26bd 100644 --- a/cliquet/storage/postgresql/__init__.py +++ b/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) diff --git a/cliquet/storage/postgresql/migrations/migration_008_009.sql b/cliquet/storage/postgresql/migrations/migration_008_009.sql index 2eb56043a..2fb40b7be 100644 --- a/cliquet/storage/postgresql/migrations/migration_008_009.sql +++ b/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,8 +22,9 @@ 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'; @@ -54,18 +32,6 @@ BEGIN 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; diff --git a/cliquet/storage/postgresql/migrations/migration_009_010.sql b/cliquet/storage/postgresql/migrations/migration_009_010.sql new file mode 100644 index 000000000..9c1b6406b --- /dev/null +++ b/cliquet/storage/postgresql/migrations/migration_009_010.sql @@ -0,0 +1,75 @@ +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 + -- + -- 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 + -- + 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; + + +-- Bump storage schema version. +INSERT INTO metadata (name, value) VALUES ('storage_schema_version', '10'); diff --git a/cliquet/storage/postgresql/schema.sql b/cliquet/storage/postgresql/schema.sql index 8f0eccfba..48bcc923e 100644 --- a/cliquet/storage/postgresql/schema.sql +++ b/cliquet/storage/postgresql/schema.sql @@ -228,4 +228,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'); From d33b3b83ca18acc76542634c1e4ee81e7374b103 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 23 Feb 2016 10:48:06 +0100 Subject: [PATCH 2/7] Fix PostgreSQL version detection --- cliquet/storage/postgresql/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cliquet/storage/postgresql/__init__.py b/cliquet/storage/postgresql/__init__.py index ec56d26bd..9c26c841b 100644 --- a/cliquet/storage/postgresql/__init__.py +++ b/cliquet/storage/postgresql/__init__.py @@ -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) From 805cf67c2e470579c223664a7e698f040333f612 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 23 Feb 2016 10:40:31 +0100 Subject: [PATCH 3/7] Fix PostgreSQL behaviour when a record was created/updated in the past. It had a different behaviour than other backends. --- cliquet/storage/postgresql/__init__.py | 1 + .../migrations/migration_009_010.sql | 27 +++- cliquet/storage/postgresql/schema.sql | 27 +++- cliquet/tests/test_storage.py | 133 ++++++++++-------- 4 files changed, 118 insertions(+), 70 deletions(-) diff --git a/cliquet/storage/postgresql/__init__.py b/cliquet/storage/postgresql/__init__.py index 9c26c841b..9f069619f 100644 --- a/cliquet/storage/postgresql/__init__.py +++ b/cliquet/storage/postgresql/__init__.py @@ -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: diff --git a/cliquet/storage/postgresql/migrations/migration_009_010.sql b/cliquet/storage/postgresql/migrations/migration_009_010.sql index 9c1b6406b..3aeadbf26 100644 --- a/cliquet/storage/postgresql/migrations/migration_009_010.sql +++ b/cliquet/storage/postgresql/migrations/migration_009_010.sql @@ -36,6 +36,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). @@ -45,25 +51,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; diff --git a/cliquet/storage/postgresql/schema.sql b/cliquet/storage/postgresql/schema.sql index 48bcc923e..bbb5238ad 100644 --- a/cliquet/storage/postgresql/schema.sql +++ b/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; diff --git a/cliquet/tests/test_storage.py b/cliquet/tests/test_storage.py index c874d2f54..2b7631939 100644 --- a/cliquet/tests/test_storage.py +++ b/cliquet/tests/test_storage.py @@ -253,21 +253,6 @@ def test_create_raise_unicity_error_if_provided_id_exists(self): self.create_record, record=record) - def test_create_preserves_the_passed_last_modified_when_provided(self): - last_modified = 1448881675541 - record = self.record.copy() - record[self.id_field] = RECORD_ID - record[self.modified_field] = last_modified - self.create_record(record=record) - - retrieved = self.storage.get(object_id=RECORD_ID, **self.storage_kw) - self.assertIn(self.modified_field, retrieved) - self.assertEquals(retrieved[self.modified_field], last_modified) - - # collection timestamp should not be modified. - collection_ts = self.storage.collection_timestamp(**self.storage_kw) - self.assertEquals(collection_ts, last_modified) - def test_create_does_generate_a_new_last_modified_field(self): record = self.record.copy() self.assertNotIn(self.modified_field, record) @@ -316,47 +301,6 @@ def test_update_generates_a_new_last_modified_field_if_not_present(self): self.assertGreater(retrieved[self.modified_field], stored[self.modified_field]) - def test_update_preserves_the_passed_last_modified_when_provided(self): - stored = self.create_record() - record_id = stored[self.id_field] - record = self.record.copy() - record[self.modified_field] = stored[self.modified_field] + 10 - - self.storage.update(object_id=record_id, record=record, - **self.storage_kw) - retrieved = self.storage.get(object_id=record_id, **self.storage_kw) - self.assertIn(self.modified_field, retrieved) - self.assertEquals(retrieved[self.modified_field], - record[self.modified_field]) - # collection timestamp should not be modified. - collection_timestamp = self.storage.collection_timestamp( - **self.storage_kw) - self.assertEquals(collection_timestamp, record[self.modified_field]) - - def test_update_preserves_record_timestamp_if_less_than_collection(self): - first_record = self.create_record() - second_record = self.create_record() - - # Update the second record to have a last_modified of t0-10 - record_id = second_record[self.id_field] - record = self.record.copy() - record[self.modified_field] = second_record[self.modified_field] - 10 - self.storage.update(object_id=record_id, record=record, - **self.storage_kw) - - # Retrieve back the updated record. - retrieved = self.storage.get(object_id=record_id, **self.storage_kw) - self.assertIn(self.modified_field, retrieved) - self.assertEquals(retrieved[self.modified_field], - record[self.modified_field]) - - # The collection timestamp should be bumped in this case. - collection_timestamp = self.storage.collection_timestamp( - **self.storage_kw) - self.assertNotEquals( - collection_timestamp, - first_record[self.modified_field]) - def test_delete_works_properly(self): stored = self.create_record() self.storage.delete(object_id=stored['id'], **self.storage_kw) @@ -553,6 +497,83 @@ def test_timestamp_are_always_incremented_above_existing_value(self): self.assertTrue(0 < current < after, '0 < %s < %s' % (current, after)) + def test_create_uses_specified_last_modified_if_collection_empty(self): + # Collection is empty, create a new record with a specified timestamp. + last_modified = 1448881675541 + record = self.record.copy() + record[self.id_field] = RECORD_ID + record[self.modified_field] = last_modified + self.create_record(record=record) + + # Check that the record was assigned the specified timestamp. + retrieved = self.storage.get(object_id=RECORD_ID, **self.storage_kw) + self.assertEquals(retrieved[self.modified_field], last_modified) + + # Collection timestamp is now the same as its only record. + collection_ts = self.storage.collection_timestamp(**self.storage_kw) + self.assertEquals(collection_ts, last_modified) + + def test_create_ignores_specified_last_modified_if_in_the_past(self): + # Create a first record, and get the timestamp. + first_record = self.create_record() + timestamp_before = first_record[self.modified_field] + + # Create a new record with its timestamp in the past. + record = self.record.copy() + record[self.id_field] = RECORD_ID + record[self.modified_field] = timestamp_before - 10 + self.create_record(record=record) + + # Check that record timestamp is the one specified. + retrieved = self.storage.get(object_id=RECORD_ID, **self.storage_kw) + self.assertLess(retrieved[self.modified_field], timestamp_before) + self.assertEquals(retrieved[self.modified_field], + record[self.modified_field]) + + # Check that collection timestamp was bumped (change happened). + timestamp = self.storage.collection_timestamp(**self.storage_kw) + self.assertGreater(timestamp, timestamp_before) + + def test_update_uses_specified_last_modified_if_in_future(self): + stored = self.create_record() + record_id = stored[self.id_field] + timestamp_before = stored[self.modified_field] + + # Set timestamp manually in the future. + stored[self.modified_field] = timestamp_before + 10 + self.storage.update(object_id=record_id, record=stored, + **self.storage_kw) + + # Check that record timestamp is the one specified. + retrieved = self.storage.get(object_id=record_id, **self.storage_kw) + self.assertGreater(retrieved[self.modified_field], timestamp_before) + self.assertEquals(retrieved[self.modified_field], + stored[self.modified_field]) + + # Check that collection timestamp took the one specified (in future). + timestamp = self.storage.collection_timestamp(**self.storage_kw) + self.assertGreater(timestamp, timestamp_before) + + def test_update_ignores_specified_last_modified_if_in_the_past(self): + stored = self.create_record() + record_id = stored[self.id_field] + timestamp_before = stored[self.modified_field] + + # Set timestamp manually in the future. + stored[self.modified_field] = timestamp_before - 10 + self.storage.update(object_id=record_id, record=stored, + **self.storage_kw) + + # Check that record timestamp is the one specified. + retrieved = self.storage.get(object_id=record_id, **self.storage_kw) + self.assertLess(retrieved[self.modified_field], timestamp_before) + self.assertEquals(retrieved[self.modified_field], + stored[self.modified_field]) + + # Check that collection timestamp was bumped (change happened). + timestamp = self.storage.collection_timestamp(**self.storage_kw) + self.assertGreater(timestamp, timestamp_before) + class FieldsUnicityTest(object): def test_does_not_fail_if_no_unique_fields_at_all(self): From 9ee6354cc8064fa70415d0c12cf925439501dd24 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 23 Feb 2016 10:53:26 +0100 Subject: [PATCH 4/7] Update changelog --- CHANGELOG.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6ed1bdd97..9e498f7d1 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -18,6 +18,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** From 01157f80f639d20bea979549d81189b6e9e8c6e0 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 23 Feb 2016 12:26:31 +0100 Subject: [PATCH 5/7] Clarify docs about timestamps --- cliquet_docs/api/resource.rst | 76 +++++++++++++++++++++++++++++---- cliquet_docs/api/timestamps.rst | 17 ++++++++ 2 files changed, 85 insertions(+), 8 deletions(-) diff --git a/cliquet_docs/api/resource.rst b/cliquet_docs/api/resource.rst index 0205e1a3f..c812cd574 100644 --- a/cliquet_docs/api/resource.rst +++ b/cliquet_docs/api/resource.rst @@ -424,6 +424,18 @@ A ``details`` attribute in the response provides the offending record and field name. See :ref:`dedicated section about errors `. +Timestamp +--------- + +When a record is created, the timestamp of the collection is incremented. + +It is possible to force the timestamp if the specified record has a +``last_modified`` attribute. + +The specified value will be ignored if it is less than the current collection +timestamp. + + HTTP Status Codes ----------------- @@ -434,6 +446,10 @@ HTTP Status Codes * ``409 Conflict``: Unicity constraint on fields is violated * ``412 Precondition Failed``: Collection changed since value in ``If-Match`` header +.. versionadded:: 2.13:: + + Enforcement of the timestamp value for records has been added. + DELETE /{collection} ==================== @@ -567,17 +583,23 @@ The consumer might decide to ignore it. If the request header ``If-Match`` is provided, and if the record has changed meanwhile, a ``412 Precondition failed`` error is returned. -The timestamp value of the deleted record can be enforced via the -``last_modified`` QueryString parameter. - .. note:: Once deleted, a record will appear in the collection when polling for changes, with a deleted status (``delete=true``) and will have most of its fields empty. -.. versionadded:: 2.13:: - Enforcement of the timestamp value for records has been added. +Timestamp +--------- + +When a record is deleted, the timestamp of the collection is incremented. + +It is possible to force the timestamp by passing it in the +querystring with ``?last_modified=``. + +The specified value will be ignored if it is less than the current collection +timestamp. + HTTP Status Code ---------------- @@ -585,6 +607,10 @@ HTTP Status Code * ``200 OK``: The record was deleted * ``412 Precondition Failed``: Record changed since value in ``If-Match`` header +.. versionadded:: 2.13:: + + Enforcement of the timestamp value for records has been added. + PUT /{collection}/ ====================== @@ -608,9 +634,6 @@ Validation and conflicts behaviour is similar to creating records (``POST``). If the request header ``If-Match`` is provided, and if the record has changed meanwhile, a ``412 Precondition failed`` error is returned. -.. versionadded:: 2.13:: - - Enforcement of the timestamp value for records has been added. **Request**: @@ -651,6 +674,20 @@ changed meanwhile, a ``412 Precondition failed`` error is returned. } +Timestamp +--------- + +When a record is created or replaced, the timestamp of the collection is incremented. + +It is possible to force the timestamp if the specified record has a +``last_modified`` attribute. + +The specified value will be ignored if: + +* it is less than the current collection timestamp; +* it is less or equal than the previously existing record timestamp (for replace). + + HTTP Status Code ---------------- @@ -666,6 +703,10 @@ HTTP Status Code A ``If-None-Match: *`` request header can be used to make sure the ``PUT`` won't overwrite any record. +.. versionadded:: 2.13:: + + Enforcement of the timestamp value for records has been added. + PATCH /{collection}/ ======================== @@ -758,6 +799,21 @@ If changing a record field violates a field unicity constraint, a ``409 Conflict`` error response is returned (see :ref:`error channel `). +Timestamp +--------- + +When a record is modified, the timestamp of the collection is incremented. + +It is possible to force the timestamp if the specified record has a +``last_modified`` attribute. + + +The specified value will be ignored if: + +* it is less than the current collection timestamp; +* it is less or equal than the previously existing record timestamp. + + HTTP Status Code ---------------- @@ -767,6 +823,10 @@ HTTP Status Code * ``409 Conflict``: If modifying this record violates a field unicity constraint * ``412 Precondition Failed``: Record changed since value in ``If-Match`` header +.. versionadded:: 2.13:: + + Enforcement of the timestamp value for records has been added. + .. _resource-permissions-attribute: diff --git a/cliquet_docs/api/timestamps.rst b/cliquet_docs/api/timestamps.rst index def37315d..9320929e7 100644 --- a/cliquet_docs/api/timestamps.rst +++ b/cliquet_docs/api/timestamps.rst @@ -68,3 +68,20 @@ The client can then choose to: * overwrite by repeating the request without ``If-Match``; * reconcile the resource by fetching, merging and repeating the request. + + +Replication +=========== + +In order to replicate the timestamps when importing existing records, +it is possible to force the last modified values. + +When a timestamp is specified, it should be strictly greater than the current +collection timestamp. + +It will be ignored if: + +* it is less than the current collection timestamp; +* it is less or equal than previously existing record timestamp (in case of replacement or modification). + +See :ref:`the resource endpoints documentation `. \ No newline at end of file From 446d9ba399987afc4c0114b0bd25c2ca43fe88ea Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 26 Feb 2016 10:54:25 +0100 Subject: [PATCH 6/7] @almet review --- cliquet/resource/__init__.py | 4 ++-- cliquet_docs/api/resource.rst | 22 ++++++++++------------ cliquet_docs/api/timestamps.rst | 15 ++++++++------- 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/cliquet/resource/__init__.py b/cliquet/resource/__init__.py index 2be81505b..aec271dec 100644 --- a/cliquet/resource/__init__.py +++ b/cliquet/resource/__init__.py @@ -558,8 +558,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 diff --git a/cliquet_docs/api/resource.rst b/cliquet_docs/api/resource.rst index c812cd574..d6d6de0e3 100644 --- a/cliquet_docs/api/resource.rst +++ b/cliquet_docs/api/resource.rst @@ -432,8 +432,8 @@ When a record is created, the timestamp of the collection is incremented. It is possible to force the timestamp if the specified record has a ``last_modified`` attribute. -The specified value will be ignored if it is less than the current collection -timestamp. +If the specified timestamp is in the past, the collection timestamp does not +take the value of the created record but is bumped into the future as usual. HTTP Status Codes @@ -597,8 +597,8 @@ When a record is deleted, the timestamp of the collection is incremented. It is possible to force the timestamp by passing it in the querystring with ``?last_modified=``. -The specified value will be ignored if it is less than the current collection -timestamp. +If the specified timestamp is in the past, the collection timestamp does not +take the value of the deleted record but is bumped into the future as usual. HTTP Status Code @@ -682,10 +682,11 @@ When a record is created or replaced, the timestamp of the collection is increme It is possible to force the timestamp if the specified record has a ``last_modified`` attribute. -The specified value will be ignored if: +For replace, if the specified timestamp is less or equal than the existing record, +the value is simply ignored and the timestamp is bumped into the future as usual. -* it is less than the current collection timestamp; -* it is less or equal than the previously existing record timestamp (for replace). +For creation, if the specified timestamp is in the past, the collection timestamp does not +take the value of the created/updated record but is bumped into the future as usual. HTTP Status Code @@ -807,11 +808,8 @@ When a record is modified, the timestamp of the collection is incremented. It is possible to force the timestamp if the specified record has a ``last_modified`` attribute. - -The specified value will be ignored if: - -* it is less than the current collection timestamp; -* it is less or equal than the previously existing record timestamp. +If the specified timestamp is less or equal than the existing record, +the value is simply ignored and the timestamp is bumped into the future as usual. HTTP Status Code diff --git a/cliquet_docs/api/timestamps.rst b/cliquet_docs/api/timestamps.rst index 9320929e7..e4eb8b457 100644 --- a/cliquet_docs/api/timestamps.rst +++ b/cliquet_docs/api/timestamps.rst @@ -76,12 +76,13 @@ Replication In order to replicate the timestamps when importing existing records, it is possible to force the last modified values. -When a timestamp is specified, it should be strictly greater than the current -collection timestamp. +When a record is created (via POST or PUT), the specified timestamp becomes +the new collection timestamp if it is in the future (i.e. greater than current +one). If it is in the past, the record is created with the timestamp in the past +but the collection timestamp is bumped into the future as usual. -It will be ignored if: +When a record is replaced, modified or deleted, if the specified timestamp is less +or equal than the existing record, the value is simply ignored and the timestamp +is bumped into the future as usual. -* it is less than the current collection timestamp; -* it is less or equal than previously existing record timestamp (in case of replacement or modification). - -See :ref:`the resource endpoints documentation `. \ No newline at end of file +See :ref:`the resource endpoints documentation `. From 2ac41d6e88ba5201f525443d003c005dd98be08d Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 26 Feb 2016 11:05:57 +0100 Subject: [PATCH 7/7] Fix inconsistencies with DELETE endpoint aand timestamps --- cliquet/resource/__init__.py | 13 +++++++++++ cliquet/tests/resource/test_record.py | 32 ++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/cliquet/resource/__init__.py b/cliquet/resource/__init__.py index aec271dec..2a7eb04f2 100644 --- a/cliquet/resource/__init__.py +++ b/cliquet/resource/__init__.py @@ -512,6 +512,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) diff --git a/cliquet/tests/resource/test_record.py b/cliquet/tests/resource/test_record.py index c6f056c4d..83348b4f4 100644 --- a/cliquet/tests/resource/test_record.py +++ b/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):