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 filter extension #165

Merged
merged 23 commits into from
Aug 18, 2021
Merged

Conversation

rsmith013
Copy link

@rsmith013 rsmith013 commented Jun 30, 2021

Adding filter extension. Also making changes to other extensions to make it easier to dynamically build the conformance classes from the implemented extensions.

rsmith013 added 6 commits June 29, 2021 15:32
@@ -37,6 +38,10 @@ class FieldsExtension(ApiExtension):
)
)

conformance_classes: List[str] = attr.ib(
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be class variables, as they won't change between instances of the class.

Copy link
Author

Choose a reason for hiding this comment

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

While that is true in most cases, it is not in all. The filter extension is a good example. As the filter extension is complex, there are varying degrees of implementation. There is a base level to be compliant but then there are other conformance classes that are optional and implementation-specific. Having these as instance variables allows the implementor to specify which classes their implementation conforms to.

https://github.com/radiantearth/stac-api-spec/tree/master/fragments/filter#conformance-classes

@@ -89,7 +88,7 @@ def item_collection(
.order_by(self.item_table.datetime.desc(), self.item_table.id)
)
count = None
if self.extension_is_enabled(ContextExtension):
if self.extension_is_enabled("ContextExtension"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reasoning behind this change?

Copy link
Author

Choose a reason for hiding this comment

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

This change links somewhat to the thinking which spawned #178. It would be really good to try and un-bake the extensions from the core client. The reasoning behind this change is to remove the need to import the Extension classes to check if the extension is active. The client instance has the Extension instances passed to it at runtime so I changed the is_extension_enabled method to enable simple name checking https://github.com/rsmith013/stac-fastapi/blob/247098fbb28345096080ea161158ce4461868abb/stac_fastapi/types/stac_fastapi/types/core.py#L127


def list_conformance_classes(self):
"""Return a list of conformance classes, including implemented extensions."""
base_conformance = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is the set of conformance classes we want to use #159

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, these are the old conformance classes here. #159 has the most up to date ones.

Copy link
Author

@rsmith013 rsmith013 Jul 6, 2021

Choose a reason for hiding this comment

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

presumably we are on beta2 now.

e.g. https://api.stacspec.org/v1.0.0-beta.2/core

Copy link
Collaborator

@philvarner philvarner Jul 6, 2021

Choose a reason for hiding this comment

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

Correct, beta.2

@geospatial-jeff
Copy link
Collaborator

This also closes #159 by adding conformance classes to the landing page.

conformance_classes: List[str] = attr.ib(
default=[
"https://api.stacspec.org/v1.0.0-beta.2/item-search#filter",
"https://api.stacspec.org/v1.0.0-beta.2/item-search#filter:simple-cql",
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, these are about to change. radiantearth/stac-api-spec#163

Copy link
Author

Choose a reason for hiding this comment

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

Have changed the conformance classes to:

        default=[
            "https://api.stacspec.org/v1.0.0-beta.2/item-search#filter:basic-cql",
            "https://api.stacspec.org/v1.0.0-beta.2/item-search#filter:item-search-filter",
            "https://api.stacspec.org/v1.0.0-beta.2/item-search#filter:cql-text",
        ]

this was my interpretation of the PR where the basic spatial and temporal operators were recommended. I added cql-text as default, as item-search must implement GET search. It would follow that the base implementation of the filters extension would also be a GET operation using cql-text

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I was actually just trying to let you know that they were going to change for beta.3. For beta.2, it should still use simple-cql

Copy link
Author

Choose a reason for hiding this comment

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

oh, ok. will revert.

@rsmith013 rsmith013 force-pushed the add-filter-extension branch from 89052c7 to 1f31da2 Compare July 6, 2021 16:47
@@ -141,6 +157,7 @@ def landing_page(self, **kwargs) -> LandingPage:
id=self.landing_page_id,
title=self.title,
description=self.description,
conformsTo=self.list_conformance_classes(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it will fix #169

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rsmith013 I thought this would fix #169, but when I ran this using docker-compose from your branch, the landing page conformsTo is still empty. maybe i'm not running it right? This is a pretty major bug in stac-fastapi, so I just wanted to make sure it gets fixed. Do you see conformsTo in the landing page when you run it locally?

Copy link
Author

Choose a reason for hiding this comment

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

yes:
image

@philvarner
Copy link
Collaborator

philvarner commented Jul 8, 2021 via email

@lossyrob
Copy link
Member

Making sure I understand - this PR adds a filter extension that implements the queryables routes, but does not implement any of CQL "filter" property on the Search object, correct? So it's up to backends to implement the actual CQL filtering, but this PR adds the necessary top level routes that are added by the extension? If I'm getting that correct, this seems like a good change that will set up the next stage of work which is to add CQL support to the pgstac and sqlalchemy backends.

@rsmith013 Can you merge in the latest master, and I'll take a closer look next week? Thanks!

rsmith013 added 2 commits July 26, 2021 12:06
…ork. Modified async classes to use name based extension matching. Matching base core client.
@rsmith013
Copy link
Author

@lossyrob Yes, that was the plan.

@lossyrob
Copy link
Member

Looking good; going to leave this off of the 2.0.0 release because there's a TODO around implementing the Async core client.

@rsmith013
Copy link
Author

@lossyrob I didn't realise you were that close to a release. Added the Async base and merged master. Should be good to go

@lossyrob
Copy link
Member

Thanks! We decided to pop one off since it had been so long. Let use kick the tires for the CI-based publishing as well. There's a few issues so I'm sure we're going to have to fix up soon so I'm expecting a 2.1 release in the next week or so

@moradology
Copy link
Collaborator

PR #214 ought to address conformance (issue #169) if you want to pull that behavior from here as mentioned in #207

@lossyrob lossyrob merged commit e590e75 into stac-utils:master Aug 18, 2021
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.

6 participants