Fix request creation when headers are the default #198
Conversation
| @@ -42,16 +42,15 @@ def __init__(self, domain, telemetry=True): | |||
|
|
|||
| def post(self, url, data=None, headers=None): | |||
| request_headers = self.base_headers.copy() | |||
| request_headers.update(headers) | |||
There was a problem hiding this comment.
this was the bug. When headers was None this call would throw.
| response = requests.get(url=url, params=params, headers=request_headers) | ||
| return response.text | ||
| return self._process_response(response) |
There was a problem hiding this comment.
Some response formats where not taken into account. This was the only method not calling _process_response. Now it handles text responses (what was there before), JSON and empty body responses.
| @@ -32,8 +32,7 @@ def login(self, client_id, username, password, connection, id_token=None, | |||
| 'device': device, | |||
| 'grant_type': grant_type, | |||
| 'scope': scope, | |||
| }, | |||
| headers={'Content-Type': 'application/json'} | |||
There was a problem hiding this comment.
All these args were removed because they match the default behavior https://github.com/auth0/auth0-python/blob/master/auth0/v3/authentication/base.py#L24
| @@ -46,7 +46,7 @@ def get(self, url, params=None): | |||
| def post(self, url, data=None): | |||
| headers = self.base_headers.copy() | |||
|
|
|||
| response = requests.post(url, data=json.dumps(data or {}), headers=headers) | |||
| response = requests.post(url, json=data, headers=headers) | |||
There was a problem hiding this comment.
No longer need to explicitly convert python dictionaries to "JSON text"
| mock_post.return_value.text = '{"x": "y"}' | ||
|
|
||
| # Only required params are passed | ||
| data = ab.post('the-url') |
There was a problem hiding this comment.
no headers value is passed, so default of None will kick in. This asserts the bug is no longer present
| @@ -215,6 +215,40 @@ def test_file_post_content_type_is_none(self, mock_post): | |||
|
|
|||
| mock_post.assert_called_once_with('the-url', data=data, files=files, headers=headers) | |||
|
|
|||
|
|
|||
| @mock.patch('requests.put') | |||
| def test_put(self, mock_put): | |||
There was a problem hiding this comment.
missing tests for the put HTTP method
|
@elkdanger I can't tell. You never told me what the actual issue was! But we can discuss it. This was affecting only the authentication client from this SDK |
Changes
There was an error affecting only the
authenticationAPI client and methods exposed on this SDK. When the call was not specifying custom headers (headers=None) a call to dictionary#update would fail.This PR fixes that issue, as well as a response converting issue.
Testing
I manually run this directly calling the requests library. Also run the AuthenticationBase#post/get calls.
Added missing tests for the "default" scenario on those 2 methods.
Added missing tests for the
management#putmethod.This change adds unit test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist