-
Notifications
You must be signed in to change notification settings - Fork 102
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
Pgstac v0.4.0b #308
Pgstac v0.4.0b #308
Conversation
…uest to conform with the Queryables spec
…her than hardcoded BaseSearchPostRequest.
…search request model.
@moradology @edkeeble This PR includes the changes from #305 and #307 adding some basic tests to make sure that cql-json and cql2-json are both working. Would be great if you could throw in a review. |
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.
Nice. And thanks for the tests. LGTM 👍
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.
Looks good to me. Thanks for wrapping this up @bitner!
I had one question around the filter_lang
validator and whether we want to transform cql-text
to cql-json
, as well as a suggestion for avoiding some duplicate code, but it isn't critical. If the validator works as you intended, then it's good with me.
retval = v | ||
if values.get("query", None) is not None: | ||
retval = "cql-json" | ||
if values.get("collections", None) is not None: | ||
retval = "cql-json" | ||
if values.get("ids", None) is not None: | ||
retval = "cql-json" | ||
if values.get("datetime", None) is not None: | ||
retval = "cql-json" | ||
if values.get("bbox", None) is not None: | ||
retval = "cql-json" | ||
if v == "cql2-json" and retval == "cql-json": | ||
raise ValueError( | ||
"query, collections, ids, datetime, and bbox" | ||
"parameters are not available in cql2-json" | ||
) | ||
return retval |
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.
cql-text
and cql-json
share the parameters listed here, but if filter_lang
is "cql-text", it will be transformed to "cql-json". Is that the intention?
We might be able to condense this and avoid some duplicate code with the following snippet. It does change the logic slightly, though:
- It will throw if any CQL parameter is provided, even if its value is
null
. e.g.{"filter-lang": "cql2-json", "query": null}
will throw the ValueError. I feel like that is still correct. - It will only default to
cql-json
iffilter-lang
isn't specified (i.e. it will preservecql-text
)
cql_params = ("query", "collections", "ids", "datetime", "bbox")
if any(cql_param in values for cql_param in cql_params):
if v == FilterLang.cql2_json:
raise ValueError(f"{', '.join(invalid_cql2_params[:-1])} and {invalid_cql2_params[-1]} parameters are not available in cql2-json")
else:
return v or FilterLang.cql_json # if `filter-lang` isn't specified and CQL parameters are provided, default to `cql-json`
return v
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.
PGStac only accepts json, so in the case of a GET request, that request is just modified into CQL json. The ultimate plan (which I will start working on this week, but probably won't finish before the holidays) for stac-fastapi-pgstac will be to add a cql2-json output to pygeofilter and to use pygeofilter to translate cql-text or cql2-text into cql2-json and then send that to pgstac.
Is there anything here that should be added in the changelog @moradology @bitner ? |
Probably not a bad idea to mention the bump and CQL2 support. I'll toss up a PR for that. Thanks for keeping an eye out |
Related Issue(s): #283
This PR should supersede #307 and #305
Description:
PR Checklist:
pre-commit run --all-files
)make test
)make docs
)