Conversation
Summary of ChangesHello @sleepyhoon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 파일 업로드 아키텍처를 클라이언트 직접 업로드 방식에서 서버 직접 업로드 방식으로 전환하여, 이미지 파일 관리에 대한 서버의 제어권을 강화하고 확장성을 확보했습니다. 새로운 엔티티와 관리자 클래스를 도입하여 이미지 메타데이터를 체계적으로 관리하며, 향후 이미지 리사이징 및 다양한 변형 처리를 위한 기반을 마련했습니다. 이를 통해 파일 업로드 과정의 안정성을 높이고, 비즈니스 로직과의 통합을 용이하게 합니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
이번 PR은 프로필 이미지 업로드 기능을 도입하면서 기존 Presigned URL 방식에서 서버 직접 업로드 방식으로 아키텍처를 개선한 점이 인상적입니다. 특히 AbstractImageManager에 템플릿 메서드 패턴을 적용하여 확장성 높은 구조를 설계하신 점이 돋보입니다. 전반적으로 코드 품질이 우수하고 문서화도 상세하게 작성되었습니다.
다만, 몇 가지 중요한 수정이 필요해 보입니다. API 문서가 변경된 업로드 흐름을 반영하지 않아 혼란을 줄 수 있으며, 테스트 코드의 상당수가 새로운 API 명세에 맞게 업데이트되지 않아 깨져있는 상태입니다. 이는 향후 유지보수에 큰 위험이 될 수 있습니다. 또한, ImageFileServiceImpl에서 데이터베이스와 파일 스토리지 간의 작업이 원자적으로 처리되지 않아 리소스 누수가 발생할 가능성이 있습니다.
자세한 내용은 각 파일에 남긴 코멘트를 참고하여 수정해주시면 감사하겠습니다. 해당 부분들이 보완된다면 더욱 안정적이고 완성도 높은 기능이 될 것입니다.
src/test/java/com/studypals/global/file/dao/AbstractImageManagerTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/studypals/global/file/service/ImageFileServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This pull request adds profile image upload functionality and refactors the file upload architecture using the template method pattern. The implementation moves from a Presigned URL approach (where clients upload directly to MinIO) to a direct server upload approach (where the server receives files and uploads them to MinIO).
Changes:
- Introduced new entity hierarchy with
ImageFileas an abstract base class, andMemberProfileImageandChatImageas concrete implementations with separate tables - Refactored file management logic into
AbstractFileManagerandAbstractImageManagerusing template method pattern - Changed API from Presigned URL generation to direct multipart file upload with new DTOs (
ImageUploadResreplacingPresignedUrlRes) - Added
FilePropertiesconfiguration class andFileUtilsutility for better separation of concerns - Updated MinIO integration to support direct file uploads and removed Presigned PUT URL functionality
- Modified storage paths from "profile/" and "chat/" to "origin/profile/" and "origin/chat/" for future variant support
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 39 comments.
Show a summary per file
| File | Description |
|---|---|
ImageFileServiceImpl.java |
Refactored to handle direct file uploads with transaction coordination between MinIO and database |
AbstractImageManager.java |
Introduced template method pattern for common image handling logic with hooks for domain-specific behavior |
MemberProfileImage.java |
New entity for profile images with update support and 1:1 relationship with Member |
ChatImage.java |
New entity for chat images with immutable characteristics and M:1 relationship with ChatRoom |
ImageFileController.java |
Changed from JSON request body to multipart/form-data for direct file uploads |
MinioStorage.java |
Added direct upload method and removed Presigned PUT URL generation |
CleanUp.java |
Enhanced test cleanup to handle non-existent tables gracefully |
FileProperties.java |
New configuration class for type-safe property binding |
FileUtils.java |
New utility class for file-related operations like extension extraction |
Comments suppressed due to low confidence (3)
src/test/java/com/studypals/domain/chatManage/worker/ChatImageManagerTest.java:44
- Critical security validation test was removed and not replaced. The old test suite had
getUploadUrl_fail_notMemberwhich verified that users who are not members of a chat room cannot upload images to it. While thevalidateTargetIdimplementation is still present in the code, there's no test coverage for this critical security check.
The test should be restored to verify:
- Members of a chat room can upload images
- Non-members of a chat room cannot upload images (should throw ChatException with CHAT_ROOM_NOT_CONTAIN_MEMBER)
- The chatRoomReader.isMemberOfChatRoom is properly called during validation
src/main/java/com/studypals/domain/memberManage/worker/MemberProfileImageManager.java:33
- The PROFILE_PATH has been changed from "profile" to "origin/profile". While this might be intentional to separate original images from resized variants, this is a breaking change that affects existing file paths in MinIO. If there are any existing profile images stored under the "profile/" path, they will become inaccessible with this change.
Consider:
- If this is intentional for new architecture, ensure a migration strategy exists for old images
- Document this breaking change clearly
- Or use a configuration property to maintain backward compatibility
src/main/java/com/studypals/domain/memberManage/worker/MemberProfileImageManager.java:51
- The
variants()method returns a list of image variant sizes (SMALL, LARGE) but there's no code that actually uses this to generate the variants. This is the same issue as in ChatImageManager - the method is defined but never called. Either implement the variant generation logic or document this as preparation for future functionality.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/com/studypals/global/file/service/ImageFileServiceImpl.java
Show resolved
Hide resolved
src/main/java/com/studypals/domain/memberManage/worker/MemberProfileImageWriter.java
Show resolved
Hide resolved
src/main/java/com/studypals/domain/memberManage/service/MemberServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/studypals/domain/memberManage/entity/MemberProfileImage.java
Show resolved
Hide resolved
src/main/java/com/studypals/domain/chatManage/worker/ChatImageManager.java
Show resolved
Hide resolved
src/test/java/com/studypals/global/file/restDocsTest/ImageFileControllerRestDocsTest.java
Show resolved
Hide resolved
src/test/java/com/studypals/global/file/service/ImageFileServiceImplTest.java
Show resolved
Hide resolved
src/test/java/com/studypals/global/file/service/ImageFileServiceImplTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/studypals/domain/memberManage/entity/MemberProfileImage.java
Show resolved
Hide resolved
|
@unikal1 리뷰 부탁드립니다 |
| @PostMapping(value = "/profile", consumes = MediaType.MULTIPART_FORM_DATA_VALUE) | ||
| public ResponseEntity<Response<ImageUploadRes>> uploadProfileImage( | ||
| @RequestPart("file") MultipartFile file, @AuthenticationPrincipal Long userId) { | ||
| ImageUploadRes response = imageFileService.uploadProfileImage(file, userId); | ||
| return ResponseEntity.ok(CommonResponse.success(ResponseCode.FILE_IMAGE_UPLOAD, response)); | ||
| } | ||
|
|
||
| @PostMapping("/chat") | ||
| public ResponseEntity<Response<PresignedUrlRes>> getUploadUrl( | ||
| @Valid @RequestBody ChatPresignedUrlReq request, @AuthenticationPrincipal Long userId) { | ||
| PresignedUrlRes response = imageFileService.getChatUploadUrl(request, userId); | ||
| @PostMapping(value = "/chat", consumes = MediaType.MULTIPART_FORM_DATA_VALUE) | ||
| public ResponseEntity<Response<ImageUploadRes>> uploadChatImage( | ||
| @RequestPart("file") MultipartFile file, | ||
| @RequestParam("chatRoomId") String chatRoomId, | ||
| @AuthenticationPrincipal Long userId) { | ||
| ImageUploadRes response = imageFileService.uploadChatImage(file, chatRoomId, userId); | ||
| return ResponseEntity.ok(CommonResponse.success(ResponseCode.FILE_IMAGE_UPLOAD, response)); | ||
| } | ||
| } |
There was a problem hiding this comment.
[P1]
해당 기능들은 도메인에 종속되어 있으므로 각 도메인 API 로 이전해야 된다고 생각합니다. 해당 도메인의 service 에서 전략 구현 객체를 추가하고 이를 토대로 업로드 하는 것이 더 자연스럽지 않을까 싶습니다.
| public class ImageFileServiceImpl implements ImageFileService { | ||
|
|
||
| private final Map<FileType, AbstractFileManager> managerMap; | ||
| private final Map<FileType, AbstractImageManager> managerMap; |
There was a problem hiding this comment.
[P2]
전략 패턴의 사용이 좀 잘못 된 것 같습니다.
현재는 각 도메인에 따라 enum 을 정의하고, 해당 enum 을 파라미터로 하여 적절한 전략 객체를 찾고 있는데, 결국 해당 enum 이 모든 도메인을 알아야 된다는 점에서 전략패턴 + DI 방식을 제대로 이용하고 있다고 보기 어려운 것 같습니다.
enum 이 아닌, 특정 인터페이스를 만들고, 이에 따른 구현체를 각 도메인에서 정의하도록 하여, 전략 패턴의 팩토리 클래스에서 적절한 전략을 반환하되, 이때 파라미터가 해당 구현체가 되도록 하는 것입니다.
아주 간단한 예시를 들어보겠습니다.
Image 라는 인터페이스를 만들고, ChatImage, ProfileImage... 등이 있다고 해봅시다. 이는 Image 의 구현 클래스입니다.
Map 을 통해 service 에서 적절한 구현 객체를 찾는 것이 아닌 ImageManagerFactory 에서 전략 패턴의 팩토리 클래스를 담당합니다.
이때, resolve 메서드는 Image 를 받으며, ImageManagerFactory 는 Map<Image, AbstractImageManager> 를 가집니다. resolver 는 Image 구현체를 받아(interface 인지 검사 먼저 하고) 위 map 에서 찾아 반환합니다.
이때, ImageManagerFactory 는 사실 상 worker 클래스이며, 각 도메인의 service 에서 해당 worker 를 사용해 이미지를 업로드/url 발급 등을 수행합니다.
|
현재는 이미지 처리상태가 PENDING에서 COMPLETE나 FAILED로 갱신하는 부분은 없는건가요? |
네 저 값은 리사이징이 도입되면 사용할 계획입니다. 원래 리사이징도 넣으려고 했는데, PR이 지나치게 길어진다고 판단해서 분리해야 할 듯 합니다. |
✨ 구현 기능 명세
파일 업로드 로직 추상화
MemberProfileImageManager와ChatImageManager에 중복되던 파일 업로드 로직을 부모 클래스(AbstractImageManager)로 통합하고, 템플릿 메서드 패턴을 적용하여 재사용성을 높였습니다.classDiagram direction TB class ImageFileService { -Map~FileType, AbstractImageManager~ managerMap +uploadProfileImage() +uploadChatImage() } class AbstractImageManager { <<Abstract>> +upload() #performUpload() [Template Method] #createObjectKey() #generateObjectKeyDetail()* [Abstract] #usePresignedUrl()* [Abstract] } class MemberProfileImageManager { +getFileType() : PROFILE_IMAGE #generateObjectKeyDetail() #usePresignedUrl() : false } class ChatImageManager { +getFileType() : CHAT_IMAGE #generateObjectKeyDetail() #validateTargetId() #usePresignedUrl() : true } class FileType { <<Interface>> } ImageFileService --> AbstractImageManager : Uses (Strategy) AbstractImageManager <|-- MemberProfileImageManager : Extends AbstractImageManager <|-- ChatImageManager : Extends MemberProfileImageManager ..|> FileType : Implements via Enum ChatImageManager ..|> FileType : Implements via Enum이해를 돕기 위해
AbstractImageManager내부에 있는 메서드인performUpload메서드를 보겠습니다.따라서 업로드에 필요한 로직은 고정시키기 위해
AbstractImageManager내부에 두었고, 구현체에 따라 달라지는 로직은 abstract 메서드로 두었습니다.엔티티 설계
추상 클래스인
ImageFile을 공통으로 두고, 프로필 이미지와 채팅 이미지의 서로 다른 생명주기를 효과적으로 관리하기 위해 테이블을 분리했습니다. 프로필 이미지는 수정이 가능하지만, 채팅 이미지는 수정이 불가능하다는 점에서 테이블을 분리하는게 좀 더 유리하다고 생각했습니다.추후 이미지 종류가 늘어나면 테이블도 늘어나겠지만, 각 이미지를 처리하는 방식이 다를 수 있기 때문에 도메인 별로 나누는 것이 더 이득이라고 생각합니다.
ImageFile의 objectKey 필드주목할만한 포인트는
ImageFile의 objectKey 필드입니다. DB에는 이미지 경로를 저장할 때 minio endpoint + bucket 정보는 제외한 objectKey 만을 저장합니다. (추후 minio 주소가 바뀌거나, bucket 이름이 바뀌는 것에 대비하기 위함입니다.)그런데 클라이언트에게 정보를 반환할 때는 (public) objectKey만 반환하게 되면 접속을 할 수 없습니다. 따라서 클라이언트에게 전달해줄 때는 minio endpoint + bucket 정보를 붙인 imageUrl을 반환해야 합니다.
따라서 이 로직을 이미지를 사용하는 코드에서 사용하도록 기존 코드를 수정했습니다.
채팅 이미지와 같은 private 한 이미지도 objectKey는 동일하게 저장하되, presigned url을 반환하도록 설계되어 있습니다. 자세한 로직은
AbstractImageManager#performUpload메서드에 구현되어 있습니다.프로필 이미지 수정 로직 개선
프로필 사진의 경우 Member 와 1대1 관계를 가지게 설계했습니다.
따라서 기존의 프로필 사진이 존재하는 경우, 기존에 minio에 저장했던 과거 프로필은 지우는 로직이 필요하다고 생각하여 추가했습니다. (과거 프로필을 볼 수 있게 하는 기능이 추후 추가된다면 삭제를 하면 안되겠습니다)
프로필 수정 API 분리
변경 전에는
PUT /profileAPI를 사용해 프로필 이미지 업데이트를 할 때String imageUrl을 받아서 갱신하였습니다.변경 후에는 API를 분리하였습니다.
PUT /profilePOST /files/image/profileTest -> CleanUp 수정
새로운 엔티티를 추가하고 테스트를 실행하니, CleanUp 로직이 문제가 생겼습니다. 기존 코드에서는 존재하는 엔티티 이름을 모두 가져와 truncate 하는 로직이었습니다.
그런데 새로 엔티티를 추가하고 CleanUp 할 경우, 아직 DB에는 테이블이 생기지 않았는데 truncate 하려고 하니 sqlException이 발생했습니다. 따라서 DB에 테이블이 존재하는 경우에만 truncate 하도록 수정했습니다.
✅ PR Point
AbstractImageManager와 템플릿 메서드 패턴을 적용하여 공통 로직을 분리했는데, 구조가 적절한지 피드백 부탁드립니다.ImageFile을 상속받는 형태로 테이블을 분리(MemberProfileImage,ChatImage)했습니다. 이 설계에 대한 의견이 궁금합니다.😭 어려웠던 점
AbstractFileManager, AbstractImageManager 을 만들면서 어떤 로직을 추상클래스에 두고, 어떤 로직을 구현체에 둬야 하는지 판단하는 일이 좀 어려웠습니다.