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

URL.port becomes Optional[int] #1080

Merged
merged 3 commits into from
Jul 24, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
18 changes: 12 additions & 6 deletions httpx/_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -543,10 +543,13 @@ def _transport_for_url(self, url: URL) -> httpcore.SyncHTTPTransport:
enforce_http_url(url)

if self._proxies and not should_not_be_proxied(url):
is_default_port = (url.scheme == "http" and url.port == 80) or (
url.scheme == "https" and url.port == 443
default_port = {"http": 80, "https": 443}[url.scheme]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a .get() to avoid KeyError: 'ftp'?

Suggested change
default_port = {"http": 80, "https": 443}[url.scheme]
default_port = {"http": 80, "https": 443}.get(url.scheme)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually no, because we've called enforce_http_url(url) before doing anything else, so url.scheme will always be http|https here, and default_port will be an int rather than an Optional[int].

(Agree that it's a bit futzy, and I think it'd potentially clean up a bit if we rethought the proxy_keys/mount API.)

is_default_port = url.port is None or url.port == default_port
hostname = (
f"{url.host}:{default_port}"
if url.port is None
else f"{url.host}:{url.port}"
)
Copy link
Contributor

@StephenBrown2 StephenBrown2 Jul 23, 2020

Choose a reason for hiding this comment

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

Could this be simplified:

Suggested change
hostname = (
f"{url.host}:{default_port}"
if url.port is None
else f"{url.host}:{url.port}"
)
port = url.port or default_port
hostname = f"{url.host}:{port}"

Copy link
Member Author

Choose a reason for hiding this comment

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

Great call yup.

hostname = f"{url.host}:{url.port}"
proxy_keys = (
f"{url.scheme}://{hostname}",
f"{url.scheme}://{url.host}" if is_default_port else None,
Expand Down Expand Up @@ -1076,10 +1079,13 @@ def _transport_for_url(self, url: URL) -> httpcore.AsyncHTTPTransport:
enforce_http_url(url)

if self._proxies and not should_not_be_proxied(url):
is_default_port = (url.scheme == "http" and url.port == 80) or (
url.scheme == "https" and url.port == 443
default_port = {"http": 80, "https": 443}[url.scheme]
is_default_port = url.port is None or url.port == default_port
hostname = (
f"{url.host}:{default_port}"
if url.port is None
else f"{url.host}:{url.port}"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as above

hostname = f"{url.host}:{url.port}"
proxy_keys = (
f"{url.scheme}://{hostname}",
f"{url.scheme}://{url.host}" if is_default_port else None,
Expand Down
10 changes: 4 additions & 6 deletions httpx/_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,9 @@ def host(self) -> str:
return self._uri_reference.host or ""

@property
def port(self) -> int:
def port(self) -> typing.Optional[int]:
port = self._uri_reference.port
if port is None:
return {"https": 443, "http": 80}[self.scheme]
return int(port)
return int(port) if port else None

@property
def path(self) -> str:
Expand All @@ -131,7 +129,7 @@ def fragment(self) -> str:
return self._uri_reference.fragment or ""

@property
def raw(self) -> typing.Tuple[bytes, bytes, int, bytes]:
def raw(self) -> typing.Tuple[bytes, bytes, typing.Optional[int], bytes]:
return (
self.scheme.encode("ascii"),
self.host.encode("ascii"),
Expand Down Expand Up @@ -167,7 +165,7 @@ def copy_with(self, **kwargs: typing.Any) -> "URL":
or "port" in kwargs
):
host = kwargs.pop("host", self.host)
port = kwargs.pop("port", None if self.is_relative_url else self.port)
port = kwargs.pop("port", self.port)
username = kwargs.pop("username", self.username)
password = kwargs.pop("password", self.password)

Expand Down
10 changes: 9 additions & 1 deletion httpx/_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -273,12 +273,20 @@ def enforce_http_url(url: "URL") -> None:
raise InvalidURL('URL scheme must be "http" or "https".')


def port_or_default(url: "URL") -> typing.Optional[int]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be used above?

if url.port is not None:
return url.port
return {"http": 80, "https": 443}.get(url.scheme)


def same_origin(url: "URL", other: "URL") -> bool:
"""
Return 'True' if the given URLs share the same origin.
"""
return (
url.scheme == other.scheme and url.host == other.host and url.port == other.port
url.scheme == other.scheme
and url.host == other.host
and port_or_default(url) == port_or_default(other)
)


Expand Down
14 changes: 11 additions & 3 deletions tests/client/test_proxies.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@
import httpx


def url_to_origin(url: str):
DEFAULT_PORTS = {b"http": 80, b"https": 443}
scheme, host, explicit_port = httpx.URL(url).raw[:3]
default_port = DEFAULT_PORTS[scheme]
port = default_port if explicit_port is None else explicit_port
return scheme, host, port


@pytest.mark.parametrize(
["proxies", "expected_proxies"],
[
Expand All @@ -27,7 +35,7 @@ def test_proxies_parameter(proxies, expected_proxies):
assert proxy_key in client._proxies
proxy = client._proxies[proxy_key]
assert isinstance(proxy, httpcore.AsyncHTTPProxy)
assert proxy.proxy_origin == httpx.URL(url).raw[:3]
assert proxy.proxy_origin == url_to_origin(url)

assert len(expected_proxies) == len(client._proxies)

Expand Down Expand Up @@ -85,7 +93,7 @@ def test_transport_for_request(url, proxies, expected):
assert transport is client._transport
else:
assert isinstance(transport, httpcore.AsyncHTTPProxy)
assert transport.proxy_origin == httpx.URL(expected).raw[:3]
assert transport.proxy_origin == url_to_origin(expected)


@pytest.mark.asyncio
Expand Down Expand Up @@ -131,4 +139,4 @@ def test_proxies_environ(monkeypatch, client_class, url, env, expected):
if expected is None:
assert transport == client._transport
else:
assert transport.proxy_origin == httpx.URL(expected).raw[:3]
assert transport.proxy_origin == url_to_origin(expected)
2 changes: 1 addition & 1 deletion tests/client/test_redirects.py
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ async def test_malformed_redirect():
client = AsyncClient(transport=AsyncMockTransport())
response = await client.get("http://example.org/malformed_redirect")
assert response.status_code == codes.OK
assert response.url == URL("https://example.org/")
assert response.url == URL("https://example.org:443/")
assert len(response.history) == 1


Expand Down
4 changes: 2 additions & 2 deletions tests/models/test_requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,11 @@ def test_url():
url = "http://example.org"
request = httpx.Request("GET", url)
assert request.url.scheme == "http"
assert request.url.port == 80
assert request.url.port is None
assert request.url.full_path == "/"

url = "https://example.org/abc?foo=bar"
request = httpx.Request("GET", url)
assert request.url.scheme == "https"
assert request.url.port == 443
assert request.url.port is None
assert request.url.full_path == "/abc?foo=bar"
4 changes: 2 additions & 2 deletions tests/models/test_url.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
"http://xn--knigsgchen-b4a3dun.de",
"xn--knigsgchen-b4a3dun.de",
"http",
80,
None,
),
("https://faß.de", "https://xn--fa-hia.de", "xn--fa-hia.de", "https", 443),
("https://faß.de", "https://xn--fa-hia.de", "xn--fa-hia.de", "https", None),
(
"https://βόλος.com:443",
"https://xn--nxasmm1c.com:443",
Expand Down
2 changes: 1 addition & 1 deletion tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ def test_ssl_config_support_for_keylog_file(tmpdir, monkeypatch): # pragma: noc
("https://example.com", "https://example.com", {}, "DEFAULT"),
(
"https://user:pass@example.com",
"https://example.com:443",
"https://example.com",
{"proxy-authorization": "Basic dXNlcjpwYXNz"},
"DEFAULT",
),
Expand Down