Skip to content
Open
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
1 change: 1 addition & 0 deletions src/sentry/objectstore/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from sentry.objectstore.service import ClientBuilder

attachments = ClientBuilder("attachments")
app_icons = ClientBuilder("app-icons")
140 changes: 140 additions & 0 deletions src/sentry/preprod/api/endpoints/project_preprod_artifact_icon.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
from __future__ import annotations

import logging

from django.http import HttpResponse

Choose a reason for hiding this comment

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

[CriticalError]

Incorrect return type for dictionary response: Django's HttpResponse expects bytes or string content, but you're passing a dictionary {"error": "Not found"}. This will cause a TypeError at runtime. Use JsonResponse instead:

Suggested change
from django.http import HttpResponse
from django.http import HttpResponse, JsonResponse

Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Context for Agents
[**CriticalError**]

Incorrect return type for dictionary response: Django's `HttpResponse` expects bytes or string content, but you're passing a dictionary `{"error": "Not found"}`. This will cause a `TypeError` at runtime. Use `JsonResponse` instead:

```suggestion
from django.http import HttpResponse, JsonResponse
```

⚡ **Committable suggestion**

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

File: src/sentry/preprod/api/endpoints/project_preprod_artifact_icon.py
Line: 5

from rest_framework.request import Request

from sentry.api.api_owners import ApiOwner
from sentry.api.api_publish_status import ApiPublishStatus
from sentry.api.base import region_silo_endpoint
from sentry.api.bases.project import ProjectEndpoint
from sentry.models.project import Project
from sentry.objectstore import app_icons
from sentry.objectstore.service import ClientError

logger = logging.getLogger(__name__)


def detect_image_content_type(image_data: bytes) -> str:
"""
Detect the content type of an image from its magic bytes.
Returns the appropriate MIME type or a default if unknown.
"""
if not image_data:
return "application/octet-stream"

# Check magic bytes for common image formats
if image_data[:8] == b"\x89PNG\r\n\x1a\n":
return "image/png"
elif image_data[:3] == b"\xff\xd8\xff":
return "image/jpeg"
elif image_data[:4] == b"RIFF" and image_data[8:12] == b"WEBP":
return "image/webp"
elif image_data[:2] in (b"BM", b"BA", b"CI", b"CP", b"IC", b"PT"):
return "image/bmp"
elif image_data[:6] in (b"GIF87a", b"GIF89a"):
return "image/gif"
elif image_data[:4] == b"\x00\x00\x01\x00":
return "image/x-icon"
elif len(image_data) >= 12 and image_data[4:12] in (b"ftypavif", b"ftypavis"):
return "image/avif"
elif len(image_data) >= 12 and image_data[4:12] in (
b"ftypheic",
b"ftypheix",
b"ftyphevc",
b"ftyphevx",
):
return "image/heic"

# Default to generic binary if we can't detect the type
logger.warning(
"Could not detect image content type from magic bytes",
extra={"first_bytes": image_data[:16].hex() if len(image_data) >= 16 else image_data.hex()},
)
return "application/octet-stream"


@region_silo_endpoint
class ProjectPreprodArtifactIconEndpoint(ProjectEndpoint):
owner = ApiOwner.EMERGE_TOOLS
publish_status = {
"GET": ApiPublishStatus.EXPERIMENTAL,
}

def get(
self,
request: Request,
project: Project,
app_icon_id: str,
) -> HttpResponse:
organization_id = project.organization_id
project_id = project.id

# object_key = f"{organization_id}/{project_id}/{app_icon_id}"
logger.info(
"Retrieving app icon from objectstore",
extra={
"organization_id": organization_id,
"project_id": project_id,
"app_icon_id": app_icon_id,
},
)
client = app_icons.for_project(organization_id, project_id)

try:
result = client.get(app_icon_id)
# Read the entire stream at once
image_data = result.payload.read()
Comment on lines +86 to +88

Choose a reason for hiding this comment

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

[BestPractice]

Resource leak: The result.payload.read() call reads from a stream but there's no guarantee that the underlying stream/connection is properly closed. If result.payload implements a context manager or has a close method, it should be used to ensure proper resource cleanup.

Consider using a context manager pattern:

with client.get(app_icon_id) as result:
    image_data = result.payload.read()

Or explicitly close the resource:

result = client.get(app_icon_id)
try:
    image_data = result.payload.read()
finally:
    if hasattr(result.payload, 'close'):
        result.payload.close()
Context for Agents
[**BestPractice**]

Resource leak: The `result.payload.read()` call reads from a stream but there's no guarantee that the underlying stream/connection is properly closed. If `result.payload` implements a context manager or has a close method, it should be used to ensure proper resource cleanup.

Consider using a context manager pattern:
```python
with client.get(app_icon_id) as result:
    image_data = result.payload.read()
```

Or explicitly close the resource:
```python
result = client.get(app_icon_id)
try:
    image_data = result.payload.read()
finally:
    if hasattr(result.payload, 'close'):
        result.payload.close()
```

File: src/sentry/preprod/api/endpoints/project_preprod_artifact_icon.py
Line: 88


# Detect content type from the image data
content_type = detect_image_content_type(image_data)

logger.info(
"Retrieved app icon from objectstore",
extra={
"organization_id": organization_id,
"project_id": project_id,
"app_icon_id": app_icon_id,
"size_bytes": len(image_data),
"content_type": content_type,
},
)
return HttpResponse(image_data, content_type=content_type)

except ClientError as e:
if e.status == 404:
logger.warning(
"App icon not found in objectstore",
extra={
"organization_id": organization_id,
"project_id": project_id,
"app_icon_id": app_icon_id,
},
)

# Upload failed, return appropriate error

Choose a reason for hiding this comment

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

[Documentation]

The comment says "Upload failed, return appropriate error" but this is actually a GET endpoint for retrieving icons, not uploading. The comment should be corrected to reflect the actual operation.

Suggested change
# Upload failed, return appropriate error
# App icon not found, return appropriate error

Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Context for Agents
[**Documentation**]

The comment says "Upload failed, return appropriate error" but this is actually a GET endpoint for retrieving icons, not uploading. The comment should be corrected to reflect the actual operation.

```suggestion
                # App icon not found, return appropriate error
```

⚡ **Committable suggestion**

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

File: src/sentry/preprod/api/endpoints/project_preprod_artifact_icon.py
Line: 116

return HttpResponse({"error": "Not found"}, status=404)

Choose a reason for hiding this comment

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

[CriticalError]

The HttpResponse constructor expects either a string/bytes as the first argument or uses keyword arguments for structured data. Line 117 and 129 are passing dictionaries directly as the first argument, which will result in the dictionary being converted to a string representation like "{'error': 'Not found'}" instead of proper JSON.

Suggested change
return HttpResponse({"error": "Not found"}, status=404)
return HttpResponse(json.dumps({"error": "Not found"}), content_type="application/json", status=404)

Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Context for Agents
[**CriticalError**]

The `HttpResponse` constructor expects either a string/bytes as the first argument or uses keyword arguments for structured data. Line 117 and 129 are passing dictionaries directly as the first argument, which will result in the dictionary being converted to a string representation like `"{'error': 'Not found'}"` instead of proper JSON.

```suggestion
return HttpResponse(json.dumps({"error": "Not found"}), content_type="application/json", status=404)
```

⚡ **Committable suggestion**

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

File: src/sentry/preprod/api/endpoints/project_preprod_artifact_icon.py
Line: 117


logger.warning(
"Failed to retrieve app icon from objectstore",
extra={
"organization_id": organization_id,
"project_id": project_id,
"app_icon_id": app_icon_id,
"error": str(e),
"status": e.status,
},
)
return HttpResponse({"error": "Failed to retrieve app icon"}, status=500)

Choose a reason for hiding this comment

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

[BestPractice]

Same issue here - the dictionary will be converted to string representation instead of proper JSON. This needs to import json module and serialize the dictionary.

Suggested change
return HttpResponse({"error": "Failed to retrieve app icon"}, status=500)
return HttpResponse(json.dumps({"error": "Failed to retrieve app icon"}), content_type="application/json", status=500)

Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Context for Agents
[**BestPractice**]

Same issue here - the dictionary will be converted to string representation instead of proper JSON. This needs to import `json` module and serialize the dictionary.

```suggestion
return HttpResponse(json.dumps({"error": "Failed to retrieve app icon"}), content_type="application/json", status=500)
```

⚡ **Committable suggestion**

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

File: src/sentry/preprod/api/endpoints/project_preprod_artifact_icon.py
Line: 129


except Exception:
logger.exception(
"Unexpected error retrieving app icon",
extra={
"organization_id": organization_id,
"project_id": project_id,
"app_icon_id": app_icon_id,
},
)
return HttpResponse({"error": "Internal server error"}, status=500)

Choose a reason for hiding this comment

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

[BestPractice]

Same issue here - the dictionary needs to be JSON serialized.

Suggested change
return HttpResponse({"error": "Internal server error"}, status=500)
return HttpResponse(json.dumps({"error": "Internal server error"}), content_type="application/json", status=500)

Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Context for Agents
[**BestPractice**]

Same issue here - the dictionary needs to be JSON serialized.

```suggestion
return HttpResponse(json.dumps({"error": "Internal server error"}), content_type="application/json", status=500)
```

⚡ **Committable suggestion**

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

File: src/sentry/preprod/api/endpoints/project_preprod_artifact_icon.py
Line: 140

8 changes: 8 additions & 0 deletions src/sentry/preprod/api/endpoints/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

from django.urls import re_path

from sentry.preprod.api.endpoints.project_preprod_artifact_icon import (
ProjectPreprodArtifactIconEndpoint,
)
from sentry.preprod.api.endpoints.size_analysis.project_preprod_size_analysis_compare import (
ProjectPreprodArtifactSizeAnalysisCompareEndpoint,
)
Expand Down Expand Up @@ -81,6 +84,11 @@
ProjectInstallablePreprodArtifactDownloadEndpoint.as_view(),
name="sentry-api-0-installable-preprod-artifact-download",
),
re_path(
r"^(?P<organization_id_or_slug>[^/]+)/(?P<project_id_or_slug>[^/]+)/files/app-icons/(?P<app_icon_id>[^/]+)/$",
ProjectPreprodArtifactIconEndpoint.as_view(),
name="sentry-api-0-project-preprod-app-icon",
),
# Size analysis
re_path(
r"^(?P<organization_id_or_slug>[^/]+)/(?P<project_id_or_slug>[^/]+)/preprodartifacts/size-analysis/compare/(?P<head_artifact_id>[^/]+)/(?P<base_artifact_id>[^/]+)/$",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class BuildDetailsAppInfo(BaseModel):
platform: Platform | None = None
is_installable: bool
build_configuration: str | None = None
app_icon_id: str | None = None


class BuildDetailsVcsInfo(BaseModel):
Expand Down Expand Up @@ -152,6 +153,7 @@ def transform_preprod_artifact_to_build_details(
build_configuration=(
artifact.build_configuration.name if artifact.build_configuration else None
),
app_icon_id=artifact.app_icon_id,
)

vcs_info = BuildDetailsVcsInfo(
Expand Down