-
-
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
Conversation
86d5e8b to
dd16818
Compare
dd16818 to
9092e62
Compare
| "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 | ||
| ) |
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 update Workflow.when_condition_group during project transfer, causing an organization boundary violation.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
When a project is transferred using Project.transfer_to(), the when_condition_group field of Workflow objects is not updated. This field is a FlexibleForeignKey to DataConditionGroup and represents trigger conditions. Consequently, the transferred workflow's when_condition_group_id will still reference a DataConditionGroup in the old organization, leading to an organization boundary violation. This can cause errors when evaluate_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 the DataConditionGroup referenced by Workflow.when_condition_group is correctly transferred or re-associated with the new organization, similar to how other DataConditionGroup types are handled.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/models/project.py#L669-L691
Potential issue: When a project is transferred using `Project.transfer_to()`, the
`when_condition_group` field of `Workflow` objects is not updated. This field is a
`FlexibleForeignKey` to `DataConditionGroup` and represents trigger conditions.
Consequently, the transferred workflow's `when_condition_group_id` will still reference
a `DataConditionGroup` in the old organization, leading to an organization boundary
violation. This can cause errors when `evaluate_trigger_conditions()` is called or
result in a dangling foreign key if the old organization is deleted.
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.
| ) | ||
| DataConditionGroup.objects.filter(id__in=detector_condition_group_ids).update( | ||
| organization_id=organization.id | ||
| ) |
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: Cross-organization reference for shared DataConditionGroups
When transferring detector-owned DataConditionGroup objects, the code doesn't check if they're also referenced by workflows that aren't being transferred. If a DataConditionGroup is referenced by both a detector's workflow_condition_group (being transferred) and a workflow's when_condition_group or via WorkflowDataConditionGroup (not being transferred because the workflow is shared), transferring the DataConditionGroup creates a cross-organization reference where a workflow in the old organization points to a DataConditionGroup in the new organization.
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.
Is this a valid concern?
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.
No. It's not forbidden by the data model, but we don't share DCGs.
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.
Project transfer is such a nightmare 😱
Do we need to have some kind of logic related to each thing linked to DataSource so that we can transfer that over as well? Otherwise we can be linking to an object that's in the old org
| ) | ||
| DataConditionGroup.objects.filter(id__in=when_condition_group_ids).update( | ||
| organization_id=organization.id | ||
| ) |
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: Shared when_condition_group incorrectly transferred between organizations
The code transfers when_condition_group DataConditionGroups to the new organization for all exclusive workflows, but when_condition_group can be shared between multiple workflows (no unique constraint on Workflow.when_condition_group). If a non-transferred workflow in the old organization shares a when_condition_group with a transferred workflow, that shared DataConditionGroup gets moved to the new organization, breaking the non-transferred workflow's reference.
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.
don't worry, I'm making it official that these can't be shared in another pr.
Yes, basically. I think Monitors and QuerySubscriptions are already covered, and UptimeSubscriptions don't seem tied to org or project by themselves, so maybe we're okay, but I assume that there are lingering issues beyond the ambiguities in workflows and such. |
ceorourke
left a comment
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.
Looks good besides the one cursor comment I replied to but I trust you may have a fix coming for that or it's already covered.
Detectors are project-associated, so they work automatically, but DataSources, DataConditionGroups, and Workflows need to be updated.
Note that some of these (Workflows in particular) are org scoped, so transferring may not make sense, so we handle the simple case and hope for the best.