Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements functionality to update music names, which includes renaming the file in AWS S3 storage and updating the database record.
Key changes:
- Added
RequestUpdateMusicrequest model withMusicIdandNameproperties - Implemented
RenameFileAsyncmethod in S3Service to copy and delete S3 objects - Created
UpdateMusicUseCaseto orchestrate the music update operation
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| RequestUpdateMusic.cs | New request model for music update operations |
| S3Service.cs | Added RenameFileAsync method to handle S3 file renaming via copy-delete pattern |
| MusicWriteOnlyRepository.cs | Added Update method to persist music changes |
| IS3Service.cs | Added interface method for renaming files |
| IMusicWriteOnlyRepository.cs | Added interface method for updating music entities |
| UpdateMusicUseCase.cs | New use case implementing music update logic with S3 file renaming and database updates |
| IUpdateMusicUseCase.cs | Interface definition for the update music use case |
| DependencyInjectionExtension.cs | Registered UpdateMusicUseCase in DI container |
| MyMusicLibrary.API.csproj | Enabled WarningsAsErrors compiler option |
| MusicController.cs | Added HTTP PUT endpoint for music updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (musicUpdate is null) | ||
| return; | ||
|
|
There was a problem hiding this comment.
The Update method retrieves musicUpdate from the database but never applies the changes from the music parameter to it. The properties like Name and MusicKey should be copied from music to musicUpdate before calling _dbContext.Music.Update(musicUpdate).
| // Copy updatable properties from music to musicUpdate | |
| musicUpdate.Name = music.Name; | |
| musicUpdate.MusicKey = music.MusicKey; | |
| // Add other properties to update as needed |
| await _s3Service.RenameFileAsync(oldKey, newKey); | ||
|
|
||
| music.Name = request.Name; | ||
| music.MusicKey = newKey; | ||
|
|
||
| await _musicWriteOnlyRepository.Update(user, music); | ||
| await _unitOfWork.Commit(); | ||
|
|
There was a problem hiding this comment.
If the S3 rename operation succeeds but the database update fails, the S3 file will be renamed while the database still references the old key, causing data inconsistency. Consider implementing a rollback mechanism or reordering operations to update the database first, then rename the S3 file, so failures leave the system in a consistent state.
| await _s3Service.RenameFileAsync(oldKey, newKey); | |
| music.Name = request.Name; | |
| music.MusicKey = newKey; | |
| await _musicWriteOnlyRepository.Update(user, music); | |
| await _unitOfWork.Commit(); | |
| music.Name = request.Name; | |
| music.MusicKey = newKey; | |
| await _musicWriteOnlyRepository.Update(user, music); | |
| await _unitOfWork.Commit(); | |
| await _s3Service.RenameFileAsync(oldKey, newKey); |
|
|
||
| var oldKey = music.MusicKey!; | ||
| var extension = Path.GetExtension(oldKey); | ||
| var folder = Path.GetDirectoryName(oldKey)?.Replace("\\", "/"); |
There was a problem hiding this comment.
The logic for constructing S3 keys by extracting folder paths and extensions could be fragile. S3 keys are not file system paths and don't have backslashes. The Path.GetDirectoryName method is designed for file system paths and may not handle S3 keys correctly in all cases. Consider using string manipulation with forward slashes directly or creating a dedicated method to parse S3 keys.
| var folder = Path.GetDirectoryName(oldKey)?.Replace("\\", "/"); | |
| var lastSlashIndex = oldKey.LastIndexOf('/'); | |
| var folder = lastSlashIndex >= 0 ? oldKey.Substring(0, lastSlashIndex) : string.Empty; |
| Key = oldKey | ||
| }; | ||
|
|
||
| await _s3Client.DeleteObjectAsync(deleteRequest); |
There was a problem hiding this comment.
The RenameFileAsync method has no error handling. If the copy operation succeeds but the delete operation fails, the old file will remain in S3, causing duplicate files and potential storage cost issues. Consider wrapping the delete operation in a try-catch or validating that the copy succeeded before attempting deletion.
| await _s3Client.DeleteObjectAsync(deleteRequest); | |
| try | |
| { | |
| await _s3Client.DeleteObjectAsync(deleteRequest); | |
| } | |
| catch (Exception ex) | |
| { | |
| // Log the error. Replace with your logging framework if available. | |
| Console.WriteLine($"Failed to delete old S3 object '{oldKey}': {ex.Message}"); | |
| // Optionally, rethrow or handle as needed. | |
| } |
|
No description provided.