-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not convert to bytes the refresh_token
#3273
Conversation
When the body is encoded, a mix of bytes and unicode happens and it fails at this line https://github.com/requests/requests-oauthlib/blob/c472e6bf0468f1bd52b70a64e9aac92bee41c491/requests_oauthlib/oauth2_session.py#L285 (Pdb) body u'grant_type=refresh_token&allow_redirects=True&refresh_token=b%27z9BSRKqf2QD4pcDhqj%27&client_id=CLIENT_ID&client_secret=CLIENT_SECRET' (note those weird `%27` in the string)
Can we add some logging around this to catch failures better in the future? |
I think we should connect the logs from this module, since it's already logging the |
I mean something that would catch the exception/error that would presumably happen on invalid refresh token? The full body isn't gonna help us catch errors, just spam logs :) |
Ideally, we should still solve the issue of a failure here not updating the GitHub repos that were successfully returned. |
I think I will need more directions here. I mean, I can catch the exception as you say but then? I updated this PR with the solution that I imagine you want :) The
I need to take a look in deep at this (each sync is ran in a separate celery task or the same?). I suppose that it's the same, otherwise it shouldn't break the other service. If this is the case, I think I solved it by returning |
Correct, all providers are synced in the same task. I am confused why catching these exceptions was even needed now though. Most of the calls in the oauth services are themselves wrapped in try catch, and ultimately raise an exception to the user. That is, we want to raise |
`paginate` method is shared between both services and each service implement `get_next_url_to_paginate` and `get_paginated_results`.
I just refactored the code to handle the exception in both services. For that I created a common If something goes wrong, it returns @agjohnson can you take a look again at this PR? |
This looks great 👍 The abstraction for |
* 'master' of github.com:rtfd/readthedocs.org: Add GLOBAL_PIP_CACHE setting (readthedocs#3299) Remove invalid attribute `exception` (readthedocs#3298) Update build from database at __exit__ (readthedocs#3292) Extra way to check OOM of a command ran inside Docker (readthedocs#3294) Add supervisord contrib example (readthedocs#3262) Allow promos to be placed in the docs footer (readthedocs#3267) Do not convert to bytes the `refresh_token` (readthedocs#3273)
When the body is encoded, a mix of bytes and unicode happens and it
fails at this line
https://github.com/requests/requests-oauthlib/blob/c472e6bf0468f1bd52b70a64e9aac92bee41c491/requests_oauthlib/oauth2_session.py#L285
(Pdb) body
u'grant_type=refresh_token&allow_redirects=True&refresh_token=b%27z9BSRKqf2QD4pcDhqj%27&client_id=CLIENT_ID&client_secret=CLIENT_SECRET'
(note those weird
%27
in the string)Closes #2993
Closes #2992