From a8cdca6141e1451dc22d1af392a801cb7a2c7cab Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Wed, 13 Jul 2016 12:22:42 +0200 Subject: [PATCH] Merge pull request #715 from Kinto/duplicate_key_error Error if we create a record without deleting its tombstone --- CHANGELOG.rst | 1 + kinto/core/storage/memory.py | 1 + kinto/core/storage/postgresql/__init__.py | 8 +++- .../migrations/migration_011_012.sql | 9 ++++ kinto/core/storage/postgresql/schema.sql | 2 +- kinto/core/storage/redis.py | 4 ++ kinto/tests/core/test_storage.py | 10 ++++ kinto/tests/core/test_storage_migrations.py | 46 +++++++++++++++++++ 8 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 kinto/core/storage/postgresql/migrations/migration_011_012.sql diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 983f58011..1d57a0b48 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,6 +7,7 @@ This document describes changes between each past release. ================== - Fix bug in object permissions with memory backend (#708) +- Make sure the tombstone is deleted when the record is created with PUT. (#715) 3.2.1 (2016-07-04) ================== diff --git a/kinto/core/storage/memory.py b/kinto/core/storage/memory.py index f87de9f53..5546b28da 100644 --- a/kinto/core/storage/memory.py +++ b/kinto/core/storage/memory.py @@ -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..cf3c9a866 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 = 12 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/schema.sql b/kinto/core/storage/postgresql/schema.sql index e8c7fd1ae..d5c4020c6 100644 --- a/kinto/core/storage/postgresql/schema.sql +++ b/kinto/core/storage/postgresql/schema.sql @@ -241,4 +241,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', '12'); diff --git a/kinto/core/storage/redis.py b/kinto/core/storage/redis.py index 56d8094f7..f21cf9d5b 100644 --- a/kinto/core/storage/redis.py +++ b/kinto/core/storage/redis.py @@ -209,6 +209,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_storage.py b/kinto/tests/core/test_storage.py index 74fdbfd28..f3c8155d6 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 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):