-
-
Couldn't load subscription status.
- Fork 4.5k
feat(preprod): Add app-icon download endpoint #102117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| from sentry.objectstore.service import ClientBuilder | ||
|
|
||
| attachments = ClientBuilder("attachments") | ||
| preprod = ClientBuilder("preprod") |
| 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 | ||
| 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 preprod | ||
| 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 ProjectPreprodArtifactImageEndpoint(ProjectEndpoint): | ||
| owner = ApiOwner.EMERGE_TOOLS | ||
| publish_status = { | ||
| "GET": ApiPublishStatus.EXPERIMENTAL, | ||
| } | ||
|
|
||
| def get( | ||
| self, | ||
| request: Request, | ||
| project: Project, | ||
| image_id: str, | ||
| ) -> HttpResponse: | ||
| organization_id = project.organization_id | ||
| project_id = project.id | ||
|
|
||
| object_key = f"{organization_id}/{project_id}/{image_id}" | ||
| logger.info( | ||
| "Retrieving image from objectstore", | ||
| extra={ | ||
| "organization_id": organization_id, | ||
| "project_id": project_id, | ||
| "image_id": image_id, | ||
| }, | ||
| ) | ||
| client = preprod.for_project(organization_id, project_id) | ||
|
|
||
| try: | ||
| result = client.get(object_key) | ||
| # Read the entire stream at once | ||
| image_data = result.payload.read() | ||
|
|
||
| # Detect content type from the image data | ||
| content_type = detect_image_content_type(image_data) | ||
|
|
||
| logger.info( | ||
| "Retrieved image from objectstore", | ||
| extra={ | ||
| "organization_id": organization_id, | ||
| "project_id": project_id, | ||
| "image_id": image_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, | ||
| "image_id": image_id, | ||
| }, | ||
| ) | ||
|
|
||
| # Upload failed, return appropriate error | ||
| return HttpResponse({"error": "Not found"}, status=404) | ||
|
|
||
| logger.warning( | ||
| "Failed to retrieve app icon from objectstore", | ||
| extra={ | ||
| "organization_id": organization_id, | ||
| "project_id": project_id, | ||
| "image_id": image_id, | ||
| "error": str(e), | ||
| "status": e.status, | ||
| }, | ||
| ) | ||
| return HttpResponse({"error": "Failed to retrieve app icon"}, status=500) | ||
|
|
||
| except Exception: | ||
| logger.exception( | ||
| "Unexpected error retrieving app icon", | ||
| extra={ | ||
| "organization_id": organization_id, | ||
| "project_id": project_id, | ||
| "image_id": image_id, | ||
| }, | ||
| ) | ||
| return HttpResponse({"error": "Internal server error"}, status=500) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,9 @@ | |
|
|
||
| from django.urls import re_path | ||
|
|
||
| from sentry.preprod.api.endpoints.project_preprod_artifact_icon import ( | ||
| ProjectPreprodArtifactImageEndpoint, | ||
|
Comment on lines
+5
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: 🔍 Detailed AnalysisThe 💡 Suggested FixCorrect the import path in 🤖 Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
| ) | ||
| from sentry.preprod.api.endpoints.size_analysis.project_preprod_size_analysis_compare import ( | ||
| ProjectPreprodArtifactSizeAnalysisCompareEndpoint, | ||
| ) | ||
|
|
@@ -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/images/(?P<image_id>[^/]+)/$", | ||
| ProjectPreprodArtifactImageEndpoint.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>[^/]+)/$", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug:
HttpResponseis incorrectly used with dictionary objects at lines 117, 129, 140.Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The code at lines 117, 129, and 140 attempts to return
HttpResponsewith Python dictionary objects, such asHttpResponse({"error": "Not found"}, status=404). TheHttpResponseconstructor does not accept dictionaries directly; it expects content as strings, bytes, or iterators. This will result in aTypeErrorduring response serialization, causing a generic 500 error to be returned to the client instead of the intended structured error response.💡 Suggested Fix
Replace
HttpResponsewithJsonResponsewhen returning dictionary objects. For example, changereturn HttpResponse({"error": "Not found"}, status=404)toreturn JsonResponse({"error": "Not found"}, status=404).🤖 Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.