Update/ 단일 이미지 업로드 로직 변경 #231
Conversation
kdh10806
requested review from
Eseas,
taekminKwon,
K-KY,
HiSeungmin and
jeongns2611
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -19,6 +19,7 @@ public class CompanyFileController { | |||
|
|
|||
| /** | |||
| * 1. 회사 로고 이미지 업로드 | |||
| * 등록된 이미지가 있는 경우 새 이미지로 등록 된다. | |||
| * @param file 업로드할 이미지 파일 | |||
| * @return 업로드된 파일 메타데이터, 성공 메시지 | |||
| */ | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
코드 주석 추가: 주석에 "등록된 이미지가 있는 경우 새 이미지로 등록 된다."라는 문구를 추가하셨습니다. 이는 기능에 대한 중요한 정보를 제공하므로 좋습니다. 그러나 명확성을 위해 '기존 이미지의 덮어쓰기'라는 점을 더 강조하는 것이 좋을 것 같습니다.
-
예외 처리: 코드 전체에서 이미지 업로드와 관련된 예외 상황을 처리하고 있는지 확인할 필요가 있습니다. 예를 들어, 파일 형식이 올바르지 않거나 파일 크기가 너무 클 경우에 대한 처리가 없으면 런타임 오류가 발생할 수 있습니다.
-
메타데이터 반환: 업로드된 파일의 메타데이터를 반환하는 부분이 명확히 되어 있어야 합니다. 메타데이터에 어떤 정보가 포함되는지 문서화하는 것이 좋습니다. 예를 들어, 파일 이름, 크기, 타입 등이 포함될 수 있습니다.
-
유효성 검사: 업로드할 이미지 파일의 유효성을 검사하는 로직이 필요합니다. Null 체크나 파일 타입 확인 등의 검사를 포함시키는 것이 중요합니다.
-
단위 테스트: 이 기능을 위한 단위 테스트가 작성되어 있는지 확인해 보세요. 다양한 시나리오(예: 이미지가 없을 때, 이미 존재하는 이미지가 있을 때 등)를 고려하여 테스트 케이스를 추가하는 것이 좋습니다.
-
리소스 관리: 업로드 작업 후 파일 핸들을 적절하게 닫는지 확인해야 합니다. 자원 누수를 방지하기 위해 try-with-resources 구문을 사용하는 것이 좋습니다.
이 외에도 구체적인 코드 내용에 따라 추가적인 리뷰가 필요할 수 있습니다. 전체적인 코드 흐름과 맥락에 따라 이 내용을 보완하면 좋겠습니다.
| //3-4. 파일 이력 등록 | ||
| companyFileHistoryService.registerLogoImageHistory(uploadedFileEntity, uploaderId); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| //4. S3파일 업로드, 메타데이터 테이블 저장 |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다. 주어진 패치에서는 로고 이미지가 이미 존재할 경우, 기존 로고 이미지를 삭제하고 새로운 이미지를 업로드하는 로직이 추가되었습니다. 아래는 몇 가지 개선점과 잠재적인 버그 위험을 정리한 것입니다.
개선점 및 버그 위험
-
예외 처리:
fileHandler.deleteFile메서드가 실패할 경우, null이 반환될 수 있습니다. 이때companyFileHistoryService.deleteLogoImageHistory(deletedEntity, uploaderId)에서 NPE(NullPointerException)가 발생할 위험이 있습니다.deletedEntity가 null인지 확인한 후에 호출해야 합니다.
-
업로드 실패 처리:
- 로고 이미지 업로드가 실패할 경우 어떠한 처리가 이루어지지 않을 수 있습니다. 예를 들어, 파일 업로드가 성공하더라도 이력을 등록하는 과정에서 실패할 경우, 일부 작업이 성공하고 일부는 실패하여 일관성이 깨질 수 있습니다. 이를 해결하기 위해 트랜잭션 관리가 필요할 수 있습니다.
-
로직 분리:
- 로고 이미지를 삭제하고 새로운 이미지를 등록하는 로직이 하나의 메서드에 포함되어 있습니다. 이들 로직을 별도의 메서드로 분리하여 재사용성과 가독성을 높이는 것이 좋습니다.
-
주석 정리:
- 주석이 영어와 한국어가 혼합되어 사용되고 있습니다. 일관된 언어를 사용하는 것이 좋습니다. 또한, 주석이 어느 정도의 설명을 포함하고 있지만, 각 단계에서 어떤 예외 상황을 처리하는지도 명시하면 더 좋습니다.
-
이력 보관 정책:
- 회사 로고로 사용할 이전 이미지를 삭제하는 대신 이력에 남길 수 있는 정책을 고려해볼 수 있습니다. 이를 통해 과거 로고를 추적할 수 있는 방법을 마련할 수 있습니다.
요약
부가적인 예외 처리와 로직 분리를 통해 코드의 안정성과 가독성을 높이는 것을 추천합니다. 또한 로고 이미지 업로드 시 실패 처리와 관련된 부분에 대한 명시적인 로직도 필요합니다. 이렇게 개선하면 더 견고하고 유지보수가 용이한 코드가 될 것입니다.
| @@ -78,7 +78,7 @@ public ResponseEntity<APIResponse<SuccessCode>> deleteMember(@AuthenticationPrin | |||
|
|
|||
| /** | |||
| * 함수명 : uploadProfileImage | |||
| * 계정 프로필 이미지를 업로드합니다. | |||
| * 계정 프로필 이미지 업로드. 이미 등록되어 있는 경우 새 이미지로 교체된다. | |||
| * @auth admin, 해당 계정주 | |||
| * @param memberId 해당 회원 Id | |||
| * @return ResponseEntity<APIResponse<SuccessCode>> 성공 응답 객체 | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 다음과 같이 진행하겠습니다.
-
주석 개선: 주석에서 "프로필 이미지를 업로드합니다."라는 문장을 "프로필 이미지 업로드. 이미 등록되어 있는 경우 새 이미지로 교체된다."로 변경하였습니다. 이는 함수의 동작을 좀 더 명확하게 설명하고 있어 좋은 개선입니다.
-
권한 정보 명시: 주석에 "@auth admin, 해당 계정주"라고 명시되어 있는데, 이 부분에서 권한 관련 설명이 명확하게 되어 있어 좋습니다. 하지만 추가적으로 권한 부족 시 어떻게 처리되는지도 주석 처리하면 좋을 것 같습니다.
-
예외 처리 확인: 해당 함수에서 업로드 시 예외가 발생할 가능성이 있습니다. 예를 들어, 이미지 형식이 잘못되거나 업로드 크기가 초과하는 경우 등에서 예외가 발생할 수 있습니다. 코드Review시 예외 처리 부분에 대한 검토가 필요합니다.
-
메소드의 반환 타입: 반환 타입이
ResponseEntity<APIResponse<SuccessCode>>로 설정되어 있는데, 이 부분이 적절한 응답을 제공하는지 확인해야 합니다. 특히, 실패 시 어떤 정보를 리턴하는지도 고려해야 할 것입니다. -
입력 검증:
memberId에 대한 입력 검증이 포함되어 있는지 확인해야 합니다. 유효하지 않은 ID가 들어올 경우 처리 방법이나 에러 메시지에 대한 고민이 필요합니다. -
코드 일관성: 다른 메소드와의 코딩 스타일 일관성을 유지하고 있는지 한 번 더 점검해 보는 것이 좋습니다.
전체적으로는 주석 개선이 긍정적인 변화이며, 예외 처리와 입력 검증 부분에 대한 추가 검토가 필요할 것으로 보입니다.
| //3-4. 업로드 이력 등록 | ||
| memberFileHistoryService.registerProfileImageHistory(uploadedFileEntity, uploaderId); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| //4. S3 파일 업로드, 메타데이터 테이블 저장 |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
긍정적인 점
-
주석 추가: 기존 코드에 대한 설명과
uploadProfileImage함수의 동작에 대한 주석이 추가되었습니다. 이는 다른 개발자들이 코드를 이해하는 데 큰 도움이 될 것입니다. -
기존 파일 삭제 로직: 프로필 이미지가 이미 존재할 때, 기존 파일을 삭제하고 교체하는 로직이 명확히 구현되었습니다. 이미지 교체에 대한 처리가 이루어져 있어 좋은 개선입니다.
버그 위험
-
삭제할 파일의 존재 유무 확인 부족:
fileHandler.deleteFile메소드 호출 전에 삭제할 파일이 실제로 존재하는지 확인하는 로직이 없습니다. 삭제할 파일이 존재하지 않을 경우, 예외가 발생할 수 있으므로 사전에 확인이 필요합니다. -
파일 업로드 실패 처리:
fileHandler.uploadFile메소드가 실패할 경우에 대한 예외 처리가 없어 보입니다. 파일 업로드 중 문제가 발생할 경우, 이를 처리할 로직이 필요합니다. -
Race Condition: 멀티스레드 환경에서 두 개 이상의 요청이 동시에
uploadProfileImage를 호출할 경우, 두 요청 모두 기존 파일이 있다고 판단해 삭제 후 업로드를 시도할 수 있습니다. 이를 방지하기 위한 동기화 또는 잠금 메커니즘이 필요할 수 있습니다.
개선 제안
-
예외 처리 강화: 파일 삭제 및 업로드 시 발생할 수 있는 예외를 적절히 처리하는 로직을 추가하는 것이 좋습니다. 예외가 발생할 경우 이를 사용자에게 알리거나 롤백하는 방식이 필요합니다.
-
로깅 추가: 파일 삭제 및 업로드 과정에서 로깅을 추가하여, 문제 발생 시 디버깅이 용이하도록 하는 것이 좋습니다.
-
중복 코드 방지: 제거된 파일의 업로드와 관련된 코드가 중복될 경우, 코드 유지보수를 쉽게 하기 위해 공통 메소드를 만들어 사용할 수 있습니다.
-
단위 테스트 추가: 기존 이미지가 존재할 때, 존재하지 않을 때 각각의 경우에 대해 단위 테스트를 추가하여 코드의 안정성을 높이는 것이 좋습니다.
이러한 점들을 고려하여 개선하면 더욱 안전하고 유지보수가 용이한 코드가 될 것입니다.
| if(fileMetadataRepository.existsByCategoryAndReferenceId(FileCategory.COMPANY_LOGO_IMAGE, companyEntity.getId())){ | ||
| throw new BusinessException(ErrorCode.LOGO_ALREADY_EXIST_ERROR); | ||
| //기존 로고 이미지 파일을 삭제한다. | ||
| //3-1. 삭제수행 | ||
| FileMetadata deletedEntity = fileHandler.deleteFile(FileCategory.COMPANY_LOGO_IMAGE, companyEntity.getId()); | ||
| //3-2. 삭제 이력 등록 | ||
| companyFileHistoryService.deleteLogoImageHistory(deletedEntity, uploaderId); | ||
|
|
||
| //새 로고 이미지를 등록한다. | ||
| //3-3. S3파일 업로드, 메타데이터 테이블 저장 | ||
| FileMetadata uploadedFileEntity = fileHandler.uploadFile(file, FileCategory.COMPANY_LOGO_IMAGE, companyEntity.getId()); | ||
| //3-4. 파일 이력 등록 | ||
| companyFileHistoryService.registerLogoImageHistory(uploadedFileEntity, uploaderId); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
로고 삭제하는 로직을 따로 뺴고 먼저 실행하면 코드 중복이 감소 될 것 같아요
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
단일 이미지 업로드 로직 변경
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
🛠️ 쓰이는 모듈
💬 기타 질문 및 특이 사항