feat(management): add outbox event translator (AIHCM-184)#291
Conversation
…-184) Add ManagementEventTranslator that translates 7 Management domain events to SpiceDB relationship operations: - KnowledgeGraphCreated → write workspace + tenant relationships - KnowledgeGraphDeleted → delete workspace, tenant, and filter-delete admin/editor/viewer grants (5 operations) - DataSourceCreated → write knowledge_graph + tenant relationships - DataSourceDeleted → delete knowledge_graph + tenant relationships - Updated/SyncRequested events → no-op (registered for validation) Register ManagementEventTranslator in CompositeEventHandler at startup. 34 translator unit tests covering all event types, operation ordering, enum values, and error cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThis change introduces a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/tests/unit/management/infrastructure/outbox/test_translator.py (1)
617-625: Consolidate duplicated unknown-event assertions into one parametrized test.These two tests validate the same behavior/message path and can be merged to reduce redundancy while keeping coverage intact.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 630-638
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/tests/unit/management/infrastructure/outbox/test_translator.py` around lines 617 - 625, Consolidate the two duplicate tests that assert unknown-event behavior into a single parametrized pytest test: replace the separate test_raises_value_error_for_unknown_event_type and the similar test at lines around 630-638 with one test using `@pytest.mark.parametrize`("event_type", ["UnknownEvent", "AnotherUnknownEvent"]) that instantiates ManagementEventTranslator and asserts translator.translate(event_type, {}) raises ValueError and that "Unknown event type" is in the exception message; reference the ManagementEventTranslator class and translate method when locating code to change and keep the assertion message check unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/management/infrastructure/outbox/translator.py`:
- Line 117: The translator currently returns handler(payload) directly which
allows missing-field KeyError exceptions from handler internals to leak; wrap
the call to handler (the invocation returning at Line 117) in a try/except that
catches KeyError (and optionally TypeError for malformed payload shapes) and
re-raise a ValueError with a clear message preserving the original exception as
the cause so malformed payloads are normalized at the translation boundary
(i.e., convert KeyError -> ValueError before propagating from the translator
function that calls handler).
---
Nitpick comments:
In `@src/api/tests/unit/management/infrastructure/outbox/test_translator.py`:
- Around line 617-625: Consolidate the two duplicate tests that assert
unknown-event behavior into a single parametrized pytest test: replace the
separate test_raises_value_error_for_unknown_event_type and the similar test at
lines around 630-638 with one test using `@pytest.mark.parametrize`("event_type",
["UnknownEvent", "AnotherUnknownEvent"]) that instantiates
ManagementEventTranslator and asserts translator.translate(event_type, {})
raises ValueError and that "Unknown event type" is in the exception message;
reference the ManagementEventTranslator class and translate method when locating
code to change and keep the assertion message check unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee8b751e-8cba-409f-b257-964368d5c1f1
📒 Files selected for processing (4)
src/api/main.pysrc/api/management/infrastructure/outbox/__init__.pysrc/api/management/infrastructure/outbox/translator.pysrc/api/tests/unit/management/infrastructure/outbox/test_translator.py
| if not handler: | ||
| raise ValueError(f"No handler for event: {event_type}") | ||
|
|
||
| return handler(payload) |
There was a problem hiding this comment.
Normalize malformed payload failures to ValueError at the translation boundary.
At Line 117, missing required fields currently bubble as KeyError from handler internals, which makes outbox failure handling/error classification inconsistent with the explicit ValueError contract used for unsupported events.
Proposed fix
def translate(
self,
event_type: str,
payload: dict[str, Any],
) -> list[SpiceDBOperation]:
@@
- return handler(payload)
+ try:
+ return handler(payload)
+ except KeyError as exc:
+ missing_key = exc.args[0]
+ raise ValueError(
+ f"Invalid payload for event {event_type}: "
+ f"missing required field '{missing_key}'"
+ ) from excAs per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return handler(payload) | |
| try: | |
| return handler(payload) | |
| except KeyError as exc: | |
| missing_key = exc.args[0] | |
| raise ValueError( | |
| f"Invalid payload for event {event_type}: " | |
| f"missing required field '{missing_key}'" | |
| ) from exc |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/management/infrastructure/outbox/translator.py` at line 117, The
translator currently returns handler(payload) directly which allows
missing-field KeyError exceptions from handler internals to leak; wrap the call
to handler (the invocation returning at Line 117) in a try/except that catches
KeyError (and optionally TypeError for malformed payload shapes) and re-raise a
ValueError with a clear message preserving the original exception as the cause
so malformed payloads are normalized at the translation boundary (i.e., convert
KeyError -> ValueError before propagating from the translator function that
calls handler).
1 participant
Summary
ManagementEventTranslatortranslating 7 domain events to SpiceDB operationsCompositeEventHandlerat startup alongside IAMEvent → SpiceDB mappings
Test plan
"handler": "management", "event_count": 7🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests