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 2 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.


### Key Bugs Fixed

### Fixed
Expand Down
15 changes: 9 additions & 6 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,14 +29,15 @@
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

from azure.core.configuration import ConnectionConfiguration
from azure.core.exceptions import (
ServiceRequestError,
ServiceResponseError
ServiceResponseError,
DecodeError
)
from . import HttpRequest # pylint: disable=unused-import

Expand All @@ -59,11 +60,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 @@ -161,6 +162,8 @@ def __next__(self):
raise StopIteration()
except requests.exceptions.StreamConsumedError:
raise
except requests.exceptions.ContentDecodingError as err:
raise DecodeError(err, error=err)
except Exception as err:
_LOGGER.warning("Unable to stream download: %s", err)
internal_response.close()
Expand Down
4 changes: 2 additions & 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 @@ -25,6 +25,7 @@
# --------------------------------------------------------------------------
import os
from azure.core import PipelineClient
from azure.core.exceptions import DecodeError

def test_decompress_plain_no_header():
# expect plain text
Expand Down Expand Up @@ -90,7 +91,6 @@ def test_compress_compressed_no_header():

def test_decompress_plain_header():
# 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 @@ -102,7 +102,7 @@ def test_decompress_plain_header():
try:
content = b"".join(list(data))
assert False
except requests.exceptions.ContentDecodingError:
except DecodeError:
pass

def test_compress_plain_header():
Expand Down