-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
chore(repositories): Switch over queries for SeerProjectRepository to use ProjectRepository
#115456
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
base: master
Are you sure you want to change the base?
Changes from all commits
9358cff
a49b4db
173ac04
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 |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| from sentry.deletions.base import BaseRelation, ModelDeletionTask, ModelRelation | ||
| from sentry.integrations.models.repository_project_path_config import ( | ||
| RepositoryProjectPathConfig, | ||
| ) | ||
| from sentry.models.projectrepository import ProjectRepository | ||
| from sentry.seer.models.project_repository import SeerProjectRepository | ||
|
|
||
|
|
||
| class ProjectRepositoryDeletionTask(ModelDeletionTask[ProjectRepository]): | ||
| def get_child_relations(self, instance: ProjectRepository) -> list[BaseRelation]: | ||
| return [ | ||
| ModelRelation(RepositoryProjectPathConfig, {"project_repository_id": instance.id}), | ||
| ModelRelation(SeerProjectRepository, {"project_repository_id": instance.id}), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| from django.db import IntegrityError, router, transaction | ||
| from django.utils import timezone | ||
|
|
||
| from sentry import features | ||
| from sentry.api.serializers import serialize | ||
| from sentry.constants import ObjectStatus | ||
| from sentry.db.postgres.transactions import enforce_constraints | ||
|
|
@@ -17,7 +18,9 @@ | |
| from sentry.models.code_review_event import CodeReviewEvent | ||
| from sentry.models.commit import Commit | ||
| from sentry.models.options.project_option import ProjectOption | ||
| from sentry.models.organization import Organization | ||
| from sentry.models.projectcodeowners import ProjectCodeOwners | ||
| from sentry.models.projectrepository import ProjectRepository | ||
| from sentry.models.pullrequest import PullRequest | ||
| from sentry.models.repository import Repository | ||
| from sentry.seer.models.project_repository import SeerProjectRepository | ||
|
|
@@ -241,10 +244,17 @@ | |
| Repository.objects.filter(id__in=repo_ids).update(integration_id=None) | ||
|
|
||
| # Delete Seer preferences for this repository | ||
| SeerProjectRepository.objects.filter( | ||
| repository_id__in=repo_ids, | ||
| project__organization_id=organization_id, | ||
| ).delete() | ||
| org = Organization.objects.get(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. Organization.objects.get() raises DoesNotExist and aborts integration cleanup transaction Line 247 introduces an unguarded VerificationRead the full Identified by Warden sentry-backend-bugs · P3Z-ATR |
||
| if features.has("organizations:project-repository-fk-reads", org): | ||
|
Check warning on line 248 in src/sentry/integrations/services/repository/impl.py
|
||
| SeerProjectRepository.objects.filter( | ||
| project_repository__repository_id__in=repo_ids, | ||
| project_repository__project__organization_id=organization_id, | ||
| ).delete() | ||
| else: | ||
| SeerProjectRepository.objects.filter( | ||
| repository_id__in=repo_ids, | ||
| project__organization_id=organization_id, | ||
| ).delete() | ||
|
|
||
| # Delete Code Owners with a Code Mapping using the OrganizationIntegration | ||
| ProjectCodeOwners.objects.filter( | ||
|
|
@@ -257,6 +267,12 @@ | |
| RepositoryProjectPathConfig.objects.filter( | ||
| organization_integration_id=organization_integration_id | ||
| ).delete() | ||
| # Delete project-repo links for the disconnected repos | ||
| if repo_ids: | ||
| ProjectRepository.objects.filter( | ||
| repository_id__in=repo_ids, | ||
| project__organization_id=organization_id, | ||
| ).delete() | ||
|
|
||
| # Clear automation_handoff project options that reference this integration. | ||
| affected_project_ids = ProjectOption.objects.filter( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -463,9 +463,18 @@ def _write_preferences_to_sentry_db( | |
| list(Project.objects.select_for_update().filter(id__in=project_ids).order_by("id")) | ||
|
|
||
| # Only delete SeerProjectRepository for active repos. | ||
| SeerProjectRepository.objects.filter( | ||
| project_id__in=project_ids, repository__status=ObjectStatus.ACTIVE | ||
| ).delete() | ||
| if features.has( | ||
| "organizations:project-repository-fk-reads", | ||
| project_preferences[0][0].organization, | ||
| ): | ||
| SeerProjectRepository.objects.filter( | ||
| project_repository__project_id__in=project_ids, | ||
| project_repository__repository__status=ObjectStatus.ACTIVE, | ||
| ).delete() | ||
|
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. Flag-gated delete misses records with null FKMedium Severity When the feature flag is on, the delete query filters through the nullable Additional Locations (1)Reviewed by Cursor Bugbot for commit 173ac04. Configure here. |
||
| else: | ||
| SeerProjectRepository.objects.filter( | ||
| project_id__in=project_ids, repository__status=ObjectStatus.ACTIVE | ||
| ).delete() | ||
|
|
||
| all_repo_ids = { | ||
| repo_def.repository_id | ||
|
|
@@ -647,13 +656,23 @@ def build_automation_handoff( | |
|
|
||
| def read_preference_from_sentry_db(project: Project) -> SeerProjectPreference: | ||
| """Read a single project's Seer preferences from Sentry DB.""" | ||
| seer_project_repo_qs = ( | ||
| SeerProjectRepository.objects.filter( | ||
| project=project, repository__status=ObjectStatus.ACTIVE | ||
| if features.has("organizations:project-repository-fk-reads", project.organization): | ||
| seer_project_repo_qs = ( | ||
| SeerProjectRepository.objects.filter( | ||
| project_repository__project=project, | ||
| project_repository__repository__status=ObjectStatus.ACTIVE, | ||
| ) | ||
| .select_related("project_repository", "project_repository__repository") | ||
|
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. N+1 queries from missing
|
||
| .prefetch_related("branch_overrides") | ||
| ) | ||
|
Comment on lines
+659
to
+667
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: The new feature-flagged code path in Suggested FixAdd Prompt for AI Agent |
||
| else: | ||
| seer_project_repo_qs = ( | ||
| SeerProjectRepository.objects.filter( | ||
| project=project, repository__status=ObjectStatus.ACTIVE | ||
| ) | ||
| .select_related("repository") | ||
| .prefetch_related("branch_overrides") | ||
| ) | ||
| .select_related("repository") | ||
| .prefetch_related("branch_overrides") | ||
| ) | ||
| repo_definitions = [ | ||
| repo_def | ||
| for project_repo in seer_project_repo_qs | ||
|
|
@@ -679,14 +698,18 @@ def bulk_read_preferences_from_sentry_db( | |
|
|
||
| projects = list(Project.objects.filter(id__in=project_ids, organization_id=organization_id)) | ||
|
|
||
| org = Organization.objects.get(id=organization_id) | ||
|
sentry-warden[bot] marked this conversation as resolved.
|
||
| repo_definitions_by_project: defaultdict[int, list[SeerRepoDefinition]] = defaultdict(list) | ||
| for project_repo in ( | ||
| SeerProjectRepository.objects.filter( | ||
| if features.has("organizations:project-repository-fk-reads", org): | ||
| seer_repo_qs = SeerProjectRepository.objects.filter( | ||
| project_repository__project_id__in=project_ids, | ||
| project_repository__repository__status=ObjectStatus.ACTIVE, | ||
| ).select_related("project_repository", "project_repository__repository") | ||
|
sentry[bot] marked this conversation as resolved.
|
||
| else: | ||
| seer_repo_qs = SeerProjectRepository.objects.filter( | ||
| project_id__in=project_ids, repository__status=ObjectStatus.ACTIVE | ||
| ) | ||
| .select_related("repository") | ||
| .prefetch_related("branch_overrides") | ||
| ): | ||
| ).select_related("repository") | ||
| for project_repo in seer_repo_qs.prefetch_related("branch_overrides"): | ||
| repo_def = build_repo_definition_from_project_repo(project_repo) | ||
| if repo_def is not None: | ||
| repo_definitions_by_project[project_repo.project_id].append(repo_def) | ||
|
|
@@ -798,6 +821,13 @@ def _set_if_not_default(key: str, value: Any, default: Any) -> None: | |
|
|
||
| def has_project_connected_repos(organization: Organization, project: Project) -> bool: | ||
| """Check if a project has connected repositories for Seer automation.""" | ||
| if features.has("organizations:project-repository-fk-reads", organization): | ||
| return SeerProjectRepository.objects.filter( | ||
| project_repository__project=project, | ||
| project_repository__project__organization_id=organization.id, | ||
| project_repository__project__status=ObjectStatus.ACTIVE, | ||
| project_repository__repository__status=ObjectStatus.ACTIVE, | ||
| ).exists() | ||
| return SeerProjectRepository.objects.filter( | ||
| project=project, | ||
| project__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.
Hidden repo cascades into unintended SeerProjectRepository deletion
High Severity
ProjectRepositoryis added to_get_repository_child_relations, which is shared betweenRepositoryDeletionTaskandrepository_cascade_delete_on_hide. The comment on line 37-38 explicitly states thatSeerProjectRepositoryrecords must NOT be deleted when a repo is merely hidden. However, deletingProjectRepositoryduring a hide operation triggersProjectRepositoryDeletionTask, whose child relations includeSeerProjectRepository(byproject_repository_id), effectively cascade-deleting the very records intended to be preserved.ProjectRepositorylikely belongs inRepositoryDeletionTask.get_child_relationsonly, alongsideSeerProjectRepository.Additional Locations (1)
src/sentry/deletions/defaults/projectrepository.py#L8-L14Reviewed by Cursor Bugbot for commit a49b4db. Configure here.