Skip to content

Commit 6e99c83

Browse files
committed
fix(reviver): make from_json idempotent for object_hook use
Codex identified that ``json.loads(..., object_hook=reviver)`` revives nested dicts bottom-up, so by the time ``ThreadImpl.from_json`` runs, a payload's ``currentMessage`` is already a ``Message`` instance, not a dict. The old code then called ``Message.from_json(current_msg_raw)`` on that Message, which raised ``AttributeError`` on ``.get`` and broke deserialization of any serialized thread containing a currentMessage. Fix: - ``Message.from_json`` returns ``data`` unchanged if it's already a Message - ``ThreadImpl.from_json`` / ``ChannelImpl.from_json`` do the same, plus pass the (possibly already-Message) ``currentMessage`` through the now-idempotent ``Message.from_json`` - Regression test exercises the exact object_hook bottom-up path with a nested currentMessage Also moved ``TestStandaloneReviver``'s per-method local imports to the module top level per gemini-code-assist review (PEP 8 E402). https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG
1 parent 0ff1703 commit 6e99c83

4 files changed

Lines changed: 79 additions & 30 deletions

File tree

src/chat_sdk/channel.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,7 @@ def to_json(self) -> dict[str, Any]:
397397
@classmethod
398398
def from_json(
399399
cls,
400-
data: dict[str, Any],
400+
data: dict[str, Any] | ChannelImpl,
401401
adapter: Adapter | None = None,
402402
chat: _ChatSingleton | None = None,
403403
) -> ChannelImpl:
@@ -411,7 +411,13 @@ def from_json(
411411
Explicit adapter. Skips singleton lookup.
412412
chat:
413413
Explicit Chat instance for adapter/state resolution.
414+
415+
Idempotent: if ``data`` is already a :class:`ChannelImpl`, it is
416+
returned unchanged. This makes it safe to call via
417+
``json.loads(..., object_hook=reviver)``.
414418
"""
419+
if isinstance(data, ChannelImpl):
420+
return data
415421
channel = cls(
416422
_ChannelImplConfigLazy(
417423
id=data["id"],

src/chat_sdk/thread.py

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -739,7 +739,7 @@ def to_json(self) -> dict[str, Any]:
739739
@classmethod
740740
def from_json(
741741
cls,
742-
data: dict[str, Any],
742+
data: dict[str, Any] | ThreadImpl,
743743
adapter: Adapter | None = None,
744744
chat: _ChatSingleton | None = None,
745745
) -> ThreadImpl:
@@ -755,11 +755,17 @@ def from_json(
755755
Explicit Chat instance. If provided, adapter and state are resolved
756756
from this instance instead of the singleton. Useful in multi-chat
757757
or test scenarios.
758+
759+
Idempotent: if ``data`` is already a :class:`ThreadImpl`, it is
760+
returned unchanged. This makes it safe to call via
761+
``json.loads(..., object_hook=reviver)``.
758762
"""
763+
if isinstance(data, ThreadImpl):
764+
return data
759765
current_msg_raw = data.get("currentMessage") or data.get("current_message")
760-
current_msg = None
761-
if current_msg_raw:
762-
current_msg = Message.from_json(current_msg_raw)
766+
# ``object_hook`` revives nested dicts first, so ``currentMessage`` may
767+
# already be a Message instance by the time this runs.
768+
current_msg = Message.from_json(current_msg_raw) if current_msg_raw else None
763769

764770
thread = cls(
765771
_ThreadImplConfig(

src/chat_sdk/types.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -436,14 +436,22 @@ def to_json(self) -> dict[str, Any]:
436436
return result
437437

438438
@classmethod
439-
def from_json(cls, data: dict[str, Any]) -> Message:
439+
def from_json(cls, data: dict[str, Any] | Message) -> Message:
440440
"""Reconstruct a Message from serialized JSON data.
441441
442442
Converts ISO date strings back to ``datetime`` objects.
443443
Accepts both camelCase (canonical output of ``to_json()``) and
444444
snake_case keys for backward compatibility. For explicit
445445
dual-format handling see :meth:`from_json_compat`.
446+
447+
Idempotent: if ``data`` is already a :class:`Message`, it is
448+
returned unchanged. This makes it safe to call via
449+
``json.loads(..., object_hook=reviver)``, where nested values are
450+
revived bottom-up and the outer dict may already contain a revived
451+
instance.
446452
"""
453+
if isinstance(data, Message):
454+
return data
447455
meta = data.get("metadata", {})
448456

449457
date_sent_raw = meta.get("dateSent") or meta.get("date_sent")

tests/test_serialization.py

Lines changed: 53 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,14 @@
1111

1212
import pytest
1313

14+
from chat_sdk import reviver
15+
from chat_sdk.channel import ChannelImpl
16+
from chat_sdk.chat import Chat
1417
from chat_sdk.testing import (
1518
create_mock_adapter,
1619
create_test_message,
1720
)
18-
from chat_sdk.thread import ThreadImpl, _ThreadImplConfig
21+
from chat_sdk.thread import ThreadImpl, _ThreadImplConfig, clear_chat_singleton
1922
from chat_sdk.types import (
2023
Attachment,
2124
Author,
@@ -877,10 +880,6 @@ class TestStandaloneReviver:
877880
"""
878881

879882
def test_should_revive_chatthread_objects(self, mock_adapter, mock_state):
880-
from chat_sdk import reviver
881-
from chat_sdk.chat import Chat
882-
from chat_sdk.thread import clear_chat_singleton
883-
884883
chat = Chat(
885884
user_name="test-bot",
886885
adapters={"slack": mock_adapter},
@@ -907,10 +906,6 @@ def test_should_revive_chatthread_objects(self, mock_adapter, mock_state):
907906
clear_chat_singleton()
908907

909908
def test_should_revive_chatmessage_objects(self, mock_adapter, mock_state):
910-
from chat_sdk import reviver
911-
from chat_sdk.chat import Chat
912-
from chat_sdk.thread import clear_chat_singleton
913-
914909
chat = Chat(
915910
user_name="test-bot",
916911
adapters={"slack": mock_adapter},
@@ -950,10 +945,6 @@ def test_should_revive_chatmessage_objects(self, mock_adapter, mock_state):
950945
clear_chat_singleton()
951946

952947
def test_should_revive_both_thread_and_message_in_same_payload(self, mock_adapter, mock_state):
953-
from chat_sdk import reviver
954-
from chat_sdk.chat import Chat
955-
from chat_sdk.thread import clear_chat_singleton
956-
957948
chat = Chat(
958949
user_name="test-bot",
959950
adapters={"slack": mock_adapter},
@@ -1000,8 +991,6 @@ def test_should_revive_both_thread_and_message_in_same_payload(self, mock_adapte
1000991
clear_chat_singleton()
1001992

1002993
def test_should_leave_nonchat_objects_unchanged(self, mock_adapter, mock_state):
1003-
from chat_sdk import reviver
1004-
1005994
payload = json.dumps(
1006995
{
1007996
"name": "test",
@@ -1015,10 +1004,6 @@ def test_should_leave_nonchat_objects_unchanged(self, mock_adapter, mock_state):
10151004
assert parsed["nested"]["_type"] == "other:Type"
10161005

10171006
def test_should_be_usable_directly_as_json_parse_second_argument(self, mock_adapter, mock_state):
1018-
from chat_sdk import reviver
1019-
from chat_sdk.chat import Chat
1020-
from chat_sdk.thread import clear_chat_singleton
1021-
10221007
chat = Chat(
10231008
user_name="test-bot",
10241009
adapters={"slack": mock_adapter},
@@ -1055,8 +1040,6 @@ def test_should_be_usable_directly_as_json_parse_second_argument(self, mock_adap
10551040
clear_chat_singleton()
10561041

10571042
def test_should_allow_reserialization_of_a_revived_thread_without_singleton(self):
1058-
from chat_sdk.thread import clear_chat_singleton
1059-
10601043
clear_chat_singleton()
10611044
data = {
10621045
"_type": "chat:Thread",
@@ -1072,9 +1055,6 @@ def test_should_allow_reserialization_of_a_revived_thread_without_singleton(self
10721055
assert reserialized["id"] == "slack:C123:1234.5678"
10731056

10741057
def test_should_allow_reserialization_of_a_revived_channel_without_singleton(self):
1075-
from chat_sdk.channel import ChannelImpl
1076-
from chat_sdk.thread import clear_chat_singleton
1077-
10781058
clear_chat_singleton()
10791059
data = {
10801060
"_type": "chat:Channel",
@@ -1088,6 +1068,55 @@ def test_should_allow_reserialization_of_a_revived_channel_without_singleton(sel
10881068
assert reserialized["adapterName"] == "slack"
10891069
assert reserialized["id"] == "C123"
10901070

1071+
def test_should_revive_thread_with_nested_current_message_via_object_hook(self, mock_adapter, mock_state):
1072+
"""``object_hook`` revives children first, so ``currentMessage`` reaches
1073+
``ThreadImpl.from_json`` as a :class:`Message` instance, not a dict.
1074+
``from_json`` must accept that without raising ``AttributeError``."""
1075+
chat = Chat(
1076+
user_name="test-bot",
1077+
adapters={"slack": mock_adapter},
1078+
state=mock_state,
1079+
logger="silent",
1080+
)
1081+
chat.register_singleton()
1082+
try:
1083+
payload = json.dumps(
1084+
{
1085+
"_type": "chat:Thread",
1086+
"id": "slack:C123:1234.5678",
1087+
"channelId": "C123",
1088+
"isDM": False,
1089+
"adapterName": "slack",
1090+
"currentMessage": {
1091+
"_type": "chat:Message",
1092+
"id": "msg-current",
1093+
"threadId": "slack:C123:1234.5678",
1094+
"text": "hi",
1095+
"formatted": {"type": "root", "children": []},
1096+
"raw": {},
1097+
"author": {
1098+
"userId": "U123",
1099+
"userName": "testuser",
1100+
"fullName": "Test User",
1101+
"isBot": False,
1102+
"isMe": False,
1103+
},
1104+
"metadata": {
1105+
"dateSent": "2024-01-15T10:30:00.000Z",
1106+
"edited": False,
1107+
},
1108+
"attachments": [],
1109+
},
1110+
}
1111+
)
1112+
thread = json.loads(payload, object_hook=reviver)
1113+
assert isinstance(thread, ThreadImpl)
1114+
assert thread._current_message is not None
1115+
assert isinstance(thread._current_message, Message)
1116+
assert thread._current_message.id == "msg-current"
1117+
finally:
1118+
clear_chat_singleton()
1119+
10911120

10921121
# ============================================================================
10931122
# @workflow/serde integration — ThreadImpl

0 commit comments

Comments
 (0)