Skip to content

Enable redirect following and avoid treating 302s as errors #685

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
merged 4 commits into from
Sep 7, 2022
Merged

Enable redirect following and avoid treating 302s as errors #685

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
7c22069
Enable redirect following and avoid treating 302s as errors
sgillies Sep 1, 2022
fa27daa
Remove debugging, reformat
sgillies Sep 6, 2022
a71c951
Change download test to fail if we don't follow redirects
sgillies Sep 7, 2022
eb79bd3
Use raise_for_status only if we have an error response
sgillies Sep 7, 2022
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
26 changes: 15 additions & 11 deletions planet/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,12 @@ def _convert_and_raise(cls, error):

@classmethod
def _raise_for_status(cls, response):
try:
response.raise_for_status()
except httpx.HTTPStatusError as e:
e.response.read()
cls._convert_and_raise(e)
if response.is_error:
try:
response.raise_for_status()
except httpx.HTTPStatusError as e:
e.response.read()
cls._convert_and_raise(e)

return

Expand Down Expand Up @@ -241,7 +242,9 @@ def __init__(self, auth: AuthType = None):

LOGGER.info(f'Session read timeout set to {READ_TIMEOUT}.')
timeout = httpx.Timeout(10.0, read=READ_TIMEOUT)
self._client = httpx.AsyncClient(auth=auth, timeout=timeout)
self._client = httpx.AsyncClient(auth=auth,
timeout=timeout,
follow_redirects=True)

self._client.headers.update({'User-Agent': self._get_user_agent()})
self._client.headers.update({'X-Planet-App': 'python-sdk'})
Expand All @@ -265,11 +268,12 @@ async def alog_response(*args, **kwargs):

@classmethod
async def _raise_for_status(cls, response):
try:
response.raise_for_status()
except httpx.HTTPStatusError as e:
await e.response.aread()
cls._convert_and_raise(e)
if response.is_error:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #685 (comment). This is probably the same thing as response.status_code >= 400 but syntactically a little neater, maybe?

try:
response.raise_for_status()
except httpx.HTTPStatusError as e:
await e.response.aread()
cls._convert_and_raise(e)

return

Expand Down
11 changes: 10 additions & 1 deletion tests/integration/test_orders_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
TEST_URL = 'http://www.MockNotRealURL.com/api/path'
TEST_BULK_CANCEL_URL = f'{TEST_URL}/bulk/orders/v2/cancel'
TEST_DOWNLOAD_URL = f'{TEST_URL}/download'
TEST_DOWNLOAD_ACTUAL_URL = f'{TEST_URL}/download_actual'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are probably better names and URLs for this. In the actual API, it's link.planet.com, but we shouldn't depend on anything but the location header.

TEST_ORDERS_URL = f'{TEST_URL}/orders/v2'
TEST_STATS_URL = f'{TEST_URL}/stats/orders/v2'

Expand Down Expand Up @@ -540,8 +541,16 @@ async def test_download_asset_md(tmpdir, session):
'Content-Type': 'application/json',
'Content-Disposition': 'attachment; filename="metadata.json"'
}

mock_redirect = httpx.Response(HTTPStatus.FOUND,
headers={
'Location': TEST_DOWNLOAD_ACTUAL_URL,
'Content-Length': '0'
Copy link
Contributor Author

@sgillies sgillies Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jreiberkyle there was an exception in the model module if the redirect response didn't have a content-length header. This feels like something we should fix; our API redirect response has an empty body and we should be able to traverse to the header location without checking the body or its length.

})
mock_resp = httpx.Response(HTTPStatus.OK, json=md_json, headers=md_headers)
respx.get(dl_url).return_value = mock_resp

respx.get(dl_url).return_value = mock_redirect
respx.get(TEST_DOWNLOAD_ACTUAL_URL).return_value = mock_resp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreiberkyle @mkshah605 @kevinlacaille @cholmes the orders API redirects when downloading. I'm simulating that here. Default behavior of httpx (no redirect following) will cause this test to fail, which is what the bug reporter was seeing.


cl = OrdersClient(session, base_url=TEST_URL)
filename = await cl.download_asset(dl_url, directory=str(tmpdir))
Expand Down