Skip to content

chore(ACI): Remove temporary fallthrough type logic #103996

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 1, 2025
Merged

chore(ACI): Remove temporary fallthrough type logic #103996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f129e3b
chore(ACI): Remove temporary fallthrough type logic
ceorourke Nov 25, 2025
7c054a4
skip migrations test
ceorourke Nov 25, 2025
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
6 changes: 0 additions & 6 deletions src/sentry/notifications/notification_action/action_handler_registry/email_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,6 @@ class EmailActionHandler(ActionHandler):
"description": "The fallthrough type for issue owners email notifications",
"enum": [*FallthroughChoiceType],
},
# XXX(CEO): temporarily support this incorrect camel case
"fallthroughType": {
"type": "string",
"description": "The fallthrough type for issue owners email notifications",
"enum": [*FallthroughChoiceType],
},
},
"additionalProperties": False,
}
Expand Down
8 changes: 1 addition & 7 deletions ...ifications/notification_action/issue_alert_registry/handlers/email_issue_alert_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,7 @@ def get_additional_fields(cls, action: Action, mapping: ActionFieldMapping) -> d
}

if target_type == ActionTarget.ISSUE_OWNERS.value:
# XXX(CEO): temporarily handle both fallthroughType and fallthrough_type
action_data = action.data.copy()
if action.data.get("fallthroughType"):
del action_data["fallthroughType"]
action_data["fallthrough_type"] = action.data["fallthroughType"]

blob = EmailDataBlob(**action_data)
blob = EmailDataBlob(**action.data)
Copy link

@sentry sentry bot Nov 25, 2025

Choose a reason for hiding this comment

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

Bug: Removing fallthroughType backward compatibility before post-deployment migration causes TypeError.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The code removes backward compatibility logic for the fallthroughType field before its corresponding post-deployment migration has run. This causes a TypeError crash when EmailDataBlob(**action.data) is called with existing Action records that still contain fallthroughType (camelCase) in their action.data, as EmailDataBlob only accepts fallthrough_type (snake_case). This affects email notifications to issue owners immediately after deployment and before the migration 0104_action_data_fallthrough_type.py completes.

💡 Suggested Fix

Reinstate the backward compatibility logic to convert fallthroughType to fallthrough_type before passing action.data to EmailDataBlob, or ensure the migration runs before the code deployment.

🤖 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/notifications/notification_action/issue_alert_registry/handlers/email_issue_alert_handler.py#L54

Potential issue: The code removes backward compatibility logic for the `fallthroughType`
field before its corresponding post-deployment migration has run. This causes a
`TypeError` crash when `EmailDataBlob(**action.data)` is called with existing `Action`
records that still contain `fallthroughType` (camelCase) in their `action.data`, as
`EmailDataBlob` only accepts `fallthrough_type` (snake_case). This affects email
notifications to issue owners immediately after deployment and before the migration
`0104_action_data_fallthrough_type.py` completes.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3588791

final_blob[EmailFieldMappingKeys.FALLTHROUGH_TYPE_KEY.value] = blob.fallthrough_type

return final_blob
20 changes: 0 additions & 20 deletions tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,26 +606,6 @@ def test_build_rule_action_blob(self) -> None:
blob = self.handler.build_rule_action_blob(action, self.organization.id)
assert blob == healed

def test_build_rule_action_blob_fallthrough_type_camel_case(self) -> None:
"""
Test that while we are temporarily allowing both fallthroughType and fallthrough_type in Action.data
that both work when building the action data blob and result in the snake case version
"""
action = Action.objects.create(
type=Action.Type.EMAIL,
data={"fallthroughType": FallthroughChoiceType.ACTIVE_MEMBERS},
config={
"target_type": ActionTarget.ISSUE_OWNERS,
"target_identifier": None,
},
)
blob = self.handler.build_rule_action_blob(action, self.organization.id)
assert blob == {
"fallthrough_type": FallthroughChoiceType.ACTIVE_MEMBERS,
"id": "sentry.mail.actions.NotifyEmailAction",
"targetType": "IssueOwners",
}


class TestPluginIssueAlertHandler(BaseWorkflowTest):
def setUp(self) -> None:
Expand Down
3 changes: 3 additions & 0 deletions tests/sentry/workflow_engine/migrations/test_0104_action_data_fallthrough_type.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import pytest

from sentry.notifications.models.notificationaction import ActionTarget
from sentry.notifications.types import FallthroughChoiceType
from sentry.testutils.cases import TestMigrations
from sentry.workflow_engine.models import Action


@pytest.mark.skip
class TestActionDataFallthroughType(TestMigrations):
migrate_from = "0103_add_unique_constraint"
migrate_to = "0104_action_data_fallthrough_type"
Expand Down
Loading