Skip to content
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

fixes handling of error responses #97

Closed
wants to merge 1 commit into from

Conversation

andrewpwade
Copy link

API now appears to return key error instead of errorCode, e.g.:

{'statusCode': 401, 'error': 'Unauthorized', 'message': 'Invalid token'}

maintained exception attribute error_code for backwards compatibility.

Reported in #86

API now appears to return key `error` instead of `errorCode`, e.g.:

`{'statusCode': 401, 'error': 'Unauthorized', 'message': 'Invalid token'}`

maintained exception attribute `error_code` for backwards compatibility.

Reported in auth0#86
@codecov-io
Copy link

codecov-io commented Apr 17, 2018

Codecov Report

Merging #97 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #97   +/-   ##
======================================
  Coverage    92.5%   92.5%           
======================================
  Files          32      32           
  Lines         547     547           
======================================
  Hits          506     506           
  Misses         41      41
Impacted Files Coverage Δ
auth0/v3/management/rest.py 74% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c616062...a57e51d. Read the comment docs.

Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase and review how the /authentication/rest.py file now handles this error and status codes. You'll probably want to copy that behavior instead.
Also, maybe it's better to keep both the errorCode and error cases instead of replacing all the old errorCode tests. I don't know for sure if an endpoint will decide to use one or the other, would need to check with the docs or do manual testing.

@andrewpwade
Copy link
Author

andrewpwade commented Apr 22, 2018

I looked at the documentation (at https://auth0.com/docs/api/management/v2 ) and did not see any docs for response errors.

It would be useful if an auth0 employee could provide inside knowledge, and perhaps fix the oversight in the documentation (if it is indeed not documented and I haven't just failed to find it) Otherwise, contributors are just making best guesses and nothing can be declared actually correct as per the documentation.

I take your note about backwards compatibility. However, we're not even completely sure what's the current contract is for error responses.

About the tests: again, if we know what all the possible responses are, they can be in the tests.

@lbalmaceda
Copy link
Contributor

I'm basing my suggestions on my own experience with the API. The docs don't seem to provide detailed error responses for each scenario but they do share a common response structure using code/description or error_code/error_description properties. Those are the changes introduced in the PR (92) and that's why this management rest logic should be similar. I understand your frustration about the lack of error documentation but unfortunately the API explorer docs are part of another team, which you can reach on https://support.auth0.com and give more visibility to this issue. In the meantime I'll close this one, merge #98 and move forward with the release procedure. Thanks for your contribution.

@lbalmaceda lbalmaceda closed this Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants