feat(ecosystem): Implement cross-system issue synchronization#12
feat(ecosystem): Implement cross-system issue synchronization#12ShashankFC wants to merge 1 commit into
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request implements cycle-protection logic for assignment synchronization between Sentry and external integrations. A new Changes
Sequence DiagramsequenceDiagram
participant User
participant Sentry as Sentry<br/>(Group Assignee)
participant SyncUtil as Sync Utility
participant Task as Sync Task
participant Integration as Integration<br/>(Mixin)
participant External as External<br/>Integration
User->>Sentry: Assign group to user
Sentry->>Sentry: Create AssignmentSource<br/>from Sentry origin
Sentry->>SyncUtil: sync_group_assignee_outbound<br/>(assignment_source)
SyncUtil->>Task: Queue sync_assignee_outbound<br/>(assignment_source_dict)
Task->>Task: Parse assignment_source_dict
Task->>Integration: should_sync()<br/>(sync_source)
alt Same Integration (Cycle Detection)
Integration-->>Integration: Detected: source from<br/>this integration
Integration-->>Task: Return False
Task-->>Sentry: ✓ Skip outbound sync
else Different/No Source
Integration-->>Integration: Allowed: safe to sync
Integration-->>Task: Return True
Task->>Integration: sync_assignee_outbound<br/>(assignment_source)
Integration->>External: Create/Update assignment
External-->>Integration: ✓ Acknowledged
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 3
🤖 Fix all issues with AI Agents
In @src/sentry/integrations/services/assignment_source.py:
- Around line 14-18: The queued default is evaluated at import time causing all
AssignmentSource instances to share one timestamp; change the queued field to
use a per-instance factory: replace queued: datetime = timezone.now() with
queued: datetime = field(default_factory=timezone.now) and add the
dataclasses.field import (from dataclasses import dataclass, field) so each
AssignmentSource gets the current time when constructed.
- Around line 27-28: Update the AssignmentSource dataclass serialization: modify
to_dict() to convert the queued datetime field to an ISO 8601 string (e.g.,
queued.isoformat()) instead of returning a raw datetime via asdict(), and update
from_dict() to parse that ISO string back into a datetime (e.g.,
datetime.fromisoformat()) when reconstructing the object; ensure the change
covers the same key name "queued" so sync_assignee_outbound.apply_async()
receives a JSON-serializable dict.
In @tests/sentry/integrations/services/test_assignment_source.py:
- Line 13: Rename the misnamed test method test_from_dict_inalid_data to
test_from_dict_invalid_data in the test class so the name reflects "invalid"
(update the def declaration and any references to it, e.g., test discovery or
direct calls), then run the test suite to ensure no other references or typos
remain; ensure the new name uses the exact spelling test_from_dict_invalid_data.
🧹 Nitpick comments (1)
src/sentry/integrations/services/assignment_source.py (1)
30-35: Consider logging deserialization failures.The method silently returns
Noneon deserialization errors. While this may be intentional for graceful degradation, adding debug logging would improve observability when invalid data is encountered.🔎 Optional improvement
+import logging + +logger = logging.getLogger(__name__) + @classmethod def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: try: return cls(**input_dict) except (ValueError, TypeError): + logger.debug( + "Failed to deserialize AssignmentSource", + extra={"input_dict": input_dict}, + exc_info=True, + ) return None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/sentry/integrations/mixins/issues.pysrc/sentry/integrations/services/assignment_source.pysrc/sentry/integrations/tasks/sync_assignee_outbound.pysrc/sentry/integrations/utils/sync.pysrc/sentry/models/groupassignee.pytests/sentry/integrations/services/test_assignment_source.pytests/sentry/models/test_groupassignee.py
🧰 Additional context used
🧬 Code graph analysis (6)
src/sentry/models/groupassignee.py (2)
src/sentry/integrations/services/assignment_source.py (1)
AssignmentSource(15-35)src/sentry/integrations/utils/sync.py (1)
sync_group_assignee_outbound(123-145)
tests/sentry/models/test_groupassignee.py (2)
src/sentry/integrations/services/assignment_source.py (2)
AssignmentSource(15-35)from_integration(21-25)src/sentry/models/groupassignee.py (2)
GroupAssignee(249-287)assign(130-199)
tests/sentry/integrations/services/test_assignment_source.py (1)
src/sentry/integrations/services/assignment_source.py (3)
AssignmentSource(15-35)from_dict(31-35)to_dict(27-28)
src/sentry/integrations/utils/sync.py (2)
src/sentry/integrations/services/assignment_source.py (3)
AssignmentSource(15-35)from_integration(21-25)to_dict(27-28)src/sentry/models/groupassignee.py (3)
GroupAssignee(249-287)deassign(201-245)assign(130-199)
src/sentry/integrations/tasks/sync_assignee_outbound.py (3)
src/sentry/integrations/services/assignment_source.py (2)
AssignmentSource(15-35)from_dict(31-35)src/sentry/integrations/mixins/issues.py (2)
should_sync(66-67)should_sync(382-394)src/sentry/models/groupassignee.py (1)
assign(130-199)
src/sentry/integrations/mixins/issues.py (4)
src/sentry/integrations/services/assignment_source.py (1)
AssignmentSource(15-35)src/sentry/integrations/jira/integration.py (1)
sync_status_outbound(990-1032)src/sentry/integrations/example/integration.py (1)
sync_status_outbound(161-162)src/sentry/integrations/jira_server/integration.py (1)
sync_status_outbound(1090-1134)
🪛 Ruff (0.14.10)
src/sentry/integrations/services/assignment_source.py
18-18: Do not perform function call timezone.now in dataclass defaults
(RUF009)
src/sentry/integrations/mixins/issues.py
66-66: Unused method argument: attribute
(ARG002)
66-66: Unused method argument: sync_source
(ARG002)
🔇 Additional comments (14)
src/sentry/integrations/utils/sync.py (2)
94-120: LGTM! Proper cycle prevention for inbound sync.The inbound sync paths correctly create
AssignmentSourcefrom the integration for both assign and deassign operations. This enables downstream cycle detection by marking the assignment's origin, preventing infinite sync loops when the same integration processes the outbound sync.
123-145: LGTM! Outbound sync properly propagates assignment source.The function signature is updated to accept an optional
assignment_sourceparameter and correctly serializes it for the async task. The conditional serialization (assignment_source.to_dict() if assignment_source else None) properly handles both cases.src/sentry/models/groupassignee.py (2)
130-199: LGTM! Assignment source properly threaded through assign flow.The
assignmethod correctly accepts an optionalassignment_sourceparameter and forwards it tosync_group_assignee_outbound. The None default maintains backward compatibility with existing callers.
201-246: LGTM! Assignment source properly threaded through deassign flow.The
deassignmethod mirrors theassignmethod pattern, correctly accepting and forwarding the optionalassignment_sourceparameter to enable cycle prevention in outbound sync.tests/sentry/models/test_groupassignee.py (3)
120-178: LGTM! Test properly verifies default assignment_source behavior.The test correctly validates that when no
assignment_sourceis provided toassign, the outbound sync is called withassignment_source=None. The additional assertions thoroughly verify the assignment's side effects (database state, activity creation).
179-236: LGTM! Excellent test coverage for cycle prevention.This test validates the core cycle-prevention logic: when an assignment originates from the same integration that would receive the outbound sync, the sync is correctly suppressed. The test verifies both the prevented sync (
assert_not_called) and the successful local assignment, ensuring cycle prevention doesn't break normal functionality.
238-288: LGTM! Deassign test properly verifies default assignment_source behavior.The test correctly validates that the deassign operation passes
assignment_source=Noneto the outbound sync when no source is provided, maintaining consistency with the assign path.src/sentry/integrations/tasks/sync_assignee_outbound.py (1)
30-61: LGTM! Task properly deserializes and propagates assignment source for cycle detection.The task implementation correctly:
- Accepts
assignment_source_dictas a serializable parameter for async task execution- Deserializes it using
AssignmentSource.from_dictwith proper None handling- Passes the parsed source to both
should_sync(for cycle detection) andsync_assignee_outbound(for audit/context)This completes the cycle-prevention chain, where
should_syncchecks if the source integration matches the target integration (per themixins/issues.pyimplementation) and prevents the sync when they match.src/sentry/integrations/services/assignment_source.py (2)
1-12: LGTM: Clean imports with proper TYPE_CHECKING usage.The TYPE_CHECKING block correctly avoids circular imports while maintaining type safety.
20-25: LGTM: Clean factory method.The
from_integrationmethod correctly extracts the required fields from the integration object.src/sentry/integrations/mixins/issues.py (4)
15-15: LGTM: Proper import of AssignmentSource.
66-67: LGTM: Base class signature updated for API consistency.The added
sync_sourceparameter maintains API consistency with the subclass override inIssueSyncIntegration. The static analysis warning about unused parameters is a false positive—this is the expected pattern for base class method signatures.
382-394: LGTM: Cycle detection logic correctly prevents sync loops.The implementation properly checks if the assignment originates from the same integration and returns
Falseto break the cycle, while still allowing other integrations to propagate changes. The null-safety is correct sinceorg_integrationis verified non-None before accessingintegration_id.
411-418: Verify all implementations accept the newassignment_sourceparameter.The signature change adds
assignment_source: AssignmentSource | None = Noneas an optional parameter to the abstract method. Ensure all concrete implementations ofsync_status_outboundin integration subclasses (Jira, Jira Server, GitHub, etc.) properly accept and propagate this parameter through their sync chains. Since the parameter is optional with a default value, backward compatibility depends on whether existing implementations use**kwargsor explicit parameter lists.
| @dataclass(frozen=True) | ||
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
Critical: Fix mutable default using field(default_factory=...).
Using timezone.now() as a default value evaluates once at class definition time, meaning all AssignmentSource instances will share the same timestamp instead of capturing when each was actually created.
🔎 Proposed fix
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
from django.utils import timezone
if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ queued: datetime = field(default_factory=timezone.now)📝 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() | |
| from dataclasses import asdict, dataclass, field | |
| from datetime import datetime | |
| from typing import TYPE_CHECKING, Any | |
| from django.utils import timezone | |
| if TYPE_CHECKING: | |
| from sentry.integrations.models import Integration | |
| from sentry.integrations.services.integration import RpcIntegration | |
| @dataclass(frozen=True) | |
| class AssignmentSource: | |
| source_name: str | |
| integration_id: int | |
| queued: datetime = field(default_factory=timezone.now) |
🧰 Tools
🪛 Ruff (0.14.10)
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 - 18,
The queued default is evaluated at import time causing all AssignmentSource
instances to share one timestamp; change the queued field to use a per-instance
factory: replace queued: datetime = timezone.now() with queued: datetime =
field(default_factory=timezone.now) and add the dataclasses.field import (from
dataclasses import dataclass, field) so each AssignmentSource gets the current
time when constructed.
| def to_dict(self) -> dict[str, Any]: | ||
| return asdict(self) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the assignment_source.py file to understand the dataclass
head -50 src/sentry/integrations/services/assignment_source.pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 220
🏁 Script executed:
# Search for to_dict() usage in the codebase
rg -n 'to_dict\(\)' --type=py | head -20Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 2364
🏁 Script executed:
# Check task/sync files that might use assignment_source
rg -l 'assignment_source' --type=py | grep -E '(task|sync|queue)'Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 175
🏁 Script executed:
# Look for datetime fields in assignment_source
rg -n 'datetime' src/sentry/integrations/services/assignment_source.pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 154
🏁 Script executed:
# Check sync.py to see how assignment_source is used
cat -n src/sentry/integrations/utils/sync.py | head -100Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 4178
🏁 Script executed:
# Check sync_assignee_outbound.py
cat -n src/sentry/integrations/tasks/sync_assignee_outbound.pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 3043
🏁 Script executed:
# Find where to_dict() is called on assignment_source before passing to task
rg -n -B 5 -A 5 'sync_assignee_outbound' src/sentry/integrations/ --type=py | head -80Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 6403
🏁 Script executed:
# Check the from_dict and to_dict implementations in assignment_source
cat -n src/sentry/integrations/services/assignment_source.pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 1307
🏁 Script executed:
# Verify Celery's JSON serialization behavior - check if there's custom encoder
rg -n 'json_encoder\|JSONEncoder\|default=' src/sentry/ --type=py | grep -E '(celery|task)' | head -10Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check if there are any successful test cases with assignment_source in Celery context
rg -n 'assignment_source.*to_dict\|sync_assignee_outbound' tests/ --type=py | head -20Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check Celery configuration in settings
rg -n 'CELERY\|kombu\|json' src/sentry/conf/ --type=py | grep -i 'json\|serial' | head -15Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Look for tests that actually call sync_assignee_outbound with assignment_source
rg -n -B 3 -A 3 'sync_assignee_outbound\|assignment_source' tests/sentry/integrations/ --type=py | head -100Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check if there's any isoformat or string conversion in from_dict or to_dict elsewhere
rg -n 'isoformat\|strftime\|fromisoformat\|fromtimestamp' src/sentry/integrations/ --type=pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check how other dataclasses handle to_dict with datetime fields
rg -n '@dataclass' src/sentry/ --type=py -A 10 | grep -B 5 -A 5 'datetime' | head -80Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 4851
🏁 Script executed:
# Look for custom serialization patterns in services
rg -n 'def to_dict\|def from_dict' src/sentry/services/ --type=py -A 3 | head -100Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check the actual Celery setup to understand serialization
fd 'celery' src/sentry/conf/ --type pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 251
🏁 Script executed:
# Check if datetime is actually causing issues in task parameters
rg -n 'apply_async.*dict' src/sentry/integrations/ --type=py -B 3 -A 1Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check the sync.py file more carefully around the apply_async call
sed -n '120,150p' src/sentry/integrations/utils/sync.pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 916
🏁 Script executed:
# Look for Celery configuration regarding datetime serialization
rg -n 'datetime\|timestamp' src/sentry/conf/settings/ --type=py | grep -i celeryRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 147
🏁 Script executed:
# Check if there are any actual test failures or usage patterns for assignment_source in tasks
find tests -name '*.py' -exec grep -l 'assignment_source' {} \;Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 184
🏁 Script executed:
# See if there's a custom JSON encoder used for Celery
rg -n 'task_serializer\|result_serializer\|accept_content\|timezone' src/sentry/conf/ --type=py | head -20Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check the assignment_source tests
cat -n tests/sentry/integrations/services/test_assignment_source.pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 1488
🏁 Script executed:
# Check the groupassignee tests
cat -n tests/sentry/models/test_groupassignee.py | head -100Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 4931
🏁 Script executed:
# Find Celery configuration
find src/sentry -name 'celery.py' -o -name '*celery*.py' | head -5Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 249
🏁 Script executed:
# Look for where Celery kombu settings are configured
rg -n 'CELERY\|kombu_parser\|accept_content' src/sentry --type=py | head -20Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check main celery configuration
cat -n src/sentry/celery.py | head -100Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 4151
🏁 Script executed:
# Check celery types
cat -n src/sentry/conf/types/celery.pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 934
🏁 Script executed:
# Check if there's a custom datetime JSON encoder
rg -n 'JSONEncoder\|json.dumps\|json.loads' src/sentry/utils/celery.py -A 3Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Test if the issue actually occurs by looking at how apply_async handles datetime
rg -n 'apply_async' src/sentry/integrations/ --type=py -B 2 -A 2 | grep -A 5 'assignment_source'Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check if there are integration tests that actually run sync_assignee_outbound with assignment_source
rg -n 'sync_assignee_outbound\|sync_group_assignee_outbound' tests/sentry/integrations/ --type=py | head -30Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check Celery broker and serializer configuration
rg -n 'CELERY_TASK_SERIALIZER\|CELERY_ACCEPT_CONTENT\|broker\|serializer' src/sentry --type=py | head -20Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Verify that datetime in to_dict is problematic by checking if there's a custom encoder
python3 << 'EOF'
import json
from datetime import datetime
# Test if datetime is JSON serializable by default
try:
test_dict = {"queued": datetime.now()}
json.dumps(test_dict)
print("datetime is JSON serializable")
except TypeError as e:
print(f"datetime is NOT JSON serializable: {e}")
EOFRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 168
🏁 Script executed:
# Check what Python's json module does with datetime
python3 << 'EOF'
import json
from datetime import datetime
# Simulate what asdict does with datetime
test_dict = {
"source_name": "test",
"integration_id": 123,
"queued": datetime.now()
}
# Try to serialize
try:
json.dumps(test_dict)
print("SUCCESS: datetime serialized")
except TypeError as e:
print(f"FAILURE: {e}")
EOFRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 139
🏁 Script executed:
# Check if there's a custom Celery setup with JSON encoder
find src/sentry -name '*.py' -exec grep -l 'json_encoder\|JSONEncoder\|default=' {} \; | xargs grep -l 'celery\|task' | head -10Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 581
🏁 Script executed:
# Look at settings or configuration for Celery serializer
rg -n 'accept_content\|task_serializer\|result_serializer' src/sentry --type=py | head -20Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
🏁 Script executed:
# Check if there are any workarounds or custom serialization for datetime in the codebase
rg -n 'isoformat\|timestamp\|strptime\|fromisoformat' src/sentry/integrations --type=pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 64
Convert datetime to ISO format string in to_dict() for Celery serialization.
The to_dict() method includes the datetime object directly via asdict(), but Python's json module cannot serialize datetime objects. When sync_assignee_outbound.apply_async() passes this dict to Celery, task serialization will fail. Convert the queued field to an ISO format string in to_dict() and update from_dict() to deserialize it back to a datetime object.
🤖 Prompt for AI Agents
In @src/sentry/integrations/services/assignment_source.py around lines 27 - 28,
Update the AssignmentSource dataclass serialization: modify to_dict() to convert
the queued datetime field to an ISO 8601 string (e.g., queued.isoformat())
instead of returning a raw datetime via asdict(), and update from_dict() to
parse that ISO string back into a datetime (e.g., datetime.fromisoformat()) when
reconstructing the object; ensure the change covers the same key name "queued"
so sync_assignee_outbound.apply_async() receives a JSON-serializable dict.
| result = AssignmentSource.from_dict(data) | ||
| assert result is None | ||
|
|
||
| def test_from_dict_inalid_data(self): |
There was a problem hiding this comment.
Fix typo in test method name.
The method name contains a typo: test_from_dict_inalid_data should be test_from_dict_invalid_data.
🔎 Proposed fix
- def test_from_dict_inalid_data(self):
+ def test_from_dict_invalid_data(self):📝 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.
| def test_from_dict_inalid_data(self): | |
| def test_from_dict_invalid_data(self): |
🤖 Prompt for AI Agents
In @tests/sentry/integrations/services/test_assignment_source.py at line 13,
Rename the misnamed test method test_from_dict_inalid_data to
test_from_dict_invalid_data in the test class so the name reflects "invalid"
(update the def declaration and any references to it, e.g., test discovery or
direct calls), then run the test suite to ensure no other references or typos
remain; ensure the new name uses the exact spelling test_from_dict_invalid_data.
2 participants
Test 7
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/sentry-coderabbit#7