Raise Auth0Error for bad status code #98
Conversation
Previously the text of the response was always returned if it could not be interpreted as JSON.
Codecov Report
@@ Coverage Diff @@
## master #98 +/- ##
==========================================
+ Coverage 92.5% 93.05% +0.55%
==========================================
Files 32 32
Lines 547 634 +87
==========================================
+ Hits 506 590 +84
- Misses 41 44 +3
Continue to review full report at Codecov.
|
|
Thanks, will do. |
There was a problem hiding this comment.
There's another open PR that fixes this very same thing but for authentication and includes parsing of "plain text error responses". I'd like to merge that one first and then we can think of management and copy that new base file here.
BTW a similar PR was proposed a few days before yours https://github.com/auth0/auth0-python/pull/97/files. I'll leave this in standby and see later which one to merge.
Thanks for your contribution 🎉
…rror Correctly throw an exception when handing a text response
auth0/v3/management/rest.py
Outdated
| raise Auth0Error(status_code=text['statusCode'], | ||
| error_code=text['errorCode'], | ||
| error_code=text.get('errorCode', ''), |
There was a problem hiding this comment.
Another PR suggests falling back to the property "error" when "errorCode" is not present in the response.
There was a problem hiding this comment.
I've looked at the latest change to /authentication/base.py and it mostly looks good because it both takes status_code into account and uses error instead of errorCode. The only issue I noticed is that it is not python 2 compatible. Using super().__init__(...) is not supported in python 2. To be supported in both python 2 and python 3 this should be super(Response, self).__init__(...)
There was a problem hiding this comment.
I will update my PR with this change in /authentication/base.py and update /management/rest.py to use the same logic.
|
@darkyen and @lbalmaceda I've updated tests to keep the coverage checks happy. Can you review these changes and let me know if they can be incorporated into the SDK? Thanks |
|
@darkyen Thanks for the approval. Any idea on the timeline for when this will be merged and available in a new version of the auth0-python package? |
|
I'll try to make a new release by Friday. |
|
@beck3905 I've just uploaded 3.2.0. It was my first time uploading a package to pypi so please, share feedback whether it works and/or is available or not. https://pypi.org/project/auth0-python/ |
|
Hi Luciano,
I just pip installed the latest auth0-python package v3.2.0 and looked at
the auth0/v3/management/rest.py file and it does not appear to have the
latest changes in it. I still see
```
def _process_response(self, response):
text = json.loads(response.text) if response.text else {}
if isinstance(text, dict) and 'errorCode' in text:
raise Auth0Error(status_code=text['statusCode'],
error_code=text['errorCode'],
message=text['message'])
return text
```
I'm not sure what happened with the publishing to pypi, but the latest
source in github looks correct.
Thanks
Brian
…On Fri, Apr 27, 2018 at 2:05 PM Luciano Balmaceda ***@***.***> wrote:
@beck3905 <https://github.com/beck3905> I've just uploaded 3.2.0. It was
my first time uploading a package to pypi so please, share feedback whether
it works and/or is available or not.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#98 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQaToBKw1yR_luSbOqZ4Isk7UKHjcfrWks5ts3obgaJpZM4Tal7y>
.
|
|
@lbalmaceda @darkyen - Any updates on this? I'm still not seeing this code when I pip install auth0-python 3.2.0. |
|
@beck3905 I just uploaded Also, I see releases before 3.2.0 had a "dumb binary" file uploaded which I can't seem to generate/upload. I've followed the steps at the distributing packages guide but even if I can generate the Does anyone know if this is the right procedure? # Generate distribution files
python setup.py sdist bdist
# Generate wheel files
python setup.py bdist_wheel --universal
# Upload files
twine upload dist/* |
|
@lbalmaceda I just pip installed the latest package and inspected the rest.py file in my site-packages directory. Looks like it has the changes in 3.2.2. Thanks. |
I copied the logic from authentication/base.py for _process_response into management/rest.py. This way any 400 or greater status codes will result in an Auth0Error.
The current functionality, which is based on 'error_code' being present in the response is not good enough because not all Auth0 API error response include 'error_code' and thus bad response get through when the expectation is a valid object. For example, if I call auth0.users.create() and get a 429 response, the API response does not have an error_code and my code expects to receive a valid user profile but instead receives a
{"error": "Too Many Requests", "status_code": 429}.This change will make consuming the auth0-python sdk more consistent as all error responses will result in an Auth0Error instead of some resulting in an Auth0Error and other returning an error object when an Auth0 object is expected.