diff --git a/src/sentry/integrations/services/repository/impl.py b/src/sentry/integrations/services/repository/impl.py index 9eaab710ad17cf..7a6bbe2e6ab168 100644 --- a/src/sentry/integrations/services/repository/impl.py +++ b/src/sentry/integrations/services/repository/impl.py @@ -128,15 +128,60 @@ def update_repositories(self, *, organization_id: int, updates: list[RpcReposito Repository.objects.bulk_update(repositories, fields=list(fields_to_update)) + def _cleanup_seer_project_repositories( + self, + organization_id: int, + repo_tuples: list[tuple[int, str | None, str | None]], + ) -> None: + """Delete SeerProjectRepository rows and schedule Seer API cleanup. + + Must be called inside a transaction.atomic() block. The SeerProjectRepository + delete participates in the transaction, while the Seer API task is dispatched + via on_commit so it only fires if the transaction succeeds. + + repo_tuples: list of (repo_id, external_id, provider) from Repository.values_list(). + """ + try: + organization = Organization.objects.get_from_cache(id=organization_id) + if features.has("organizations:seer-project-settings-dual-write", organization): + SeerProjectRepository.objects.filter( + repository_id__in=[repo_id for repo_id, _, _ in repo_tuples] + ).delete() + except Organization.DoesNotExist: + pass + + repos_to_clean = [ + {"repo_external_id": external_id, "repo_provider": repo_provider} + for _, external_id, repo_provider in repo_tuples + if external_id and repo_provider + ] + if repos_to_clean: + transaction.on_commit( + lambda: bulk_cleanup_seer_repository_preferences.apply_async( + kwargs={ + "organization_id": organization_id, + "repos": repos_to_clean, + } + ), + using=router.db_for_write(Repository), + ) + def disable_repositories_for_integration( self, *, organization_id: int, integration_id: int, provider: str ) -> None: with transaction.atomic(router.db_for_write(Repository)): - Repository.objects.filter( - organization_id=organization_id, - integration_id=integration_id, - provider=provider, - ).update(status=ObjectStatus.DISABLED) + repos = list( + Repository.objects.filter( + organization_id=organization_id, + integration_id=integration_id, + provider=provider, + ).values_list("id", "external_id", "provider") + ) + repo_ids = [repo_id for repo_id, _, _ in repos] + + if repo_ids: + Repository.objects.filter(id__in=repo_ids).update(status=ObjectStatus.DISABLED) + self._cleanup_seer_project_repositories(organization_id, repos) def disable_repositories_by_external_ids( self, @@ -147,13 +192,20 @@ def disable_repositories_by_external_ids( external_ids: list[str], ) -> None: with transaction.atomic(router.db_for_write(Repository)): - Repository.objects.filter( - organization_id=organization_id, - integration_id=integration_id, - provider=provider, - external_id__in=external_ids, - status=ObjectStatus.ACTIVE, - ).update(status=ObjectStatus.DISABLED) + repos = list( + Repository.objects.filter( + organization_id=organization_id, + integration_id=integration_id, + provider=provider, + external_id__in=external_ids, + status=ObjectStatus.ACTIVE, + ).values_list("id", "external_id", "provider") + ) + repo_ids = [repo_id for repo_id, _, _ in repos] + + if repo_ids: + Repository.objects.filter(id__in=repo_ids).update(status=ObjectStatus.DISABLED) + self._cleanup_seer_project_repositories(organization_id, repos) def disassociate_organization_integration( self, @@ -169,21 +221,10 @@ def disassociate_organization_integration( ).values_list("id", "external_id", "provider") ) repo_ids = [repo_id for repo_id, _, _ in repos] - if repo_ids: # Disassociate repos from the organization integration being deleted Repository.objects.filter(id__in=repo_ids).update(integration_id=None) - - # Delete Seer project preferences for the affected repos. - # Organization may already be deleted if org deletion and integration - # uninstall overlap; skip SeerProjectRepository cleanup in that case - # since cascades will handle it. - try: - organization = Organization.objects.get_from_cache(id=organization_id) - if features.has("organizations:seer-project-settings-dual-write", organization): - SeerProjectRepository.objects.filter(repository_id__in=repo_ids).delete() - except Organization.DoesNotExist: - pass + self._cleanup_seer_project_repositories(organization_id, repos) # Delete Code Owners with a Code Mapping using the OrganizationIntegration ProjectCodeOwners.objects.filter( @@ -196,17 +237,3 @@ def disassociate_organization_integration( RepositoryProjectPathConfig.objects.filter( organization_integration_id=organization_integration_id ).delete() - - # Delete Seer project preferences for the affected repos via Seer API - repos_to_clean = [ - {"repo_external_id": external_id, "repo_provider": provider} - for _, external_id, provider in repos - if external_id and provider - ] - if repos_to_clean: - bulk_cleanup_seer_repository_preferences.apply_async( - kwargs={ - "organization_id": organization_id, - "repos": repos_to_clean, - } - ) diff --git a/tests/sentry/integrations/github/test_webhook.py b/tests/sentry/integrations/github/test_webhook.py index f0b2f838fbabe4..a975d6a56ab29e 100644 --- a/tests/sentry/integrations/github/test_webhook.py +++ b/tests/sentry/integrations/github/test_webhook.py @@ -455,7 +455,8 @@ def test_end_to_end_repos_added(self) -> None: assert repos[0].provider == "integrations:github" assert repos[1].name == "getsentry/snuba" - def test_end_to_end_repos_removed(self) -> None: + @patch("sentry.integrations.services.repository.impl.bulk_cleanup_seer_repository_preferences") + def test_end_to_end_repos_removed(self, mock_seer_cleanup: MagicMock) -> None: """Full end-to-end: webhook URL → handler → task → Repository disabled.""" future_expires = datetime.now().replace(microsecond=0) + timedelta(minutes=5) integration = self.create_integration( diff --git a/tests/sentry/integrations/services/repository/test_impl.py b/tests/sentry/integrations/services/repository/test_impl.py index a92df36cc47066..896850e192c36e 100644 --- a/tests/sentry/integrations/services/repository/test_impl.py +++ b/tests/sentry/integrations/services/repository/test_impl.py @@ -1,7 +1,14 @@ +from unittest.mock import MagicMock, patch + +import pytest + from sentry.constants import ObjectStatus +from sentry.integrations.models.repository_project_path_config import RepositoryProjectPathConfig from sentry.integrations.services.repository.service import repository_service from sentry.models.repository import Repository +from sentry.seer.models.project_repository import SeerProjectRepository from sentry.testutils.cases import TestCase +from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import cell_silo_test @@ -15,7 +22,8 @@ def setUp(self) -> None: ) self.provider = "integrations:github" - def test_disables_matching_active_repos(self) -> None: + @patch("sentry.integrations.services.repository.impl.bulk_cleanup_seer_repository_preferences") + def test_disables_matching_active_repos(self, mock_seer_cleanup: MagicMock) -> None: repo1 = Repository.objects.create( organization_id=self.organization.id, name="getsentry/sentry", @@ -111,7 +119,8 @@ def test_does_not_affect_repos_from_other_orgs(self) -> None: repo.refresh_from_db() assert repo.status == ObjectStatus.ACTIVE - def test_only_disables_specified_external_ids(self) -> None: + @patch("sentry.integrations.services.repository.impl.bulk_cleanup_seer_repository_preferences") + def test_only_disables_specified_external_ids(self, mock_seer_cleanup: MagicMock) -> None: repo_to_disable = Repository.objects.create( organization_id=self.organization.id, name="getsentry/sentry", @@ -160,3 +169,192 @@ def test_empty_external_ids_is_noop(self) -> None: repo.refresh_from_db() assert repo.status == ObjectStatus.ACTIVE + + @with_feature("organizations:seer-project-settings-dual-write") + @patch("sentry.integrations.services.repository.impl.bulk_cleanup_seer_repository_preferences") + def test_cleans_up_seer_preferences(self, mock_cleanup: MagicMock) -> None: + project = self.create_project(organization=self.organization) + repo = Repository.objects.create( + organization_id=self.organization.id, + name="getsentry/sentry", + external_id="100", + provider=self.provider, + integration_id=self.integration.id, + status=ObjectStatus.ACTIVE, + ) + SeerProjectRepository.objects.create(project=project, repository_id=repo.id) + + repository_service.disable_repositories_by_external_ids( + organization_id=self.organization.id, + integration_id=self.integration.id, + provider=self.provider, + external_ids=["100"], + ) + + repo.refresh_from_db() + assert repo.status == ObjectStatus.DISABLED + + assert not SeerProjectRepository.objects.filter(repository_id=repo.id).exists() + mock_cleanup.apply_async.assert_called_once_with( + kwargs={ + "organization_id": self.organization.id, + "repos": [{"repo_external_id": "100", "repo_provider": self.provider}], + } + ) + + +@cell_silo_test +class DisableRepositoriesForIntegrationTest(TestCase): + def setUp(self) -> None: + self.integration = self.create_integration( + organization=self.organization, + external_id="1", + provider="github", + ) + self.provider = "integrations:github" + + @patch("sentry.integrations.services.repository.impl.bulk_cleanup_seer_repository_preferences") + def test_disables_matching_active_repos(self, mock_seer_cleanup: MagicMock) -> None: + repo = Repository.objects.create( + organization_id=self.organization.id, + name="getsentry/sentry", + external_id="100", + provider=self.provider, + integration_id=self.integration.id, + status=ObjectStatus.ACTIVE, + ) + + repository_service.disable_repositories_for_integration( + organization_id=self.organization.id, + integration_id=self.integration.id, + provider=self.provider, + ) + + repo.refresh_from_db() + assert repo.status == ObjectStatus.DISABLED + + @with_feature("organizations:seer-project-settings-dual-write") + @patch("sentry.integrations.services.repository.impl.bulk_cleanup_seer_repository_preferences") + def test_cleans_up_seer_preferences(self, mock_cleanup: MagicMock) -> None: + project = self.create_project(organization=self.organization) + repo = Repository.objects.create( + organization_id=self.organization.id, + name="getsentry/sentry", + external_id="100", + provider=self.provider, + integration_id=self.integration.id, + status=ObjectStatus.ACTIVE, + ) + SeerProjectRepository.objects.create(project=project, repository_id=repo.id) + + repository_service.disable_repositories_for_integration( + organization_id=self.organization.id, + integration_id=self.integration.id, + provider=self.provider, + ) + + repo.refresh_from_db() + assert repo.status == ObjectStatus.DISABLED + + assert not SeerProjectRepository.objects.filter(repository_id=repo.id).exists() + mock_cleanup.apply_async.assert_called_once_with( + kwargs={ + "organization_id": self.organization.id, + "repos": [{"repo_external_id": "100", "repo_provider": self.provider}], + } + ) + + +@cell_silo_test +class DisassociateOrganizationIntegrationTest(TestCase): + def setUp(self) -> None: + self.integration = self.create_integration( + organization=self.organization, + external_id="1", + provider="github", + ) + self.provider = "integrations:github" + self.org_integration = self.integration.organizationintegration_set.first() + + @patch("sentry.integrations.services.repository.impl.bulk_cleanup_seer_repository_preferences") + def test_disassociates_repos(self, mock_cleanup: MagicMock) -> None: + repo = Repository.objects.create( + organization_id=self.organization.id, + name="getsentry/sentry", + external_id="100", + provider=self.provider, + integration_id=self.integration.id, + status=ObjectStatus.ACTIVE, + ) + + repository_service.disassociate_organization_integration( + organization_id=self.organization.id, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + ) + + repo.refresh_from_db() + assert repo.integration_id is None + mock_cleanup.apply_async.assert_called_once() + + @with_feature("organizations:seer-project-settings-dual-write") + @patch("sentry.integrations.services.repository.impl.bulk_cleanup_seer_repository_preferences") + def test_cleans_up_seer_preferences(self, mock_cleanup: MagicMock) -> None: + project = self.create_project(organization=self.organization) + repo = Repository.objects.create( + organization_id=self.organization.id, + name="getsentry/sentry", + external_id="100", + provider=self.provider, + integration_id=self.integration.id, + status=ObjectStatus.ACTIVE, + ) + SeerProjectRepository.objects.create(project=project, repository_id=repo.id) + + repository_service.disassociate_organization_integration( + organization_id=self.organization.id, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + ) + + repo.refresh_from_db() + assert repo.integration_id is None + assert not SeerProjectRepository.objects.filter(repository_id=repo.id).exists() + mock_cleanup.apply_async.assert_called_once_with( + kwargs={ + "organization_id": self.organization.id, + "repos": [{"repo_external_id": "100", "repo_provider": self.provider}], + } + ) + + @patch("sentry.integrations.services.repository.impl.bulk_cleanup_seer_repository_preferences") + def test_transaction_rollback_does_not_dispatch_seer_cleanup( + self, mock_cleanup: MagicMock + ) -> None: + repo = Repository.objects.create( + organization_id=self.organization.id, + name="getsentry/sentry", + external_id="100", + provider=self.provider, + integration_id=self.integration.id, + status=ObjectStatus.ACTIVE, + ) + + with patch.object( + RepositoryProjectPathConfig.objects, + "filter", + side_effect=RuntimeError("simulated failure"), + ): + with pytest.raises(RuntimeError): + repository_service.disassociate_organization_integration( + organization_id=self.organization.id, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + ) + + # Transaction rolled back: repo should still have its integration + repo.refresh_from_db() + assert repo.integration_id == self.integration.id + + # Task should not have been dispatched + mock_cleanup.apply_async.assert_not_called()