From 57af7800214584070fb96c741ac1cf475939739e Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 29 Jan 2025 15:37:31 -0800 Subject: [PATCH 01/25] feat(ACI): Delete endpoint for a detector --- .../endpoints/project_detector_details.py | 60 ++++++++++++++++++- src/sentry/workflow_engine/endpoints/urls.py | 1 - .../test_project_detector_details.py | 51 ++++++++++++++-- 3 files changed, 105 insertions(+), 7 deletions(-) diff --git a/src/sentry/workflow_engine/endpoints/project_detector_details.py b/src/sentry/workflow_engine/endpoints/project_detector_details.py index d692aba489db22..586f0343f00da7 100644 --- a/src/sentry/workflow_engine/endpoints/project_detector_details.py +++ b/src/sentry/workflow_engine/endpoints/project_detector_details.py @@ -11,12 +11,20 @@ from sentry.apidocs.constants import ( RESPONSE_BAD_REQUEST, RESPONSE_FORBIDDEN, + RESPONSE_NO_CONTENT, RESPONSE_NOT_FOUND, RESPONSE_UNAUTHORIZED, ) from sentry.apidocs.parameters import DetectorParams, GlobalParams +from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion +from sentry.models.project import Project from sentry.workflow_engine.endpoints.serializers import DetectorSerializer -from sentry.workflow_engine.models import Detector +from sentry.workflow_engine.models import ( + DataConditionGroup, + DataSource, + DataSourceDetector, + Detector, +) @region_silo_endpoint @@ -57,7 +65,7 @@ def convert_args(self, request: Request, detector_id, *args, **kwargs): 404: RESPONSE_NOT_FOUND, }, ) - def get(self, request: Request, project, detector): + def get(self, request: Request, project: Project, detector: Detector): """ Fetch a detector ````````````````````````` @@ -69,3 +77,51 @@ def get(self, request: Request, project, detector): DetectorSerializer(), ) return Response(serialized_detector) + + @extend_schema( + operation_id="Delete a Detector", + parameters=[ + GlobalParams.ORG_ID_OR_SLUG, + GlobalParams.PROJECT_ID_OR_SLUG, + DetectorParams.DETECTOR_ID, + ], + responses={ + 204: RESPONSE_NO_CONTENT, + 403: RESPONSE_FORBIDDEN, + 404: RESPONSE_NOT_FOUND, + }, + ) + def delete(self, request: Request, project: Project, detector: Detector): + """ + Delete a detector + """ + try: + data_condition_group = DataConditionGroup.objects.get( + id=detector.workflow_condition_group.id + ) + except DataConditionGroup.DoesNotExist: + pass + + if data_condition_group: + RegionScheduledDeletion.schedule(data_condition_group, days=0, actor=request.user) + + try: + data_source_detector = DataSourceDetector.objects.get(detector_id=detector.id) + except DataSourceDetector.DoesNotExist: + pass + + if data_source_detector: + RegionScheduledDeletion.schedule(data_source_detector, days=0, actor=request.user) + + try: + data_sources = DataSource.objects.filter(detector=detector.id) + except DataSource.DoesNotExist: + pass + + if data_sources: + for data_source in data_sources: + RegionScheduledDeletion.schedule(data_source, days=0, actor=request.user) + + RegionScheduledDeletion.schedule(detector, days=0, actor=request.user) + # TODO add audit log entry + return Response(status=204) diff --git a/src/sentry/workflow_engine/endpoints/urls.py b/src/sentry/workflow_engine/endpoints/urls.py index 15dcf661f601df..bd6e21d66a65ef 100644 --- a/src/sentry/workflow_engine/endpoints/urls.py +++ b/src/sentry/workflow_engine/endpoints/urls.py @@ -10,7 +10,6 @@ # - GET /detector w/ filters # - GET /detector/:id # - PUT /detector/:id -# - DELETE /detector/:id # Remaining Workflows Endpoints # - GET /workflow w/ filters diff --git a/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py b/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py index 92ddf261433f22..59c23e4c9c39ad 100644 --- a/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py @@ -1,7 +1,10 @@ from sentry.api.serializers import serialize +from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion from sentry.incidents.grouptype import MetricAlertFire from sentry.testutils.cases import APITestCase +from sentry.testutils.outbox import outbox_runner from sentry.testutils.silo import region_silo_test +from sentry.workflow_engine.models import DataConditionGroup, DataSource, DataSourceDetector class ProjectDetectorDetailsBaseTest(APITestCase): @@ -10,16 +13,56 @@ class ProjectDetectorDetailsBaseTest(APITestCase): def setUp(self): super().setUp() self.login_as(user=self.user) + self.data_condition_group = self.create_data_condition_group() + self.detector = self.create_detector( + project_id=self.project.id, + name="Test Detector", + type=MetricAlertFire.slug, + workflow_condition_group=self.data_condition_group, + ) + data_source = DataSource.objects.filter( + id__in=[data_source.id for data_source in self.detector.data_sources.all()] + ).first() + self.create_data_source_detector(data_source=data_source, detector=self.detector) @region_silo_test class ProjectDetectorIndexGetTest(ProjectDetectorDetailsBaseTest): def test_simple(self): - detector = self.create_detector( - project_id=self.project.id, name="Test Detector", type=MetricAlertFire.slug + response = self.get_success_response( + self.organization.slug, self.project.slug, self.detector.id ) - response = self.get_success_response(self.organization.slug, self.project.slug, detector.id) - assert response.data == serialize(detector) + assert response.data == serialize(self.detector) def test_does_not_exist(self): self.get_error_response(self.organization.slug, self.project.slug, 3, status_code=404) + + +@region_silo_test +class ProjectDetectorIndexDeleteTest(ProjectDetectorDetailsBaseTest): + method = "DELETE" + + def test_simple(self): + detector_id = self.detector.id + data_condition_group = DataConditionGroup.objects.get( + id=self.detector.workflow_condition_group.id + ) + data_source_detector = DataSourceDetector.objects.get(detector_id=detector_id) + data_source = DataSource.objects.get(detector=detector_id) + + with outbox_runner(): + self.get_success_response(self.organization.slug, self.project.slug, self.detector.id) + + self.detector.refresh_from_db() + assert RegionScheduledDeletion.objects.filter( + model_name="Detector", object_id=detector_id + ).exists() + assert RegionScheduledDeletion.objects.filter( + model_name="DataConditionGroup", object_id=data_condition_group.id + ).exists() + assert RegionScheduledDeletion.objects.filter( + model_name="DataSourceDetector", object_id=data_source_detector.id + ).exists() + assert RegionScheduledDeletion.objects.filter( + model_name="DataSource", object_id=data_source.id + ).exists() From b06a41e1a6f06dc0fd5523dd2d7486051ddc1a05 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 29 Jan 2025 16:11:31 -0800 Subject: [PATCH 02/25] use --- .../test_project_detector_details.py | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py b/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py index 59c23e4c9c39ad..5bd61c9dc109dd 100644 --- a/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py @@ -1,10 +1,15 @@ from sentry.api.serializers import serialize -from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion +from sentry.deletions.tasks.scheduled import run_scheduled_deletions from sentry.incidents.grouptype import MetricAlertFire from sentry.testutils.cases import APITestCase from sentry.testutils.outbox import outbox_runner from sentry.testutils.silo import region_silo_test -from sentry.workflow_engine.models import DataConditionGroup, DataSource, DataSourceDetector +from sentry.workflow_engine.models import ( + DataConditionGroup, + DataSource, + DataSourceDetector, + Detector, +) class ProjectDetectorDetailsBaseTest(APITestCase): @@ -53,16 +58,10 @@ def test_simple(self): with outbox_runner(): self.get_success_response(self.organization.slug, self.project.slug, self.detector.id) - self.detector.refresh_from_db() - assert RegionScheduledDeletion.objects.filter( - model_name="Detector", object_id=detector_id - ).exists() - assert RegionScheduledDeletion.objects.filter( - model_name="DataConditionGroup", object_id=data_condition_group.id - ).exists() - assert RegionScheduledDeletion.objects.filter( - model_name="DataSourceDetector", object_id=data_source_detector.id - ).exists() - assert RegionScheduledDeletion.objects.filter( - model_name="DataSource", object_id=data_source.id - ).exists() + with self.tasks(): + run_scheduled_deletions() + + assert not Detector.objects.filter(id=detector_id).exists() + assert not DataConditionGroup.objects.filter(id=data_condition_group.id).exists() + assert not DataSourceDetector.objects.filter(id=data_source_detector.id).exists() + assert not DataSource.objects.filter(id=data_source.id).exists() From 1052592cb0064a3ceefdf743f37d986795267028 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 29 Jan 2025 16:50:31 -0800 Subject: [PATCH 03/25] add deletions task --- src/sentry/deletions/defaults/detector.py | 10 ++++++ .../endpoints/project_detector_details.py | 15 +------- tests/sentry/deletions/test_detector.py | 35 +++++++++++++++++++ .../test_project_detector_details.py | 35 ++++++------------- 4 files changed, 56 insertions(+), 39 deletions(-) create mode 100644 src/sentry/deletions/defaults/detector.py create mode 100644 tests/sentry/deletions/test_detector.py diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py new file mode 100644 index 00000000000000..2c57699a004f05 --- /dev/null +++ b/src/sentry/deletions/defaults/detector.py @@ -0,0 +1,10 @@ +from sentry.deletions.base import BaseRelation, ModelDeletionTask, ModelRelation +from sentry.incidents.models.alert_rule import AlertRule +from sentry.workflow_engine.models import Detector + + +class DetectorDeletionTask(ModelDeletionTask[Detector]): + def get_child_relations(self, instance: AlertRule) -> list[BaseRelation]: + from sentry.workflow_engine.models import DataSourceDetector + + return [ModelRelation(DataSourceDetector, {"detector_id": instance.id})] diff --git a/src/sentry/workflow_engine/endpoints/project_detector_details.py b/src/sentry/workflow_engine/endpoints/project_detector_details.py index 586f0343f00da7..13aff4f57d0c4d 100644 --- a/src/sentry/workflow_engine/endpoints/project_detector_details.py +++ b/src/sentry/workflow_engine/endpoints/project_detector_details.py @@ -19,12 +19,7 @@ from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion from sentry.models.project import Project from sentry.workflow_engine.endpoints.serializers import DetectorSerializer -from sentry.workflow_engine.models import ( - DataConditionGroup, - DataSource, - DataSourceDetector, - Detector, -) +from sentry.workflow_engine.models import DataConditionGroup, DataSource, Detector @region_silo_endpoint @@ -105,14 +100,6 @@ def delete(self, request: Request, project: Project, detector: Detector): if data_condition_group: RegionScheduledDeletion.schedule(data_condition_group, days=0, actor=request.user) - try: - data_source_detector = DataSourceDetector.objects.get(detector_id=detector.id) - except DataSourceDetector.DoesNotExist: - pass - - if data_source_detector: - RegionScheduledDeletion.schedule(data_source_detector, days=0, actor=request.user) - try: data_sources = DataSource.objects.filter(detector=detector.id) except DataSource.DoesNotExist: diff --git a/tests/sentry/deletions/test_detector.py b/tests/sentry/deletions/test_detector.py new file mode 100644 index 00000000000000..f1052f51f6a523 --- /dev/null +++ b/tests/sentry/deletions/test_detector.py @@ -0,0 +1,35 @@ +from sentry.deletions.tasks.scheduled import run_scheduled_deletions +from sentry.incidents.grouptype import MetricAlertFire +from sentry.testutils.cases import TestCase +from sentry.testutils.hybrid_cloud import HybridCloudTestMixin +from sentry.workflow_engine.models import ( + DataConditionGroup, + DataSource, + DataSourceDetector, + Detector, +) + + +class DeleteDetectorTest(TestCase, HybridCloudTestMixin): + def test_simple(self): + data_condition_group = self.create_data_condition_group() + data_source = self.create_data_source(organization=self.organization) + detector = self.create_detector( + project_id=self.project.id, + name="Test Detector", + type=MetricAlertFire.slug, + workflow_condition_group=data_condition_group, + ) + data_source_detector = self.create_data_source_detector( + data_source=data_source, detector=detector + ) + + self.ScheduledDeletion.schedule(instance=detector, days=0) + + with self.tasks(): + run_scheduled_deletions() + + assert not Detector.objects.filter(id=detector.id).exists() + assert not DataSourceDetector.objects.filter(id=data_source_detector.id).exists() + assert not DataConditionGroup.objects.filter(id=data_condition_group.id).exists() + assert not DataSource.objects.filter(id=data_source.id).exists() diff --git a/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py b/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py index 5bd61c9dc109dd..b586c4614a5c2c 100644 --- a/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py @@ -1,15 +1,9 @@ from sentry.api.serializers import serialize -from sentry.deletions.tasks.scheduled import run_scheduled_deletions +from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion from sentry.incidents.grouptype import MetricAlertFire from sentry.testutils.cases import APITestCase from sentry.testutils.outbox import outbox_runner from sentry.testutils.silo import region_silo_test -from sentry.workflow_engine.models import ( - DataConditionGroup, - DataSource, - DataSourceDetector, - Detector, -) class ProjectDetectorDetailsBaseTest(APITestCase): @@ -18,6 +12,9 @@ class ProjectDetectorDetailsBaseTest(APITestCase): def setUp(self): super().setUp() self.login_as(user=self.user) + self.data_source = self.data_source = self.create_data_source( + organization=self.organization + ) self.data_condition_group = self.create_data_condition_group() self.detector = self.create_detector( project_id=self.project.id, @@ -25,10 +22,9 @@ def setUp(self): type=MetricAlertFire.slug, workflow_condition_group=self.data_condition_group, ) - data_source = DataSource.objects.filter( - id__in=[data_source.id for data_source in self.detector.data_sources.all()] - ).first() - self.create_data_source_detector(data_source=data_source, detector=self.detector) + self.data_source_detector = self.create_data_source_detector( + data_source=self.data_source, detector=self.detector + ) @region_silo_test @@ -48,20 +44,9 @@ class ProjectDetectorIndexDeleteTest(ProjectDetectorDetailsBaseTest): method = "DELETE" def test_simple(self): - detector_id = self.detector.id - data_condition_group = DataConditionGroup.objects.get( - id=self.detector.workflow_condition_group.id - ) - data_source_detector = DataSourceDetector.objects.get(detector_id=detector_id) - data_source = DataSource.objects.get(detector=detector_id) - with outbox_runner(): self.get_success_response(self.organization.slug, self.project.slug, self.detector.id) - with self.tasks(): - run_scheduled_deletions() - - assert not Detector.objects.filter(id=detector_id).exists() - assert not DataConditionGroup.objects.filter(id=data_condition_group.id).exists() - assert not DataSourceDetector.objects.filter(id=data_source_detector.id).exists() - assert not DataSource.objects.filter(id=data_source.id).exists() + assert RegionScheduledDeletion.objects.filter( + model_name="Detector", object_id=self.detector.id + ).exists() From 0259a25faa328e302372a1354a8471ac3c5e4ac0 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 30 Jan 2025 13:41:33 -0800 Subject: [PATCH 04/25] remove manual deletes --- src/sentry/deletions/defaults/detector.py | 10 --------- .../endpoints/project_detector_details.py | 21 +------------------ 2 files changed, 1 insertion(+), 30 deletions(-) delete mode 100644 src/sentry/deletions/defaults/detector.py diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py deleted file mode 100644 index 2c57699a004f05..00000000000000 --- a/src/sentry/deletions/defaults/detector.py +++ /dev/null @@ -1,10 +0,0 @@ -from sentry.deletions.base import BaseRelation, ModelDeletionTask, ModelRelation -from sentry.incidents.models.alert_rule import AlertRule -from sentry.workflow_engine.models import Detector - - -class DetectorDeletionTask(ModelDeletionTask[Detector]): - def get_child_relations(self, instance: AlertRule) -> list[BaseRelation]: - from sentry.workflow_engine.models import DataSourceDetector - - return [ModelRelation(DataSourceDetector, {"detector_id": instance.id})] diff --git a/src/sentry/workflow_engine/endpoints/project_detector_details.py b/src/sentry/workflow_engine/endpoints/project_detector_details.py index 13aff4f57d0c4d..973004ff7c9138 100644 --- a/src/sentry/workflow_engine/endpoints/project_detector_details.py +++ b/src/sentry/workflow_engine/endpoints/project_detector_details.py @@ -19,7 +19,7 @@ from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion from sentry.models.project import Project from sentry.workflow_engine.endpoints.serializers import DetectorSerializer -from sentry.workflow_engine.models import DataConditionGroup, DataSource, Detector +from sentry.workflow_engine.models import Detector @region_silo_endpoint @@ -90,25 +90,6 @@ def delete(self, request: Request, project: Project, detector: Detector): """ Delete a detector """ - try: - data_condition_group = DataConditionGroup.objects.get( - id=detector.workflow_condition_group.id - ) - except DataConditionGroup.DoesNotExist: - pass - - if data_condition_group: - RegionScheduledDeletion.schedule(data_condition_group, days=0, actor=request.user) - - try: - data_sources = DataSource.objects.filter(detector=detector.id) - except DataSource.DoesNotExist: - pass - - if data_sources: - for data_source in data_sources: - RegionScheduledDeletion.schedule(data_source, days=0, actor=request.user) - RegionScheduledDeletion.schedule(detector, days=0, actor=request.user) # TODO add audit log entry return Response(status=204) From a11b6f3c74499a65d36ee0b40f5d62f587613738 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 30 Jan 2025 16:05:11 -0800 Subject: [PATCH 05/25] what is going on --- src/sentry/deletions/defaults/__init__.py | 1 + src/sentry/deletions/defaults/detector.py | 18 ++++++++++++++++++ src/sentry/workflow_engine/models/workflow.py | 3 ++- tests/sentry/deletions/test_detector.py | 12 ++++++++---- 4 files changed, 29 insertions(+), 5 deletions(-) create mode 100644 src/sentry/deletions/defaults/detector.py diff --git a/src/sentry/deletions/defaults/__init__.py b/src/sentry/deletions/defaults/__init__.py index 9d4e90cdfcc214..23bb3f8c21786b 100644 --- a/src/sentry/deletions/defaults/__init__.py +++ b/src/sentry/deletions/defaults/__init__.py @@ -5,6 +5,7 @@ from .artifactbundle import * # noqa: F401,F403 from .commit import * # noqa: F401,F403 from .commitauthor import * # noqa: F401,F403 +from .detector import * # noqa: F401,F403 from .discoversavedquery import * # noqa: F401,F403 from .group import * # noqa: F401,F403 from .grouphash import * # noqa: F401,F403 diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py new file mode 100644 index 00000000000000..feb202c0674ced --- /dev/null +++ b/src/sentry/deletions/defaults/detector.py @@ -0,0 +1,18 @@ +from sentry.deletions.base import BaseRelation, ModelDeletionTask +from sentry.workflow_engine.models.detector import Detector + + +class DetectorDeletionTask(ModelDeletionTask[Detector]): + # # The default manager for alert rules excludes snapshots + # # which we want to include when deleting an organization. + # manager_name = "objects_with_snapshots" + + def get_child_relations(self, instance: Detector) -> list[BaseRelation]: + # from sentry.workflow_engine.models.detector import Detector + + # print("hello") + + return [] + + # model_list = (AlertRuleTrigger, Incident) + # return [ModelRelation(m, {"alert_rule_id": instance.id}) for m in model_list] diff --git a/src/sentry/workflow_engine/models/workflow.py b/src/sentry/workflow_engine/models/workflow.py index 367edc6d435ba8..cbd2db7e0f3ba7 100644 --- a/src/sentry/workflow_engine/models/workflow.py +++ b/src/sentry/workflow_engine/models/workflow.py @@ -10,7 +10,6 @@ from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey from sentry.models.owner_base import OwnerModel from sentry.workflow_engine.models.data_condition import DataCondition, is_slow_condition -from sentry.workflow_engine.processors.data_condition_group import evaluate_condition_group from sentry.workflow_engine.types import WorkflowJob from .json_config import JSONConfigBase @@ -72,6 +71,8 @@ def evaluate_trigger_conditions(self, job: WorkflowJob) -> bool: Evaluate the conditions for the workflow trigger and return if the evaluation was successful. If there aren't any workflow trigger conditions, the workflow is considered triggered. """ + from sentry.workflow_engine.processors.data_condition_group import evaluate_condition_group + if self.when_condition_group is None: return True diff --git a/tests/sentry/deletions/test_detector.py b/tests/sentry/deletions/test_detector.py index f1052f51f6a523..d065f5fcbb41a2 100644 --- a/tests/sentry/deletions/test_detector.py +++ b/tests/sentry/deletions/test_detector.py @@ -2,17 +2,18 @@ from sentry.incidents.grouptype import MetricAlertFire from sentry.testutils.cases import TestCase from sentry.testutils.hybrid_cloud import HybridCloudTestMixin -from sentry.workflow_engine.models import ( +from sentry.workflow_engine.models import ( # DataCondition,; DataSource,; Workflow, DataConditionGroup, - DataSource, DataSourceDetector, Detector, + DetectorWorkflow, ) class DeleteDetectorTest(TestCase, HybridCloudTestMixin): def test_simple(self): data_condition_group = self.create_data_condition_group() + self.create_data_condition(condition_group=data_condition_group) data_source = self.create_data_source(organization=self.organization) detector = self.create_detector( project_id=self.project.id, @@ -20,10 +21,11 @@ def test_simple(self): type=MetricAlertFire.slug, workflow_condition_group=data_condition_group, ) + workflow = self.create_workflow() data_source_detector = self.create_data_source_detector( data_source=data_source, detector=detector ) - + detector_workflow = DetectorWorkflow.objects.create(detector=detector, workflow=workflow) self.ScheduledDeletion.schedule(instance=detector, days=0) with self.tasks(): @@ -31,5 +33,7 @@ def test_simple(self): assert not Detector.objects.filter(id=detector.id).exists() assert not DataSourceDetector.objects.filter(id=data_source_detector.id).exists() + assert not DetectorWorkflow.objects.filter(id=detector_workflow.id).exists() assert not DataConditionGroup.objects.filter(id=data_condition_group.id).exists() - assert not DataSource.objects.filter(id=data_source.id).exists() + # assert not DataConditon.objects.filter(id=data_condition.id).exists() + # assert not DataSource.objects.filter(id=data_source.id).exists() From c16df7a52348720d21b62c973d54590b95c3952a Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 30 Jan 2025 16:37:13 -0800 Subject: [PATCH 06/25] get dcg deleted --- src/sentry/deletions/__init__.py | 2 ++ src/sentry/deletions/defaults/detector.py | 17 +++++------------ tests/sentry/deletions/test_detector.py | 7 ++++--- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/sentry/deletions/__init__.py b/src/sentry/deletions/__init__.py index 5daf033399de8d..a65c0eed5416ef 100644 --- a/src/sentry/deletions/__init__.py +++ b/src/sentry/deletions/__init__.py @@ -111,6 +111,7 @@ def load_defaults(manager: DeletionTaskManager) -> None: from sentry.sentry_apps.models.sentry_app_installation_token import SentryAppInstallationToken from sentry.sentry_apps.models.servicehook import ServiceHook from sentry.snuba import models as snuba_models + from sentry.workflow_engine.models import Detector from . import defaults @@ -125,6 +126,7 @@ def load_defaults(manager: DeletionTaskManager) -> None: manager.register(models.Commit, defaults.CommitDeletionTask) manager.register(models.CommitAuthor, defaults.CommitAuthorDeletionTask) manager.register(CommitFileChange, BulkModelDeletionTask) + manager.register(Detector, defaults.DetectorDeletionTask) manager.register(models.Deploy, BulkModelDeletionTask) manager.register(DiscoverSavedQuery, defaults.DiscoverSavedQueryDeletionTask) manager.register(DiscoverSavedQueryProject, BulkModelDeletionTask) diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py index feb202c0674ced..957db36ce25fb9 100644 --- a/src/sentry/deletions/defaults/detector.py +++ b/src/sentry/deletions/defaults/detector.py @@ -1,18 +1,11 @@ -from sentry.deletions.base import BaseRelation, ModelDeletionTask +from sentry.deletions.base import BaseRelation, ModelDeletionTask, ModelRelation from sentry.workflow_engine.models.detector import Detector class DetectorDeletionTask(ModelDeletionTask[Detector]): - # # The default manager for alert rules excludes snapshots - # # which we want to include when deleting an organization. - # manager_name = "objects_with_snapshots" - def get_child_relations(self, instance: Detector) -> list[BaseRelation]: - # from sentry.workflow_engine.models.detector import Detector - - # print("hello") - - return [] + from sentry.workflow_engine.models import DataConditionGroup - # model_list = (AlertRuleTrigger, Incident) - # return [ModelRelation(m, {"alert_rule_id": instance.id}) for m in model_list] + return [ + ModelRelation(DataConditionGroup, {"id": instance.workflow_condition_group.id}), + ] diff --git a/tests/sentry/deletions/test_detector.py b/tests/sentry/deletions/test_detector.py index d065f5fcbb41a2..bfac5d6d4c204e 100644 --- a/tests/sentry/deletions/test_detector.py +++ b/tests/sentry/deletions/test_detector.py @@ -2,7 +2,8 @@ from sentry.incidents.grouptype import MetricAlertFire from sentry.testutils.cases import TestCase from sentry.testutils.hybrid_cloud import HybridCloudTestMixin -from sentry.workflow_engine.models import ( # DataCondition,; DataSource,; Workflow, +from sentry.workflow_engine.models import ( # DataSource,; Workflow, + DataCondition, DataConditionGroup, DataSourceDetector, Detector, @@ -13,7 +14,7 @@ class DeleteDetectorTest(TestCase, HybridCloudTestMixin): def test_simple(self): data_condition_group = self.create_data_condition_group() - self.create_data_condition(condition_group=data_condition_group) + data_condition = self.create_data_condition(condition_group=data_condition_group) data_source = self.create_data_source(organization=self.organization) detector = self.create_detector( project_id=self.project.id, @@ -35,5 +36,5 @@ def test_simple(self): assert not DataSourceDetector.objects.filter(id=data_source_detector.id).exists() assert not DetectorWorkflow.objects.filter(id=detector_workflow.id).exists() assert not DataConditionGroup.objects.filter(id=data_condition_group.id).exists() - # assert not DataConditon.objects.filter(id=data_condition.id).exists() + assert not DataCondition.objects.filter(id=data_condition.id).exists() # assert not DataSource.objects.filter(id=data_source.id).exists() From 5e28cebd50e25bb7cf107e53ff3c8bf8a2b71ed4 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 31 Jan 2025 10:35:03 -0800 Subject: [PATCH 07/25] conditionally delete data source --- src/sentry/deletions/defaults/detector.py | 20 +++++- tests/sentry/deletions/test_detector.py | 74 +++++++++++++++++------ 2 files changed, 73 insertions(+), 21 deletions(-) diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py index 957db36ce25fb9..0215e13f60fa88 100644 --- a/src/sentry/deletions/defaults/detector.py +++ b/src/sentry/deletions/defaults/detector.py @@ -4,8 +4,22 @@ class DetectorDeletionTask(ModelDeletionTask[Detector]): def get_child_relations(self, instance: Detector) -> list[BaseRelation]: - from sentry.workflow_engine.models import DataConditionGroup + from sentry.workflow_engine.models import DataConditionGroup, DataSource - return [ - ModelRelation(DataConditionGroup, {"id": instance.workflow_condition_group.id}), + model_relations = [ + ModelRelation(DataConditionGroup, {"id": instance.workflow_condition_group.id}) ] + + data_sources = DataSource.objects.filter(detector=instance.id) + delete = True + + for data_source in data_sources: + for detector in data_source.detectors.all(): + if detector.id != instance.id: + delete = False + break + + if delete: + model_relations.append(ModelRelation(DataSource, {"detector": instance.id})) + + return model_relations diff --git a/tests/sentry/deletions/test_detector.py b/tests/sentry/deletions/test_detector.py index bfac5d6d4c204e..ed3457e07897a2 100644 --- a/tests/sentry/deletions/test_detector.py +++ b/tests/sentry/deletions/test_detector.py @@ -2,9 +2,10 @@ from sentry.incidents.grouptype import MetricAlertFire from sentry.testutils.cases import TestCase from sentry.testutils.hybrid_cloud import HybridCloudTestMixin -from sentry.workflow_engine.models import ( # DataSource,; Workflow, +from sentry.workflow_engine.models import ( DataCondition, DataConditionGroup, + DataSource, DataSourceDetector, Detector, DetectorWorkflow, @@ -12,29 +13,66 @@ class DeleteDetectorTest(TestCase, HybridCloudTestMixin): - def test_simple(self): - data_condition_group = self.create_data_condition_group() - data_condition = self.create_data_condition(condition_group=data_condition_group) - data_source = self.create_data_source(organization=self.organization) - detector = self.create_detector( + def setUp(self): + self.data_condition_group = self.create_data_condition_group() + self.data_condition = self.create_data_condition(condition_group=self.data_condition_group) + self.data_source = self.create_data_source(organization=self.organization) + self.detector = self.create_detector( project_id=self.project.id, name="Test Detector", type=MetricAlertFire.slug, - workflow_condition_group=data_condition_group, + workflow_condition_group=self.data_condition_group, + ) + self.workflow = self.create_workflow() + self.data_source_detector = self.create_data_source_detector( + data_source=self.data_source, detector=self.detector + ) + self.detector_workflow = DetectorWorkflow.objects.create( + detector=self.detector, workflow=self.workflow + ) + + def test_simple(self): + data_source_2 = self.create_data_source(organization=self.organization) + data_source_detector_2 = self.create_data_source_detector( + data_source=data_source_2, detector=self.detector + ) + self.ScheduledDeletion.schedule(instance=self.detector, days=0) + + with self.tasks(): + run_scheduled_deletions() + + assert not Detector.objects.filter(id=self.detector.id).exists() + assert not DataSourceDetector.objects.filter( + id__in=[self.data_source_detector.id, data_source_detector_2.id] + ).exists() + assert not DetectorWorkflow.objects.filter(id=self.detector_workflow.id).exists() + assert not DataConditionGroup.objects.filter(id=self.data_condition_group.id).exists() + assert not DataCondition.objects.filter(id=self.data_condition.id).exists() + assert not DataSource.objects.filter( + id__in=[self.data_source.id, data_source_2.id] + ).exists() + + def test_data_source_not_deleted(self): + """ + Test that we do not delete a DataSource that is connected to another Detector + """ + detector_2 = self.create_detector( + project_id=self.project.id, + name="Testy Detector", + type=MetricAlertFire.slug, ) - workflow = self.create_workflow() - data_source_detector = self.create_data_source_detector( - data_source=data_source, detector=detector + data_source_detector_2 = self.create_data_source_detector( + data_source=self.data_source, detector=detector_2 ) - detector_workflow = DetectorWorkflow.objects.create(detector=detector, workflow=workflow) - self.ScheduledDeletion.schedule(instance=detector, days=0) + self.ScheduledDeletion.schedule(instance=self.detector, days=0) with self.tasks(): run_scheduled_deletions() - assert not Detector.objects.filter(id=detector.id).exists() - assert not DataSourceDetector.objects.filter(id=data_source_detector.id).exists() - assert not DetectorWorkflow.objects.filter(id=detector_workflow.id).exists() - assert not DataConditionGroup.objects.filter(id=data_condition_group.id).exists() - assert not DataCondition.objects.filter(id=data_condition.id).exists() - # assert not DataSource.objects.filter(id=data_source.id).exists() + assert not Detector.objects.filter(id=self.detector.id).exists() + assert not DataSourceDetector.objects.filter(id=self.data_source_detector.id).exists() + assert not DetectorWorkflow.objects.filter(id=self.detector_workflow.id).exists() + assert not DataConditionGroup.objects.filter(id=self.data_condition_group.id).exists() + assert not DataCondition.objects.filter(id=self.data_condition.id).exists() + assert DataSource.objects.filter(id=self.data_source.id).exists() + assert DataSourceDetector.objects.filter(id=data_source_detector_2.id).exists() From f6e9c87930112c4635643707989b90a6290d1e42 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 31 Jan 2025 13:45:23 -0800 Subject: [PATCH 08/25] Add test that makees me realize the solution doesnt work --- src/sentry/deletions/defaults/detector.py | 1 + tests/sentry/deletions/test_detector.py | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py index 0215e13f60fa88..b387f2b2baa37f 100644 --- a/src/sentry/deletions/defaults/detector.py +++ b/src/sentry/deletions/defaults/detector.py @@ -13,6 +13,7 @@ def get_child_relations(self, instance: Detector) -> list[BaseRelation]: data_sources = DataSource.objects.filter(detector=instance.id) delete = True + # this doesn't work if a data source is also connected to a different detector that's not being deleted for data_source in data_sources: for detector in data_source.detectors.all(): if detector.id != instance.id: diff --git a/tests/sentry/deletions/test_detector.py b/tests/sentry/deletions/test_detector.py index ed3457e07897a2..1666ec8cc7a9a1 100644 --- a/tests/sentry/deletions/test_detector.py +++ b/tests/sentry/deletions/test_detector.py @@ -76,3 +76,24 @@ def test_data_source_not_deleted(self): assert not DataCondition.objects.filter(id=self.data_condition.id).exists() assert DataSource.objects.filter(id=self.data_source.id).exists() assert DataSourceDetector.objects.filter(id=data_source_detector_2.id).exists() + + def test_multiple_data_sources(self): + """ + Test that if we have multiple data sources where one is connected to another Detector but the other one isn't, we only delete one + """ + data_condition_group = self.create_data_condition_group() + self.create_data_condition(condition_group=data_condition_group) + detector = self.create_detector( + project_id=self.project.id, + name="Testy Detector", + type=MetricAlertFire.slug, + workflow_condition_group=data_condition_group, + ) + data_source_2 = self.create_data_source(organization=self.organization) + + # multiple data sources for one detector + self.create_data_source_detector(data_source=data_source_2, detector=self.detector) + # but the data source is also connected to a different detector + self.create_data_source_detector(data_source=data_source_2, detector=detector) + assert not DataSource.objects.filter(id=self.data_source.id).exists() + assert DataSource.objects.filter(id=data_source_2.id).exists() From fda76f67fa21119df33cc53a57a8e750fd6c8f87 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 3 Feb 2025 12:54:40 -0800 Subject: [PATCH 09/25] assume 1 detector per data source --- src/sentry/deletions/defaults/detector.py | 23 ++++---- tests/sentry/deletions/test_detector.py | 64 ++++++----------------- 2 files changed, 26 insertions(+), 61 deletions(-) diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py index b387f2b2baa37f..454ff446b8c2ce 100644 --- a/src/sentry/deletions/defaults/detector.py +++ b/src/sentry/deletions/defaults/detector.py @@ -4,23 +4,22 @@ class DetectorDeletionTask(ModelDeletionTask[Detector]): def get_child_relations(self, instance: Detector) -> list[BaseRelation]: + from sentry.snuba.models import QuerySubscription, SnubaQuery from sentry.workflow_engine.models import DataConditionGroup, DataSource + # XXX: this assumes a data source is connected to a single detector. it's not possible in the UI + # to do anything else today, but if this changes we'll need to add custom conditional deletion logic model_relations = [ - ModelRelation(DataConditionGroup, {"id": instance.workflow_condition_group.id}) + ModelRelation(DataConditionGroup, {"id": instance.workflow_condition_group.id}), + ModelRelation(DataSource, {"detector": instance.id}), ] + data_source = DataSource.objects.filter(detector=instance.id).first() + subscription = QuerySubscription.objects.filter(id=data_source.query_id).first() - data_sources = DataSource.objects.filter(detector=instance.id) - delete = True + if data_source: + model_relations.append(ModelRelation(QuerySubscription, {"id": data_source.query_id})) - # this doesn't work if a data source is also connected to a different detector that's not being deleted - for data_source in data_sources: - for detector in data_source.detectors.all(): - if detector.id != instance.id: - delete = False - break - - if delete: - model_relations.append(ModelRelation(DataSource, {"detector": instance.id})) + if subscription: + model_relations.append(ModelRelation(SnubaQuery, {"id": subscription.snuba_query.id})) return model_relations diff --git a/tests/sentry/deletions/test_detector.py b/tests/sentry/deletions/test_detector.py index 1666ec8cc7a9a1..c38e9e7e80985d 100644 --- a/tests/sentry/deletions/test_detector.py +++ b/tests/sentry/deletions/test_detector.py @@ -1,6 +1,6 @@ from sentry.deletions.tasks.scheduled import run_scheduled_deletions from sentry.incidents.grouptype import MetricAlertFire -from sentry.testutils.cases import TestCase +from sentry.snuba.models import QuerySubscription, SnubaQuery from sentry.testutils.hybrid_cloud import HybridCloudTestMixin from sentry.workflow_engine.models import ( DataCondition, @@ -10,13 +10,23 @@ Detector, DetectorWorkflow, ) +from tests.sentry.workflow_engine.test_base import BaseWorkflowTest -class DeleteDetectorTest(TestCase, HybridCloudTestMixin): +class DeleteDetectorTest(BaseWorkflowTest, HybridCloudTestMixin): def setUp(self): self.data_condition_group = self.create_data_condition_group() self.data_condition = self.create_data_condition(condition_group=self.data_condition_group) - self.data_source = self.create_data_source(organization=self.organization) + self.snuba_query = self.create_snuba_query() + self.subscription = QuerySubscription.objects.create( + project=self.project, + status=QuerySubscription.Status.ACTIVE.value, + subscription_id="123", + snuba_query=self.snuba_query, + ) + self.data_source = self.create_data_source( + organization=self.organization, query_id=self.subscription.id + ) self.detector = self.create_detector( project_id=self.project.id, name="Test Detector", @@ -51,49 +61,5 @@ def test_simple(self): assert not DataSource.objects.filter( id__in=[self.data_source.id, data_source_2.id] ).exists() - - def test_data_source_not_deleted(self): - """ - Test that we do not delete a DataSource that is connected to another Detector - """ - detector_2 = self.create_detector( - project_id=self.project.id, - name="Testy Detector", - type=MetricAlertFire.slug, - ) - data_source_detector_2 = self.create_data_source_detector( - data_source=self.data_source, detector=detector_2 - ) - self.ScheduledDeletion.schedule(instance=self.detector, days=0) - - with self.tasks(): - run_scheduled_deletions() - - assert not Detector.objects.filter(id=self.detector.id).exists() - assert not DataSourceDetector.objects.filter(id=self.data_source_detector.id).exists() - assert not DetectorWorkflow.objects.filter(id=self.detector_workflow.id).exists() - assert not DataConditionGroup.objects.filter(id=self.data_condition_group.id).exists() - assert not DataCondition.objects.filter(id=self.data_condition.id).exists() - assert DataSource.objects.filter(id=self.data_source.id).exists() - assert DataSourceDetector.objects.filter(id=data_source_detector_2.id).exists() - - def test_multiple_data_sources(self): - """ - Test that if we have multiple data sources where one is connected to another Detector but the other one isn't, we only delete one - """ - data_condition_group = self.create_data_condition_group() - self.create_data_condition(condition_group=data_condition_group) - detector = self.create_detector( - project_id=self.project.id, - name="Testy Detector", - type=MetricAlertFire.slug, - workflow_condition_group=data_condition_group, - ) - data_source_2 = self.create_data_source(organization=self.organization) - - # multiple data sources for one detector - self.create_data_source_detector(data_source=data_source_2, detector=self.detector) - # but the data source is also connected to a different detector - self.create_data_source_detector(data_source=data_source_2, detector=detector) - assert not DataSource.objects.filter(id=self.data_source.id).exists() - assert DataSource.objects.filter(id=data_source_2.id).exists() + assert not QuerySubscription.objects.filter(id=self.subscription.id).exists() + assert not SnubaQuery.objects.filter(id=self.subscription.snuba_query.id).exists() From 7620751aa2346e3ed39b127166993e5c3b5611a3 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 3 Feb 2025 13:49:50 -0800 Subject: [PATCH 10/25] typing --- src/sentry/deletions/defaults/detector.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py index 454ff446b8c2ce..7d6afc1baec7c0 100644 --- a/src/sentry/deletions/defaults/detector.py +++ b/src/sentry/deletions/defaults/detector.py @@ -9,17 +9,23 @@ def get_child_relations(self, instance: Detector) -> list[BaseRelation]: # XXX: this assumes a data source is connected to a single detector. it's not possible in the UI # to do anything else today, but if this changes we'll need to add custom conditional deletion logic - model_relations = [ - ModelRelation(DataConditionGroup, {"id": instance.workflow_condition_group.id}), - ModelRelation(DataSource, {"detector": instance.id}), - ] + + model_relations: list[BaseRelation] = [ModelRelation(DataSource, {"detector": instance.id})] + + if instance.workflow_condition_group: + model_relations.append( + ModelRelation(DataConditionGroup, {"id": instance.workflow_condition_group.id}) + ) + data_source = DataSource.objects.filter(detector=instance.id).first() - subscription = QuerySubscription.objects.filter(id=data_source.query_id).first() if data_source: model_relations.append(ModelRelation(QuerySubscription, {"id": data_source.query_id})) + subscription = QuerySubscription.objects.filter(id=data_source.query_id).first() - if subscription: - model_relations.append(ModelRelation(SnubaQuery, {"id": subscription.snuba_query.id})) + if subscription: + model_relations.append( + ModelRelation(SnubaQuery, {"id": subscription.snuba_query.id}) + ) return model_relations From f0698891c29b3286667d855192c98703941027a2 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 3 Feb 2025 14:21:18 -0800 Subject: [PATCH 11/25] prevent deleting error detector --- .../endpoints/project_detector_details.py | 4 ++++ .../test_project_detector_details.py | 21 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/sentry/workflow_engine/endpoints/project_detector_details.py b/src/sentry/workflow_engine/endpoints/project_detector_details.py index 973004ff7c9138..8ea11df067c7c7 100644 --- a/src/sentry/workflow_engine/endpoints/project_detector_details.py +++ b/src/sentry/workflow_engine/endpoints/project_detector_details.py @@ -17,6 +17,7 @@ ) from sentry.apidocs.parameters import DetectorParams, GlobalParams from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion +from sentry.grouping.grouptype import ErrorGroupType from sentry.models.project import Project from sentry.workflow_engine.endpoints.serializers import DetectorSerializer from sentry.workflow_engine.models import Detector @@ -90,6 +91,9 @@ def delete(self, request: Request, project: Project, detector: Detector): """ Delete a detector """ + if detector.type == ErrorGroupType.slug: + return Response(status=403) + RegionScheduledDeletion.schedule(detector, days=0, actor=request.user) # TODO add audit log entry return Response(status=204) diff --git a/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py b/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py index b586c4614a5c2c..470aed5a050616 100644 --- a/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py @@ -1,5 +1,6 @@ from sentry.api.serializers import serialize from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion +from sentry.grouping.grouptype import ErrorGroupType from sentry.incidents.grouptype import MetricAlertFire from sentry.testutils.cases import APITestCase from sentry.testutils.outbox import outbox_runner @@ -50,3 +51,23 @@ def test_simple(self): assert RegionScheduledDeletion.objects.filter( model_name="Detector", object_id=self.detector.id ).exists() + + def test_error_group_type(self): + """ + Test that we do not delete the required error detector + """ + data_condition_group = self.create_data_condition_group() + error_detector = self.create_detector( + project_id=self.project.id, + name="Error Detector", + type=ErrorGroupType.slug, + workflow_condition_group=data_condition_group, + ) + with outbox_runner(): + self.get_error_response( + self.organization.slug, self.project.slug, error_detector.id, status_code=403 + ) + + assert not RegionScheduledDeletion.objects.filter( + model_name="Detector", object_id=error_detector.id + ).exists() From f1891a5adc787036cc0cea65a198f4729ca11b8f Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 3 Feb 2025 15:12:54 -0800 Subject: [PATCH 12/25] create separate deletion task for data source --- src/sentry/deletions/__init__.py | 3 ++- src/sentry/deletions/defaults/__init__.py | 1 + src/sentry/deletions/defaults/data_source.py | 9 +++++++++ src/sentry/deletions/defaults/detector.py | 12 ------------ tests/sentry/deletions/test_detector.py | 3 +-- 5 files changed, 13 insertions(+), 15 deletions(-) create mode 100644 src/sentry/deletions/defaults/data_source.py diff --git a/src/sentry/deletions/__init__.py b/src/sentry/deletions/__init__.py index a65c0eed5416ef..60fc70b27b1dd0 100644 --- a/src/sentry/deletions/__init__.py +++ b/src/sentry/deletions/__init__.py @@ -111,7 +111,7 @@ def load_defaults(manager: DeletionTaskManager) -> None: from sentry.sentry_apps.models.sentry_app_installation_token import SentryAppInstallationToken from sentry.sentry_apps.models.servicehook import ServiceHook from sentry.snuba import models as snuba_models - from sentry.workflow_engine.models import Detector + from sentry.workflow_engine.models import DataSource, Detector from . import defaults @@ -127,6 +127,7 @@ def load_defaults(manager: DeletionTaskManager) -> None: manager.register(models.CommitAuthor, defaults.CommitAuthorDeletionTask) manager.register(CommitFileChange, BulkModelDeletionTask) manager.register(Detector, defaults.DetectorDeletionTask) + manager.register(DataSource, defaults.DataSourceDeletionTask) manager.register(models.Deploy, BulkModelDeletionTask) manager.register(DiscoverSavedQuery, defaults.DiscoverSavedQueryDeletionTask) manager.register(DiscoverSavedQueryProject, BulkModelDeletionTask) diff --git a/src/sentry/deletions/defaults/__init__.py b/src/sentry/deletions/defaults/__init__.py index 23bb3f8c21786b..216288ad992e82 100644 --- a/src/sentry/deletions/defaults/__init__.py +++ b/src/sentry/deletions/defaults/__init__.py @@ -5,6 +5,7 @@ from .artifactbundle import * # noqa: F401,F403 from .commit import * # noqa: F401,F403 from .commitauthor import * # noqa: F401,F403 +from .data_source import * # noqa: F401,F403 from .detector import * # noqa: F401,F403 from .discoversavedquery import * # noqa: F401,F403 from .group import * # noqa: F401,F403 diff --git a/src/sentry/deletions/defaults/data_source.py b/src/sentry/deletions/defaults/data_source.py new file mode 100644 index 00000000000000..7a28746aea1e42 --- /dev/null +++ b/src/sentry/deletions/defaults/data_source.py @@ -0,0 +1,9 @@ +from sentry.deletions.base import BaseRelation, ModelDeletionTask, ModelRelation +from sentry.workflow_engine.models.data_source import DataSource + + +class DataSourceDeletionTask(ModelDeletionTask[DataSource]): + def get_child_relations(self, instance: DataSource) -> list[BaseRelation]: + from sentry.snuba.models import QuerySubscription + + return [ModelRelation(QuerySubscription, {"id": instance.query_id})] diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py index 7d6afc1baec7c0..5a99d9556c004e 100644 --- a/src/sentry/deletions/defaults/detector.py +++ b/src/sentry/deletions/defaults/detector.py @@ -4,7 +4,6 @@ class DetectorDeletionTask(ModelDeletionTask[Detector]): def get_child_relations(self, instance: Detector) -> list[BaseRelation]: - from sentry.snuba.models import QuerySubscription, SnubaQuery from sentry.workflow_engine.models import DataConditionGroup, DataSource # XXX: this assumes a data source is connected to a single detector. it's not possible in the UI @@ -17,15 +16,4 @@ def get_child_relations(self, instance: Detector) -> list[BaseRelation]: ModelRelation(DataConditionGroup, {"id": instance.workflow_condition_group.id}) ) - data_source = DataSource.objects.filter(detector=instance.id).first() - - if data_source: - model_relations.append(ModelRelation(QuerySubscription, {"id": data_source.query_id})) - subscription = QuerySubscription.objects.filter(id=data_source.query_id).first() - - if subscription: - model_relations.append( - ModelRelation(SnubaQuery, {"id": subscription.snuba_query.id}) - ) - return model_relations diff --git a/tests/sentry/deletions/test_detector.py b/tests/sentry/deletions/test_detector.py index c38e9e7e80985d..93ae8202ab49e4 100644 --- a/tests/sentry/deletions/test_detector.py +++ b/tests/sentry/deletions/test_detector.py @@ -1,6 +1,6 @@ from sentry.deletions.tasks.scheduled import run_scheduled_deletions from sentry.incidents.grouptype import MetricAlertFire -from sentry.snuba.models import QuerySubscription, SnubaQuery +from sentry.snuba.models import QuerySubscription from sentry.testutils.hybrid_cloud import HybridCloudTestMixin from sentry.workflow_engine.models import ( DataCondition, @@ -62,4 +62,3 @@ def test_simple(self): id__in=[self.data_source.id, data_source_2.id] ).exists() assert not QuerySubscription.objects.filter(id=self.subscription.id).exists() - assert not SnubaQuery.objects.filter(id=self.subscription.snuba_query.id).exists() From 716e88843d2dbd54f4200265ef755b30abc51789 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 3 Feb 2025 15:43:32 -0800 Subject: [PATCH 13/25] oops --- .../endpoints/test_project_detector_details.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py b/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py index 470aed5a050616..9f61e1b2d1e662 100644 --- a/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_project_detector_details.py @@ -13,9 +13,7 @@ class ProjectDetectorDetailsBaseTest(APITestCase): def setUp(self): super().setUp() self.login_as(user=self.user) - self.data_source = self.data_source = self.create_data_source( - organization=self.organization - ) + self.data_source = self.create_data_source(organization=self.organization) self.data_condition_group = self.create_data_condition_group() self.detector = self.create_detector( project_id=self.project.id, From 845a106cb3c15ae38a5308a4f6bcc186b4cab7de Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 4 Feb 2025 12:32:03 -0800 Subject: [PATCH 14/25] conditionally delete datasource --- src/sentry/deletions/defaults/detector.py | 10 ++++++--- tests/sentry/db/test_silo_models.py | 2 ++ tests/sentry/deletions/test_detector.py | 25 +++++++++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py index 5a99d9556c004e..704619e3859433 100644 --- a/src/sentry/deletions/defaults/detector.py +++ b/src/sentry/deletions/defaults/detector.py @@ -6,10 +6,14 @@ class DetectorDeletionTask(ModelDeletionTask[Detector]): def get_child_relations(self, instance: Detector) -> list[BaseRelation]: from sentry.workflow_engine.models import DataConditionGroup, DataSource - # XXX: this assumes a data source is connected to a single detector. it's not possible in the UI - # to do anything else today, but if this changes we'll need to add custom conditional deletion logic + model_relations: list[BaseRelation] = [] - model_relations: list[BaseRelation] = [ModelRelation(DataSource, {"detector": instance.id})] + # check that no other rows are related to the data source + data_source_ids = DataSource.objects.filter(detector=instance.id).values_list( + "id", flat=True + ) + if Detector.objects.filter(data_sources__in=[data_source_ids[0]]).count() == 1: + model_relations.append(ModelRelation(DataSource, {"detector": instance.id})) if instance.workflow_condition_group: model_relations.append( diff --git a/tests/sentry/db/test_silo_models.py b/tests/sentry/db/test_silo_models.py index 2a81fb339f1e08..5c2b5ec07480cd 100644 --- a/tests/sentry/db/test_silo_models.py +++ b/tests/sentry/db/test_silo_models.py @@ -3,6 +3,7 @@ from django.db.models import Model from sentry.api.serializers.base import registry +from sentry.testutils.pytest.fixtures import django_db_all from sentry.testutils.silo import ( validate_models_have_silos, validate_no_cross_silo_deletions, @@ -22,6 +23,7 @@ def test_silo_foreign_keys(): raise ValueError(f"fk_exemptions includes non conflicting relation {unused!r}") +@django_db_all def test_cross_silo_deletions(): validate_no_cross_silo_deletions(fk_exemptions) diff --git a/tests/sentry/deletions/test_detector.py b/tests/sentry/deletions/test_detector.py index 93ae8202ab49e4..4cc6ab61cc2846 100644 --- a/tests/sentry/deletions/test_detector.py +++ b/tests/sentry/deletions/test_detector.py @@ -62,3 +62,28 @@ def test_simple(self): id__in=[self.data_source.id, data_source_2.id] ).exists() assert not QuerySubscription.objects.filter(id=self.subscription.id).exists() + + def test_data_source_not_deleted(self): + """ + Test that we do not delete a DataSource that is connected to another Detector + """ + detector_2 = self.create_detector( + project_id=self.project.id, + name="Testy Detector", + type=MetricAlertFire.slug, + ) + data_source_detector_2 = self.create_data_source_detector( + data_source=self.data_source, detector=detector_2 + ) + self.ScheduledDeletion.schedule(instance=self.detector, days=0) + + with self.tasks(): + run_scheduled_deletions() + + assert not Detector.objects.filter(id=self.detector.id).exists() + assert not DataSourceDetector.objects.filter(id=self.data_source_detector.id).exists() + assert not DetectorWorkflow.objects.filter(id=self.detector_workflow.id).exists() + assert not DataConditionGroup.objects.filter(id=self.data_condition_group.id).exists() + assert not DataCondition.objects.filter(id=self.data_condition.id).exists() + assert DataSource.objects.filter(id=self.data_source.id).exists() + assert DataSourceDetector.objects.filter(id=data_source_detector_2.id).exists() From 3f79434b0812243f5df4a5347863a7f841f38d0b Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 4 Feb 2025 12:57:52 -0800 Subject: [PATCH 15/25] update tests, check for snubaquery usage --- src/sentry/deletions/defaults/data_source.py | 14 ++++++-- tests/sentry/deletions/test_detector.py | 35 ++++++++++++++++++-- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/sentry/deletions/defaults/data_source.py b/src/sentry/deletions/defaults/data_source.py index 7a28746aea1e42..5ee6e2e6fe4601 100644 --- a/src/sentry/deletions/defaults/data_source.py +++ b/src/sentry/deletions/defaults/data_source.py @@ -4,6 +4,16 @@ class DataSourceDeletionTask(ModelDeletionTask[DataSource]): def get_child_relations(self, instance: DataSource) -> list[BaseRelation]: - from sentry.snuba.models import QuerySubscription + from sentry.incidents.models.alert_rule import AlertRule + from sentry.snuba.models import QuerySubscription, SnubaQuery - return [ModelRelation(QuerySubscription, {"id": instance.query_id})] + model_relations: list[BaseRelation] = [ + ModelRelation(QuerySubscription, {"id": instance.query_id}) + ] + + query_sub = QuerySubscription.objects.get(id=instance.query_id) + if AlertRule.objects.filter(snuba_query=query_sub.snuba_query).count() > 1: + return model_relations + + model_relations.append(ModelRelation(SnubaQuery, {"id": query_sub.snuba_query.id})) + return model_relations diff --git a/tests/sentry/deletions/test_detector.py b/tests/sentry/deletions/test_detector.py index 4cc6ab61cc2846..eca02702ca4808 100644 --- a/tests/sentry/deletions/test_detector.py +++ b/tests/sentry/deletions/test_detector.py @@ -1,6 +1,6 @@ from sentry.deletions.tasks.scheduled import run_scheduled_deletions from sentry.incidents.grouptype import MetricAlertFire -from sentry.snuba.models import QuerySubscription +from sentry.snuba.models import QuerySubscription, SnubaQuery from sentry.testutils.hybrid_cloud import HybridCloudTestMixin from sentry.workflow_engine.models import ( DataCondition, @@ -42,7 +42,31 @@ def setUp(self): ) def test_simple(self): - data_source_2 = self.create_data_source(organization=self.organization) + self.ScheduledDeletion.schedule(instance=self.detector, days=0) + + with self.tasks(): + run_scheduled_deletions() + + assert not Detector.objects.filter(id=self.detector.id).exists() + assert not DataSourceDetector.objects.filter(id=self.data_source_detector.id).exists() + assert not DetectorWorkflow.objects.filter(id=self.detector_workflow.id).exists() + assert not DataConditionGroup.objects.filter(id=self.data_condition_group.id).exists() + assert not DataCondition.objects.filter(id=self.data_condition.id).exists() + assert not DataSource.objects.filter(id=self.data_source.id).exists() + assert not QuerySubscription.objects.filter(id=self.subscription.id).exists() + assert not SnubaQuery.objects.filter(id=self.snuba_query.id).exists() + + def test_multiple_data_sources(self): + snuba_query_2 = self.create_snuba_query() + subscription_2 = QuerySubscription.objects.create( + project=self.project, + status=QuerySubscription.Status.ACTIVE.value, + subscription_id="456", + snuba_query=snuba_query_2, + ) + data_source_2 = self.create_data_source( + organization=self.organization, query_id=subscription_2.id + ) data_source_detector_2 = self.create_data_source_detector( data_source=data_source_2, detector=self.detector ) @@ -61,7 +85,12 @@ def test_simple(self): assert not DataSource.objects.filter( id__in=[self.data_source.id, data_source_2.id] ).exists() - assert not QuerySubscription.objects.filter(id=self.subscription.id).exists() + assert not QuerySubscription.objects.filter( + id__in=[self.subscription.id, subscription_2.id] + ).exists() + assert not SnubaQuery.objects.filter( + id__in=[self.snuba_query.id, snuba_query_2.id] + ).exists() def test_data_source_not_deleted(self): """ From cfeb4deabeca7f8f0f53d1d3f3ecdcacce44c194 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 4 Feb 2025 13:30:52 -0800 Subject: [PATCH 16/25] add datasource deletion tests, conditionally delete snubaquery --- src/sentry/deletions/defaults/data_source.py | 8 ++-- tests/sentry/deletions/test_data_source.py | 44 ++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) create mode 100644 tests/sentry/deletions/test_data_source.py diff --git a/src/sentry/deletions/defaults/data_source.py b/src/sentry/deletions/defaults/data_source.py index 5ee6e2e6fe4601..2436ce9f59654d 100644 --- a/src/sentry/deletions/defaults/data_source.py +++ b/src/sentry/deletions/defaults/data_source.py @@ -4,7 +4,6 @@ class DataSourceDeletionTask(ModelDeletionTask[DataSource]): def get_child_relations(self, instance: DataSource) -> list[BaseRelation]: - from sentry.incidents.models.alert_rule import AlertRule from sentry.snuba.models import QuerySubscription, SnubaQuery model_relations: list[BaseRelation] = [ @@ -12,8 +11,9 @@ def get_child_relations(self, instance: DataSource) -> list[BaseRelation]: ] query_sub = QuerySubscription.objects.get(id=instance.query_id) - if AlertRule.objects.filter(snuba_query=query_sub.snuba_query).count() > 1: - return model_relations + snuba_query = SnubaQuery.objects.get(id=query_sub.snuba_query.id) + # if there are more than 1 querysubscriptions for the related snubaquery, don't delete the snubaquery + if QuerySubscription.objects.filter(snuba_query_id=snuba_query.id).count() == 1: + model_relations.append(ModelRelation(SnubaQuery, {"id": query_sub.snuba_query.id})) - model_relations.append(ModelRelation(SnubaQuery, {"id": query_sub.snuba_query.id})) return model_relations diff --git a/tests/sentry/deletions/test_data_source.py b/tests/sentry/deletions/test_data_source.py new file mode 100644 index 00000000000000..74490b4443ca10 --- /dev/null +++ b/tests/sentry/deletions/test_data_source.py @@ -0,0 +1,44 @@ +from sentry.deletions.tasks.scheduled import run_scheduled_deletions +from sentry.snuba.models import QuerySubscription, SnubaQuery +from sentry.testutils.hybrid_cloud import HybridCloudTestMixin +from sentry.workflow_engine.models import DataSource +from tests.sentry.workflow_engine.test_base import BaseWorkflowTest + + +class DeleteDataSourceTest(BaseWorkflowTest, HybridCloudTestMixin): + def setUp(self): + self.alert_rule = self.create_alert_rule() + self.snuba_query = self.alert_rule.snuba_query + self.subscription = QuerySubscription.objects.get(snuba_query_id=self.snuba_query.id) + self.data_source = self.create_data_source( + organization=self.organization, query_id=self.subscription.id + ) + + def test_simple(self): + self.ScheduledDeletion.schedule(instance=self.data_source, days=0) + + with self.tasks(): + run_scheduled_deletions() + + assert not DataSource.objects.filter(id=self.data_source.id).exists() + assert not QuerySubscription.objects.filter(id=self.subscription.id).exists() + assert not SnubaQuery.objects.filter(id=self.snuba_query.id).exists() + + def test_snuba_query_shared(self): + """ + Test that if a SnubaQuery is shared amongst multiple QuerySubscriptions we do not delete it + """ + alert_rule_2 = self.create_alert_rule() + sub_2 = QuerySubscription.objects.get(snuba_query_id=alert_rule_2.snuba_query.id) + sub_2.update(snuba_query=self.snuba_query) + sub_2.save() + sub_2.refresh_from_db() + + self.ScheduledDeletion.schedule(instance=self.data_source, days=0) + + with self.tasks(): + run_scheduled_deletions() + + assert not DataSource.objects.filter(id=self.data_source.id).exists() + assert not QuerySubscription.objects.filter(id=self.subscription.id).exists() + assert SnubaQuery.objects.filter(id=self.snuba_query.id).exists() From ac55163270d28abd69b2caf023dc93f85e9aeffd Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 4 Feb 2025 13:58:44 -0800 Subject: [PATCH 17/25] add checks around existence --- src/sentry/deletions/defaults/data_source.py | 12 ++++++++++-- src/sentry/deletions/defaults/detector.py | 5 +++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/sentry/deletions/defaults/data_source.py b/src/sentry/deletions/defaults/data_source.py index 2436ce9f59654d..f06b2114d1fbc6 100644 --- a/src/sentry/deletions/defaults/data_source.py +++ b/src/sentry/deletions/defaults/data_source.py @@ -9,9 +9,17 @@ def get_child_relations(self, instance: DataSource) -> list[BaseRelation]: model_relations: list[BaseRelation] = [ ModelRelation(QuerySubscription, {"id": instance.query_id}) ] + query_sub = None + try: + query_sub = QuerySubscription.objects.get(id=instance.query_id) + except QuerySubscription.DoesNotExist: + return model_relations - query_sub = QuerySubscription.objects.get(id=instance.query_id) - snuba_query = SnubaQuery.objects.get(id=query_sub.snuba_query.id) + if query_sub: + try: + snuba_query = SnubaQuery.objects.get(id=query_sub.snuba_query.id) + except SnubaQuery.DoesNotExist: + return model_relations # if there are more than 1 querysubscriptions for the related snubaquery, don't delete the snubaquery if QuerySubscription.objects.filter(snuba_query_id=snuba_query.id).count() == 1: model_relations.append(ModelRelation(SnubaQuery, {"id": query_sub.snuba_query.id})) diff --git a/src/sentry/deletions/defaults/detector.py b/src/sentry/deletions/defaults/detector.py index 704619e3859433..e0a511bcaf464c 100644 --- a/src/sentry/deletions/defaults/detector.py +++ b/src/sentry/deletions/defaults/detector.py @@ -12,8 +12,9 @@ def get_child_relations(self, instance: Detector) -> list[BaseRelation]: data_source_ids = DataSource.objects.filter(detector=instance.id).values_list( "id", flat=True ) - if Detector.objects.filter(data_sources__in=[data_source_ids[0]]).count() == 1: - model_relations.append(ModelRelation(DataSource, {"detector": instance.id})) + if data_source_ids: + if Detector.objects.filter(data_sources__in=[data_source_ids[0]]).count() == 1: + model_relations.append(ModelRelation(DataSource, {"detector": instance.id})) if instance.workflow_condition_group: model_relations.append( From f6ceec54f4aa308cf1b5217386ba8768cecd890c Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 5 Feb 2025 11:01:43 -0800 Subject: [PATCH 18/25] remove unnecessary stuff --- src/sentry/deletions/defaults/data_source.py | 22 +-------- .../deletions/defaults/querysubscription.py | 11 ++++- tests/sentry/deletions/test_data_source.py | 22 +-------- .../deletions/test_querysubscription.py | 48 +++++++++++++++++++ 4 files changed, 61 insertions(+), 42 deletions(-) create mode 100644 tests/sentry/deletions/test_querysubscription.py diff --git a/src/sentry/deletions/defaults/data_source.py b/src/sentry/deletions/defaults/data_source.py index f06b2114d1fbc6..7a28746aea1e42 100644 --- a/src/sentry/deletions/defaults/data_source.py +++ b/src/sentry/deletions/defaults/data_source.py @@ -4,24 +4,6 @@ class DataSourceDeletionTask(ModelDeletionTask[DataSource]): def get_child_relations(self, instance: DataSource) -> list[BaseRelation]: - from sentry.snuba.models import QuerySubscription, SnubaQuery + from sentry.snuba.models import QuerySubscription - model_relations: list[BaseRelation] = [ - ModelRelation(QuerySubscription, {"id": instance.query_id}) - ] - query_sub = None - try: - query_sub = QuerySubscription.objects.get(id=instance.query_id) - except QuerySubscription.DoesNotExist: - return model_relations - - if query_sub: - try: - snuba_query = SnubaQuery.objects.get(id=query_sub.snuba_query.id) - except SnubaQuery.DoesNotExist: - return model_relations - # if there are more than 1 querysubscriptions for the related snubaquery, don't delete the snubaquery - if QuerySubscription.objects.filter(snuba_query_id=snuba_query.id).count() == 1: - model_relations.append(ModelRelation(SnubaQuery, {"id": query_sub.snuba_query.id})) - - return model_relations + return [ModelRelation(QuerySubscription, {"id": instance.query_id})] diff --git a/src/sentry/deletions/defaults/querysubscription.py b/src/sentry/deletions/defaults/querysubscription.py index 9c9641706c406f..70318f9cad1f6e 100644 --- a/src/sentry/deletions/defaults/querysubscription.py +++ b/src/sentry/deletions/defaults/querysubscription.py @@ -1,4 +1,4 @@ -from sentry.deletions.base import ModelDeletionTask +from sentry.deletions.base import BaseRelation, ModelDeletionTask, ModelRelation from sentry.snuba.models import QuerySubscription @@ -9,3 +9,12 @@ def delete_instance(self, instance: QuerySubscription) -> None: # Clear the foreign key as the schema was created without a cascade clause Incident.objects.filter(subscription_id=instance.id).update(subscription_id=None) super().delete_instance(instance) + + def get_child_relations(self, instance: QuerySubscription) -> list[BaseRelation]: + from sentry.incidents.models.alert_rule import AlertRule + from sentry.snuba.models import SnubaQuery + + if not AlertRule.objects.filter(snuba_query_id=instance.snuba_query_id).exists(): + return [ModelRelation(SnubaQuery, {"id": instance.snuba_query_id})] + + return [] diff --git a/tests/sentry/deletions/test_data_source.py b/tests/sentry/deletions/test_data_source.py index 74490b4443ca10..a5718e21399172 100644 --- a/tests/sentry/deletions/test_data_source.py +++ b/tests/sentry/deletions/test_data_source.py @@ -1,5 +1,5 @@ from sentry.deletions.tasks.scheduled import run_scheduled_deletions -from sentry.snuba.models import QuerySubscription, SnubaQuery +from sentry.snuba.models import QuerySubscription from sentry.testutils.hybrid_cloud import HybridCloudTestMixin from sentry.workflow_engine.models import DataSource from tests.sentry.workflow_engine.test_base import BaseWorkflowTest @@ -22,23 +22,3 @@ def test_simple(self): assert not DataSource.objects.filter(id=self.data_source.id).exists() assert not QuerySubscription.objects.filter(id=self.subscription.id).exists() - assert not SnubaQuery.objects.filter(id=self.snuba_query.id).exists() - - def test_snuba_query_shared(self): - """ - Test that if a SnubaQuery is shared amongst multiple QuerySubscriptions we do not delete it - """ - alert_rule_2 = self.create_alert_rule() - sub_2 = QuerySubscription.objects.get(snuba_query_id=alert_rule_2.snuba_query.id) - sub_2.update(snuba_query=self.snuba_query) - sub_2.save() - sub_2.refresh_from_db() - - self.ScheduledDeletion.schedule(instance=self.data_source, days=0) - - with self.tasks(): - run_scheduled_deletions() - - assert not DataSource.objects.filter(id=self.data_source.id).exists() - assert not QuerySubscription.objects.filter(id=self.subscription.id).exists() - assert SnubaQuery.objects.filter(id=self.snuba_query.id).exists() diff --git a/tests/sentry/deletions/test_querysubscription.py b/tests/sentry/deletions/test_querysubscription.py new file mode 100644 index 00000000000000..15188ab967a09a --- /dev/null +++ b/tests/sentry/deletions/test_querysubscription.py @@ -0,0 +1,48 @@ +from sentry.deletions.tasks.scheduled import run_scheduled_deletions +from sentry.snuba.models import QuerySubscription, SnubaQuery +from sentry.testutils.hybrid_cloud import HybridCloudTestMixin +from tests.sentry.workflow_engine.test_base import BaseWorkflowTest + + +class DeleteQuerySubscriptionTest(BaseWorkflowTest, HybridCloudTestMixin): + def test_alert_rule(self): + """ + Test that we do not delete a SnubaQuery if it's still attached to an AlertRule + """ + alert_rule = self.create_alert_rule() + snuba_query = alert_rule.snuba_query + subscription = QuerySubscription.objects.get(snuba_query_id=snuba_query.id) + incident = self.create_incident( + projects=[self.project], status=20, alert_rule=alert_rule, subscription=subscription + ) + + self.ScheduledDeletion.schedule(instance=subscription, days=0) + + with self.tasks(): + run_scheduled_deletions() + + assert SnubaQuery.objects.filter(id=snuba_query.id).exists() + incident.refresh_from_db() + assert incident.subscription_id is None + assert not QuerySubscription.objects.filter(id=subscription.id).exists() + + def test_data_source(self): + """ + Test that we delete the related SnubaQuery when the QuerySubscription was linked from a DataSource + """ + snuba_query = self.create_snuba_query() + subscription = QuerySubscription.objects.create( + project=self.project, + status=QuerySubscription.Status.ACTIVE.value, + subscription_id="123", + snuba_query=snuba_query, + ) + self.create_data_source(organization=self.organization, query_id=subscription.id) + + self.ScheduledDeletion.schedule(instance=subscription, days=0) + + with self.tasks(): + run_scheduled_deletions() + + assert not QuerySubscription.objects.filter(id=subscription.id).exists() + assert not SnubaQuery.objects.filter(id=snuba_query.id).exists() From 75cb83f8ccba2d7fc523279d9e543bfc4c060c40 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 5 Feb 2025 11:37:14 -0800 Subject: [PATCH 19/25] update project deletion test --- tests/sentry/deletions/test_project.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/sentry/deletions/test_project.py b/tests/sentry/deletions/test_project.py index 8bdff6f9749ef9..832b3b37714aab 100644 --- a/tests/sentry/deletions/test_project.py +++ b/tests/sentry/deletions/test_project.py @@ -109,14 +109,7 @@ def test_simple(self): date_added=monitor.date_added, status=CheckInStatus.OK, ) - snuba_query = SnubaQuery.objects.create( - environment=env, - type=0, - query="title:ohno", - aggregate="", - time_window=3600, - resolution=60, - ) + snuba_query = metric_alert_rule.snuba_query query_sub = QuerySubscription.objects.create( project=project, snuba_query=snuba_query, From 763532c64a6d7fd8548f4c3b2bfe5cecf7441206 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 7 Feb 2025 11:37:36 -0800 Subject: [PATCH 20/25] add related_model --- src/sentry/deletions/defaults/data_source.py | 6 ++---- src/sentry/snuba/models.py | 5 +++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/sentry/deletions/defaults/data_source.py b/src/sentry/deletions/defaults/data_source.py index 7a28746aea1e42..1d6cd9267d4a17 100644 --- a/src/sentry/deletions/defaults/data_source.py +++ b/src/sentry/deletions/defaults/data_source.py @@ -1,9 +1,7 @@ -from sentry.deletions.base import BaseRelation, ModelDeletionTask, ModelRelation +from sentry.deletions.base import BaseRelation, ModelDeletionTask from sentry.workflow_engine.models.data_source import DataSource class DataSourceDeletionTask(ModelDeletionTask[DataSource]): def get_child_relations(self, instance: DataSource) -> list[BaseRelation]: - from sentry.snuba.models import QuerySubscription - - return [ModelRelation(QuerySubscription, {"id": instance.query_id})] + return instance.type_handler.related_model(instance) diff --git a/src/sentry/snuba/models.py b/src/sentry/snuba/models.py index 2bba4e0c7d6a55..14361686a2c22d 100644 --- a/src/sentry/snuba/models.py +++ b/src/sentry/snuba/models.py @@ -154,3 +154,8 @@ def bulk_get_query_object( for qs in QuerySubscription.objects.filter(id__in=[ds.query_id for ds in data_sources]) } return {ds.id: qs_lookup.get(ds.query_id) for ds in data_sources} + + def related_model(instance): + from sentry.deletions.base import ModelRelation + + return [ModelRelation(QuerySubscription, {"id": instance.query_id})] From e1a5682615da204ed21086b9cc96755742b0f429 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 7 Feb 2025 11:46:30 -0800 Subject: [PATCH 21/25] typing, imports, add to base --- src/sentry/snuba/models.py | 6 +++--- src/sentry/workflow_engine/types.py | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/sentry/snuba/models.py b/src/sentry/snuba/models.py index 14361686a2c22d..18f6fd556bef67 100644 --- a/src/sentry/snuba/models.py +++ b/src/sentry/snuba/models.py @@ -12,6 +12,7 @@ from sentry.backup.scopes import ImportScope, RelocationScope from sentry.db.models import FlexibleForeignKey, Model, region_silo_model from sentry.db.models.manager.base import BaseManager +from sentry.deletions.base import ModelRelation from sentry.incidents.utils.types import DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION from sentry.models.team import Team from sentry.users.models.user import User @@ -155,7 +156,6 @@ def bulk_get_query_object( } return {ds.id: qs_lookup.get(ds.query_id) for ds in data_sources} - def related_model(instance): - from sentry.deletions.base import ModelRelation - + @staticmethod + def related_model(instance) -> list[ModelRelation]: return [ModelRelation(QuerySubscription, {"id": instance.query_id})] diff --git a/src/sentry/workflow_engine/types.py b/src/sentry/workflow_engine/types.py index b8200ab5a217df..7fb63dd871518d 100644 --- a/src/sentry/workflow_engine/types.py +++ b/src/sentry/workflow_engine/types.py @@ -6,6 +6,7 @@ from sentry.types.group import PriorityLevel if TYPE_CHECKING: + from sentry.deletions.base import ModelRelation from sentry.eventstore.models import GroupEvent from sentry.eventstream.base import GroupState from sentry.workflow_engine.models import Action, Detector, Workflow @@ -60,6 +61,10 @@ class DataSourceTypeHandler(Generic[T]): def bulk_get_query_object(data_sources) -> dict[int, T | None]: raise NotImplementedError + @staticmethod + def related_model(instance) -> list[ModelRelation]: + raise NotImplementedError + class DataConditionHandler(Generic[T]): type: ClassVar[DataConditionHandlerType] = DataConditionHandlerType.ACTION_FILTER From 5b25ea8f13ec54dee4e73f5f820ac0db96d226a9 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 7 Feb 2025 13:00:43 -0800 Subject: [PATCH 22/25] check for other query subscriptions for the snuba query --- src/sentry/deletions/defaults/querysubscription.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/deletions/defaults/querysubscription.py b/src/sentry/deletions/defaults/querysubscription.py index 70318f9cad1f6e..8d12a272623952 100644 --- a/src/sentry/deletions/defaults/querysubscription.py +++ b/src/sentry/deletions/defaults/querysubscription.py @@ -15,6 +15,10 @@ def get_child_relations(self, instance: QuerySubscription) -> list[BaseRelation] from sentry.snuba.models import SnubaQuery if not AlertRule.objects.filter(snuba_query_id=instance.snuba_query_id).exists(): - return [ModelRelation(SnubaQuery, {"id": instance.snuba_query_id})] + if ( + QuerySubscription.objects.filter(snuba_query_id=instance.snuba_query_id).count() + == 1 + ): + return [ModelRelation(SnubaQuery, {"id": instance.snuba_query_id})] return [] From 96e4232876ecaf712a8eebfe68bea1cead5f9fdf Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 7 Feb 2025 13:27:58 -0800 Subject: [PATCH 23/25] typing --- src/sentry/deletions/defaults/data_source.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sentry/deletions/defaults/data_source.py b/src/sentry/deletions/defaults/data_source.py index 1d6cd9267d4a17..1e32d66dd44d3e 100644 --- a/src/sentry/deletions/defaults/data_source.py +++ b/src/sentry/deletions/defaults/data_source.py @@ -1,7 +1,9 @@ +from typing import cast + from sentry.deletions.base import BaseRelation, ModelDeletionTask from sentry.workflow_engine.models.data_source import DataSource class DataSourceDeletionTask(ModelDeletionTask[DataSource]): def get_child_relations(self, instance: DataSource) -> list[BaseRelation]: - return instance.type_handler.related_model(instance) + return cast(list[BaseRelation], instance.type_handler.related_model(instance)) From 4265fea592166097a625a854a89afb3c42df2ecf Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 7 Feb 2025 15:45:54 -0800 Subject: [PATCH 24/25] hack to make datasource have a type --- src/sentry/testutils/silo.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/sentry/testutils/silo.py b/src/sentry/testutils/silo.py index c0744794776ea0..2c8acae7f7bc49 100644 --- a/src/sentry/testutils/silo.py +++ b/src/sentry/testutils/silo.py @@ -564,12 +564,19 @@ def validate_no_cross_silo_deletions( ) -> None: from sentry import deletions from sentry.deletions.base import BaseDeletionTask + from sentry.incidents.utils.types import DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION + from sentry.workflow_engine.models.data_source import DataSource + + # hack for datasource registry, needs type + instantiation_params = {DataSource: {"type": DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION}} for model_class in iter_models(app_name): if not hasattr(model_class._meta, "silo_limit"): continue deletion_task: BaseDeletionTask = deletions.get(model=model_class, query={}) - for relation in deletion_task.get_child_relations(model_class()): + for relation in deletion_task.get_child_relations( + model_class(**instantiation_params.get(model_class, {})) + ): to_model = relation.params["model"] if (model_class, to_model) in exemptions or (to_model, model_class) in exemptions: continue From add6b3d9e29492d340f45af228ba4282e090e81e Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 7 Feb 2025 16:02:36 -0800 Subject: [PATCH 25/25] Typing --- src/sentry/testutils/silo.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sentry/testutils/silo.py b/src/sentry/testutils/silo.py index 2c8acae7f7bc49..0357acba2af8f2 100644 --- a/src/sentry/testutils/silo.py +++ b/src/sentry/testutils/silo.py @@ -568,7 +568,9 @@ def validate_no_cross_silo_deletions( from sentry.workflow_engine.models.data_source import DataSource # hack for datasource registry, needs type - instantiation_params = {DataSource: {"type": DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION}} + instantiation_params: dict[type[Model], dict[str, str]] = { + DataSource: {"type": DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION} + } for model_class in iter_models(app_name): if not hasattr(model_class._meta, "silo_limit"):