feat(aci): schema validation for DataCondition.comparison

src/sentry/workflow_engine/handlers/condition/age_comparison_handler.py

@@ -11,13 +11,29 @@
 
 @condition_handler_registry.register(Condition.AGE_COMPARISON)
 class AgeComparisonConditionHandler(DataConditionHandler[WorkflowJob]):
+    @staticmethod
+    def comparison_json_schema() -> dict[str, Any]:
+        return {
+            "type": "object",
+            "properties": {
+                "comparison_type": {
+                    "type": "string",
+                    "enum": [AgeComparisonType.OLDER, AgeComparisonType.NEWER],
+                },
+                "value": {"type": "integer", "minimum": 0},
+                "time": {"type": "string", "enum": list(timeranges.keys())},
+            },
+            "required": ["comparison_type", "value", "time"],
+            "additionalProperties": False,
+        }
+
     @staticmethod
     def evaluate_value(job: WorkflowJob, comparison: Any) -> bool:
         event = job["event"]
         first_seen = event.group.first_seen
         current_time = timezone.now()
-        comparison_type = comparison.get("comparison_type")
-        time = comparison.get("time")
+        comparison_type = comparison["comparison_type"]
+        time = comparison["time"]
 
         if (
             not comparison_type
@@ -31,7 +47,7 @@ def evaluate_value(job: WorkflowJob, comparison: Any) -> bool:
             return False
 
         try:
-            value = int(comparison.get("value"))
+            value = int(comparison["value"])
         except (TypeError, ValueError):
             return False
 

src/sentry/workflow_engine/migration_helpers/issue_alert_conditions.py

@@ -165,10 +165,9 @@ def create_tagged_event_data_condition(
 def create_age_comparison_data_condition(
     data: dict[str, Any], dcg: DataConditionGroup
 ) -> DataCondition:
-    # TODO: Add comparison validation (error if not enough information)
     comparison = {
         "comparison_type": data["comparison_type"],
-        "value": data["value"],
+        "value": int(data["value"]),
         "time": data["time"],
     }
 

src/sentry/workflow_engine/models/data_condition.py

@@ -3,6 +3,8 @@
 from typing import Any, TypeVar, cast
 
 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, region_silo_model, sane_repr
@@ -121,3 +123,15 @@ def evaluate_value(self, value: T) -> DataConditionResult:
 
         result = handler.evaluate_value(value, self.comparison)
         return self.get_condition_result() if result else None
+
+
+@receiver(pre_save, sender=DataCondition)
+def enforce_comparison_schema(sender, instance: DataCondition, **kwargs):
+    from jsonschema import ValidationError, validate
+
+    schema = condition_handler_registry.get(instance.type).comparison_json_schema()
+
+    try:
+        validate(instance.comparison, schema)
+    except ValidationError as e:
+        raise ValidationError(f"Invalid config: {e.message}")

src/sentry/workflow_engine/types.py

@@ -55,6 +55,11 @@ def bulk_get_query_object(data_sources) -> dict[int, T | None]:
 
 
 class DataConditionHandler(Generic[T]):
+    @staticmethod
+    def comparison_json_schema() -> dict[str, Any]:
+        # TODO(cathy): raise NotImplementedError
+        return {}
+
     @staticmethod
     def evaluate_value(value: T, comparison: Any) -> DataConditionResult:
         raise NotImplementedError

tests/sentry/workflow_engine/handlers/condition/test_age_comparison_handler.py

@@ -1,5 +1,8 @@
 from datetime import datetime, timedelta, timezone
 
+import pytest
+from jsonschema import ValidationError
+
 from sentry.rules.age import AgeComparisonType
 from sentry.rules.filters.age_comparison import AgeComparisonFilter
 from sentry.testutils.helpers.datetime import freeze_time
@@ -36,7 +39,7 @@ def setUp(self):
         )
         self.dc = self.create_data_condition(
             type=self.condition,
-            comparison={"comparison_type": AgeComparisonType.OLDER, "value": "10", "time": "hour"},
+            comparison={"comparison_type": AgeComparisonType.OLDER, "value": 10, "time": "hour"},
             condition_result=True,
         )
 
@@ -47,16 +50,30 @@ def test_dual_write(self):
         assert dc.type == self.condition
         assert dc.comparison == {
             "comparison_type": AgeComparisonType.OLDER,
-            "value": "10",
+            "value": 10,
             "time": "hour",
         }
         assert dc.condition_result is True
         assert dc.condition_group == dcg
 
+    def test_json_schema(self):
+        with pytest.raises(ValidationError):
+            self.dc.comparison.update({"time": "asdf"})
+            self.dc.save()
+
+        with pytest.raises(ValidationError):
+            self.dc.comparison.update({"value": "bad_value"})
+            self.dc.save()
+
+        with pytest.raises(ValidationError):
+            self.dc.comparison.update({"comparison_type": "bad_value"})
+            self.dc.save()
+
     def test_older_applies_correctly(self):
-        self.dc.update(
-            comparison={"comparison_type": AgeComparisonType.OLDER, "value": "10", "time": "hour"}
+        self.dc.comparison.update(
+            {"comparison_type": AgeComparisonType.OLDER, "value": 10, "time": "hour"}
         )
+        self.dc.save()
 
         self.group.update(first_seen=datetime.now(timezone.utc) - timedelta(hours=3))
         self.assert_does_not_pass(self.dc, self.job)
@@ -67,22 +84,13 @@ def test_older_applies_correctly(self):
         self.assert_passes(self.dc, self.job)
 
     def test_newer_applies_correctly(self):
-        self.dc.update(
-            comparison={"comparison_type": AgeComparisonType.NEWER, "value": "10", "time": "hour"}
+        self.dc.comparison.update(
+            {"comparison_type": AgeComparisonType.NEWER, "value": 10, "time": "hour"}
         )
+        self.dc.save()
 
         self.group.update(first_seen=datetime.now(timezone.utc) - timedelta(hours=3))
         self.assert_passes(self.dc, self.job)
 
         self.group.update(first_seen=datetime.now(timezone.utc) - timedelta(hours=10))
         self.assert_does_not_pass(self.dc, self.job)
-
-    def test_fails_on_insufficient_data(self):
-        self.dc.update(comparison={"time": "hour"})
-        self.assert_does_not_pass(self.dc, self.job)
-
-        self.dc.update(comparison={"value": "bad_value"})
-        self.assert_does_not_pass(self.dc, self.job)
-
-        self.dc.update(comparison={"comparison_type": "bad_value"})
-        self.assert_does_not_pass(self.dc, self.job)