refactor: goundwork/cleanup before passing custom http client

CHANGELOG.md

@@ -4,6 +4,7 @@
 * (Major): Settings can no longer be changed on an existing client. The `unleash_*` settings properties and the `strategy_mapping` and `connector_mode` properties have been removed, together with their setters.
 * (Major): The API token is passed as `api_key="<API token>"`. An `Authorization` header in `HttpOptions.headers` raises `ValueError`.
 * (Major): The deprecated `environment` setting has been removed. The `environment` field of the evaluation context and the impact metrics `environment` label come from the API token, or are `"default"` when the token doesn't name one.
+* (Major): The `UnleashClient` and `AsyncUnleashClient` constructors raise `ValueError` if `url` is not an absolute http(s) URL with a host, for every client.
 * (Major): Upgraded to yggdrasil-engine 3, new engine built with PyO3 for better performance.
 * (Minor): New `FETCH_COMPLETED` event, emitted after every feature fetch with how it ended: `CHANGED`, `NOT_CHANGED`, `UNAUTHORIZED`, `RATE_LIMITED`, `UNAVAILABLE` or `INVALID_RESPONSE`.
 * (Minor): `feature_definitions()` reports whether each feature is enabled in the environment.

UnleashClient/clients/async_unleash_client.py

@@ -37,7 +37,7 @@
 from UnleashClient.constants import ETAG, METRIC_LAST_SENT_TIME
 from UnleashClient.events import BaseEvent
 from UnleashClient.impact_metrics import ImpactMetrics
-from UnleashClient.utils import LOGGER, InstanceAllowType
+from UnleashClient.utils import LOGGER, InstanceAllowType, _validate_url
 
 
 class AsyncUnleashClient:
@@ -65,7 +65,7 @@ class AsyncUnleashClient:
             client.impact_metrics.define_counter("purchases", "Number of purchases")
             client.impact_metrics.increment_counter("purchases")
 
-    :param url: URL of the Unleash server, required.
+    :param url: URL of the Unleash server, required. Must be an absolute http(s) URL with a host.
     :param app_name: Name of the application using the client, required.
     :param api_key: Client API token.  Optional, because a client that only loads from a backup never talks to the server.
     :param identity: How the client identifies itself.
@@ -307,8 +307,10 @@ async def initialize_client(self, fetch_toggles: bool = True) -> None:
 
         :param fetch_toggles: Accepted for parity with :class:`UnleashClient`. It
                               has no effect: the client always polls.
-        :raises aiohttp.InvalidURL: If registration is enabled and the URL is invalid.
         :raises ValueError: If a custom strategy is invalid.
+        :raises aiohttp.InvalidURL: If registration is enabled and aiohttp
+                                    rejects a URL that passed the constructor's
+                                    check.
         """
         if self._closed or self._starting or self._run_state > _RunState.UNINITIALIZED:
             warnings.warn(
@@ -318,6 +320,8 @@ async def initialize_client(self, fetch_toggles: bool = True) -> None:
 
         self._starting = True
         try:
+            _validate_url(self._config.url)
+
             self._cache.mset(
                 {METRIC_LAST_SENT_TIME: datetime.now(timezone.utc), ETAG: ""}
             )

UnleashClient/clients/unleash_client.py

@@ -49,6 +49,7 @@
 from UnleashClient.utils import (
     LOGGER,
     InstanceAllowType,
+    _validate_url,
 )
 
 INSTANCES = _get_instance_registry()
@@ -272,7 +273,7 @@ def initialize_client(self, fetch_toggles: bool = True) -> None:
         usually used to cater the multi-process setups, e.g. Django, Celery,
         etc.
 
-        This will raise an exception on registration if the URL is invalid. It is done automatically if called inside a context manager as in:
+        It is done automatically if called inside a context manager as in:
 
         .. code-block:: python
 
@@ -289,6 +290,8 @@ def initialize_client(self, fetch_toggles: bool = True) -> None:
             try:
                 start_scheduler = False
 
+                _validate_url(self._config.url)
+
                 if self._custom_strategies:
                     self._engine.register_custom_strategies(self._custom_strategies)
 

UnleashClient/config.py

@@ -6,7 +6,11 @@
 from typing import Any, Mapping, Optional, Union
 
 from UnleashClient.constants import REQUEST_RETRIES, REQUEST_TIMEOUT
-from UnleashClient.utils import LOGGER, extract_environment_from_api_key
+from UnleashClient.utils import (
+    LOGGER,
+    _validate_url,
+    extract_environment_from_api_key,
+)
 
 
 def redact_to_print_safely(value: Optional[str]) -> Optional[str]:
@@ -120,6 +124,12 @@ class UnleashConfig:
     Types are not validated or coerced beyond ``url.rstrip("/")`` and ``int()``
     on the two jitters.  Option groups left as None get their defaults.
 
+    ``url`` must be an absolute http(s) URL with a host, even for a client that
+    never talks to the server: deciding which settings make requests would need
+    updating with every new server call.
+
+    :raises ValueError: if ``url`` is not an absolute http(s) URL with a host.
+
     ``api_key`` is optional because a client that only loads from a backup never talks to the server.
     """
 
@@ -140,6 +150,7 @@ class UnleashConfig:
 
     def __post_init__(self) -> None:
         object.__setattr__(self, "url", self.url.rstrip("/"))
+        _validate_url(self.url)
         if self.identity is None:
             object.__setattr__(self, "identity", IdentityOptions())
         if self.http is None:

UnleashClient/constants.py

@@ -1,12 +1,13 @@
 from importlib_metadata import version
+from yggdrasil_engine.engine import UnleashEngine
 
 # Library
 SDK_NAME = "unleash-python-sdk"
 SDK_VERSION = version("UnleashClient")
 REQUEST_TIMEOUT = 30
 REQUEST_RETRIES = 3
 METRIC_LAST_SENT_TIME = "mlst"
-CLIENT_SPEC_VERSION = "6.1.0"
+CLIENT_SPEC_VERSION = UnleashEngine.get_spec_version()
 
 # =Unleash=
 APPLICATION_HEADERS = {

UnleashClient/utils.py

@@ -1,8 +1,7 @@
 import logging
 from enum import Enum
 from typing import Any, Optional
-
-import mmh3  # pylint: disable=import-error
+from urllib.parse import urlsplit
 
 LOGGER = logging.getLogger("UnleashClient")
 
@@ -13,16 +12,6 @@ class InstanceAllowType(Enum):
     SILENTLY_ALLOW = 3
 
 
-def normalized_hash(
-    identifier: str, activation_group: str, normalizer: int = 100, seed: int = 0
-) -> int:
-    return (
-        mmh3.hash(f"{activation_group}:{identifier}", signed=False, seed=seed)
-        % normalizer
-        + 1
-    )
-
-
 def get_identifier(context_key_name: str, context: dict) -> Any:
     if context_key_name in context.keys():
         value = context[context_key_name]
@@ -47,3 +36,29 @@ def extract_environment_from_api_key(api_key: Optional[str]) -> Optional[str]:
 
     environment, _, _ = after_colon.partition(".")
     return environment or None
+
+
+def _validate_url(url: str) -> None:
+    """
+    Checks that ``url`` has the shape of an Unleash API url.
+
+    Only the shape is checked, not whether the host exists or answers: that is a
+    network failure, which the client reports.
+
+    :raises ValueError: if ``url`` is not an absolute http(s) url with a host.
+    """
+    if any(c.isspace() for c in url):
+        raise ValueError(f"Unleash URL contains whitespace: {url!r}")
+
+    try:
+        parts = urlsplit(url)
+        _ = parts.port
+    except ValueError as exc:
+        raise ValueError(f"Invalid Unleash URL {url!r}: {exc}") from exc
+
+    if parts.scheme not in ("http", "https"):
+        raise ValueError(
+            f"Unleash URL must start with http:// or https://, got {url!r}"
+        )
+    if not parts.hostname:
+        raise ValueError(f"Unleash URL has no host: {url!r}")

pyproject.toml

@@ -24,7 +24,6 @@ classifiers=[
 dependencies=[
     "requests",
     "fcache",
-    "mmh3",
     "apscheduler < 4.0.0",
     "importlib_metadata",
     "python-dateutil",

requirements.txt

@@ -2,7 +2,6 @@
 APScheduler
 fcache
 importlib_metadata
-mmhash3
 python-dateutil
 requests
 semver

tests/unit_tests/clients/test_async_unleash_client.py

@@ -627,6 +627,17 @@ async def test_initializing_twice_warns_and_registers_once(
     assert len(server.calls("POST", REGISTER_PATH)) == 1
 
 
+@pytest.mark.parametrize(
+    "metrics",
+    [MetricsOptions(), MetricsOptions(disabled=True, disable_registration=True)],
+)
+def test_async_client_with_an_invalid_url_raises_on_construction(tmpdir, metrics):
+    with pytest.raises(ValueError, match="must start with http:// or https://"):
+        build_async_client(
+            tmpdir, url="thisisnotavalidurl", app_name=APP_NAME, metrics=metrics
+        )
+
+
 @pytest.mark.asyncio
 async def test_initializing_a_destroyed_client_warns_and_does_nothing(
     server, build_running_client

tests/unit_tests/clients/test_unleash_client.py

@@ -839,14 +839,48 @@ def test_uc_server_error(readyable_unleash_client):
     assert unleash_client.is_enabled("testFlag")
 
 
-def test_uc_with_invalid_url(tmp_path):
+@pytest.mark.parametrize(
+    "metrics",
+    [
+        MetricsOptions(),
+        MetricsOptions(disable_registration=True),
+        MetricsOptions(disabled=True, disable_registration=True),
+    ],
+)
+def test_uc_with_invalid_url_raises_on_construction(tmp_path, metrics):
+    with pytest.raises(ValueError, match="must start with http:// or https://"):
+        UnleashClient(
+            "thisisnotavalidurl",
+            APP_NAME,
+            metrics=metrics,
+            cache_directory=str(tmp_path),
+        )
+
+
+def test_uc_with_invalid_url_is_not_counted_as_an_instance(tmp_path, caplog):
+    with pytest.raises(ValueError):
+        UnleashClient("thisisnotavalidurl", APP_NAME, cache_directory=str(tmp_path))
+
+    unleash_client = UnleashClient(URL, APP_NAME, cache_directory=str(tmp_path))
+    try:
+        assert "You already have" not in caplog.text
+    finally:
+        unleash_client.destroy()
+
+
+def test_uc_offline_starts_with_a_well_formed_url(tmp_path):
     unleash_client = UnleashClient(
-        "thisisnotavalidurl", APP_NAME, cache_directory=str(tmp_path)
+        "http://localhost",
+        APP_NAME,
+        metrics=MetricsOptions(disabled=True, disable_registration=True),
+        cache_directory=str(tmp_path),
     )
+    try:
+        unleash_client.initialize_client(fetch_toggles=False)
 
-    with pytest.raises(ValueError):
-        unleash_client.initialize_client()
-    unleash_client.destroy()
+        assert isinstance(unleash_client.connector, OfflineConnector)
+    finally:
+        unleash_client.destroy()
 
 
 def test_uc_with_network_error(tmp_path):

tests/unit_tests/test_async_transport.py

@@ -264,7 +264,17 @@ async def test_register_sends_the_payload_it_is_given(server, transport):
     assert json.loads(body) == {"appName": APP_NAME, "strategies": []}
 
 
+@pytest.fixture
+def skip_url_validation(monkeypatch):
+    """
+    UnleashConfig rejects these urls on construction, so register() only sees
+    one that slipped past that check. Skip it to reach the re-raise.
+    """
+    monkeypatch.setattr("UnleashClient.config._validate_url", lambda url: None)
+
+
 @mark.asyncio
+@mark.usefixtures("skip_url_validation")
 @mark.parametrize(
     "url", ("thisisnotavalidurl", "localhost:4242/api"), ids=("no-scheme", "bad-scheme")
 )
@@ -278,6 +288,7 @@ async def test_register_reraises_an_unusable_url(server, build_transport, url):
 
 
 @mark.asyncio
+@mark.usefixtures("skip_url_validation")
 async def test_register_reraises_a_missing_scheme_as_a_value_error(build_transport):
     transport = build_transport(url="thisisnotavalidurl")
 

tests/unit_tests/test_config.py

@@ -78,6 +78,16 @@ def test_url_is_rstripped():
     assert UnleashConfig(URL, APP_NAME).url == URL
 
 
+def test_url_is_validated():
+    with pytest.raises(ValueError, match="must start with http:// or https://"):
+        UnleashConfig("thisisnotavalidurl", APP_NAME)
+
+
+def test_url_is_validated_after_rstripping():
+    with pytest.raises(ValueError, match="has no host"):
+        UnleashConfig("https://", APP_NAME)
+
+
 def test_jitters_are_int_coerced():
     config = UnleashConfig(
         URL,

tests/unit_tests/test_transport.py

@@ -233,18 +233,28 @@ def test_register_sends_the_payload_it_is_given(transport):
     }
 
 
+@pytest.fixture
+def skip_url_validation(monkeypatch):
+    """
+    UnleashConfig rejects these urls on construction, so register() only sees
+    one that slipped past that check. Skip it to reach the re-raise.
+    """
+    monkeypatch.setattr("UnleashClient.config._validate_url", lambda url: None)
+
+
 @responses.activate
+@pytest.mark.usefixtures("skip_url_validation")
 def test_register_reraises_a_missing_schema(build_transport):
     transport = build_transport(url="thisisnotavalidurl")
 
-    # initialize_client() fails loudly on a malformed URL because these four
-    # are re-raised rather than reported as False. See test_uc_with_invalid_url,
-    # which only sees the ValueError that MissingSchema also subclasses.
+    # These four are re-raised rather than reported as False, so a url
+    # requests rejects still fails initialize_client() loudly.
     with pytest.raises(MissingSchema):
         transport.register({})
 
 
 @responses.activate
+@pytest.mark.usefixtures("skip_url_validation")
 def test_register_reraises_an_invalid_schema(build_transport):
     transport = build_transport(url="localhost:4242/api")
 

tests/unit_tests/test_utils.py

@@ -1,4 +1,6 @@
-from UnleashClient.utils import extract_environment_from_api_key
+import pytest
+
+from UnleashClient.utils import _validate_url, extract_environment_from_api_key
 
 
 def test_valid_api_key():
@@ -14,3 +16,37 @@ def test_api_key_not_present():
 def test_environment_part_is_empty():
     result = extract_environment_from_api_key("project:.hash")
     assert result is None
+
+
+@pytest.mark.parametrize(
+    "url",
+    [
+        "http://localhost:4242/api",
+        "https://unleash.example.com/api",
+        "https://unleash.example.com:8443/api",
+        "http://127.0.0.1/api",
+        "http://[::1]:4242/api",
+    ],
+)
+def test_validate_url_accepts_http_urls_with_a_host(url):
+    _validate_url(url)
+
+
+@pytest.mark.parametrize(
+    "url, message",
+    [
+        ("thisisnotavalidurl", "must start with http:// or https://"),
+        ("localhost:4242/api", "must start with http:// or https://"),
+        ("ftp://unleash.example.com", "must start with http:// or https://"),
+        ("", "must start with http:// or https://"),
+        ("https:", "has no host"),
+        ("https:///api", "has no host"),
+        ("https://unleash.example.com:abc/api", "Invalid Unleash URL"),
+        ("https://unleash.example.com:99999/api", "Invalid Unleash URL"),
+        ("https://unleash.example.com/my api", "contains whitespace"),
+        (" https://unleash.example.com/api", "contains whitespace"),
+    ],
+)
+def test_validate_url_rejects_malformed_urls(url, message):
+    with pytest.raises(ValueError, match=message):
+        _validate_url(url)