Currently when using the admin api all stores (tokens) share the same global httpx.AsyncClient instance:
|
client: ClassVar[AsyncClient] = AsyncClient() |
While I understand the motivation, I think this can lead to confusing behaviour and could potentially mean spylib users have to eject from spylib entirely in certain situations.
I think each token containing its own AsyncClient instance would be very helpful. Maybe I want to do my own error handling, or implement different rate limit handling (round-robin on multiple tokens for instance). That way we could say here are the nice features we provide, but if you need anything custom just grab mytoken.client which has base_url, headers, etc. set up with sane defaults.
This then also leads into a discussion about who is responsible for calling aclose on the client...but that may be a discussion for another time 😜
An (contrived) example test where things don't work as expected:
|
async def test_event_based_credential_leak(respx_mock: MockRouter): |
|
from spylib.admin_api import OfflineTokenABC |
|
from spylib.utils.rest import GET |
|
|
|
class OfflineToken(OfflineTokenABC): |
|
async def save(self): |
|
raise NotImplementedError() |
|
|
|
@classmethod |
|
async def load(cls, store_name: str): |
|
raise NotImplementedError() |
|
|
|
store_one = 'store_one' |
|
store_two = 'store_two' |
|
|
|
token_one = OfflineToken( |
|
store_name=store_one, access_token=f'secret_token_for_{store_one}', scope=[] |
|
) |
|
token_two = OfflineToken( |
|
store_name=store_two, access_token=f'secret_token_for_{store_two}', scope=[] |
|
) |
|
|
|
token_one_headers = [] |
|
|
|
async def capture_request(request): |
|
token_one_headers.append(request.headers['X-Shopify-Access-Token']) |
|
|
|
token_one.client.event_hooks['request'] = [capture_request] |
|
|
|
respx_mock.get().mock( |
|
return_value=Response(200, json={}, headers={'X-Shopify-Shop-Api-Call-Limit': '10/20'}) |
|
) |
|
|
|
await token_two.execute_rest( |
|
request=GET, |
|
endpoint='/test', |
|
) |
|
|
|
assert token_one_headers == [] |
Currently when using the admin api all stores (tokens) share the same global
httpx.AsyncClientinstance:spylib/spylib/admin_api.py
Line 81 in 63b4cf4
While I understand the motivation, I think this can lead to confusing behaviour and could potentially mean spylib users have to eject from spylib entirely in certain situations.
I think each token containing its own
AsyncClientinstance would be very helpful. Maybe I want to do my own error handling, or implement different rate limit handling (round-robin on multiple tokens for instance). That way we could say here are the nice features we provide, but if you need anything custom just grabmytoken.clientwhich hasbase_url,headers, etc. set up with sane defaults.This then also leads into a discussion about who is responsible for calling
acloseon the client...but that may be a discussion for another time 😜An (contrived) example test where things don't work as expected:
spylib/tests/admin_api/test_credential_leak.py
Lines 7 to 45 in 2dd3fd8