feat(workflow_engine): Refactor the Data Condition Validator #88050

saponifi3d · 2025-03-26T23:25:14Z

Description

Separate the DataCondition and DataConditionGroup validators
Add tests to the BaseDataConditionGroupValidator
Add tests for BaseDataConditionValidator
Create AbstractDataConditionValidator to easily allow definitions, this meant we could completely remove the numeric validator as we can just use the abstract validator.

… conditions

…add support for comparison schema validation

saponifi3d · 2025-03-26T23:26:13Z

src/sentry/workflow_engine/endpoints/validators/base/data_condition.py

+
+    comparison = serializers.JSONField(required=True)
+    condition_result = serializers.JSONField(required=True)
+    # condition_group_id = serializers.IntegerField(required=True)


fyi, this is on purpose. need to think through the data condition group a little more in another PR then think about how this ties together. I think in general, it's correct that we would just want the ID at this point.

codecov · 2025-03-26T23:46:40Z

Codecov Report

Attention: Patch coverage is 92.12963% with 17 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...engine/endpoints/validators/base/data_condition.py	78.72%	10 Missing ⚠️
src/sentry/incidents/metric_alert_detector.py	85.71%	4 Missing ⚠️
...oints/validators/test_base_data_condition_group.py	85.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #88050       +/-   ##
===========================================
+ Coverage   51.71%   87.77%   +36.05%     
===========================================
  Files        9914    10016      +102     
  Lines      563395   568062     +4667     
  Branches    22208    22208               
===========================================
+ Hits       291358   498613   +207255     
+ Misses     271635    69047   -202588     
  Partials      402      402

…data condition validator. base condition validator now provides more

… SOLID design for metric alert comparison with new base

src/sentry/incidents/endpoints/validators/numeric_comparison_validator.py

src/sentry/incidents/metric_alert_detector.py

src/sentry/workflow_engine/endpoints/validators/base/data_condition_group.py

cathteng · 2025-03-27T16:50:29Z

src/sentry/workflow_engine/endpoints/validators/base/data_condition_group.py

+
+class BaseDataConditionGroupValidator(CamelSnakeSerializer):
+    logic_type = serializers.CharField(required=True)
+    organization_id = serializers.IntegerField(required=True)


do we need to validate that this is a real organization id? i think project/org is usually left out of the serializer because we validate the project/org based on the url path param, and we pass it as context instead

sentry/src/sentry/api/endpoints/project_rules.py

Lines 753 to 755 in 9090ac8

serializer = DrfRuleSerializer(

context={"project": project, "organization": project.organization}, data=request.data

)

👍 yeah, i saw some of those too. can update to follow that pattern. (there are so many patterns for these in our codebase it's hard to tell what is "right" 😅)

Yeah, that's a common pattern, although we could also build an OrganizationField or something if we need to validate in this class

i'll address in a separate PR for the Groups 👍 -- this is just moving the code out of the data_condition file and giving it a new home :)

src/sentry/incidents/endpoints/validators/numeric_comparison_validator.py

src/sentry/workflow_engine/endpoints/validators/base/data_condition.py

wedamija · 2025-03-28T19:10:06Z

src/sentry/incidents/metric_alert_detector.py

+    conditions = serializers.ListField(required=True)
+
+    def validate_conditions(self, value):
+        MetricAlertComparisonConditionValidator(data=value, many=True).is_valid(


Does it make sense do define this many=True behaviour on the base class, and define the validator as a property? Or do you think there will be more customizations to validate_conditions in the future that don't fit this model?

As an example of what I mean, in BaseDataConditionGroupValidator we could do

def validate_conditions(self, value) -> list: self.validator(data=value, many=True).is_valid(raise_exception=True)

Then we wouldn't need to define this at all on this subclass. Not sure if the validator needs to explicitly implement something to support many?

agree -- this was just to fix some mypy typing issues - hoping to address the DataConditionGroup stuff in a separate PR, but wanted to move it away from the DataCondition code and there was a little bit of extra fallout 😅

wedamija · 2025-03-28T19:12:49Z

src/sentry/workflow_engine/endpoints/validators/base/data_condition_group.py

+
+class BaseDataConditionGroupValidator(CamelSnakeSerializer):
+    logic_type = serializers.CharField(required=True)
+    organization_id = serializers.IntegerField(required=True)


Yeah, that's a common pattern, although we could also build an OrganizationField or something if we need to validate in this class

…ition and updated implementation of the detector condition. refocused pr to data condition and removed some things around data condition groups

src/sentry/workflow_engine/endpoints/validators/base/data_condition.py

…he validate condition results shared method

saponifi3d · 2025-04-02T20:46:15Z

src/sentry/workflow_engine/endpoints/validators/base/action.py

-
-    return value
-
-
 class BaseActionValidator(CamelSnakeModelSerializer[T], Generic[T]):


just fyi, my plan is to come back in another PR to restructure this + the data condition group stuff, once we hammer down a format in this PR.

src/sentry/workflow_engine/endpoints/validators/base/data_condition.py

cathteng · 2025-04-03T17:44:53Z

src/sentry/workflow_engine/endpoints/validators/base/data_condition.py

+class BaseDataConditionValidator(
+    AbstractDataConditionValidator[Any, Any],
+):


just to make sure i understand, this validator is for any condition that is not one of the CONDITION_OPS, conditions that use something in CONDITION_OPS will implement AbstractDataConditionValidator like MetricAlertComparisonConditionValidator?

💯 yeah - yuuup. this BaseDataConditionValidator will respond to any custom handler. if we don't have a custom handler and it's just a condition op, you only need to setup the validator in the API layer using this.

PS, i am planning to change the BaseDataConditionValidator name to DataConditionValidator in a future pr to help with any confusion around it. Base reads like it should be extended to me, and in this case, it's the generic implementation. i figured the diff was big enough on this PR already though.

cathteng · 2025-04-03T17:46:20Z

src/sentry/workflow_engine/endpoints/validators/base/data_condition.py

-    organization_id = serializers.IntegerField(required=True)
-    conditions = BaseDataConditionValidator(many=True)
+        try:
+            return validate_json_schema(value, handler.condition_result_schema)


need to make a task to add these for each handler, should they be enforced in the pre-save signal too? 😅

yeah - another area i'm trying to limit scope of the PR to. This was mostly to confirm it works.

…ition.py Co-authored-by: Cathy Teng <[email protected]>

## Description - Separate the DataCondition and DataConditionGroup validators - Add tests to the BaseDataConditionGroupValidator - Add tests for BaseDataConditionValidator - Create `AbstractDataConditionValidator` to easily allow definitions, this meant we could completely remove the numeric validator as we can just use the abstract validator. --------- Co-authored-by: Cathy Teng <[email protected]>

saponifi3d added 5 commits March 26, 2025 16:22

setup the scaffolding for the refactor

7ab3a71

start fleshing out data conditions and data condition groups

637c1b1

Add validate_condition_result and a basic validate_comparison method.

2ddc31c

Refactor the actions validator a little to share some stuff with data…

f603cd5

… conditions

Refactor to be a generic base, this will allow for easier extension. …

8358cea

…add support for comparison schema validation

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 26, 2025

saponifi3d commented Mar 26, 2025

View reviewed changes

vercel bot deployed to Preview March 26, 2025 23:27 View deployment

saponifi3d added 2 commits March 26, 2025 21:54

fix mypy errors in numeric comparison validator with updates to base …

a6986d0

…data condition validator. base condition validator now provides more

make numeric comparison code moar SOLID - Listkov Subs

e77fdb9

vercel bot deployed to Preview March 27, 2025 05:03 View deployment

Update some tests to match the new generic error messages. update for…

6cec08d

… SOLID design for metric alert comparison with new base

vercel bot deployed to Preview March 27, 2025 05:54 View deployment

fix mypy error in test_validators

4bc0d5b

vercel bot deployed to Preview March 27, 2025 05:59 View deployment

saponifi3d marked this pull request as ready for review March 27, 2025 06:44

saponifi3d requested review from a team as code owners March 27, 2025 06:44

cathteng reviewed Mar 27, 2025

View reviewed changes

src/sentry/incidents/endpoints/validators/numeric_comparison_validator.py Outdated Show resolved Hide resolved

src/sentry/incidents/endpoints/validators/numeric_comparison_validator.py Outdated Show resolved Hide resolved

src/sentry/incidents/metric_alert_detector.py Show resolved Hide resolved

cathteng requested a review from a team March 27, 2025 16:24

ceorourke reviewed Mar 27, 2025

View reviewed changes

src/sentry/workflow_engine/endpoints/validators/base/data_condition_group.py Outdated Show resolved Hide resolved

cathteng reviewed Mar 27, 2025

View reviewed changes

cathteng requested a review from a team March 27, 2025 16:50

Use a choice field to enforce something in the enum

cc6bbe6

vercel bot deployed to Preview March 27, 2025 23:37 View deployment

wedamija reviewed Mar 28, 2025

View reviewed changes

Refactor the base data condition to be a generic. remove numeric cond…

c4b1efe

…ition and updated implementation of the detector condition. refocused pr to data condition and removed some things around data condition groups

vercel bot deployed to Preview April 2, 2025 00:31 View deployment

github-advanced-security bot found potential problems Apr 2, 2025

View reviewed changes

src/sentry/workflow_engine/endpoints/validators/base/data_condition.py Fixed Show fixed Hide fixed

copilot autofix had good intent, but not the right fix

b3bb95d

vercel bot deployed to Preview April 2, 2025 01:11 View deployment

saponifi3d added 3 commits April 1, 2025 22:41

remove base properties

fa28436

add the id and require the condition group id, start thinking about t…

02b6fd9

…he validate condition results shared method

finish cleaning up the abstraction and fix some tests

956516f

vercel bot deployed to Preview April 2, 2025 18:16 View deployment

saponifi3d added 2 commits April 2, 2025 11:16

make a better error message

e966de8

yagni

89db6d5

saponifi3d requested review from wedamija, cathteng and ceorourke April 2, 2025 18:20

vercel bot deployed to Preview April 2, 2025 18:25 View deployment

saponifi3d commented Apr 2, 2025

View reviewed changes

cathteng approved these changes Apr 3, 2025

View reviewed changes

Update src/sentry/workflow_engine/endpoints/validators/base/data_cond…

01f0261

…ition.py Co-authored-by: Cathy Teng <[email protected]>

saponifi3d enabled auto-merge (squash) April 3, 2025 20:11

vercel bot deployed to Preview April 3, 2025 20:13 View deployment

saponifi3d merged commit f0e63ed into master Apr 3, 2025
48 checks passed

saponifi3d deleted the jcallender/aci/data-condition-validators branch April 3, 2025 20:39

github-actions bot locked and limited conversation to collaborators Apr 19, 2025

	serializer = DrfRuleSerializer(
	context={"project": project, "organization": project.organization}, data=request.data
	)


		return value


		class BaseActionValidator(CamelSnakeModelSerializer[T], Generic[T]):

Uh oh!

feat(workflow_engine): Refactor the Data Condition Validator #88050

feat(workflow_engine): Refactor the Data Condition Validator #88050

Uh oh!

Conversation

saponifi3d commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

saponifi3d commented Mar 26, 2025 •

edited

Loading

codecov bot commented Mar 26, 2025 •

edited

Loading