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.

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

Replace requests with httpx #2041

Merged
merged 5 commits into from
Oct 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions changes/2039.misc.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Briefcase now uses `httpx <https://www.python-httpx.org/>`_ internally instead of ``requests``.
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ dependencies = [
"platformdirs >= 2.6, < 5.0",
"psutil >= 5.9, < 7.0",
"python-dateutil >= 2.9.0.post0", # transitive dependency (beeware/briefcase#1428)
"requests >= 2.28, < 3.0",
"httpx >= 0.20, < 1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really wasn't sure what version range to select here. I chose 0.20 by searching httpx's changelog and finding the last time they made any noted change to streaming. Other notable API usages are response.url as httpx.URL, which at least pre-dates 0.20 based on changelog notes that mention it receiving bug fixes in 0.18.0, as does the introduction of retries to httpx.HTTPTransport (0.15.0).

Aside from that, 0.20.0 is kind of arbitrary. It might be possible to go back even further like to 0.15.0, but 0.20.0 is 3 years old at this point, and I don't really know how to validate that any older version is actually supported (to be fair, I haven't done anything to validate that 0.20.0 is either, other than check feature availability).

Copy link
Member

Choose a reason for hiding this comment

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

0.20.0 seems like a completely reasonable arbitrary version to me :-)

"rich >= 12.6, < 14.0",
"tomli >= 2.0, < 3.0; python_version <= '3.10'",
"tomli_w >= 1.0, < 2.0",
Expand Down
7 changes: 5 additions & 2 deletions src/briefcase/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,12 @@ def __str__(self):


class NetworkFailure(BriefcaseCommandError):
def __init__(self, action):
DEFAULT_HINT = "is your computer offline?"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default hint is meant to prevent needing to modify other code paths that already relied on NetworkFailure. Doing the change this way prevents those areas from needing to change or duplicate the generic hint.


def __init__(self, action, hint=None):
self.action = action
super().__init__(msg=f"Unable to {action}; is your computer offline?")
self.hint = hint if hint else self.DEFAULT_HINT
super().__init__(msg=f"Unable to {action}; {self.hint}")


class MissingNetworkResourceError(BriefcaseCommandError):
Expand Down
4 changes: 2 additions & 2 deletions src/briefcase/integrations/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from pathlib import Path
from typing import TYPE_CHECKING, DefaultDict, TypeVar

import requests
import httpx
from cookiecutter.main import cookiecutter

from briefcase.config import AppConfig
Expand Down Expand Up @@ -169,7 +169,7 @@ class ToolCache(Mapping):

# Third party tools
cookiecutter = staticmethod(cookiecutter)
requests = requests
httpx = httpx

def __init__(
self,
Expand Down
102 changes: 60 additions & 42 deletions src/briefcase/integrations/file.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@
from contextlib import suppress
from email.message import Message
from pathlib import Path
from urllib.parse import urlparse

import requests.exceptions as requests_exceptions
from requests import Response
import httpx

from briefcase.exceptions import (
BadNetworkResourceError,
Expand Down Expand Up @@ -176,57 +174,76 @@ def download(self, url: str, download_path: Path, role: str | None = None) -> Pa
download_path.mkdir(parents=True, exist_ok=True)
filename: Path = None
try:
response = self.tools.requests.get(url, stream=True)
if response.status_code == 404:
raise MissingNetworkResourceError(url=url)
elif response.status_code != 200:
raise BadNetworkResourceError(url=url, status_code=response.status_code)

# The initial URL might (read: will) go through URL redirects, so
# we need the *final* response. We look at either the `Content-Disposition`
# header, or the final URL, to extract the cache filename.
cache_full_name = urlparse(response.url).path
header_value = response.headers.get("Content-Disposition")
if header_value:
# Neither requests nor httplib provides a way to parse RFC6266 headers.
# The cgi module *did* have a way to parse these headers, but
# it was deprecated as part of PEP594. PEP594 recommends
# using the email.message module to parse these headers as they
# are near identical format.
# See also:
# * https://tools.ietf.org/html/rfc6266
# * https://peps.python.org/pep-0594/#cgi
msg = Message()
msg["Content-Disposition"] = header_value
filename = msg.get_filename()
if filename:
cache_full_name = filename
cache_name = cache_full_name.split("/")[-1]
filename = download_path / cache_name

if filename.exists():
self.tools.logger.info(f"{cache_name} already downloaded")
else:
self.tools.logger.info(f"Downloading {cache_name}...")
self._fetch_and_write_content(response, filename)
except requests_exceptions.ConnectionError as e:
with self.tools.httpx.stream("GET", url, follow_redirects=True) as response:
if response.status_code == 404:
raise MissingNetworkResourceError(url=url)
elif response.status_code != 200:
raise BadNetworkResourceError(
url=url, status_code=response.status_code
)

# The initial URL might (read: will) go through URL redirects, so
# we need the *final* response. We look at either the `Content-Disposition`
# header, or the final URL, to extract the cache filename.
cache_full_name = response.url.path
header_value = response.headers.get("Content-Disposition")
if header_value:
# Httpx does not provide a way to parse RFC6266 headers.
# The cgi module *did* have a way to parse these headers, but
# it was deprecated as part of PEP594. PEP594 recommends
# using the email.message module to parse these headers as they
# are near identical format.
# See also:
# * https://tools.ietf.org/html/rfc6266
# * https://peps.python.org/pep-0594/#cgi
msg = Message()
msg["Content-Disposition"] = header_value
filename = msg.get_filename()
if filename:
cache_full_name = filename
cache_name = cache_full_name.split("/")[-1]
filename = download_path / cache_name

if filename.exists():
self.tools.logger.info(f"{cache_name} already downloaded")
else:
self.tools.logger.info(f"Downloading {cache_name}...")
self._fetch_and_write_content(response, filename)
except httpx.RequestError as e:
if role:
description = role
else:
description = filename.name if filename else url
raise NetworkFailure(f"download {description}") from e

if isinstance(e, httpx.TooManyRedirects):
# httpx, unlike requests, will not follow redirects indefinitely, and defaults to
# 20 redirects before calling it quits. If the download attempt exceeds 20 redirects,
# Briefcase probably needs to re-evaluate the URLs it is using for that download
# and ideally find a starting point that won't have so many redirects.
hint = "exceeded redirects when downloading the file.\n\nPlease report this as a bug to Briefcase."
elif isinstance(e, httpx.DecodingError):
hint = "the server sent a malformed response."
else:
# httpx.TransportError
# Use the default hint for generic network communication errors
hint = None

raise NetworkFailure(
f"download {description}",
hint,
) from e

return filename

def _fetch_and_write_content(self, response: Response, filename: Path):
"""Write the content from the requests Response to file.
def _fetch_and_write_content(self, response: httpx.Response, filename: Path):
"""Write the content from the httpx Response to file.

The data is initially written in to a temporary file in the Briefcase
cache. This avoids partially downloaded files masquerading as complete
downloads in later Briefcase runs. The temporary file is only moved
to ``filename`` if the download is successful; otherwise, it is deleted.

:param response: ``requests.Response``
:param response: ``httpx.Response``
:param filename: full filesystem path to save data
"""
temp_file = tempfile.NamedTemporaryFile(
Expand All @@ -239,12 +256,13 @@ def _fetch_and_write_content(self, response: Response, filename: Path):
with temp_file:
total = response.headers.get("content-length")
if total is None:
response.read()
temp_file.write(response.content)
else:
progress_bar = self.tools.input.progress_bar()
task_id = progress_bar.add_task("Downloader", total=int(total))
with progress_bar:
for data in response.iter_content(chunk_size=1024 * 1024):
for data in response.iter_bytes(chunk_size=1024 * 1024):
temp_file.write(data)
progress_bar.update(task_id, advance=len(data))

Expand Down
7 changes: 4 additions & 3 deletions tests/commands/create/test_install_app_support_package.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
import sys
from unittest import mock

import httpx
import pytest
from requests import exceptions as requests_exceptions

from briefcase.exceptions import (
InvalidSupportPackage,
Expand Down Expand Up @@ -428,8 +428,9 @@ def test_offline_install(
app_requirements_path_index,
):
"""If the computer is offline, an error is raised."""
create_command.tools.requests.get = mock.MagicMock(
side_effect=requests_exceptions.ConnectionError
stream_mock = create_command.tools.httpx.stream = mock.MagicMock()
stream_mock.return_value.__enter__.side_effect = httpx.TransportError(
"Unstable connection"
)

# Installing while offline raises an error
Expand Down
7 changes: 4 additions & 3 deletions tests/commands/create/test_install_stub_binary.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
import sys
from unittest import mock

import httpx
import pytest
from requests import exceptions as requests_exceptions

from briefcase.exceptions import (
InvalidStubBinary,
Expand Down Expand Up @@ -412,8 +412,9 @@ def test_offline_install(
stub_binary_revision_path_index,
):
"""If the computer is offline, an error is raised."""
create_command.tools.requests.get = mock.MagicMock(
side_effect=requests_exceptions.ConnectionError
stream_mock = create_command.tools.httpx.stream = mock.MagicMock()
stream_mock.return_value.__enter__.side_effect = httpx.TransportError(
Copy link
Contributor Author

@sarayourfriend sarayourfriend Oct 18, 2024

Choose a reason for hiding this comment

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

httpx.stream returns a context manager, so it's necessary to mock __enter__ with either the mocked response or exception. This pattern repeats throughout the PR, especially in test_File__download.py.

"Unstable connection"
)

# Installing while offline raises an error
Expand Down
4 changes: 2 additions & 2 deletions tests/integrations/base/test_ToolCache.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
from pathlib import Path
from unittest.mock import MagicMock

import httpx
import pytest
import requests
from cookiecutter.main import cookiecutter

import briefcase.integrations
Expand Down Expand Up @@ -83,7 +83,7 @@ def test_third_party_tools_available():
assert ToolCache.sys is sys

assert ToolCache.cookiecutter is cookiecutter
assert ToolCache.requests is requests
assert ToolCache.httpx is httpx


def test_always_true(simple_tools, tmp_path):
Expand Down
Loading