Conversation
fix: get file from s3
fix: add name for image
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the to_representation methods in the authentication and space serializers to rename keys for presigned URLs (e.g., avatar to url_avatar) and removes hardcoded .png extensions from S3 paths. It also adds a new presigned URL for build artifacts. The review feedback identifies that renaming these keys is a breaking change for the API and that removing the file extension might lead to invalid paths if the underlying data does not include it. Additionally, the reviewer noted that the new S3 calls in the space serializer lack the error handling found in the authentication serializer.
| data["url_avatar"] = get_presigned_url( | ||
| settings.AWS_S3.get("AWS_STORAGE_BUCKET_NAME"), | ||
| f"uploads/{instance.avatar}.png", | ||
| f"uploads/{instance.avatar}", | ||
| ) |
There was a problem hiding this comment.
This change introduces two potential issues: 1. Breaking Change: Renaming the key from avatar to url_avatar is a breaking change for the API. Since avatar remains in Meta.fields, the data dictionary will now contain the raw model value (filename) under the avatar key, while the presigned URL is moved to url_avatar. 2. Path Logic: Removing the .png extension assumes that the instance.avatar field now stores the full filename including the extension. If the database still only stores the identifier without the extension, the generated S3 path will be invalid.
| data["url_logo"] = get_presigned_url( | ||
| settings.AWS_S3.get("AWS_STORAGE_BUCKET_NAME"), | ||
| f"uploads/{instance.logo}.png", | ||
| f"uploads/{instance.logo}", | ||
| ) | ||
| if instance.build_artifact: | ||
| data["url_build_artifact"] = get_presigned_url( | ||
| settings.AWS_S3.get("AWS_STORAGE_BUCKET_NAME"), | ||
| f"uploads/{instance.build_artifact}", | ||
| ) |
There was a problem hiding this comment.
There are two main concerns with this implementation: 1. Inconsistent Error Handling: Unlike ProfileSerializer.to_representation, these calls to get_presigned_url are not wrapped in a try...except block. Failures in the external S3 service will cause the entire request to fail. 2. Breaking Change: Renaming the logo key to url_logo is a breaking change. The logo key will now return the raw value from the model instead of the presigned URL, which may break existing frontend integrations.
1 participant
What?
Why?
How?
Testing?
Anything Else?