-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(aci): Try our best in Project.transfer_to #103704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
| import sentry_sdk | ||
| from django.conf import settings | ||
| from django.db import IntegrityError, models, router, transaction | ||
| from django.db.models import Q, QuerySet, Subquery | ||
| from django.db.models import Count, Q, QuerySet, Subquery | ||
| from django.db.models.signals import pre_delete | ||
| from django.utils import timezone | ||
| from django.utils.http import urlencode | ||
|
|
@@ -55,6 +55,7 @@ | |
| if TYPE_CHECKING: | ||
| from sentry.models.options.project_option import ProjectOptionManager | ||
| from sentry.models.options.project_template_option import ProjectTemplateOptionManager | ||
| from sentry.models.organization import Organization | ||
| from sentry.users.models.user import User | ||
|
|
||
| # NOTE: | ||
|
|
@@ -499,7 +500,7 @@ def get_audit_log_data(self): | |
| def get_full_name(self): | ||
| return self.slug | ||
|
|
||
| def transfer_to(self, organization): | ||
| def transfer_to(self, organization: Organization) -> None: | ||
| from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion | ||
| from sentry.incidents.models.alert_rule import AlertRule | ||
| from sentry.integrations.models.external_issue import ExternalIssue | ||
|
|
@@ -514,6 +515,7 @@ def transfer_to(self, organization): | |
| from sentry.models.rule import Rule | ||
| from sentry.monitors.models import Monitor, MonitorEnvironment, MonitorStatus | ||
| from sentry.snuba.models import SnubaQuery | ||
| from sentry.workflow_engine.models import DataConditionGroup, DataSource, Detector, Workflow | ||
|
|
||
| old_org_id = self.organization_id | ||
| org_changed = old_org_id != organization.id | ||
|
|
@@ -637,6 +639,97 @@ def transfer_to(self, organization): | |
|
|
||
| AlertRule.objects.fetch_for_project(self).update(organization=organization) | ||
|
|
||
| # Transfer DataSource, Workflow, and DataConditionGroup objects for Detectors attached to this project. | ||
| # * DataSources link detectors to their data sources (QuerySubscriptions, Monitors, etc.). | ||
| # * Workflows are connected to detectors and define what actions to take. | ||
| # * DataConditionGroups are connected to workflows (unique 1:1 via WorkflowDataConditionGroup). | ||
| # Since Detectors are project-scoped and their DataSources are project-specific, | ||
| # we need to update all related organization-scoped workflow_engine models. | ||
| # | ||
| # IMPORTANT: Workflows and DataConditionGroups can be shared across multiple projects | ||
| # in the same organization. We only transfer them if they're exclusively used by | ||
| # detectors in this project. Shared workflows remain in the original organization. | ||
| # There are certainly more correct ways to do this, but this should cover most cases. | ||
|
|
||
| detector_ids = Detector.objects.filter(project_id=self.id).values_list("id", flat=True) | ||
| if detector_ids: | ||
| # Update DataSources | ||
| # DataSources are 1:1 with their source (e.g., QuerySubscription) so they always transfer | ||
| data_source_ids = ( | ||
| DataSource.objects.filter(detectors__id__in=detector_ids) | ||
| .distinct() | ||
| .values_list("id", flat=True) | ||
| ) | ||
| DataSource.objects.filter(id__in=data_source_ids).update( | ||
| organization_id=organization.id | ||
| ) | ||
|
|
||
| # Update Workflows connected to these detectors | ||
| # Only transfer workflows that are exclusively used by detectors in this project | ||
| all_workflow_ids = ( | ||
| Workflow.objects.filter(detectorworkflow__detector_id__in=detector_ids) | ||
| .distinct() | ||
| .values_list("id", flat=True) | ||
| ) | ||
|
|
||
| # Find workflows that are ONLY connected to detectors in this project | ||
| exclusive_workflow_ids = ( | ||
| Workflow.objects.filter(id__in=all_workflow_ids) | ||
| .annotate( | ||
| detector_count=Count("detectorworkflow__detector"), | ||
| project_detector_count=Count( | ||
| "detectorworkflow__detector", | ||
| filter=Q(detectorworkflow__detector_id__in=detector_ids), | ||
| ), | ||
| ) | ||
| .filter(detector_count=models.F("project_detector_count")) | ||
| .values_list("id", flat=True) | ||
| ) | ||
|
|
||
| Workflow.objects.filter(id__in=exclusive_workflow_ids).update( | ||
| organization_id=organization.id | ||
| ) | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| # Update DataConditionGroups connected to the transferred workflows | ||
| # These are linked via WorkflowDataConditionGroup with a unique constraint on condition_group | ||
| workflow_condition_group_ids = ( | ||
| DataConditionGroup.objects.filter( | ||
| workflowdataconditiongroup__workflow_id__in=exclusive_workflow_ids | ||
| ) | ||
| .distinct() | ||
| .values_list("id", flat=True) | ||
| ) | ||
| DataConditionGroup.objects.filter(id__in=workflow_condition_group_ids).update( | ||
| organization_id=organization.id | ||
| ) | ||
|
|
||
| # Update DataConditionGroups that are directly owned by detectors | ||
| # These are linked via Detector.workflow_condition_group (unique FK) | ||
| # and are exclusively owned by the detector, so they always transfer | ||
| detector_condition_group_ids = ( | ||
| Detector.objects.filter( | ||
| id__in=detector_ids, workflow_condition_group_id__isnull=False | ||
| ) | ||
| .values_list("workflow_condition_group_id", flat=True) | ||
| .distinct() | ||
| ) | ||
| DataConditionGroup.objects.filter(id__in=detector_condition_group_ids).update( | ||
| organization_id=organization.id | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Cross-organization reference for shared DataConditionGroupsWhen transferring detector-owned
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a valid concern?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. It's not forbidden by the data model, but we don't share DCGs. |
||
|
|
||
| # Update DataConditionGroups used as when_condition_group in transferred workflows | ||
| # DataConditionGroups are never shared, so transfer all when_condition_groups | ||
| when_condition_group_ids = ( | ||
| Workflow.objects.filter( | ||
| id__in=exclusive_workflow_ids, when_condition_group_id__isnull=False | ||
| ) | ||
| .values_list("when_condition_group_id", flat=True) | ||
| .distinct() | ||
| ) | ||
| DataConditionGroup.objects.filter(id__in=when_condition_group_ids).update( | ||
| organization_id=organization.id | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Shared when_condition_group incorrectly transferred between organizationsThe code transfers
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't worry, I'm making it official that these can't be shared in another pr. |
||
|
|
||
| # Manually move over external issues to the new org | ||
| linked_groups = GroupLink.objects.filter(project_id=self.id).values_list( | ||
| "linked_id", flat=True | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug:
Project.transfer_to()fails to updateWorkflow.when_condition_groupduring project transfer, causing an organization boundary violation.Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
When a project is transferred using
Project.transfer_to(), thewhen_condition_groupfield ofWorkflowobjects is not updated. This field is aFlexibleForeignKeytoDataConditionGroupand represents trigger conditions. Consequently, the transferred workflow'swhen_condition_group_idwill still reference aDataConditionGroupin the old organization, leading to an organization boundary violation. This can cause errors whenevaluate_trigger_conditions()is called or result in a dangling foreign key if the old organization is deleted.💡 Suggested Fix
Modify the
Project.transfer_to()method to ensure that theDataConditionGroupreferenced byWorkflow.when_condition_groupis correctly transferred or re-associated with the new organization, similar to how otherDataConditionGrouptypes are handled.🤖 Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2830139
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point.