fix(workflow-engine): Prevent ValueError for missing ActionTarget in serializer#119119
fix(workflow-engine): Prevent ValueError for missing ActionTarget in serializer#119119sentry[bot] wants to merge 3 commits into
Conversation
|
hmmmmmm i kinda want tests here........ @sentry can u write some regression tests 👀 |
| if target_type is not None | ||
| else None, | ||
| "targetIdentifier": get_identifier_from_action( | ||
| type_value, str(target_identifier), target_display | ||
| ), |
There was a problem hiding this comment.
Bug: If target_identifier is None in the config, casting it to an int will cause a TypeError or ValueError, crashing the serializer.
Severity: HIGH
Suggested Fix
Add a check to ensure target_identifier is not None before attempting to cast it to an integer. Provide a default value or handle the None case gracefully. For example, retrieve the value with target_identifier = obj.config.get("target_identifier") and then add a conditional check if target_identifier is not None: before calling int().
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/incidents/endpoints/serializers/workflow_engine_action.py#L79-L83
Potential issue: If an action's configuration in the database is missing the
`target_identifier` field, the serializer will fail. For actions of type `SENTRY_APP`,
an attempt to call `int(obj.config.get("target_identifier"))` will receive `None`,
raising a `TypeError`. For `PAGERDUTY` or `SENTRY_APP` actions, the code calls
`get_identifier_from_action` with `str(target_identifier)`. If `target_identifier` is
`None`, this becomes `int("None")`, which raises a `ValueError`. This scenario is
plausible in production, as evidenced by the similar bug for `target_type` that this
pull request aims to fix.
Did we get this right? 👍 / 👎 to inform future reviews.
| ), | ||
| "alertRuleTriggerId": str(alert_rule_trigger_id), | ||
| "type": obj.type, |
There was a problem hiding this comment.
Bug: The serializer can crash with a ValueError when a PagerDuty or Sentry App action is missing a target_identifier, as it attempts to run int("None").
Severity: HIGH
Suggested Fix
Before calling get_identifier_from_action, check if target_identifier is None. If it is, handle it gracefully, perhaps by returning None or a default value, to prevent the str() and subsequent int() conversion from crashing when the action type is PagerDuty or Sentry App.
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/incidents/endpoints/serializers/workflow_engine_action.py#L84-L86
Potential issue: When serializing a PagerDuty or Sentry App action that is missing a
`target_identifier` in its configuration, the value defaults to `None`. This `None`
value is then cast to the string "None" and passed to the `get_identifier_from_action`
function. For PagerDuty or Sentry App action types, this function attempts to cast the
string "None" to an integer, which raises a `ValueError`. This scenario is likely for
older actions created before the field was required, and the incomplete fix in the pull
request fails to guard against it.
Backend Test FailuresFailures on
|
This PR addresses issue SENTRY-5QVV, where the
WorkflowEngineActionSerializerwould raise aValueError: None is not a valid ActionTarget.Problem:
The
WorkflowEngineActionSerializerinsrc/sentry/incidents/endpoints/serializers/workflow_engine_action.pywas attempting to instantiate theActionTargetenum withNonewhen anActionobject'sconfigdictionary was missing thetarget_typekey. This scenario could occur for olderActionrecords or specific action types (e.g., Sentry App actions) wheretarget_typewas not present in the config, leading to aValueErrorduring serialization, particularly when accessing endpoints like/api/0/organizations/{organization_id_or_slug}/combined-rules/.Solution:
serializemethod inWorkflowEngineActionSerializerto explicitly check iftarget_typeisNonebefore attempting to convert it to anActionTargetenum member. Iftarget_typeisNone, the serializedtargetTypefield will now beNone.target_typefrominttoint | Noneto accurately reflect its potential nullability.logger.warningcall whentarget_typeisNoneto provide visibility into these cases in production logs without causing a serialization failure.test_missing_target_type_does_not_raisetotests/sentry/incidents/serializers/test_workflow_engine_action.pyto ensure thatActionobjects with a missingtarget_typein their config are serialized correctly without raising an error.Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
Fixes SENTRY-5QVV
Comment
@sentry <feedback>on this PR to have Autofix iterate on the changes.