From f7b497f5c853ad11b41251cd858416c75735dde9 Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Fri, 22 May 2026 10:06:01 +0200 Subject: [PATCH 1/2] fix(rules): Sanitize corrupted dynamic_form_fields labels on legacy Rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #115855 backfilled `workflow_engine_action.data.dynamic_form_fields[*].choices` to recover or coerce labels left as JSON-stripped React elements (`{key, ref, props}`) by the old AbstractExternalIssueForm. The same corruption exists in `Rule.data["actions"][*]["dynamic_form_fields"][*]["choices"]` for legacy issue-alert rules — those use the same TicketRuleModal under the hood and crash the same way. Walk the Rule table, restrict to known ticket-creation action ids, and apply the same `_extract_choice_label` strategy: pull readable text from `props.children` when present, otherwise replace the broken label with the choice value so the row at least renders. Post-deploy migration; runs manually after the backend code deploys. Co-Authored-By: Claude Opus 4.7 --- migrations_lockfile.txt | 2 +- ...anitize_rule_dynamic_form_field_choices.py | 156 +++++++++++++++++ ...anitize_rule_dynamic_form_field_choices.py | 164 ++++++++++++++++++ 3 files changed, 321 insertions(+), 1 deletion(-) create mode 100644 src/sentry/migrations/1101_sanitize_rule_dynamic_form_field_choices.py create mode 100644 tests/sentry/migrations/test_1101_sanitize_rule_dynamic_form_field_choices.py diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index a440e18f04a4..11ae0a015adc 100644 --- a/migrations_lockfile.txt +++ b/migrations_lockfile.txt @@ -31,7 +31,7 @@ replays: 0007_organizationmember_replay_access seer: 0017_drop_old_fk_columns -sentry: 1100_add_relocation_file_bucket_path +sentry: 1101_sanitize_rule_dynamic_form_field_choices social_auth: 0003_social_auth_json_field diff --git a/src/sentry/migrations/1101_sanitize_rule_dynamic_form_field_choices.py b/src/sentry/migrations/1101_sanitize_rule_dynamic_form_field_choices.py new file mode 100644 index 000000000000..a19819f44591 --- /dev/null +++ b/src/sentry/migrations/1101_sanitize_rule_dynamic_form_field_choices.py @@ -0,0 +1,156 @@ +import logging + +from django.db import migrations +from django.db.backends.base.schema import BaseDatabaseSchemaEditor +from django.db.migrations.state import StateApps + +from sentry.new_migrations.migrations import CheckedMigration +from sentry.utils.query import RangeQuerySetWrapper + +logger = logging.getLogger(__name__) + +# The legacy issue-alert ticket-create actions whose `data.dynamic_form_fields` +# can hold corrupted choice labels left behind by the old AbstractExternalIssueForm. +# Mirrors the action types covered by workflow_engine migration 0114. +TICKET_ACTION_IDS = frozenset( + [ + "sentry.integrations.github.notify_action.GitHubCreateTicketAction", + "sentry.integrations.github_enterprise.notify_action.GitHubEnterpriseCreateTicketAction", + "sentry.integrations.jira.notify_action.JiraCreateTicketAction", + "sentry.integrations.jira_server.notify_action.JiraServerCreateTicketAction", + "sentry.integrations.vsts.notify_action.AzureDevopsCreateTicketAction", + ] +) + + +def _extract_choice_label(value: object, label: object) -> tuple[object, bool]: + """ + The legacy frontend persisted React elements as choice labels. JSON + serialization stripped $$typeof (Symbol) and type (function), leaving + plain {key, ref, props} shells which the new form components reject. + + Recover the readable text from props.children when possible; otherwise + fall back to the choice value so the row at least renders. + + All serialized React elements look like: + + ``` + { + "key": None, + "ref": None, + "props": { + "children": [ + {...}, + " ", + "This is the actual label text", + ] + } + } + ``` + """ + if isinstance(label, dict) and "props" in label: + props = label.get("props") + children = props.get("children") if isinstance(props, dict) else None + if isinstance(children, list): + text = "".join(c for c in children if isinstance(c, str)).strip() + if text: + return text, True + + return value, True + + return label, False + + +def sanitize_rule_dynamic_form_field_choices( + apps: StateApps, schema_editor: BaseDatabaseSchemaEditor +) -> None: + Rule = apps.get_model("sentry", "Rule") + + count = 0 + updated = 0 + + for rule in RangeQuerySetWrapper(Rule.objects.all()): + count += 1 + + actions = (rule.data or {}).get("actions") + if not isinstance(actions, list): + continue + + row_changed = False + + for action in actions: + if not isinstance(action, dict): + continue + if action.get("id") not in TICKET_ACTION_IDS: + continue + + dynamic_form_fields = action.get("dynamic_form_fields") + if not isinstance(dynamic_form_fields, list): + continue + + for field_def in dynamic_form_fields: + if not isinstance(field_def, dict): + continue + + choices = field_def.get("choices") + if not isinstance(choices, list): + continue + + for choice in choices: + if not isinstance(choice, list) or len(choice) < 2: + continue + + new_label, was_changed = _extract_choice_label(choice[0], choice[1]) + if was_changed: + choice[1] = new_label + row_changed = True + + if row_changed: + rule.save(update_fields=["data"]) + updated += 1 + + if count % 1000 == 0: + logger.info( + "sanitize_rule_dynamic_form_field_choices.progress", + extra={ + "count": count, + "updated": updated, + "current_rule_id": rule.id, + }, + ) + + logger.info( + "sanitize_rule_dynamic_form_field_choices.complete", + extra={ + "count": count, + "updated": updated, + }, + ) + + +class Migration(CheckedMigration): + # This flag is used to mark that a migration shouldn't be automatically run in production. + # This should only be used for operations where it's safe to run the migration after your + # code has deployed. So this should not be used for most operations that alter the schema + # of a table. + # Here are some things that make sense to mark as post deployment: + # - Large data migrations. Typically we want these to be run manually so that they can be + # monitored and not block the deploy for a long period of time while they run. + # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to + # run this outside deployments so that we don't block them. Note that while adding an index + # is a schema change, it's completely safe to run the operation after the code has deployed. + # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment + + is_post_deployment = True + + dependencies = [ + ("sentry", "1100_add_relocation_file_bucket_path"), + ] + + operations = [ + migrations.RunPython( + code=sanitize_rule_dynamic_form_field_choices, + reverse_code=migrations.RunPython.noop, + hints={"tables": ["sentry_rule"]}, + ), + ] diff --git a/tests/sentry/migrations/test_1101_sanitize_rule_dynamic_form_field_choices.py b/tests/sentry/migrations/test_1101_sanitize_rule_dynamic_form_field_choices.py new file mode 100644 index 000000000000..37073ef4cc17 --- /dev/null +++ b/tests/sentry/migrations/test_1101_sanitize_rule_dynamic_form_field_choices.py @@ -0,0 +1,164 @@ +import pytest + +from sentry.testutils.cases import TestMigrations + + +@pytest.mark.skip +class TestSanitizeRuleDynamicFormFieldChoices(TestMigrations): + migrate_from = "1100_add_relocation_file_bucket_path" + migrate_to = "1101_sanitize_rule_dynamic_form_field_choices" + app = "sentry" + + def setup_initial_state(self) -> None: + project = self.create_project(organization=self.create_organization()) + + Rule = self.apps_before.get_model("sentry", "Rule") + + # Corrupt Jira ticket action with a serialized React element label + # and a clean tuple alongside it. + self.corrupted_rule = Rule.objects.create( + project_id=project.id, + label="corrupted-jira-rule", + data={ + "actions": [ + { + "id": "sentry.integrations.jira.notify_action.JiraCreateTicketAction", + "dynamic_form_fields": [ + { + "name": "reporter", + "choices": [ + [ + "user-123", + { + "key": None, + "ref": None, + "props": { + "children": [ + { + "key": None, + "ref": None, + "props": { + "size": "xs", + "title": "tooltip", + }, + }, + " ", + "Jane Doe", + ], + }, + }, + ], + ["user-456", "Already Clean"], + ], + } + ], + } + ], + }, + ) + + # No-recovery shape: the carcass has no string children. We expect + # the helper to fall back to the value. + self.unrecoverable_rule = Rule.objects.create( + project_id=project.id, + label="unrecoverable-jira-rule", + data={ + "actions": [ + { + "id": "sentry.integrations.jira.notify_action.JiraCreateTicketAction", + "dynamic_form_fields": [ + { + "name": "reporter", + "choices": [ + [ + "user-789", + { + "key": None, + "ref": None, + "props": {"children": []}, + }, + ], + ], + } + ], + } + ], + }, + ) + + # Clean ticket action — should be untouched. + self.clean_rule = Rule.objects.create( + project_id=project.id, + label="clean-jira-rule", + data={ + "actions": [ + { + "id": "sentry.integrations.jira.notify_action.JiraCreateTicketAction", + "dynamic_form_fields": [ + { + "name": "assignee", + "choices": [["user-1", "Alice"], ["user-2", "Bob"]], + } + ], + } + ], + }, + ) + + # Non-ticket action with a coincidentally-shaped dict label. We + # only touch known ticket-action ids, so this must stay untouched. + self.non_ticket_rule = Rule.objects.create( + project_id=project.id, + label="email-rule-with-suspicious-shape", + data={ + "actions": [ + { + "id": "sentry.mail.actions.NotifyEmailAction", + "dynamic_form_fields": [ + { + "name": "whatever", + "choices": [ + [ + "x", + { + "key": None, + "ref": None, + "props": {"children": ["leave me alone"]}, + }, + ], + ], + } + ], + } + ], + }, + ) + + self.no_actions_rule = Rule.objects.create( + project_id=project.id, + label="rule-with-no-actions", + data={"conditions": []}, + ) + + def test_migration(self) -> None: + from sentry.models.rule import Rule + + corrupted = Rule.objects.get(id=self.corrupted_rule.id) + choices = corrupted.data["actions"][0]["dynamic_form_fields"][0]["choices"] + assert choices[0] == ["user-123", "Jane Doe"] + assert choices[1] == ["user-456", "Already Clean"] + + unrecoverable = Rule.objects.get(id=self.unrecoverable_rule.id) + choices = unrecoverable.data["actions"][0]["dynamic_form_fields"][0]["choices"] + assert choices[0] == ["user-789", "user-789"] + + clean = Rule.objects.get(id=self.clean_rule.id) + choices = clean.data["actions"][0]["dynamic_form_fields"][0]["choices"] + assert choices == [["user-1", "Alice"], ["user-2", "Bob"]] + + non_ticket = Rule.objects.get(id=self.non_ticket_rule.id) + choices = non_ticket.data["actions"][0]["dynamic_form_fields"][0]["choices"] + assert isinstance(choices[0][1], dict) + + no_actions = Rule.objects.get(id=self.no_actions_rule.id) + assert "actions" not in no_actions.data From 708ee5753ac93392fbb1187110464ea583da8ab3 Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Fri, 22 May 2026 11:38:52 +0200 Subject: [PATCH 2/2] test(rules): Split migration test into focused per-scenario methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One assert per test gives clearer failure output than the single multi-block test_migration. Also drops a stale `self.apps_before` reference that would have AttributeError'd if the class were ever unskipped — use the live Rule model in setup, matching #115855's style. Co-Authored-By: Claude Opus 4.7 --- ...anitize_rule_dynamic_form_field_choices.py | 74 +++++++++---------- 1 file changed, 35 insertions(+), 39 deletions(-) diff --git a/tests/sentry/migrations/test_1101_sanitize_rule_dynamic_form_field_choices.py b/tests/sentry/migrations/test_1101_sanitize_rule_dynamic_form_field_choices.py index 37073ef4cc17..ae076b36ad61 100644 --- a/tests/sentry/migrations/test_1101_sanitize_rule_dynamic_form_field_choices.py +++ b/tests/sentry/migrations/test_1101_sanitize_rule_dynamic_form_field_choices.py @@ -1,7 +1,11 @@ import pytest +from sentry.models.rule import Rule from sentry.testutils.cases import TestMigrations +JIRA_ACTION_ID = "sentry.integrations.jira.notify_action.JiraCreateTicketAction" +EMAIL_ACTION_ID = "sentry.mail.actions.NotifyEmailAction" + @pytest.mark.skip class TestSanitizeRuleDynamicFormFieldChoices(TestMigrations): @@ -12,17 +16,15 @@ class TestSanitizeRuleDynamicFormFieldChoices(TestMigrations): def setup_initial_state(self) -> None: project = self.create_project(organization=self.create_organization()) - Rule = self.apps_before.get_model("sentry", "Rule") - - # Corrupt Jira ticket action with a serialized React element label - # and a clean tuple alongside it. + # Recoverable: the carcass has string children we can extract ("Jane Doe"). + # The second choice is a clean tuple — proves we don't touch healthy rows. self.corrupted_rule = Rule.objects.create( project_id=project.id, label="corrupted-jira-rule", data={ "actions": [ { - "id": "sentry.integrations.jira.notify_action.JiraCreateTicketAction", + "id": JIRA_ACTION_ID, "dynamic_form_fields": [ { "name": "reporter", @@ -37,10 +39,7 @@ def setup_initial_state(self) -> None: { "key": None, "ref": None, - "props": { - "size": "xs", - "title": "tooltip", - }, + "props": {"size": "xs", "title": "tooltip"}, }, " ", "Jane Doe", @@ -57,26 +56,21 @@ def setup_initial_state(self) -> None: }, ) - # No-recovery shape: the carcass has no string children. We expect - # the helper to fall back to the value. + # Carcass with no string children — recovery fails, helper falls back to value. self.unrecoverable_rule = Rule.objects.create( project_id=project.id, label="unrecoverable-jira-rule", data={ "actions": [ { - "id": "sentry.integrations.jira.notify_action.JiraCreateTicketAction", + "id": JIRA_ACTION_ID, "dynamic_form_fields": [ { "name": "reporter", "choices": [ [ "user-789", - { - "key": None, - "ref": None, - "props": {"children": []}, - }, + {"key": None, "ref": None, "props": {"children": []}}, ], ], } @@ -86,14 +80,14 @@ def setup_initial_state(self) -> None: }, ) - # Clean ticket action — should be untouched. + # Already-clean ticket action — must be untouched. self.clean_rule = Rule.objects.create( project_id=project.id, label="clean-jira-rule", data={ "actions": [ { - "id": "sentry.integrations.jira.notify_action.JiraCreateTicketAction", + "id": JIRA_ACTION_ID, "dynamic_form_fields": [ { "name": "assignee", @@ -105,15 +99,15 @@ def setup_initial_state(self) -> None: }, ) - # Non-ticket action with a coincidentally-shaped dict label. We - # only touch known ticket-action ids, so this must stay untouched. + # Non-ticket action whose label coincidentally looks like a carcass. + # Proves the action-id scoping prevents collateral damage. self.non_ticket_rule = Rule.objects.create( project_id=project.id, label="email-rule-with-suspicious-shape", data={ "actions": [ { - "id": "sentry.mail.actions.NotifyEmailAction", + "id": EMAIL_ACTION_ID, "dynamic_form_fields": [ { "name": "whatever", @@ -134,31 +128,33 @@ def setup_initial_state(self) -> None: }, ) + # Rule whose data has no actions key at all — migration must not crash. self.no_actions_rule = Rule.objects.create( project_id=project.id, label="rule-with-no-actions", data={"conditions": []}, ) - def test_migration(self) -> None: - from sentry.models.rule import Rule + def _get_choices(self, rule_id: int) -> list: + rule = Rule.objects.get(id=rule_id) + return rule.data["actions"][0]["dynamic_form_fields"][0]["choices"] + + def test_recovers_text_from_react_element_carcass(self) -> None: + assert self._get_choices(self.corrupted_rule.id)[0] == ["user-123", "Jane Doe"] - corrupted = Rule.objects.get(id=self.corrupted_rule.id) - choices = corrupted.data["actions"][0]["dynamic_form_fields"][0]["choices"] - assert choices[0] == ["user-123", "Jane Doe"] - assert choices[1] == ["user-456", "Already Clean"] + def test_preserves_clean_tuple_within_corrupted_rule(self) -> None: + assert self._get_choices(self.corrupted_rule.id)[1] == ["user-456", "Already Clean"] - unrecoverable = Rule.objects.get(id=self.unrecoverable_rule.id) - choices = unrecoverable.data["actions"][0]["dynamic_form_fields"][0]["choices"] - assert choices[0] == ["user-789", "user-789"] + def test_falls_back_to_value_when_unrecoverable(self) -> None: + assert self._get_choices(self.unrecoverable_rule.id)[0] == ["user-789", "user-789"] - clean = Rule.objects.get(id=self.clean_rule.id) - choices = clean.data["actions"][0]["dynamic_form_fields"][0]["choices"] - assert choices == [["user-1", "Alice"], ["user-2", "Bob"]] + def test_does_not_touch_clean_rule(self) -> None: + assert self._get_choices(self.clean_rule.id) == [["user-1", "Alice"], ["user-2", "Bob"]] - non_ticket = Rule.objects.get(id=self.non_ticket_rule.id) - choices = non_ticket.data["actions"][0]["dynamic_form_fields"][0]["choices"] - assert isinstance(choices[0][1], dict) + def test_does_not_touch_non_ticket_actions(self) -> None: + # The label is still the original dict — action-id filter held. + assert isinstance(self._get_choices(self.non_ticket_rule.id)[0][1], dict) - no_actions = Rule.objects.get(id=self.no_actions_rule.id) - assert "actions" not in no_actions.data + def test_skips_rules_without_actions(self) -> None: + rule = Rule.objects.get(id=self.no_actions_rule.id) + assert "actions" not in rule.data