chore(ACI): Normalize sentry app data in Actions

[ceorourke]

Sentry app Action config data currently has 2 different types of data stored - sometimes we store the sentry app installation uuid, and sometimes we store the sentry app id. Going forward we are only creating actions with the sentry app id and need to migrate the data with the installation uuid to lookup the sentry app id and store that instead. Currently we handle both pieces of data, but once we run this migration we can go back and only support one.

Example of a config we do not want:

{
    "target_type":3,
    "target_display":null,
    "target_identifier":"abc123-def456",
    "sentry_app_identifier":"sentry_app_installation_uuid"
}

Example of a config we do want:

{
    "target_type":3,
    "target_display":"My WebHook",
    "target_identifier":"12345",
    "sentry_app_identifier":"sentry_app_id"
}

Fixes https://linear.app/getsentry/issue/ISWF-783/

[linear]

ISWF-783 Migrate sentry app data

[github-actions]

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

for 0106_migrate_actions_sentry_app_data in workflow_engine

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

[GabeVillalobos]

Is there something else we should do to the action if no valid sentry app installation is found? I remember Cathy doing some cleanup logic for enabling/disabling actions based on sentry app status.

[ceorourke]

Ooh I'll ask her tomorrow, I could see disabling being a reasonable outcome if we can't find it.

[cathteng]

There's logic today to disable actions when the corresponding sentry app or install is deleted, e.g. #101492 and #100813

It already handles both types of sentry app action configs, and I've done a backfill migration to disable too

[GabeVillalobos]

TIL these UUIDs are not unique 😱 Do we even have duplicates in our DB?

[ceorourke]

oh this was a "just in case" since I used get_many(), I don't actually expect it to happen.

[GabeVillalobos]

I'm shocked this works. I would have expected this to fail because we're not explicitly flushing these outboxes, so there must be some setup flushing these coincidentally?

[sentry]

Bug: In-place modification of the action.config JSONField may not be persisted to the database upon calling action.save() because Django does not detect the change.
_{Severity: MEDIUM}

Suggested Fix

To ensure the changes are saved, create a copy of the action.config dictionary, apply the modifications to the copy, and then reassign the entire modified dictionary back to action.config before calling action.save(). For example: new_config = action.config.copy(); new_config['key'] = 'value'; action.config = new_config; action.save().

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/receivers/outbox/region.py#L80-L82

Potential issue: The code in the outbox receiver at
`src/sentry/receivers/outbox/region.py` modifies the `action.config` dictionary
in-place. Specifically, it sets `target_identifier` and `sentry_app_identifier` on the
existing dictionary object. According to Django's ORM behavior, in-place mutations to a
`JSONField` are not automatically detected. As a result, the subsequent call to
`action.save()` may not persist these changes to the database, leaving the action's
configuration in an incorrect state after the migration logic runs.

[ceorourke]

Gah this is why I did the copy and update but now I am not sure which is best, going to check Django docs

[ceorourke]

I can't find anything directly on the Django docs but I checked a handful of pages and they all say the way I have it now is the way to do it.

[cursor]

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

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[cursor]

Redundant conditional checks on the same variable

Low Severity

The handler checks if not installs: (line 70) without returning, then immediately checks if installs: (line 79). These conditions are mutually exclusive, making the second check redundant. The if installs: block would be clearer as an else: clause, since it's logically the opposite of if not installs:.

 

[getsentry-bot]

PR reverted: 75500c1