Skip to content

fix(workflow_engine): Sanitize corrupted dynamic_form_fields choice labels#115855

Merged
malwilley merged 5 commits into
masterfrom
malwilley/fix-dynamic-form-labels
May 21, 2026
Merged

fix(workflow_engine): Sanitize corrupted dynamic_form_fields choice labels#115855
malwilley merged 5 commits into
masterfrom
malwilley/fix-dynamic-form-labels

Conversation

@malwilley
Copy link
Copy Markdown
Member

Fixes https://linear.app/getsentry/issue/ISWF-2670/alerts-form-crashes-when-opening-ticket-action-details-modal-in-some

A legacy code path (in the old AbstractExternalIssueForm) could persist React element labels (like <QuestionTooltip />) into choice tuples stored in Action.data.dynamic_form_fields. After JSON round-tripping, these become plain {key, ref, props} objects which, now with the new form components, crash on render. We never should have been saving these react components to the database, so this PR aims to fix that bad data.

This is a post-deploy migration which goes over all ticketing actions and fixes any which have objects saved as the label.

See here for a full list of bad actions in the DB: https://redash.getsentry.net/queries/11274/source

The serialized react elements all look like this, so we can easily extract the actual label:

{
        "key": None,
        "ref": None,
        "props": {
            "children": [
                {...},
                " ",
                "This is the actual label text",
            ]
        }
    }

@malwilley malwilley requested review from a team as code owners May 19, 2026 22:45
@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 19, 2026

ISWF-2670

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 19, 2026
…abels

A legacy code path (ensureCurrentOption in AbstractExternalIssueForm) could
persist React element labels into choice tuples stored in Action.data. After
JSON round-tripping these become {key, ref, props} objects that crash on
render. Add a post-deployment data migration that iterates ticket-type actions,
extracts visible text from the serialized React elements, and replaces the
corrupted labels with plain strings.
@malwilley malwilley force-pushed the malwilley/fix-dynamic-form-labels branch from fab5775 to ad837f8 Compare May 19, 2026 22:51
Comment thread src/sentry/workflow_engine/migrations/0114_sanitize_dynamic_form_field_choices.py Outdated
@github-actions
Copy link
Copy Markdown
Contributor

This PR has a migration; here is the generated SQL for src/sentry/workflow_engine/migrations/0114_sanitize_dynamic_form_field_choices.py

for 0114_sanitize_dynamic_form_field_choices in workflow_engine

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

row_changed = True

if row_changed:
action.save(update_fields=["data"])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need to modify the json schema for the data field?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The schema only defines dynamic_form_fields as an array of objects:

"dynamic_form_fields": {
"type": "array",
"description": "Dynamic form fields from customer configuration",
"items": {"type": "object"},
"default": [],
},

Comment on lines +103 to +108
"dynamic_form_fields": [
{
"name": "assignee",
"choices": [["user-1", "Alice"], ["user-2", "Bob"]],
}
],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this data replicated on each action? 🤔 i wonder if it'd be better to have it normalized and shared. probably too big of a lift for now, but food for thought.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah this is probably something we could do? I'm not too well versed on dynamic_form_fields

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sg - can think of it as a follow-up / cleanup thing if we can get to it.

Comment on lines +59 to +60
for action in RangeQuerySetWrapper(Action.objects.filter(type__in=TICKET_ACTION_TYPES)):
count += 1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
for action in RangeQuerySetWrapper(Action.objects.filter(type__in=TICKET_ACTION_TYPES)):
count += 1
for count, action in enumerate(RangeQuerySetWrapper(Action.objects.filter(type__in=TICKET_ACTION_TYPES))):

i'm not sure if enumerate does something to RangeQuerySetWrapper or not, but you can use enumerate like this to also get the count, so you don't have to track it.

Comment thread src/sentry/workflow_engine/migrations/0114_sanitize_dynamic_form_field_choices.py Outdated
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2192d98. Configure here.

Comment thread src/sentry/workflow_engine/migrations/0114_sanitize_dynamic_form_field_choices.py Outdated
Copy link
Copy Markdown
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

lgtm

@malwilley malwilley enabled auto-merge (squash) May 21, 2026 16:20
@malwilley malwilley merged commit f5026e9 into master May 21, 2026
83 of 84 checks passed
@malwilley malwilley deleted the malwilley/fix-dynamic-form-labels branch May 21, 2026 16:34
JonasBa pushed a commit that referenced this pull request May 21, 2026
…abels (#115855)

A legacy code path (in the old `AbstractExternalIssueForm`) could
persist React element labels (like `<QuestionTooltip />`) into choice
tuples stored in `Action.data.dynamic_form_fields`. After JSON
round-tripping, these become plain `{key, ref, props}` objects which,
now with the new form components, crash on render. We never should have
been saving these react components to the database, so this PR aims to
fix that bad data.

This is a post-deploy migration which goes over all ticketing actions
and fixes any which have objects saved as the label.

See here for a full list of bad actions in the DB:
https://redash.getsentry.net/queries/11274/source

The serialized react elements all look like this, so we can easily
extract the actual label:

```
{
        "key": None,
        "ref": None,
        "props": {
            "children": [
                {...},
                " ",
                "This is the actual label text",
            ]
        }
    }
```
natemoo-re pushed a commit that referenced this pull request May 21, 2026
…abels (#115855)

A legacy code path (in the old `AbstractExternalIssueForm`) could
persist React element labels (like `<QuestionTooltip />`) into choice
tuples stored in `Action.data.dynamic_form_fields`. After JSON
round-tripping, these become plain `{key, ref, props}` objects which,
now with the new form components, crash on render. We never should have
been saving these react components to the database, so this PR aims to
fix that bad data.

This is a post-deploy migration which goes over all ticketing actions
and fixes any which have objects saved as the label.

See here for a full list of bad actions in the DB:
https://redash.getsentry.net/queries/11274/source

The serialized react elements all look like this, so we can easily
extract the actual label:

```
{
        "key": None,
        "ref": None,
        "props": {
            "children": [
                {...},
                " ",
                "This is the actual label text",
            ]
        }
    }
```
priscilawebdev added a commit that referenced this pull request May 22, 2026
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 <noreply@anthropic.com>
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.

2 participants