Skip to content

hodl#109397

Closed
NicoHinderling wants to merge 0 commit intonh/add-endpoint-logic-to-trigger-taskfrom
02-23-nh-snapshots-wip-testing
Closed

hodl#109397
NicoHinderling wants to merge 0 commit intonh/add-endpoint-logic-to-trigger-taskfrom
02-23-nh-snapshots-wip-testing

Conversation

@NicoHinderling
Copy link
Contributor

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

Copy link
Contributor Author

NicoHinderling commented Feb 25, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Feb 25, 2026
@github-actions
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

Comment on lines 55 to 58
try:
artifact = PreprodArtifact.objects.select_related("commit_comparison").get(
id=snapshot_id, project_id=project.id
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unhandled ValueError when snapshot_id is non-numeric string

The endpoint accepts snapshot_id as a URL parameter (matched by [^/]+ pattern allowing any string) and passes it directly to .get(id=snapshot_id). When snapshot_id is a non-integer string like 'abc', Django raises ValueError: invalid literal for int() with base 10. Only DoesNotExist is caught, so the ValueError propagates as an unhandled 500 error. Other preprod endpoints like PreprodArtifactEndpoint explicitly catch both DoesNotExist and ValueError.

Verification

Verified by reading: 1) The URL pattern in urls.py line 84 uses [^/]+ which accepts any string for snapshot_id. 2) The PreprodArtifactEndpoint base class (preprod_artifact_endpoint.py lines 55-60) properly catches both exceptions. 3) The PreprodArtifactAdminRerunAnalysisEndpoint (lines 147-153) explicitly validates the ID is numeric. 4) Django ORM raises ValueError when a non-integer string is used for an integer PK lookup.

Suggested fix: Catch ValueError in addition to DoesNotExist and return a 404 response

Suggested change
try:
artifact = PreprodArtifact.objects.select_related("commit_comparison").get(
id=snapshot_id, project_id=project.id
)
id=int(snapshot_id), project_id=project.id
except (PreprodArtifact.DoesNotExist, ValueError):
Also found at 1 additional location
  • src/sentry/preprod/api/endpoints/snapshot_diff.py:28-28

Identified by Warden sentry-backend-bugs · 54R-HMN

Comment on lines 43 to 54
{
"diff_mask_png": result.diff_mask_png,
"diff_score": result.diff_score,
"changed_pixels": result.changed_pixels,
"total_pixels": result.total_pixels,
"aligned_height": result.aligned_height,
"width": result.width,
"before_width": result.before_width,
"before_height": result.before_height,
"after_width": result.after_width,
"after_height": result.after_height,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

AttributeError when compare_images returns None

The compare_images() function can return None when image processing fails (see _compare_pairs exception handling in compare.py lines 81-83). On lines 43-54, the code accesses result.diff_mask_png, result.diff_score, etc. without checking if result is None, causing AttributeError: 'NoneType' object has no attribute 'diff_mask_png'. This matches the SENTRY-3Z3P pattern where serializers return None instead of a dict.

Verification

Read compare.py and confirmed compare_images() has return type DiffResult | None. The _compare_pairs function catches exceptions and appends None to results (lines 81-83). The endpoint accesses result attributes without a None check.

Suggested fix: Add a None check after calling compare_images and return an appropriate error response.

Suggested change
{
"diff_mask_png": result.diff_mask_png,
"diff_score": result.diff_score,
"changed_pixels": result.changed_pixels,
"total_pixels": result.total_pixels,
"aligned_height": result.aligned_height,
"width": result.width,
"before_width": result.before_width,
"before_height": result.before_height,
"after_width": result.after_width,
"after_height": result.after_height,
}
if result is None:
return Response({"detail": "Failed to compare images"}, status=400)

Identified by Warden sentry-backend-bugs · 2S4-7SS

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 Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant