-
Notifications
You must be signed in to change notification settings - Fork 1
feat(chat): message.subject + fetch_subject adapter hook #131
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 4 commits
a62c91e
44e86c1
fd92931
6640546
ec1a030
583643a
66ef156
bf498d0
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 |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import weakref | ||
| from collections.abc import AsyncIterable, Awaitable, Callable | ||
| from dataclasses import dataclass, field | ||
| from datetime import datetime | ||
|
|
@@ -387,6 +389,111 @@ class SerializedMessage(_SerializedMessageRequired, total=False): | |
| links: list[SerializedLinkPreview] | ||
|
|
||
|
|
||
| @dataclass | ||
| class MessageSubjectParty: | ||
| """A person referenced by a :class:`MessageSubject` (assignee/author). | ||
|
|
||
| Mirrors the inline ``{ id: string; name: string }`` shape used by | ||
| upstream's ``MessageSubject.assignee`` / ``MessageSubject.author``. | ||
| """ | ||
|
|
||
| id: str | ||
| name: str | ||
|
|
||
|
|
||
| @dataclass | ||
| class MessageSubject: | ||
| """The external subject a message refers to (e.g. a Linear issue or GitHub PR). | ||
|
|
||
| Python port of the TS ``MessageSubject`` interface | ||
| (``packages/chat/src/types.ts``). Resolved lazily via | ||
| :attr:`Message.subject`, which delegates to the owning adapter's | ||
| optional :meth:`Adapter.fetch_subject` hook. | ||
|
|
||
| Field names are snake_case per the Python port convention; ``raw`` is | ||
| the platform-specific escape hatch. | ||
| """ | ||
|
|
||
| # ``id`` and ``type`` are the only required fields upstream; everything | ||
| # else is optional. ``raw`` is required upstream but defaults to ``None`` | ||
| # here so partially-populated subjects (e.g. in tests) construct cleanly. | ||
| id: str | ||
| type: str | ||
| raw: Any = None | ||
| assignee: MessageSubjectParty | None = None | ||
| author: MessageSubjectParty | None = None | ||
| description: str | None = None | ||
| labels: list[str] | None = None | ||
| status: str | None = None | ||
| title: str | None = None | ||
| url: str | None = None | ||
|
|
||
|
|
||
| # -------------------------------------------------------------------------- | ||
| # Message -> Adapter registry (powers ``Message.subject``) | ||
| # -------------------------------------------------------------------------- | ||
| # | ||
| # Upstream (``packages/chat/src/message.ts``) uses | ||
| # ``const adapterMap = new WeakMap<Message, Adapter>()`` so a dispatched | ||
| # message can lazily ask its owning adapter to resolve its subject, without | ||
| # the message holding a hard reference to the adapter and without leaking | ||
| # messages after they fall out of scope. | ||
| # | ||
| # Python port hazard — hashability/weakref: | ||
| # ``Message`` is a plain ``@dataclass`` (``eq=True``), which makes instances | ||
| # *unhashable*. A ``weakref.WeakKeyDictionary[Message, Adapter]`` therefore | ||
| # raises ``TypeError: unhashable type: 'Message'``. We deliberately do NOT | ||
| # change ``Message`` to ``eq=False``/``frozen=True`` (that would alter its | ||
| # public equality contract). Instead we key a plain ``dict`` by | ||
| # ``id(message)`` (object identity, matching ``WeakMap`` semantics) and | ||
| # register a ``weakref.finalize`` callback per message that pops the entry | ||
| # when the message is garbage-collected. ``weakref.ref(message)`` works on a | ||
| # plain dataclass even though ``hash()`` does not, so this is safe. The | ||
| # finalizer also closes the ``id()`` reuse hole: the entry is removed before | ||
| # CPython can recycle the id for a new object. | ||
| _message_adapter_map: dict[int, Adapter] = {} | ||
|
|
||
|
|
||
| def set_message_adapter(message: Message, adapter: Adapter) -> None: | ||
| """Register the adapter that owns ``message`` (powers ``message.subject``). | ||
|
|
||
| Called by :class:`~chat_sdk.chat.Chat` at the dispatch bind site so every | ||
| message handed to a handler can resolve its subject via the adapter's | ||
| optional :meth:`Adapter.fetch_subject` hook. | ||
|
|
||
| Mirrors upstream ``setMessageAdapter`` (``packages/chat/src/message.ts``). | ||
| The mapping is keyed by object identity and weakly scoped: when ``message`` | ||
| is garbage-collected, its entry is removed automatically. | ||
| """ | ||
| key = id(message) | ||
| already_registered = key in _message_adapter_map | ||
| _message_adapter_map[key] = adapter | ||
|
|
||
| # Register the GC finalizer only on first registration for a given message | ||
| # identity; re-registering the same live message just overwrites the adapter | ||
| # value above. This prevents an accumulation of redundant finalizers when a | ||
| # message is registered more than once (re-dispatch, rehydrate, multiple | ||
| # handler passes). The ``id()``-reuse hole stays closed: if a prior message | ||
| # with the same id was GC'd, its finalizer already popped the entry, so | ||
| # ``key not in _message_adapter_map`` is true again and a fresh finalizer is | ||
| # registered for the new object. | ||
| if not already_registered: | ||
| # Drop the entry when the message is GC'd. A zero-arg closure (rather | ||
| # than ``weakref.finalize(message, dict.pop, key, None)``) captures | ||
| # ``key`` and keeps the finalizer callable's type unambiguous for the | ||
| # type-checker. ``pop(key, None)`` is a no-op if the entry was already | ||
| # removed. | ||
| def _cleanup() -> None: | ||
| _message_adapter_map.pop(key, None) | ||
|
|
||
| weakref.finalize(message, _cleanup) | ||
|
|
||
|
|
||
| def _get_message_adapter(message: Message) -> Adapter | None: | ||
| """Return the adapter registered for ``message``, or ``None``.""" | ||
| return _message_adapter_map.get(id(message)) | ||
|
|
||
|
|
||
| def _strip_none(d: dict[str, Any]) -> dict[str, Any]: | ||
| """Remove keys whose value is ``None`` from a dict. | ||
|
|
||
|
|
@@ -412,6 +519,72 @@ class Message: | |
| links: list[LinkPreview] | None = None | ||
| raw: Any = None | ||
|
|
||
| # Cached awaitable for ``subject``. Mirrors upstream's ``_subjectPromise``: | ||
| # the first ``await message.subject`` stores the in-flight future here so a | ||
| # second access reuses it instead of re-calling ``fetch_subject``. | ||
| # ``init=False``/``compare=False``/``repr=False`` keep it out of the | ||
| # dataclass ``__init__``, equality, and ``repr`` — it is purely internal | ||
| # resolution state, not message data. | ||
| _subject_future: Any = field(default=None, init=False, compare=False, repr=False) | ||
|
|
||
| async def _resolve_subject(self) -> MessageSubject | None: | ||
| """Resolve the subject via the owning adapter's ``fetch_subject`` hook. | ||
|
|
||
| Returns ``None`` when no adapter is registered, the adapter has no | ||
| ``fetch_subject`` hook, the hook returns ``None``, or the hook raises | ||
| (failures are swallowed, mirroring upstream's ``.catch(() => null)``). | ||
| """ | ||
| adapter = _get_message_adapter(self) | ||
| if adapter is None: | ||
| return None | ||
| fetch_subject = getattr(adapter, "fetch_subject", None) | ||
| if fetch_subject is None: | ||
| return None | ||
|
Comment on lines
+537
to
+542
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. When adapter = _get_message_adapter(self)
if adapter is None:
return None
fetch_subject = getattr(adapter, "fetch_subject", None)
if fetch_subject is None:
return NoneReferences
|
||
| try: | ||
| return await fetch_subject(self.raw) | ||
| except Exception: | ||
| return None | ||
|
|
||
| async def _subject(self) -> MessageSubject | None: | ||
| """Coroutine backing the :attr:`subject` accessor (caches the result). | ||
|
|
||
| The first await schedules ``_resolve_subject`` once via | ||
| ``ensure_future`` and stores the shared future on the instance; every | ||
| later/concurrent await reuses it, so ``fetch_subject`` runs at most | ||
| once. Mirrors upstream's cached ``_subjectPromise``. | ||
|
|
||
| The cached future is awaited through :func:`asyncio.shield` so that a | ||
| caller cancellation (e.g. ``asyncio.wait_for(msg.subject, timeout=...)`` | ||
| firing) propagates ``CancelledError`` to the caller but does *not* | ||
| cancel the shared inner task. Without shielding, the first cancelled | ||
| awaiter would poison the cache and every subsequent ``await | ||
| msg.subject`` would raise ``CancelledError``. | ||
| """ | ||
| if self._subject_future is None: | ||
| self._subject_future = asyncio.ensure_future(self._resolve_subject()) | ||
| return await asyncio.shield(self._subject_future) | ||
|
|
||
| @property | ||
| def subject(self) -> Awaitable[MessageSubject | None]: | ||
| """The external subject this message refers to (issue, PR, etc.), or ``None``. | ||
|
|
||
| Lazily resolved via the owning adapter's optional | ||
| :meth:`Adapter.fetch_subject` hook. The adapter is registered at | ||
| dispatch time by :func:`set_message_adapter`. | ||
|
|
||
| Mirrors upstream ``Message.subject`` (``packages/chat/src/message.ts``): | ||
| it is an awaitable, the result is cached after the first access, and a | ||
| second ``await message.subject`` does NOT re-call ``fetch_subject``. | ||
| Concurrent awaits share a single in-flight resolution. | ||
|
|
||
| Usage:: | ||
|
|
||
| subject = await message.subject | ||
| if subject is not None: | ||
| ... | ||
| """ | ||
| return self._subject() | ||
|
|
||
| def to_json(self) -> dict[str, Any]: | ||
| """Serialize to JSON-compatible dict. | ||
|
|
||
|
|
@@ -1257,6 +1430,17 @@ async def get_user(self, user_id: str) -> UserInfo | None: | |
| """ | ||
| return None | ||
|
|
||
| # NOTE: ``fetch_subject`` is intentionally NOT declared here. Upstream's | ||
| # ``Adapter.fetchSubject`` is an *optional* member (``fetchSubject?(...)``), | ||
| # and in this Python port the established convention for optional adapter | ||
| # hooks (``stream``, ``open_dm``, ``rehydrate_attachment``, | ||
| # ``get_channel_visibility``, ...) is to declare them on :class:`BaseAdapter` | ||
| # only — NOT on this structural ``Protocol`` — so that adapters which don't | ||
| # implement them still satisfy ``Adapter`` for type-checking. Declaring it | ||
| # on the Protocol would make it a *required* attribute and break every | ||
| # adapter that doesn't define it. :attr:`Message.subject` reads the hook via | ||
| # ``getattr(adapter, "fetch_subject", None)``, so presence is fully optional. | ||
|
|
||
|
|
||
| class BaseAdapter: | ||
| """Base adapter with default implementations for optional methods. | ||
|
|
@@ -1415,6 +1599,22 @@ async def get_user(self, user_id: str) -> UserInfo | None: | |
| """ | ||
| raise ChatNotImplementedError(self.name, "getUser") | ||
|
|
||
| async def fetch_subject(self, raw: Any) -> MessageSubject | None: | ||
| """Resolve the external subject a message refers to (issue, PR, etc.). | ||
|
|
||
| Optional — the default returns ``None`` (no subject). Adapters that | ||
| can resolve a backing entity (a Linear issue, a GitHub PR, etc.) from a | ||
| message's raw payload should override this. Unlike most optional | ||
| :class:`BaseAdapter` hooks it does *not* raise | ||
| :class:`~chat_sdk.errors.ChatNotImplementedError`, because | ||
| :attr:`Message.subject` is best-effort: "this adapter has no subject | ||
| concept" is a normal, non-error outcome that maps to ``None``. | ||
|
|
||
| Mirrors upstream's optional ``Adapter.fetchSubject`` | ||
| (``packages/chat/src/types.ts``). | ||
| """ | ||
| return None | ||
|
|
||
| def rehydrate_attachment(self, attachment: Attachment) -> Attachment: | ||
| """Reconstruct ``fetch_data`` on an attachment after deserialization. | ||
|
|
||
|
|
||
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 queue/burst drains collapse multiple messages, only the latest
messagepassed to_dispatch_to_handlersis registered here; the earlier messages are delivered to handlers ascontext.skippedwithout ever callingset_message_adapter(they were only enqueued while the lock was busy). In that scenario,await context.skipped[0].subjectalways resolves toNoneeven when the adapter implementsfetch_subject, so handlers cannot use the new subject accessor on skipped messages preserved by the concurrency context. Register the skipped messages in the context with the same adapter before invoking handlers.Useful? React with 👍 / 👎.