From 466111b8187c45d98c86148480d699c743573ba8 Mon Sep 17 00:00:00 2001 From: Pete Gadomski Date: Tue, 7 Feb 2023 07:30:45 -0700 Subject: [PATCH 1/2] feat: remove links from default include list **sqlalchemy** uses `FieldsExtension.default_includes`, so its fix was pretty *easy -- I did have to add an explicit include+exclude check to the search function to ensure the fields extension was applied _only if_ either `include` or `exclude` was set. **pgstac** was a little trickier, because it doesn't use the `default_includes` list. I did modify one existing test -- `test_app_fields_extension` in the **sqlalchemy** tests was assuming fields would be stripped just by providing a `collections` parameter to the query, which I think was incorrect -- I added an explicit `include` field to the test. --- .../extensions/core/fields/fields.py | 1 - .../pgstac/stac_fastapi/pgstac/core.py | 41 ++++++++++++++----- .../pgstac/tests/resources/test_item.py | 15 ++++++- .../stac_fastapi/sqlalchemy/core.py | 4 +- stac_fastapi/sqlalchemy/tests/api/test_api.py | 8 +++- .../sqlalchemy/tests/resources/test_item.py | 14 +++++++ 6 files changed, 67 insertions(+), 16 deletions(-) diff --git a/stac_fastapi/extensions/stac_fastapi/extensions/core/fields/fields.py b/stac_fastapi/extensions/stac_fastapi/extensions/core/fields/fields.py index 93a69a2bc..8c44e1701 100644 --- a/stac_fastapi/extensions/stac_fastapi/extensions/core/fields/fields.py +++ b/stac_fastapi/extensions/stac_fastapi/extensions/core/fields/fields.py @@ -39,7 +39,6 @@ class FieldsExtension(ApiExtension): "stac_version", "geometry", "bbox", - "links", "assets", "properties.datetime", "collection", diff --git a/stac_fastapi/pgstac/stac_fastapi/pgstac/core.py b/stac_fastapi/pgstac/stac_fastapi/pgstac/core.py index a8c73d9f8..d2ee7c205 100644 --- a/stac_fastapi/pgstac/stac_fastapi/pgstac/core.py +++ b/stac_fastapi/pgstac/stac_fastapi/pgstac/core.py @@ -190,29 +190,48 @@ async def _search_base( if include and len(include) == 0: include = None - async def _add_item_links( + features = collection.get("features", []) + if ( + include + and features + and not ( + set(include) + & set(f"properties.{p}" for p in features[0].get("properties", [])) + ) + ): + # If the `include` list is only non-existent fields, keep links + exclude_links = False + else: + exclude_links = ( + search_request.fields.exclude + and "links" in search_request.fields.exclude + ) or ( + search_request.fields.include + and "links" not in search_request.fields.include + ) + + async def _update_item_links( feature: Item, collection_id: Optional[str] = None, item_id: Optional[str] = None, ) -> None: - """Add ItemLinks to the Item. + """Update this item's links. - If the fields extension is excluding links, then don't add them. - Also skip links if the item doesn't provide collection and item ids. + If the fields extension is excluding links, or including other + fields but _not_ links, then remove all links. + Also skip adding links if the item doesn't provide collection and item ids. """ collection_id = feature.get("collection") or collection_id item_id = feature.get("id") or item_id - if ( - search_request.fields.exclude is None - or "links" not in search_request.fields.exclude - and all([collection_id, item_id]) - ): + if not exclude_links and all([collection_id, item_id]): feature["links"] = await ItemLinks( collection_id=collection_id, item_id=item_id, request=request, ).get_links(extra_links=feature.get("links")) + elif exclude_links and "links" in feature: + del feature["links"] cleaned_features: List[Item] = [] @@ -234,12 +253,12 @@ async def _get_base_item(collection_id: str) -> Dict[str, Any]: item_id = feature.get("id") feature = filter_fields(feature, include, exclude) - await _add_item_links(feature, collection_id, item_id) + await _update_item_links(feature, collection_id, item_id) cleaned_features.append(feature) else: for feature in collection.get("features") or []: - await _add_item_links(feature) + await _update_item_links(feature) cleaned_features.append(feature) collection["features"] = cleaned_features diff --git a/stac_fastapi/pgstac/tests/resources/test_item.py b/stac_fastapi/pgstac/tests/resources/test_item.py index 43e1f22ef..9cfb1088a 100644 --- a/stac_fastapi/pgstac/tests/resources/test_item.py +++ b/stac_fastapi/pgstac/tests/resources/test_item.py @@ -1200,11 +1200,22 @@ async def test_field_extension_exclude_links( assert "links" not in resp_json["features"][0] +async def test_field_extension_include_but_not_links( + app_client, load_test_item, load_test_collection +): + # https://github.com/stac-utils/stac-fastapi/issues/395 + body = {"fields": {"include": ["properties.eo:cloud_cover"]}} + resp = await app_client.post("/search", json=body) + assert resp.status_code == 200 + resp_json = resp.json() + assert "links" not in resp_json["features"][0] + + async def test_field_extension_include_only_non_existant_field( app_client, load_test_item, load_test_collection ): - """Including only a non-existant field should return the full item""" - body = {"fields": {"include": ["non_existant_field"]}} + """Including only a non-existent field should return the full item""" + body = {"fields": {"include": ["non_existent_field"]}} resp = await app_client.post("/search", json=body) assert resp.status_code == 200 diff --git a/stac_fastapi/sqlalchemy/stac_fastapi/sqlalchemy/core.py b/stac_fastapi/sqlalchemy/stac_fastapi/sqlalchemy/core.py index 68995d209..6e7a30765 100644 --- a/stac_fastapi/sqlalchemy/stac_fastapi/sqlalchemy/core.py +++ b/stac_fastapi/sqlalchemy/stac_fastapi/sqlalchemy/core.py @@ -481,7 +481,9 @@ def post_search( ) # Use pydantic includes/excludes syntax to implement fields extension - if self.extension_is_enabled("FieldsExtension"): + if self.extension_is_enabled("FieldsExtension") and ( + search_request.fields.include or search_request.fields.exclude + ): if search_request.query is not None: query_include: Set[str] = set( [ diff --git a/stac_fastapi/sqlalchemy/tests/api/test_api.py b/stac_fastapi/sqlalchemy/tests/api/test_api.py index 6fdbb6ed8..c0ab71b28 100644 --- a/stac_fastapi/sqlalchemy/tests/api/test_api.py +++ b/stac_fastapi/sqlalchemy/tests/api/test_api.py @@ -142,7 +142,13 @@ def test_app_fields_extension(load_test_data, app_client, postgres_transactions) item["collection"], item, request=MockStarletteRequest ) - resp = app_client.get("/search", params={"collections": ["test-collection"]}) + resp = app_client.post( + "/search", + json={ + "collections": ["test-collection"], + "fields": {"include": ["datetime"]}, + }, + ) assert resp.status_code == 200 resp_json = resp.json() assert list(resp_json["features"][0]["properties"]) == ["datetime"] diff --git a/stac_fastapi/sqlalchemy/tests/resources/test_item.py b/stac_fastapi/sqlalchemy/tests/resources/test_item.py index b44aa9afc..aaf600230 100644 --- a/stac_fastapi/sqlalchemy/tests/resources/test_item.py +++ b/stac_fastapi/sqlalchemy/tests/resources/test_item.py @@ -879,6 +879,20 @@ def test_field_extension_exclude_default_includes(app_client, load_test_data): assert "geometry" not in resp_json["features"][0] +def test_field_extension_include_but_not_links(app_client, load_test_data): + # https://github.com/stac-utils/stac-fastapi/issues/395 + test_item = load_test_data("test_item.json") + resp = app_client.post( + f"/collections/{test_item['collection']}/items", json=test_item + ) + assert resp.status_code == 200 + body = {"fields": {"include": ["properties.eo:cloud_cover"]}} + resp = app_client.post("/search", json=body) + assert resp.status_code == 200 + resp_json = resp.json() + assert "links" not in resp_json["features"][0] + + def test_search_intersects_and_bbox(app_client): """Test POST search intersects and bbox are mutually exclusive (core)""" bbox = [-118, 34, -117, 35] From ed858bd5b025de4d5862bb0ea3d3bf781d121f60 Mon Sep 17 00:00:00 2001 From: Pete Gadomski Date: Tue, 7 Feb 2023 10:31:03 -0700 Subject: [PATCH 2/2] chore: update changelog --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index b29958eaa..45e263051 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -12,6 +12,7 @@ * Updated CI to test against [pgstac v0.6.12](https://github.com/stac-utils/pgstac/releases/tag/v0.6.12) ([#511](https://github.com/stac-utils/stac-fastapi/pull/511)) * Reworked `update_openapi` and added a test for it ([#523](https://github.com/stac-utils/stac-fastapi/pull/523)) * Limit values above 10,000 are now replaced with 10,000 instead of returning a 400 error ([#526](https://github.com/stac-utils/stac-fastapi/pull/526)) +* Links are no longer part of the default include list ([#524](https://github.com/stac-utils/stac-fastapi/pull/524)) ### Removed