feat(ecosystem): Implement cross-system issue synchronization#9
Conversation
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis change introduces assignment source tracking to prevent synchronization cycles when integrations exchange issue assignments. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant GroupAssignee
participant SyncUtil
participant SyncTask
participant Integration
Client->>GroupAssignee: assign(group, user, assignment_source)
activate GroupAssignee
GroupAssignee->>GroupAssignee: Create/update assignee
GroupAssignee->>SyncUtil: sync_group_assignee_outbound(group, user, assignment_source)
deactivate GroupAssignee
activate SyncUtil
SyncUtil->>SyncTask: Enqueue sync_assignee_outbound(external_issue_id, user_id, assign=True, assignment_source_dict)
deactivate SyncUtil
activate SyncTask
SyncTask->>SyncTask: Parse assignment_source_dict to AssignmentSource
SyncTask->>Integration: should_sync("outbound_assignee", parsed_assignment_source)
alt Source matches integration ID
Integration-->>SyncTask: False (prevent cycle)
SyncTask->>SyncTask: Skip sync
else Source is different or None
Integration-->>SyncTask: True
SyncTask->>Integration: sync_assignee_outbound(external_issue, user, assignment_source)
Integration->>Integration: Update external system
end
deactivate SyncTask
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (3)
tests/sentry/integrations/services/test_assignment_source.py (1)
8-38: Good coverage offrom_dict/to_dict, with minor naming nitsThe tests nicely exercise the happy path and two failure modes for
from_dict, plus basicto_dictshape (includingqueued). Two small nits: the method nametest_from_dict_empty_arraydeals with an empty dict rather than an array, andtest_from_dict_inalid_datahas a typo in “invalid”. Renaming these for clarity would make the test suite a bit easier to scan, but behaviour-wise everything looks solid.src/sentry/integrations/utils/sync.py (1)
94-116: Inbound assign/deassign now tagging source correctlyPassing
assignment_source=AssignmentSource.from_integration(integration)through both the deassign and assign paths wires the inbound webhook’s originating integration into the later outbound checks, which is exactly what’s needed to enable the cycle guard. The usage is type-consistent withfrom_integration(RpcIntegration) andGroupAssignee.deassign/assign’s new parameter. If you care about micro-allocations, you could compute theAssignmentSourceonce per call and reuse it for all affected groups, but functionally this is sound.src/sentry/integrations/mixins/issues.py (1)
65-67: Baseshould_syncnow takessync_sourcebut ignores it (Ruff ARG002)The new
sync_sourceparameter onIssueBasicIntegration.should_synckeeps the base API aligned withIssueSyncIntegration, but bothattributeandsync_sourceare unused here, which is what Ruff is flagging. If Ruff is enforced in CI, consider marking these as intentionally unused (e.g.,_attribute,_sync_source) or adding adel attribute, sync_sourceto quiet ARG002.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/sentry/integrations/mixins/issues.py(4 hunks)src/sentry/integrations/services/assignment_source.py(1 hunks)src/sentry/integrations/tasks/sync_assignee_outbound.py(3 hunks)src/sentry/integrations/utils/sync.py(4 hunks)src/sentry/models/groupassignee.py(5 hunks)tests/sentry/integrations/services/test_assignment_source.py(1 hunks)tests/sentry/models/test_groupassignee.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
src/sentry/integrations/mixins/issues.py
66-66: Unused method argument: attribute
(ARG002)
66-66: Unused method argument: sync_source
(ARG002)
src/sentry/integrations/services/assignment_source.py
18-18: Do not perform function call timezone.now in dataclass defaults
(RUF009)
🔇 Additional comments (6)
src/sentry/integrations/utils/sync.py (1)
123-144: Outbound helper correctly threads optionalAssignmentSourceinto the taskThe updated
sync_group_assignee_outboundsignature and itsassignment_source_dict=assignment_source.to_dict() if assignment_source else Nonehandoff to the task look correct and remain backwards-compatible for existing callers that don’t passassignment_source. This keeps the celery payload JSON-serializable while preserving enough structure forAssignmentSource.from_dictdownstream.src/sentry/integrations/mixins/issues.py (2)
382-394: Cycle guard inIssueSyncIntegration.should_syncis correctly scopedThe updated
should_syncimplementation first validateskeyandself.org_integration, then rejects sync whensync_source.integration_id == self.org_integration.integration_id. That prevents an integration from re‑syncing its own outbound changes while still allowing other integrations to propagate them, which matches the intended “no cycles, but do fan-out” behaviour. The final config lookup remains unchanged, so existing feature flags still control whether sync happens.
411-418: Newassignment_sourceparam onsync_status_outbound—ensure all implementations are updatedExtending
sync_status_outboundwithassignment_source: AssignmentSource | None = Noneat the abstract base level is reasonable, but it does mean every concreteIssueSyncIntegrationimplementation must now accept this parameter. Please double-check (e.g., via a search fordef sync_status_outbound) that all subclasses’ signatures are updated to includeassignment_sourceand**kwargs, so callers passing the new kwarg don’t hitTypeError: got an unexpected keyword argument 'assignment_source'.src/sentry/models/groupassignee.py (1)
130-241: Assign/deassign correctly threadassignment_sourceinto outbound sync without breaking callersAdding the optional
assignment_sourceparameter toGroupAssigneeManager.assignanddeassign, and forwarding it intosync_group_assignee_outbound, cleanly exposes the new metadata while keeping existing call sites intact (all prior uses can still omit the new argument). The feature/gating logic and side-effects (subscriptions, activity, metrics, history) remain unchanged, so the only behavioural difference is that outbound syncs now carry provenance when provided.tests/sentry/models/test_groupassignee.py (1)
7-237: Tests correctly assert newassignment_sourcewiring and cycle-prevention behaviourThe updated outbound assign/unassign tests and the new “matching source integration” test line up well with the new API: they verify that default flows pass
assignment_source=None, that the manager persists assignments and activities as before, and that whenassignment_sourceis derived from the same integration,sync_assignee_outboundonExampleIntegrationis not called. This gives good end-to-end coverage of the provenance threading and cycle guard without overfitting to implementation details.src/sentry/integrations/tasks/sync_assignee_outbound.py (1)
1-61: Task-level wiring ofassignment_sourcelooks correct; confirm install implementations accept the new argsThe task’s new
assignment_source_dictparameter is threaded cleanly: it’s parsed viaAssignmentSource.from_dict, passed intoinstallation.should_sync("outbound_assignee", parsed_assignment_source), and then intoinstallation.sync_assignee_outbound(..., assignment_source=parsed_assignment_source). Existing calls without this extra kwarg stay valid thanks to the default, and malformed dicts safely degrade toNone. Please double-check that all integration installations providingshould_syncandsync_assignee_outboundnow accept the extrasync_source/assignment_sourceparameters (ideally with a**kwargsfallback) so these new calls can’t triggerTypeErrorat runtime.
| @dataclass(frozen=True) | ||
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() | ||
|
|
||
| @classmethod | ||
| def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource: | ||
| return AssignmentSource( | ||
| source_name=integration.name, | ||
| integration_id=integration.id, | ||
| ) | ||
|
|
||
| def to_dict(self) -> dict[str, Any]: | ||
| return asdict(self) | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): | ||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
Fix dataclass default for queued to avoid a shared timestamp; consider making from_dict more robust
Right now queued: datetime = timezone.now() is evaluated once at import, so every AssignmentSource instance created without an explicit queued will share the same timestamp. That’s almost certainly not what you want for a “queued” field and is what Ruff’s RUF009 is warning about. You can make this per-instance by using a default factory:
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
@@
- queued: datetime = timezone.now()
+ queued: datetime = field(default_factory=timezone.now)Separately, from_dict currently does a raw cls(**input_dict), which will return None if any extra or malformed keys are present. If this struct might cross version boundaries, you may want to pre-filter to known keys ({"source_name", "integration_id", "queued"}) before constructing, so older or newer producers don’t break deserialization just because of an unexpected field.
I'll verify the review comment by inspecting the file, checking usage patterns, and confirming the technical accuracy of the suggestions.Now let me verify the file exists and check the actual imports and usage patterns:Now let me check the actual file and verify implementation details:
<
🏁 Script executed:
#!/bin/bash
# Check if the file exists and inspect its current state
if [ -f "src/sentry/integrations/services/assignment_source.py" ]; then
echo "=== File exists, showing content ==="
cat "src/sentry/integrations/services/assignment_source.py"
else
echo "File not found at expected path; searching for it..."
find . -name "assignment_source.py" -type f 2>/dev/null | head -5
fiLength of output: 1017
🏁 Script executed:
#!/bin/bash
# Search for usages of AssignmentSource to understand from_dict behavior
echo "=== Searching for AssignmentSource usages ==="
rg -n "AssignmentSource" --type py -A 3 -B 1 | head -60Length of output: 4932
🏁 Script executed:
#!/bin/bash
# Check if RUF009 is configured to be raised in the codebase
echo "=== Checking for ruff configuration ==="
if [ -f "pyproject.toml" ]; then
grep -A 20 "\[tool.ruff" pyproject.toml | grep -E "(RUF009|RUF008|extend-immutable-calls)" | head -20
fi
if [ -f ".ruff.toml" ]; then
cat ".ruff.toml"
fi
if [ -f "ruff.toml" ]; then
cat "ruff.toml"
fiLength of output: 936
Let me check the test file to understand the exact expectations for from_dict:
🏁 Script executed:
#!/bin/bash
# Inspect the full test file to understand from_dict behavior
cat "tests/sentry/integrations/services/test_assignment_source.py"Length of output: 1140
🏁 Script executed:
#!/bin/bash
# Check what's imported in assignment_source.py specifically
head -20 "src/sentry/integrations/services/assignment_source.py"Length of output: 477
Fix dataclass default for queued to use per-instance factory
The queued: datetime = timezone.now() default is evaluated once at module import, so every AssignmentSource instance created without an explicit queued value shares the same timestamp. This is a semantic bug and is what Ruff's RUF009 warns about. Fix it by using a default factory:
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
@@
- queued: datetime = timezone.now()
+ queued: datetime = field(default_factory=timezone.now)Separately, from_dict currently rejects any dict with unexpected keys or missing required fields by catching TypeError and returning None. If this struct crosses version boundaries (e.g., older producers sending unknown fields), consider pre-filtering to known keys ({"source_name", "integration_id", "queued"}) before unpacking to improve forward/backward compatibility—though the current behavior is deliberate per the tests.
📝 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.
| @dataclass(frozen=True) | |
| class AssignmentSource: | |
| source_name: str | |
| integration_id: int | |
| queued: datetime = timezone.now() | |
| @classmethod | |
| def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource: | |
| return AssignmentSource( | |
| source_name=integration.name, | |
| integration_id=integration.id, | |
| ) | |
| def to_dict(self) -> dict[str, Any]: | |
| return asdict(self) | |
| @classmethod | |
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | |
| try: | |
| return cls(**input_dict) | |
| except (ValueError, TypeError): | |
| return None | |
| from dataclasses import asdict, dataclass, field | |
| @dataclass(frozen=True) | |
| class AssignmentSource: | |
| source_name: str | |
| integration_id: int | |
| queued: datetime = field(default_factory=timezone.now) | |
| @classmethod | |
| def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource: | |
| return AssignmentSource( | |
| source_name=integration.name, | |
| integration_id=integration.id, | |
| ) | |
| def to_dict(self) -> dict[str, Any]: | |
| return asdict(self) | |
| @classmethod | |
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | |
| try: | |
| return cls(**input_dict) | |
| except (ValueError, TypeError): | |
| return None |
🧰 Tools
🪛 Ruff (0.14.5)
18-18: Do not perform function call timezone.now in dataclass defaults
(RUF009)
🤖 Prompt for AI Agents
In src/sentry/integrations/services/assignment_source.py around lines 14 to 35,
the dataclass uses queued: datetime = timezone.now() which evaluates once at
import and must be changed to use a per-instance factory (use dataclasses.field
with default_factory=timezone.now) so each AssignmentSource gets a fresh
timestamp; additionally, if you want forward/backward compatibility for
from_dict, pre-filter input_dict to only the known keys {"source_name",
"integration_id", "queued"} before unpacking to avoid rejecting dicts with extra
fields (keep the existing exception handling for missing/invalid types if
desired).
2 participants
Test 7
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Replicated from ai-code-review-evaluation/sentry-coderabbit#7