Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion backend/app/channels/store.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,12 @@ def _save(self) -> None:

@staticmethod
def _key(channel_name: str, chat_id: str, topic_id: str | None = None) -> str:
if topic_id:
# Use ``is not None`` (not truthiness) so an empty ``topic_id`` stays a
# distinct key from the topic-less base mapping. ``remove()`` already
# treats any non-None topic_id as topic-specific; collapsing ``""`` here
# would let a topic write/delete clobber the base ``channel:chat`` entry
# (e.g. DingTalk group messages whose ``message_id`` is missing).
if topic_id is not None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct fix. One thing worth a follow-up: the upstream source of empty topic_id values is still live in dingtalk.py, where msg_id = message.message_id or "" feeds topic_id = msg_id if GROUP else None. With this change those group messages no longer clobber the base key (good), but every message_id-less group message in the same conversation now buckets into one shared thread at channel:conv:. If the intended DingTalk group semantics is one-thread-per-message, that's still broken upstream whenever message_id is missing — normalizing msg_id or None would make empty genuinely topic-less. Out of scope for this PR, but worth filing so the upstream source of "" isn't forgotten.

return f"{channel_name}:{chat_id}:{topic_id}"
return f"{channel_name}:{chat_id}"

Expand Down
17 changes: 17 additions & 0 deletions backend/tests/test_channels.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,23 @@ def test_corrupt_file_handled(self, tmp_path):
store = ChannelStore(path=path)
assert store.get_thread_id("x", "y") is None

def test_empty_topic_id_is_distinct_from_none(self, store):
"""Empty topic_id must not collapse onto the topic-less base key.

Channels such as DingTalk may pass ``topic_id=""`` when a group
message lacks ``message_id``. Truthy keying would overwrite/delete the
base conversation mapping instead of a topic-specific entry.
"""
store.set_thread_id("dingtalk", "conv", "base-thread", topic_id=None)
store.set_thread_id("dingtalk", "conv", "empty-topic-thread", topic_id="")

assert store.get_thread_id("dingtalk", "conv") == "base-thread"
assert store.get_thread_id("dingtalk", "conv", topic_id="") == "empty-topic-thread"

assert store.remove("dingtalk", "conv", topic_id="") is True
assert store.get_thread_id("dingtalk", "conv", topic_id="") is None
assert store.get_thread_id("dingtalk", "conv") == "base-thread"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice targeted regression test. Small gap worth closing: nothing here (or in test_remove) locks in the contract that the topic-less bulk remove(channel, chat) still sweeps the empty-topic key — i.e. "channel:chat:".startswith("channel:chat:"). A few lines that re-set the empty-topic entry and then assert store.remove("dingtalk", "conv") is True would protect that against a future change to the prefix matching.



# ---------------------------------------------------------------------------
# Channel base class tests
Expand Down
Loading