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..ae076b36ad61 --- /dev/null +++ b/tests/sentry/migrations/test_1101_sanitize_rule_dynamic_form_field_choices.py @@ -0,0 +1,160 @@ +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): + 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()) + + # 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": JIRA_ACTION_ID, + "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"], + ], + } + ], + } + ], + }, + ) + + # 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": JIRA_ACTION_ID, + "dynamic_form_fields": [ + { + "name": "reporter", + "choices": [ + [ + "user-789", + {"key": None, "ref": None, "props": {"children": []}}, + ], + ], + } + ], + } + ], + }, + ) + + # Already-clean ticket action — must be untouched. + self.clean_rule = Rule.objects.create( + project_id=project.id, + label="clean-jira-rule", + data={ + "actions": [ + { + "id": JIRA_ACTION_ID, + "dynamic_form_fields": [ + { + "name": "assignee", + "choices": [["user-1", "Alice"], ["user-2", "Bob"]], + } + ], + } + ], + }, + ) + + # 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": EMAIL_ACTION_ID, + "dynamic_form_fields": [ + { + "name": "whatever", + "choices": [ + [ + "x", + { + "key": None, + "ref": None, + "props": {"children": ["leave me alone"]}, + }, + ], + ], + } + ], + } + ], + }, + ) + + # 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 _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"] + + def test_preserves_clean_tuple_within_corrupted_rule(self) -> None: + assert self._get_choices(self.corrupted_rule.id)[1] == ["user-456", "Already Clean"] + + def test_falls_back_to_value_when_unrecoverable(self) -> None: + assert self._get_choices(self.unrecoverable_rule.id)[0] == ["user-789", "user-789"] + + def test_does_not_touch_clean_rule(self) -> None: + assert self._get_choices(self.clean_rule.id) == [["user-1", "Alice"], ["user-2", "Bob"]] + + 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) + + def test_skips_rules_without_actions(self) -> None: + rule = Rule.objects.get(id=self.no_actions_rule.id) + assert "actions" not in rule.data