diff --git a/docs/adr/044-slack-log-driver-post-message.md b/docs/adr/044-slack-log-driver-post-message.md new file mode 100644 index 00000000..d476f1fd --- /dev/null +++ b/docs/adr/044-slack-log-driver-post-message.md @@ -0,0 +1,53 @@ +# 044. Slack log driver posts straight to chat.postMessage + +Date: 2026-10-29 + +## Problem + +`LogSlackDriver.send` resolved the configured channel name to an id by calling `channels.list` before every `chat.postMessage`. Slack retired `channels.list` in 2021. It now answers `{"ok": false, "error": "method_deprecated"}`, so `response.json()["channels"]` raised `KeyError: 'channels'` and every log sent to the `slack` channel crashed the caller. + +`chat.postMessage` itself never needed the lookup: its `channel` argument accepts a channel name such as `#alerts` or a channel id such as `C0123456789`. + +The driver also never looked at the `chat.postMessage` response, so a wrong token or a missing channel failed silently, and it sent the token as a form field instead of the `Authorization` header that Slack recommends. + +## Alternatives + +1. Replace `channels.list` with `conversations.list`. It needs the extra `channels:read` scope, is paginated (a workspace with more than one page of channels needs cursor handling), and adds a request to every log line for no gain. +2. Cache the id from `conversations.list`. Same scope and pagination cost, plus cache invalidation when a channel is renamed. +3. Drop the lookup and pass the configured channel to `chat.postMessage` as is. + +## Decision + +Option 3, approved by the user. + +`send` posts JSON to `https://slack.com/api/chat.postMessage` with `Authorization: Bearer `, the configured `channel` unchanged, the formatted `text`, and `username`/`icon_emoji` when they are set. It calls `raise_for_status()` (non-2xx, for example a 429 rate limit, raises `requests.HTTPError`) and then checks the body: when `ok` is not true it raises `SlackApiError` with the Slack error code, for example `Slack chat.postMessage failed for channel '#alerts': not_authed`. When the code is `channel_not_found` it raises `SlackChannelNotFound`, now a subclass of `SlackApiError`, so code that caught the old exception still catches the same situation. Raising matches the other drivers, which let file and syslog errors propagate rather than swallowing them. Requests use a 10 second timeout so a slow Slack API cannot hang the caller. + +The config surface is unchanged: `driver`, `channel`, `emoji`, `username`, `token`, `level`. No setting existed only for the lookup, so none is removed. `channel` now also accepts a channel id. + +Removed from the payload: `token` (moved to the header), `as_user` (classic apps only), `reply_broadcast` (only meaningful with `thread_ts`, which the driver never sends), and the explicit `unfurl_links`/`unfurl_media` flags. `find_channel` is removed. + +## Implementation + +- `logging/drivers/LogSlackDriver.py`: `SlackApiError`, `SlackChannelNotFound(SlackApiError)`, `send` as above, `find_channel` deleted. +- `tests/logging/test_logging.py`: the `find_channel` tests are replaced by tests that mock `requests.post`. + +User configuration: + +```python +from fastapi_startkit.logging.config import SlackChannel + +"slack": SlackChannel( + channel="#alerts", + token=env("SLACK_TOKEN"), + level="error", +), +``` + +## Validation + +- `chat.postMessage` is called once with channel `#alerts`, the `Authorization: Bearer` header and the formatted message text; `channels.list` is never called. +- `{"ok": false, "error": "not_authed"}` raises `SlackApiError`, not `KeyError`. +- `{"ok": false, "error": "channel_not_found"}` raises `SlackChannelNotFound`. +- An HTTP error status raises `requests.HTTPError`. +- Unset `username`/`emoji` are not sent. +- `uv run pytest --ignore=tests/masoniteorm/postgres --cov` (at least 80%), `ruff`, `basedpyright`. diff --git a/docs/adr/AGENTS.md b/docs/adr/AGENTS.md index 7ce87eec..7fb67748 100644 --- a/docs/adr/AGENTS.md +++ b/docs/adr/AGENTS.md @@ -39,4 +39,5 @@ Read this index first, then open the records relevant to the change. Before impl | [038](038-storage-binary-copy-and-request-scoped-vite-inertia.md) | 2026-10-25 | Storage binary reads and relative copy; request-scoped Vite and Inertia state | `LocalDriver.copy` resolves both paths through `get_path`. Local and S3 `get` read bytes and return `str` for UTF-8 content or raw `bytes` otherwise, and a new `get_bytes` (also on `StorageManager`/`Storage`) always returns bytes. `get` returns `bytes` for any non-UTF-8 content, including latin-1 text. `S3Driver.append` keeps the existing content. Vite `use_*_tag_attributes` accept dicts and `False`; the self-referencing lambda is gone. A pure ASGI `ViteMiddleware` (registered by `ViteProvider`) opens `Vite.request_scope()`, which holds the request's CSP nonce and preloaded assets in a per-instance `ContextVar` shared with child tasks. Outside a scope, the instance values are process-wide defaults, so a nonce set at startup still applies, and middleware inside the scope can read preloaded assets after `call_next`. `Inertia.set_root_view`/`Inertia.version` write to the current `InertiaRequestState` inside a request and to global defaults otherwise. | | [040](040-deprecate-application-env-argument.md) | 2026-10-25 | Deprecate the `env` argument of `Application` | The deployment environment must come from outside the code: `APP_ENV`, the CLI `--env` option (turned into `APP_ENV` before Cleo parses arguments) or pytest detection (`testing`). `Application(env=...)` now emits a `DeprecationWarning` at the caller (`stacklevel=2`) and will be removed in 1.0; behaviour and precedence are unchanged until then. `set_environment()` stays. Framework tests no longer pass `env=`, and pytest `filterwarnings` turns this warning into an error so the suite cannot reintroduce it. | | [041](041-database-install-command.md) | 2026-10-22 | InstallCommand base class and db:install | `console/install_command.py` adds an `InstallCommand` base class with a fluent class builder: `InstallCommand.register(name).copy(stub, dest).add_provider(Provider, "config.x.XConfig")`. Each call returns a new subclass. Providers and configs can be classes or dotted strings (strings for app-side configs and optional extras). `--force`, `publish_file` (skips existing files unless forced) and `register_provider` are shared. `register_provider` finds `Application(...)` in `bootstrap/application.py` with `ast` and inserts the entry and any missing imports at AST-reported positions, so formatting is kept. It handles `providers` passed by keyword or positionally. It does nothing if the provider is already listed, by bare name or attribute. It stops with manual steps and exit 1 if the bootstrap file is missing, or if a needed name is already imported from a different module (no aliasing). `db:install` (registered by `AppProvider`) subclasses it to add `--driver` and render `config/database.py` from the installer stub. After a successful run it prints next steps from the installer's database section (the `DB_*` env entries, `uv add` when SQLAlchemy is missing, migrations). Both live in `console/` so the optional SQLAlchemy extra is not imported. | +| [044](044-slack-log-driver-post-message.md) | 2026-10-29 | Slack log driver posts straight to chat.postMessage | The driver resolved channel ids through `channels.list`, retired by Slack in 2021, so every Slack log raised `KeyError: 'channels'`. The lookup and `find_channel` are gone: `send` posts JSON to `chat.postMessage` with `Authorization: Bearer ` and the configured channel as is (`#alerts` or an id), with a 10 s timeout. `raise_for_status()` covers HTTP errors; `ok: false` raises `SlackApiError` with the Slack error code, and `channel_not_found` raises `SlackChannelNotFound` (now a `SlackApiError` subclass). Config surface unchanged; `as_user`, `reply_broadcast` and the form `token` field are dropped from the payload. | | [047](047-ai-bedrock-provider.md) | 2026-10-29 | AWS Bedrock provider for the AI module | Adds `provider="bedrock"` via LangChain's `bedrock_converse` (`ChatBedrockConverse`), the same `init_chat_model` path as other providers. `BedrockConfig` reads `AWS_REGION`/`AWS_DEFAULT_REGION`, IAM keys, session token, `AWS_PROFILE`, `AWS_BEARER_TOKEN_BEDROCK` and `AWS_BEDROCK_MODEL`. `Lab.get_client_options()` maps them to ChatBedrockConverse arguments and drops empty values so boto3's default credential chain applies; other providers return `{}`. The `bedrock` extra is `fastapi-startkit[ai]` plus `langchain-aws`, because `langchain-aws` does not pull in `langchain`. `langchain-aws` is imported only by `init_chat_model` when a Bedrock model is built. | diff --git a/fastapi_startkit/src/fastapi_startkit/logging/drivers/LogSlackDriver.py b/fastapi_startkit/src/fastapi_startkit/logging/drivers/LogSlackDriver.py index ae6f003d..bae17cd4 100644 --- a/fastapi_startkit/src/fastapi_startkit/logging/drivers/LogSlackDriver.py +++ b/fastapi_startkit/src/fastapi_startkit/logging/drivers/LogSlackDriver.py @@ -2,7 +2,11 @@ import requests -class SlackChannelNotFound(Exception): +class SlackApiError(Exception): + pass + + +class SlackChannelNotFound(SlackApiError): pass @@ -44,36 +48,23 @@ def get_format(self, message, level): ) def send(self, message): - requests.post( + payload = { + "channel": self.channel, + "text": message, + "username": self.username, + "icon_emoji": self.emoji, + } + response = requests.post( self.slack_url, - { - "token": self.token, - "channel": self.find_channel(self.channel), - "text": message, - "username": self.username, - "icon_emoji": self.emoji, - "as_user": False, - "reply_broadcast": True, - "unfurl_links": True, - "unfurl_media": True, - }, + json={key: value for key, value in payload.items() if value is not None}, + headers={"Authorization": f"Bearer {self.token}"}, + timeout=10, ) - - def find_channel(self, name): - """Calls the Slack API to find the channel name. - This is so we do not have to specify the channel ID's. Slack requires channel ID's - to be used. - Arguments: - name {string} -- The channel name to find. - Raises: - SlackChannelNotFound -- Thrown if the channel name is not found. - Returns: - self - """ - response = requests.post("https://slack.com/api/channels.list", {"token": self.token}) - - for channel in response.json()["channels"]: - if channel["name"] == name.split("#")[1]: - return channel["id"] - - raise SlackChannelNotFound("Could not find the {} channel".format(name)) + response.raise_for_status() + body = response.json() + if body.get("ok"): + return + + error = body.get("error", "unknown_error") + exception = SlackChannelNotFound if error == "channel_not_found" else SlackApiError + raise exception(f"Slack chat.postMessage failed for channel {self.channel!r}: {error}") diff --git a/fastapi_startkit/tests/logging/test_logging.py b/fastapi_startkit/tests/logging/test_logging.py index 6fb49227..7689b8d5 100644 --- a/fastapi_startkit/tests/logging/test_logging.py +++ b/fastapi_startkit/tests/logging/test_logging.py @@ -4,6 +4,8 @@ from contextlib import redirect_stdout from unittest.mock import MagicMock, patch +import requests + from fastapi_startkit.application import Application, app as get_app from fastapi_startkit.logging.ChannelFactory import ChannelFactory from fastapi_startkit.logging.channels import ( @@ -20,7 +22,7 @@ from fastapi_startkit.logging.config import channels as config_channels from fastapi_startkit.logging.drivers.BaseDriver import BaseDriver from fastapi_startkit.logging.drivers.LogSingleDriver import LogSingleDriver -from fastapi_startkit.logging.drivers.LogSlackDriver import LogSlackDriver, SlackChannelNotFound +from fastapi_startkit.logging.drivers.LogSlackDriver import LogSlackDriver, SlackApiError, SlackChannelNotFound from fastapi_startkit.logging.drivers.LogSyslogDriver import LogSyslogDriver from fastapi_startkit.logging.drivers.LogTerminalDriver import LogTerminalDriver from fastapi_startkit.logging.factory import DriverFactory @@ -292,48 +294,62 @@ def test_change_format_replaces_handlers(self): class SlackDriverTest(unittest.TestCase): - def _driver(self): - return LogSlackDriver(token="tok", channel="#bot", username="bot", emoji=":x:") + def _driver(self, **overrides): + options = {"token": "xoxb-tok", "channel": "#alerts", "username": "bot", "emoji": ":x:"} + return LogSlackDriver(**{**options, **overrides}) + + def _post(self, body): + response = MagicMock() + response.json.return_value = body + return patch("fastapi_startkit.logging.drivers.LogSlackDriver.requests.post", return_value=response) def test_get_format_includes_level_and_message(self): formatted = self._driver().get_format("hi", "ERROR") self.assertIn("ERROR", formatted) self.assertIn("hi", formatted) - def test_level_methods_post_to_slack(self): - driver = self._driver() - with ( - patch("fastapi_startkit.logging.drivers.LogSlackDriver.requests") as requests_mock, - patch.object(driver, "find_channel", return_value="C123"), - ): - driver.error("boom") - requests_mock.post.assert_called_once() - url, payload = requests_mock.post.call_args.args - # Assert the concrete Slack endpoint, not driver.slack_url (that would be - # circular — it is the value that produced the call). - self.assertEqual(url, "https://slack.com/api/chat.postMessage") - self.assertIn("boom", payload["text"]) - self.assertIn("ERROR", payload["text"]) - self.assertEqual(payload["token"], "tok") - self.assertEqual(payload["channel"], "C123") - self.assertEqual(payload["username"], "bot") - - def test_find_channel_resolves_channel_id(self): - driver = self._driver() - response = MagicMock() - response.json.return_value = {"channels": [{"name": "bot", "id": "C123"}]} - with patch("fastapi_startkit.logging.drivers.LogSlackDriver.requests") as requests_mock: - requests_mock.post.return_value = response - self.assertEqual(driver.find_channel("#bot"), "C123") - - def test_find_channel_raises_when_missing(self): - driver = self._driver() - response = MagicMock() - response.json.return_value = {"channels": [{"name": "other", "id": "C999"}]} - with patch("fastapi_startkit.logging.drivers.LogSlackDriver.requests") as requests_mock: - requests_mock.post.return_value = response - with self.assertRaises(SlackChannelNotFound): - driver.find_channel("#bot") + def test_error_posts_to_chat_post_message_with_channel_name(self): + with self._post({"ok": True}) as post: + self._driver().error("boom") + + post.assert_called_once() + self.assertEqual(post.call_args.args, ("https://slack.com/api/chat.postMessage",)) + kwargs = post.call_args.kwargs + self.assertEqual(kwargs["headers"], {"Authorization": "Bearer xoxb-tok"}) + self.assertEqual(kwargs["json"]["channel"], "#alerts") + self.assertIn("ERROR - boom", kwargs["json"]["text"]) + self.assertEqual(kwargs["json"]["username"], "bot") + self.assertEqual(kwargs["json"]["icon_emoji"], ":x:") + self.assertNotIn("token", kwargs["json"]) + + def test_never_calls_retired_channels_list(self): + with self._post({"ok": True}) as post: + self._driver().critical("down") + + urls = [call.args[0] for call in post.call_args_list] + self.assertEqual(urls, ["https://slack.com/api/chat.postMessage"]) + + def test_unset_username_is_not_sent(self): + with self._post({"ok": True}) as post: + self._driver(username=None).info("ping") + + self.assertNotIn("username", post.call_args.kwargs["json"]) + + def test_slack_error_response_raises_slack_api_error(self): + with self._post({"ok": False, "error": "not_authed"}): + with self.assertRaisesRegex(SlackApiError, "not_authed"): + self._driver().error("boom") + + def test_channel_not_found_raises_slack_channel_not_found(self): + with self._post({"ok": False, "error": "channel_not_found"}): + with self.assertRaisesRegex(SlackChannelNotFound, "#alerts"): + self._driver().error("boom") + + def test_http_error_status_is_raised(self): + with self._post({"ok": False}) as post: + post.return_value.raise_for_status.side_effect = requests.HTTPError("429") + with self.assertRaises(requests.HTTPError): + self._driver().error("boom") class SyslogDriverTest(unittest.TestCase):