Skip to content

fix(aci milestone 3): check for resolution condition in metric detector validator #102922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 7, 2025
Merged

fix(aci milestone 3): check for resolution condition in metric detector validator #102922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a17f68e
check for resolution condition in validator
mifu67 Nov 6, 2025
54f99e6
some more tests + a little fix
mifu67 Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/sentry/incidents/metric_issue_detector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,13 @@ def schedule_update_project_config(detector: Detector) -> None:

class MetricIssueComparisonConditionValidator(BaseDataConditionValidator):
supported_conditions = frozenset(
(Condition.GREATER, Condition.LESS, Condition.ANOMALY_DETECTION)
(
Condition.GREATER,
Condition.LESS,
Condition.GREATER_OR_EQUAL,
Copy link
Contributor Author

@mifu67 mifu67 Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this and the condition below it are used for resolution

Condition.LESS_OR_EQUAL,
Condition.ANOMALY_DETECTION,
)
)
supported_condition_results = frozenset(
(DetectorPriorityLevel.HIGH, DetectorPriorityLevel.MEDIUM, DetectorPriorityLevel.OK)
Expand Down Expand Up @@ -132,6 +138,12 @@ def validate_conditions(self, value):
MetricIssueComparisonConditionValidator(data=value, many=True).is_valid(
raise_exception=True
)
if not any(
condition["condition_result"] == DetectorPriorityLevel.OK for condition in value
) and not any(condition["type"] == Condition.ANOMALY_DETECTION for condition in value):
raise serializers.ValidationError(
"Resolution condition required for metric issue detector."
)
Copy link
Contributor

@cursor cursor bot Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Validation bypass: raw input used after validation

The validate_conditions method validates the condition data with MetricIssueComparisonConditionValidator but discards the validated data. The resolution check then uses the raw input value instead of the validated data where condition_result has been properly converted to DetectorPriorityLevel enum. This causes the validation to incorrectly fail when condition_result is passed as a string (e.g., "0") rather than an integer, since string-to-enum comparison returns false.

Fix in Cursor Fix in Web

return value


Expand All @@ -146,7 +158,7 @@ def validate(self, attrs):

if "condition_group" in attrs:
conditions = attrs.get("condition_group", {}).get("conditions")
if len(conditions) > 2:
if len(conditions) > 3:
Copy link
Contributor Author

@mifu67 mifu67 Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high, medium, ok

raise serializers.ValidationError("Too many conditions")

return attrs
Expand Down
39 changes: 38 additions & 1 deletion tests/sentry/incidents/endpoints/validators/test_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,12 @@ def setUp(self) -> None:
"conditionResult": DetectorPriorityLevel.HIGH,
"conditionGroupId": self.data_condition_group.id,
},
{
"type": Condition.LESS_OR_EQUAL,
"comparison": 100,
"conditionResult": DetectorPriorityLevel.OK,
"conditionGroupId": self.data_condition_group.id,
},
],
},
"config": {
Expand Down Expand Up @@ -244,7 +250,7 @@ def test_create_with_valid_data(

# Verify conditions in DB
conditions = list(DataCondition.objects.filter(condition_group=condition_group))
assert len(conditions) == 1
assert len(conditions) == 2
condition = conditions[0]
assert condition.type == Condition.GREATER
assert condition.comparison == 100
Expand Down Expand Up @@ -399,6 +405,31 @@ def test_invalid_detector_type(self) -> None:
)
]

def test_no_resolution_condition(self) -> None:
data = {
**self.valid_data,
"conditionGroup": {
"id": self.data_condition_group.id,
"organizationId": self.organization.id,
"logicType": self.data_condition_group.logic_type,
"conditions": [
{
"type": Condition.GREATER,
"comparison": 100,
"conditionResult": DetectorPriorityLevel.HIGH,
"conditionGroupId": self.data_condition_group.id,
},
],
},
}
validator = MetricIssueDetectorValidator(data=data, context=self.context)
assert not validator.is_valid()
assert validator.errors.get("conditionGroup", {}).get("conditions") == [
ErrorDetail(
string="Resolution condition required for metric issue detector.", code="invalid"
)
]

def test_too_many_conditions(self) -> None:
data = {
**self.valid_data,
Expand All @@ -425,6 +456,12 @@ def test_too_many_conditions(self) -> None:
"conditionResult": DetectorPriorityLevel.HIGH,
"conditionGroupId": self.data_condition_group.id,
},
{
"type": Condition.LESS_OR_EQUAL,
"comparison": 100,
"conditionResult": DetectorPriorityLevel.OK,
"conditionGroupId": self.data_condition_group.id,
},
],
},
}
Expand Down
17 changes: 15 additions & 2 deletions tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ def setUp(self) -> None:
comparison=50,
condition_result=DetectorPriorityLevel.LOW,
)
self.resolve_condition = self.create_data_condition(
condition_group=self.data_condition_group,
type=Condition.GREATER_OR_EQUAL,
comparison=50,
condition_result=DetectorPriorityLevel.OK,
)
self.detector = self.create_detector(
project_id=self.project.id,
name="Test Detector",
Expand Down Expand Up @@ -180,6 +186,13 @@ def setUp(self) -> None:
"conditionResult": DetectorPriorityLevel.HIGH,
"conditionGroupId": self.condition.condition_group.id,
},
{
"id": self.resolve_condition.id,
"comparison": 100,
"type": Condition.LESS_OR_EQUAL,
"conditionResult": DetectorPriorityLevel.OK,
"conditionGroupId": self.condition.condition_group.id,
},
],
},
"config": self.detector.config,
Expand Down Expand Up @@ -223,7 +236,7 @@ def test_update(self, mock_schedule_update_project_config: mock.MagicMock) -> No
self.assert_condition_group_updated(condition_group)

conditions = list(DataCondition.objects.filter(condition_group=condition_group))
assert len(conditions) == 1
assert len(conditions) == 2
condition = conditions[0]
self.assert_data_condition_updated(condition)

Expand Down Expand Up @@ -276,7 +289,7 @@ def test_update_add_data_condition(self) -> None:
condition_group = detector.workflow_condition_group
assert condition_group
conditions = list(DataCondition.objects.filter(condition_group=condition_group))
assert len(conditions) == 2
assert len(conditions) == 3

def test_update_bad_schema(self) -> None:
"""
Expand Down
10 changes: 8 additions & 2 deletions tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,13 @@ def setUp(self) -> None:
"comparison": 100,
"conditionResult": DetectorPriorityLevel.HIGH,
"conditionGroupId": self.data_condition_group.id,
}
},
{
"type": Condition.LESS_OR_EQUAL,
"comparison": 100,
"conditionResult": DetectorPriorityLevel.OK,
"conditionGroupId": self.data_condition_group.id,
},
],
},
"config": {
Expand Down Expand Up @@ -868,7 +874,7 @@ def test_valid_creation(
assert condition_group.organization_id == self.organization.id

conditions = list(DataCondition.objects.filter(condition_group=condition_group))
assert len(conditions) == 1
assert len(conditions) == 2
condition = conditions[0]
assert condition.type == Condition.GREATER
assert condition.comparison == 100
Expand Down
Loading