Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return empty item collection instead of error when searching #233

Merged
merged 10 commits into from
Aug 18, 2021
25 changes: 16 additions & 9 deletions stac_fastapi/pgstac/stac_fastapi/pgstac/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ async def get_collection(self, id: str, **kwargs) -> Collection:
)
collection = await conn.fetchval(q, *p)
if collection is None:
raise NotFoundError
raise NotFoundError(f"Collection {id} does not exist.")
links = await CollectionLinks(collection_id=id, request=request).get_links()
collection["links"] = links
return Collection(**collection)
Expand Down Expand Up @@ -128,8 +128,6 @@ async def _search_base(
prev: Optional[str] = items.pop("prev", None)
collection = ItemCollection(**items)
cleaned_features: List[Item] = []
if collection["features"] is None or len(collection["features"]) == 0:
raise NotFoundError("No features found")

for feature in collection["features"]:
feature = Item(**feature)
Expand Down Expand Up @@ -176,13 +174,16 @@ async def item_collection(
Returns:
An ItemCollection.
"""
# If collection does not exist, NotFoundError wil be raised
zstatmanweil marked this conversation as resolved.
Show resolved Hide resolved
await self.get_collection(id, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious what convinced you to safeguard against non-existent collections here. Maybe you saw some performance hits in that case which can be avoided?

Copy link
Contributor Author

@zstatmanweil zstatmanweil Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition is to maintain the existing logic that a request to this endpoint would result in an error if a collection doesn't exist. Previously it was the item_collection = await self._search_base(req, **kwargs) that would result in a 404 since no items would be found, but since that now returns an empty collection and 200 if no items are found, I wanted to make sure an error was raised.

I was most unsure about this change, especially since The Planetary Computer STAC actually doesn't raise a 404 in this instance when a collection doesn't exist. I think logically it should though. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - the PC should def return a 404 instead of an empty collection in this case, so this change LGTM


req = PgstacSearch(collections=[id], limit=limit, token=token)
collection = await self._search_base(req, **kwargs)
item_collection = await self._search_base(req, **kwargs)
links = await CollectionLinks(
collection_id=id, request=kwargs["request"]
).get_links(extra_links=collection["links"])
collection["links"] = links
return collection
).get_links(extra_links=item_collection["links"])
item_collection["links"] = links
return item_collection

async def get_item(self, item_id: str, collection_id: str, **kwargs) -> Item:
"""Get item by id.
Expand All @@ -195,9 +196,15 @@ async def get_item(self, item_id: str, collection_id: str, **kwargs) -> Item:
Returns:
Item.
"""
# If collection does not exist, NotFoundError wil be raised
await self.get_collection(collection_id, **kwargs)
Comment on lines +199 to +200
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above regarding the benefits of this safeguard

Copy link
Contributor Author

@zstatmanweil zstatmanweil Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea as above - checking if a collection exists because if not, I think a 404 should be returned


req = PgstacSearch(ids=[item_id], limit=1)
collection = await self._search_base(req, **kwargs)
return Item(**collection["features"][0])
item_collection = await self._search_base(req, **kwargs)
if not item_collection["features"]:
raise NotFoundError(f"Collection {collection_id} does not exist.")

return Item(**item_collection["features"][0])

async def post_search(
self, search_request: PgstacSearch, **kwargs
Expand Down
27 changes: 26 additions & 1 deletion stac_fastapi/pgstac/tests/resources/test_item.py
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,22 @@ async def test_item_search_by_id_post(app_client, load_test_data, load_test_coll
assert set([feat["id"] for feat in resp_json["features"]]) == set(ids)


@pytest.mark.asyncio
async def test_item_search_by_id_no_results_post(
app_client, load_test_data, load_test_collection
):
"""Test POST search by item id (core) when there are no results"""
test_item = load_test_data("test_item.json")

search_ids = ["nonexistent_id"]

params = {"collections": [test_item["collection"]], "ids": search_ids}
resp = await app_client.post("/search", json=params)
assert resp.status_code == 200
resp_json = resp.json()
assert len(resp_json["features"]) == 0


@pytest.mark.asyncio
async def test_item_search_spatial_query_post(
app_client, load_test_data, load_test_collection
Expand Down Expand Up @@ -650,7 +666,9 @@ async def test_item_search_get_query_extension(
),
}
resp = await app_client.get("/search", params=params)
assert resp.status_code == 404
# No items found should still return a 200 but with an empty list of features
assert resp.status_code == 200
assert len(resp.json()["features"]) == 0

params["query"] = json.dumps(
{"proj:epsg": {"eq": test_item["properties"]["proj:epsg"]}}
Expand All @@ -671,6 +689,13 @@ async def test_get_missing_item_collection(app_client):
assert resp.status_code == 404


@pytest.mark.asyncio
async def test_get_item_from_missing_item_collection(app_client):
"""Test reading an item from a collection which does not exist"""
resp = await app_client.get("/collections/invalid-collection/items/some-item")
assert resp.status_code == 404


@pytest.mark.asyncio
async def test_pagination_item_collection(
app_client, load_test_data, load_test_collection
Expand Down