From 456c51564d7b4e50ef8ab574dff00445a382a7ab Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Mon, 7 Oct 2024 21:51:10 +0200 Subject: [PATCH 01/14] 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 02/14] 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 03/14] 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 04/14] 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 From de4da9f5e146c22ca48b348e7f8447fd88a21c38 Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Tue, 7 Jan 2025 21:59:03 +0100 Subject: [PATCH 05/14] update to pgstac 0.9.2 --- docker-compose.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker-compose.yml b/docker-compose.yml index 32fd9536..07cc7822 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -46,7 +46,7 @@ services: database: container_name: stac-db - image: ghcr.io/stac-utils/pgstac:v0.9.1 + image: ghcr.io/stac-utils/pgstac:v0.9.2 environment: - POSTGRES_USER=username - POSTGRES_PASSWORD=password From 0d44b2ccc1f9be2403d995f155b6daacab4d3d17 Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Wed, 8 Jan 2025 16:33:17 +0100 Subject: [PATCH 06/14] update prev logic --- stac_fastapi/pgstac/models/links.py | 20 +++++++++++++---- tests/resources/test_collection.py | 34 ++++++++++++++++++++--------- 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/stac_fastapi/pgstac/models/links.py b/stac_fastapi/pgstac/models/links.py index c6fa50a2..f182072c 100644 --- a/stac_fastapi/pgstac/models/links.py +++ b/stac_fastapi/pgstac/models/links.py @@ -206,11 +206,23 @@ 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") + u = urlparse(self.url) + params = parse_qs(u.query) + params.update(self.prev["body"]) - href = merge_params(self.url, self.prev["body"]) + # if offset is equal to default value (0), drop it + if params.get("offset", -1) == 0: + _ = params.pop("offset") + + param_string = unquote(urlencode(params, True)) + href = ParseResult( + scheme=u.scheme, + netloc=u.netloc, + path=u.path, + params=u.params, + query=param_string, + fragment=u.fragment, + ).geturl() # if prev link is equal to this link, skip it if href == self.url: diff --git a/tests/resources/test_collection.py b/tests/resources/test_collection.py index a710c3cf..32d5f377 100644 --- a/tests/resources/test_collection.py +++ b/tests/resources/test_collection.py @@ -347,9 +347,8 @@ async def test_get_collections_search_limit_offset( 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} + assert len(links) == 2 + assert {"root", "self"} == {link["rel"] for link in links} ################### # limit=3, there should not be a next/previous link @@ -406,20 +405,36 @@ async def test_get_collections_search_limit_offset( assert prev_link["href"].endswith("?limit=2&offset=1") ################### - # offset=1, should have a `previous` link + # offset=1,limit=1 should have a `previous` link resp = await app_client.get( "/collections", - params={"offset": 1}, + params={"offset": 1, "limit": 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 "offset" not in prev_link["href"] + + ################### + # BUG: pgstac doesn't return a `prev` link when limit is not set + # 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 # 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 be in the previous link (because it's useless) + # assert "offset" not in prev_link["href"] ################### # offset=0, should not have next/previous link @@ -430,6 +445,5 @@ async def test_get_collections_search_limit_offset( 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} + assert len(links) == 2 + assert {"root", "self"} == {link["rel"] for link in links} From e2aa27cd5ad339b7b6773b564e9a9d5da6940d92 Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Wed, 8 Jan 2025 16:44:19 +0100 Subject: [PATCH 07/14] only test 0.9.0 --- .github/workflows/cicd.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/cicd.yaml b/.github/workflows/cicd.yaml index 0ae6f963..82d1a229 100644 --- a/.github/workflows/cicd.yaml +++ b/.github/workflows/cicd.yaml @@ -13,10 +13,10 @@ jobs: matrix: include: - {python: '3.12', pypgstac: '0.9.*'} - - {python: '3.12', pypgstac: '0.8.*'} - - {python: '3.11', pypgstac: '0.8.*'} - - {python: '3.9', pypgstac: '0.8.*'} - - {python: '3.8', pypgstac: '0.8.*'} + - {python: '3.11', pypgstac: '0.9.*'} + - {python: '3.10', pypgstac: '0.9.*'} + - {python: '3.9', pypgstac: '0.9.*'} + - {python: '3.8', pypgstac: '0.9.*'} timeout-minutes: 20 From c95f39a2facb8193ad386b4d4e2f4a5dd01cdbd5 Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Wed, 8 Jan 2025 16:44:56 +0100 Subject: [PATCH 08/14] update pypstac version --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 16edf782..1619ce0e 100644 --- a/setup.py +++ b/setup.py @@ -17,7 +17,7 @@ "buildpg", "brotli_asgi", "pygeofilter>=0.2", - "pypgstac>=0.8,<0.10", + "pypgstac>=0.9,<0.10", ] extra_reqs = { From 5108be44a8cb72d5c8ab81b4c273f31225fe1978 Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Thu, 9 Jan 2025 09:31:36 +0100 Subject: [PATCH 09/14] add back 0.8 support but allow skip tests --- .github/workflows/cicd.yaml | 9 +++--- setup.py | 2 +- tests/conftest.py | 7 +++++ tests/resources/test_collection.py | 44 ++++++++++++++++++------------ 4 files changed, 39 insertions(+), 23 deletions(-) diff --git a/.github/workflows/cicd.yaml b/.github/workflows/cicd.yaml index 82d1a229..5afebf1b 100644 --- a/.github/workflows/cicd.yaml +++ b/.github/workflows/cicd.yaml @@ -13,10 +13,11 @@ jobs: matrix: include: - {python: '3.12', pypgstac: '0.9.*'} - - {python: '3.11', pypgstac: '0.9.*'} - - {python: '3.10', pypgstac: '0.9.*'} - - {python: '3.9', pypgstac: '0.9.*'} - - {python: '3.8', pypgstac: '0.9.*'} + - {python: '3.12', pypgstac: '0.8.*'} + - {python: '3.11', pypgstac: '0.8.*'} + - {python: '3.10', pypgstac: '0.8.*'} + - {python: '3.9', pypgstac: '0.8.*'} + - {python: '3.8', pypgstac: '0.8.*'} timeout-minutes: 20 diff --git a/setup.py b/setup.py index 1619ce0e..16edf782 100644 --- a/setup.py +++ b/setup.py @@ -17,7 +17,7 @@ "buildpg", "brotli_asgi", "pygeofilter>=0.2", - "pypgstac>=0.9,<0.10", + "pypgstac>=0.8,<0.10", ] extra_reqs = { diff --git a/tests/conftest.py b/tests/conftest.py index 3b1838ac..d96993f3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,6 +12,7 @@ from fastapi import APIRouter from fastapi.responses import ORJSONResponse from httpx import ASGITransport, AsyncClient +from pypgstac import __version__ as pgstac_version from pypgstac.db import PgstacDB from pypgstac.migrate import Migrate from pytest_postgresql.janitor import DatabaseJanitor @@ -48,6 +49,12 @@ logger = logging.getLogger(__name__) +requires_pgstac_0_9_2 = pytest.mark.skipif( + tuple(map(int, pgstac_version.split("."))) < (0, 9, 2), + reason="at least PgSTAC>0.9.2 required", +) + + @pytest.fixture(scope="session") def event_loop(): return asyncio.get_event_loop() diff --git a/tests/resources/test_collection.py b/tests/resources/test_collection.py index 32d5f377..f07724ff 100644 --- a/tests/resources/test_collection.py +++ b/tests/resources/test_collection.py @@ -4,6 +4,8 @@ import pytest from stac_pydantic import Collection +from ..conftest import requires_pgstac_0_9_2 + async def test_create_collection(app_client, load_test_data: Callable): in_json = load_test_data("test_collection.json") @@ -305,8 +307,9 @@ async def test_get_collections_search( assert len(resp.json()["collections"]) == 2 +@requires_pgstac_0_9_2 @pytest.mark.asyncio -async def test_get_collections_search_limit_offset( +async def test_get_collections_search_pagination( app_client, load_test_collection, load_test2_collection ): resp = await app_client.get("/collections") @@ -419,23 +422,6 @@ async def test_get_collections_search_limit_offset( prev_link = list(filter(lambda link: link["rel"] == "previous", links))[0] assert "offset" not in prev_link["href"] - ################### - # BUG: pgstac doesn't return a `prev` link when limit is not set - # 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 - # 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] - # # offset=0 should not be in the previous link (because it's useless) - # assert "offset" not in prev_link["href"] - ################### # offset=0, should not have next/previous link resp = await app_client.get( @@ -447,3 +433,25 @@ async def test_get_collections_search_limit_offset( assert len(cols) == 2 assert len(links) == 2 assert {"root", "self"} == {link["rel"] for link in links} + + +@pytest.mark.xfail(strict=False) +@pytest.mark.asyncio +async def test_get_collections_search_offset_1( + app_client, load_test_collection, load_test2_collection +): + # BUG: pgstac doesn't return a `prev` link when limit is not set + # 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 + 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] + # offset=0 should not be in the previous link (because it's useless) + assert "offset" not in prev_link["href"] From d025c9d828c4bf6432ea146cd54e8bcb0dd63664 Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Thu, 9 Jan 2025 09:38:36 +0100 Subject: [PATCH 10/14] skip for 0.8 --- tests/resources/test_collection.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/resources/test_collection.py b/tests/resources/test_collection.py index f07724ff..e808a7c6 100644 --- a/tests/resources/test_collection.py +++ b/tests/resources/test_collection.py @@ -435,6 +435,7 @@ async def test_get_collections_search_pagination( assert {"root", "self"} == {link["rel"] for link in links} +@requires_pgstac_0_9_2 @pytest.mark.xfail(strict=False) @pytest.mark.asyncio async def test_get_collections_search_offset_1( From 9293dd89bc151b6ef246ad3e95a5d84892fcaacf Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Thu, 9 Jan 2025 10:12:54 +0100 Subject: [PATCH 11/14] remove warnings --- stac_fastapi/pgstac/app.py | 2 -- tests/conftest.py | 7 +++---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/stac_fastapi/pgstac/app.py b/stac_fastapi/pgstac/app.py index 87b37ea2..f64095f4 100644 --- a/stac_fastapi/pgstac/app.py +++ b/stac_fastapi/pgstac/app.py @@ -59,8 +59,6 @@ "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(), } diff --git a/tests/conftest.py b/tests/conftest.py index d96993f3..3d998c18 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -150,10 +150,9 @@ def api_client(request, database): FilterExtension(client=FiltersClient()), OffsetPaginationExtension(), ] - with pytest.warns(UserWarning): - collection_search_extension = CollectionSearchExtension.from_extensions( - collection_extensions - ) + collection_search_extension = CollectionSearchExtension.from_extensions( + collection_extensions + ) items_get_request_model = create_request_model( model_name="ItemCollectionUri", From bd136828d67a26df12a466782247fc3a6f17ed5f Mon Sep 17 00:00:00 2001 From: Vincent Sarago Date: Fri, 10 Jan 2025 16:00:18 +0100 Subject: [PATCH 12/14] fallback to all_collections when `CollectionSearchExtension` is not enabled (#179) * fallback to all_collections when `CollectionSearchExtension` is not enabled * test all_collection fallback --- stac_fastapi/pgstac/core.py | 75 +++++++++++++++++------------- tests/conftest.py | 45 ++++++++++++++++++ tests/resources/test_collection.py | 49 +++++++++++++++++++ 3 files changed, 136 insertions(+), 33 deletions(-) diff --git a/stac_fastapi/pgstac/core.py b/stac_fastapi/pgstac/core.py index 9c993dca..4a1d351a 100644 --- a/stac_fastapi/pgstac/core.py +++ b/stac_fastapi/pgstac/core.py @@ -65,42 +65,51 @@ async def all_collections( # noqa: C901 """ base_url = get_base_url(request) - # Parse request parameters - base_args = { - "bbox": bbox, - "limit": limit, - "offset": offset, - "query": orjson.loads(unquote_plus(query)) if query else query, - } + next_link: Optional[Dict[str, Any]] = None + prev_link: Optional[Dict[str, Any]] = None + collections_result: Collections + + if self.extension_is_enabled("CollectionSearchExtension"): + base_args = { + "bbox": bbox, + "limit": limit, + "offset": offset, + "query": orjson.loads(unquote_plus(query)) if query else query, + } + + clean_args = clean_search_args( + base_args=base_args, + datetime=datetime, + fields=fields, + sortby=sortby, + filter_query=filter, + filter_lang=filter_lang, + ) - clean_args = clean_search_args( - base_args=base_args, - datetime=datetime, - fields=fields, - sortby=sortby, - filter_query=filter, - filter_lang=filter_lang, - ) + async with request.app.state.get_connection(request, "r") as conn: + q, p = render( + """ + SELECT * FROM collection_search(:req::text::jsonb); + """, + req=json.dumps(clean_args), + ) + collections_result = await conn.fetchval(q, *p) - async with request.app.state.get_connection(request, "r") as conn: - q, p = render( - """ - SELECT * FROM collection_search(:req::text::jsonb); - """, - req=json.dumps(clean_args), - ) - collections_result: Collections = await conn.fetchval(q, *p) + if links := collections_result.get("links"): + for link in links: + if link["rel"] == "next": + next_link = link + elif link["rel"] == "prev": + prev_link = link - next_link: Optional[Dict[str, Any]] = None - prev_link: Optional[Dict[str, Any]] = None - if links := collections_result.get("links"): - next_link = None - prev_link = None - for link in links: - if link["rel"] == "next": - next_link = link - elif link["rel"] == "prev": - prev_link = link + else: + async with request.app.state.get_connection(request, "r") as conn: + cols = await conn.fetchval( + """ + SELECT * FROM all_collections(); + """ + ) + collections_result = {"collections": cols, "links": []} linked_collections: List[Collection] = [] collections = collections_result["collections"] diff --git a/tests/conftest.py b/tests/conftest.py index 3d998c18..632a89d8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -268,3 +268,48 @@ async def load_test2_item(app_client, load_test_data, load_test2_collection): ) assert resp.status_code == 201 return Item.model_validate(resp.json()) + + +@pytest.fixture( + scope="session", +) +def api_client_no_ext(database): + api_settings = Settings( + postgres_user=database.user, + postgres_pass=database.password, + postgres_host_reader=database.host, + postgres_host_writer=database.host, + postgres_port=database.port, + postgres_dbname=database.dbname, + testing=True, + ) + return StacApi( + settings=api_settings, + extensions=[ + TransactionExtension(client=TransactionsClient(), settings=api_settings) + ], + client=CoreCrudClient(), + ) + + +@pytest.fixture(scope="function") +async def app_no_ext(api_client_no_ext): + logger.info("Creating app Fixture") + time.time() + app = api_client_no_ext.app + await connect_to_db(app) + + yield app + + await close_db_connection(app) + + logger.info("Closed Pools.") + + +@pytest.fixture(scope="function") +async def app_client_no_ext(app_no_ext): + logger.info("creating app_client") + async with AsyncClient( + transport=ASGITransport(app=app_no_ext), base_url="http://test" + ) as c: + yield c diff --git a/tests/resources/test_collection.py b/tests/resources/test_collection.py index e808a7c6..1a5ad68d 100644 --- a/tests/resources/test_collection.py +++ b/tests/resources/test_collection.py @@ -307,6 +307,55 @@ async def test_get_collections_search( assert len(resp.json()["collections"]) == 2 +@requires_pgstac_0_9_2 +@pytest.mark.asyncio +async def test_all_collections_with_pagination(app_client, load_test_data): + data = load_test_data("test_collection.json") + collection_id = data["id"] + for ii in range(0, 12): + data["id"] = collection_id + f"_{ii}" + resp = await app_client.post( + "/collections", + json=data, + ) + assert resp.status_code == 201 + + resp = await app_client.get("/collections") + cols = resp.json()["collections"] + assert len(cols) == 10 + links = resp.json()["links"] + assert len(links) == 3 + assert {"root", "self", "next"} == {link["rel"] for link in links} + + resp = await app_client.get("/collections", params={"limit": 12}) + cols = resp.json()["collections"] + assert len(cols) == 12 + links = resp.json()["links"] + assert len(links) == 2 + assert {"root", "self"} == {link["rel"] for link in links} + + +@requires_pgstac_0_9_2 +@pytest.mark.asyncio +async def test_all_collections_without_pagination(app_client_no_ext, load_test_data): + data = load_test_data("test_collection.json") + collection_id = data["id"] + for ii in range(0, 12): + data["id"] = collection_id + f"_{ii}" + resp = await app_client_no_ext.post( + "/collections", + json=data, + ) + assert resp.status_code == 201 + + resp = await app_client_no_ext.get("/collections") + cols = resp.json()["collections"] + assert len(cols) == 12 + links = resp.json()["links"] + assert len(links) == 2 + assert {"root", "self"} == {link["rel"] for link in links} + + @requires_pgstac_0_9_2 @pytest.mark.asyncio async def test_get_collections_search_pagination( From 60deedd3ec5f5c6ec552c624e1231cc14e0db35d Mon Sep 17 00:00:00 2001 From: vincentsarago Date: Fri, 10 Jan 2025 17:39:23 +0100 Subject: [PATCH 13/14] add offset=0 --- stac_fastapi/pgstac/core.py | 3 --- stac_fastapi/pgstac/models/links.py | 18 +----------------- tests/resources/test_collection.py | 2 +- 3 files changed, 2 insertions(+), 21 deletions(-) diff --git a/stac_fastapi/pgstac/core.py b/stac_fastapi/pgstac/core.py index 4a1d351a..ff5ec16b 100644 --- a/stac_fastapi/pgstac/core.py +++ b/stac_fastapi/pgstac/core.py @@ -134,9 +134,6 @@ async def all_collections( # noqa: C901 linked_collections.append(coll) - if not collections: - next_link = None - links = await CollectionSearchPagingLinks( request=request, next=next_link, diff --git a/stac_fastapi/pgstac/models/links.py b/stac_fastapi/pgstac/models/links.py index f182072c..c0ec4455 100644 --- a/stac_fastapi/pgstac/models/links.py +++ b/stac_fastapi/pgstac/models/links.py @@ -206,23 +206,7 @@ def link_prev(self): if self.prev is not None: method = self.request.method if method == "GET": - u = urlparse(self.url) - params = parse_qs(u.query) - params.update(self.prev["body"]) - - # if offset is equal to default value (0), drop it - if params.get("offset", -1) == 0: - _ = params.pop("offset") - - param_string = unquote(urlencode(params, True)) - href = ParseResult( - scheme=u.scheme, - netloc=u.netloc, - path=u.path, - params=u.params, - query=param_string, - fragment=u.fragment, - ).geturl() + href = merge_params(self.url, self.prev["body"]) # if prev link is equal to this link, skip it if href == self.url: diff --git a/tests/resources/test_collection.py b/tests/resources/test_collection.py index 1a5ad68d..dec20881 100644 --- a/tests/resources/test_collection.py +++ b/tests/resources/test_collection.py @@ -469,7 +469,7 @@ async def test_get_collections_search_pagination( 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 "offset" not in prev_link["href"] + assert "offset" in prev_link["href"] ################### # offset=0, should not have next/previous link From 041f2dee7c27e84b3fb22106f4a8b55725784efd Mon Sep 17 00:00:00 2001 From: Vincent Sarago Date: Mon, 13 Jan 2025 14:15:48 +0100 Subject: [PATCH 14/14] Update tests/conftest.py Co-authored-by: Henry Rodman --- tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 632a89d8..4b180147 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -51,7 +51,7 @@ requires_pgstac_0_9_2 = pytest.mark.skipif( tuple(map(int, pgstac_version.split("."))) < (0, 9, 2), - reason="at least PgSTAC>0.9.2 required", + reason="PgSTAC>=0.9.2 required", )