chore(aci): enforce config schema without subclassing (#81979)

Author: cathteng

Diff for: src/sentry/incidents/grouptype.py

@@ -26,3 +26,4 @@ class MetricAlertFire(GroupType):
     enable_escalation_detection = False
     detector_handler = MetricAlertDetectorHandler
     detector_validator = MetricAlertsDetectorValidator
+    detector_config_schema = {}  # TODO(colleen): update this

Diff for: src/sentry/issues/grouptype.py

@@ -5,7 +5,7 @@
 from dataclasses import dataclass, field
 from datetime import timedelta
 from enum import Enum, StrEnum
-from typing import TYPE_CHECKING, Any
+from typing import TYPE_CHECKING, Any, ClassVar
 
 import sentry_sdk
 from django.apps import apps
@@ -174,6 +174,7 @@ class GroupType:
     notification_config: NotificationConfig = NotificationConfig()
     detector_handler: type[DetectorHandler] | None = None
     detector_validator: type[BaseGroupTypeDetectorValidator] | None = None
+    detector_config_schema: ClassVar[dict[str, Any]] = {}
 
     def __init_subclass__(cls: type[GroupType], **kwargs: Any) -> None:
         super().__init_subclass__(**kwargs)

Diff for: src/sentry/testutils/fixtures.py

@@ -12,6 +12,7 @@
 from sentry.incidents.models.alert_rule import AlertRule, AlertRuleMonitorTypeInt
 from sentry.integrations.models.integration import Integration
 from sentry.integrations.models.organization_integration import OrganizationIntegration
+from sentry.issues.grouptype import ErrorGroupType
 from sentry.models.activity import Activity
 from sentry.models.environment import Environment
 from sentry.models.grouprelease import GroupRelease
@@ -635,12 +636,13 @@ def create_detector(
         self,
         *args,
         project=None,
+        type=ErrorGroupType.slug,
         **kwargs,
     ) -> Detector:
         if project is None:
             project = self.create_project(organization=self.organization)
 
-        return Factories.create_detector(*args, project=project, **kwargs)
+        return Factories.create_detector(*args, project=project, type=type, **kwargs)
 
     def create_detector_state(self, *args, **kwargs) -> DetectorState:
         return Factories.create_detector_state(*args, **kwargs)

Diff for: src/sentry/workflow_engine/models/detector.py

@@ -7,6 +7,8 @@
 from django.conf import settings
 from django.db import models
 from django.db.models import UniqueConstraint
+from django.db.models.signals import pre_save
+from django.dispatch import receiver
 
 from sentry.backup.scopes import RelocationScope
 from sentry.db.models import DefaultFieldsModel, FlexibleForeignKey, region_silo_model
@@ -60,10 +62,6 @@ class Detector(DefaultFieldsModel, OwnerModel, JSONConfigBase):
     # The user that created the detector
     created_by_id = HybridCloudForeignKey(settings.AUTH_USER_MODEL, null=True, on_delete="SET_NULL")
 
-    @property
-    def CONFIG_SCHEMA(self) -> dict[str, Any]:
-        raise NotImplementedError('Subclasses must define a "CONFIG_SCHEMA" attribute')
-
     class Meta(OwnerModel.Meta):
         constraints = OwnerModel.Meta.constraints + [
             UniqueConstraint(
@@ -83,7 +81,6 @@ def detector_handler(self) -> DetectorHandler | None:
             logger.error(
                 "No registered grouptype for detector",
                 extra={
-                    "group_type": str(group_type),
                     "detector_id": self.id,
                     "detector_type": self.type,
                 },
@@ -105,3 +102,12 @@ def detector_handler(self) -> DetectorHandler | None:
     def get_audit_log_data(self) -> dict[str, Any]:
         # TODO: Create proper audit log data for the detector, group and conditions
         return {}
+
+
+@receiver(pre_save, sender=Detector)
+def enforce_config_schema(sender, instance: Detector, **kwargs):
+    group_type = instance.group_type
+    if not group_type:
+        raise ValueError(f"No group type found with type {instance.type}")
+    config_schema = group_type.detector_config_schema
+    instance.validate_config(config_schema)

Diff for: src/sentry/workflow_engine/models/json_config.py

@@ -1,4 +1,3 @@
-from abc import abstractproperty
 from typing import Any
 
 from django.db import models
@@ -8,13 +7,9 @@
 class JSONConfigBase(models.Model):
     config = models.JSONField(db_default={})
 
-    @abstractproperty
-    def CONFIG_SCHEMA(self) -> dict[str, Any]:
-        pass
-
-    def validate_config(self) -> None:
+    def validate_config(self, schema: dict[str, Any]) -> None:
         try:
-            validate(self.config, self.CONFIG_SCHEMA)
+            validate(self.config, schema)
         except ValidationError as e:
             raise ValidationError(f"Invalid config: {e.message}")
 

Diff for: src/sentry/workflow_engine/models/workflow.py

@@ -2,6 +2,8 @@
 
 from django.conf import settings
 from django.db import models
+from django.db.models.signals import pre_save
+from django.dispatch import receiver
 
 from sentry.backup.scopes import RelocationScope
 from sentry.db.models import DefaultFieldsModel, FlexibleForeignKey, region_silo_model, sane_repr
@@ -35,8 +37,9 @@ class Workflow(DefaultFieldsModel, OwnerModel, JSONConfigBase):
     created_by_id = HybridCloudForeignKey(settings.AUTH_USER_MODEL, null=True, on_delete="SET_NULL")
 
     @property
-    def CONFIG_SCHEMA(self) -> dict[str, Any]:
-        raise NotImplementedError('Subclasses must define a "CONFIG_SCHEMA" attribute')
+    def config_schema(self) -> dict[str, Any]:
+        # TODO: fill in
+        return {}
 
     __repr__ = sane_repr("name", "organization_id")
 
@@ -60,3 +63,8 @@ def evaluate_trigger_conditions(self, evt: GroupEvent) -> bool:
 
         evaluation, _ = evaluate_condition_group(self.when_condition_group, evt)
         return evaluation
+
+
+@receiver(pre_save, sender=Workflow)
+def enforce_config_schema(sender, instance: Workflow, **kwargs):
+    instance.validate_config(instance.config_schema)

Diff for: src/sentry/workflow_engine/processors/detector.py

@@ -3,11 +3,12 @@
 import logging
 
 from sentry.eventstore.models import GroupEvent
+from sentry.issues.grouptype import ErrorGroupType
 from sentry.issues.issue_occurrence import IssueOccurrence
 from sentry.issues.producer import PayloadType, produce_occurrence_to_kafka
 from sentry.workflow_engine.handlers.detector import DetectorEvaluationResult
 from sentry.workflow_engine.models import DataPacket, Detector
-from sentry.workflow_engine.types import DetectorGroupKey, DetectorType
+from sentry.workflow_engine.types import DetectorGroupKey
 
 logger = logging.getLogger(__name__)
 
@@ -17,7 +18,7 @@ def get_detector_by_event(evt: GroupEvent) -> Detector:
     issue_occurrence = evt.occurrence
 
     if issue_occurrence is None:
-        detector = Detector.objects.get(project_id=evt.project_id, type=DetectorType.ERROR)
+        detector = Detector.objects.get(project_id=evt.project_id, type=ErrorGroupType.slug)
     else:
         detector = Detector.objects.get(id=issue_occurrence.evidence_data.get("detector_id", None))
 

Diff for: tests/sentry/workflow_engine/models/test_json_config_base.py

@@ -0,0 +1,82 @@
+from dataclasses import dataclass
+from unittest.mock import PropertyMock, patch
+
+import pytest
+from jsonschema import ValidationError
+
+from sentry.issues.grouptype import GroupCategory, GroupType
+from tests.sentry.issues.test_grouptype import BaseGroupTypeTest
+
+
+class TestJsonConfigBase(BaseGroupTypeTest):
+    def setUp(self):
+        super().setUp()
+        self.correct_config = {
+            "username": "user123",
+            "email": "[email protected]",
+            "fullName": "John Doe",
+            "age": 30,
+            "location": "Cityville",
+            "interests": ["Travel", "Technology"],
+        }
+
+        @dataclass(frozen=True)
+        class TestGroupType(GroupType):
+            type_id = 1
+            slug = "test"
+            description = "Test"
+            category = GroupCategory.ERROR.value
+            detector_config_schema = self.example_schema
+
+    @pytest.fixture(autouse=True)
+    def initialize_configs(self):
+        self.example_schema = {
+            "$id": "https://example.com/user-profile.schema.json",
+            "$schema": "https://json-schema.org/draft/2020-12/schema",
+            "description": "A representation of a user profile",
+            "type": "object",
+            "required": ["username", "email"],
+            "properties": {
+                "username": {"type": "string"},
+                "email": {"type": "string", "format": "email"},
+                "fullName": {"type": "string"},
+                "age": {"type": "integer", "minimum": 0},
+                "location": {"type": "string"},
+                "interests": {"type": "array", "items": {"type": "string"}},
+            },
+        }
+        with (
+            patch(
+                "sentry.workflow_engine.models.Workflow.config_schema",
+                return_value=self.example_schema,
+                new_callable=PropertyMock,
+            ),
+        ):
+            # Run test case
+            yield
+
+
+class TestDetectorConfig(TestJsonConfigBase):
+    def test_detector_no_registration(self):
+        with pytest.raises(ValueError):
+            self.create_detector(name="test_detector", type="no_registration")
+
+    def test_detector_mismatched_schema(self):
+        with pytest.raises(ValidationError):
+            self.create_detector(name="test_detector", type="test", config={"hi": "there"})
+
+    def test_detector_correct_schema(self):
+        self.create_detector(name="test_detector", type="test", config=self.correct_config)
+
+
+class TestWorkflowConfig(TestJsonConfigBase):
+    def test_workflow_mismatched_schema(self):
+        with pytest.raises(ValidationError):
+            self.create_workflow(
+                organization=self.organization, name="test_workflow", config={"hi": "there"}
+            )
+
+    def test_workflow_correct_schema(self):
+        self.create_workflow(
+            organization=self.organization, name="test_workflow", config=self.correct_config
+        )

Diff for: tests/sentry/workflow_engine/models/test_workflow.py

@@ -1,11 +1,10 @@
-from sentry.workflow_engine.types import DetectorType
 from tests.sentry.workflow_engine.test_base import BaseWorkflowTest
 
 
 class WorkflowTest(BaseWorkflowTest):
     def setUp(self):
         self.workflow, self.detector, self.detector_workflow, self.data_condition_group = (
-            self.create_detector_and_workflow(detector_type=DetectorType.ERROR)
+            self.create_detector_and_workflow()
         )
         self.data_condition = self.data_condition_group.conditions.first()
         self.group, self.event, self.group_event = self.create_group_event()

Diff for: tests/sentry/workflow_engine/processors/test_detector.py

@@ -121,9 +121,16 @@ def test_state_results_multi_group(self, mock_produce_occurrence_to_kafka):
         )
 
     def test_no_issue_type(self):
-        detector = self.create_detector(type="invalid slug")
+        detector = self.create_detector(type=self.handler_state_type.slug)
         data_packet = self.build_data_packet()
-        with mock.patch("sentry.workflow_engine.models.detector.logger") as mock_logger:
+        with (
+            mock.patch("sentry.workflow_engine.models.detector.logger") as mock_logger,
+            mock.patch(
+                "sentry.workflow_engine.models.Detector.group_type",
+                return_value=None,
+                new_callable=mock.PropertyMock,
+            ),
+        ):
             results = process_detectors(data_packet, [detector])
             assert mock_logger.error.call_args[0][0] == "No registered grouptype for detector"
         assert results == []
@@ -326,7 +333,7 @@ def test_above_below_threshold(self):
         }
 
     def test_no_condition_group(self):
-        detector = self.create_detector()
+        detector = self.create_detector(type=self.handler_type.slug)
         handler = MockDetectorStateHandler(detector)
         with mock.patch(
             "sentry.workflow_engine.handlers.detector.stateful.metrics"

Diff for: tests/sentry/workflow_engine/processors/test_workflow.py

@@ -1,9 +1,9 @@
 from unittest import mock
 
+from sentry.incidents.grouptype import MetricAlertFire
 from sentry.workflow_engine.models import DataConditionGroup
 from sentry.workflow_engine.models.data_condition import Condition
 from sentry.workflow_engine.processors.workflow import evaluate_workflow_triggers, process_workflows
-from sentry.workflow_engine.types import DetectorType
 from tests.sentry.workflow_engine.test_base import BaseWorkflowTest
 
 
@@ -14,13 +14,12 @@ def setUp(self):
             self.detector,
             self.detector_workflow,
             self.workflow_triggers,
-        ) = self.create_detector_and_workflow()
+        ) = self.create_detector_and_workflow(detector_type=MetricAlertFire.slug)
 
         self.error_workflow, self.error_detector, self.detector_workflow_error, _ = (
             self.create_detector_and_workflow(
                 name_prefix="error",
                 workflow_triggers=self.create_data_condition_group(),
-                detector_type=DetectorType.ERROR,
             )
         )
 

Diff for: tests/sentry/workflow_engine/test_base.py

@@ -2,6 +2,7 @@
 from uuid import uuid4
 
 from sentry.eventstore.models import Event, GroupEvent
+from sentry.issues.grouptype import ErrorGroupType
 from sentry.models.group import Group
 from sentry.snuba.models import SnubaQuery
 from sentry.testutils.cases import TestCase
@@ -14,7 +15,6 @@
     Workflow,
 )
 from sentry.workflow_engine.models.data_condition import Condition
-from sentry.workflow_engine.types import DetectorType
 from tests.sentry.issues.test_utils import OccurrenceTestMixin
 
 
@@ -66,7 +66,7 @@ def create_detector_and_workflow(
         self,
         name_prefix="test",
         workflow_triggers: DataConditionGroup | None = None,
-        detector_type: DetectorType | str = "TestDetector",
+        detector_type: str = ErrorGroupType.slug,
         **kwargs,
     ) -> tuple[Workflow, Detector, DetectorWorkflow, DataConditionGroup]:
         workflow_triggers = workflow_triggers or self.create_data_condition_group()