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

filter-lang parameter not being passed correctly to pgstac backend causing incorrect response for listing items in a collection #29

Open
robintw opened this issue Jan 24, 2022 · 12 comments
Milestone

Comments

@robintw
Copy link
Contributor

robintw commented Jan 24, 2022

I've recently upgraded to the latest version of stac-fastapi, and upgraded my pgstac database too.

I've found problems where going to a URL like /collections/collection-name gives results that include items from all collections, rather than filtering results to just items from a single collection.

I raised an issue on the pgstac repository, as I thought it was a pgstac problem. They told me that it was due to changes in v4 of pgstac, which require the filter-lang parameter to be passed, or alternatively to be set in the pgstac settings to be able to use cql-json rather than cql2-json. I'm continuing to talk to them on that issue as I can't seem to get my change to pgstac settings to be recognised, but I've tested using raw SQL queries and I do get the correct results when I pass the filter-lang parameter.

However, back in stac-fastapi: in stac-utils/stac-fastapi#308, the upgrade was made to use pgstac 4.0.0, and code was added to set filter-lang when certain parameters (like query or collections) were passed as part of the code (see here). However, this filter-lang parameter doesn't actually seem to be getting set.

I've modified the code slightly to add a print statement below this line, and that shows that when visiting a URL like /collections/collection-name, you get JSON of {"collections": ["collection-name"], "limit": 10, "fields": {"include": [], "exclude": []}} - that is, without the filter-lang parameter.

Doing some very hacky string manipulation to set the filter-lang parameter in that JSON fixes the results.

I've been trying to debug this myself, but I don't know much about pydantic and I can't work out how validate_query_uses_cql is being used - it looks like the decorator means it should be called 'automatically' and set the value, but it doesn't seem to be.

I'm wondering whether this problem is also the cause of stac-utils/stac-fastapi#333.

Any help would be much appreciated.

@robintw
Copy link
Contributor Author

robintw commented Jan 24, 2022

Actually, as a quick update: I've found an alteration to the code that seems to work.

Adding filter_lang: Optional[str] = None immediately after:

https://github.com/stac-utils/stac-fastapi/blob/5dc8e0eb63d9cc07663968ed5ed9505e753bf298/stac_fastapi/pgstac/stac_fastapi/pgstac/types/search.py#L16

seems to fix it.

It seems like the filter_lang parameter wasn't actually defined in the model.

I could be completely wrong here, as I'm not experienced with pydantic or FastAPI. If I'm right then I'd be more than happy to do a PR.

@drnextgis
Copy link
Contributor

drnextgis commented Jan 28, 2022

It seems I faced the similar issue here.

Expected behavior

Search using query parameter works properly when QueryExtension is enabled.

Current behavior

Search with query doesn't work. Request example:

curl --request POST \
  --url https://stac-api.***/search \
  --data '{"query": {"title": {"eq": "task stac-utils/stac-fastapi#252"}}}'

It just returns everything.

Workaround

Enabling FilterExtension makes query work as expected 🤷‍♂️

Packages

stac-fastapi.api==2.3.0
stac-fastapi.extensions==2.3.0
stac-fastapi.pgstac==2.3.0
stac-fastapi.types==2.3.0
stac-pydantic==2.0.2
pypgstac==0.4.3

cc @bitner

@robintw
Copy link
Contributor Author

robintw commented Jan 31, 2022

Thanks @drnextgis - I think you're right in this issue and in stac-utils/stac-fastapi#333, it seems like the FilterExtension needs to be enabled to allow various bits of base functionality (like viewing a specific item by ID, or viewing items in a collection). It turned out that I hadn't enabled the FilterExtension in this particular clone of the repo, and by enabling it I don't need to add my line of code to define the filter_lang.

This raises the question of whether the FilterExtension needs to be enabled by default, as it seems some of the base functionality depends on it?

@philvarner
Copy link
Contributor

My opinion is that FilterExtension should not be enabled by default right now, as it is not currently compliant with the latest (1.0.0-beta.5) version of FilterExtension. This would cause all users to be affected by the future change in behavior to become compliant, instead of users opting-in to that.

@robintw
Copy link
Contributor Author

robintw commented Jan 31, 2022

That's a good point, but I'm concerned that it seems that basic functionality like listing the items in a collection (using the URL /collections/collection-name/items) seems to give incorrect results when the filter extension isn't enabled (because it's not passing the filter-lang properly, I think).

@philvarner
Copy link
Contributor

Right, I just think it needs to be fixed somehow without enabling the filter extension.

@vincentsarago
Copy link
Member

can we try to write a test for this?

Maybe @bitner can 👀 this too. If pgstac database requires filter-lang to be passed then it makes sense to make the filter extension enable by default.

@bitner
Copy link
Collaborator

bitner commented Jan 31, 2022

Right now, pgstac using cql2-json does not support use of the ids, collections, datetime, bbox, intersects, or query top level (https://github.com/stac-utils/pgstac/blob/main/CHANGELOG.md#v040). As of now, if you are using cql2-json, everything must be included in the filter. The only stac extension that can be directly enabled/disabled in pgstac is the context extension, otherwise in pgstac filter is always enabled. The (deprecated) query extension is essentially disabled if the filter-lang is set to cql2-json.

There may be a bug in the stac-fastapi-pgstac implementation for collections// and // when used with cql2-json as those endpoints are just passing on the collections= and ids= parameters to the backend which is then ignoring them. I'll work to get a fix in to enable collections, ids, datetime, bbox, and intersects parameters, but I am not planning to add usage of the (deprecated) query parameter along with cql2-json.

Help and PR's are always welcome here or in pgstac! Particularly if there is any documentation or tests that can be added to match a broader set of use cases. Otherwise, I tend to have blinders on towards the way that the implementation of stac-fastapi and pgstac that I am working on has. If anyone would like a walk through of anything to be able to help out, let me know!

@bitner
Copy link
Collaborator

bitner commented Jan 31, 2022

My opinion is that FilterExtension should not be enabled by default right now, as it is not currently compliant with the latest (1.0.0-beta.5) version of FilterExtension. This would cause all users to be affected by the future change in behavior to become compliant, instead of users opting-in to that.

@philvarner As far as my latest reading of the spec, the PGStac cql2-json implementation should be compliant with the latest version (1.0.0-beta.5) version of FilterExtension. It has tests for all the examples that are currently on the spec. Are there any holes or bugs that you are aware of (if so, it would be great if you could add an issue on the pgstac repo).

@philvarner
Copy link
Contributor

@bitner great -- I wasn't expecting the recent changes around the structure of the json to have been made yet, so it's great that they have been.

@bitner bitner self-assigned this Jan 31, 2022
@bitner
Copy link
Collaborator

bitner commented Feb 1, 2022

Support for ids, collections, datetime, intersects, and bbox while using filter-lang=cql2-json has been added to pgstac in the latest pgstac PR (to become pgstac v0.4.4). Once this is merged in pgstac, we can remove some of the auto-version detection which I don't think is working how we want and bump the version in stac-fastapi. This does NOT, however add support for using the deprecated query parameter along with filter-lang=cql2-json.

@duckontheweb
Copy link
Contributor

@robintw @drnextgis Can you confirm whether this issue has been fixed in the most recent version of stac-fastapi?

@gadomski gadomski assigned gadomski and unassigned bitner Jan 31, 2023
@gadomski gadomski added this to the 2.5.0 milestone Mar 2, 2023
@gadomski gadomski transferred this issue from stac-utils/stac-fastapi May 11, 2023
@gadomski gadomski modified the milestones: 2.5.0, 2.4.7 May 15, 2023
@gadomski gadomski removed their assignment May 17, 2023
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

No branches or pull requests

7 participants