Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/sentry/integrations/mixins/issues.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from sentry.eventstore.models import GroupEvent
from sentry.integrations.base import IntegrationInstallation
from sentry.integrations.models.external_issue import ExternalIssue
from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.integrations.services.integration import integration_service
from sentry.integrations.tasks.sync_status_inbound import (
sync_status_inbound as sync_status_inbound_task,
Expand Down Expand Up @@ -62,7 +63,7 @@ def from_resolve_unresolve(


class IssueBasicIntegration(IntegrationInstallation, ABC):
def should_sync(self, attribute):
def should_sync(self, attribute, sync_source: AssignmentSource | None = None):
return False

def get_group_title(self, group, event, **kwargs):
Expand Down Expand Up @@ -378,10 +379,17 @@ class IssueSyncIntegration(IssueBasicIntegration, ABC):
outbound_assignee_key: ClassVar[str | None] = None
inbound_assignee_key: ClassVar[str | None] = None

def should_sync(self, attribute: str) -> bool:
def should_sync(self, attribute: str, sync_source: AssignmentSource | None = None) -> bool:
key = getattr(self, f"{attribute}_key", None)
if key is None or self.org_integration is None:
return False

# Check that the assignment source isn't this same integration in order to
# prevent sync-cycles from occurring. This should still allow other
# integrations to propagate changes outward.
if sync_source and sync_source.integration_id == self.org_integration.integration_id:
return False

value: bool = self.org_integration.config.get(key, False)
return value

Expand All @@ -400,7 +408,14 @@ def sync_assignee_outbound(
raise NotImplementedError

@abstractmethod
def sync_status_outbound(self, external_issue, is_resolved, project_id, **kwargs):
def sync_status_outbound(
self,
external_issue,
is_resolved,
project_id,
assignment_source: AssignmentSource | None = None,
**kwargs,
):
"""
Propagate a sentry issue's status to a linked issue's status.
"""
Expand Down
35 changes: 35 additions & 0 deletions src/sentry/integrations/services/assignment_source.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
from __future__ import annotations

from dataclasses import asdict, dataclass
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 AI 代码审查发现问题

📋 问题概述

可变默认参数陷阱

line 17 处 queued: datetime = timezone.now() 在类定义时即求值,导致所有未显式传入 queued 的实例共享同一时间戳(类定义时的时间),而非实例创建时间。当通过 line 21-24 from_integration() 创建实例时,若调用方未提供 queued 参数,记录的时间将永远是模块首次加载时的时间,造成分配来源时间记录错误。

📍 问题详情

🟡 问题 1 | 严重程度: MEDIUM | 行号: 17

💬 详细说明:

  • 可变默认参数陷阱
  • line 17 处 queued: datetime = timezone.now() 在类定义时即求值,导致所有未显式传入 queued 的实例共享同一时间戳(类定义时的时间),而非实例创建时间。当通过 line 21-24 from_integration() 创建实例时,若调用方未提供 queued 参数,记录的时间将永远是模块首次加载时的时间,造成分配来源时间记录错误。

📝 问题代码:

    queued: datetime = timezone.now()

💡 修复建议:

使用 dataclasses.field(default_factory=...) 替代直接默认值,确保每次实例化时动态获取当前时间。将 timezone.now() 改为 field(default_factory=timezone.now),使 queued 在实例创建时才求值。

✅ 修复示例:

    queued: datetime = field(default_factory=timezone.now)

🔗 参考链接

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
19 changes: 16 additions & 3 deletions src/sentry/integrations/tasks/sync_assignee_outbound.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
from typing import Any

from sentry import analytics, features
from sentry.integrations.models.external_issue import ExternalIssue
from sentry.integrations.models.integration import Integration
from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.integrations.services.integration import integration_service
from sentry.models.organization import Organization
from sentry.silo.base import SiloMode
Expand All @@ -24,7 +27,12 @@
Organization.DoesNotExist,
)
)
def sync_assignee_outbound(external_issue_id: int, user_id: int | None, assign: bool) -> None:
def sync_assignee_outbound(
external_issue_id: int,
user_id: int | None,
assign: bool,
assignment_source_dict: dict[str, Any] | None = None,
) -> None:
# Sync Sentry assignee to an external issue.
external_issue = ExternalIssue.objects.get(id=external_issue_id)

Expand All @@ -42,10 +50,15 @@ def sync_assignee_outbound(external_issue_id: int, user_id: int | None, assign:
):
return

if installation.should_sync("outbound_assignee"):
parsed_assignment_source = (
AssignmentSource.from_dict(assignment_source_dict) if assignment_source_dict else None
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 AI 代码审查发现问题

📋 问题概述

向后兼容性风险:方法签名不匹配

line 48-50 处仅检查 should_syncsync_assignee_outbound 方法是否存在,但 line 55 和 line 59 处调用这些方法时传入了新增的 assignment_source 参数。若某些 integration installation 的旧实现仅支持原有签名(如 should_sync(self, sync_type)sync_assignee_outbound(self, external_issue, user, assign)),调用将抛出 TypeError 导致任务失败。

当调用方传入 assignment_source_dict 且 installation 为旧实现时,line 55 的 should_sync 调用会因参数过多而崩溃。

📍 问题详情

🟡 问题 1 | 严重程度: MEDIUM | 行号: 55

💬 详细说明:

  • 向后兼容性风险:方法签名不匹配
  • line 48-50 处仅检查 should_syncsync_assignee_outbound 方法是否存在,但 line 55 和 line 59 处调用这些方法时传入了新增的 assignment_source 参数。若某些 integration installation 的旧实现仅支持原有签名(如 should_sync(self, sync_type)sync_assignee_outbound(self, external_issue, user, assign)),调用将抛出 TypeError 导致任务失败。
  • 当调用方传入 assignment_source_dict 且 installation 为旧实现时,line 55 的 should_sync 调用会因参数过多而崩溃。

📝 问题代码:

    if installation.should_sync("outbound_assignee", parsed_assignment_source):

💡 修复建议:

在调用前检查方法签名是否支持新参数,或采用安全降级策略:

  1. 使用 inspect.signature 检查 should_syncsync_assignee_outbound 是否接受 assignment_source 参数
  2. 若不支持,则降级为旧版调用(不传 assignment_source 参数)
  3. 或在 hasattr 检查后增加版本/能力检查,确保 installation 支持新接口

具体实现:在 line 52-54 后增加签名检查逻辑,对不支持新参数的 installation 使用兼容模式调用。

✅ 修复示例:

    # Check if installation supports assignment_source parameter
    import inspect
    
    should_sync_supports_source = False
    sync_assignee_supports_source = False
    
    try:
        should_sync_sig = inspect.signature(installation.should_sync)
        should_sync_supports_source = len(should_sync_sig.parameters) > 2  # self + sync_type + source
    except (ValueError, TypeError):
        pass
    
    try:
        sync_sig = inspect.signature(installation.sync_assignee_outbound)
        sync_assignee_supports_source = 'assignment_source' in sync_sig.parameters
    except (ValueError, TypeError):
        pass
    
    parsed_assignment_source = (
        AssignmentSource.from_dict(assignment_source_dict) if assignment_source_dict else None
    )
    
    if should_sync_supports_source:
        should_sync_result = installation.should_sync("outbound_assignee", parsed_assignment_source)
    else:
        should_sync_result = installation.should_sync("outbound_assignee")
    
    if should_sync_result:
        user = user_service.get_user(user_id) if user_id else None
        if sync_assignee_supports_source:
            installation.sync_assignee_outbound(
                external_issue, user, assign=assign, assignment_source=parsed_assignment_source
            )
        else:
            installation.sync_assignee_outbound(external_issue, user, assign=assign)

🔗 参考链接

if installation.should_sync("outbound_assignee", parsed_assignment_source):
# Assume unassign if None.
user = user_service.get_user(user_id) if user_id else None
installation.sync_assignee_outbound(external_issue, user, assign=assign)
installation.sync_assignee_outbound(
external_issue, user, assign=assign, assignment_source=parsed_assignment_source
)
analytics.record(
"integration.issue.assignee.synced",
provider=integration.provider,
Expand Down
29 changes: 25 additions & 4 deletions src/sentry/integrations/utils/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from typing import TYPE_CHECKING

from sentry import features
from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.integrations.services.integration import integration_service
from sentry.integrations.tasks.sync_assignee_outbound import sync_assignee_outbound
from sentry.models.group import Group
Expand Down Expand Up @@ -92,7 +93,11 @@ def sync_group_assignee_inbound(

if not assign:
for group in affected_groups:
GroupAssignee.objects.deassign(group)
GroupAssignee.objects.deassign(
group,
assignment_source=AssignmentSource.from_integration(integration),
)

return affected_groups

users = user_service.get_many_by_email(emails=[email], is_verified=True)
Expand All @@ -104,14 +109,23 @@ def sync_group_assignee_inbound(
user_id = get_user_id(projects_by_user, group)
user = users_by_id.get(user_id)
if user:
GroupAssignee.objects.assign(group, user)
GroupAssignee.objects.assign(
group,
user,
assignment_source=AssignmentSource.from_integration(integration),
)
groups_assigned.append(group)
else:
logger.info("assignee-not-found-inbound", extra=log_context)
return groups_assigned


def sync_group_assignee_outbound(group: Group, user_id: int | None, assign: bool = True) -> None:
def sync_group_assignee_outbound(
group: Group,
user_id: int | None,
assign: bool = True,
assignment_source: AssignmentSource | None = None,
) -> None:
from sentry.models.grouplink import GroupLink

external_issue_ids = GroupLink.objects.filter(
Expand All @@ -120,5 +134,12 @@ def sync_group_assignee_outbound(group: Group, user_id: int | None, assign: bool

for external_issue_id in external_issue_ids:
sync_assignee_outbound.apply_async(
kwargs={"external_issue_id": external_issue_id, "user_id": user_id, "assign": assign}
kwargs={
"external_issue_id": external_issue_id,
"user_id": user_id,
"assign": assign,
"assignment_source_dict": assignment_source.to_dict()
if assignment_source
else None,
}
)
11 changes: 9 additions & 2 deletions src/sentry/models/groupassignee.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from sentry.db.models import FlexibleForeignKey, Model, region_silo_model, sane_repr
from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
from sentry.db.models.manager.base import BaseManager
from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.models.grouphistory import GroupHistoryStatus, record_group_history
from sentry.models.groupowner import GroupOwner
from sentry.models.groupsubscription import GroupSubscription
Expand Down Expand Up @@ -134,6 +135,7 @@ def assign(
create_only: bool = False,
extra: dict[str, str] | None = None,
force_autoassign: bool = False,
assignment_source: AssignmentSource | None = None,
):
from sentry.integrations.utils import sync_group_assignee_outbound
from sentry.models.activity import Activity
Expand Down Expand Up @@ -187,7 +189,9 @@ def assign(
if assignee_type == "user" and features.has(
"organizations:integrations-issue-sync", group.organization, actor=acting_user
):
sync_group_assignee_outbound(group, assigned_to.id, assign=True)
sync_group_assignee_outbound(
group, assigned_to.id, assign=True, assignment_source=assignment_source
)

if not created: # aka re-assignment
self.remove_old_assignees(group, assignee, assigned_to_id, assignee_type)
Expand All @@ -200,6 +204,7 @@ def deassign(
acting_user: User | RpcUser | None = None,
assigned_to: Team | RpcUser | None = None,
extra: dict[str, str] | None = None,
assignment_source: AssignmentSource | None = None,
) -> None:
from sentry.integrations.utils import sync_group_assignee_outbound
from sentry.models.activity import Activity
Expand Down Expand Up @@ -230,7 +235,9 @@ def deassign(
if features.has(
"organizations:integrations-issue-sync", group.organization, actor=acting_user
):
sync_group_assignee_outbound(group, None, assign=False)
sync_group_assignee_outbound(
group, None, assign=False, assignment_source=assignment_source
)

issue_unassigned.send_robust(
project=group.project, group=group, user=acting_user, sender=self.__class__
Expand Down
38 changes: 38 additions & 0 deletions tests/sentry/integrations/services/test_assignment_source.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
from typing import Any

from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.testutils.cases import TestCase


class TestAssignmentSource(TestCase):
def test_from_dict_empty_array(self):
data: dict[str, Any] = {}
result = AssignmentSource.from_dict(data)
assert result is None

def test_from_dict_inalid_data(self):
data = {
"foo": "bar",
}

result = AssignmentSource.from_dict(data)
assert result is None

def test_from_dict_valid_data(self):
data = {"source_name": "foo-source", "integration_id": 123}

result = AssignmentSource.from_dict(data)
assert result is not None
assert result.source_name == "foo-source"
assert result.integration_id == 123

def test_to_dict(self):
source = AssignmentSource(
source_name="foo-source",
integration_id=123,
)

result = source.to_dict()
assert result.get("queued") is not None
assert result.get("source_name") == "foo-source"
assert result.get("integration_id") == 123
74 changes: 71 additions & 3 deletions tests/sentry/models/test_groupassignee.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from sentry.integrations.example.integration import ExampleIntegration
from sentry.integrations.models.external_issue import ExternalIssue
from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.integrations.utils import sync_group_assignee_inbound
from sentry.models.activity import Activity
from sentry.models.groupassignee import GroupAssignee
Expand Down Expand Up @@ -148,12 +149,77 @@ def test_assignee_sync_outbound_assign(self, mock_sync_assignee_outbound):

with self.feature({"organizations:integrations-issue-sync": True}):
with self.tasks():
GroupAssignee.objects.assign(self.group, self.user)
GroupAssignee.objects.assign(
self.group,
self.user,
)

mock_sync_assignee_outbound.assert_called_with(
external_issue, user_service.get_user(self.user.id), assign=True
external_issue,
user_service.get_user(self.user.id),
assign=True,
assignment_source=None,
)

assert GroupAssignee.objects.filter(
project=self.group.project,
group=self.group,
user_id=self.user.id,
team__isnull=True,
).exists()

activity = Activity.objects.get(
project=self.group.project, group=self.group, type=ActivityType.ASSIGNED.value
)

assert activity.data["assignee"] == str(self.user.id)
assert activity.data["assigneeEmail"] == self.user.email
assert activity.data["assigneeType"] == "user"

@mock.patch.object(ExampleIntegration, "sync_assignee_outbound")
def test_assignee_sync_outbound_assign_with_matching_source_integration(
self, mock_sync_assignee_outbound
):
group = self.group
integration = self.create_integration(
organization=group.organization,
external_id="123456",
provider="example",
oi_params={
"config": {
"sync_comments": True,
"sync_status_outbound": True,
"sync_status_inbound": True,
"sync_assignee_outbound": True,
"sync_assignee_inbound": True,
}
},
)

external_issue = ExternalIssue.objects.create(
organization_id=group.organization.id, integration_id=integration.id, key="APP-123"
)

GroupLink.objects.create(
group_id=group.id,
project_id=group.project_id,
linked_type=GroupLink.LinkedType.issue,
linked_id=external_issue.id,
relationship=GroupLink.Relationship.references,
)

with self.feature({"organizations:integrations-issue-sync": True}):
with self.tasks():
# Assert that we don't perform an outbound assignment if
# the source of the assignment is the same target integration
GroupAssignee.objects.assign(
self.group,
self.user,
assignment_source=AssignmentSource.from_integration(integration),
)

mock_sync_assignee_outbound.assert_not_called()

assert GroupAssignee.objects.filter(
project=self.group.project,
group=self.group,
Expand Down Expand Up @@ -205,7 +271,9 @@ def test_assignee_sync_outbound_unassign(self, mock_sync_assignee_outbound):
with self.feature({"organizations:integrations-issue-sync": True}):
with self.tasks():
GroupAssignee.objects.deassign(self.group)
mock_sync_assignee_outbound.assert_called_with(external_issue, None, assign=False)
mock_sync_assignee_outbound.assert_called_with(
external_issue, None, assign=False, assignment_source=None
)

assert not GroupAssignee.objects.filter(
project=self.group.project,
Expand Down