Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
from drf_spectacular.utils import extend_schema
from rest_framework.response import Response

from sentry.api.api_owners import ApiOwner
from sentry.api.api_publish_status import ApiPublishStatus
from sentry.api.base import region_silo_endpoint
from sentry.api.bases import OrganizationEndpoint
from sentry.api.bases.organization import OrganizationDetectorPermission
from sentry.api.exceptions import ResourceDoesNotExist
from sentry.api.serializers import serialize
from sentry.apidocs.constants import (
RESPONSE_BAD_REQUEST,
RESPONSE_FORBIDDEN,
RESPONSE_NOT_FOUND,
RESPONSE_UNAUTHORIZED,
)
from sentry.apidocs.parameters import GlobalParams
from sentry.workflow_engine.endpoints.serializers.alertrule_workflow_serializer import (
AlertRuleWorkflowSerializer,
)
from sentry.workflow_engine.endpoints.validators.alertrule_workflow import (
AlertRuleWorkflowValidator,
)
from sentry.workflow_engine.models.alertrule_workflow import AlertRuleWorkflow


@region_silo_endpoint
class OrganizationAlertRuleWorkflowIndexEndpoint(OrganizationEndpoint):
publish_status = {
"GET": ApiPublishStatus.EXPERIMENTAL,
}
owner = ApiOwner.ISSUES
permission_classes = (OrganizationDetectorPermission,)

@extend_schema(
operation_id="Fetch Dual-Written Rule/Alert Rules and Workflows",
parameters=[
GlobalParams.ORG_ID_OR_SLUG,
],
responses={
200: AlertRuleWorkflowSerializer,
400: RESPONSE_BAD_REQUEST,
401: RESPONSE_UNAUTHORIZED,
403: RESPONSE_FORBIDDEN,
404: RESPONSE_NOT_FOUND,
},
)
def get(self, request, organization):
"""
Returns a dual-written rule/alert rule and its associated workflow.
"""
validator = AlertRuleWorkflowValidator(data=request.query_params)
validator.is_valid(raise_exception=True)
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)
Copy link
Member

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()

Copy link
Member

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

Copy link
Member Author

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


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)
Comment on lines +54 to +67
Copy link
Contributor

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 AlertRuleWorkflowValidator defines rule_id, alert_rule_id, and workflow_id as serializers.CharField. This allows non-numeric strings to pass validation. However, when these string values are used in the ORM filter, the underlying BoundedBigIntegerField model fields attempt to cast the string to an integer via int(value). If a non-numeric string like "abc" is provided in the query parameters, this cast raises a ValueError, leading to an unhandled exception and an HTTP 500 server error.
  • Suggested fix: Change the validator fields rule_id, alert_rule_id, and workflow_id in AlertRuleWorkflowValidator from serializers.CharField to serializers.IntegerField. This will ensure that only numeric values are accepted, providing proper validation upfront and preventing the ValueError in the ORM layer.
    severity: 0.7, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.


alert_rule_workflow = queryset.first()
if not alert_rule_workflow:
raise ResourceDoesNotExist

return Response(serialize(alert_rule_workflow, request.user))
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
from collections.abc import Mapping
from typing import Any, TypedDict

from sentry.api.serializers import Serializer, register
from sentry.workflow_engine.models import AlertRuleWorkflow


class ActionHandlerSerializerResponse(TypedDict):
ruleId: str | None
alertRuleId: str | None
workflowId: str


@register(AlertRuleWorkflow)
class AlertRuleWorkflowSerializer(Serializer):
def serialize(
self, obj: AlertRuleWorkflow, attrs: Mapping[str, Any], user, **kwargs
) -> ActionHandlerSerializerResponse:
return {
"ruleId": str(obj.rule_id) if obj.rule_id else None,
"alertRuleId": str(obj.alert_rule_id) if obj.alert_rule_id else None,
"workflowId": str(obj.workflow.id),
}
9 changes: 9 additions & 0 deletions src/sentry/workflow_engine/endpoints/urls.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
from django.urls import re_path

from sentry.workflow_engine.endpoints.organization_alertrule_workflow_index import (
OrganizationAlertRuleWorkflowIndexEndpoint,
)

from .organization_available_action_index import OrganizationAvailableActionIndexEndpoint
from .organization_data_condition_index import OrganizationDataConditionIndexEndpoint
from .organization_detector_count import OrganizationDetectorCountEndpoint
Expand Down Expand Up @@ -97,4 +101,9 @@
OrganizationOpenPeriodsEndpoint.as_view(),
name="sentry-api-0-organization-open-periods",
),
re_path(
r"^(?P<organization_id_or_slug>[^/]+)/alert-rule-workflow/$",
OrganizationAlertRuleWorkflowIndexEndpoint.as_view(),
name="sentry-api-0-organization-alert-rule-workflow-index",
),
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from rest_framework import serializers


class AlertRuleWorkflowValidator(serializers.Serializer):
rule_id = serializers.CharField(required=False)
alert_rule_id = serializers.CharField(required=False)
workflow_id = serializers.CharField(required=False)

Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Non-Numeric IDs Cause Queryset Filtering Errors

The validator accepts non-numeric strings for rule_id, alert_rule_id, and workflow_id. Since these fields map to underlying integer/foreign key columns, providing non-numeric input causes a ValueError during queryset filtering, resulting in a 500 error instead of a 4xx.

Fix in Cursor Fix in Web

def validate(self, attrs):
super().validate(attrs)
if (
not attrs.get("rule_id")
and not attrs.get("alert_rule_id")
and not attrs.get("workflow_id")
):
raise serializers.ValidationError(
"One of 'rule_id', 'alert_rule_id', or 'workflow_id' must be provided."
)
return attrs
Copy link
Contributor

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.

Fix in Cursor Fix in Web

3 changes: 1 addition & 2 deletions static/app/utils/api/knownSentryApiUrls.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,8 @@ export type KnownSentryApiUrls =
| '/internal/prevent/pr-review/configs/resolved/'
| '/internal/prevent/pr-review/github/sentry-org/'
| '/internal/project-config/'
| '/internal/queue/tasks/'
| '/internal/rpc/$serviceName/$methodName/'
| '/internal/seer-rpc/$methodName/'
| '/internal/stats/'
| '/internal/warnings/'
| '/issues/$issueId/'
| '/issues/$issueId/activities/'
Expand Down Expand Up @@ -192,6 +190,7 @@ export type KnownSentryApiUrls =
| '/organizations/$organizationIdOrSlug/access-requests/'
| '/organizations/$organizationIdOrSlug/access-requests/$requestId/'
| '/organizations/$organizationIdOrSlug/ai-conversations/'
| '/organizations/$organizationIdOrSlug/alert-rule-workflow/'
| '/organizations/$organizationIdOrSlug/alert-rules/'
| '/organizations/$organizationIdOrSlug/alert-rules/$alertRuleId/'
| '/organizations/$organizationIdOrSlug/alert-rules/available-actions/'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
from sentry.api.serializers import serialize
from sentry.testutils.cases import APITestCase
from sentry.testutils.silo import region_silo_test


class OrganizationAlertRuleWorkflowAPITestCase(APITestCase):
endpoint = "sentry-api-0-organization-alert-rule-workflow-index"

def setUp(self) -> None:
super().setUp()
self.login_as(user=self.user)

self.workflow_1 = self.create_workflow(organization=self.organization)
self.workflow_2 = self.create_workflow(organization=self.organization)
self.workflow_3 = self.create_workflow(organization=self.organization)

self.alert_rule_workflow_1 = self.create_alert_rule_workflow(
alert_rule_id=12345, workflow=self.workflow_1
)
self.alert_rule_workflow_2 = self.create_alert_rule_workflow(
rule_id=67890, workflow=self.workflow_2
)
self.alert_rule_workflow_3 = self.create_alert_rule_workflow(
alert_rule_id=11111, workflow=self.workflow_3
)

# Create workflow in different organization to test filtering
self.other_org = self.create_organization()
self.other_workflow = self.create_workflow(organization=self.other_org)
self.other_alert_rule_workflow = self.create_alert_rule_workflow(
alert_rule_id=99999, workflow=self.other_workflow
)


@region_silo_test
class OrganizationAlertRuleWorkflowIndexGetTest(OrganizationAlertRuleWorkflowAPITestCase):
def test_get_with_workflow_id_filter(self) -> None:
response = self.get_success_response(
self.organization.slug, workflow_id=str(self.workflow_1.id)
)
assert response.data == serialize(self.alert_rule_workflow_1, self.user)

def test_get_with_alert_rule_id_filter(self) -> None:
response = self.get_success_response(self.organization.slug, alert_rule_id="12345")

assert response.data["alertRuleId"] == "12345"
assert response.data["ruleId"] is None
assert response.data["workflowId"] == str(self.workflow_1.id)

def test_get_with_rule_id_filter(self) -> None:
response = self.get_success_response(self.organization.slug, rule_id="67890")

assert response.data["ruleId"] == "67890"
assert response.data["alertRuleId"] is None
assert response.data["workflowId"] == str(self.workflow_2.id)

def test_get_with_multiple_filters(self) -> None:
response = self.get_success_response(
self.organization.slug,
workflow_id=str(self.workflow_1.id),
alert_rule_id="12345",
)

assert response.data == serialize(self.alert_rule_workflow_1, self.user)

def test_get_with_multiple_filters_with_invalid_filter(self) -> None:
self.get_error_response(
self.organization.slug,
workflow_id=str(self.workflow_1.id),
alert_rule_id="this is not a valid ID",
)

def test_get_with_nonexistent_workflow_id(self) -> None:
self.get_error_response(self.organization.slug, workflow_id="99999", status_code=404)

def test_get_with_nonexistent_alert_rule_id(self) -> None:
self.get_error_response(self.organization.slug, alert_rule_id="99999", status_code=404)

def test_get_with_nonexistent_rule_id(self) -> None:
self.get_error_response(self.organization.slug, rule_id="99999", status_code=404)

def test_organization_isolation(self) -> None:
self.get_error_response(
self.organization.slug, workflow_id=str(self.other_workflow.id), status_code=404
)
Loading