Merge pull request Kinto#715 from Kinto/duplicate_key_error

Error if we create a record without deleting its tombstone

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)
 ==================

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,

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,

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');

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');

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

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

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):