Repository: stac-utils/stac-fastapi (language: python)

Issue:
in pgstac transaction extension delete operation, any item with the given id may be deleted
in pgstac/transactions.py, the db is only passed the item_id, and not the collection_id the item is in. This leads me to believe that this would delete any Item with that ID, rather than being scoped to the specific collection in the path. 

```python
    async def delete_item(
        self, item_id: str, collection_id: str, **kwargs
    ) -> stac_types.Item:
        ...
        await dbfunc(pool, "delete_item", item_id) # no collection_id in call
        ...
```

@bitner 


Discussion / hints:
https://github.com/stac-utils/pgstac/issues/84
Given that PgSTAC enforces unique Item IDs within the API, this will not be an issue. If the PgSTAC behavior changes then we should revisit this.

@geospatial-jeff I believe this can be closed.
> Given that PgSTAC enforces unique Item IDs within the API, this will not be an issue. If the PgSTAC behavior changes then we should revisit this.
> 
> @geospatial-jeff I believe this can be closed.

Actually, I do not think this is true (see [stac-utils/pgstac#84](https://github.com/stac-utils/pgstac/issues/84#issuecomment-1234523125)). Since the `items` tables is partitioned Items can only be unique on the combination of `{partition_key}` and `id`. Prior to PgSTAC 0.5.0 the `{partition_key}` was `datetime`, and in that version it was changed to `collection`. I have not tested this yet, but I think this means:

1. Multiple Items with the same `id` can exist (as long as they do not also have identical values for the partition key)
2. Prior to PgSTAC 0.5.0 it would be possible to add Items with duplicate `id`s *to the same Collection* as long as they have different `datetime` values.

@bitner Can you confirm that this is the case?

Yes, this is correct. We can only enforce uniqueness within a partition. In pgstac >0.5 there is always an initial partition by collection and _optionally_ a second level partition by datetime chunks (month or year) and so it is possible to have duplicate ids that fall within different partitions.

Fix patch:
diff --git a/CHANGES.md b/CHANGES.md
index d04917a10..990fd1e99 100644
--- a/CHANGES.md
+++ b/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
 
diff --git a/stac_fastapi/pgstac/stac_fastapi/pgstac/db.py b/stac_fastapi/pgstac/stac_fastapi/pgstac/db.py
index 2991a92a5..bc6cc96e6 100644
--- a/stac_fastapi/pgstac/stac_fastapi/pgstac/db.py
+++ b/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:
diff --git a/stac_fastapi/pgstac/stac_fastapi/pgstac/transactions.py b/stac_fastapi/pgstac/stac_fastapi/pgstac/transactions.py
index 9013a6319..68479aa34 100644
--- a/stac_fastapi/pgstac/stac_fastapi/pgstac/transactions.py
+++ b/stac_fastapi/pgstac/stac_fastapi/pgstac/transactions.py
@@ -4,6 +4,7 @@
 from typing import Optional, Union
 
 import attr
+from buildpg import render
 from fastapi import HTTPException
 from starlette.responses import JSONResponse, Response
 
@@ -11,7 +12,7 @@
     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(


Test patch:
diff --git a/stac_fastapi/pgstac/tests/api/test_api.py b/stac_fastapi/pgstac/tests/api/test_api.py
index 06a80675e..c4f9442b3 100644
--- a/stac_fastapi/pgstac/tests/api/test_api.py
+++ b/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"]