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.

Sign up for GitHub

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

Jump to bottom

feat: error message return from api #235

Merged
merged 10 commits into from Aug 26, 2020
Merged

feat: error message return from api #235

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d18c3b1
feat(storage): error message retyrn from api
HemangChothani Aug 7, 2020
1820f55
Merge branch 'master' into storage_issue_224
tseaver Aug 10, 2020
970ece9
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani Aug 13, 2020
ae8de9d
Merge branch 'storage_issue_224' of https://github.com/q-logic/python…
HemangChothani Aug 13, 2020
57a1358
feat: add comment for clarification
HemangChothani Aug 14, 2020
dc9f4ff
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani Aug 14, 2020
aa332f6
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani Aug 19, 2020
07c2478
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani Aug 25, 2020
39f00df
Merge branch 'master' into storage_issue_224
frankyn Aug 25, 2020
562abfd
fix: remove unknown error
HemangChothani Aug 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion google/cloud/storage/blob.py
View file
Open in desktop
Expand Up @@ -3253,7 +3253,10 @@ def _raise_from_invalid_response(error):
to the failed status code
"""
response = error.response
error_message = str(error)
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:.


message = u"{method} {url}: {error}".format(
method=response.request.method, url=response.request.url, error=error_message
Expand Down
14 changes: 10 additions & 4 deletions tests/unit/test_blob.py
View file
Open in desktop
Expand Up @@ -4254,14 +4254,15 @@ def _call_fut(error):

return _raise_from_invalid_response(error)

def _helper(self, message, code=http_client.BAD_REQUEST, args=()):
def _helper(self, message, code=http_client.BAD_REQUEST, reason=None, args=()):
import requests

from google.resumable_media import InvalidResponse
from google.api_core import exceptions

response = requests.Response()
response.request = requests.Request("GET", "http://example.com").prepare()
response._content = reason
response.status_code = code
error = InvalidResponse(response, message, *args)

Expand All @@ -4273,15 +4274,20 @@ def _helper(self, message, code=http_client.BAD_REQUEST, args=()):
def test_default(self):
message = "Failure"
exc_info = self._helper(message)
expected = "GET http://example.com/: {}".format(message)
expected = "GET http://example.com/: unknown error: {}".format(message)
self.assertEqual(exc_info.exception.message, expected)
self.assertEqual(exc_info.exception.errors, [])

def test_w_206_and_args(self):
message = "Failure"
reason = b"Not available"
args = ("one", "two")
exc_info = self._helper(message, code=http_client.PARTIAL_CONTENT, args=args)
expected = "GET http://example.com/: {}".format((message,) + args)
exc_info = self._helper(
message, code=http_client.PARTIAL_CONTENT, reason=reason, args=args
)
expected = "GET http://example.com/: {}: {}".format(
reason.decode("utf-8"), (message,) + args
)
self.assertEqual(exc_info.exception.message, expected)
self.assertEqual(exc_info.exception.errors, [])

Expand Down