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

feat: error message return from api #235

Merged
merged 10 commits into from Aug 26, 2020

Conversation

HemangChothani
Copy link
Contributor

Fixes #224 🦕

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 7, 2020
@tseaver
Copy link
Contributor

tseaver commented Aug 10, 2020

Nit: please drop the (apiname) part of the prefix for your commit summaries. That convention made sense in the mono-repository, but post-repo-split, it is redundant and cluttery.

@tseaver tseaver changed the title feat(storage): error message return from api feat: error message return from api Aug 10, 2020
if response.text:
error_message = response.text + ": " + str(error)
else:
error_message = "unknown error: " + str(error)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer not to add the prefix here. @frankyn WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify the difference between response.text and error (add comments I suspect others will ask this question when reviewing the code in the future).

I also think repsonse.text could be added to the line below when constructing the message value instead of this conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frankyn I have added condition here to match the method which is responsible for other http responses and errors declared in pytho-api-core.exceptions.from_http_response you can find here https://github.com/googleapis/python-api-core/blob/622931721ce34839d630aa1e974c7d8f47b5d25e/google/api_core/exceptions.py#L390-L411

response.text: gives the actual reason of error
str(error): gives ('Request failed with status code', 404(Error Code), 'Expected one of', <HTTPStatus.OK: 200>, <HTTPStatus.PARTIAL_CONTENT: 206>)

I think here we can remove str(error) as it's not meaning full for the users, also not included in other http methods from python-api-core as mention above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frankyn I think it's necessary to add actual reason in error message, Ex: in the #249 issue user share a stack trace of error but it's hard to debug as actual reason isn't there in message.

Copy link
Member

Choose a reason for hiding this comment

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

I think I need to pull in @andrewsg on this one when he has a moment.

Andrew do you have guidance on raising the actual error that occurred in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, the problem here is the same as I've observed elsewhere where the API responds with human-readable text which is not converted into a specific error message by our client library. Prepending this human-readable text does make sense though I'm neutral on whether we should include the "unknown error" prepending in the else case.

We should make sure that the traceback still includes the original function call that caused the error.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @andrewsg.

I don't see the value of unknown error: and could be confusing otherwise. Please only preppend response.text when it's available. Remove unknown error:.

if response.text:
error_message = response.text + ": " + str(error)
else:
error_message = "unknown error: " + str(error)
Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify the difference between response.text and error (add comments I suspect others will ask this question when reviewing the code in the future).

I also think repsonse.text could be added to the line below when constructing the message value instead of this conditional.

@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label Aug 21, 2020
@frankyn frankyn merged commit a8de586 into googleapis:master Aug 26, 2020
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* feat(storage): error message retyrn from api

* feat: add comment for clarification

* fix: remove unknown error

Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* feat(storage): error message retyrn from api

* feat: add comment for clarification

* fix: remove unknown error

Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/python-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Except with error messages returned from the API
4 participants