fix(aci): Add support for an empty conditionGroup in the API for the Error detector#112641
fix(aci): Add support for an empty conditionGroup in the API for the Error detector#112641saponifi3d wants to merge 3 commits intomasterfrom
Conversation
| if value is not None: | ||
| # We want to allow an empty DataConditionGroup, but not create anything. | ||
| return None |
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
There was a problem hiding this comment.
the top level condition also allows for it to be null, which is what None would represent here.
There was a problem hiding this comment.
Fix attempt detected (commit a6bf103)
The fix changed the validation logic to accept empty conditionGroup and returns None, but the base class update() method will still fail with AttributeError when trying to call .get() on None, exactly as the reported issue describes; the suggested fix to return {} instead of None was not implemented.
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
src/sentry/workflow_engine/endpoints/validators/error_detector.py
Outdated
Show resolved
Hide resolved
malwilley
left a comment
There was a problem hiding this comment.
I like this approach, looks like there is a type error and potentially an issue with returning None?
| "Condition group is not supported for error detectors" | ||
| ) | ||
|
|
||
| if value is not None: |
There was a problem hiding this comment.
Seems like you can just do return None without the if statement right? (Or return {} if the bot comments are correct)
There was a problem hiding this comment.
yeah, 💯 true -- i'll have this return None. (the bot is not right about {} because that'd try to then make a data condition group and error because it doesn't have a logic type)
…and use the data conditions to store the grouping rules.
| condition_group = serializers.DictField( | ||
| required=False, | ||
| allow_null=True, | ||
| ) # type: ignore[assignment] # Intentionally overrides nested serializer to accept empty dicts |
There was a problem hiding this comment.
just fyi - the TODO is the point in which we could clean this up. the problem is mostly related to the error detector not storing the grouping rules as data conditions / this model.
| def validate_condition_group(self, value: Any) -> Any: | ||
| if value is not None: | ||
| if value: | ||
| raise serializers.ValidationError( | ||
| "Condition group is not supported for error detectors" | ||
| ) | ||
| return value | ||
|
|
||
| return None |
There was a problem hiding this comment.
Bug: Returning None from validate_condition_group for an empty dictionary causes an AttributeError during the update operation because the update method doesn't expect a None value.
Severity: CRITICAL
Suggested Fix
Either modify validate_condition_group() to return an empty dictionary instead of None, or add a check in the update method to handle cases where condition_group is None before attempting to access its attributes.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/workflow_engine/endpoints/validators/error_detector.py#L40-L46
Potential issue: The `validate_condition_group` method was changed to return `None` when
an empty dictionary is provided. While this allows the validation to pass, the
serializer's `update` method does not handle this `None` value. When updating an Error
Detector with an empty `condition_group`, the code later attempts to call the `.get()`
method on this `None` value, causing an `AttributeError: 'NoneType' object has no
attribute 'get'` and crashing the request. This occurs because there is no null check
before the attribute access in the `update` logic.
There was a problem hiding this comment.
changed the .update logic to also ensure that it's not none for safety.
Description
Fixes: https://linear.app/getsentry/issue/ISWF-2357/saving-error-monitor-after-creating-alert-fails
The issue seems like that the validator for ErrorDetectors was throwing because it has an empty data condition group; this should be okay and makes sense in our UI to provide as a default.
Rather than updating the UI to not send the field, this PR will allow the
{}for the condition group.