From 456c51564d7b4e50ef8ab574dff00445a382a7ab Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Mon, 7 Oct 2024 21:51:10 +0200 Subject: [PATCH 1/4] add failing test --- tests/resources/test_collection.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/resources/test_collection.py b/tests/resources/test_collection.py index 634747bc..4921c2ab 100644 --- a/tests/resources/test_collection.py +++ b/tests/resources/test_collection.py @@ -303,3 +303,15 @@ async def test_get_collections_search( "/collections", ) assert len(resp.json()["collections"]) == 2 + + +@pytest.mark.asyncio +async def test_get_collections_search_limit_offset( + app_client, load_test_collection, load_test2_collection +): + resp = await app_client.get( + "/collections", + params={"limit": 1}, + ) + assert len(resp.json()["collections"]) == 1 + assert resp.json()["collections"][0]["id"] == load_test_collection.id From a9dce7e8d819824dd1d2330ebbc8633b86b682e7 Mon Sep 17 00:00:00 2001 From: Henry Rodman Date: Tue, 8 Oct 2024 04:08:31 -0500 Subject: [PATCH 2/4] handle collection paging differently (#156) * handle collection paging differently * test next link --- docker-compose.yml | 2 ++ stac_fastapi/pgstac/core.py | 17 +++++---- stac_fastapi/pgstac/models/links.py | 54 +++++++++++++++++++++++++++++ tests/resources/test_collection.py | 14 ++++++-- 4 files changed, 79 insertions(+), 8 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index ec1080aa..95e7d430 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -35,6 +35,8 @@ services: build: context: . dockerfile: Dockerfile.tests + volumes: + - .:/app environment: - ENVIRONMENT=local - DB_MIN_CONN_SIZE=1 diff --git a/stac_fastapi/pgstac/core.py b/stac_fastapi/pgstac/core.py index e7dcab21..1a962165 100644 --- a/stac_fastapi/pgstac/core.py +++ b/stac_fastapi/pgstac/core.py @@ -25,6 +25,7 @@ from stac_fastapi.pgstac.config import Settings from stac_fastapi.pgstac.models.links import ( CollectionLinks, + CollectionSearchPagingLinks, ItemCollectionLinks, ItemLinks, PagingLinks, @@ -90,12 +91,16 @@ async def all_collections( # noqa: C901 ) collections_result: Collections = await conn.fetchval(q, *p) - next: Optional[str] = None - prev: Optional[str] = None - + next: Optional[Dict[str, Any]] = None + prev: Optional[Dict[str, Any]] = None if links := collections_result.get("links"): - next = collections_result["links"].pop("next") - prev = collections_result["links"].pop("prev") + next = None + prev = None + for link in links: + if link["rel"] == "next": + next = link + elif link["rel"] == "prev": + prev = link linked_collections: List[Collection] = [] collections = collections_result["collections"] @@ -120,7 +125,7 @@ async def all_collections( # noqa: C901 linked_collections.append(coll) - links = await PagingLinks( + links = await CollectionSearchPagingLinks( request=request, next=next, prev=prev, diff --git a/stac_fastapi/pgstac/models/links.py b/stac_fastapi/pgstac/models/links.py index 0e6d9071..10e0f3b9 100644 --- a/stac_fastapi/pgstac/models/links.py +++ b/stac_fastapi/pgstac/models/links.py @@ -173,6 +173,60 @@ def link_prev(self) -> Optional[Dict[str, Any]]: return None +@attr.s +class CollectionSearchPagingLinks(BaseLinks): + next: Optional[Dict[str, Any]] = attr.ib(kw_only=True, default=None) + prev: Optional[Dict[str, Any]] = attr.ib(kw_only=True, default=None) + + def link_next(self) -> Optional[Dict[str, Any]]: + """Create link for next page.""" + if self.next is not None: + method = self.request.method + if method == "GET": + # if offset is equal to default value (0), drop it + if self.next["body"].get("offset", -1) == 0: + _ = self.next["body"].pop("offset") + + href = merge_params(self.url, self.next["body"]) + + # if next link is equal to this link, skip it + if href == self.url: + print(self.request.body()) + return None + + link = { + "rel": Relations.next.value, + "type": MimeTypes.geojson.value, + "method": method, + "href": href, + } + return link + + return None + + def link_prev(self): + if self.prev is not None: + method = self.request.method + if method == "GET": + # if offset is equal to default value (0), drop it + if self.prev["body"].get("offset", -1) == 0: + _ = self.prev["body"].pop("offset") + + href = merge_params(self.url, self.prev["body"]) + + # if prev link is equal to this link, skip it + if href == self.url: + return None + return { + "rel": Relations.previous.value, + "type": MimeTypes.geojson.value, + "method": method, + "href": href, + } + + return None + + @attr.s class CollectionLinksBase(BaseLinks): """Create inferred links specific to collections.""" diff --git a/tests/resources/test_collection.py b/tests/resources/test_collection.py index 4921c2ab..f67f6983 100644 --- a/tests/resources/test_collection.py +++ b/tests/resources/test_collection.py @@ -313,5 +313,15 @@ async def test_get_collections_search_limit_offset( "/collections", params={"limit": 1}, ) - assert len(resp.json()["collections"]) == 1 - assert resp.json()["collections"][0]["id"] == load_test_collection.id + response = resp.json() + assert len(response["collections"]) == 1 + assert response["collections"][0]["id"] == load_test_collection["id"] + + # check next link + next_link = [link["href"] for link in response["links"] if link["rel"] == "next"][0] + next_url = next_link.replace(str(app_client.base_url), "") + next_resp = await app_client.get(next_url) + next_response = next_resp.json() + + assert len(next_response["collections"]) == 1 + assert next_response["collections"][0]["id"] == load_test2_collection.id From 8862c931fab125e7e26d6dfdc27a79a5504ac080 Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Wed, 9 Oct 2024 17:20:39 +0200 Subject: [PATCH 3/4] add OffsetPaginationExtension --- setup.py | 6 +- stac_fastapi/pgstac/app.py | 4 + stac_fastapi/pgstac/core.py | 23 ++--- stac_fastapi/pgstac/models/links.py | 5 +- tests/conftest.py | 9 +- tests/resources/test_collection.py | 132 +++++++++++++++++++++++++--- 6 files changed, 148 insertions(+), 31 deletions(-) diff --git a/setup.py b/setup.py index 74b63833..fd2943ca 100644 --- a/setup.py +++ b/setup.py @@ -10,9 +10,9 @@ "orjson", "pydantic", "stac_pydantic==3.1.*", - "stac-fastapi.api~=3.0.2", - "stac-fastapi.extensions~=3.0.2", - "stac-fastapi.types~=3.0.2", + "stac-fastapi.api~=3.0.3", + "stac-fastapi.extensions~=3.0.3", + "stac-fastapi.types~=3.0.3", "asyncpg", "buildpg", "brotli_asgi", diff --git a/stac_fastapi/pgstac/app.py b/stac_fastapi/pgstac/app.py index 9ba27e08..ac608a5d 100644 --- a/stac_fastapi/pgstac/app.py +++ b/stac_fastapi/pgstac/app.py @@ -19,6 +19,7 @@ from stac_fastapi.extensions.core import ( FieldsExtension, FilterExtension, + OffsetPaginationExtension, SortExtension, TokenPaginationExtension, TransactionExtension, @@ -55,6 +56,9 @@ "sort": SortExtension(), "fields": FieldsExtension(), "filter": FilterExtension(client=FiltersClient()), + # NOTE: there is no conformance class for the Pagination extension + # so `CollectionSearchExtension.from_extensions` will raise a warning + "pagination": OffsetPaginationExtension(), } enabled_extensions = ( diff --git a/stac_fastapi/pgstac/core.py b/stac_fastapi/pgstac/core.py index 1a962165..f8cb47ef 100644 --- a/stac_fastapi/pgstac/core.py +++ b/stac_fastapi/pgstac/core.py @@ -47,8 +47,8 @@ async def all_collections( # noqa: C901 bbox: Optional[BBox] = None, datetime: Optional[DateTimeType] = None, limit: Optional[int] = None, + offset: Optional[int] = None, query: Optional[str] = None, - token: Optional[str] = None, fields: Optional[List[str]] = None, sortby: Optional[str] = None, filter: Optional[str] = None, @@ -69,7 +69,7 @@ async def all_collections( # noqa: C901 base_args = { "bbox": bbox, "limit": limit, - "token": token, + "offset": offset, "query": orjson.loads(unquote_plus(query)) if query else query, } @@ -91,16 +91,16 @@ async def all_collections( # noqa: C901 ) collections_result: Collections = await conn.fetchval(q, *p) - next: Optional[Dict[str, Any]] = None - prev: Optional[Dict[str, Any]] = None + next_link: Optional[Dict[str, Any]] = None + prev_link: Optional[Dict[str, Any]] = None if links := collections_result.get("links"): - next = None - prev = None + next_link = None + prev_link = None for link in links: if link["rel"] == "next": - next = link + next_link = link elif link["rel"] == "prev": - prev = link + prev_link = link linked_collections: List[Collection] = [] collections = collections_result["collections"] @@ -125,10 +125,13 @@ async def all_collections( # noqa: C901 linked_collections.append(coll) + if not collections: + next_link = None + links = await CollectionSearchPagingLinks( request=request, - next=next, - prev=prev, + next=next_link, + prev=prev_link, ).get_links() return Collections( diff --git a/stac_fastapi/pgstac/models/links.py b/stac_fastapi/pgstac/models/links.py index 10e0f3b9..c6fa50a2 100644 --- a/stac_fastapi/pgstac/models/links.py +++ b/stac_fastapi/pgstac/models/links.py @@ -191,16 +191,14 @@ def link_next(self) -> Optional[Dict[str, Any]]: # if next link is equal to this link, skip it if href == self.url: - print(self.request.body()) return None - link = { + return { "rel": Relations.next.value, "type": MimeTypes.geojson.value, "method": method, "href": href, } - return link return None @@ -217,6 +215,7 @@ def link_prev(self): # if prev link is equal to this link, skip it if href == self.url: return None + return { "rel": Relations.previous.value, "type": MimeTypes.geojson.value, diff --git a/tests/conftest.py b/tests/conftest.py index e571cae6..3b1838ac 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -26,6 +26,7 @@ CollectionSearchExtension, FieldsExtension, FilterExtension, + OffsetPaginationExtension, SortExtension, TokenPaginationExtension, TransactionExtension, @@ -140,10 +141,12 @@ def api_client(request, database): SortExtension(), FieldsExtension(), FilterExtension(client=FiltersClient()), + OffsetPaginationExtension(), ] - collection_search_extension = CollectionSearchExtension.from_extensions( - collection_extensions - ) + with pytest.warns(UserWarning): + collection_search_extension = CollectionSearchExtension.from_extensions( + collection_extensions + ) items_get_request_model = create_request_model( model_name="ItemCollectionUri", diff --git a/tests/resources/test_collection.py b/tests/resources/test_collection.py index f67f6983..3429a3d6 100644 --- a/tests/resources/test_collection.py +++ b/tests/resources/test_collection.py @@ -309,19 +309,127 @@ async def test_get_collections_search( async def test_get_collections_search_limit_offset( app_client, load_test_collection, load_test2_collection ): + resp = await app_client.get("/collections") + cols = resp.json()["collections"] + assert len(cols) == 2 + links = resp.json()["links"] + assert len(links) == 2 + assert {"root", "self"} == {link["rel"] for link in links} + + ################### + # limit should be positive + resp = await app_client.get("/collections", params={"limit": 0}) + assert resp.status_code == 400 + + ################### + # limit=1, should have a `next` link resp = await app_client.get( "/collections", params={"limit": 1}, ) - response = resp.json() - assert len(response["collections"]) == 1 - assert response["collections"][0]["id"] == load_test_collection["id"] - - # check next link - next_link = [link["href"] for link in response["links"] if link["rel"] == "next"][0] - next_url = next_link.replace(str(app_client.base_url), "") - next_resp = await app_client.get(next_url) - next_response = next_resp.json() - - assert len(next_response["collections"]) == 1 - assert next_response["collections"][0]["id"] == load_test2_collection.id + cols = resp.json()["collections"] + links = resp.json()["links"] + assert len(cols) == 1 + assert cols[0]["id"] == load_test_collection["id"] + assert len(links) == 3 + assert {"root", "self", "next"} == {link["rel"] for link in links} + next_link = list(filter(lambda link: link["rel"] == "next", links))[0] + assert next_link["href"].endswith("?limit=1&offset=1") + + ################### + # limit=2, there should not be a next link + resp = await app_client.get( + "/collections", + params={"limit": 2}, + ) + cols = resp.json()["collections"] + links = resp.json()["links"] + assert len(cols) == 2 + assert cols[0]["id"] == load_test_collection["id"] + assert cols[1]["id"] == load_test2_collection.id + # TODO: check with pgstac + # assert len(links) == 2 + # assert {"root", "self"} == {link["rel"] for link in links} + + ################### + # limit=3, there should not be a next/previous link + resp = await app_client.get( + "/collections", + params={"limit": 3}, + ) + cols = resp.json()["collections"] + links = resp.json()["links"] + assert len(cols) == 2 + assert cols[0]["id"] == load_test_collection["id"] + assert cols[1]["id"] == load_test2_collection.id + assert len(links) == 2 + assert {"root", "self"} == {link["rel"] for link in links} + + ################### + # offset=3, because there are 2 collections, we should not have `next` or `prev` links + resp = await app_client.get( + "/collections", + params={"offset": 3}, + ) + cols = resp.json()["collections"] + links = resp.json()["links"] + assert len(cols) == 0 + assert len(links) == 2 + assert {"root", "self"} == {link["rel"] for link in links} + + ################### + # offset=3,limit=1 + resp = await app_client.get( + "/collections", + params={"limit": 1, "offset": 3}, + ) + cols = resp.json()["collections"] + links = resp.json()["links"] + assert len(cols) == 0 + assert len(links) == 3 + assert {"root", "self", "previous"} == {link["rel"] for link in links} + prev_link = list(filter(lambda link: link["rel"] == "previous", links))[0] + assert prev_link["href"].endswith("?limit=1&offset=2") + + # ################### + # # offset=3,limit=2 + # resp = await app_client.get( + # "/collections", + # params={"limit": 2,"offset": 3}, + # ) + # cols = resp.json()["collections"] + # links = resp.json()["links"] + # assert len(cols) == 0 + # assert len(links) == 3 + # assert {"root", "self", "previous"} == {link["rel"] for link in links} + # prev_link = list(filter(lambda link: link["rel"] == "previous", links))[0] + # assert prev_link["href"].endswith("?limit=1&offset=2") + + ################### + # offset=1, should have a `previous` link + resp = await app_client.get( + "/collections", + params={"offset": 1}, + ) + cols = resp.json()["collections"] + links = resp.json()["links"] + assert len(cols) == 1 + assert cols[0]["id"] == load_test2_collection.id + # TODO: Check with pgstac + # assert len(links) == 3 + # assert {"root", "self", "previous"} == {link["rel"] for link in links} + # prev_link = list(filter(lambda link: link["rel"] == "previous", links))[0] + # assert prev_link["href"].endswith("?offset=0") + + ################### + # offset=0, should not have next/previous link + resp = await app_client.get( + "/collections", + params={"offset": 0}, + ) + cols = resp.json()["collections"] + links = resp.json()["links"] + assert len(cols) == 2 + # TODO: Check with pgstac + # assert len(links) == 2 + # assert {"root", "self"} == {link["rel"] for link in links} From 576a9dc981b859905ac1a9bd8b6c94e643a42621 Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Wed, 9 Oct 2024 17:25:02 +0200 Subject: [PATCH 4/4] uncomment test --- tests/resources/test_collection.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/tests/resources/test_collection.py b/tests/resources/test_collection.py index 3429a3d6..a710c3cf 100644 --- a/tests/resources/test_collection.py +++ b/tests/resources/test_collection.py @@ -391,19 +391,19 @@ async def test_get_collections_search_limit_offset( prev_link = list(filter(lambda link: link["rel"] == "previous", links))[0] assert prev_link["href"].endswith("?limit=1&offset=2") - # ################### - # # offset=3,limit=2 - # resp = await app_client.get( - # "/collections", - # params={"limit": 2,"offset": 3}, - # ) - # cols = resp.json()["collections"] - # links = resp.json()["links"] - # assert len(cols) == 0 - # assert len(links) == 3 - # assert {"root", "self", "previous"} == {link["rel"] for link in links} - # prev_link = list(filter(lambda link: link["rel"] == "previous", links))[0] - # assert prev_link["href"].endswith("?limit=1&offset=2") + ################### + # limit=2, offset=3, there should not be a next link + resp = await app_client.get( + "/collections", + params={"limit": 2, "offset": 3}, + ) + cols = resp.json()["collections"] + links = resp.json()["links"] + assert len(cols) == 0 + assert len(links) == 3 + assert {"root", "self", "previous"} == {link["rel"] for link in links} + prev_link = list(filter(lambda link: link["rel"] == "previous", links))[0] + assert prev_link["href"].endswith("?limit=2&offset=1") ################### # offset=1, should have a `previous` link