Skip to content

fix(rules): Sanitize legacy Rule dynamic_form_fields choice labels#116081

Closed
priscilawebdev wants to merge 4 commits into
masterfrom
priscilawebdev/fix/rules-backfill-legacy-rule-choice-labels
Closed

fix(rules): Sanitize legacy Rule dynamic_form_fields choice labels#116081
priscilawebdev wants to merge 4 commits into
masterfrom
priscilawebdev/fix/rules-backfill-legacy-rule-choice-labels

Conversation

@priscilawebdev
Copy link
Copy Markdown
Member

@priscilawebdev priscilawebdev commented May 22, 2026

Companion to #115855, which backfilled workflow_engine_action.data.dynamic_form_fields. The same corruption (React element labels serialized into {key, ref, props} shells) exists on the legacy Rule model at Rule.data["actions"][*]["dynamic_form_fields"][*]["choices"], and the same TicketRuleModal crashes on it.

Post-deploy migration that walks Rule, restricts to known ticket-creation action ids, and applies the same _extract_choice_label strategy: pull readable text from props.children when present, fall back to the choice value otherwise.

Redash query: https://redash.getsentry.net/queries/11297/source

Refs JAVASCRIPT-3921

#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 <[email protected]>
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 22, 2026
@github-actions
Copy link
Copy Markdown
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/1101_sanitize_rule_dynamic_form_field_choices.py

for 1101_sanitize_rule_dynamic_form_field_choices in sentry

--
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL

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 <[email protected]>
@priscilawebdev priscilawebdev marked this pull request as ready for review May 22, 2026 09:48
@priscilawebdev priscilawebdev requested a review from a team as a code owner May 22, 2026 09:48
@priscilawebdev priscilawebdev requested review from a team, malwilley and markstory May 22, 2026 09:49
Comment thread src/sentry/migrations/1101_sanitize_rule_dynamic_form_field_choices.py Outdated
@malwilley
Copy link
Copy Markdown
Member

malwilley commented May 26, 2026

@priscilawebdev we are no longer using the old Rule models, all users are now on the new UI (aside from just a couple exceptions). The old models will be deleted soon, so I don't think this is necessary. The linked Sentry error shows an example that was fixed by #115855

@github-actions
Copy link
Copy Markdown
Contributor

Backend Test Failures

Failures on da270c7 in this run:

tests/symbolicator/test_minidump_full.py::SymbolicatorMinidumpIntegrationTest::test_full_minidumplog
[gw0] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/symbolicator/test_minidump_full.py:87: in test_full_minidump
    event = self.post_and_retrieve_minidump(
src/sentry/testutils/relay.py:127: in post_and_retrieve_minidump
    assert resp.ok
E   assert False
E    +  where False = <Response [503]>.ok

@priscilawebdev
Copy link
Copy Markdown
Member Author

@priscilawebdev we are no longer using the old Rule models, all users are now on the new UI (aside from just a couple exceptions). The old models will be deleted soon, so I don't think this is necessary. The linked Sentry error shows an example that was fixed by #115855

Oh nice! I was not aware of that. Will close this PR. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants