Skip to content

raise decode error instead of ContentDecodingError #19433

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

Merged
xiangyan99 merged 4 commits into from
Nov 10, 2021
Merged

raise decode error instead of ContentDecodingError #19433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7951899
raise decode error instead of ContentDecodingError
xiangyan99 Jun 24, 2021
936c5a2
Merge branch 'main' into core_stream_decode_error
xiangyan99 Jul 8, 2021
d354653
Merge branch 'main' into core_stream_decode_error
xiangyan99 Nov 9, 2021
84bc38d
Update _requests_basic.py
xiangyan99 Nov 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions sdk/core/azure-core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

### Breaking Changes

- Sync stream downloading now raises `azure.core.exceptions.DecodeError` rather than `requests.exceptions.ContentDecodingError`
Copy link
Member

@annatisch annatisch Jul 8, 2021

Choose a reason for hiding this comment

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

Are we okay with this breaking change because no one should have written code to explicitly handle this error?

Copy link
Member Author

@xiangyan99 xiangyan99 Jul 8, 2021

Choose a reason for hiding this comment

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

Yes. Checked with @johanste and he agreed this was an acceptable breaking change.


### Bugs Fixed

### Other Changes
Expand Down
13 changes: 8 additions & 5 deletions sdk/core/azure-core/azure/core/pipeline/transport/_requests_basic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import urllib3 # type: ignore
from urllib3.util.retry import Retry # type: ignore
from urllib3.exceptions import (
DecodeError, ReadTimeoutError, ProtocolError
DecodeError as CoreDecodeError, ReadTimeoutError, ProtocolError
)
import requests

Expand All @@ -39,6 +39,7 @@
ServiceResponseError,
IncompleteReadError,
HttpResponseError,
DecodeError
)
from . import HttpRequest # pylint: disable=unused-import

Expand Down Expand Up @@ -71,11 +72,11 @@ def _read_raw_stream(response, chunk_size=1):
for chunk in response.raw.stream(chunk_size, decode_content=False):
yield chunk
except ProtocolError as e:
raise requests.exceptions.ChunkedEncodingError(e)
except DecodeError as e:
raise requests.exceptions.ContentDecodingError(e)
raise ServiceResponseError(e, error=e)
except CoreDecodeError as e:
raise DecodeError(e, error=e)
Copy link
Member

@annatisch annatisch Jul 8, 2021

Choose a reason for hiding this comment

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

This scenario is being correctly handled in the other transports?

Copy link
Member Author

@xiangyan99 xiangyan99 Jul 8, 2021

Choose a reason for hiding this comment

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

Currently different transport raises different type of error. (That's the reason we need this change).

We want to uniform the errors so user can catch all by using except AzureError.

except ReadTimeoutError as e:
raise requests.exceptions.ConnectionError(e)
raise ServiceRequestError(e, error=e)
else:
# Standard file-like object.
while True:
Expand Down Expand Up @@ -174,6 +175,8 @@ def __next__(self):
raise StopIteration()
except requests.exceptions.StreamConsumedError:
raise
except requests.exceptions.ContentDecodingError as err:
raise DecodeError(err, error=err)
except requests.exceptions.ChunkedEncodingError as err:
msg = err.__str__()
if 'IncompleteRead' in msg:
Expand Down
3 changes: 1 addition & 2 deletions sdk/core/azure-core/tests/test_streaming.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ def test_compress_compressed_no_header(http_request):
@pytest.mark.parametrize("http_request", HTTP_REQUESTS)
def test_decompress_plain_header(http_request):
# expect error
import requests
account_name = "coretests"
account_url = "https://{}.blob.core.windows.net".format(account_name)
url = "https://{}.blob.core.windows.net/tests/test_with_header.txt".format(account_name)
Expand All @@ -109,7 +108,7 @@ def test_decompress_plain_header(http_request):
try:
content = b"".join(list(data))
assert False
except (requests.exceptions.ContentDecodingError, DecodeError):
except DecodeError:
pass

@pytest.mark.parametrize("http_request", HTTP_REQUESTS)
Expand Down