Repository navigation
feat(runtime): add topic_router to DispatchResultApplier for per-event-type topic routing - #810
Conversation
…t-type topic routing Adds `topic_router: dict[str, str] | None` param to DispatchResultApplier.__init__. When provided, each output event is published to its per-type declared Kafka topic instead of the single `output_topic` fallback. Events not in the map fall back to `output_topic` (backward-compatible). The router is built from contract.yaml published_events via build_topic_router_from_contract() (wired in OMN-4883). Fixes: OMN-4881 Epic: OMN-4880
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/omnibase_infra/runtime/service_dispatch_result_applier.py (1)
136-166: Defensively copytopic_routerin__init__.Storing the passed dict directly allows external mutation after construction, which can change routing behavior at runtime unexpectedly.
Suggested patch
- self._topic_router: dict[str, str] = topic_router or {} + self._topic_router: dict[str, str] = ( + dict(topic_router) if topic_router is not None else {} + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/runtime/service_dispatch_result_applier.py` around lines 136 - 166, The constructor stores the passed topic_router reference directly (topic_router -> self._topic_router), which allows external mutation to affect routing; defensively copy the mapping in __init__ so self._topic_router is an independent dict (e.g., use a shallow copy when topic_router is not None) and keep the existing fallback to an empty dict when topic_router is None; update the assignment that sets self._topic_router in ServiceDispatchResultApplier.__init__ to create that copy.tests/unit/runtime/test_service_dispatch_result_applier.py (2)
423-468: Strengthen topic-router tests with single-call assertions.Each test currently inspects only the last publish call; asserting exactly one publish call will catch accidental duplicate publishes.
Suggested patch
async def test_topic_router_routes_known_event_to_declared_topic() -> None: @@ result = _make_result_with([_make_accepted_event()]) await applier.apply(result) + bus.publish_envelope.assert_awaited_once() published_topic = bus.publish_envelope.call_args.kwargs["topic"] assert published_topic == "onex.evt.platform.node-registration-accepted.v1" @@ async def test_topic_router_falls_back_for_unknown_event_type() -> None: @@ result = _make_result_with([_make_accepted_event()]) await applier.apply(result) + bus.publish_envelope.assert_awaited_once() published_topic = bus.publish_envelope.call_args.kwargs["topic"] assert published_topic == "responses" @@ async def test_no_topic_router_uses_output_topic() -> None: @@ result = _make_result_with([_make_accepted_event()]) await applier.apply(result) + bus.publish_envelope.assert_awaited_once() published_topic = bus.publish_envelope.call_args.kwargs["topic"] assert published_topic == "responses"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/runtime/test_service_dispatch_result_applier.py` around lines 423 - 468, Add an assertion that the mock publish method was called exactly once in each of the three tests (test_topic_router_routes_known_event_to_declared_topic, test_topic_router_falls_back_for_unknown_event_type, test_no_topic_router_uses_output_topic) to catch unexpected duplicate publishes: after awaiting applier.apply(result) assert the AsyncMock for event_bus.publish_envelope was called exactly once (use bus.publish_envelope.assert_awaited_once() or bus.publish_envelope.assert_called_once() depending on whether publish_envelope is awaited) before inspecting call_args.kwargs["topic"].
412-413: Removetype: ignore[type-arg]by typing theeventsparameter aslist[BaseModel].The
eventsparameter can be precisely typed to match theModelDispatchResult.output_eventsfield annotation (list[BaseModel]), eliminating the need for the ignore comment.BaseModelis already imported in this file.Change:
def _make_result_with(events: list) -> ModelDispatchResult: # type: ignore[type-arg]to:
def _make_result_with(events: list[BaseModel]) -> ModelDispatchResult:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/runtime/test_service_dispatch_result_applier.py` around lines 412 - 413, The helper function _make_result_with currently uses a broad events parameter and silences a typing error with "type: ignore[type-arg]"; change the signature to type events as list[BaseModel] to match ModelDispatchResult.output_events and remove the ignore. Update the function definition for _make_result_with(events: list[BaseModel]) -> ModelDispatchResult so callers and the ModelDispatchResult construction use the correct typed list of BaseModel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/omnibase_infra/runtime/service_dispatch_result_applier.py`:
- Around line 136-166: The constructor stores the passed topic_router reference
directly (topic_router -> self._topic_router), which allows external mutation to
affect routing; defensively copy the mapping in __init__ so self._topic_router
is an independent dict (e.g., use a shallow copy when topic_router is not None)
and keep the existing fallback to an empty dict when topic_router is None;
update the assignment that sets self._topic_router in
ServiceDispatchResultApplier.__init__ to create that copy.
In `@tests/unit/runtime/test_service_dispatch_result_applier.py`:
- Around line 423-468: Add an assertion that the mock publish method was called
exactly once in each of the three tests
(test_topic_router_routes_known_event_to_declared_topic,
test_topic_router_falls_back_for_unknown_event_type,
test_no_topic_router_uses_output_topic) to catch unexpected duplicate publishes:
after awaiting applier.apply(result) assert the AsyncMock for
event_bus.publish_envelope was called exactly once (use
bus.publish_envelope.assert_awaited_once() or
bus.publish_envelope.assert_called_once() depending on whether publish_envelope
is awaited) before inspecting call_args.kwargs["topic"].
- Around line 412-413: The helper function _make_result_with currently uses a
broad events parameter and silences a typing error with "type:
ignore[type-arg]"; change the signature to type events as list[BaseModel] to
match ModelDispatchResult.output_events and remove the ignore. Update the
function definition for _make_result_with(events: list[BaseModel]) ->
ModelDispatchResult so callers and the ModelDispatchResult construction use the
correct typed list of BaseModel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66ad1cfe-d8ae-4b5b-be84-b6e3f57ceff9
📒 Files selected for processing (2)
src/omnibase_infra/runtime/service_dispatch_result_applier.pytests/unit/runtime/test_service_dispatch_result_applier.py
Summary
topic_router: dict[str, str] | Noneparam toDispatchResultApplier.__init__output_topicfallbackoutput_topic(backward-compatible — no existing callers broken)contract.yamlpublished_eventsviabuild_topic_router_from_contract()(wired in OMN-4883)Test plan
Tickets
Summary by CodeRabbit
Release Notes
New Features
Tests