Subscriptions API client #607
Conversation
Mocking the service in the tests is the hard part. There are several different approaches we can take and none of them are obviously the best.
Tests use respx to mock server routing. Subscriptions results is not covered in this batch of work.
| 'name': 'test', | ||
| 'delivery': 'yes, please', | ||
| 'source': 'test' | ||
| })) |
There was a problem hiding this comment.
This is most of the work: creating the mock router configurations that allow us to test features.
And subclass Session with custom x-planet-app header
|
|
||
| def __init__(self, auth: AuthType = None): | ||
| super().__init__(auth) | ||
| self._client.headers.update({'X-Planet-App': 'python-cli'}) |
There was a problem hiding this comment.
We might consider making this header a class attribute @jreiberkyle ? Then we wouldn't need to override the constructor like I'm doing here. Not a big deal, though.
| self._pages = None | ||
| self._items = [] | ||
| self.i = 0 | ||
| self.limit = limit |
There was a problem hiding this comment.
@jreiberkyle I subclassed Paged while developing this feature to reduce layers of abstraction and make things easier to debug. I do not mind at all if this extra class is deleted on the way to enabling automatic retries (if we need that).
I do think the Paged API would be cleaner if the constructor took a client (as I did) or session object as an argument instead of a "request doer". So maybe that could be something that comes out of this PR into other modules?
| limit=limit): | ||
| async for sub in _SubscriptionsPager(self.session._client, | ||
| req, | ||
| limit=limit): |
There was a problem hiding this comment.
@jreiberkyle I've made the subscriptions pager an implementation detail here. I can't think of a reason why Paged needs to be part of this library's public API. Other than the async iterator magic methods its methods are all private. We could make the class itself private and reduce our API surface area and documentation needs. What do you think?
| raise APIError("Subscription failure.") from server_error | ||
| else: | ||
| return sub | ||
| _ = await self.session._client.send(req) |
There was a problem hiding this comment.
According to the Subscription API specs and docs, canceling doesn't return anything. I'm going to ask about this.
| except ClientError: # pragma: no cover | ||
| raise | ||
| else: | ||
| sub = resp.json() |
There was a problem hiding this comment.
@jreiberkyle I did not use Request and Response from planet.models when I developed this feature because I didn't see the value in doing so for the subscriptions MVP. We're not using streaming requests or responses in subscriptions. This implementation only uses httpx requests and responses. It's simple and easy to reason about, I believe. Definitely helped me finish the MVP quickly. Less abstraction at the start can be good sometimes.
There was a problem hiding this comment.
The retry logic in http.Session is something I would want to use for sure, but I would really like to use it differently. As a standalone utility. Think of it as an inline decorator.
url = URL(f'https://api.planet.com/subscriptions/v1/{subscription_id}')
req = self.session._client.build_request('PUT', url, json=request)
resp = await retry(retry_config, self.session._client.send, req)
return resp.json()
This way, retrying wouldn't have to be one size fits all. We could use as much or as little of it as we needed, on a per-request basis.
Resolves #512
Resolves #513
Resolves #514
Resolves #339