Skip to content

fix(spans-migration): add validation to not update alerts to transactions or generic_metrics#107626

Merged
nikkikapadia merged 1 commit intomasterfrom
nikki/fix/user-updated-migrated-alerts
Feb 5, 2026
Merged

fix(spans-migration): add validation to not update alerts to transactions or generic_metrics#107626
nikkikapadia merged 1 commit intomasterfrom
nikki/fix/user-updated-migrated-alerts

Conversation

@nikkikapadia
Copy link
Member

@nikkikapadia nikkikapadia commented Feb 4, 2026

had a few cases where orgs were making updates to their migrated transaction alerts via some automated update call which ended up migrating them back to the transactions dataset. We're making sure that updates to alerts like this raise an error.

Cursor Bugbot found 2 potential issues for commit 20bb86a

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 4, 2026
@nikkikapadia nikkikapadia marked this pull request as ready for review February 4, 2026 21:59
@nikkikapadia nikkikapadia requested a review from a team as a code owner February 4, 2026 21:59
Copy link
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 2 potential issues.

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

or data_source.get("event_types", snuba_query.event_types) != snuba_query.event_types
organization = self.context.get("organization")
current_dataset = Dataset(snuba_query.dataset)
new_dataset = data_source.get("dataset", current_dataset)
Copy link
Contributor

Choose a reason for hiding this comment

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

Stricter validation blocks updates without explicit dataset change

Medium Severity

The refactored is_editing_transaction_dataset uses data_source.get("dataset", current_dataset) which defaults to the current dataset if not specified. The old code used data_source.get("dataset") without a default, returning None if unspecified. This behavioral change means updates to alerts currently on transactions or generic_metrics datasets will now be blocked even when the update doesn't explicitly change the dataset (e.g., only modifying the query parameter). The added tests only cover cases where dataset is explicitly specified in the update request, leaving this edge case untested.

Fix in Cursor Fix in Web

Copy link
Member Author

Choose a reason for hiding this comment

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

but when dataset is passed as None it will just use the current dataset 🤩

new_dataset = data_source.get("dataset", current_dataset)

if (
features.has("organizations:discover-saved-queries-deprecation", organization)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing organization check allows silent validation bypass on updates

Medium Severity

The is_editing_transaction_dataset method retrieves organization from context but doesn't validate it before use. If organization is None, features.has(..., None) will likely return False, causing the validation to silently pass and allowing the update to proceed. In contrast, the parallel create-time validation in _validate_transaction_dataset_deprecation (lines 226-228) explicitly raises a ValidationError when organization is missing. This inconsistency means updates could bypass the deprecation check if organization context is somehow absent, while creates would properly fail.

Fix in Cursor Fix in Web

@nikkikapadia nikkikapadia merged commit e3a6a4c into master Feb 5, 2026
75 checks passed
@nikkikapadia nikkikapadia deleted the nikki/fix/user-updated-migrated-alerts branch February 5, 2026 15:19
jaydgoss pushed a commit that referenced this pull request Feb 12, 2026
…ions or generic_metrics (#107626)

had a few cases where orgs were making updates to their migrated
transaction alerts via some automated update call which ended up
migrating them back to the transactions dataset. We're making sure that
updates to alerts like this raise an error.

<!-- BUGBOT_STATUS --><sup><a
href="https://cursor.com/dashboard?tab=bugbot">Cursor Bugbot</a> found 2
potential issues for commit <u>20bb86a</u></sup><!-- /BUGBOT_STATUS -->
dcramer pushed a commit that referenced this pull request Feb 17, 2026
…ions or generic_metrics (#107626)

had a few cases where orgs were making updates to their migrated
transaction alerts via some automated update call which ended up
migrating them back to the transactions dataset. We're making sure that
updates to alerts like this raise an error.

<!-- BUGBOT_STATUS --><sup><a
href="https://cursor.com/dashboard?tab=bugbot">Cursor Bugbot</a> found 2
potential issues for commit <u>20bb86a</u></sup><!-- /BUGBOT_STATUS -->
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