-
Notifications
You must be signed in to change notification settings - Fork 44
fix: fields extension on collections endpoint #326
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
base: main
Are you sure you want to change the base?
Conversation
e4c9d3c to
4aa6542
Compare
f0fbab4 to
a915f2a
Compare
4b57864 to
b5e71b4
Compare
b5e71b4 to
04600a3
Compare
d1df3ca to
4452922
Compare
hrodmn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this @alukach!
| resp_collections = resp_json["collections"] | ||
|
|
||
| assert len(resp_collections) > 0 | ||
| # NOTE: It's a bug that 'collection' is always included; see #327 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should check to make sure this isn't coming from the pgstac side.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
| includes.add(field) | ||
|
|
||
| base_args["fields"] = {"include": includes, "exclude": excludes} | ||
| base_args["fields"] = {"include": list(includes), "exclude": list(excludes)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI
The STAC Search model expects Sets https://github.com/stac-utils/stac-fastapi/blob/52a647af23af5dcb7d3c9a518f40db312bb4be5d/stac_fastapi/extensions/stac_fastapi/extensions/core/fields/request.py#L21-L23
When we call self._clean_search_args in the all_collections method we directly apply json.dumps() on the response but for other methods (e.g item_collection) we put it back within the Pydantic model
stac-fastapi-pgstac/stac_fastapi/pgstac/core.py
Lines 402 to 413 in 990b2af
| clean = self._clean_search_args( | |
| base_args=base_args, | |
| filter_query=filter_expr, | |
| filter_lang=filter_lang, | |
| fields=fields, | |
| sortby=sortby, | |
| **kwargs, | |
| ) | |
| try: | |
| search_request = self.pgstac_search_model(**clean) | |
| except ValidationError as e: |
Hopefully Pydantic is smart enough to convert back end list to a set
from stac_fastapi.api.models import create_post_request_model
from stac_fastapi.pgstac.types.search import PgstacSearch
from stac_fastapi.extensions.core import FieldsExtension
post_request_model = create_post_request_model([FieldsExtension()], base_model=PgstacSearch)
# Normal input (set)
post_request_model(fields={"include": {"yo"}}).fields
>> PostFieldsExtension(include={'yo'}, exclude=set())
# Input with this PR (list)
post_request_model(fields={"include": ["yo"]}).fields
>> PostFieldsExtension(include={'yo'}, exclude=set())
# Check output json (Pydantic transform set to list)
post_request_model(fields={"include": {"yo"}}).model_dump_json()
>> '{"collections":null,"ids":null,"bbox":null,"intersects":null,"datetime":null,"limit":10,"conf":null,"fields":{"include":["yo"],"exclude":[]}}'
I see you marked this comment as off-topic, but I will share that I was going off of: https://github.com/stac-api-extensions/collection-search/blob/v1.0.0-rc.1/README.md?plain=1#L103-L108 |
| if fields: | ||
| return JSONResponse(collections) # type: ignore | ||
|
|
||
| return collections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related Issue(s):
Description:
Currently, the
fieldsextension fails on the Collections endpoint due to the fact that we process theincludeandexcludearguments asset()values and then attempt to convert it to JSON.setvalues are not supported byjson.dumpsand thus an error is thrown.This PR converts these values to a
listbeforejson.dumps().Additionally, I have added a test to validate this behavior. #327 and #328 came out of these tests, requiring the testing to be adjusted slightly.
PR Checklist:
pre-commithooks pass locallymake test)make docs)