New compliance hook and error handling #317
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
During token fetch and token refresh we go through a similar flow to actually perform the request. Refactor existing code to use the same function to perform the request. This should have no change to the existing functionality.
I'm working with an oauth2 server we know is broken, however it works for other systems and the company has no interest in fixing it. It requires that I submit auth in JSON format rather than form encoded. We shouldn't support this specific problem, but if we have a compliance hook that allows me to change the sending format of the payload as required I can fix my problem and potentially others in future. Related to: requests#244 (but not the reason for the patch)
requests-mock is already a dependency that is being used in the compliance tests. It simplifies the process of testing exactly what was sent to the server and mocking out return values. Use requests-mock for the rest of the tests rather than a series of custom maintained mocks.
If the token request fails with an error such as Unauthenticated or anything else the existing code doesn't check for this code and will try and retrieve the token anyway. This gives a somewhat difficult to debug key error when the response doesn't contain the token data. This is wrong, we should always be checking the response code before trusting the response data. This is a slight change in behaviour, we now return this exception instead of a KeyError, however the other exception was difficult to catch. This reuses the failure exception from OAuth1 and makes that public. Closes: requests#302
|
Sorry for how these are combined, my biggest rant about github is not being able to do them as dependencies, but the code is intertwined so I can't completely separate them. I recommend thinking of it as the 3 PRs itemized in the PR body with the first 2 part of the compliance hook. If parts of this are an easier merge than other i'm more than willing to split it up in whatever way is most acceptable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow us to modify the request that is sent when a new token is requested. This
helps in situations where the standard format is not accepted.
requests-mock is already a dependency for some tests but extend it to the more
core tests.
Throw an error when you get a non 2XX status code because at that point we are
confident there's not going to be a token in the body.