Add Preprod endpoint to download project app icons #91
Conversation
propel-test-bot
bot
changed the title
|
Add API endpoint for downloading pre-prod app icons Introduces a new public (experimental) endpoint No existing code is removed; all changes are additive. Key Changes• Added endpoint implementation Affected Areas• This summary was automatically generated by @propel-code-bot |
![propel-test-bot[bot]](https://avatars.githubusercontent.com/u/175999184?s=60&v=4){kind=small}
| ) | ||
|
|
||
| # Upload failed, return appropriate error | ||
| return HttpResponse({"error": "Not found"}, status=404) |
There was a problem hiding this comment.
[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.
| 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| "status": e.status, | ||
| }, | ||
| ) | ||
| return HttpResponse({"error": "Failed to retrieve app icon"}, status=500) |
There was a problem hiding this comment.
[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.
| 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| "app_icon_id": app_icon_id, | ||
| }, | ||
| ) | ||
| return HttpResponse({"error": "Internal server error"}, status=500) |
There was a problem hiding this comment.
[BestPractice]
Same issue here - the dictionary needs to be JSON serialized.
| 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| result = client.get(app_icon_id) | ||
| # Read the entire stream at once | ||
| image_data = result.payload.read() |
There was a problem hiding this comment.
[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| }, | ||
| ) | ||
|
|
||
| # Upload failed, return appropriate error |
There was a problem hiding this comment.
[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.
| # 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|
|
||
| import logging | ||
|
|
||
| from django.http import HttpResponse |
There was a problem hiding this comment.
[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:
| 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
jasonyuezhang
force-pushed
the
ryan/add_api_to_serve_preprod_app_icons
branch
from
dec7590 to
1027469
Compare
propel-test-bot
bot
changed the title
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.
Copied from getsentry#102117
Original PR: getsentry#102117