Skip to content

fix type for advanced freetext #263

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Fixed

- fix root-path handling when setting via env var or on app instance
- Allow `q` parameter to be a `str` not a `list[str]` for Advanced Free-Text extension

### Changed

Expand Down
36 changes: 25 additions & 11 deletions stac_fastapi/pgstac/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ async def all_collections( # noqa: C901
sortby: Optional[str] = None,
filter_expr: Optional[str] = None,
filter_lang: Optional[str] = None,
q: Optional[List[str]] = None,
**kwargs,
**kwargs: Any,
Copy link
Member Author

Choose a reason for hiding this comment

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

In this PR we changed and we now forward kwargs to _clean_search_args method.

Choose a reason for hiding this comment

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

What happens if POST /search with {"q": 123} is submitted? Does q make it all the way to the DB and raises due to invalid types? There won't be any early API model validation of the parameter?

) -> Collections:
"""Cross catalog search (GET).

Expand Down Expand Up @@ -86,7 +85,7 @@ async def all_collections( # noqa: C901
sortby=sortby,
filter_query=filter_expr,
filter_lang=filter_lang,
q=q,
**kwargs,
)

async with request.app.state.get_connection(request, "r") as conn:
Expand Down Expand Up @@ -157,7 +156,10 @@ async def all_collections( # noqa: C901
)

async def get_collection(
self, collection_id: str, request: Request, **kwargs
self,
collection_id: str,
request: Request,
**kwargs: Any,
) -> Collection:
"""Get collection by id.

Expand Down Expand Up @@ -202,7 +204,9 @@ async def get_collection(
return Collection(**collection)

async def _get_base_item(
self, collection_id: str, request: Request
self,
collection_id: str,
request: Request,
) -> Dict[str, Any]:
"""Get the base item of a collection for use in rehydrating full item collection properties.

Expand Down Expand Up @@ -359,7 +363,7 @@ async def item_collection(
filter_expr: Optional[str] = None,
filter_lang: Optional[str] = None,
token: Optional[str] = None,
**kwargs,
**kwargs: Any,
) -> ItemCollection:
"""Get all items from a specific collection.

Expand Down Expand Up @@ -391,6 +395,7 @@ async def item_collection(
filter_lang=filter_lang,
fields=fields,
sortby=sortby,
**kwargs,
)

try:
Expand All @@ -417,7 +422,11 @@ async def item_collection(
return ItemCollection(**item_collection)

async def get_item(
self, item_id: str, collection_id: str, request: Request, **kwargs
self,
item_id: str,
collection_id: str,
request: Request,
**kwargs: Any,
) -> Item:
"""Get item by id.

Expand Down Expand Up @@ -445,7 +454,10 @@ async def get_item(
return Item(**item_collection["features"][0])

async def post_search(
self, search_request: PgstacSearch, request: Request, **kwargs
self,
search_request: PgstacSearch,
request: Request,
**kwargs: Any,
) -> ItemCollection:
"""Cross catalog search (POST).

Expand Down Expand Up @@ -489,7 +501,7 @@ async def get_search(
filter_expr: Optional[str] = None,
filter_lang: Optional[str] = None,
token: Optional[str] = None,
**kwargs,
**kwargs: Any,
) -> ItemCollection:
"""Cross catalog search (GET).

Expand All @@ -516,6 +528,7 @@ async def get_search(
sortby=sortby,
filter_query=filter_expr,
filter_lang=filter_lang,
**kwargs,
)

try:
Expand Down Expand Up @@ -550,7 +563,8 @@ def _clean_search_args( # noqa: C901
sortby: Optional[str] = None,
filter_query: Optional[str] = None,
filter_lang: Optional[str] = None,
q: Optional[List[str]] = None,
q: Optional[Union[str, List[str]]] = None,
**kwargs: Any,
) -> Dict[str, Any]:
"""Clean up search arguments to match format expected by pgstac"""
if filter_query:
Expand Down Expand Up @@ -596,7 +610,7 @@ def _clean_search_args( # noqa: C901
base_args["fields"] = {"include": includes, "exclude": excludes}

if q:
base_args["q"] = " OR ".join(q)
base_args["q"] = " OR ".join(q) if isinstance(q, list) else q

# Remove None values from dict
clean = {}
Expand Down
61 changes: 57 additions & 4 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import json
import logging
import os
import time
from typing import Callable, Dict
from urllib.parse import quote_plus as quote
from urllib.parse import urljoin
Expand All @@ -26,6 +25,7 @@
CollectionSearchExtension,
CollectionSearchFilterExtension,
FieldsExtension,
FreeTextAdvancedExtension,
FreeTextExtension,
ItemCollectionFilterExtension,
OffsetPaginationExtension,
Expand Down Expand Up @@ -139,6 +139,7 @@ def api_client(request):
FieldsExtension(),
SearchFilterExtension(client=FiltersClient()),
TokenPaginationExtension(),
FreeTextExtension(), # not recommended by PgSTAC
]
application_extensions.extend(search_extensions)

Expand Down Expand Up @@ -167,6 +168,7 @@ def api_client(request):
FieldsExtension(conformance_classes=[FieldsConformanceClasses.ITEMS]),
ItemCollectionFilterExtension(client=FiltersClient()),
TokenPaginationExtension(),
FreeTextExtension(), # not recommended by PgSTAC
]
application_extensions.extend(item_collection_extensions)

Expand Down Expand Up @@ -207,7 +209,6 @@ async def app(api_client, database):
pgdatabase=database.dbname,
)
logger.info("Creating app Fixture")
time.time()
app = api_client.app
await connect_to_db(
app,
Expand Down Expand Up @@ -314,7 +315,6 @@ async def app_no_ext(database):
pgdatabase=database.dbname,
)
logger.info("Creating app Fixture")
time.time()
await connect_to_db(
api_client_no_ext.app,
postgres_settings=postgres_settings,
Expand Down Expand Up @@ -354,7 +354,6 @@ async def app_no_transaction(database):
pgdatabase=database.dbname,
)
logger.info("Creating app Fixture")
time.time()
await connect_to_db(
api.app,
postgres_settings=postgres_settings,
Expand Down Expand Up @@ -402,3 +401,57 @@ async def default_client(default_app):
transport=ASGITransport(app=default_app), base_url="http://test"
) as c:
yield c


@pytest.fixture(scope="function")
async def app_advanced_freetext(database):
"""Default stac-fastapi-pgstac application without only the transaction extensions."""
api_settings = Settings(testing=True)

application_extensions = [
TransactionExtension(client=TransactionsClient(), settings=api_settings)
]

collection_extensions = [
FreeTextAdvancedExtension(),
OffsetPaginationExtension(),
]
collection_search_extension = CollectionSearchExtension.from_extensions(
collection_extensions
)
application_extensions.append(collection_search_extension)

app = StacApi(
settings=api_settings,
extensions=application_extensions,
client=CoreCrudClient(),
health_check=health_check,
collections_get_request_model=collection_search_extension.GET,
)

postgres_settings = PostgresSettings(
pguser=database.user,
pgpassword=database.password,
pghost=database.host,
pgport=database.port,
pgdatabase=database.dbname,
)
logger.info("Creating app Fixture")
await connect_to_db(
app.app,
postgres_settings=postgres_settings,
add_write_connection_pool=True,
)
yield app.app
await close_db_connection(app.app)

logger.info("Closed Pools.")


@pytest.fixture(scope="function")
async def app_client_advanced_freetext(app_advanced_freetext):
logger.info("creating app_client")
async with AsyncClient(
transport=ASGITransport(app=app_advanced_freetext), base_url="http://test"
) as c:
yield c
65 changes: 65 additions & 0 deletions tests/resources/test_collection.py

Choose a reason for hiding this comment

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

There are no tests for the /search?q= and /collections/{col}/items?q=... cases?

Also, there could be POST /search with {"q": "advanced AND search"} or {"q": ["basic", "search"]}.

Copy link
Member

@gadomski gadomski Jul 24, 2025

Choose a reason for hiding this comment

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

I don't believe this PR is meant to implement free-text item search, just correct some stuff around free-text collection search.

Choose a reason for hiding this comment

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

Function self._clean_search_args calls are updated with the **kwargs, so q will now trickle down within these calls as well (which is good), and should be considered (but could be in a separate PR though not to block this one).

Copy link
Member Author

Choose a reason for hiding this comment

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

q won't be in /search because we don't usually use the free-text extension for items

if you're passing a dict, pydantic should raise a validation https://github.com/stac-utils/stac-fastapi/blob/fa42985255fad0bab7dbe3aadbf1f74cb1635f3a/stac_fastapi/extensions/stac_fastapi/extensions/core/free_text/request.py#L37-L43

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't believe this PR is meant to implement free-text item search, just correct some stuff around free-text collection search.

Exactly, but it also enables free-text for items by using kwargs, as for other unknown extension people would want to implement. Now they would have to just pass a custom _clean_search_args to support any kind of input passed through kwargs

Choose a reason for hiding this comment

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

OK. Good point about the Pydantic model.

I'm not sure to understand the "q won't be in /search".
Isn't it available if the conformance is applied?
https://github.com/stac-utils/stac-fastapi/blob/fa42985255fad0bab7dbe3aadbf1f74cb1635f3a/stac_fastapi/extensions/stac_fastapi/extensions/core/free_text/free_text.py#L27-L40

I was able to activate it is my implementation (https://github.com/crim-ca/stac-app/pull/28/files). It should do the title/description/keywords free-text search across all collections' items, as if filter or query was used, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure to understand the "q won't be in /search".

I just mean that q parameter will ONLY be available if enabled at the application level.

As mentioned in #263 (comment) it works only for Advanced because we don't do str -> list -> str transformation but keep the values as str

Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,71 @@ async def test_collection_search_freetext(
assert resp.json()["collections"][0]["id"] == load_test2_collection.id

resp = await app_client.get(
"/collections",
params={"q": "temperature,calibrated"},
)
assert resp.json()["numberReturned"] == 2
assert resp.json()["numberMatched"] == 2
assert len(resp.json()["collections"]) == 2

resp = await app_client.get(
"/collections",
params={"q": "temperature,yo"},
Copy link
Member

Choose a reason for hiding this comment

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

)
assert resp.json()["numberReturned"] == 1
assert resp.json()["numberMatched"] == 1
assert len(resp.json()["collections"]) == 1
assert resp.json()["collections"][0]["id"] == load_test2_collection.id

resp = await app_client.get(
"/collections",
params={"q": "nosuchthing"},
)
assert len(resp.json()["collections"]) == 0


@requires_pgstac_0_9_2
@pytest.mark.asyncio
async def test_collection_search_freetext_advanced(
app_client_advanced_freetext, load_test_collection, load_test2_collection
):
# free-text
resp = await app_client_advanced_freetext.get(
"/collections",
params={"q": "temperature"},
)
assert resp.json()["numberReturned"] == 1
assert resp.json()["numberMatched"] == 1
assert len(resp.json()["collections"]) == 1
assert resp.json()["collections"][0]["id"] == load_test2_collection.id

resp = await app_client_advanced_freetext.get(
"/collections",
params={"q": "temperature,calibrated"},
)
assert resp.json()["numberReturned"] == 2
assert resp.json()["numberMatched"] == 2
assert len(resp.json()["collections"]) == 2

resp = await app_client_advanced_freetext.get(
"/collections",
params={"q": "temperature,yo"},
)
assert resp.json()["numberReturned"] == 1
assert resp.json()["numberMatched"] == 1
assert len(resp.json()["collections"]) == 1
assert resp.json()["collections"][0]["id"] == load_test2_collection.id

resp = await app_client_advanced_freetext.get(
"/collections",
params={"q": "temperature OR yo"},
)
assert resp.json()["numberReturned"] == 1
assert resp.json()["numberMatched"] == 1
assert len(resp.json()["collections"]) == 1
assert resp.json()["collections"][0]["id"] == load_test2_collection.id

resp = await app_client_advanced_freetext.get(
"/collections",
params={"q": "nosuchthing"},
)
Expand Down
55 changes: 55 additions & 0 deletions tests/resources/test_item.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

from stac_fastapi.pgstac.models.links import CollectionLinks

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")
Expand Down Expand Up @@ -1689,3 +1691,56 @@ async def test_get_search_link_media(app_client):
assert len(links) == 2
get_self_link = next((link for link in links if link["rel"] == "self"), None)
assert get_self_link["type"] == "application/geo+json"


@requires_pgstac_0_9_2
@pytest.mark.asyncio
async def test_item_search_freetext(app_client, load_test_data, load_test_collection):
test_item = load_test_data("test_item.json")
resp = await app_client.post(
f"/collections/{test_item['collection']}/items", json=test_item
)
assert resp.status_code == 201

# free-text
resp = await app_client.get(
"/search",
params={"q": "temperature"},
)
print(resp.json())
# assert resp.json()["numberReturned"] == 1
# assert resp.json()["numberMatched"] == 1
# assert len(resp.json()["collections"]) == 1
# assert resp.json()["collections"][0]["id"] == load_test2_collection.id

# resp = await app_client_advanced_freetext.get(
# "/collections",
# params={"q": "temperature,calibrated"},
# )
# assert resp.json()["numberReturned"] == 2
# assert resp.json()["numberMatched"] == 2
# assert len(resp.json()["collections"]) == 2

# resp = await app_client_advanced_freetext.get(
# "/collections",
# params={"q": "temperature,yo"},
# )
# assert resp.json()["numberReturned"] == 1
# assert resp.json()["numberMatched"] == 1
# assert len(resp.json()["collections"]) == 1
# assert resp.json()["collections"][0]["id"] == load_test2_collection.id

# resp = await app_client_advanced_freetext.get(
# "/collections",
# params={"q": "temperature OR yo"},
# )
# assert resp.json()["numberReturned"] == 1
# assert resp.json()["numberMatched"] == 1
# assert len(resp.json()["collections"]) == 1
# assert resp.json()["collections"][0]["id"] == load_test2_collection.id

# resp = await app_client_advanced_freetext.get(
# "/collections",
# params={"q": "nosuchthing"},
# )
# assert len(resp.json()["collections"]) == 0
Loading