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鈥檒l 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
Merged

Conversation

@HemangChothani
Copy link
Contributor

@HemangChothani HemangChothani commented Aug 7, 2020

Fixes #224 馃

@HemangChothani HemangChothani requested review from tseaver and frankyn Aug 7, 2020
@google-cla google-cla bot added the cla: yes label Aug 7, 2020
@tseaver
Copy link
Contributor

@tseaver 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

@tseaver tseaver Aug 10, 2020

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

Copy link
Member

@frankyn frankyn Aug 11, 2020

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

@HemangChothani HemangChothani Aug 13, 2020

@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

@HemangChothani HemangChothani Aug 19, 2020

@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

@frankyn frankyn Aug 24, 2020

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

@andrewsg andrewsg Aug 24, 2020

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

@frankyn frankyn Aug 25, 2020

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

@frankyn frankyn Aug 11, 2020

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.

@frankyn frankyn merged commit a8de586 into googleapis:master Aug 26, 2020
4 checks passed
cojenco added a commit to cojenco/python-storage that referenced this issue 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 added a commit to cojenco/python-storage that referenced this issue 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants