Enable redirect following and avoid treating 302s as errors #685
Conversation
planet/http.py
Outdated
| @@ -241,7 +243,7 @@ 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) | |||
There was a problem hiding this comment.
Needed since httpx 0.20.
planet/http.py
Outdated
| except httpx.HTTPStatusError as e: | ||
| e.response.read() | ||
| cls._convert_and_raise(e) | ||
| if response.status_code >= 400: |
There was a problem hiding this comment.
Unless we do this we will trip on the fact that httpx.Response(302).raise_for_status() raises an exception in version 0.23, even when a redirect could be followed.
There was a problem hiding this comment.
Good news is that httpx.Response(302).is_error is False.
| @@ -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' | |||
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
@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.
| mock_redirect = httpx.Response(HTTPStatus.FOUND, | ||
| headers={ | ||
| 'Location': TEST_DOWNLOAD_ACTUAL_URL, | ||
| 'Content-Length': '0' |
There was a problem hiding this comment.
@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.
This works around httpx's preference to raise in the redirect case.
| except httpx.HTTPStatusError as e: | ||
| await e.response.aread() | ||
| cls._convert_and_raise(e) | ||
| if response.is_error: |
There was a problem hiding this comment.
See #685 (comment). This is probably the same thing as response.status_code >= 400 but syntactically a little neater, maybe?
Towards resolving #678.
Without these changes, an attempt to download an order with 0.23 produces
With these changes, I'm able to download an order.