Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

from sentry.preprod.build_distribution_utils import is_installable_artifact
from sentry.preprod.models import PreprodArtifact, PreprodArtifactSizeMetrics
from sentry.preprod.vcs.status_checks.size.tasks import StatusCheckErrorType

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -52,6 +53,32 @@ class BuildDetailsVcsInfo(BaseModel):
pr_number: int | None = None


class StatusCheckResultSuccess(BaseModel):
"""Result of a successfully posted status check."""

success: Literal[True] = True
check_id: str | None = None


class StatusCheckResultFailure(BaseModel):
"""Result of a failed status check post."""

success: Literal[False] = False
error_type: StatusCheckErrorType | None = None


StatusCheckResult = Annotated[
StatusCheckResultSuccess | StatusCheckResultFailure,
Field(discriminator="success"),
]


class PostedStatusChecks(BaseModel):
"""Status checks that have been posted to the VCS provider."""

size: StatusCheckResult | None = None


class SizeInfoSizeMetric(BaseModel):
metrics_artifact_type: PreprodArtifactSizeMetrics.MetricsArtifactType
install_size_bytes: int
Expand Down Expand Up @@ -101,6 +128,7 @@ class BuildDetailsApiResponse(BaseModel):
app_info: BuildDetailsAppInfo
vcs_info: BuildDetailsVcsInfo
size_info: SizeInfo | None = None
posted_status_checks: PostedStatusChecks | None = None


def platform_from_artifact_type(artifact_type: PreprodArtifact.ArtifactType) -> Platform:
Expand Down Expand Up @@ -237,10 +265,41 @@ def transform_preprod_artifact_to_build_details(
pr_number=(artifact.commit_comparison.pr_number if artifact.commit_comparison else None),
)

posted_status_checks = None
if artifact.extras and "posted_status_checks" in artifact.extras:
raw_checks = artifact.extras["posted_status_checks"]
size_check: StatusCheckResult | None = None

if isinstance(raw_checks, dict) and "size" in raw_checks:
raw_size = raw_checks["size"]
if isinstance(raw_size, dict):
if raw_size.get("success"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Truthiness check mishandles non-boolean success values

The success field check uses truthiness rather than explicit boolean comparison. This causes {"success": "false"} to be treated as success since the string is truthy, and missing or null success fields to be treated as failures. The check should verify success is True or validate that it's a boolean before branching, ensuring corrupted data is handled consistently with other validation (skipped rather than misinterpreted).

Fix in Cursor Fix in Web

check_id = raw_size.get("check_id")
size_check = StatusCheckResultSuccess(check_id=check_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Unvalidated check_id causes Pydantic validation crash

The check_id value is passed directly to StatusCheckResultSuccess without type validation. If corrupted data contains a non-string value like a dict or list, Pydantic raises ValidationError, crashing the endpoint with a 500 error. This contradicts the tests expecting corrupted data to return 200 with posted_status_checks: None. The code should validate check_id is a string or None, or wrap the Pydantic construction in try-except.

Fix in Cursor Fix in Web

else:
error_type: StatusCheckErrorType | None = None
error_type_str = raw_size.get("error_type")
if error_type_str:
try:
error_type = StatusCheckErrorType(error_type_str)
except ValueError:
logger.warning(
"preprod.build_details.invalid_error_type",
extra={
"artifact_id": artifact.id,
"error_type": error_type_str,
},
)
size_check = StatusCheckResultFailure(error_type=error_type)

if size_check is not None:
posted_status_checks = PostedStatusChecks(size=size_check)

return BuildDetailsApiResponse(
id=artifact.id,
state=artifact.state,
app_info=app_info,
vcs_info=vcs_info,
size_info=size_info,
posted_status_checks=posted_status_checks,
)
Original file line number Diff line number Diff line change
Expand Up @@ -275,3 +275,129 @@ def test_get_build_details_with_missing_proguard_mapping(self) -> None:
resp_data = response.json()
assert resp_data["app_info"]["android_app_info"]["has_proguard_mapping"] is False
assert resp_data["app_info"]["apple_app_info"] is None

def test_posted_status_checks_success(self) -> None:
"""Test that successfully posted status checks are returned."""
self.preprod_artifact.extras = {
"posted_status_checks": {"size": {"success": True, "check_id": "12345"}}
}
self.preprod_artifact.save()

url = self._get_url()
response = self.client.get(
url, format="json", HTTP_AUTHORIZATION=f"Bearer {self.api_token.token}"
)

assert response.status_code == 200
resp_data = response.json()
assert resp_data["posted_status_checks"] is not None
assert resp_data["posted_status_checks"]["size"]["success"] is True
assert resp_data["posted_status_checks"]["size"]["check_id"] == "12345"

def test_posted_status_checks_failure(self) -> None:
"""Test that failed status check posts are returned."""
self.preprod_artifact.extras = {
"posted_status_checks": {"size": {"success": False, "error_type": "api_error"}}
}
self.preprod_artifact.save()

url = self._get_url()
response = self.client.get(
url, format="json", HTTP_AUTHORIZATION=f"Bearer {self.api_token.token}"
)

assert response.status_code == 200
resp_data = response.json()
assert resp_data["posted_status_checks"] is not None
assert resp_data["posted_status_checks"]["size"]["success"] is False
assert resp_data["posted_status_checks"]["size"]["error_type"] == "api_error"

def test_posted_status_checks_none_when_not_present(self) -> None:
"""Test that posted_status_checks is None when not in extras."""
url = self._get_url()
response = self.client.get(
url, format="json", HTTP_AUTHORIZATION=f"Bearer {self.api_token.token}"
)

assert response.status_code == 200
resp_data = response.json()
assert resp_data["posted_status_checks"] is None

def test_posted_status_checks_success_without_check_id(self) -> None:
"""Test that successful status checks without check_id are still exposed."""
self.preprod_artifact.extras = {"posted_status_checks": {"size": {"success": True}}}
self.preprod_artifact.save()

url = self._get_url()
response = self.client.get(
url, format="json", HTTP_AUTHORIZATION=f"Bearer {self.api_token.token}"
)

assert response.status_code == 200
resp_data = response.json()
assert resp_data["posted_status_checks"] is not None
assert resp_data["posted_status_checks"]["size"]["success"] is True
assert resp_data["posted_status_checks"]["size"]["check_id"] is None

def test_posted_status_checks_with_corrupted_checks_structure(self) -> None:
"""Test that corrupted posted_status_checks structure doesn't crash."""
self.preprod_artifact.extras = {"posted_status_checks": "not_a_dict"}
self.preprod_artifact.save()

url = self._get_url()
response = self.client.get(
url, format="json", HTTP_AUTHORIZATION=f"Bearer {self.api_token.token}"
)

assert response.status_code == 200
resp_data = response.json()
assert resp_data["posted_status_checks"] is None

def test_posted_status_checks_with_corrupted_size_structure(self) -> None:
"""Test that corrupted size check structure doesn't crash."""
self.preprod_artifact.extras = {"posted_status_checks": {"size": "not_a_dict"}}
self.preprod_artifact.save()

url = self._get_url()
response = self.client.get(
url, format="json", HTTP_AUTHORIZATION=f"Bearer {self.api_token.token}"
)

assert response.status_code == 200
resp_data = response.json()
# Corrupted size data is skipped, so posted_status_checks should be None
assert resp_data["posted_status_checks"] is None

def test_posted_status_checks_failure_without_error_type(self) -> None:
"""Test that failed status checks without error_type are still exposed."""
self.preprod_artifact.extras = {"posted_status_checks": {"size": {"success": False}}}
self.preprod_artifact.save()

url = self._get_url()
response = self.client.get(
url, format="json", HTTP_AUTHORIZATION=f"Bearer {self.api_token.token}"
)

assert response.status_code == 200
resp_data = response.json()
assert resp_data["posted_status_checks"] is not None
assert resp_data["posted_status_checks"]["size"]["success"] is False
assert resp_data["posted_status_checks"]["size"]["error_type"] is None

def test_posted_status_checks_failure_with_invalid_error_type(self) -> None:
"""Test that failed status checks with invalid error_type are still exposed."""
self.preprod_artifact.extras = {
"posted_status_checks": {"size": {"success": False, "error_type": "not_valid"}}
}
self.preprod_artifact.save()

url = self._get_url()
response = self.client.get(
url, format="json", HTTP_AUTHORIZATION=f"Bearer {self.api_token.token}"
)

assert response.status_code == 200
resp_data = response.json()
assert resp_data["posted_status_checks"] is not None
assert resp_data["posted_status_checks"]["size"]["success"] is False
assert resp_data["posted_status_checks"]["size"]["error_type"] is None
Loading