Add bbox and datetime to /collections/{collectionId}/items #477

carderne · 2022-10-07T09:46:04Z

Related Issue(s):

Description:
Add bbox and datetime query parameters to the /collections{collection_id}/items route.
Updated base classes, request models and pgstac and sqlalchemy clients.

PR Checklist:

Code is formatted and linted (run pre-commit run --all-files)
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.

Imported str2list function from stac_fastapi.types.search as already have dependency on it.

Using same type signature as get_search

Used the same base_args -> clean from the search routes Probably not necessary in this situation but thought I'd keep it similar (and extensible).

carderne · 2022-10-08T10:58:28Z

Some comments:

Updated the pgstac code to use the base_args dict approach from the search route. Bit of duplication but it seems okay.
For sqlalchemy, I did the same thing with the filtering code from the search route. Could be worth pulling some of that filtering code out into two separate functions, as it's pretty self-contained?
~~Haven't added any tests, but could add some similar to the search tests, just checking bbox and datetime functionality?~~ Added some basic tests.

Then three separate points that probably need attention:

There's some type inconsistency in datetime parameters. In the type defs it's str but in the clients, it's Union[str, datetime]. This causes a type issue here calling .split() on it. It should probably just be str everywhere, as it's supposed to allow /-separated ranges. Could also type it more cleverly and check before spliting.
datetime parameters shadow from datetime import datetime eg here. Should probably rename to datetime_range or something.
bbox is alternately types as List[NumType], str and BBox. Should probably be BBox everywhere, and the str2list converter could be a bit more aggressive in converting it into a BBox (a tuple of floats, not just a list of anything). For now I did this but it's not great.

Copy the bbox and datetime filtering from the search routes

m-mohr · 2022-10-14T08:34:12Z

stac_fastapi/api/stac_fastapi/api/models.py

@@ -123,6 +124,8 @@ class ItemCollectionUri(CollectionUri):
    """Get item collection."""

    limit: int = attr.ib(default=10)
+    bbox: Optional[str] = attr.ib(default=None, converter=str2list)


Would it make sense to convert this directly to e.g. Optional[List[float]] for bbox and Optional[List[Union[datetime, None]]] for datetime so that you don't need to handle the conversion issues downstream?

Yes I mentioned that in my comment above point 3.

I held off on changing it until I get some feedback, as that is the current behavior in the similar routes.

datetime is a little more complicated, because it also supports ranges like 2020-01-01/2022-01-01 or 2020-01-01/.., so it must allow str input (or a clever pydantic type enforcing either datetime or something with a slash in it).

Ah, I did not see that comment. Still, a list of datetime/None should work. None could be used for open date ranges as specified also for STAC extents in collections...

Oh sorry I misunderstood you. Yes that makes sense, but as I've looked into this I've found some inconsistency.

For sqlalchemy, I added code based on the post_search implementation: it has some complicated logic on deciding how to treat the datetime parameter, and treats a single datetime as an exact datetime. It's also more lenient on datetime formatting.

For pgstac, I just used post_request_model, which treats a single value as an end-date, and is stricter on enforcing RFC3339.

So a few questions:

Should we update the sqlalchemy post_search and item_collection routes to use post_request_model?

Should a single datetime be considered as an end date? It seems much more useful (if less obvious) than the unlikely case you're able to hit on the exact datetime of an item...

If the latter, datetime could be typed as (being pedantic): None | tuple[datetime, datetime] | tuple[datetime, None] | tuple[None, datetime]. This is all already validated in post_request_model (see the last link above). I think actually enforcing this typing would break the pgstac implementation, as it passes the raw datetime/datetime string to the DB's search function.

Should we update the sqlalchemy post_search and item_collection routes to use post_request_model?

I think the sqlalchemy backend would be better if, like pgstac, it had one function (_search_base) which contains most of the important query logic and is wrapped by all of the other methods.

Should a single datetime be considered as an end date? It seems much more useful (if less obvious) than the unlikely case you're able to hit on the exact datetime of an item...

My interpretation of the OAF datetime parameter is a single date time is an exact date time (==) while date time ranges are either ../end_date or start_date/... I agree that == is arguably less useful when searching than a datetime range but the distinction is there to let users do both.

If the latter, datetime could be typed as (being pedantic): None | tuple[datetime, datetime] | tuple[datetime, None] | tuple[None, datetime]. This is all already validated in post_request_model (see the last link above). I think actually enforcing this typing would break the pgstac implementation, as it passes the raw datetime/datetime string to the DB's search function.

Lol I should have read this thread first, I proposed the same thing in PR comment below. We shouldn't be too concerned about breaking backend implementations w/ changes to the API layer as long as those changes make the API layer stronger and improve separation of concerns (I think here they do). Backend implementations shouldn't have to guess what information is included in a request, that information should be presented in a way that is clear and easy to use and I'm not in love with the current solution for datetime. Pgstac can always coerce these back to a string.

geospatial-jeff · 2022-10-17T02:49:13Z

Sorry its taken me a while to get around to this.

There's some type inconsistency in datetime parameters. In the type defs it's str but in the clients, it's Union[str, datetime]. This causes a type issue here calling .split() on it. It should probably just be str everywhere, as it's supposed to allow /-separated ranges. Could also type it more cleverly and check before spliting.

I agree that the Union[str, datetime] is messy. I think str is more ambiguous because the spec allows both single dates and date time ranges. If we were to make changes to datetime handling I would prefer for the API layer to coerce the datetime parameters to a tuple of datetime objects. Maybe (None, datetime.datetime), (datetime.datetime, None) for single tailed date ranges and (datetime.datetime, datetime.datetime) for two tailed date ranges.

datetime parameters shadow from datetime import datetime eg here. Should probably rename to datetime_range or something.

Parameters aim to use the same language as what is in the API spec for clarity. I'm fine changing this but I don't think its too straight forward. Pydantic does support aliasing but I don't believe attrs does.

bbox is alternately types as List[NumType], str and BBox. Should probably be BBox everywhere, and the str2list converter could be a bit more aggressive in converting it into a BBox (a tuple of floats, not just a list of anything). For now I did this but it's not great.

I agree with all of this, bbox should be consistently typed as BBox and str2list should be more aggressive.

geospatial-jeff

This LGTM, thanks for the PR! We can continue discussion in this PR, but if its okay with you @carderne I think any other changes should become new issues / new PRs.

carderne · 2022-10-17T07:49:46Z

Sounds good!

To sort-of summarise the other issues:

Coerce datetime to a well-typed tuple at the API surface, fix in the backends.
Same for bbox.
Datetime treatment should follow the spec. Single date is ==; intervals don't need the .. before/after the /.
Update sqlalchemy backend to use a single search route.
datetime shadowing. Easiest way would be to from datetime import datetime as dt_lib (or something less horrible). As it is now, this if statement (and probably others) is broken.

carderne · 2022-10-22T16:00:24Z

@geospatial-jeff I don’t think I can merge this

carderne · 2022-10-31T11:53:26Z

Fixed conflicts introduced by #482.

carderne · 2022-11-05T18:16:08Z

Fixed the linting issue from the merge above

carderne · 2022-11-05T18:21:19Z

@geospatial-jeff @bitner

Just bumping, any reason not to merge?

bitner

Sorry, didn't see this until just now. Looks good to me.

carderne added 3 commits October 7, 2022 10:05

Add bbox and datetime to ItemCollectionUri

d9480c1

Imported str2list function from stac_fastapi.types.search as already have dependency on it.

Add bbox and datetime to ABC item_collection

de9d18b

Using same type signature as get_search

Add bbox and datetime to PgStac item_collection

26cd5f3

Used the same base_args -> clean from the search routes Probably not necessary in this situation but thought I'd keep it similar (and extensible).

carderne force-pushed the master branch 2 times, most recently from debac82 to 158e2c4 Compare October 7, 2022 13:20

Update CHANGES.md

35ed1c2

carderne force-pushed the master branch from 158e2c4 to 935dcdb Compare October 8, 2022 10:08

carderne marked this pull request as ready for review October 8, 2022 10:59

carderne added 3 commits October 10, 2022 08:18

Add bbox and datetime to SQLAlchemy item_collection

a68abdf

Copy the bbox and datetime filtering from the search routes

Fix: convert SQLAlchemy bbox to list[int]

8281744

Add item_collection bbox/datetime tests

3ea9232

carderne force-pushed the master branch from 79e97aa to 3ea9232 Compare October 10, 2022 07:18

carderne mentioned this pull request Oct 14, 2022

/collections/{collectionId}/items endpoint only does not apply bbox or datetime parameters to results #380

Closed

m-mohr reviewed Oct 14, 2022

View reviewed changes

geospatial-jeff self-requested a review October 17, 2022 03:04

geospatial-jeff approved these changes Oct 17, 2022

View reviewed changes

Merge branch 'master' into master

4ec751e

carderne force-pushed the master branch from 3321c7b to 4ec751e Compare November 5, 2022 18:15

bitner approved these changes Nov 7, 2022

View reviewed changes

bitner merged commit 06218c5 into stac-utils:master Nov 7, 2022

carderne mentioned this pull request Nov 9, 2022

Properly type bbox and datetime #490

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add bbox and datetime to /collections/{collectionId}/items #477

Add bbox and datetime to /collections/{collectionId}/items #477

Uh oh!

carderne commented Oct 7, 2022 •

edited

Loading

Uh oh!

carderne commented Oct 8, 2022 •

edited

Loading

Uh oh!

m-mohr Oct 14, 2022 •

edited

Loading

Uh oh!

carderne Oct 14, 2022

Uh oh!

carderne Oct 14, 2022

Uh oh!

m-mohr Oct 14, 2022

Uh oh!

carderne Oct 14, 2022

Uh oh!

geospatial-jeff Oct 17, 2022

Uh oh!

geospatial-jeff commented Oct 17, 2022

Uh oh!

geospatial-jeff left a comment

Uh oh!

carderne commented Oct 17, 2022 •

edited

Loading

Uh oh!

carderne commented Oct 22, 2022

Uh oh!

carderne commented Oct 31, 2022

Uh oh!

carderne commented Nov 5, 2022

Uh oh!

carderne commented Nov 5, 2022

Uh oh!

bitner left a comment

Uh oh!

Uh oh!

Add bbox and datetime to /collections/{collectionId}/items #477

Add bbox and datetime to /collections/{collectionId}/items #477

Uh oh!

Conversation

carderne commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carderne commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-mohr Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carderne Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

carderne Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

m-mohr Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

carderne Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

geospatial-jeff Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

geospatial-jeff commented Oct 17, 2022

Uh oh!

geospatial-jeff left a comment

Choose a reason for hiding this comment

Uh oh!

carderne commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carderne commented Oct 22, 2022

Uh oh!

carderne commented Oct 31, 2022

Uh oh!

carderne commented Nov 5, 2022

Uh oh!

carderne commented Nov 5, 2022

Uh oh!

bitner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carderne commented Oct 7, 2022 •

edited

Loading

carderne commented Oct 8, 2022 •

edited

Loading

m-mohr Oct 14, 2022 •

edited

Loading

carderne commented Oct 17, 2022 •

edited

Loading