From fd1b644c94eb98dcfb4244808413782385cc3a69 Mon Sep 17 00:00:00 2001 From: Chandrasekharan M Date: Thu, 10 Sep 2026 17:42:54 +0530 Subject: [PATCH] refactor: fold the platform client into a shared facade base PR #29 gave the platform client its own copy of the transport: the pool, the close/reopen handling and the httpx-to-requests exception translation were duplicated from APIDeploymentsClient, and the copy had no retry policy at all. Two copies of that code drift; only one of them was getting fixes. Both clients now inherit `_HttpxFacade`, which owns the lazily built pooled transport, `close`/context-manager support, the exception translation and the retry-with-Retry-After policy. A client subclass supplies its error class and its methods, nothing else. That drops ~184 duplicated lines and gives the platform operations the retry behaviour the README already promised. Three defects fall out of sharing the code: - The transport pool is now built inside the lock. `AuthenticatedClient .get_httpx_client()` builds lazily and unsynchronised, so publishing the client before warming it let two threads build two pools. - A close during flight no longer escapes untranslated. httpx answers a send on a closed client with a bare `RuntimeError`, which is not in the subtree `_translate_transport_errors` covers, so it reached callers catching the documented `requests` types. It is translated at the send. - `list_deployments` no longer sends `workflow=None`. The generated builder renders that parameter with `str()` before it filters `None` out, so the literal string "None" went on the wire as a filter matching no workflow on every otherwise unfiltered call. Unset filters are omitted instead, which also holds if the generator special-cases another parameter later. Exceptions get a hierarchy. `APIDeploymentsClientException` never worked -- its `__init__` nested three more `def`s that were never bound to the class, so `message` was dropped and `Exception.__init__` was never called, leaving `str(e)` empty and the documented `error_message()` non-existent. It is now an alias of a new `UnstractError` base, with `APIDeploymentError` and `PlatformClientError` beneath it. Catching the old name still catches both clients, including anything added later. Also here: - The generated models are re-exported, so callers who want typing can `WhoAmIResponse.from_dict(...)` instead of us hand-writing a mirror of the spec that regeneration would not update. Facade methods keep returning `dict[str, Any]`. - The platform client gets its own logger. Both clients shared the module logger, so levelling one re-levelled the other, switching a live sibling's debug output -- which includes response bodies -- on or off as a side effect. - A 2xx body that is unreadable, or JSON that is not an object, is now an error naming what arrived rather than an `AttributeError` downstream. The ERROR log for it is bounded to the same excerpt the exception carries. - An `org_id` that is empty or blank is refused before the request, and a path on `base_url` is warned about rather than silently discarded by `urljoin`. - `.claude/skills/spec-upgrade/SKILL.md` step 5 gains the recipe for adding an operation: which class it belongs to, the method shape, and why it builds from `_get_kwargs` rather than `sync_detailed`. No runtime breaking change. The one visible shift is `type(e).__name__`, which becomes `APIDeploymentError` where it was `APIDeploymentsClientException`; `except APIDeploymentsClientException` is unaffected. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CnhmFYFRBrK376tZyDtwUM --- .claude/skills/spec-upgrade/SKILL.md | 31 +- README.md | 39 +- src/unstract/api_deployments/__init__.py | 17 +- src/unstract/api_deployments/client.py | 608 +++++++++++------------ tests/test_compat.py | 371 ++++++++++++-- 5 files changed, 722 insertions(+), 344 deletions(-) diff --git a/.claude/skills/spec-upgrade/SKILL.md b/.claude/skills/spec-upgrade/SKILL.md index b3d66d4..e177cb0 100644 --- a/.claude/skills/spec-upgrade/SKILL.md +++ b/.claude/skills/spec-upgrade/SKILL.md @@ -71,6 +71,31 @@ Upstream, the spec is produced by the backend that serves these endpoints the facade is where it becomes public API. Fixes belong here or upstream in the spec, never in the generated tree — regeneration overwrites that wholesale. + A new operation belongs to `APIDeploymentsClient` if it takes a deployment + key, `PlatformKeyClient` if it takes a platform key, otherwise a new subclass + of `_HttpxFacade` — never a free-standing class, or the transport, retry and + exception translation get reimplemented and drift. The method is two lines: + + ```python + def list_widgets(self, org_id: str, *, page: int | None = None) -> dict[str, Any]: + kwargs = list_widgets._get_kwargs(org_id, **{"page": page} if page else {}) + return self._read_or_raise(self._request_with_retry(**kwargs), "list_widgets") + ``` + + - Build from `_get_kwargs`, not `sync_detailed`: the generated + `_parse_response` calls `from_dict` on an error body unguarded, so an + undeclared one raises before the facade sees the status. Omit unset + parameters rather than passing `None` — the builder renders some before it + filters `None` out. Both are private to the generator, so pin them in + `tests/test_compat.py`. + - Send through `_request_with_retry`, and read through `_read_or_raise`, + which checks the status first. + - Return `dict[str, Any]`. The generated models are exported for callers who + want typing; a hand-written `TypedDict` would not survive regeneration. + - A new error type subclasses `UnstractError`. + + `PlatformKeyClient.whoami` is the smallest example in the tree. + 6. **Run the tests:** `uv run pytest tests/`. `tests/test_compat.py` compares this client against the last released one, vendored under `tests/baseline/`. Refresh that baseline only when you mean to move the parity reference point, @@ -87,8 +112,10 @@ fail the same way. If it is red, run step 3 and commit the result. Choose the bump by what changed for callers: **major** when the spec removed or renamed something callers depend on, **minor** for new endpoints or new -behaviour, **patch** for fixes that keep the surface identical. A generated diff -with removals in it is the signal for major — spec upgrades produce those. +behaviour — a new facade method included — **patch** for fixes that keep the +surface identical. A generated diff with removals in it is the signal for major +— spec upgrades produce those. Behaviour the baseline pinned that has moved goes +in `ACCEPTED_DIVERGENCES` in the same commit. Do not touch `__version__` in `src/unstract/api_deployments/__init__.py` in your PR. The in-repo value is the *last released* version; `main.yml` reads it, diff --git a/README.md b/README.md index fc8c92a..8cd5c6c 100644 --- a/README.md +++ b/README.md @@ -113,12 +113,47 @@ client = APIDeploymentsClient( The retry logic uses exponential backoff with full jitter and respects the `Retry-After` header on 429 responses. +## Listing deployments with a platform key + +`PlatformKeyClient` takes a **platform** API key, not a deployment key, and reads +the account that key belongs to. It cannot run a deployment. + +```python +from unstract.api_deployments import PlatformKeyClient + +with PlatformKeyClient("https://us-central.unstract.com", "your_platform_key") as client: + org_id = client.whoami()["organization_id"] + page = client.list_deployments(org_id, page_size=50) + for deployment in page["results"]: + print(deployment["api_name"], deployment["api_endpoint"]) +``` + +Follow `next` for further pages. `api_key` falls back to `$UNSTRACT_PLATFORM_KEY`. + +## Errors + +Every error either client raises derives from `UnstractError`: + +| Exception | Raised by | +|-----------|-----------| +| `UnstractError` | base of both — catch this to catch everything | +| `APIDeploymentError` | `APIDeploymentsClient` | +| `PlatformClientError` | `PlatformKeyClient` | + +`APIDeploymentsClientException` is an alias of `UnstractError`, so existing +`except` clauses keep working. + +Transport failures are raised as the `requests` exception types +(`ConnectionError`, `Timeout`, and friends) rather than the httpx ones. + ## Internals `unstract.api_deployments._sdk_docstudio` is generated from the deployment API's OpenAPI spec by `tools/gen_sdk.sh` and is an implementation detail of the -transport. `APIDeploymentsClient` is the supported surface — import from it, not -from the generated tree, which is regenerated wholesale whenever the spec moves. +transport. `APIDeploymentsClient` and `PlatformKeyClient` are the supported +surface — import from those, or from the response models re-exported alongside +them, not from the generated tree, which is regenerated wholesale whenever the +spec moves. ## Cloning an organization diff --git a/src/unstract/api_deployments/__init__.py b/src/unstract/api_deployments/__init__.py index 6a4c528..b0fe340 100644 --- a/src/unstract/api_deployments/__init__.py +++ b/src/unstract/api_deployments/__init__.py @@ -1,7 +1,22 @@ __version__ = "1.6.0" +from ._sdk_docstudio.models import ( + APIDeploymentSummary as APIDeploymentSummary, +) +from ._sdk_docstudio.models import ( + PaginatedAPIDeploymentSummaryList as PaginatedAPIDeploymentSummaryList, +) +from ._sdk_docstudio.models import ( + WhoAmIResponse as WhoAmIResponse, +) +from .client import APIDeploymentError as APIDeploymentError from .client import APIDeploymentsClient as APIDeploymentsClient -from .client import PlatformAPIClient as PlatformAPIClient +from .client import ( + APIDeploymentsClientException as APIDeploymentsClientException, +) +from .client import PlatformClientError as PlatformClientError +from .client import PlatformKeyClient as PlatformKeyClient +from .client import UnstractError as UnstractError def get_sdk_version(): diff --git a/src/unstract/api_deployments/client.py b/src/unstract/api_deployments/client.py index 0343f62..df57093 100644 --- a/src/unstract/api_deployments/client.py +++ b/src/unstract/api_deployments/client.py @@ -3,14 +3,14 @@ Classes: APIDeploymentsClient: A class to invoke APIs deployed on the Unstract platform. - PlatformAPIClient: A class to read the account a platform API key belongs to + PlatformKeyClient: A class to read the account a platform API key belongs to and the deployments in it. - APIDeploymentsClientException: A class to handle exceptions raised by both - client classes. + UnstractError: Base of the exceptions both clients raise, aliased as + ``APIDeploymentsClientException`` for callers who catch that name. The two clients take different credentials and are not interchangeable. A -deployment key runs one deployment and cannot describe the account; a platform -key describes the account and lists what is in it but cannot run anything. That +deployment key runs deployments and cannot describe the account; a platform key +describes the account and lists what is in it but cannot run anything. That split is the API's, not this module's. """ @@ -20,8 +20,9 @@ import os import threading import time -from typing import Any +from typing import Any, Self from urllib.parse import parse_qs, urljoin, urlparse +from uuid import UUID import attrs import httpx @@ -125,7 +126,7 @@ def _query_value(url: str, key: str) -> str: if not value: # Only the path is reported: the query is the service's to shape, and # the documented usage prints this exception straight to a log. - raise APIDeploymentsClientException( + raise APIDeploymentError( f"No {key} in the query of {parsed.path!r}. The status endpoint the " "service returned carries it; pass that endpoint unmodified." ) @@ -176,28 +177,24 @@ def _error_text(body: Any, response) -> str: value = body.get(key) if isinstance(value, str) and value: return value - # Both facades hand this an httpx response, which has `.text`. The - # `.content` fallback is for the generated `Response` wrapper -- an attrs - # class carrying only bytes -- so passing one here reports the reason - # instead of raising AttributeError on the way to reporting it. - text = getattr(response, "text", None) - if text is None: - text = (getattr(response, "content", b"") or b"").decode("utf-8", "replace") - return (text or "").strip()[:_ERROR_TEXT_LIMIT] + return (response.text or "").strip()[:_ERROR_TEXT_LIMIT] -class APIDeploymentsClientException(Exception): - """A class to handle exceptions raised by the APIClient class.""" +class UnstractError(Exception): + """Base for every error the clients in this package raise.""" - def __init__(self, message): - def __init__(self, value): - self.value = value - def __str__(self): - return repr(self.value) +class APIDeploymentError(UnstractError): + """Raised by :class:`APIDeploymentsClient`.""" - def error_message(self): - return self.value + +class PlatformClientError(UnstractError): + """Raised by :class:`PlatformKeyClient`.""" + + +#: The name this exception shipped under. Aliased to the base, not a leaf, so +#: it keeps catching everything either client raises. +APIDeploymentsClientException = UnstractError class _WaitRetryAfterOrExponentialJitter(wait_base): @@ -246,89 +243,18 @@ def __call__(self, retry_state: RetryCallState) -> float: _STATUS_SEND_ONLY = frozenset({"execution_id", "include_metadata"}) -class APIDeploymentsClient: - """A class to invoke APIs deployed on the Unstract platform.""" - - formatter = logging.Formatter( - "%(asctime)s - %(name)s - %(levelname)s - %(message)s" - ) - logger = logging.getLogger(__name__) - log_stream_handler = logging.StreamHandler() - log_stream_handler.setFormatter(formatter) - logger.addHandler(log_stream_handler) - - api_key = "" - api_timeout = 300 - in_progress_statuses = ["PENDING", "EXECUTING", "READY", "QUEUED", "INITIATED"] - - def __init__( - self, - api_url: str, - api_key: str, - api_timeout: int = 300, - logging_level: str = "INFO", - include_metadata: bool = False, - verify: bool = True, - max_retries: int = 4, - initial_delay: float = 2.0, - max_delay: float = 60.0, - backoff_factor: float = 2.0, - jitter: float = 1.0, - *, - transport_timeout: float | None = None, - ): - """Initializes the APIClient class. - - Args: - api_key (str): The API key to authenticate the API request. - api_timeout (int): Backend execution mode sent with the request — - see ``timeout`` on ``structure_file``. ``0`` or below queues the - execution and returns; above it the call runs synchronously and - the value bounds how long the backend waits. - logging_level (str): The logging level to log messages. - max_retries (int): Maximum number of retry attempts for failed requests. - initial_delay (float): Initial delay in seconds before the first retry. - max_delay (float): Maximum delay in seconds between retries. - backoff_factor (float): Multiplier applied to delay for each retry. - jitter (float): Maximum additive jitter in seconds added to each delay. - transport_timeout (float | None): Socket timeout in seconds. Unset - means a stalled connection blocks forever, which is what the - released client did; ``api_timeout`` cannot serve here because - it is an execution mode, not a socket timeout. - """ - if logging_level == "": - logging_level = os.getenv("UNSTRACT_API_CLIENT_LOGGING_LEVEL", "INFO") - if logging_level == "DEBUG": - self.logger.setLevel(logging.DEBUG) - elif logging_level == "INFO": - self.logger.setLevel(logging.INFO) - elif logging_level == "WARNING": - self.logger.setLevel(logging.WARNING) - elif logging_level == "ERROR": - self.logger.setLevel(logging.ERROR) - - # self.logger.setLevel(logging_level) - self.logger.debug("Logging level set to: " + logging_level) +class _HttpxFacade: + """Transport shared by the clients in this module. - if api_key == "": - self.api_key = os.getenv("UNSTRACT_API_DEPLOYMENT_KEY", "") - else: - self.api_key = api_key - self.logger.debug("API key set to: " + UnstractUtils.redact_key(self.api_key)) + The pooled transport, the retry policy and the translation of httpx + failures into the ``requests`` types callers catch live here, so the + clients cannot drift apart on them. A subclass sets ``base_url``, + ``api_key``, ``verify``, ``transport_timeout`` and the retry knobs in its + own ``__init__``, and ``_error_class`` to the exception it raises. + """ - self.api_timeout = api_timeout - self.api_url = api_url - self.__save_base_url(api_url) - self.include_metadata = include_metadata - self.verify = verify - self.max_retries = max_retries - self.initial_delay = initial_delay - self.max_delay = max_delay - self.backoff_factor = backoff_factor - self.jitter = jitter - self.transport_timeout = transport_timeout - self._transport_client = None - self._transport_lock = threading.Lock() + #: Exception raised for anything this transport reports. Subclasses narrow it. + _error_class: type[UnstractError] = UnstractError def _is_retryable_status(self, status_code: int) -> bool: """Checks whether a status code should trigger a retry. @@ -341,33 +267,19 @@ def _is_retryable_status(self, status_code: int) -> bool: """ return status_code >= 500 or status_code == 429 - def __save_base_url(self, full_url: str): - """Extracts the base URL from the full URL and saves it. - - Args: - full_url (str): The full URL of the API. - """ - parsed_url = urlparse(full_url) - self.base_url = parsed_url.scheme + "://" + parsed_url.netloc - self.logger.debug("Base URL: " + self.base_url) - @property - def _transport(self): - """The HTTP client, built on first use. - - Untimed by default, matching the previous behaviour. ``api_timeout`` is - a backend execution mode (0 selects async execution), never a socket - timeout; feeding it to the transport fails deep in the connection layer - for the negative values the API accepts. ``transport_timeout`` is the - way to bound a stalled connection. + def _transport(self) -> AuthenticatedClient: + """The HTTP client and its connection pool, built on first use. - Built under a lock: two threads racing the first call would otherwise - each build a pool and one would be dropped still holding its sockets. + Both the wrapper and the pool inside it are built under the lock. The + pool is what holds sockets, and ``AuthenticatedClient`` builds it lazily + without synchronising, so two threads racing the first call would + otherwise each build one and drop the loser still holding its sockets. """ if self._transport_client is None: with self._transport_lock: if self._transport_client is None: - self._transport_client = AuthenticatedClient( + transport = AuthenticatedClient( base_url=self.base_url, token=self.api_key, verify_ssl=self.verify, @@ -379,6 +291,8 @@ def _transport(self): # finished-and-empty job. follow_redirects=True, ) + transport.get_httpx_client() + self._transport_client = transport return self._transport_client def close(self) -> None: @@ -394,67 +308,12 @@ def close(self) -> None: if transport is not None: transport.get_httpx_client().close() - def __enter__(self) -> "APIDeploymentsClient": + def __enter__(self) -> Self: return self def __exit__(self, exc_type, exc_value, traceback) -> None: self.close() - @property - def _deployment_route(self) -> tuple[str, str]: - """Organisation and API name, from the deployment URL's last two - segments.""" - segments = urlparse(self.api_url).path.strip("/").split("/") - if len(segments) < 2: - raise APIDeploymentsClientException( - f"Cannot derive organisation and API name from api_url: {self.api_url}" - ) - return segments[-2], segments[-1] - - def _spec_route(self) -> str: - """The path the spec routes a poll to, or ``""`` when the deployment URL - carries no organisation and API name to build one from. - - Built through the generated builder so it follows the spec rather than a - copy of it. - """ - try: - org_name, api_name = self._deployment_route - except APIDeploymentsClientException: - return "" - return status._get_kwargs(org_name, api_name, execution_id="")["url"] - - def _status_url(self, endpoint: str) -> str: - """Absolute URL to poll, under the deployment's own path prefix. - - ``base_url`` is scheme and host only, so a deployment served under a path - prefix would execute -- the execute call sends the caller's URL verbatim - -- and then never poll. The prefix is whatever precedes the spec route - inside the deployment URL. Where the two do not line up there is no - prefix to derive, and the path the service returned is used as it came: - a guessed path polls nothing, and the execution behind it has already - been paid for. - - A deployment URL with no organisation and API name in it -- an ingress - rewrite short enough to have neither -- has no route to line up against - and takes that same branch. The released client polled those, and the - execution has already been submitted by the time this runs. - - Only the path is taken. A scheme and host in the reply would otherwise - decide where the deployment key is sent, and the reply is not the thing - that gets to choose that. - """ - path = self._spec_route() - route = path.rstrip("/") - prefix = urlparse(self.api_url).path.rstrip("/") - if route and prefix.endswith(route): - return self.base_url + prefix[: -len(route)] + path - # Joined rather than concatenated: the query travels as params. - return urljoin( - self.base_url, - urlparse(endpoint)._replace(scheme="", netloc="", query="").geturl(), - ) - def _send(self, method: str, url: str, **kwargs) -> httpx.Response: """Issue one request, translating transport failures on the way out. @@ -470,9 +329,19 @@ def _send(self, method: str, url: str, **kwargs) -> httpx.Response: **(kwargs.get("headers") or {}), "Authorization": f"Bearer {self.api_key}", } - return _translate_transport_errors( - self._transport.get_httpx_client().request, method, url, **kwargs - ) + + def _issue() -> httpx.Response: + client = self._transport.get_httpx_client() + try: + return client.request(method, url, **kwargs) + except RuntimeError as e: + # A close on another thread mid-send. httpx raises a bare + # RuntimeError for it, which the translator does not cover. + if not client.is_closed: + raise + raise ConnectionError(str(e)) from e + + return _translate_transport_errors(_issue) @staticmethod def _read_body(response): @@ -588,6 +457,157 @@ def _retry_error_callback(retry_state: RetryCallState): return retrier(self._send, method, url, **kwargs) + +class APIDeploymentsClient(_HttpxFacade): + """A class to invoke APIs deployed on the Unstract platform.""" + + _error_class = APIDeploymentError + + formatter = logging.Formatter( + "%(asctime)s - %(name)s - %(levelname)s - %(message)s" + ) + logger = logging.getLogger(__name__) + log_stream_handler = logging.StreamHandler() + log_stream_handler.setFormatter(formatter) + logger.addHandler(log_stream_handler) + + api_key = "" + api_timeout = 300 + in_progress_statuses = ["PENDING", "EXECUTING", "READY", "QUEUED", "INITIATED"] + + def __init__( + self, + api_url: str, + api_key: str, + api_timeout: int = 300, + logging_level: str = "INFO", + include_metadata: bool = False, + verify: bool = True, + max_retries: int = 4, + initial_delay: float = 2.0, + max_delay: float = 60.0, + backoff_factor: float = 2.0, + jitter: float = 1.0, + *, + transport_timeout: float | None = None, + ): + """Initializes the APIClient class. + + Args: + api_key (str): The API key to authenticate the API request. + api_timeout (int): Backend execution mode sent with the request — + see ``timeout`` on ``structure_file``. ``0`` or below queues the + execution and returns; above it the call runs synchronously and + the value bounds how long the backend waits. + logging_level (str): The logging level to log messages. + max_retries (int): Maximum number of retry attempts for failed requests. + initial_delay (float): Initial delay in seconds before the first retry. + max_delay (float): Maximum delay in seconds between retries. + backoff_factor (float): Multiplier applied to delay for each retry. + jitter (float): Maximum additive jitter in seconds added to each delay. + transport_timeout (float | None): Socket timeout in seconds. Unset + means a stalled connection blocks forever, which is what the + released client did; ``api_timeout`` cannot serve here because + it is an execution mode, not a socket timeout. + """ + if logging_level == "": + logging_level = os.getenv("UNSTRACT_API_CLIENT_LOGGING_LEVEL", "INFO") + if logging_level == "DEBUG": + self.logger.setLevel(logging.DEBUG) + elif logging_level == "INFO": + self.logger.setLevel(logging.INFO) + elif logging_level == "WARNING": + self.logger.setLevel(logging.WARNING) + elif logging_level == "ERROR": + self.logger.setLevel(logging.ERROR) + + self.logger.debug("Logging level set to: " + logging_level) + + if api_key == "": + self.api_key = os.getenv("UNSTRACT_API_DEPLOYMENT_KEY", "") + else: + self.api_key = api_key + self.logger.debug("API key set to: " + UnstractUtils.redact_key(self.api_key)) + + self.api_timeout = api_timeout + self.api_url = api_url + self.__save_base_url(api_url) + self.include_metadata = include_metadata + self.verify = verify + self.max_retries = max_retries + self.initial_delay = initial_delay + self.max_delay = max_delay + self.backoff_factor = backoff_factor + self.jitter = jitter + self.transport_timeout = transport_timeout + self._transport_client = None + self._transport_lock = threading.Lock() + + def __save_base_url(self, full_url: str): + """Extracts the base URL from the full URL and saves it. + + Args: + full_url (str): The full URL of the API. + """ + parsed_url = urlparse(full_url) + self.base_url = parsed_url.scheme + "://" + parsed_url.netloc + self.logger.debug("Base URL: " + self.base_url) + + @property + def _deployment_route(self) -> tuple[str, str]: + """Organisation and API name, from the deployment URL's last two + segments.""" + segments = urlparse(self.api_url).path.strip("/").split("/") + if len(segments) < 2: + raise APIDeploymentError( + f"Cannot derive organisation and API name from api_url: {self.api_url}" + ) + return segments[-2], segments[-1] + + def _spec_route(self) -> str: + """The path the spec routes a poll to, or ``""`` when the deployment URL + carries no organisation and API name to build one from. + + Built through the generated builder so it follows the spec rather than a + copy of it. + """ + try: + org_name, api_name = self._deployment_route + except APIDeploymentError: + return "" + return status._get_kwargs(org_name, api_name, execution_id="")["url"] + + def _status_url(self, endpoint: str) -> str: + """Absolute URL to poll, under the deployment's own path prefix. + + ``base_url`` is scheme and host only, so a deployment served under a path + prefix would execute -- the execute call sends the caller's URL verbatim + -- and then never poll. The prefix is whatever precedes the spec route + inside the deployment URL. Where the two do not line up there is no + prefix to derive, and the path the service returned is used as it came: + a guessed path polls nothing, and the execution behind it has already + been paid for. + + A deployment URL with no organisation and API name in it -- an ingress + rewrite short enough to have neither -- has no route to line up against + and takes that same branch. The released client polled those, and the + execution has already been submitted by the time this runs. + + Only the path is taken. A scheme and host in the reply would otherwise + decide where the deployment key is sent, and the reply is not the thing + that gets to choose that. + """ + path = self._spec_route() + route = path.rstrip("/") + prefix = urlparse(self.api_url).path.rstrip("/") + if route and prefix.endswith(route): + return self.base_url + prefix[: -len(route)] + path + # Joined rather than concatenated: the query travels as params. + return urljoin( + self.base_url, + urlparse(endpoint)._replace(scheme="", netloc="", query="").geturl(), + ) + def structure_file( self, file_paths: list[str], @@ -687,7 +707,7 @@ def structure_file( if isinstance(e, FileNotFoundError) else "Cannot read file" ) - raise APIDeploymentsClientException(f"{reason}: {e}") from e + raise APIDeploymentError(f"{reason}: {e}") from e body = ExecuteRequest( files=[ @@ -944,7 +964,7 @@ def check_execution_status( return obj_to_return -class PlatformAPIClient: +class PlatformKeyClient(_HttpxFacade): """Read the account a platform API key belongs to, and its deployments. Separate from `APIDeploymentsClient` because the credential and the URL @@ -953,13 +973,13 @@ class PlatformAPIClient: a platform key and address the account. Folding them together would mean a class whose required `api_url` is meaningless for half its methods. - Everything else about the contract is deliberately the same as that class: - the request is issued through `_send`, so transport failures arrive as the - `requests` exception types callers already catch; the body is read as JSON - rather than through the generated response model; the credential is read per - request; and the pooled connections are released by `close`. + The transport, retry policy and error translation are the shared ones in + `_HttpxFacade`, so a caller catching the `requests` exception types or + relying on retries gets the same behaviour from either client. """ + _error_class = PlatformClientError + def __init__( self, base_url: str, @@ -968,30 +988,42 @@ def __init__( verify: bool = True, transport_timeout: float | None = None, logging_level: str = "INFO", + max_retries: int = 4, + initial_delay: float = 2.0, + max_delay: float = 60.0, + backoff_factor: float = 2.0, + jitter: float = 1.0, ) -> None: """ Args: base_url (str): Scheme and host of the Unstract deployment, e.g. - ``https://us-central.unstract.com``. A path is ignored: these - operations carry their own, and the generated builders join them - onto the origin. + ``https://us-central.unstract.com``. A path is ignored -- these + operations carry their own, which httpx resolves against the + origin -- and dropping a non-empty one is logged, because an + install served under a path prefix is unreachable this way. api_key (str | None): Platform API key. Falls back to - ``$UNSTRACT_PLATFORM_KEY``, matching how `APIDeploymentsClient` - falls back for the deployment key. + ``$UNSTRACT_PLATFORM_KEY``. verify (bool): Verify TLS certificates. transport_timeout (float | None): Seconds before a stalled connection is given up on. Unset means no bound. logging_level (str): Level for this client's logger. + max_retries (int): Maximum number of retry attempts for failed requests. + initial_delay (float): Initial delay in seconds before the first retry. + max_delay (float): Maximum delay in seconds between retries. + backoff_factor (float): Multiplier applied to delay for each retry. + jitter (float): Maximum additive jitter in seconds added to each delay. """ - self.logger = logging.getLogger(__name__) + # Its own logger: the module one is shared, so levelling it here would + # re-level a live instance of the sibling client. + self.logger = logging.getLogger(f"{__name__}.PlatformKeyClient") self.logger.setLevel(getattr(logging, logging_level.upper(), logging.INFO)) if api_key is None: self.api_key = os.getenv("UNSTRACT_PLATFORM_KEY", "") else: self.api_key = api_key - if not self.api_key: - raise APIDeploymentsClientException( + if not self.api_key.strip(): + raise PlatformClientError( "A platform API key is required: pass api_key or set " "$UNSTRACT_PLATFORM_KEY." ) @@ -1001,130 +1033,87 @@ def __init__( parsed = urlparse(base_url) if not parsed.scheme or not parsed.netloc: - raise APIDeploymentsClientException( + raise PlatformClientError( f"base_url must include a scheme and host, got {base_url!r}." ) + if parsed.path.strip("/"): + self.logger.warning( + "Ignoring path %r on base_url: these operations carry their own. " + "An install served under a path prefix is not reachable this way.", + parsed.path, + ) self.base_url = parsed.scheme + "://" + parsed.netloc self.verify = verify self.transport_timeout = transport_timeout + self.max_retries = max_retries + self.initial_delay = initial_delay + self.max_delay = max_delay + self.backoff_factor = backoff_factor + self.jitter = jitter self._transport_client: AuthenticatedClient | None = None self._transport_lock = threading.Lock() - @property - def _transport(self) -> AuthenticatedClient: - """The HTTP client, built on first use and under a lock. - - Same reasoning as `APIDeploymentsClient._transport`: two threads racing - the first call would each build a pool and one would be dropped while - still holding its sockets. - - The token given here is not what authenticates a request -- `_send` - sets the header per call -- but `AuthenticatedClient` requires one. - """ - if self._transport_client is None: - with self._transport_lock: - if self._transport_client is None: - self._transport_client = AuthenticatedClient( - base_url=self.base_url, - token=self.api_key, - verify_ssl=self.verify, - timeout=httpx.Timeout(self.transport_timeout), - raise_on_unexpected_status=False, - follow_redirects=True, - ) - return self._transport_client - - def close(self) -> None: - """Release the pooled connections this client holds. - - As on `APIDeploymentsClient`: the transport is kept between calls so - connections are reused, nothing else releases its sockets, and a client - built per job would otherwise accumulate pools. Safe to call twice. - """ - with self._transport_lock: - transport, self._transport_client = self._transport_client, None - if transport is not None: - transport.get_httpx_client().close() - - def __enter__(self) -> "PlatformAPIClient": - return self - - def __exit__(self, exc_type, exc_value, traceback) -> None: - self.close() - - def _send(self, method: str, url: str, **kwargs) -> httpx.Response: - """Issue one request, translating transport failures on the way out. + def _read_or_raise(self, response: httpx.Response, what: str) -> dict[str, Any]: + """The JSON object of a 2xx, or an exception naming why it was refused. - The credential is read per request rather than captured with the - transport, so assigning ``api_key`` takes effect on the next call -- - `AuthenticatedClient` bakes its own header on first use, which would - otherwise pin whatever key the client was built with. + Read directly rather than through the generated model, which is built + only for the statuses the spec declares and raises on any other body. """ - kwargs["headers"] = { - **(kwargs.get("headers") or {}), - "Authorization": f"Bearer {self.api_key}", - } - return _translate_transport_errors( - self._transport.get_httpx_client().request, method, url, **kwargs - ) - - def _read_or_raise(self, response: httpx.Response, what: str) -> Any: - """The JSON body of a 2xx, or an exception naming why it was refused. - - The body is read directly rather than through the generated response - model, for the reason `APIDeploymentsClient._read_body` gives: a model - is built only for the statuses the spec declares, and its `from_dict` - indexes required keys with no default. A gateway answering 401 with HTML, - or the API answering with a DRF-shaped ``{"detail": ...}``, would raise - `JSONDecodeError` or `KeyError` out of the generated parser -- before - this facade ever sees the response -- instead of reporting a refusal. - """ - body = APIDeploymentsClient._read_body(response) + body = self._read_body(response) + self.logger.debug("%s returned %d", what, response.status_code) if not 200 <= response.status_code < 300: - raise APIDeploymentsClientException( + raise self._error_class( f"{what} failed with {response.status_code}: " f"{_error_text(body, response)}" ) if body is None: - raise APIDeploymentsClientException( - f"{what} returned {response.status_code} with a body that is not JSON." + self.logger.error( + "%s returned %d with a body that could not be read as JSON: %s", + what, + response.status_code, + _error_text(None, response), + ) + raise self._error_class( + f"{what} returned {response.status_code} with an unreadable body " + f"(content-type {response.headers.get('content-type')!r}): " + f"{_error_text(None, response)}" + ) + if not isinstance(body, dict): + raise self._error_class( + f"{what} returned {response.status_code} with a JSON " + f"{type(body).__name__} where an object was expected: " + f"{_error_text(None, response)}" ) return body - def whoami(self) -> dict: + def whoami(self) -> dict[str, Any]: """The organisation this key belongs to, and the key's own scope. - The organisation is read from the key row server-side, so this takes no - organisation argument -- resolving one is the point of the call. Use it - to obtain the ``org_id`` that `list_deployments` needs. - Returns: dict: ``organization_id``, ``organization_name``, ``permission`` - and ``key_name``. + and ``key_name``, the shape `WhoAmIResponse` models. Use the + ``organization_id`` as the ``org_id`` other operations take. """ - self.logger.debug("Resolving identity via /unstract/whoami/") - request_kwargs = whoami._get_kwargs() - response = self._send(**request_kwargs) + self.logger.debug("Resolving identity via the whoami operation") + response = self._request_with_retry(**whoami._get_kwargs()) return self._read_or_raise(response, "whoami") def list_deployments( self, org_id: str, *, - api_name: str | Unset = UNSET, - search: str | Unset = UNSET, - ordering: str | Unset = UNSET, - page: int | Unset = UNSET, - page_size: int | Unset = UNSET, - workflow: Any | Unset = UNSET, - ) -> dict: - """The API deployments in one organisation. - - The keyword arguments are the query parameters the endpoint accepts, - named as the API names them; one left unset is not sent, so the server - picks its own default. The result is paginated -- read ``next`` rather - than assuming ``results`` is the whole set. + api_name: str | None = None, + search: str | None = None, + ordering: str | None = None, + page: int | None = None, + page_size: int | None = None, + workflow: UUID | str | None = None, + ) -> dict[str, Any]: + """The API deployments in one organisation, one page at a time. + + A filter left as ``None`` is not sent. Follow ``next`` rather than + assuming ``results`` is the whole set. Args: org_id (str): Organisation to list within. `whoami` resolves this @@ -1134,20 +1123,29 @@ def list_deployments( ordering (str): Field to order by. page (int): 1-based page number. page_size (int): Rows per page. - workflow (UUID): Return only deployments of this workflow. + workflow (UUID | str): Return only deployments of this workflow. Returns: - dict: ``count``, ``next``, ``previous`` and ``results``. + dict: ``count``, ``next``, ``previous`` and ``results``, the shape + `PaginatedAPIDeploymentSummaryList` models. """ - self.logger.debug("Listing deployments for organisation: " + org_id) + if not org_id.strip(): + raise PlatformClientError( + "org_id is required; whoami() resolves it from the key." + ) + self.logger.debug("Listing deployments for organisation: %s", org_id) + # Omitted rather than passed as None: the builder renders some + # parameters before it filters None out, sending the string "None". + filters = { + "api_name": api_name, + "search": search, + "ordering": ordering, + "page": page, + "page_size": page_size, + "workflow": workflow, + } request_kwargs = list_deployments._get_kwargs( - org_id, - api_name=api_name, - search=search, - ordering=ordering, - page=page, - page_size=page_size, - workflow=workflow, + org_id, **{k: v for k, v in filters.items() if v is not None} ) - response = self._send(**request_kwargs) + response = self._request_with_retry(**request_kwargs) return self._read_or_raise(response, "list_deployments") diff --git a/tests/test_compat.py b/tests/test_compat.py index 98ee284..8fe3bf4 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -25,6 +25,7 @@ import inspect import io import json +import logging import os import re import socket @@ -52,14 +53,21 @@ ) from unstract import api_deployments +from unstract.api_deployments import ( + PaginatedAPIDeploymentSummaryList, + WhoAmIResponse, +) from unstract.api_deployments._sdk_docstudio import AuthenticatedClient from unstract.api_deployments._sdk_docstudio.types import UNSET from unstract.api_deployments.client import ( _EXECUTE_SEND_ONLY, _STATUS_SEND_ONLY, + APIDeploymentError, APIDeploymentsClient, APIDeploymentsClientException, - PlatformAPIClient, + PlatformClientError, + PlatformKeyClient, + UnstractError, ) BASELINE_VERSION = "1.5.3" @@ -1896,7 +1904,7 @@ def test_every_declared_operation_is_wrapped(): Two sets, unioned, because the spec now serves two credentials: the deployment-key operations reached through `APIDeploymentsClient` and the - platform-key ones through `PlatformAPIClient`. The union keeps the whole + platform-key ones through `PlatformKeyClient`. The union keeps the whole comparison intact -- an operation belonging to neither still fails here. """ spec = json.loads(SPEC_PATH.read_text()) @@ -1994,11 +2002,46 @@ def _deployment_page() -> dict: } +def _identity_body() -> dict: + """The identity body `whoami` answers with, in the shape the spec declares.""" + return { + "organization_id": "org-a", + "organization_name": "Org A", + "permission": "read", + "key_name": "cli-key", + } + + +@contextmanager +def caplog_at_error(): + """Collect this client's own ERROR records. + + `PlatformKeyClient` configures its logger itself, so the level and handlers + are not the ones `caplog` attaches to the root. + """ + records = [] + + class _Collect(logging.Handler): + def emit(self, record): + records.append(record) + + handler = _Collect() + logger = logging.getLogger(f"{PlatformKeyClient.__module__}.PlatformKeyClient") + logger.addHandler(handler) + try: + yield records + finally: + logger.removeHandler(handler) + + def _platform_client(**kwargs): kwargs.setdefault("base_url", "https://example.unstract.com") kwargs.setdefault("api_key", "pk-test") kwargs.setdefault("logging_level", "ERROR") - return PlatformAPIClient(**kwargs) + # Retries off by default: the error statuses exercised below include ones + # the client retries, and the backoff is real time. + kwargs.setdefault("max_retries", 0) + return PlatformKeyClient(**kwargs) @contextmanager @@ -2010,7 +2053,7 @@ def _platform_reply(status_code, json_data): real and observable on `transport.request`. It does **not** exercise the generated `_parse_response` or the response - models: `PlatformAPIClient` reads the body itself, deliberately, because + models: `PlatformKeyClient` reads the body itself, deliberately, because those parsers raise on an error body the spec did not declare. Their coverage is `test_the_generated_platform_models_read_the_bodies_the_server_sends` and `test_the_generated_platform_parsers_read_a_declared_response`. @@ -2021,6 +2064,20 @@ def _platform_reply(status_code, json_data): yield transport +#: How each platform operation is reached on the facade. Kept beside the +#: manifest and asserted against it: dispatching on a name meant an operation +#: with no method behind it could be listed and silently never called. +PLATFORM_CALLS = { + "whoami": lambda client: client.whoami(), + "list_deployments": lambda client: client.list_deployments("org-a"), +} + + +def test_every_platform_operation_has_a_method_behind_it(): + assert set(PLATFORM_CALLS) == PLATFORM_OPERATIONS + assert not (WRAPPED_OPERATIONS & PLATFORM_OPERATIONS) + + @pytest.mark.parametrize("operation", sorted(PLATFORM_OPERATIONS)) def test_the_platform_operations_declare_the_statuses_pinned_here(operation): """The spec is the source; this manifest is the pin. A status the spec adds @@ -2042,10 +2099,7 @@ def test_every_platform_error_status_is_reported_with_its_reason(operation): body, expected = _body_for(schema) if schema else (None, "") with _platform_reply(status_code, body): with pytest.raises(APIDeploymentsClientException) as caught: - if operation == "whoami": - _platform_client().whoami() - else: - _platform_client().list_deployments("org-a") + PLATFORM_CALLS[operation](_platform_client()) message = str(caught.value) assert str(status_code) in message, (operation, status_code) if expected: @@ -2055,16 +2109,10 @@ def test_every_platform_error_status_is_reported_with_its_reason(operation): def test_whoami_returns_the_four_fields_the_spec_declares(): """The organisation is read from the key server-side, so this is the call that turns a bare key into the `org_id` every other operation needs.""" - identity = { - "organization_id": "org-a", - "organization_name": "Org A", - "permission": "read", - "key_name": "cli-key", - } - with _platform_reply(200, identity) as transport: + with _platform_reply(200, _identity_body()) as transport: result = _platform_client().whoami() - assert result == identity + assert result == _identity_body() # Issued through `_send`, which passes method and url positionally the way # the sibling client does. method, url = transport.request.call_args.args[:2] @@ -2091,13 +2139,13 @@ def test_a_missing_platform_key_is_refused_at_construction(): """Rather than at the first call, where it would look like a server refusal.""" with patch.dict(os.environ, {}, clear=True): with pytest.raises(APIDeploymentsClientException) as caught: - PlatformAPIClient(base_url="https://example.unstract.com") + PlatformKeyClient(base_url="https://example.unstract.com") assert "UNSTRACT_PLATFORM_KEY" in str(caught.value) def test_the_platform_key_is_taken_from_the_environment_when_unset(): with patch.dict(os.environ, {"UNSTRACT_PLATFORM_KEY": "pk-from-env"}, clear=True): - client = PlatformAPIClient(base_url="https://example.unstract.com") + client = PlatformKeyClient(base_url="https://example.unstract.com") assert client.api_key == "pk-from-env" @@ -2106,19 +2154,6 @@ def test_a_base_url_without_a_host_is_refused(): _platform_client(base_url="not-a-url") -def test_a_path_on_the_base_url_is_discarded(): - """These operations carry their own paths; keeping a caller's would produce - a URL no deployment serves.""" - client = _platform_client( - base_url="https://example.unstract.com/deployment/api/x/y/" - ) - assert client.base_url == "https://example.unstract.com" - - -# The findings below were raised on PR #29 and each fix is pinned here, so a -# regression to the pre-review behaviour fails rather than passing quietly. - - @pytest.mark.parametrize( ("label", "body_text"), [ @@ -2208,12 +2243,19 @@ def test_both_clients_are_reachable_from_the_package_root(): surface, and the sibling is re-exported.""" from unstract import api_deployments as pkg - assert pkg.PlatformAPIClient is PlatformAPIClient + assert pkg.PlatformKeyClient is PlatformKeyClient assert pkg.APIDeploymentsClient is APIDeploymentsClient + # The models these operations answer with, so a caller can type a response + # without importing from the generated tree. + assert pkg.WhoAmIResponse is WhoAmIResponse + assert pkg.PaginatedAPIDeploymentSummaryList is PaginatedAPIDeploymentSummaryList + assert issubclass(pkg.APIDeploymentError, pkg.UnstractError) + assert issubclass(pkg.PlatformClientError, pkg.UnstractError) + def test_the_generated_platform_models_read_the_bodies_the_server_sends(): - """`PlatformAPIClient` reads bodies itself, so nothing else here would + """`PlatformKeyClient` reads bodies itself, so nothing else here would notice a generated model that silently lost a field. They are still public surface for anyone importing them, and the parity tests above cover only the deployment models. @@ -2307,7 +2349,7 @@ def test_the_generated_platform_parsers_read_a_declared_response(): def test_the_generated_parsers_raise_on_an_undeclared_error_body(): - """The reason `PlatformAPIClient` does not use them. `from_dict` indexes + """The reason `PlatformKeyClient` does not use them. `from_dict` indexes required keys with no default and `response.json()` is unguarded, so a gateway's HTML 401 or a DRF-shaped body reaches a caller of `sync_detailed` as an exception rather than a refusal. Pinned so the facade's decision to @@ -2325,3 +2367,264 @@ def test_the_generated_parsers_raise_on_an_undeclared_error_body(): whoami._parse_response( client=client, response=_httpx_response(401, {"detail": "Invalid token."}) ) + + +def test_the_generated_request_builders_still_return_what_the_facade_splats(): + """The facade builds requests from the generated `_get_kwargs`, which is + private to the generator. A generator upgrade that renames it or changes + the keys it returns has to fail here rather than at a customer's call.""" + from unstract.api_deployments._sdk_docstudio.api.deployment import ( + list_deployments as list_deployments_op, + ) + from unstract.api_deployments._sdk_docstudio.api.identity import ( + whoami as whoami_op, + ) + + identity_kwargs = whoami_op._get_kwargs() + assert identity_kwargs["method"] == "get" + assert identity_kwargs["url"] == "/api/v1/unstract/whoami/" + + listing_kwargs = list_deployments_op._get_kwargs("org-a", api_name="x") + assert listing_kwargs["method"] == "get" + assert "/org-a/" in listing_kwargs["url"] + assert listing_kwargs["params"] == {"api_name": "x"} + # Unset parameters are dropped rather than sent as a sentinel. + assert list_deployments_op._get_kwargs("org-a")["params"] == {} + + +def test_a_platform_request_carries_the_current_key_over_a_real_transport(): + """Asserted on the request as httpx composed it, not on a mock's recorded + kwargs: `AuthenticatedClient` bakes a header of its own at construction, so + which one wins is httpx's merge behaviour rather than this client's.""" + seen = [] + + def handler(request): + seen.append(request) + return httpx.Response(200, json=_identity_body()) + + client = _platform_client() + transport = client._transport + transport.get_httpx_client()._transport = httpx.MockTransport(handler) + + client.whoami() + assert seen[-1].headers["Authorization"] == "Bearer pk-test" + + client.api_key = "pk-rotated" + client.whoami() + assert seen[-1].headers["Authorization"] == "Bearer pk-rotated" + assert str(seen[-1].url).endswith("/api/v1/unstract/whoami/") + + +def test_closing_the_platform_client_releases_the_pool_and_the_next_call_rebuilds(): + """The mock-based check cannot see a pool that was dropped rather than + closed, nor that the rebuild path still works.""" + client = _platform_client() + httpx_client = client._transport.get_httpx_client() + client.close() + assert httpx_client.is_closed + assert client._transport_client is None + client.close() + assert client._transport.get_httpx_client() is not httpx_client + client.close() + + +def test_the_platform_transport_is_built_once_under_contention(): + """Both the wrapper and the pool inside it are built under the lock; a pool + built twice leaves one holding sockets that nothing closes.""" + client = _platform_client() + barrier = threading.Barrier(8) + seen = [] + + def build(): + barrier.wait() + seen.append(client._transport) + + threads = [threading.Thread(target=build) for _ in range(8)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert len({id(transport) for transport in seen}) == 1 + client.close() + + +def test_the_platform_client_retries_a_retryable_status(): + """Two idempotent GETs, and the README promises those are always retried. + Without this the sibling retries and this client does not.""" + replies = [ + _httpx_response(503, None), + _httpx_response(503, None), + _httpx_response(200, _identity_body()), + ] + transport = MagicMock() + transport.request.side_effect = replies + with patch.object(AuthenticatedClient, "get_httpx_client", return_value=transport): + result = _platform_client(max_retries=2, initial_delay=0.01).whoami() + + assert result == _identity_body() + assert transport.request.call_count == 3 + + +def test_the_platform_transport_uses_the_settings_it_was_given(): + """A dropped `verify_ssl` disables certificate checking silently.""" + client = _platform_client(transport_timeout=7.5, verify=False) + httpx_client = client._transport.get_httpx_client() + assert httpx_client.timeout.connect == 7.5 + assert client._transport_client._verify_ssl is False + client.close() + + +def test_a_json_body_that_is_not_an_object_is_refused(): + """Nothing else checks the shape, so a bare list would reach the caller as + a `dict` and fail on their first subscript instead of here.""" + with _platform_reply(200, [{"id": 1}]): + with pytest.raises(APIDeploymentsClientException) as caught: + _platform_client().list_deployments("org-a") + assert "list" in str(caught.value) + + +def test_a_2xx_body_that_is_not_json_reports_what_arrived(): + """An SSO or maintenance page answering 200 is the everyday cause, and the + status alone does not distinguish it from a wrong host.""" + response = httpx.Response( + 200, + text="login", + headers={"content-type": "text/html"}, + request=httpx.Request("GET", "https://example.unstract.com/"), + ) + transport = MagicMock() + transport.request.return_value = response + with patch.object(AuthenticatedClient, "get_httpx_client", return_value=transport): + with pytest.raises(APIDeploymentsClientException) as caught: + _platform_client().whoami() + message = str(caught.value) + assert "text/html" in message + assert "login" in message + + +def test_an_unfiltered_listing_sends_no_filters_at_all(): + """The builder renders `workflow` with `str()` before it drops the + parameters that are None, so a None default reaches the server as the + literal string "None" -- a filter matching no workflow, on every otherwise + unfiltered call.""" + with _platform_reply(200, _deployment_page()) as transport: + _platform_client().list_deployments("org-a") + + assert transport.request.call_args.kwargs["params"] == {} + + with _platform_reply(200, _deployment_page()) as transport: + _platform_client().list_deployments( + "org-a", workflow="22222222-2222-2222-2222-222222222222" + ) + + params = transport.request.call_args.kwargs["params"] + assert params == {"workflow": "22222222-2222-2222-2222-222222222222"} + + +def test_a_close_during_a_call_arrives_as_the_documented_exception(): + """httpx answers a send on a closed client with a bare RuntimeError, which + is not in the subtree the translator covers. A caller catching the + documented `requests` classes would not catch it.""" + client = _platform_client() + httpx_client = client._transport.get_httpx_client() + + def close_then_send(*args, **kwargs): + httpx_client.close() + return original(*args, **kwargs) + + original = httpx_client.request + with patch.object(httpx_client, "request", side_effect=close_then_send): + with pytest.raises(ConnectionError): + client.whoami() + + +def test_an_unreadable_body_is_bounded_in_the_log_as_well_as_the_error(): + """The error truncates it and the log did not, so the default level was the + wider disclosure of the two.""" + body = "x" * 4000 + response = httpx.Response( + 200, + text=body, + headers={"content-type": "text/plain"}, + request=httpx.Request("GET", "https://example.unstract.com/"), + ) + transport = MagicMock() + transport.request.return_value = response + with caplog_at_error() as records: + with patch.object( + AuthenticatedClient, "get_httpx_client", return_value=transport + ): + with pytest.raises(APIDeploymentsClientException): + _platform_client().whoami() + + logged = "".join(record.getMessage() for record in records) + assert "x" in logged + assert len(logged) < len(body) + + +def test_a_whitespace_organisation_is_refused_before_the_request(): + """It is not empty, so the emptiness check passed it, and `quote` then + encoded it into the path as %20 segments.""" + with pytest.raises(APIDeploymentsClientException) as caught: + _platform_client().list_deployments(" ") + assert "whoami()" in str(caught.value) + + +def test_an_empty_organisation_is_refused_before_the_request(): + """An empty segment builds a path the router answers for something else.""" + with pytest.raises(APIDeploymentsClientException) as caught: + _platform_client().list_deployments("") + assert "whoami()" in str(caught.value) + + +def test_a_path_on_the_base_url_is_discarded_and_the_drop_is_reported(caplog): + """Discarding it is deliberate -- these operations carry their own paths, and + a pasted deployment URL would otherwise build one no deployment serves. But + an install served under a path prefix becomes unreachable that way, and a + bare 404 from the proxy does not say so.""" + client = _platform_client( + base_url="https://example.unstract.com/deployment/api/x/y/" + ) + assert client.base_url == "https://example.unstract.com" + + with caplog.at_level(logging.WARNING): + _platform_client( + base_url="https://internal.corp/unstract/", logging_level="WARNING" + ) + assert "/unstract/" in caplog.text + + +def test_the_published_exception_name_still_catches_both_clients(): + """It is the name callers already catch, so it has to stay the widest one.""" + assert APIDeploymentsClientException is UnstractError + assert issubclass(APIDeploymentError, UnstractError) + assert issubclass(PlatformClientError, UnstractError) + + with _platform_reply(401, {"message": "bad key"}): + with pytest.raises(APIDeploymentsClientException): + _platform_client().whoami() + + with pytest.raises(APIDeploymentsClientException): + _client(api_url="https://example.com/").check_execution_status("") + + +def test_the_exception_carries_its_message(): + """The released class accepted a message and dropped it, leaving `str()` + working only through `BaseException.args`.""" + error = UnstractError("something went wrong") + assert str(error) == "something went wrong" + assert error.args == ("something went wrong",) + + +def test_the_two_clients_do_not_share_a_logger(): + """`APIDeploymentsClient.logger` is the module logger. Levelling that one + from here would re-level a live instance of the sibling, turning its debug + output -- which includes response bodies -- on or off as a side effect.""" + deployment = _client(logging_level="DEBUG") + assert deployment.logger.level == logging.DEBUG + + platform = _platform_client(logging_level="ERROR") + assert platform.logger is not deployment.logger + assert deployment.logger.level == logging.DEBUG + assert platform.logger.level == logging.ERROR