-
Notifications
You must be signed in to change notification settings - Fork 1
feat(slack): expose web_client property on SlackAdapter (#98) #127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
d0692a4
a7f5b53
dcb798a
41c8f31
e8a7dae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| import os | ||
| import re | ||
| import time | ||
| import warnings | ||
| from collections import OrderedDict | ||
| from collections.abc import AsyncIterable, Awaitable, Callable | ||
| from contextvars import ContextVar | ||
|
|
@@ -466,6 +467,16 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: | |
| self._client_cache: OrderedDict[str, Any] = OrderedDict() | ||
| self._client_cache_max = config.client_cache_max if config.client_cache_max is not None else 100 | ||
|
|
||
| # Cache of synchronous slack_sdk.WebClient instances keyed by bot | ||
| # token, backing the public ``web_client`` property (the direct port | ||
| # of upstream's ``getClientForToken``). Kept separate from | ||
| # ``_client_cache`` because that one holds async ``AsyncWebClient`` | ||
| # instances used by the adapter's own API calls; the two client types | ||
| # are not interchangeable. Mirrors upstream's plain (unbounded) Map — | ||
| # one entry per distinct token — since callers reach for this escape | ||
| # hatch rarely and tokens are low-cardinality. | ||
| self._web_client_cache: dict[str, Any] = {} | ||
|
|
||
| # Multi-workspace OAuth fields. | ||
| # ``is not None`` (not truthiness) so an explicit empty-string user | ||
| # config does not silently fall back to env (hazard #1). Empty env | ||
|
|
@@ -590,6 +601,61 @@ def current_client(self) -> Any: | |
| """ | ||
| return self._get_client() | ||
|
|
||
| @property | ||
| def web_client(self) -> Any: | ||
| """Direct access to a synchronous ``slack_sdk.WebClient``. | ||
|
|
||
| Bound to the bot token for the current request context | ||
| (multi-workspace) or the configured default token | ||
| (single-workspace). Use for any Slack Web API call not covered by | ||
| the adapter's high-level methods — e.g. | ||
| ``adapter.web_client.pins_add(...)`` or | ||
| ``adapter.web_client.usergroups_list(...)``. | ||
|
|
||
| Resolution order (the standard 3-level resolver): | ||
|
|
||
| 1. Token from the current request context (set during webhook | ||
| handling, or by :meth:`with_bot_token` / :meth:`with_bot_token_async`). | ||
| 2. The default bot token, when configured as a static string or | ||
| already-resolved value. | ||
| 3. Otherwise raise :class:`AuthenticationError`. | ||
|
|
||
| Raises :class:`AuthenticationError` if neither is available — | ||
| typical causes are accessing ``web_client`` outside any | ||
| webhook / :meth:`with_bot_token` context in multi-workspace mode, | ||
| or having configured ``bot_token`` as an async resolver that has | ||
| not run yet. In the latter case await | ||
| :meth:`current_token_async` (or process the work inside the | ||
| webhook flow) so the resolver primes the token first. | ||
|
|
||
| Return type is ``Any`` (rather than the concrete ``WebClient``) | ||
| because ``slack_sdk`` is an optional dependency — consumers who do | ||
| not install the ``slack`` extra should not pay an import cost. | ||
|
|
||
| This is the direct port of upstream's ``adapter.webClient`` getter | ||
| (vercel/chat ``2f108bd``). Unlike :attr:`current_client` it returns | ||
| the *synchronous* ``WebClient`` (the analog of the single TS | ||
| ``WebClient``), so its methods are not awaitables. | ||
| """ | ||
| return self._get_web_client_for_token(self._get_token()) | ||
|
|
||
| @property | ||
| def client(self) -> Any: | ||
| """Deprecated alias for :attr:`web_client`. | ||
|
|
||
| .. deprecated:: | ||
| Use :attr:`web_client` instead. This alias mirrors upstream's | ||
| pre-rename ``adapter.client`` (vercel/chat ``8366b8b``) and is | ||
| kept for one release for backwards compatibility; it will be | ||
| removed in a future version. Emits :class:`DeprecationWarning`. | ||
| """ | ||
| warnings.warn( | ||
| "SlackAdapter.client is deprecated; use SlackAdapter.web_client instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return self.web_client | ||
|
|
||
| # ------------------------------------------------------------------ | ||
| # Token management | ||
| # ------------------------------------------------------------------ | ||
|
|
@@ -742,6 +808,26 @@ def _get_client(self, token: str | None = None) -> Any: | |
| def _invalidate_client(self, token: str) -> None: | ||
| """Remove a cached client (e.g., on token revocation).""" | ||
| self._client_cache.pop(token, None) | ||
| self._web_client_cache.pop(token, None) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| def _get_web_client_for_token(self, token: str) -> Any: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The newly introduced def _invalidate_client(self, token: str) -> None:
"""Remove a cached client (e.g., on token revocation)."""
self._client_cache.pop(token, None)
self._web_client_cache.pop(token, None) self._client_cache.pop(token, None)
self._web_client_cache.pop(token, None) |
||
| """Return a synchronous ``slack_sdk.WebClient`` for *token*, cached. | ||
|
|
||
| Backs the public :attr:`web_client` property and is the direct port | ||
| of upstream's ``getClientForToken`` (vercel/chat ``2f108bd``): one | ||
| cached ``WebClient`` instance per distinct token. The import is | ||
| deferred so ``slack_sdk`` stays an optional dependency (hazard #10). | ||
|
|
||
| Distinct from :meth:`_get_client`, which caches the *async* | ||
| ``AsyncWebClient`` used by the adapter's own API calls. | ||
| """ | ||
| client = self._web_client_cache.get(token) | ||
| if client is None: | ||
| from slack_sdk import WebClient | ||
|
|
||
| client = WebClient(token=token) | ||
| self._web_client_cache[token] = client | ||
| return client | ||
|
|
||
| # ------------------------------------------------------------------ | ||
| # Initialization | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,244 @@ | ||
| """Tests for the public ``SlackAdapter.web_client`` property and ``client`` alias. | ||
|
|
||
| Port of upstream vercel/chat's ``webClient getter`` test block | ||
| (``packages/adapter-slack/src/index.test.ts``), added by commits | ||
| ``8366b8b`` / ``fdebde7`` / ``2f108bd`` (PRs #471 / #476 / #478): the Slack | ||
| adapter exposes a synchronous ``WebClient`` bound to the current | ||
| request-context token (multi-workspace) or the configured default token | ||
| (single-workspace), with a one-release deprecated ``client`` alias. | ||
|
|
||
| ``web_client`` resolves its token via the standard 3-level resolver | ||
| (ContextVar token > static default token > ``AuthenticationError``) and | ||
| caches one ``WebClient`` per distinct token. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import sys | ||
| import warnings | ||
| from types import ModuleType | ||
|
|
||
| import pytest | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Stub slack_sdk so tests run without the real dependency installed. | ||
| # Stub BOTH the sync ``slack_sdk.WebClient`` (backing ``web_client``) and the | ||
| # async ``slack_sdk.web.async_client.AsyncWebClient`` (backing the adapter's | ||
| # own API calls) so the deferred imports inside the adapter resolve here. | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class _FakeWebClient: | ||
| """Minimal stand-in for the synchronous ``slack_sdk.WebClient``.""" | ||
|
|
||
| def __init__(self, *, token: str = "") -> None: | ||
| self.token = token | ||
|
|
||
|
|
||
| class _FakeAsyncWebClient: | ||
| """Minimal stand-in for ``slack_sdk.web.async_client.AsyncWebClient``.""" | ||
|
|
||
| def __init__(self, *, token: str = "") -> None: | ||
| self.token = token | ||
|
|
||
|
|
||
| # Reuse any ``slack_sdk`` stub already registered by a sibling test module | ||
| # (e.g. ``test_slack_client_cache.py``) when collected in the same process — | ||
| # ``setdefault`` would otherwise no-op and leave a stub that lacks the sync | ||
| # ``WebClient`` symbol. Attach the symbols we need either way so the deferred | ||
| # imports inside the adapter resolve regardless of collection order. | ||
| _fake_slack_sdk = sys.modules.setdefault("slack_sdk", ModuleType("slack_sdk")) | ||
| _fake_slack_sdk_web = sys.modules.setdefault("slack_sdk.web", ModuleType("slack_sdk.web")) | ||
| _fake_slack_sdk_web_async = sys.modules.setdefault( | ||
| "slack_sdk.web.async_client", ModuleType("slack_sdk.web.async_client") | ||
| ) | ||
|
|
||
| if not hasattr(_fake_slack_sdk, "WebClient"): | ||
| _fake_slack_sdk.WebClient = _FakeWebClient # type: ignore[attr-defined] | ||
| if not hasattr(_fake_slack_sdk_web_async, "AsyncWebClient"): | ||
| _fake_slack_sdk_web_async.AsyncWebClient = _FakeAsyncWebClient # type: ignore[attr-defined] | ||
| _fake_slack_sdk_web.async_client = _fake_slack_sdk_web_async # type: ignore[attr-defined] | ||
| _fake_slack_sdk.web = _fake_slack_sdk_web # type: ignore[attr-defined] | ||
|
|
||
| from chat_sdk.adapters.slack.adapter import SlackAdapter # noqa: E402, I001 | ||
| from chat_sdk.adapters.slack.types import SlackAdapterConfig # noqa: E402 | ||
| from chat_sdk.shared.errors import AuthenticationError # noqa: E402 | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Helpers | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| _SECRET = "test-signing-secret" | ||
|
|
||
|
|
||
| def _single_workspace_adapter(bot_token: str = "xoxb-static-token") -> SlackAdapter: | ||
| """Single-workspace adapter with a static default bot token.""" | ||
| return SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=bot_token)) | ||
|
|
||
|
|
||
| def _multi_workspace_adapter() -> SlackAdapter: | ||
| """Multi-workspace adapter (no default bot token; tokens are per-team).""" | ||
| return SlackAdapter( | ||
| SlackAdapterConfig( | ||
| signing_secret=_SECRET, | ||
| client_id="test-client-id", | ||
| client_secret="test-client-secret", | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Single-workspace | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestWebClientSingleWorkspace: | ||
| def test_returns_client_bound_to_static_bot_token(self): | ||
| """``web_client`` returns a WebClient bound to the configured token.""" | ||
| adapter = _single_workspace_adapter("xoxb-static-token") | ||
|
|
||
| web_client = adapter.web_client | ||
|
|
||
| # Constructed via ``slack_sdk.WebClient`` (whichever class is | ||
| # registered in this process — real or sibling stub). | ||
| assert isinstance(web_client, _fake_slack_sdk.WebClient) | ||
| assert web_client.token == "xoxb-static-token" | ||
|
|
||
| def test_returns_same_instance_per_token(self): | ||
| """Repeated access returns the exact same cached object (identity).""" | ||
| adapter = _single_workspace_adapter() | ||
|
|
||
| # Bind each property access to a name so the identity check reads as a | ||
| # caching assertion (two calls → same object), not a tautological | ||
| # ``x is x`` self-comparison. | ||
| first = adapter.web_client | ||
| second = adapter.web_client | ||
|
|
||
| assert first is second | ||
|
|
||
| def test_caches_under_the_resolved_token(self): | ||
| """The cached client is keyed by the resolved token.""" | ||
| adapter = _single_workspace_adapter("xoxb-cache-key") | ||
|
|
||
| client = adapter.web_client | ||
|
|
||
| assert adapter._web_client_cache["xoxb-cache-key"] is client | ||
|
|
||
| def test_invalidate_client_clears_web_client_cache(self): | ||
| """``_invalidate_client`` evicts the sync WebClient too (token revocation). | ||
|
|
||
| Load-bearing: without the ``_web_client_cache.pop`` in | ||
| ``_invalidate_client`` a revoked token's stale ``WebClient`` would | ||
| survive eviction and this assertion fails. | ||
| """ | ||
| adapter = _single_workspace_adapter("xoxb-revoke") | ||
| client = adapter.web_client | ||
| assert adapter._web_client_cache["xoxb-revoke"] is client | ||
|
|
||
| adapter._invalidate_client("xoxb-revoke") | ||
|
|
||
| assert "xoxb-revoke" not in adapter._web_client_cache | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Deprecated ``client`` alias | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestClientAlias: | ||
| def test_alias_returns_same_object_as_web_client(self): | ||
| """The deprecated ``client`` alias returns the same instance.""" | ||
| adapter = _single_workspace_adapter() | ||
|
|
||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore", DeprecationWarning) | ||
| assert adapter.client is adapter.web_client | ||
|
|
||
| def test_alias_emits_deprecation_warning(self): | ||
| """Accessing ``client`` warns; ``web_client`` does not.""" | ||
| adapter = _single_workspace_adapter() | ||
|
|
||
| with warnings.catch_warnings(record=True) as caught: | ||
| warnings.simplefilter("always") | ||
| _ = adapter.client | ||
|
|
||
| deprecations = [w for w in caught if issubclass(w.category, DeprecationWarning)] | ||
| assert len(deprecations) == 1 | ||
| assert "web_client" in str(deprecations[0].message) | ||
|
|
||
| def test_web_client_does_not_warn(self): | ||
| """The non-deprecated ``web_client`` property emits no warning.""" | ||
| adapter = _single_workspace_adapter() | ||
|
|
||
| with warnings.catch_warnings(record=True) as caught: | ||
| warnings.simplefilter("always") | ||
| _ = adapter.web_client | ||
|
|
||
| deprecations = [w for w in caught if issubclass(w.category, DeprecationWarning)] | ||
| assert deprecations == [] | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Multi-workspace (request-context token resolution) | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestWebClientMultiWorkspace: | ||
| def test_uses_request_context_token_under_with_bot_token(self): | ||
| """Inside ``with_bot_token`` the context token wins.""" | ||
| adapter = _multi_workspace_adapter() | ||
|
|
||
| observed: dict[str, str] = {} | ||
|
|
||
| def capture() -> None: | ||
| observed["token"] = adapter.web_client.token | ||
|
|
||
| adapter.with_bot_token("xoxb-context-token", capture) | ||
|
|
||
| assert observed["token"] == "xoxb-context-token" | ||
|
|
||
| def test_context_token_overrides_static_default(self): | ||
| """A context token takes precedence over the configured default.""" | ||
| adapter = _single_workspace_adapter("xoxb-default") | ||
|
|
||
| observed: dict[str, str] = {} | ||
|
|
||
| def capture() -> None: | ||
| observed["token"] = adapter.web_client.token | ||
|
|
||
| adapter.with_bot_token("xoxb-override", capture) | ||
|
|
||
| assert observed["token"] == "xoxb-override" | ||
| # Outside the context, resolution falls back to the static default. | ||
| assert adapter.web_client.token == "xoxb-default" | ||
|
|
||
| def test_raises_without_context_in_multi_workspace_mode(self): | ||
| """No context + no default token raises on both accessors.""" | ||
| adapter = _multi_workspace_adapter() | ||
|
|
||
| with pytest.raises(AuthenticationError): | ||
| _ = adapter.web_client | ||
|
|
||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore", DeprecationWarning) | ||
| with pytest.raises(AuthenticationError): | ||
| _ = adapter.client | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Async resolver: not-yet-resolved default token raises (sync property) | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestWebClientAsyncResolver: | ||
| def test_unresolved_async_resolver_raises(self): | ||
| """A callable ``bot_token`` that has not run yet cannot resolve sync.""" | ||
|
|
||
| async def resolver() -> str: | ||
| return "xoxb-async-resolved" | ||
|
|
||
| adapter = SlackAdapter(SlackAdapterConfig(signing_secret=_SECRET, bot_token=resolver)) | ||
|
|
||
| with pytest.raises(AuthenticationError): | ||
| _ = adapter.web_client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
SlackAdapterConfig.bot_tokenis a synchronous callable andweb_clientis accessed before a webhook orcurrent_token_async()has primed_default_bot_token_cache, this path calls_get_token(), which raisesAuthenticationErrorinstead of invoking the configured resolver. That leaves single-workspace apps that use a sync resolver for rotation or lazy secret loading unable to use the new direct WebClient outside a request context, even though the resolver can be evaluated synchronously here.Useful? React with 👍 / 👎.