Modify token auto refresh handling. #265
Conversation
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
Cool, this sounds great so far! I have a few notes but at a high level this seems like it's in really good shape. Thanks for this!
| old_showwarning(message, category, filename, lineno, file=file) | ||
|
|
||
| # Install our new handler. | ||
| warnings.showwarning = logwarning |
There was a problem hiding this comment.
I am very nervous about monkeypatching the warnings module. I'd rather not do it.
There was a problem hiding this comment.
There was a problem hiding this comment.
Sure, but it is still unnecessary for us to do this. We can just use a regular function, rather than change global state.
requests_oauthlib/oauth2_session.py
Outdated
| log.debug('Adding auto refresh key word arguments %s.', | ||
| self.auto_refresh_kwargs) | ||
| kwargs.update(self.auto_refresh_kwargs) | ||
| log.debug('Kwargs: %s', kwargs) |
There was a problem hiding this comment.
This log message isn't really useful anymore: it might be better to remove it rather than leave it like this.
There was a problem hiding this comment.
Yeah, that is a leftover, will remove it.
requests_oauthlib/oauth2_session.py
Outdated
| """Intercept all requests and add the OAuth 2 token if present.""" | ||
| if not is_secure_transport(url): | ||
| raise InsecureTransportError() | ||
|
|
There was a problem hiding this comment.
FYI... it was not "random" I was trying to make it more readable... Nevertheless the whitespace is removed.
There was a problem hiding this comment.
Legibility changes are fine, but they're better in seaparate PRs so that I can keep track of what is going on
requests_oauthlib/oauth2_session.py
Outdated
| # Start with kwargs explicitly requested for refresh. | ||
| refresh_kwargs = self.auto_refresh_kwargs.copy() | ||
|
|
||
| if 'auth' in kwargs: |
There was a problem hiding this comment.
We should defend against auth being present but None.
| warnings.warn('client_id provided, but ' | ||
| 'client_secret missing') | ||
| client_secret = '' | ||
| refresh_kwargs['auth'] = \ |
There was a problem hiding this comment.
A style note: we'd rather use implicit newlines inside parentheses than explicit ones.
There was a problem hiding this comment.
The newline does not show up in the resulting string, it is strictly for formatting, the two strings end up concatenated into one (single line) string.
There was a problem hiding this comment.
Example:
>>> print('this is a multi-'
... 'line string in the code but'
... ' a single line when printed.')
this is a multi-line string in the code but a single line when printed.There was a problem hiding this comment.
I know. I'm saying don't use \ (the explicit line continuation character), use parentheses instead.
| auth = requests.auth.HTTPBasicAuth(client_id, client_secret) | ||
| if not client_secret: | ||
| warnings.warn('client_id provided, but ' | ||
| 'client_secret missing') |
There was a problem hiding this comment.
Do we need this warning? I'm not certain how likely this is to be an error.
There was a problem hiding this comment.
I added it because the code replaces the missing argument with an empty string and continues with authentication. I thought that seemed like an omission on the callers part, something they might need warned about. This could be an incorrect assumption.
There was a problem hiding this comment.
Rather than do that, let's just require both the client ID and secret.
|
Even after I make the requested changes, I am still testing, so please don't merge this. I will likely add new unit tests. |
|
This change worked perfectly for me. |
| # compatibility | ||
|
|
||
| # Start with kwargs explicitly requested for refresh. | ||
| refresh_kwargs = self.auto_refresh_kwargs.copy() |
There was a problem hiding this comment.
I wonder, isn't it going to change the semantic of auto_refresh_kwargs parameter? Previously it was intended to contain only key-value pairs to include into the body of the request, but now it also holds arguments of refresh_token() method such as auth, timeout, proxies, etc. For instance, if somebody wants to send auth parameter in the request body together with refresh_token and grant_type, this change will break their code.
@btimby, are you still working on this? If you're still interested in getting this change merged, you should probably rebase it on the latest master, and then we can try again to get it reviewed and merged. |
6 participants
I think these changes make things more explicit. I chose to preserve old functionality with warnings rather than outright remove them.
All of the old functionality can be achieved through the use of
auto_refresh_kwargs, the preferred explicit method.I have not yet tested this in my codebase, but the old and new tests all pass. I added some missing tests for old functionality as well as tests for the new stuff.