Skip to content

Conversation

@vincentsarago
Copy link
Member

@vincentsarago vincentsarago commented Jan 23, 2025

This is not yet ready but will love some feedback @hrodmn @m-mohr

This PR does:

  • add fields, sort and query extension for item_collection endpoint

response_class=self.response_class,
endpoint=create_async_endpoint(self.client.get_queryables, CollectionUri),
)
app.include_router(self.router, tags=["Filter Extension"])
Copy link
Member Author

Choose a reason for hiding this comment

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

register /collections/{col}/queryables endpoint for item_collection

@hrodmn
Copy link
Collaborator

hrodmn commented Jan 23, 2025

@vincentsarago I am sure you have already thought of this but would it make sense to define SearchFilterExtension, ItemCollectionFilterExtension and CollectionSearchFilterExtension in stac-fastapi rather than in this package? Those seem like classes that will be useful for any backend that attempts to support this web of extensions.

@hrodmn
Copy link
Collaborator

hrodmn commented Jan 23, 2025

I am struggling to find the problem that the refactor solves 😅
The reason the existing FilterExtension class could use some refactoring is that a user could decide to not add filter to the list of extensions for the item collection endpoint but the conformance class would still be present because it is automatically included in the conformance classes if you register the extension for item search.

@vincentsarago vincentsarago force-pushed the feature/more-extensions-support branch from cb9824b to d4e229d Compare January 30, 2025 15:47
@vincentsarago vincentsarago marked this pull request as ready for review January 30, 2025 15:47
@vincentsarago
Copy link
Member Author

vincentsarago commented Jan 30, 2025

well it seems I broke something 🙈

Not sure what's going on! The /queryables endpoint should return application/schema+json but right now application/json

from stac_fastapi.extensions.core import FilterExtension, SearchFilterExtension

FilterExtension().response_class
>>> stac_fastapi.api.models.JSONSchemaResponse

SearchFilterExtension().response_class
>>> stac_fastapi.api.models.JSONSchemaResponse

maybe it's something else 😬

Edit: fixed in stac-utils/stac-fastapi#796

@vincentsarago
Copy link
Member Author

@m-mohr
Copy link
Contributor

m-mohr commented Jan 30, 2025

The issue looks unrelated (bands). Weird. Is that maybe an issue with the recently released upgrade of pystac to v1.1.0?

@vincentsarago
Copy link
Member Author

@m-mohr yeah pretty sure this is related

@m-mohr
Copy link
Contributor

m-mohr commented Jan 30, 2025

Pinning pystac to the latest 1.0.0 version probably helps until the tests are migrated to 1.1.0.

@vincentsarago
Copy link
Member Author

this is the error we are getting

            # The way bands were declared in assets changed.
            # In 1.0.0-beta.1 they are inlined into assets as
            # opposed to having indices back into a property-level array.
            if "eo:bands" in obj["properties"]:
                bands = obj["properties"]["eo:bands"]
                for asset in obj["assets"].values():
                    if "eo:bands" in asset:
                        new_bands: list[dict[str, Any]] = []
                        for band_index in asset["eo:bands"]:
>                           new_bands.append(bands[band_index])
E                           TypeError: list indices must be integers or slices, not dict

maybe we have to update our items

"eo:bands": [
{
"gsd": 30,
"name": "SR_B1",
"common_name": "coastal",
"center_wavelength": 0.44,
"full_width_half_max": 0.02
}
],

cc @gadomski @emmanuelmathot

@vincentsarago
Copy link
Member Author

I think we're good, I just tried locally with migrate=False (the behaviour was changed in https://github.com/stac-utils/pystac/compare/v1.12.0..v1.12.1)

@vincentsarago vincentsarago changed the title add query/sort/fields extension support for item_collection and refac… add query/sort/fields extension support for item_collection Jan 30, 2025
@gadomski
Copy link
Member

Yeah, I second-guessed switching migrate after we did it b/c I knew it'd break tests. Might have been a mistake ...

@vincentsarago
Copy link
Member Author

@gadomski it's fine 👍

@vincentsarago vincentsarago merged commit d2749b2 into main Jan 30, 2025
7 checks passed
@vincentsarago vincentsarago deleted the feature/more-extensions-support branch January 30, 2025 21:05
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