diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 252dd7c71..fafc55d06 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -3,10 +3,14 @@ Changelog This document describes changes between each past release. -3.2.2 (unreleased) +3.2.2 (2016-07-13) ================== -- Nothing changed yet. +- Fix bug in object permissions with memory backend (#708) +- Make sure the tombstone is deleted when the record is created with PUT. (#715) +- Bump ``last_modified`` on record when provided value is equal to previous + in storage ``update()`` method (#713) + 3.2.1 (2016-07-04) ================== diff --git a/docs/conf.py b/docs/conf.py index aa5e9d04e..ac390e4d3 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -74,7 +74,7 @@ # The short X.Y version. version = '3.2' # The full version, including alpha/beta/rc tags. -release = '3.2.1' +release = '3.2.2' # List of patterns, relative to source directory, that match files and # directories to ignore when looking for source files. diff --git a/kinto/core/permission/memory.py b/kinto/core/permission/memory.py index fa2fd3ac7..c90be9b14 100644 --- a/kinto/core/permission/memory.py +++ b/kinto/core/permission/memory.py @@ -118,7 +118,7 @@ def get_authorized_principals(self, object_id, permission, def get_object_permissions(self, object_id, permissions=None): if permissions is None: aces = [k for k in self._store.keys() - if k.startswith('permission:%s' % object_id)] + if k.startswith('permission:%s:' % object_id)] else: aces = ['permission:%s:%s' % (object_id, permission) for permission in permissions] diff --git a/kinto/core/storage/memory.py b/kinto/core/storage/memory.py index f87de9f53..89f01fbdf 100644 --- a/kinto/core/storage/memory.py +++ b/kinto/core/storage/memory.py @@ -208,7 +208,7 @@ def _bump_timestamp(self, collection_id, parent_id, record=None, # 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: + if not is_specified or previous == current: current = collection_timestamp self._timestamps[collection_id][parent_id] = collection_timestamp @@ -254,6 +254,7 @@ def update(self, collection_id, parent_id, object_id, record, self.set_record_timestamp(collection_id, parent_id, record, modified_field=modified_field) self._store[collection_id][parent_id][object_id] = record + self._cemetery[collection_id][parent_id].pop(object_id, None) return record def delete(self, collection_id, parent_id, object_id, diff --git a/kinto/core/storage/postgresql/__init__.py b/kinto/core/storage/postgresql/__init__.py index d89916f20..324ee1a1d 100644 --- a/kinto/core/storage/postgresql/__init__.py +++ b/kinto/core/storage/postgresql/__init__.py @@ -66,7 +66,7 @@ class Storage(StorageBase): """ # NOQA - schema_version = 11 + schema_version = 13 def __init__(self, client, max_fetch_size, *args, **kwargs): super(Storage, self).__init__(*args, **kwargs) @@ -270,6 +270,12 @@ def update(self, collection_id, parent_id, object_id, record, modified_field=DEFAULT_MODIFIED_FIELD, auth=None): query_create = """ + WITH delete_potential_tombstone AS ( + DELETE FROM deleted + WHERE id = :object_id + AND parent_id = :parent_id + AND collection_id = :collection_id + ) INSERT INTO records (id, parent_id, collection_id, data, last_modified) VALUES (:object_id, :parent_id, :collection_id, (:data)::JSONB, diff --git a/kinto/core/storage/postgresql/migrations/migration_011_012.sql b/kinto/core/storage/postgresql/migrations/migration_011_012.sql new file mode 100644 index 000000000..5806b498c --- /dev/null +++ b/kinto/core/storage/postgresql/migrations/migration_011_012.sql @@ -0,0 +1,9 @@ +-- Select all existing records and delete their tombstone if any. +DELETE FROM deleted d +USING records r +WHERE d.id = r.id +AND d.parent_id = r.parent_id +AND d.collection_id = r.collection_id; + +-- Bump storage schema version. +INSERT INTO metadata (name, value) VALUES ('storage_schema_version', '12'); diff --git a/kinto/core/storage/postgresql/migrations/migration_012_013.sql b/kinto/core/storage/postgresql/migrations/migration_012_013.sql new file mode 100644 index 000000000..eb74c1039 --- /dev/null +++ b/kinto/core/storage/postgresql/migrations/migration_012_013.sql @@ -0,0 +1,71 @@ +-- +-- Triggers to set last_modified on INSERT/UPDATE +-- +DROP TRIGGER IF EXISTS tgr_records_last_modified ON records; +DROP TRIGGER IF EXISTS tgr_deleted_last_modified ON deleted; + +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 OR + (previous IS NOT NULL AND as_epoch(NEW.last_modified) = as_epoch(previous)) THEN + -- If record does not carry last-modified, or if the one specified + -- is equal to previous, assign it to current (i.e. bump it). + 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; + +CREATE TRIGGER tgr_records_last_modified +BEFORE INSERT OR UPDATE ON records +FOR EACH ROW EXECUTE PROCEDURE bump_timestamp(); + +CREATE TRIGGER tgr_deleted_last_modified +BEFORE INSERT OR UPDATE ON deleted +FOR EACH ROW EXECUTE PROCEDURE bump_timestamp(); + +-- Bump storage schema version. +INSERT INTO metadata (name, value) VALUES ('storage_schema_version', '13'); diff --git a/kinto/core/storage/postgresql/schema.sql b/kinto/core/storage/postgresql/schema.sql index e8c7fd1ae..01d4191d4 100644 --- a/kinto/core/storage/postgresql/schema.sql +++ b/kinto/core/storage/postgresql/schema.sql @@ -194,9 +194,10 @@ BEGIN 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. + IF NEW.last_modified IS NULL OR + (previous IS NOT NULL AND as_epoch(NEW.last_modified) = as_epoch(previous)) THEN + -- If record does not carry last-modified, or if the one specified + -- is equal to previous, assign it to current (i.e. bump it). NEW.last_modified := current; ELSE -- Use record last-modified as collection timestamp. @@ -241,4 +242,4 @@ INSERT INTO metadata (name, value) VALUES ('created_at', NOW()::TEXT); -- Set storage schema version. -- Should match ``kinto.core.storage.postgresql.PostgreSQL.schema_version`` -INSERT INTO metadata (name, value) VALUES ('storage_schema_version', '11'); +INSERT INTO metadata (name, value) VALUES ('storage_schema_version', '13'); diff --git a/kinto/core/storage/redis.py b/kinto/core/storage/redis.py index 56d8094f7..a12e9ad1b 100644 --- a/kinto/core/storage/redis.py +++ b/kinto/core/storage/redis.py @@ -123,7 +123,8 @@ def _bump_timestamp(self, collection_id, parent_id, record=None, # Return the newly generated timestamp as the current one # only if nothing else was specified. - if not is_specified: + is_equal = previous and int(previous) == current + if not is_specified or is_equal: current = collection_timestamp pipe.set(key, collection_timestamp) @@ -209,6 +210,10 @@ def update(self, collection_id, parent_id, object_id, record, '{0}.{1}.records'.format(collection_id, parent_id), object_id ) + multi.srem( + '{0}.{1}.deleted'.format(collection_id, parent_id), + object_id + ) multi.execute() return record diff --git a/kinto/tests/core/test_permission.py b/kinto/tests/core/test_permission.py index de0766136..e0417491c 100644 --- a/kinto/tests/core/test_permission.py +++ b/kinto/tests/core/test_permission.py @@ -342,6 +342,7 @@ def test_object_permissions_return_all_object_acls(self): self.permission.add_principal_to_ace('/url/a/id/1', 'write', 'user2') self.permission.add_principal_to_ace('/url/a/id/1', 'read', 'user3') self.permission.add_principal_to_ace('/url/a/id/1', 'obj:del', 'user1') + self.permission.add_principal_to_ace('/url/a/id/1/sub', 'create', 'me') permissions = self.permission.get_object_permissions('/url/a/id/1') self.assertDictEqual(permissions, { "write": {"user1", "user2"}, diff --git a/kinto/tests/core/test_storage.py b/kinto/tests/core/test_storage.py index 74fdbfd28..664f33076 100644 --- a/kinto/tests/core/test_storage.py +++ b/kinto/tests/core/test_storage.py @@ -311,6 +311,16 @@ def test_delete_works_properly(self): **self.storage_kw ) + def test_delete_works_even_on_second_time(self): + # Create a record + self.storage.create('test', '1234', {"id": "demo"}) + # Delete the record + self.storage.delete('test', '1234', "demo", with_deleted=True) + # Update a record (it recreates it.) + self.storage.update('test', '1234', "demo", {"id": "demo"}) + # Delete the record without errors + self.storage.delete('test', '1234', "demo", with_deleted=True) + def test_delete_can_specify_the_last_modified(self): stored = self.create_record() last_modified = stored[self.modified_field] + 10 @@ -580,6 +590,27 @@ def test_create_ignores_specified_last_modified_if_in_the_past(self): timestamp = self.storage.collection_timestamp(**self.storage_kw) self.assertGreater(timestamp, timestamp_before) + def test_create_ignores_specified_last_modified_if_equal(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 + 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.assertGreater(retrieved[self.modified_field], timestamp_before) + self.assertGreater(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] @@ -620,6 +651,25 @@ def test_update_ignores_specified_last_modified_if_in_the_past(self): timestamp = self.storage.collection_timestamp(**self.storage_kw) self.assertGreater(timestamp, timestamp_before) + def test_update_ignores_specified_last_modified_if_equal(self): + stored = self.create_record() + record_id = stored[self.id_field] + timestamp_before = stored[self.modified_field] + + # Do not change the timestamp. + self.storage.update(object_id=record_id, record=stored, + **self.storage_kw) + + # Check that record timestamp was bumped. + retrieved = self.storage.get(object_id=record_id, **self.storage_kw) + self.assertGreater(retrieved[self.modified_field], timestamp_before) + self.assertGreater(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): diff --git a/kinto/tests/core/test_storage_migrations.py b/kinto/tests/core/test_storage_migrations.py index f85ed0907..5218654f9 100644 --- a/kinto/tests/core/test_storage_migrations.py +++ b/kinto/tests/core/test_storage_migrations.py @@ -151,6 +151,52 @@ def test_every_available_migration_succeeds_if_tables_were_flushed(self): version = self.storage._get_installed_version() self.assertEqual(version, self.version) + def test_migration_12_clean_tombstones(self): + self._delete_everything() + postgresql.Storage.schema_version = 11 + self.storage.initialize_schema() + # Set the schema version back to 11 in the base as well + with self.storage.client.connect() as conn: + query = """ + UPDATE metadata SET value = '11' + WHERE name = 'storage_schema_version'; + """ + conn.execute(query) + r = self.storage.create('test', 'jean-louis', {'drink': 'mate'}) + self.storage.delete('test', 'jean-louis', r['id']) + + # Insert back the record without removing the tombstone. + with self.storage.client.connect() as conn: + query = """ + INSERT INTO records (id, parent_id, collection_id, + data, last_modified) + VALUES (:id, :parent_id, :collection_id, + (:data)::JSONB, from_epoch(:last_modified)); + """ + placeholders = dict(id=r['id'], + collection_id='test', + parent_id='jean-louis', + data=json.dumps({'drink': 'mate'}), + last_modified=1468400666777) + conn.execute(query, placeholders) + + records, count = self.storage.get_all('test', 'jean-louis', + include_deleted=True) + # Check that we have the tombstone + assert len(records) == 2 + assert count == 1 + + # Execute the 011 to 012 migration + postgresql.Storage.schema_version = 12 + self.storage.initialize_schema() + + # Check that the rotted tombstone have been removed. + records, count = self.storage.get_all('test', 'jean-louis', + include_deleted=True) + # Only the record remains. + assert len(records) == 1 + assert count == 1 + class PostgresqlExceptionRaisedTest(unittest.TestCase): def setUp(self): diff --git a/requirements.txt b/requirements.txt index 91cf35d42..929630ff8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -9,7 +9,7 @@ iso8601==0.1.11 jsonschema==2.5.1 newrelic==2.66.0.49 PasteDeploy==1.5.2 -psycopg2==2.6.1 +psycopg2==2.6.2 pyramid==1.7 pyramid-multiauth==0.8.0 pyramid-tm==0.12.1 @@ -20,10 +20,10 @@ repoze.lru==0.6 requests==2.10.0 simplejson==3.8.2 six==1.10.0 -SQLAlchemy==1.0.13 +SQLAlchemy==1.0.14 statsd==3.2.1 structlog==16.1.0 -transaction==1.6.0 +transaction==1.6.1 translationstring==1.3 ujson==1.35 venusian==1.0 @@ -32,4 +32,4 @@ WebOb==1.6.1 Werkzeug==0.11.10 zope.deprecation==4.1.2 zope.interface==4.1.3 -zope.sqlalchemy==0.7.6 +zope.sqlalchemy==0.7.7 diff --git a/setup.py b/setup.py index 9a08ca77b..3b38e2eb9 100644 --- a/setup.py +++ b/setup.py @@ -71,7 +71,7 @@ def read_file(filename): setup(name='kinto', - version='3.2.1', + version='3.2.2', description='Kinto Web Service - Store, Sync, Share, and Self-Host.', long_description=README + "\n\n" + CHANGELOG + "\n\n" + CONTRIBUTORS, license='Apache License (2.0)',