1897 add type hints connection #2162

franekmagiera · 2021-02-15T16:35:33Z

Related to #1897
Added type hints to connection.py and util/connection.py.
I also added some type hints for ssl_.py, wait.py and proxy.py, but only for the functions that are used in connection.py. I can complete the rest later or as a part of another PR, I didn't want to do it now because it already feels like there are a lot of changes here and wanted to get some feedback first.

There are still some issues, will comment the code shortly.

Also, I used Mapping[str, str] for headers. I saw recent discussions about using HTTPHeaderDict for requests. I think it would be ok to leave headers as Mapping[str, str] for connections and let some upper layer class (like ConnectionPool or PoolManager) worry about transforming HTTPHeaderDict to Mapping[str, str] - what do you think about that?

codecov · 2021-02-15T16:37:32Z

Codecov Report

Merging #2162 (9181469) into main (44ee4ad) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 9181469 differs from pull request most recent head 8b51e60. Consider uploading reports for the commit 8b51e60 to get more accurate results

@@            Coverage Diff            @@
##              main     #2162   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         2240      2219   -21     
=========================================
- Hits          2240      2219   -21

Impacted Files	Coverage Δ
src/urllib3/poolmanager.py	`100.00% <ø> (ø)`
src/urllib3/util/proxy.py	`100.00% <ø> (ø)`
src/urllib3/connection.py	`100.00% <100.00%> (ø)`
src/urllib3/util/connection.py	`100.00% <100.00%> (ø)`
src/urllib3/util/ssl_.py	`100.00% <100.00%> (ø)`
src/urllib3/util/wait.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44ee4ad...8b51e60. Read the comment docs.

src/urllib3/connection.py

franekmagiera · 2021-02-15T16:39:16Z

src/urllib3/connection.py

@@ -160,22 +211,28 @@ def _new_conn(self):



Line 210 - NewConnectionError expects a ConnectionPool - should it then be raised by ConnectionPool or this exception should be modified?

src/urllib3/connection.py

franekmagiera · 2021-02-15T16:48:08Z

src/urllib3/util/connection.py


 from urllib3.exceptions import LocationParseError

 from .wait import wait_for_read

+SOCKET_GLOBAL_DEFAULT_TIMEOUT = socket._GLOBAL_DEFAULT_TIMEOUT  # type: ignore


mypy returns an error - it cannot find socket._GLOBAL_DEFAULT_TIMEOUT, hence type: ignore. Should urllib3's own GLOBAL_DEFAULT_TIMEOUT be defined?

Nah, using a type ignore for this case is fine.

franekmagiera · 2021-02-15T16:48:52Z

src/urllib3/util/connection.py


 from urllib3.exceptions import LocationParseError

 from .wait import wait_for_read

+SOCKET_GLOBAL_DEFAULT_TIMEOUT = socket._GLOBAL_DEFAULT_TIMEOUT  # type: ignore
+SocketOptions = List[Tuple[int, int, int]]


Should this be restricted to tuples of ints or the last value could also be str/bytes?

The type for socket.socket.setsockopt() appears to be Revealed type is 'def (self: socket.socket, level: builtins.int, optname: builtins.int, value: Union[builtins.int, builtins.bytes]) so do int/bytes for last type.

Changed to List[Tuple[int, int, Union[int, bytes]]]

franekmagiera · 2021-02-15T16:50:30Z

src/urllib3/util/ssl_.py

@@ -109,6 +114,9 @@ def assert_fingerprint(cert, fingerprint):
        Fingerprint as string of hexdigits, can be interspersed by colons.
    """

+    if cert is None:


Probably will have to add a test for that.
cert can be None because getpeercert can return None.

getpeercert returns None only for server SSL sockets, so I added a test that directly tests the assert_fingerprint function in test_ssl.py

…, ...]

sethmlarson

Thanks for this, lots of improvements! Here's a bundle of comments for you :)

sethmlarson · 2021-02-21T20:42:02Z

src/urllib3/connection.py


 from .util.proxy import create_proxy_ssl_context
 from .util.util import to_str

-try:  # Compiled with SSL?
+if TYPE_CHECKING:


Isn't BaseSSLError required for being imported elsewhere? If so we can't have TYPE_CHECKING bounding it

Right, it is required by connectionpool and response. I assumed that type checking won't be performed in environments where there is no SSL, in that case should I just ignore everything in the except branch? Also, there are some issues regarding conditional imports in mypy python/mypy#1153

sethmlarson · 2021-02-21T20:42:33Z

src/urllib3/connection.py

@@ -55,6 +74,14 @@ class BaseSSLError(BaseException):
 _CONTAINS_CONTROL_CHAR_RE = re.compile(r"[^-!#$%&'*+.^_`|~0-9a-zA-Z]")


+_DefaultType = Union[bytes, IO[Any], Iterable[bytes], str]  # Type for HTTP body.


Let's have a more descriptive global here for HTTP body type

Changed to HTTPBody

src/urllib3/connection.py

sethmlarson · 2021-02-21T20:45:30Z

src/urllib3/connection.py

+        url: str,
+        body: Optional[_DefaultType] = None,
+        headers: Optional[Mapping[str, str]] = None,
+        *,


The encode_chunked parameter wasn't available before, I don't think we should add it.

Yes, it was added to http.client in 3.6. If it is not added then request method violates LSP because of incompatible signatures. I can't think of a workaround that wouldn't require # type: ignore or a refactor. Having two ways to do the same thing is also not ideal. What about refactoring the request method to do all the work including sending chunked requests and then making request_chunked a convenience method for compatibility that would call request?

I think this is a case where we want to violate LSP as our API in the future is different from http.client.HTTPConnection's API and our subclassing for now is incidental (see BaseHTTPConnection). I'm comfortable having a type: ignore along with a comment about us intentionally violating LSP

Makes sense. Will add comments and type ignores.

By the way, do I understand it correctly that in the future when urllib3.HTTPConnection will inherit from BaseHTTPConnection it would have to contain http.client.HTTPConnection (namely, the relationship will be composition instead of inheritance)?

@franekmagiera Things will remain how they are for 2.x and then in 3.x we'll be using a different HTTP implementation h2 or h11 instead of only http.client.HTTPConnection. BaseHTTPConnection can be used as a parent class for the new HTTP connections.

Added the comment and type ignore for the request method.

src/urllib3/connection.py

sethmlarson · 2021-02-21T20:52:11Z

src/urllib3/util/connection.py


 from urllib3.exceptions import LocationParseError

 from .wait import wait_for_read

+SOCKET_GLOBAL_DEFAULT_TIMEOUT = socket._GLOBAL_DEFAULT_TIMEOUT  # type: ignore
+SocketOptions = List[Tuple[int, int, int]]


The type for socket.socket.setsockopt() appears to be Revealed type is 'def (self: socket.socket, level: builtins.int, optname: builtins.int, value: Union[builtins.int, builtins.bytes]) so do int/bytes for last type.

sethmlarson · 2021-02-21T20:53:13Z

src/urllib3/util/proxy.py

+    cert_reqs: Optional[int] = None,
+    ca_certs: Optional[str] = None,
+    ca_cert_dir: Optional[str] = None,
+    ca_cert_data: Optional[str] = None,


Can ca_cert_data be of type bytes? I'm not sure

Right, typeshed uses cadata: Union[Text, bytes, None] = ...

sethmlarson · 2021-02-21T20:54:01Z

src/urllib3/util/ssl_.py

@@ -33,32 +35,35 @@ def _is_ge_openssl_v1_1_1(
    )


-try:  # Do we have ssl at all?
+if TYPE_CHECKING:


This if statement can be independent from the try-except below I believe? Basically we still want the from ssl import ... to happen even when type checking

sethmlarson · 2021-02-21T20:55:49Z

test/with_dummyserver/test_connectionpool.py

        # This test needs to be here in order to be run. socket.create_connection actually tries
        # to connect to the host provided so we need a dummyserver to be running.
-        with HTTPConnectionPool(self.host, self.port, socket_options=None) as pool:
+        with HTTPConnectionPool(self.host, self.port, socket_options=[]) as pool:


Since it seems we're supporting None as well can we instead parametize this test for both None and []?

Parameterized the test for None and []

sethmlarson · 2021-02-22T16:14:08Z

src/urllib3/connection.py

        if headers is None:
            headers = {}
        else:
            # Avoid modifying the headers passed into .request()
-            headers = headers.copy()
+            headers = copy(headers)


Can we avoid using copy.copy()? Why does mypy balk at headers = headers.copy()?

This is because headers are of type Mapping[str, str] and Mapping does not have a copy method. Changing headers type would again violate LSP, so the same situation as with the code_enchunked discussion. So, probably makes sense to add comments and type ignores again and make headers to be dict for now? This also raises the question if in the BaseHTTPConnection.request should take headers as dict or HTTPHeaderDict (or union of both, which seems best I think). Related to #2158

franekmagiera · 2021-02-22T19:29:14Z

@sethmlarson Thanks for doing the review! I made some changes, will address the rest tomorrow.

sethmlarson · 2021-02-27T16:51:24Z

src/urllib3/connection.py

@@ -23,6 +23,8 @@
    cast,
 )

+from typing_extensions import Literal


Can't have typing_extensions be top-level, instead use if typing.TYPE_CHECKING: from typing import Literal and then string literals "Literal[False]"

Right, forgot its just a dev dependency... Changed that.

…-connection

KapJI · 2021-04-03T11:37:00Z

@sethmlarson any updates on this one? Looking forward to see fully typed urllib3 🙂

sethmlarson

LGTM, thank you for all of this work!!! Sorry it took so long for me to review, these days are busy :)

franekmagiera added 9 commits February 11, 2021 18:44

Add some type hints to HTTPConnection

f43d482

Add more types to HTTPConnection

ec4afd1

Add more type hints

79cfb18

Add more type hints

db2149b

Add more type hints

d32cc52

Add more type hints, fix failing tests

b60244e

Remove comments

7905d13

Merge branch 'main' into 1897-add-type-hints-connection

4673fe5

Merge master and fix conflicts

a177582