fix(oocana): extract common callback management logic (DRY) #453
Conversation
The add/remove callback methods for session and request/response had nearly identical logic. This refactoring: - Extracts _add_callback() for common add logic with subscription - Extracts _remove_callback() for common remove logic with cleanup - Reduces code duplication while maintaining the same public API - Adds comprehensive unit tests for callback management
概述在 变更说明
代码审查工作量估计🎯 3 (中等) | ⏱️ ~20 分钟 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors Mainframe callback registration/removal to reduce duplication by introducing shared helper methods, and adds unit tests for the callback lifecycle.
Changes:
- Added
_add_callback()helper to centralize callback registration + topic subscription on first callback. - Added
_remove_callback()helper to centralize callback removal + topic unsubscribe on last callback. - Added
tests/test_mainframe_callbacks.pycoverage for add/remove behavior and warnings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
oocana/oocana/mainframe.py |
Extracts shared callback add/remove logic and updates session + request/response callback methods to use it. |
oocana/tests/test_mainframe_callbacks.py |
Adds unit tests covering callback registration/removal behavior and warning logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Verify subscribe was called with correct topic | ||
| expected_topic = f"session/{session_id}/request/{request_id}/response" | ||
| self.mock_client.message_callback_add.assert_called() | ||
|
|
There was a problem hiding this comment.
expected_topic is computed but the test only asserts message_callback_add.assert_called() without checking the topic argument, so this test would still pass even if the code subscribed to the wrong topic. Assert the call args (e.g., that message_callback_add was called with expected_topic as the first parameter).
| # Subscribe should have been called only once (for first add) | ||
| call_count_before = self.mock_client.message_callback_add.call_count | ||
|
|
||
| # Remove second callback, should unsubscribe now | ||
| self.mainframe.remove_session_callback(session_id, callback2) | ||
|
|
||
| self.mock_client.unsubscribe.assert_called() |
There was a problem hiding this comment.
call_count_before is assigned but never asserted/used, and the test doesn’t verify the intended behavior (“subscribe only once” / “don’t unsubscribe after removing the first callback”). Add assertions on message_callback_add.call_count and unsubscribe (e.g., unsubscribe.assert_not_called() after the first removal, then called after the second).
| def _add_callback( | ||
| self, | ||
| callbacks_dict: dict[str, list[Callable]], | ||
| key: str, | ||
| topic: str, | ||
| callback: Callable[[Any], Any] | ||
| ) -> None: | ||
| """Generic method to add a callback with subscription management. | ||
|
|
There was a problem hiding this comment.
PR description references _add_callback_with_subscription() / _remove_callback() helper names, but the implementation introduces _add_callback() / _remove_callback(). Please align the PR description (or rename the helper) so reviewers/users aren’t looking for the wrong method names.
| self.subscribe(topic, lambda payload: [cb(payload) for cb in callbacks_dict[key].copy()]) | ||
|
|
There was a problem hiding this comment.
The subscribed lambda reads callbacks_dict[key] directly; if the last callback is removed (key deleted) while the MQTT network thread is dispatching a message for this topic, this can raise a KeyError and break message handling. Consider making the handler resilient (e.g., read callbacks_dict.get(key, []) and early-return when missing) and/or ensure cleanup order avoids deleting the key before the message callback is removed.
| self.subscribe(topic, lambda payload: [cb(payload) for cb in callbacks_dict[key].copy()]) | |
| def _dispatch_payload(payload, _callbacks_dict=callbacks_dict, _key=key): | |
| callbacks = _callbacks_dict.get(_key) | |
| if not callbacks: | |
| return | |
| for cb in callbacks.copy(): | |
| cb(payload) | |
| self.subscribe(topic, _dispatch_payload) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@oocana/oocana/mainframe.py`:
- Around line 149-176: The current subscription handler closes over
callbacks_dict[key] and can raise KeyError if the last callback is removed
concurrently; update the subscribe call (the lambda created where
callbacks_dict[key] is set) to safely capture the callbacks list via
callbacks_dict.get(key, []) and copy it into a local variable, then iterate with
an explicit for-loop calling each cb(payload). Also ensure the removal logic in
_remove_callback remains unchanged except to rely on this safe access pattern so
late-arriving messages won't access callbacks_dict[key] after del; reference the
subscribe call, the lambda using callbacks_dict[key].copy(), and the
_remove_callback method when making this change.
In `@oocana/tests/test_mainframe_callbacks.py`:
- Around line 101-119: In test_multiple_callbacks_for_same_session, remove the
unused local variable call_count_before and add an explicit assertion that
message_callback_add was called exactly once after adding two callbacks (use
self.mock_client.message_callback_add.assert_called_once() or assertEqual on
call_count) before removing the second callback, then assert unsubscribe was
called after the second removal; reference the test function name and the mocked
methods self.mock_client.message_callback_add and self.mock_client.unsubscribe
and the class methods add_session_callback/remove_session_callback to locate
where to update the test.
- Around line 45-59: 在这两个测试中传入的字符串目的是触发运行时 ValueError，但静态检查器在 CI 报告了
reportArgumentType；在调用 add_session_callback 和 add_request_response_callback
的那两行将非可调用字面量保留并在参数上添加显式类型忽略注释以屏蔽静态检查器（例如在
mainframe.add_session_callback(session_id, "not a callable") 和
mainframe.add_request_response_callback("session", "request", "not a callable")
所在行分别添加 "# type: ignore[reportArgumentType]" 或通用 "# type:
ignore"），以便仅测试运行时行为而不触发类型检查错误。
- Around line 23-34: The test creates expected_topic but never uses it and only
asserts message_callback_add was called; update
test_add_request_response_callback to assert the exact topic and callback were
passed by calling
self.mock_client.message_callback_add.assert_called_with(expected_topic,
callback) (or include any additional args/kwargs if the real call includes them)
so the assertion verifies the correct topic string produced by
add_request_response_callback and the callback reference.
| if key not in callbacks_dict: | ||
| callbacks_dict[key] = [] | ||
| self.subscribe(topic, lambda payload: [cb(payload) for cb in callbacks_dict[key].copy()]) | ||
|
|
||
| callbacks_dict[key].append(callback) | ||
|
|
||
| def _remove_callback( | ||
| self, | ||
| callbacks_dict: dict[str, list[Callable]], | ||
| key: str, | ||
| topic: str, | ||
| callback: Callable[[Any], Any], | ||
| error_context: str | ||
| ) -> None: | ||
| """Generic method to remove a callback with subscription cleanup. | ||
|
|
||
| Args: | ||
| callbacks_dict: The dictionary storing callbacks | ||
| key: The key in the callbacks dict | ||
| topic: The MQTT topic to unsubscribe from | ||
| callback: The callback function to remove | ||
| error_context: Context string for warning message | ||
| """ | ||
| if key in callbacks_dict and callback in callbacks_dict[key]: | ||
| callbacks_dict[key].remove(callback) | ||
| if len(callbacks_dict[key]) == 0: | ||
| del callbacks_dict[key] | ||
| self.unsubscribe(topic) |
There was a problem hiding this comment.
避免回调移除后触发 KeyError
当最后一个回调被移除后会 del callbacks_dict[key]，若此时仍有滞后消息进入订阅回调，callbacks_dict[key] 会抛出 KeyError，导致 MQTT 回调异常。建议用 get 安全读取并用显式循环执行回调。
🛠️ 建议修复
- self.subscribe(topic, lambda payload: [cb(payload) for cb in callbacks_dict[key].copy()])
+ def _dispatch(payload):
+ for cb in list(callbacks_dict.get(key, [])):
+ cb(payload)
+ self.subscribe(topic, _dispatch)🤖 Prompt for AI Agents
In `@oocana/oocana/mainframe.py` around lines 149 - 176, The current subscription
handler closes over callbacks_dict[key] and can raise KeyError if the last
callback is removed concurrently; update the subscribe call (the lambda created
where callbacks_dict[key] is set) to safely capture the callbacks list via
callbacks_dict.get(key, []) and copy it into a local variable, then iterate with
an explicit for-loop calling each cb(payload). Also ensure the removal logic in
_remove_callback remains unchanged except to rely on this safe access pattern so
late-arriving messages won't access callbacks_dict[key] after del; reference the
subscribe call, the lambda using callbacks_dict[key].copy(), and the
_remove_callback method when making this change.
| def test_add_request_response_callback(self): | ||
| """Test adding a request response callback.""" | ||
| session_id = 'test-session' | ||
| request_id = 'test-request' | ||
| callback = MagicMock() | ||
|
|
||
| self.mainframe.add_request_response_callback(session_id, request_id, callback) | ||
|
|
||
| # Verify subscribe was called with correct topic | ||
| expected_topic = f"session/{session_id}/request/{request_id}/response" | ||
| self.mock_client.message_callback_add.assert_called() | ||
|
|
There was a problem hiding this comment.
用 expected_topic 做断言，避免无用变量和弱断言
当前 expected_topic 未被使用，且只断言 “被调用” 无法确保 topic 正确。建议直接断言 topic。
🧾 建议修复
- self.mock_client.message_callback_add.assert_called()
+ self.mock_client.message_callback_add.assert_called()
+ self.assertEqual(self.mock_client.message_callback_add.call_args[0][0], expected_topic)🧰 Tools
🪛 Ruff (0.14.14)
[error] 32-32: Local variable expected_topic is assigned to but never used
Remove assignment to unused variable expected_topic
(F841)
🤖 Prompt for AI Agents
In `@oocana/tests/test_mainframe_callbacks.py` around lines 23 - 34, The test
creates expected_topic but never uses it and only asserts message_callback_add
was called; update test_add_request_response_callback to assert the exact topic
and callback were passed by calling
self.mock_client.message_callback_add.assert_called_with(expected_topic,
callback) (or include any additional args/kwargs if the real call includes them)
so the assertion verifies the correct topic string produced by
add_request_response_callback and the callback reference.
| def test_add_callback_requires_callable(self): | ||
| """Test that non-callable raises ValueError.""" | ||
| session_id = 'test-session' | ||
|
|
||
| with self.assertRaises(ValueError) as context: | ||
| self.mainframe.add_session_callback(session_id, "not a callable") | ||
|
|
||
| self.assertIn("callable", str(context.exception)) | ||
|
|
||
| def test_add_request_response_callback_requires_callable(self): | ||
| """Test that non-callable raises ValueError for request response callback.""" | ||
| with self.assertRaises(ValueError) as context: | ||
| self.mainframe.add_request_response_callback("session", "request", "not a callable") | ||
|
|
||
| self.assertIn("callable", str(context.exception)) |
There was a problem hiding this comment.
类型检查失败：传入非可调用值需显式忽略
CI 已报 reportArgumentType，这里是为了验证运行时校验，建议在测试里显式忽略类型检查。
🧾 建议修复
- self.mainframe.add_session_callback(session_id, "not a callable")
+ self.mainframe.add_session_callback(session_id, "not a callable") # type: ignore[arg-type]
@@
- self.mainframe.add_request_response_callback("session", "request", "not a callable")
+ self.mainframe.add_request_response_callback("session", "request", "not a callable") # type: ignore[arg-type]🧰 Tools
🪛 GitHub Actions: layer
[error] 50-50: Argument of type "Literal['not a callable']" cannot be assigned to parameter "callback" of type "(dict[Unknown, Unknown]) -> Any" in function "add_session_callback". Type "Literal['not a callable']" is not assignable to type "(dict[Unknown, Unknown]) -> Any" (reportArgumentType)
[error] 57-57: Argument of type "Literal['not a callable']" cannot be assigned to parameter "callback" of type "(Any) -> Any" in function "add_request_response_callback". Type "Literal['not a callable']" is not assignable to type "(Any) -> Any" (reportArgumentType)
🪛 GitHub Actions: pr
[error] 50-50: Argument of type "Literal['not a callable']" cannot be assigned to parameter "callback" of type "(dict[Unknown, Unknown]) -> Any" in function "add_session_callback" (reportArgumentType)
[error] 57-57: Argument of type "Literal['not a callable']" cannot be assigned to parameter "callback" of type "(Any) -> Any" in function "add_request_response_callback" (reportArgumentType)
🤖 Prompt for AI Agents
In `@oocana/tests/test_mainframe_callbacks.py` around lines 45 - 59,
在这两个测试中传入的字符串目的是触发运行时 ValueError，但静态检查器在 CI 报告了 reportArgumentType；在调用
add_session_callback 和 add_request_response_callback
的那两行将非可调用字面量保留并在参数上添加显式类型忽略注释以屏蔽静态检查器（例如在
mainframe.add_session_callback(session_id, "not a callable") 和
mainframe.add_request_response_callback("session", "request", "not a callable")
所在行分别添加 "# type: ignore[reportArgumentType]" 或通用 "# type:
ignore"），以便仅测试运行时行为而不触发类型检查错误。
| def test_multiple_callbacks_for_same_session(self): | ||
| """Test that multiple callbacks can be added for the same session.""" | ||
| session_id = 'test-session' | ||
| callback1 = MagicMock() | ||
| callback2 = MagicMock() | ||
|
|
||
| self.mainframe.add_session_callback(session_id, callback1) | ||
| self.mainframe.add_session_callback(session_id, callback2) | ||
|
|
||
| # Remove first callback, should not unsubscribe yet | ||
| self.mainframe.remove_session_callback(session_id, callback1) | ||
|
|
||
| # Subscribe should have been called only once (for first add) | ||
| call_count_before = self.mock_client.message_callback_add.call_count | ||
|
|
||
| # Remove second callback, should unsubscribe now | ||
| self.mainframe.remove_session_callback(session_id, callback2) | ||
|
|
||
| self.mock_client.unsubscribe.assert_called() |
There was a problem hiding this comment.
补上订阅次数断言，消除未使用变量
call_count_before 未使用，且当前未真正断言“只订阅一次”。建议直接断言调用次数。
🧾 建议修复
- call_count_before = self.mock_client.message_callback_add.call_count
+ self.assertEqual(self.mock_client.message_callback_add.call_count, 1)🧰 Tools
🪛 Ruff (0.14.14)
[error] 114-114: Local variable call_count_before is assigned to but never used
Remove assignment to unused variable call_count_before
(F841)
🤖 Prompt for AI Agents
In `@oocana/tests/test_mainframe_callbacks.py` around lines 101 - 119, In
test_multiple_callbacks_for_same_session, remove the unused local variable
call_count_before and add an explicit assertion that message_callback_add was
called exactly once after adding two callbacks (use
self.mock_client.message_callback_add.assert_called_once() or assertEqual on
call_count) before removing the second callback, then assert unsubscribe was
called after the second removal; reference the test function name and the mocked
methods self.mock_client.message_callback_add and self.mock_client.unsubscribe
and the class methods add_session_callback/remove_session_callback to locate
where to update the test.
leavesster
changed the title
Summary
_add_callback_with_subscription()helper method_remove_callback()helper methodProblem
add_session_callback(),add_request_response_callback()had duplicate patterns for:Solution
Extracted common logic into private helper methods that handle the subscription lifecycle.
Test Plan
tests/test_mainframe_callbacks.py