Bugfix client proxy autodiscovery (#40)

* Remove the HTTPTransport from the client building ...

Adding a blank transport removes our auto-discovery of the
proxy configurations. Removing it for the base level tests
appears to allow for automatic proxy configuration discovery

* Re-add the alwayslist utility function

* Add custom proxy mount creation for the cache clients ...

Due to the httpx client constructor design, if we pass
and HTTPTransport, then it assumes the proxy has been configured
and will not attempt to discover it from the environment.
Therefore we have to do that manually for the cache clients
as hishel requires an HTTPTransport. We due this through the
mounts property provided by the client constructor

* Disable client request timeouts ...

I don't believe this will be permanent behavior but due
to the transition from 0.3.1 -> 0.4.0 with the new HTTP library,
I want to maintain as much backwards compatibility for the moment
so that behavior doesn't suddenly change for users. In the future
I'd likely set the timeout to around 60 seconds with the option
for user configuration upon client creation to allow customization

---------

Co-authored-by: jschaff <jschaff@scripps.edu>

Author: ctrl-schaff

biothings_client/__init__.py

@@ -6,13 +6,15 @@
 from biothings_client.client.base import BiothingClient, get_client
 from biothings_client.__version__ import __version__
 from biothings_client._dependencies import _CACHING, _PANDAS
+from biothings_client.utils._external import alwayslist
 
 __all__ = [
     "AsyncBiothingClient",
     "BiothingClient",
     "_CACHING",
     "_PANDAS",
     "__version__",
+    "alwayslist",
     "get_async_client",
     "get_client",
 ]

biothings_client/client/asynchronous.py

@@ -48,6 +48,9 @@
 logger.setLevel(logging.INFO)
 
 
+PROXY_MOUNT = Dict[str, Union[httpx.BaseTransport, None]]
+
+
 # Future work:
 # Consider use "verbose" settings to control default logging output level
 # by doing this instead of using branching throughout the application,
@@ -100,15 +103,20 @@ async def _build_http_client(self, cache_db: Union[str, Path] = None) -> None:
         This modifies the state of the BiothingsClient instance
         to set the values for the http_client property
 
+        For the moment, we have disabled timeouts. This matches our prior
+        behavior we had with the requests library, which by default did not
+        specify a timeout when making a request. In the future this should
+        be modified to prevent indefinite hanging with potentially bad network
+        connections
+
         Inputs:
         :param cache_db: pathlike object to the local sqlite3 cache database file
 
         Outputs:
         :return: None
         """
         if not self.http_client_setup:
-            http_transport = httpx.AsyncHTTPTransport()
-            self.http_client = httpx.AsyncClient(transport=http_transport)
+            self.http_client = httpx.AsyncClient(timeout=None)
             self.http_client_setup = True
             self.http_cache_client_setup = False
 
@@ -122,6 +130,12 @@ async def _build_cache_http_client(self, cache_db: Union[str, Path] = None) -> N
         This modifies the state of the BiothingsClient instance
         to set the values for the http_client property and the cache_storage property
 
+        For the moment, we have disabled timeouts. This matches our prior
+        behavior we had with the requests library, which by default did not
+        specify a timeout when making a request. In the future this should
+        be modified to prevent indefinite hanging with potentially bad network
+        connections
+
         Inputs:
         :param cache_db: pathlike object to the local sqlite3 cache database file
 
@@ -135,15 +149,47 @@ async def _build_cache_http_client(self, cache_db: Union[str, Path] = None) -> N
 
             self.cache_storage = AsyncBiothingsClientSqlite3Cache()
             await self.cache_storage.setup_database_connection(cache_db)
-            cache_transport = hishel.AsyncCacheTransport(
-                transport=httpx.AsyncHTTPTransport(), storage=self.cache_storage
-            )
+
+            async_http_transport = httpx.AsyncHTTPTransport()
+            cache_transport = hishel.AsyncCacheTransport(transport=async_http_transport, storage=self.cache_storage)
             cache_controller = hishel.Controller(cacheable_methods=["GET", "POST"])
+
+            # Have to manually build the proxy mounts as httpx will not auto-discover
+            # proxies if we provide our own HTTPTransport to the Client constructor
+            proxy_mounts = self._build_caching_proxy_mounts()
             self.http_client = hishel.AsyncCacheClient(
-                controller=cache_controller, transport=cache_transport, storage=self.cache_storage
+                controller=cache_controller,
+                transport=cache_transport,
+                storage=self.cache_storage,
+                mounts=proxy_mounts,
+                timeout=None,
             )
             self.http_client_setup = True
 
+    def _build_caching_proxy_mounts(self) -> PROXY_MOUNT:
+        """
+        Builds the proxy mounts for case where we have a CacheTransport.
+        Autodiscovery of proxies only works when don't provide a transport
+        to the client so this method acts as a replacement for that
+        """
+        proxy_map = httpx._utils.get_environment_proxies()
+        proxy_mounts: PROXY_MOUNT = {}
+        for key, proxy in proxy_map.items():
+            proxy_transport = None
+            if proxy is not None:
+                proxy_transport = httpx.AsyncHTTPTransport(
+                    verify=True,
+                    cert=None,
+                    trust_env=True,
+                    http1=True,
+                    http2=False,
+                    limits=httpx._config.DEFAULT_LIMITS,
+                    proxy=proxy,
+                )
+            proxy_mounts[key] = proxy_transport
+        proxy_mounts = dict(sorted(proxy_mounts.items()))
+        return proxy_mounts
+
     async def __del__(self):
         """
         Destructor for the client to ensure that we close any potential

biothings_client/client/base.py

@@ -49,6 +49,9 @@
 logger.setLevel(logging.INFO)
 
 
+PROXY_MOUNT = Dict[str, Union[httpx.BaseTransport, None]]
+
+
 # Future work:
 # Consider use "verbose" settings to control default logging output level
 # by doing this instead of using branching throughout the application,
@@ -103,10 +106,15 @@ def _build_http_client(self, cache_db: Union[str, Path] = None) -> None:
 
         This modifies the state of the BiothingsClient instance
         to set the values for the http_client property
+
+        For the moment, we have disabled timeouts. This matches our prior
+        behavior we had with the requests library, which by default did not
+        specify a timeout when making a request. In the future this should
+        be modified to prevent indefinite hanging with potentially bad network
+        connections
         """
         if not self.http_client_setup:
-            http_transport = httpx.HTTPTransport()
-            self.http_client = httpx.Client(transport=http_transport)
+            self.http_client = httpx.Client(timeout=None)
             self.http_client_setup = True
             self.http_cache_client_setup = False
 
@@ -119,6 +127,12 @@ def _build_cache_http_client(self, cache_db: Union[str, Path] = None) -> None:
 
         This modifies the state of the BiothingsClient instance
         to set the values for the http_client property and the cache_storage property
+
+        For the moment, we have disabled timeouts. This matches our prior
+        behavior we had with the requests library, which by default did not
+        specify a timeout when making a request. In the future this should
+        be modified to prevent indefinite hanging with potentially bad network
+        connections
         """
         if not self.http_client_setup:
             if cache_db is None:
@@ -127,13 +141,47 @@ def _build_cache_http_client(self, cache_db: Union[str, Path] = None) -> None:
 
             self.cache_storage = BiothingsClientSqlite3Cache()
             self.cache_storage.setup_database_connection(cache_db)
-            cache_transport = hishel.CacheTransport(transport=httpx.HTTPTransport(), storage=self.cache_storage)
+
+            http_transport = httpx.HTTPTransport()
+            cache_transport = hishel.CacheTransport(transport=http_transport, storage=self.cache_storage)
             cache_controller = hishel.Controller(cacheable_methods=["GET", "POST"])
+
+            # Have to manually build the proxy mounts as httpx will not auto-discover
+            # proxies if we provide our own HTTPTransport to the Client constructor
+            proxy_mounts = self._build_caching_proxy_mounts()
             self.http_client = hishel.CacheClient(
-                controller=cache_controller, transport=cache_transport, storage=self.cache_storage
+                controller=cache_controller,
+                transport=cache_transport,
+                storage=self.cache_storage,
+                mounts=proxy_mounts,
+                timeout=None,
             )
             self.http_client_setup = True
 
+    def _build_caching_proxy_mounts(self) -> PROXY_MOUNT:
+        """
+        Builds the proxy mounts for case where we have a CacheTransport.
+        Autodiscovery of proxies only works when don't provide a transport
+        to the client so this method acts as a replacement for that
+        """
+        proxy_map = httpx._utils.get_environment_proxies()
+        proxy_mounts: PROXY_MOUNT = {}
+        for key, proxy in proxy_map.items():
+            proxy_transport = None
+            if proxy is not None:
+                proxy_transport = httpx.HTTPTransport(
+                    verify=True,
+                    cert=None,
+                    trust_env=True,
+                    http1=True,
+                    http2=False,
+                    limits=httpx._config.DEFAULT_LIMITS,
+                    proxy=proxy,
+                )
+            proxy_mounts[key] = proxy_transport
+        proxy_mounts = dict(sorted(proxy_mounts.items()))
+        return proxy_mounts
+
     def __del__(self):
         """
         Destructor for the client to ensure that we close any potential

biothings_client/utils/_external.py

@@ -0,0 +1,36 @@
+"""
+Any functions we provide as auxillary or helper
+functions for the client are stored here and exposed in the
+__init__ for usage by the users
+
+We likely don't use these throughout the biothings.api core
+code, but the users might so we want to maintain that here while
+also indicating the purposes of the collection of functions in the
+module
+"""
+
+from typing import Any, Union
+
+
+def alwayslist(value: Any) -> Union[list, tuple]:
+    """
+    Simple transformation function that ensures the output is an iterable.
+
+    Control Flow:
+    If the input value is a {list, tuple}, we no-opt and return the value unchanged
+    Otherwise, we wrap the value in a list and then return that list
+
+    Example:
+
+    >>> x = 'abc'
+    >>> for xx in alwayslist(x):
+    ...     print xx
+    >>> x = ['abc', 'def']
+    >>> for xx in alwayslist(x):
+    ...     print xx
+
+    """
+    if isinstance(value, (list, tuple)):
+        return value
+    else:
+        return [value]

tests/conftest.py

@@ -2,6 +2,8 @@
 Fixtures for the biothings_client testing
 """
 
+import os
+
 import pytest
 
 from biothings_client import get_client, get_async_client
@@ -111,3 +113,12 @@ def async_geneset_client() -> AsyncMyGenesetInfo:
     client = "geneset"
     geneset_client = get_async_client(client)
     return geneset_client
+
+
+@pytest.fixture(scope="function")
+def mock_client_proxy_configuration() -> None:
+    os.environ["HTTP_PROXY"] = "http://fakehttpproxyhost:6374"
+    os.environ["HTTPS_PROXY"] = "http://fakehttpsproxyhost:6375"
+    yield
+    os.environ.pop("HTTP_PROXY", None)
+    os.environ.pop("HTTPS_PROXY", None)

tests/test_async.py

@@ -4,6 +4,7 @@
 
 from typing import List
 
+import httpx
 import pytest
 
 import biothings_client
@@ -82,3 +83,73 @@ async def test_url_protocol(client_name: str):
     # Transform back to HTTPS
     client_instance.use_https()
     client_instance.url.startswith(https_protocol)
+
+
+@pytest.mark.asyncio
+async def test_async_client_proxy_discovery(mock_client_proxy_configuration):
+    """
+    Tests for verifying that we properly auto-discover the
+    proxy configuration from the environment using the built-in
+    methods provided by HTTPX
+
+    Brought to light by user issues on VPN
+    https://github.com/biothings/mygene.py/issues/26#issuecomment-2588065562
+    """
+    client_name = "gene"
+    gene_client = biothings_client.get_async_client(client_name)
+    await gene_client._build_http_client()
+    for url_pattern, http_transport in gene_client.http_client._mounts.items():
+        assert isinstance(url_pattern, httpx._utils.URLPattern)
+        assert isinstance(http_transport, httpx.AsyncHTTPTransport)
+
+        if url_pattern.pattern == "https://":
+            proxy_url = http_transport._pool._proxy_url
+            assert proxy_url.scheme == b"http"
+            assert proxy_url.host == b"fakehttpsproxyhost"
+            assert proxy_url.port == 6375
+            assert proxy_url.target == b"/"
+
+        elif url_pattern.pattern == "http://":
+            proxy_url = http_transport._pool._proxy_url
+            assert proxy_url.scheme == b"http"
+            assert proxy_url.host == b"fakehttpproxyhost"
+            assert proxy_url.port == 6374
+            assert proxy_url.target == b"/"
+
+
+@pytest.mark.asyncio
+@pytest.mark.skipif(not biothings_client._CACHING, reason="caching libraries not installed")
+async def test_async_cache_client_proxy_discovery(mock_client_proxy_configuration):
+    """
+    Tests for verifying that we properly auto-discover the
+    proxy configuration from the environment using the built-in
+    methods provided by HTTPX
+
+    Brought to light by user issues on VPN
+    https://github.com/biothings/mygene.py/issues/26#issuecomment-2588065562
+    """
+    client_name = "gene"
+    gene_client = biothings_client.get_async_client(client_name)
+    await gene_client._build_cache_http_client()
+
+    http_mounts = gene_client.http_client._mounts
+    assert isinstance(http_mounts, dict)
+    assert len(http_mounts) == 2
+
+    for url_pattern, http_transport in gene_client.http_client._mounts.items():
+        assert isinstance(url_pattern, httpx._utils.URLPattern)
+        assert isinstance(http_transport, httpx.AsyncHTTPTransport)
+
+        if url_pattern.pattern == "https://":
+            proxy_url = http_transport._pool._proxy_url
+            assert proxy_url.scheme == b"http"
+            assert proxy_url.host == b"fakehttpsproxyhost"
+            assert proxy_url.port == 6375
+            assert proxy_url.target == b"/"
+
+        elif url_pattern.pattern == "http://":
+            proxy_url = http_transport._pool._proxy_url
+            assert proxy_url.scheme == b"http"
+            assert proxy_url.host == b"fakehttpproxyhost"
+            assert proxy_url.port == 6374
+            assert proxy_url.target == b"/"