oauth token caching #110
oauth token caching #110
Conversation
geokollias
left a comment
geokollias
left a comment
There was a problem hiding this comment.
LGTM Helmi! Have added some (mostly minor) comments/questions.
railib/rest.py
Outdated
| except Exception: | ||
| return None |
There was a problem hiding this comment.
I see we swallow these exceptions in these cache functions (the go sdk does sth similar as well). Is that on purpose?
There was a problem hiding this comment.
as far as I do understand, if the cache is not accessible for some reason we should not block the user and instead we request a new token
| else: | ||
| cache = {creds.client_id: creds.access_token} | ||
| with open(_cache_file(), 'w') as f: | ||
| f.write(json.dumps(cache, default=vars)) |
There was a problem hiding this comment.
what's default=vars needed for?
There was a problem hiding this comment.
this is one way to make python class json serializable
railib/rest.py
Outdated
| if cache: | ||
| cache[creds.client_id] = creds.access_token | ||
| else: | ||
| cache = {creds.client_id: creds.access_token} |
There was a problem hiding this comment.
maybe we could avoid this branch by returning {} from _read_cache's exception path
| def _read_token_cache(creds: ClientCredentials) -> AccessToken: | ||
| try: | ||
| cache = _read_cache() | ||
| return AccessToken(**cache[creds.client_id]) |
There was a problem hiding this comment.
I assume if the cache doesn't contain that client id an exception is thrown?
There was a problem hiding this comment.
yep that's true and we just by-pass it, if _read_token_cache function return None we will request a new token anyway
2 participants
This PR adds support to oauth token caching