Support OAuth endpoints which don't return expires_in
Support OAuth endpoints which don't return expires_in field
Fixes #297 (closed)
Previously when using the OAuthAuthenticator
with an OAuth endpoint which didn't include the expires_in
field in the response you'd see a KeyError
, e.g.
File "/.../site-packages/singer_sdk/authenticators.py", line 432, in update_access_token
self.expires_in = token_json["expires_in"]
KeyError: 'expires_in'
The expires_in
field is RECOMMENDED but not REQUIRED1 - so this is a use-case we want to handle. To solve the issue we add a default_expiration argument to OAuthAuthenticator
, which will be used as a fallback if the OAuth endpoint does not include an expires_in
with its response. This change also comes with a slightly more subtle side-effect, now if no default_expiration
is set, and the OAuth response has no expires_in
then the authenticator will assume the token never expires (see the second condition in is_token_valid()
).
Questions
Firstly, I haven't written much Python in my life - I'm very open to feedback on code style, testing approach, naming convention, etc., - but here are the thing I think I'd like advice on most:
- I'm not too familiar with the python testing ecosystem, does introducing requests-mock here feel useful or is there something already used in this project which we could use to test the
OAauthAuthenticator
behaviour I'm interested in here? - The test setup feels clunky, any advice appreciated on making improvements here
- Does testing the authenticator behaviour at this level make sense? I notice we don't seem to have many direct tests for authenticator behaviour in general
- Does treating no
expires_in
and nodefault_expiration
as "token never expires" seem ok? Or would we prefer raise some kind of error?