chore(spans-migration): add post and put validation for EAP extrapolation mode #102970
Conversation
github-actions
bot
added
the
Scope: Backend
| if ( | ||
| new_extrapolation_mode == ExtrapolationMode.SERVER_WEIGHTED.name.lower() | ||
| and old_extrapolation_mode != ExtrapolationMode.SERVER_WEIGHTED.name.lower() | ||
| ): | ||
| return True | ||
| if ( | ||
| new_extrapolation_mode == ExtrapolationMode.NONE.name.lower() | ||
| and old_extrapolation_mode != ExtrapolationMode.NONE.name.lower() | ||
| ): | ||
| return True | ||
| if ( | ||
| new_extrapolation_mode == ExtrapolationMode.CLIENT_AND_SERVER_WEIGHTED.name.lower() | ||
| or new_extrapolation_mode == ExtrapolationMode.UNKNOWN.name.lower() | ||
| ): | ||
| return False | ||
| return False |
There was a problem hiding this comment.
I think we don't need to be super restrictive here. We should only guard against people trying to set serve weighted so we don't have any un-intended consequences.
| if ( | |
| new_extrapolation_mode == ExtrapolationMode.SERVER_WEIGHTED.name.lower() | |
| and old_extrapolation_mode != ExtrapolationMode.SERVER_WEIGHTED.name.lower() | |
| ): | |
| return True | |
| if ( | |
| new_extrapolation_mode == ExtrapolationMode.NONE.name.lower() | |
| and old_extrapolation_mode != ExtrapolationMode.NONE.name.lower() | |
| ): | |
| return True | |
| if ( | |
| new_extrapolation_mode == ExtrapolationMode.CLIENT_AND_SERVER_WEIGHTED.name.lower() | |
| or new_extrapolation_mode == ExtrapolationMode.UNKNOWN.name.lower() | |
| ): | |
| return False | |
| return False | |
| if ( | |
| new_extrapolation_mode == ExtrapolationMode.SERVER_WEIGHTED.name.lower() | |
| and old_extrapolation_mode != ExtrapolationMode.SERVER_WEIGHTED.name.lower() | |
| ): | |
| return True | |
| return False |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| if self.is_editing_transaction_dataset(snuba_query, data_source): | ||
| raise serializers.ValidationError( | ||
| "Updates to transaction-based alerts is disabled, as we migrate to the span dataset. Create span-based alerts (dataset: events_analytics_platform) with the is_transaction:true filter instead." |
There was a problem hiding this comment.
Bug: Type mismatch in extrapolation_mode validation allows bypass when integer values are sent, preventing server_weighted mode restriction.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
A type mismatch occurs in the validation logic for extrapolation_mode in src/sentry/incidents/endpoints/organization_alert_rule_details.py (lines 135-144) and src/sentry/incidents/endpoints/organization_alert_rule_index.py (line 104). When extrapolation_mode is sent as an integer (e.g., 3) in the request data, it is compared against a string representation (e.g., "server_weighted"). This comparison always evaluates to false, silently bypassing the intended validation that prevents users from setting the server_weighted mode.
💡 Suggested Fix
Ensure new_extrapolation_mode is consistently converted to a string or integer before comparison, matching the type of ExtrapolationMode.SERVER_WEIGHTED.name.lower() or .value.
🤖 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/incidents/metric_issue_detector.py#L224
Potential issue: A type mismatch occurs in the validation logic for `extrapolation_mode`
in `src/sentry/incidents/endpoints/organization_alert_rule_details.py` (lines 135-144)
and `src/sentry/incidents/endpoints/organization_alert_rule_index.py` (line 104). When
`extrapolation_mode` is sent as an integer (e.g., `3`) in the request data, it is
compared against a string representation (e.g., `"server_weighted"`). This comparison
always evaluates to false, silently bypassing the intended validation that prevents
users from setting the `server_weighted` mode.
Did we get this right? 👍 / 👎 to inform future reviews.
| return Response(serialized_rule) | ||
|
|
||
|
|
||
| def _is_invalid_extrapolation_mode(old_extrapolation_mode, new_extrapolation_mode) -> bool: |
There was a problem hiding this comment.
Seems like you could combine this and the one in metric_issue_detector.py since they're doing the same thing
There was a problem hiding this comment.
ooop yes good point
| ) | ||
|
|
||
| if self.is_invalid_extrapolation_mode(snuba_query, data_source): | ||
| raise serializers.ValidationError("Invalid extrapolation mode for this alert type.") |
There was a problem hiding this comment.
| raise serializers.ValidationError("Invalid extrapolation mode for this alert type.") | |
| raise serializers.ValidationError("Invalid extrapolation mode for this detector type.") |
| @@ -617,3 +618,74 @@ def test_transaction_dataset_deprecation_generic_metrics_update(self) -> None: | |||
| with_feature("organizations:discover-saved-queries-deprecation"), | |||
| ): | |||
| update_validator.save() | |||
|
|
|||
| def test_invalid_extrapolation_mode_create(self) -> None: | |||
There was a problem hiding this comment.
Should a valid extrapolation mode be added to any of the existing tests? This isn't live yet but I have an open PR with a simple valid update test:
There was a problem hiding this comment.
we have a default in place if there is no explicit extrapolation mode passed so it's not necessary to be passed in 😌
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| partial=True, | ||
| ) | ||
| if validator.is_valid(): | ||
| if data.get("dataset") == Dataset.EventsAnalyticsPlatform.value: |
There was a problem hiding this comment.
Bug: Extrapolation Mode Validation Bypass
The validation checks data.get("dataset") from the request instead of checking the actual alert rule's dataset (alert_rule.snuba_query.dataset). This allows bypassing the extrapolation mode validation during partial updates by omitting the dataset field, enabling users to set server_weighted on EAP alerts.
| new_extrapolation_mode = data_source.get( | ||
| "extrapolation_mode", snuba_query.extrapolation_mode | ||
| ) | ||
| if data_source.get("dataset") == Dataset.EventsAnalyticsPlatform: |
There was a problem hiding this comment.
Bug: Data Validation Flaw Allows Bypass
The validation checks data_source.get("dataset") from the request instead of the final dataset value. This allows bypassing extrapolation mode validation during partial updates where dataset is omitted, enabling server_weighted to be set on EAP detectors.
| ) | ||
|
|
||
| if data.get("extrapolation_mode") == ExtrapolationMode.SERVER_WEIGHTED.name.lower(): | ||
| raise ValidationError("server_weighted extrapolation mode is not supported for new alerts.") |
There was a problem hiding this comment.
Bug: Extrapolation Validation: Type Mismatch Bug
The validation compares extrapolation_mode against ExtrapolationMode.SERVER_WEIGHTED.name.lower() (a string), but the serializer field is IntegerField, so users send integers. This check will never trigger - it should compare against ExtrapolationMode.SERVER_WEIGHTED.value instead, matching the pattern used in _validate_extrapolation_mode.
We've added a new extrapolation mode field in metrics alerts for EAP. We want to make sure the
server_weightedvalue is not put in by the user. They also shouldn't be able to set this extrapolation mode if there already is another mode when updating. I've added tests to confirm but let me know if there are better ways to set and validate the extrapolation mode in the new alerts code.