-
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
Return empty item collection instead of error when searching #233
Conversation
…o collection exists
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 found an incredibly minor issue and one bit that I had some questions about, but this PR looks good to me. Nice tests in particular.
@@ -161,13 +159,16 @@ async def item_collection( | |||
Returns: | |||
An ItemCollection. | |||
""" | |||
# If collection does not exist, NotFoundError wil be raised | |||
await self.get_collection(id, **kwargs) |
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'm curious what convinced you to safeguard against non-existent collections here. Maybe you saw some performance hits in that case which can be avoided?
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.
This addition is to maintain the existing logic that a request to this endpoint would result in an error if a collection doesn't exist. Previously it was the item_collection = await self._search_base(req, **kwargs)
that would result in a 404 since no items would be found, but since that now returns an empty collection and 200 if no items are found, I wanted to make sure an error was raised.
I was most unsure about this change, especially since The Planetary Computer STAC actually doesn't raise a 404 in this instance when a collection doesn't exist. I think logically it should though. What do you think?
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.
Good catch - the PC should def return a 404 instead of an empty collection in this case, so this change LGTM
# If collection does not exist, NotFoundError wil be raised | ||
await self.get_collection(collection_id, **kwargs) |
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.
Same question as above regarding the benefits of this safeguard
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.
Same idea as above - checking if a collection exists because if not, I think a 404 should be returned
This PR addresses issue #172.
/search
that returns no items now returns an empty list of features rather than a 404 andNotFoundError
;/collections/{collectionId}/items
with an invalid collection ID still returns a 404 rather than a 200 with an empty features list/collections/{collectionId}/items/{itemId}
with an invalid collection ID returns a 404Testing:
cc: @moradology, @lossyrob