-
Notifications
You must be signed in to change notification settings - Fork 53
Fix recursive search in Client.get_items #799
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
6456d5a
92ba2c1
00aff72
9f9c5f8
a7fce5b
7c9e855
967eb79
a0ebc89
8a268d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -443,27 +443,34 @@ def get_collections(self) -> Iterator[Collection]: | |
call_modifier(self.modifier, collection) | ||
yield collection | ||
|
||
def get_items( | ||
self, *ids: str, recursive: bool | None = None | ||
) -> Iterator["Item_Type"]: | ||
def get_items(self, *ids: str, recursive: bool = True) -> Iterator["Item_Type"]: | ||
"""Return all items of this catalog. | ||
|
||
Args: | ||
ids: Zero or more item ids to find. | ||
recursive: unused in pystac-client, but needed for falling back to pystac | ||
recursive : If True, search this catalog and all children for the | ||
item; otherwise, only search the items of this catalog. Defaults | ||
to True. | ||
|
||
Return: | ||
Iterator[Item]: Iterator of items whose parent is this | ||
catalog. | ||
""" | ||
if self.conforms_to(ConformanceClasses.ITEM_SEARCH): | ||
search = self.search(ids=ids) | ||
if recursive: | ||
search = self.search(ids=ids) | ||
try: | ||
yield from search.items() | ||
return | ||
except APIError: | ||
child_catalogs = [catalog for catalog, _, _ in self.walk()] | ||
search = self.search(ids=ids, collections=[self, *child_catalogs]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like it would be pretty easy to do accidentally. I think I'd prefer to just let the error raise and make it a little harder to get every single item in planetary computer for instance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern is that without something like this, many functions that call
Note that the spec doesn't say one way or another that these arguments must be optional so I'm guessing that planetary computer's API is still spec compliant technically. However, the examples show that a search without collections should be supported so I don't really know one way or the other how to interpret that: Otherwise the only way to make this work for APIs like planetary computer is to override the Client class like: import pystac_client
class Client(pystac_client.Client):
def search(self, *args, **kwargs):
if kwargs["collections"] is None:
kwargs["collections"] = [self.id *[catalog.id for catalog, _, _ in self.walk()]]
return super().search(*args, **kwargs)
pystac_client.client.Client = Client # so that sub-catalogs also use the updated search method If that's the approach we want to go with that's fine, but maybe we should document this workaround in case users want to interact with planetary computer. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for taking the time to write that all up! I think as long as a clear error surfaces it is fine to have those methods fail on Planetary Computer. Requiring collections is not technically compliant with the spec, so I think it is better to not bake in special handling for this scenario especially since it is likely to result in a surprising user experience (setting collections to include every collection might be very very slow). |
||
else: | ||
search = self.search(ids=ids, collections=[self.id]) | ||
yield from search.items() | ||
else: | ||
self._warn_about_fallback("ITEM_SEARCH") | ||
for item in super().get_items( | ||
*ids, recursive=recursive is None or recursive | ||
): | ||
for item in super().get_items(*ids, recursive=recursive): | ||
mishaschwartz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
call_modifier(self.modifier, item) | ||
yield item | ||
|
||
|
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Uh oh!
There was an error while loading. Please reload this page.