-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(aci): add alertrule/workflow GET endpoint #97351
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.
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #97351 +/- ##
=======================================
Coverage 80.66% 80.66%
=======================================
Files 9202 9205 +3
Lines 393026 393087 +61
Branches 25001 25001
=======================================
+ Hits 317022 317082 +60
- Misses 75579 75580 +1
Partials 425 425 |
| rule_id = validator.validated_data.get("rule_id") | ||
| alert_rule_id = validator.validated_data.get("alert_rule_id") | ||
| workflow_id = validator.validated_data.get("workflow_id") | ||
|
|
||
| queryset = AlertRuleWorkflow.objects.filter(workflow__organization=organization) | ||
|
|
||
| if workflow_id: | ||
| queryset = queryset.filter(workflow_id=workflow_id) | ||
|
|
||
| if alert_rule_id: | ||
| queryset = queryset.filter(alert_rule_id=alert_rule_id) | ||
|
|
||
| if rule_id: | ||
| queryset = queryset.filter(rule_id=rule_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: Using `CharField` for ID fields in `AlertRuleWorkflowValidator` allows non-numeric strings, causing a `ValueError` and server crash in the ORM filter.
- Description: The
AlertRuleWorkflowValidatordefinesrule_id,alert_rule_id, andworkflow_idasserializers.CharField. This allows non-numeric strings to pass validation. However, when these string values are used in the ORM filter, the underlyingBoundedBigIntegerFieldmodel fields attempt to cast the string to an integer viaint(value). If a non-numeric string like"abc"is provided in the query parameters, this cast raises aValueError, leading to an unhandled exception and an HTTP 500 server error. - Suggested fix: Change the validator fields
rule_id,alert_rule_id, andworkflow_idinAlertRuleWorkflowValidatorfromserializers.CharFieldtoserializers.IntegerField. This will ensure that only numeric values are accepted, providing proper validation upfront and preventing theValueErrorin the ORM layer.
severity: 0.7, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
src/sentry/workflow_engine/endpoints/organization_alertrule_workflow_index.py
Outdated
Show resolved
Hide resolved
| alert_rule_id = validator.validated_data.get("alert_rule_id") | ||
| workflow_id = validator.validated_data.get("workflow_id") | ||
|
|
||
| queryset = AlertRuleWorkflow.objects.filter(workflow__organization=organization) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternately I think you could do this in one filter like (this might not be totally correct):
from django.db.models import Q
alert_rule_workflow = AlertRuleWorkflow.objects.filter(
workflow__organization=organization,
Q(workflow_id=workflow_id) | Q(workflow_id=None),
Q(alert_rule_id=alert_rule_id) | Q(alert_rule_id=None),
Q(rule_id=rule_id) | Q(rule_id=None),
).first()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you could do some When stuff, tbh I haven't used this yet myself https://docs.djangoproject.com/en/5.2/ref/models/conditional-expressions/#when
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm I think your suggestion actually filters for when workflow_id=workflow_id OR workflow_id=None, but what I want to do here is only filter on workflow_id if a workflow_id is specified by the user
src/sentry/workflow_engine/endpoints/validators/alertrule_workflow.py
Outdated
Show resolved
Hide resolved
tests/sentry/workflow_engine/endpoints/test_organization_alertrule_workflow.py
Show resolved
Hide resolved
e30576f to
e5d511c
Compare
|
taking a step back, should we make this api? could we handle the redirect on the server side w/o making an API call? I think we'd just need to make sure to use a full page nav vs a react-router in page only... although I was expecting the UI to only support the new models (detector / workflows) to avoid a lot of this complexity. |
|
@saponifi3d Unfortunately due to Slack retention I can't link the conversation, but this is the solution was agreed on for redirects. We decided that we wouldn't add alert_id to the workflow engine endpoints and we would do this instead. |
e5d511c to
933da60
Compare
| raise serializers.ValidationError( | ||
| "One of 'rule_id', 'alert_rule_id', or 'workflow_id' must be provided." | ||
| ) | ||
| return attrs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Validation Fails, Returns Misleading Error.
The validator allows both rule_id and alert_rule_id to be provided simultaneously, but these fields are mutually exclusive according to the database constraint on AlertRuleWorkflow. When both are provided, the query will always return no results (404) instead of returning a proper validation error (400). The validator should reject requests where both rule_id and alert_rule_id are provided together.
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
used to get a rule/alert rule for a given workflow id or vice versa, for redirecting urls --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
used to get a rule/alert rule for a given workflow id or vice versa, for redirecting urls --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
same as #97351 need a lookup endpoint to redirect between the old and new alerts UI --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
used to get a rule/alert rule for a given workflow id or vice versa, for redirecting urls