Merge pull request #1984 from Kinto/1981-deleted-tombstones

 Fix bumping of tombstones timestamps in PostgreSQL storage backend  (fixes #1981)

CHANGELOG.rst

@@ -6,7 +6,8 @@ This document describes changes between each past release.
 12.1.0 (unreleased)
 -------------------
 
-- Nothing changed yet.
+- Fix bumping of tombstones timestamps when deleting objects in PostgreSQL storage backend (fixes #1981)
+- Fix ETag header in responses of DELETE on plural endpoints (ref #1981)
 
 
 12.0.0 (2019-01-10)

kinto/core/resource/__init__.py

@@ -471,14 +471,14 @@ def plural_delete(self):
         )
         if deleted:
             lastobject = deleted[-1]
-            # Get timestamp of the last deleted field
-            timestamp = lastobject[self.model.modified_field]
-            self._add_timestamp_header(self.request.response, timestamp=timestamp)
-
             # Add pagination header, but only if there are more objects beyond the limit.
             if limit and len(objects) == limit + 1:
                 next_page = self._next_page_url(sorting, limit, lastobject, offset)
                 self.request.response.headers["Next-Page"] = next_page
+
+            timestamp = max({d[self.model.modified_field] for d in deleted})
+            self._add_timestamp_header(self.request.response, timestamp=timestamp)
+
         else:
             self._add_timestamp_header(self.request.response)
 

kinto/core/storage/postgresql/__init__.py

@@ -445,10 +445,10 @@ def delete(
             result = conn.execute(query, placeholders)
             if result.rowcount == 0:
                 raise exceptions.ObjectNotFoundError(object_id)
-            inserted = result.fetchone()
+            updated = result.fetchone()
 
         obj = {}
-        obj[modified_field] = inserted["last_modified"]
+        obj[modified_field] = updated["last_modified"]
         obj[id_field] = object_id
 
         obj[deleted_field] = True
@@ -484,7 +484,7 @@ def delete_all(
                     FOR UPDATE
             )
             UPDATE objects
-               SET deleted=TRUE, data=(:deleted_data)::JSONB
+               SET deleted=TRUE, data=(:deleted_data)::JSONB, last_modified=NULL
               FROM matching_objects
              WHERE objects.id = matching_objects.id
                AND objects.parent_id = matching_objects.parent_id

kinto/core/storage/testing.py

@@ -1125,10 +1125,43 @@ def test_deleting_without_tombstone_should_raise_not_found(self):
     def test_delete_all_deletes_objects(self):
         self.create_object()
         self.create_object()
+
         self.storage.delete_all(**self.storage_kw)
+
         count = self.storage.count_all(**self.storage_kw)
         self.assertEqual(count, 0)
 
+    def test_delete_all_bumps_collection_timestamp(self):
+        self.create_object()
+        self.create_object()
+        timestamp_before = self.storage.resource_timestamp(**self.storage_kw)
+
+        self.storage.delete_all(**self.storage_kw)
+
+        timestamp_after = self.storage.resource_timestamp(**self.storage_kw)
+        self.assertNotEqual(timestamp_after, timestamp_before)
+
+    def test_delete_all_keeps_tombstones(self):
+        self.create_object()
+        self.create_object()
+
+        self.storage.delete_all(**self.storage_kw)
+
+        self.assertEqual(len(self.storage.list_all(include_deleted=True, **self.storage_kw)), 2)
+
+    def test_delete_all_bumps_tombstones_timestamps(self):
+        self.create_object()
+        self.create_object()
+        timestamps_before = {r["last_modified"] for r in self.storage.list_all(**self.storage_kw)}
+
+        self.storage.delete_all(**self.storage_kw)
+
+        timestamps_after = {
+            r["last_modified"]
+            for r in self.storage.list_all(include_deleted=True, **self.storage_kw)
+        }
+        self.assertTrue(timestamps_after.isdisjoint(timestamps_before))
+
     def test_delete_all_can_delete_by_parent_id(self):
         self.create_object(parent_id="abc", resource_name="c")
         self.create_object(parent_id="abc", resource_name="c")

tests/core/resource/test_base.py

@@ -67,7 +67,8 @@ def setUp(self):
 
         req = DummyRequest()
         req.validated = {"body": {}, "header": {}, "querystring": {}}
-        req.registry.storage.get_all.return_value = ([], 0)
+        req.registry.storage.list_all.return_value = []
+        req.registry.storage.delete_all.return_value = []
         req.registry.storage.create.return_value = {"id": "abc", "last_modified": 123}
 
         self.resource = Resource(context=mock.MagicMock(), request=req)

tests/core/resource/test_views.py

@@ -670,6 +670,41 @@ def test_next_page_url_relies_on_headers_information(self):
         self.assertIn("https://server.name:443", resp.headers["Next-Page"])
 
 
+class PluralDeleteTest(BaseWebTest, unittest.TestCase):
+    def setUp(self):
+        super().setUp()
+        body = {"data": MINIMALIST_OBJECT}
+        self.app.post_json(self.plural_url, body, headers=self.headers)
+        self.app.post_json(self.plural_url, body, headers=self.headers)
+
+    def test_timestamp_is_bumped_if_some_were_deleted(self):
+        timestamp_before = self.app.get(self.plural_url, headers=self.headers).headers["ETag"]
+
+        timestamp_delete = self.app.delete(self.plural_url, headers=self.headers).headers["ETag"]
+
+        timestamp_after = self.app.get(self.plural_url, headers=self.headers).headers["ETag"]
+
+        self.assertTrue(timestamp_before < timestamp_after)
+        self.assertEqual(timestamp_delete, timestamp_after)
+
+    def test_timestamp_is_not_bumped_if_none_deleted(self):
+        timestamp_before = self.app.get(self.plural_url, headers=self.headers).headers["ETag"]
+
+        url = self.plural_url + "?title=42"
+        timestamp_delete = self.app.delete(url, headers=self.headers).headers["ETag"]
+
+        timestamp_after = self.app.get(self.plural_url, headers=self.headers).headers["ETag"]
+
+        self.assertEqual(timestamp_before, timestamp_after)
+        self.assertEqual(timestamp_delete, timestamp_after)
+
+    def test_timestamp_is_latest_tombstone(self):
+        resp = self.app.delete(self.plural_url + "?_sort=-last_modified", headers=self.headers)
+
+        timestamp = int(resp.headers["ETag"].strip('"'))
+        self.assertTrue(timestamp >= resp.json["data"][0]["last_modified"])
+
+
 class SchemaLessPartialResponseTest(BaseWebTest, unittest.TestCase):
     """Extra tests for :mod:`tests.core.resource.test_partial_response`
     """