Skip to content
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

add collection search extension #136

Merged
merged 19 commits into from
Oct 2, 2024

Conversation

hrodmn
Copy link
Collaborator

@hrodmn hrodmn commented Jul 26, 2024

Related Issue(s):

Description:
Adds the collection search extension by pointing the all_collections method at the pgstac function collection_search instead of all_collections.

I have not tested to see if paging is implemented yet, need to upload a bunch of collections to my local db I guess! 😅

PR Checklist:

  • pre-commit hooks pass locally
  • Tests pass (run make test)
  • Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
  • Changes are added to the CHANGELOG.

@m-mohr
Copy link
Contributor

m-mohr commented Aug 1, 2024

Great work. A queryables link seems to be missing in GET /collections, right?

@vincentsarago
Copy link
Member

Great work. A queryables link seems to be missing in GET /collections, right?

@m-mohr correct, we will get the changes we just pushed to main from #89

@hrodmn hrodmn force-pushed the collection-search branch 2 times, most recently from dab6638 to 4ab53fa Compare August 2, 2024 12:05
@hrodmn hrodmn marked this pull request as ready for review August 2, 2024 12:09
@hrodmn hrodmn force-pushed the collection-search branch from 4ab53fa to 2c64fc2 Compare August 2, 2024 12:17
@hrodmn
Copy link
Collaborator Author

hrodmn commented Aug 2, 2024

Great work. A queryables link seems to be missing in GET /collections, right?

@m-mohr correct, we will get the changes we just pushed to main from #89

I rebased on main so this branch should be up-to-date with the queryables link now.

@hrodmn
Copy link
Collaborator Author

hrodmn commented Aug 19, 2024

@vincentsarago are you worried about the possibility of inconsistencies between the arguments in PgstacSearch and the collection-search GET request model? I believe the collection-search GET request should have a subset of the parameters from item-level search and never any extra parameters. If I pass a collections_get_request_model that should limit the args that get passed to PgstacSearch in the all_collections method to the ones that are available in the collection-search endpoint with all of the enabled extensions.

I'm sorry about all of the force pushes, I had to clean some things up in order to make the changes mergeable with main 😅

stac_fastapi/pgstac/app.py Outdated Show resolved Hide resolved
Comment on lines +45 to +47
bbox: Optional[BBox] = None,
datetime: Optional[DateTimeType] = None,
limit: Optional[int] = None,
Copy link
Collaborator

@alukach alukach Aug 19, 2024

Choose a reason for hiding this comment

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

Thoughts from discussion: consider adding id to the endpoint, despite it being omitted from the Collection Search Extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that the same as just calling /collections/:id ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The use-case for including an ids parameter would be to limit the scope of a search in the context of scoped authentication for a STAC API, but we discussed some more and it probably makes more sense to use the filter extension for injecting scope limits in a search request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, ids would be a better parameter name than id to evoke the ability to provide multiple as a filter. And for our specific needs today, we will make use of filter as @hrodmn said. I believe it was @bitner who suggested adding the ability to filter by ID parameter, I'll let him weigh in on whether we should go without.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense just to be parallel to the items spec (and yes, "ids" plural which is how it works in items)

Copy link
Contributor

Choose a reason for hiding this comment

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

If you add it, I'd recommend to add a separate conformance class for it so that clients actually know whether it's supported or not.

PS: ids is not included in collection search as we just inherit from OGC API - Records, which doesn't have it. It's orthogonal to how ids is not part of OGC API - Features for items. ids is a STAC-specific thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unless someone else feels strongly, I think we can leave ids out since it is not in the extension spec and if a user knows the collection IDs they could just make several calls to /collections/{id}.

@hrodmn
Copy link
Collaborator Author

hrodmn commented Sep 12, 2024

@vincentsarago I think we need a new release of stac-fastapi with the changes from stac-utils/stac-fastapi#745 in order to properly configure the collection-search extension with the other configured extensions.

@vincentsarago
Copy link
Member

@hrodmn yes, I waited to see if we could also have stac-utils/stac-fastapi#744 merged but I haven't had a lot of time recently to work on this.
I'll target next week to make a new release 🙏

@hrodmn
Copy link
Collaborator Author

hrodmn commented Sep 27, 2024

It looks like I am having some datetime json issues in Python 3.10, I'll figure out a fix.

Copy link
Collaborator

@alukach alukach left a comment

Choose a reason for hiding this comment

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

LGTM. Left some very non-critical suggestions.

CHANGES.md Outdated Show resolved Hide resolved
stac_fastapi/pgstac/app.py Show resolved Hide resolved
@hrodmn
Copy link
Collaborator Author

hrodmn commented Oct 2, 2024

@vincentsarago thanks for your help getting this cleaned up, I think everything is looking good. I added some commits based on @alukach's suggestions. The only outstanding conversation is about the ids parameter which might be occasionally convenient but I think it would be ok to proceed without it since it's not in the collection search extension spec.

@vincentsarago vincentsarago merged commit 9a3797a into stac-utils:main Oct 2, 2024
7 checks passed
@hrodmn hrodmn deleted the collection-search branch October 8, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants