fix: Default ProxyInfo port if httpx.URL port is None#619
fix: Default ProxyInfo port if httpx.URL port is None#619vdusek merged 3 commits intoapify:masterfrom
Conversation
Fixes an issue where the string `:None` was being appended to Playwright Proxy configurations.
```
proxy_info: {'server': 'http://p.webshare.io:None', 'username': 'foo', 'password': 'bar'}
```
Neither Firefox or Chrome were able to use a URL like `http://foo:[email protected]:80/`, because the :80 was being stripped off by HTTPX. `httpx.URL.port` cannot be set to 80 (for HTTP) or 443 (HTTPS), see example below.
This change explicitly sets the proxy port if it's unset.
```
In [1]: from httpx import URL
In [2]: url = URL(scheme="http", host="example.com", port=8080)
In [3]: url.port
Out[3]: 8080
In [4]: url = URL(scheme="http", host="example.com", port=80)
In [5]: url.port
In [6]: url = URL(scheme="http", host="example.com", port=443)
In [7]: url.port
Out[7]: 443
In [8]: url = URL(scheme="https", host="example.com", port=443)
In [9]: url.port
```
steffansafey
changed the title
There was a problem hiding this comment.
This looks good, thank you! I just did some polishing... And one more thing, could you please provide also a simple unit test covering this? It should not be hard. Place it on tests/unit/proxy_configuration/test_new_proxy_info.py.
|
...LGTM, but a test would be great, as @vdusek said. |
|
Cheers @janbuchar, @vdusek, I'll send the test through in the next hour or so 👍 |
|
Ready to go again @vdusek, covered a few cases in the test to ensure we can set it explicitly & implicitly. Tests fail when the fix is rolled back so all looking good! Happy for you to touch it up before merge if needed. Good call adding the |
|
Thank you @steffansafey and good job! |
3 participants
Description
Fixes an issue where the string
:Nonewas being appended to Playwright Proxy configurations.Neither Firefox or Chrome were able to use a proxy URL like
http://foo:[email protected]:80/, because the :80 was being stripped off by HTTPX.httpx.URL.portcannot be set to 80 (for HTTP) or 443 (HTTPS), see example below.This change explicitly sets the proxy port if it's unset.
Issues
Closes #618
Testing
Tested locally, Firefox is now able to load pages using a proxy with port 80.
Checklist