Skip to content

Conversation

@saponifi3d
Copy link
Contributor

Description

This PR finishes the dataSource to dataSources transition on the API validator, removing data_source from validators and updating tests.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 27, 2025
@codecov
Copy link

codecov bot commented Oct 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #102184      +/-   ##
===========================================
- Coverage   80.96%    80.90%   -0.06%     
===========================================
  Files        8747      8749       +2     
  Lines      389157    390152     +995     
  Branches    24729     24729              
===========================================
+ Hits       315082    315671     +589     
- Misses      73721     74127     +406     
  Partials      354       354              

@saponifi3d saponifi3d marked this pull request as ready for review October 29, 2025 17:06
@saponifi3d saponifi3d requested review from a team as code owners October 29, 2025 17:06
Comment on lines +469 to 472
if "data_sources" in validated_data:
data_source = validated_data.pop("data_sources")[0]

if data_source is not None:
Copy link

Choose a reason for hiding this comment

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

Bug: Accessing [0] on an empty data_sources list, allowed by ListField without min_length, causes an IndexError.
Severity: CRITICAL | Confidence: 0.95

🔍 Detailed Analysis

The data_sources field, defined as a serializers.ListField without a min_length constraint, allows an empty list [] to pass validation. When a client submits an empty list for data_sources, the code attempts to access validated_data.pop("data_sources")[0]. This operation on an empty list results in an IndexError: list index out of range, causing an unhandled exception. This occurs in the update() methods of UptimeDomainCheckFailureValidator, MetricIssueDetectorValidator, and MonitorIncidentDetectorValidator, as well as the create() method of MetricIssueDetectorValidator.

💡 Suggested Fix

Add min_length=1 to the data_sources ListField definitions in UptimeDomainCheckFailureValidator, MetricIssueDetectorValidator, and MonitorIncidentDetectorValidator. Alternatively, add a check for a non-empty list before accessing [0].

🤖 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/uptime/endpoints/validators.py#L469-L472

Potential issue: The `data_sources` field, defined as a `serializers.ListField` without
a `min_length` constraint, allows an empty list `[]` to pass validation. When a client
submits an empty list for `data_sources`, the code attempts to access
`validated_data.pop("data_sources")[0]`. This operation on an empty list results in an
`IndexError: list index out of range`, causing an unhandled exception. This occurs in
the `update()` methods of `UptimeDomainCheckFailureValidator`,
`MetricIssueDetectorValidator`, and `MonitorIncidentDetectorValidator`, as well as the
`create()` method of `MetricIssueDetectorValidator`.

Did we get this right? 👍 / 👎 to inform future reviews.

@saponifi3d saponifi3d merged commit 6ffaacf into master Oct 29, 2025
68 checks passed
@saponifi3d saponifi3d deleted the jcallender/aci/clean-data-source branch October 29, 2025 17:45
@sentry
Copy link

sentry bot commented Oct 31, 2025

Issues attributed to commits in this pull request

This pull request was merged and Sentry observed the following issues:

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