[pgstac] Delete items using collection id (#520)

* fix, pgstac: delete items with collection id

* chore: update changelog

Author: gadomski

CHANGES.md

@@ -22,6 +22,7 @@
 * `self` link rel for `/collections/{c_id}/items` ([#508](https://github.com/stac-utils/stac-fastapi/pull/508))
 * Media type of the item collection endpoint ([#508](https://github.com/stac-utils/stac-fastapi/pull/508))
 * Manually exclude non-truthy optional values from sqlalchemy serialization of Collections ([#508](https://github.com/stac-utils/stac-fastapi/pull/508))
+* Deleting items that had repeated ids in other collections ([#520](https://github.com/stac-utils/stac-fastapi/pull/520))
 
 ## [2.4.3] - 2022-11-25
 

stac_fastapi/pgstac/stac_fastapi/pgstac/db.py

@@ -1,7 +1,8 @@
 """Database connection handling."""
 
 import json
-from typing import Dict, Union
+from contextlib import contextmanager
+from typing import Dict, Generator, Union
 
 import attr
 import orjson
@@ -61,7 +62,7 @@ async def dbfunc(pool: pool, func: str, arg: Union[str, Dict]):
     arg -- the argument to the PostgreSQL function as either a string
     or a dict that will be converted into jsonb
     """
-    try:
+    with translate_pgstac_errors():
         if isinstance(arg, str):
             async with pool.acquire() as conn:
                 q, p = render(
@@ -80,6 +81,13 @@ async def dbfunc(pool: pool, func: str, arg: Union[str, Dict]):
                     item=json.dumps(arg),
                 )
                 return await conn.fetchval(q, *p)
+
+
+@contextmanager
+def translate_pgstac_errors() -> Generator[None, None, None]:
+    """Context manager that translates pgstac errors into FastAPI errors."""
+    try:
+        yield
     except exceptions.UniqueViolationError as e:
         raise ConflictError from e
     except exceptions.NoDataFoundError as e:

stac_fastapi/pgstac/stac_fastapi/pgstac/transactions.py

@@ -4,14 +4,15 @@
 from typing import Optional, Union
 
 import attr
+from buildpg import render
 from fastapi import HTTPException
 from starlette.responses import JSONResponse, Response
 
 from stac_fastapi.extensions.third_party.bulk_transactions import (
     AsyncBaseBulkTransactionsClient,
     Items,
 )
-from stac_fastapi.pgstac.db import dbfunc
+from stac_fastapi.pgstac.db import dbfunc, translate_pgstac_errors
 from stac_fastapi.pgstac.models.links import CollectionLinks, ItemLinks
 from stac_fastapi.types import stac as stac_types
 from stac_fastapi.types.core import AsyncBaseTransactionsClient
@@ -98,12 +99,19 @@ async def update_collection(
         return stac_types.Collection(**collection)
 
     async def delete_item(
-        self, item_id: str, **kwargs
+        self, item_id: str, collection_id: str, **kwargs
     ) -> Optional[Union[stac_types.Item, Response]]:
         """Delete item."""
         request = kwargs["request"]
         pool = request.app.state.writepool
-        await dbfunc(pool, "delete_item", item_id)
+        async with pool.acquire() as conn:
+            q, p = render(
+                "SELECT * FROM delete_item(:item::text, :collection::text);",
+                item=item_id,
+                collection=collection_id,
+            )
+            with translate_pgstac_errors():
+                await conn.fetchval(q, *p)
         return JSONResponse({"deleted item": item_id})
 
     async def delete_collection(

stac_fastapi/pgstac/tests/api/test_api.py

@@ -3,6 +3,7 @@
 
 import orjson
 import pytest
+from pystac import Collection, Extent, Item, SpatialExtent, TemporalExtent
 
 STAC_CORE_ROUTES = [
     "GET /",
@@ -24,6 +25,24 @@
     "PUT /collections/{collection_id}/items/{item_id}",
 ]
 
+GLOBAL_BBOX = [-180.0, -90.0, 180.0, 90.0]
+GLOBAL_GEOMETRY = {
+    "type": "Polygon",
+    "coordinates": (
+        (
+            (180.0, -90.0),
+            (180.0, 90.0),
+            (-180.0, 90.0),
+            (-180.0, -90.0),
+            (180.0, -90.0),
+        ),
+    ),
+}
+DEFAULT_EXTENT = Extent(
+    SpatialExtent(GLOBAL_BBOX),
+    TemporalExtent([[datetime.now(), None]]),
+)
+
 
 async def test_post_search_content_type(app_client):
     params = {"limit": 1}
@@ -513,3 +532,33 @@ async def test_bad_collection_queryables(
 ):
     resp = await app_client.get("/collections/bad-collection/queryables")
     assert resp.status_code == 404
+
+
+async def test_deleting_items_with_identical_ids(app_client):
+    collection_a = Collection("collection-a", "The first collection", DEFAULT_EXTENT)
+    collection_b = Collection("collection-b", "The second collection", DEFAULT_EXTENT)
+    item = Item("the-item", GLOBAL_GEOMETRY, GLOBAL_BBOX, datetime.now(), {})
+
+    for collection in (collection_a, collection_b):
+        response = await app_client.post(
+            "/collections", json=collection.to_dict(include_self_link=False)
+        )
+        assert response.status_code == 200
+        item_as_dict = item.to_dict(include_self_link=False)
+        item_as_dict["collection"] = collection.id
+        response = await app_client.post(
+            f"/collections/{collection.id}/items", json=item_as_dict
+        )
+        assert response.status_code == 200
+        response = await app_client.get(f"/collections/{collection.id}/items")
+        assert response.status_code == 200, response.json()
+        assert len(response.json()["features"]) == 1
+
+    for collection in (collection_a, collection_b):
+        response = await app_client.delete(
+            f"/collections/{collection.id}/items/{item.id}"
+        )
+        assert response.status_code == 200, response.json()
+        response = await app_client.get(f"/collections/{collection.id}/items")
+        assert response.status_code == 200, response.json()
+        assert not response.json()["features"]