-
Notifications
You must be signed in to change notification settings - Fork 112
Add bbox and datetime to /collections/{collectionId}/items #477
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
d9480c1
Add bbox and datetime to ItemCollectionUri
carderne de9d18b
Add bbox and datetime to ABC item_collection
carderne 26cd5f3
Add bbox and datetime to PgStac item_collection
carderne 35ed1c2
Update CHANGES.md
carderne a68abdf
Add bbox and datetime to SQLAlchemy item_collection
carderne 8281744
Fix: convert SQLAlchemy bbox to list[int]
carderne 3ea9232
Add item_collection bbox/datetime tests
carderne 4ec751e
Merge branch 'master' into master
carderne File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Would it make sense to convert this directly to e.g.
Optional[List[float]]
for bbox andOptional[List[Union[datetime, None]]]
for datetime so that you don't need to handle the conversion issues downstream?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.
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.
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.
datetime
is a little more complicated, because it also supports ranges like2020-01-01/2022-01-01
or2020-01-01/..
, so it must allowstr
input (or a clever pydantic type enforcing either datetime or something with a slash in it).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.
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...
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.
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 thedatetime
parameter, and treats a single datetime as an exact datetime. It's also more lenient on datetime formatting.For
pgstac
, I just usedpost_request_model
, which treats a single value as an end-date, and is stricter on enforcing RFC3339.So a few questions:
sqlalchemy
post_search
anditem_collection
routes to usepost_request_model
?None | tuple[datetime, datetime] | tuple[datetime, None] | tuple[None, datetime]
. This is all already validated inpost_request_model
(see the last link above). I think actually enforcing this typing would break the pgstac implementation, as it passes the rawdatetime/datetime
string to the DB'ssearch
function.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.
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.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
orstart_date/..
. I agree that==
is arguably less useful when searching than a datetime range but the distinction is there to let users do both.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.