feat(client): implement ShadeClient for per-instance configuration (#2) - #55
Conversation
Implements issue ShadeProtocol#2. ShadeClient binds credentials and connection settings to a single object so a multi-tenant application can hold one client per merchant instead of mutating the global shade module config. - ShadeClient takes api_key, environment, api_base, timeout and max_retries, each falling back to the matching global setting when omitted. The fallback resolves once at construction, so later global changes never mutate an existing client. - Adds a global shade.api_key setting backing that fallback. - ShadeClient.from_env() builds a client from SHADE_API_KEY and SHADE_ENVIRONMENT, with keyword arguments overriding either. - A missing api_key with no global key set now raises AuthenticationError instead of ValueError, naming all three ways to supply one. - BaseResource gives resources the optional client= kwarg, resolving to the shared global client when omitted. The client is resolved per access, so a resource built before shade.api_key was assigned still picks it up. ShadeClient was previously an alias for Gateway, with a separate unrelated ShadeClient in client.py. Gateway is now a ShadeClient subclass carrying the payment methods, and the httpx-backed transport that occupied client.py moves to http.py as HTTPXTransport, alongside the other transports. The two tests asserting the old alias now assert the subclass relationship. Also fixes http.py resolving `from . import config` to the config module rather than the Config instance. That only worked because of the order of imports in __init__.py, and broke as soon as client.py imported http.py earlier in the chain.
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hello @daveades |
Reconciles ShadeClient with the thread-local global config merged in ShadeProtocol#53. Resolutions: - config.py: take main's thread-safe Config and get_config wholesale; the api_key global this branch added is already there. Only the missing-key message changes, to name all three ways to supply a key. - client.py: ShadeClient keeps its per-instance role but adopts main's lazy resolution. Explicit arguments are pinned to the instance; omitted ones resolve against the global config per request, so a missing key surfaces as AuthenticationError at request time rather than at construction. Gains main's api_key/environment setters, which propagate to the sub-clients. - gateway.py: Gateway stays a ShadeClient subclass, dropping the constructor and accessors now inherited. Keeps main's positional parameter order. - http.py: take main's dynamic SyncHTTPClient/AsyncHTTPClient; the httpx transport this branch moved out of client.py lands as HTTPXTransport and resolves through get_config like main's version did. - __init__.py: drop the "ShadeClient = Gateway" alias, since ShadeClient is now a real class, and keep main's other exports. Tests asserting construction-time snapshotting are rewritten for the lazy semantics. 353 passed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/shade/resources/base.py (1)
36-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResource layer reaches into
ShadeClient's private_http/_async_httpattributes.Works correctly per the tests, but crosses a module boundary through underscore-prefixed attributes rather than a public accessor on
ShadeClient. Consider exposing a small public method (e.g.ShadeClient.request_json(...)/request_json_async(...)) thatBaseResourcecalls instead, so the transport internals stay encapsulated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shade/resources/base.py` around lines 36 - 52, Expose public synchronous and asynchronous JSON request methods on ShadeClient, such as request_json and request_json_async, that delegate to the existing HTTP transports. Update BaseResource._request and _request_async to call these public methods instead of accessing client._http or client._async_http, preserving the current arguments and return behavior.src/shade/client.py (1)
179-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBase-url resolution logic duplicated across three classes.
_base_urlreimplements the same "instance override → globalapi_base→ environment default" fallback chain that also exists inSyncHTTPClient.base_url,AsyncHTTPClient.base_url, andget_config(). Divergence risk if one copy is updated without the others.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shade/client.py` around lines 179 - 191, Centralize the “instance override → global api_base → environment default” resolution currently implemented by _base_url, SyncHTTPClient.base_url, AsyncHTTPClient.base_url, and get_config(). Update these consumers to reuse one shared resolver while preserving trailing-slash normalization and the existing api_base property behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shade/client.py`:
- Around line 99-119: Propagate the client’s debug setting into the resource
request path so the documented logging behavior applies consistently. Update the
SyncHTTPClient and AsyncHTTPClient construction in ShadeClient to support and
receive debug, implementing or reusing the same _should_debug, log_request, and
log_response behavior used by HTTPXTransport; ensure the shade.config.debug
fallback remains effective for resource-backed calls.
- Around line 238-250: Synchronize the first-time initialization in
default_client so concurrent callers cannot both construct ShadeClient
instances. Add or reuse a module-level initialization lock around the
_default_client None check and assignment, while keeping the existing
cached-client behavior for subsequent calls and ensuring only the stored client
owns its httpx.Client.
In `@src/shade/http.py`:
- Around line 399-465: Update HTTPXTransport.__init__ and ShadeClient’s
transport construction to accept and forward the resolved timeout and
max_retries values into the HTTPX client. Modify HTTPXTransport.request to apply
the configured timeout and retry failed requests using the existing
SyncHTTPClient/AsyncHTTPClient retry behavior, while preserving request/response
logging and returning the final response.
---
Nitpick comments:
In `@src/shade/client.py`:
- Around line 179-191: Centralize the “instance override → global api_base →
environment default” resolution currently implemented by _base_url,
SyncHTTPClient.base_url, AsyncHTTPClient.base_url, and get_config(). Update
these consumers to reuse one shared resolver while preserving trailing-slash
normalization and the existing api_base property behavior.
In `@src/shade/resources/base.py`:
- Around line 36-52: Expose public synchronous and asynchronous JSON request
methods on ShadeClient, such as request_json and request_json_async, that
delegate to the existing HTTP transports. Update BaseResource._request and
_request_async to call these public methods instead of accessing client._http or
client._async_http, preserving the current arguments and return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a2608d09-8978-4ec1-9540-a33824031c4b
📒 Files selected for processing (10)
src/shade/__init__.pysrc/shade/client.pysrc/shade/config.pysrc/shade/gateway.pysrc/shade/http.pysrc/shade/resources/__init__.pysrc/shade/resources/base.pytests/test_api_base.pytests/test_client_settings.pytests/test_shade_client.py
| class HTTPXTransport: | ||
| """httpx-backed transport returning raw responses, with debug logging. | ||
|
|
||
| Used by :class:`~shade.client.ShadeClient` for calls that need the whole | ||
| response (headers, streaming, non-JSON bodies) rather than a decoded body. | ||
| Credentials and the target host are resolved per request, so a client left | ||
| on the global defaults follows later changes to ``shade.api_key`` and | ||
| friends. Logging is enabled per-instance via ``debug`` or globally via | ||
| ``shade.config.debug``, and the ``Authorization`` header is masked either way. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| api_key: Optional[str] = None, | ||
| base_url: Optional[str] = None, | ||
| environment: Optional[Environment | str] = None, | ||
| debug: bool = False, | ||
| http_client: Optional["httpx.Client"] = None, | ||
| ) -> None: | ||
| self.api_key = api_key | ||
| self._base_url = base_url.rstrip("/") if base_url else None | ||
| self.environment = environment | ||
| self.debug = debug | ||
| self._http = http_client or httpx.Client() | ||
| self._owns_http_client = http_client is None | ||
|
|
||
| def close(self) -> None: | ||
| if self._owns_http_client: | ||
| self._http.close() | ||
|
|
||
| def _should_debug(self) -> bool: | ||
| return self.debug or _config.debug | ||
|
|
||
| def request( | ||
| self, | ||
| method: str, | ||
| path: str, | ||
| *, | ||
| headers: Optional[Mapping[str, str]] = None, | ||
| json: Any = None, | ||
| content: Optional[bytes] = None, | ||
| ) -> "httpx.Response": | ||
| cfg = get_config( | ||
| api_key=self.api_key, | ||
| environment=self.environment, | ||
| api_base=self._base_url, | ||
| ) | ||
|
|
||
| normalized_path = path if path.startswith("/") else f"/{path}" | ||
| url = f"{cfg.base_url}{normalized_path}" | ||
| request_headers = {"Authorization": f"Bearer {cfg.api_key}", **(headers or {})} | ||
|
|
||
| if self._should_debug(): | ||
| log_request(method, url, request_headers, content if content is not None else json) | ||
|
|
||
| response = self._http.request( | ||
| method, | ||
| url, | ||
| headers=request_headers, | ||
| json=json, | ||
| content=content, | ||
| ) | ||
|
|
||
| if self._should_debug(): | ||
| log_response(response.status_code, response.headers, response.text) | ||
|
|
||
| return response |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does httpx.Client() apply a default 5 second timeout when no timeout argument is passed to the constructor?
💡 Result:
Yes, httpx.Client applies a default 5-second timeout when no timeout argument is passed to the constructor [1][2]. The HTTPX library is designed to enforce timeouts by default for all network operations to prevent requests from hanging indefinitely [3]. When you initialize a client without a specific timeout argument, it defaults to a configuration equivalent to httpx.Timeout(5.0) [1][4][2]. This default timeout applies to all requests made with that client instance unless explicitly overridden on a per-request basis [1].
Citations:
- 1: https://www.python-httpx.org/advanced/timeouts/
- 2: https://www.python-httpx.org/api/
- 3: https://www.python-httpx.org/quickstart/
- 4: httpx.Timeout must include a default encode/httpx#1085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)(client|http)\.py$' || true
echo
echo "http.py outline:"
ast-grep outline src/shade/http.py --view compact || true
echo
echo "client.py outline:"
ast-grep outline src/shade/client.py --view compact || true
echo
echo "Relevant http.py lines 350-475:"
sed -n '350,475p' src/shade/http.py | nl -ba -v350
echo
echo "Relevant client.py ShadeClient/HTTPXTransport references:"
rg -n "HTTPXTransport|ShadeClient|timeout|max_retries|_client" src/shade/client.py src/shade/http.pyRepository: ShadeProtocol/shade-python
Length of output: 623
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant http.py lines 350-475:"
sed -n '350,475p' src/shade/http.py
echo
echo "Relevant client.py lines 1-260:"
sed -n '1,260p' src/shade/client.py
echo
echo "Retry/timeout references:"
rg -n "max_retries|timeout|HTTPXTransport|SyncHTTPClient|AsyncHTTPClient|request\(" src/shade/client.py src/shade/http.pyRepository: ShadeProtocol/shade-python
Length of output: 19742
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "SyncHTTPClient request and timeout handling:"
sed -n '476,608p' src/shade/http.py
echo
echo "AsyncHTTPClient timeout setup:"
sed -n '700,750p' src/shade/http.py
echo
echo "Config defaults/types:"
rg -n "timeout|max_retries|validate_client_settings|client_settings|CONFIG" src/shade/config.py src/shade/__init__.py
sed -n '1,220p' src/shade/config.py
sed -n '1,160p' src/shade/__init__.pyRepository: ShadeProtocol/shade-python
Length of output: 21510
Wire timeout and max_retries into HTTPXTransport.
ShadeClient constructs HTTPXTransport without forwarding timeout/max_retries, and HTTPXTransport accepts neither before creating httpx.Client() with httpx’s built-in timeout and making a single self._http.request(...) call. As a result, ShadeClient(request(...), timeout=..., max_retries=...) is silently ignored. Pass the resolved config values into the transport/client and use a retry loop similar to SyncHTTPClient/AsyncHTTPClient.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shade/http.py` around lines 399 - 465, Update HTTPXTransport.__init__ and
ShadeClient’s transport construction to accept and forward the resolved timeout
and max_retries values into the HTTPX client. Modify HTTPXTransport.request to
apply the configured timeout and retry failed requests using the existing
SyncHTTPClient/AsyncHTTPClient retry behavior, while preserving request/response
logging and returning the final response.
…ransport Two findings from review on the merge: default_client() had an unsynchronized check-then-set, so concurrent first calls could each build a client and leak the loser's httpx connection pool. Guard it with a lock, and close the client that reset_default_client() drops. HTTPXTransport created its httpx client without a timeout, so ShadeClient's timeout was silently ignored for requests going through it -- httpx applied its own 5s default instead. Resolve the timeout with the rest of the config and pass it per request. Wiring max_retries into the transport needs a retry loop of its own and is left alone, as is debug reaching SyncHTTPClient/AsyncHTTPClient; both gaps predate this branch.
|
@codebestia |
codebestia
left a comment
codebestia
left a comment
There was a problem hiding this comment.
LGTM!
Thank you for your contribution
2 participants
Description
Implements
ShadeClientas a real per-instance configuration object, so a multi-tenant application (a SaaS acting for many merchants) can hold one client per tenant instead of mutating the globalshademodule config.ShadeClient(src/shade/client.py) takesapi_key,environment,api_base,timeoutandmax_retries, each falling back to the matching global setting when omitted. The fallback resolves once at construction, so assigningshade.timeoutlater never mutates a client that already exists.shade.api_keyis added as a global setting to back that fallback, alongside the existingshade.api_base/shade.timeout/shade.max_retries.ShadeClient.from_env()builds a client fromSHADE_API_KEYandSHADE_ENVIRONMENT. Either variable may be absent, in which case the normal global fallback applies. Keyword arguments override the environment, soShadeClient.from_env(timeout=5.0)takes the key from the environment and sets the rest explicitly.Missing credentials now raise
AuthenticationErrorrather thanValueError, and the message names all three ways to supply a key:BaseResource(src/shade/resources/base.py) gives every resource the optionalclient=kwarg, resolving to a shared client built from the global config when omitted. That shared client is cached but rebuilt whenever a global setting changes, and it is resolved on each access rather than captured at construction — so a resource built beforeshade.api_keywas assigned still picks it up.Restructuring
ShadeClientwas previously an alias forGateway(ShadeClient = Gatewayin__init__.py), whileclient.pyheld a different, unrelatedShadeClientwrapping httpx. That placeholder is what this issue replaces:Gatewayis now aShadeClientsubclass carrying the payment methods. Every existingGateway(...)call keeps working unchanged.client.pymoves tohttp.pyasHTTPXTransport, next toSyncHTTPClientandAsyncHTTPClient, leavingclient.pyforShadeClientas the issue describes.ShadeClient is Gatewaynow assertissubclass(Gateway, ShadeClient).Incidental fix
http.pyusedfrom . import config as _configand then read_config.max_retriesoff it. That resolved to theConfiginstance only because__init__.pyhappened to rebind theshade.configpackage attribute before importinghttp. It broke the momentclient.pyimportedhttpearlier in the chain. Now it imports the instance explicitly (from .config import config as _config).Fixes #2
Type of change
How Has This Been Tested?
34 new tests in
tests/test_shade_client.py, with an autouse fixture isolating each test from global-config and environment-variable leakage.__repr__masking the keyAuthorizationheaderclient=used over global config, fallback when omitted, two resources on different clients not interfering, a global key set after construction still picked up,AuthenticationErrorwhen neither is availabledefault_client(): cached between calls, rebuilt when a global setting changes, raises without a global keyfrom_env(): reads both variables, invalidSHADE_ENVIRONMENTrejected, keyword overrides win, global fallback when unset, raises when nothing is set, empty variable treated as absentshade.api_key: defaults, assignment, config write-through, and use by clients built without a keyFull suite: 285 passed, 3 skipped, no regressions. Both CI flake8 gates clean on the new code.
Checklist:
Note on scope
The proposed step "each resource class should accept an optional
client=kwarg" has no resource classes to apply to yet —shade/resources/did not exist. This PR adds the package andBaseResource, which establishes that contract; the resource modules themselves (Payments, Invoices, …) are separate issues. The acceptance criterion about resource calls is tested against a minimalBaseResourcesubclass standing in for a real resource.Summary by CodeRabbit
New Features
Bug Fixes