Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: S3 파일 업로드 베이스 코드 구현 #24
Feat: S3 파일 업로드 베이스 코드 구현 #24
Changes from 2 commits
3384dc8
424f6a7
f52893b
efeabe2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치를 검토한 결과, 아래와 같은 사항을 고려할 수 있습니다:
에러 코드 일관성:
S3_UPLOAD_FAIL_ERROR
의 에러 코드가 4000대인데, 주석에 "4000번대 코드 : 파일 s3 버킷 업/다운로드 관련"이라는 설명이 있습니다. 주석과 코드가 일치하므로 이 부분은 괜찮습니다.HTTP 상태 코드:
HttpStatus.INTERNAL_SERVER_ERROR
는 500번대 오류 코드로, 일반적으로 서버에서 처리할 수 없는 오류를 나타냅니다. 파일 업로드 실패는 클라이언트 요청에 의해 발생할 수 있으므로, 이 경우HttpStatus.BAD_REQUEST
(400) 또는HttpStatus.UNPROCESSABLE_ENTITY
(422)가 더 적절할 수 있습니다. 다시 한번 검토해 보시기 바랍니다.에러 메시지: 에러 메시지는 충분히 이해하기 쉬우며, 사용자에게 유용한 정보를 제공합니다. 하지만 다국어 지원이 필요한 경우, 메시지를 외부 파일로 관리하는 것이 좋습니다.
주석 추가: 새로운 enum 값을 추가할 때, 그 값의 의미를 더 구체적으로 설명하는 주석을 추가하는 것이 좋습니다. 이를 통해 코드의 가독성을 높이고, 팀원들이 이해하기 쉽게 만듭니다.
테스트 케이스: 새로운 에러 코드에 대해 기존의 테스트 케이스가 적절히 업데이트되었는지 확인해야 합니다. 이 코드를 사용하는 모든 경로에서 이 에러 코드가 올바르게 처리되는지 테스트할 필요가 있습니다.
요약하자면, 코드 자체는 큰 문제가 없어 보입니다. 그러나 HTTP 상태 코드의 적절성, 추가적인 주석 및 테스트 케이스 확인이 필요할 것으로 보입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 리뷰를 진행하겠습니다. 전반적으로 잘 작성된 코드로 보이며, 몇 가지 개선점과 버그 리스크를 지적해보겠습니다.
파일 크기 검증 주석 처리:
validateFileSize(file.getSize());
부분이 주석 처리되어 있습니다. 파일 크기 검증은 매우 중요하므로, 이 검증을 활성화해야 할 것 같습니다. 주석 처리된 이유가 있다면 그에 대한 주석을 추가하는 것도 좋겠습니다.확장자 추출 방식: 파일 확장자를 추출하는 부분에서,
fileName.substring(fileName.lastIndexOf("."))
는 파일 이름에 점이 없는 경우StringIndexOutOfBoundsException
을 발생시킬 수 있습니다. 이를 방지하기 위해 점이 존재하는지 확인한 후에 추출하는 방식이 안전할 것 같습니다.파일 사이즈 상수: 현재
MAX_FILE_SIZE
를 20MB로 설정하였으나, 주석에서는 10MB로 표기된 부분이 있어 주의가 필요합니다. 주석을 수정하거나 상수를 일관되게 사용하는 것이 필요합니다.예외 메시지 개선: 현재 예외 메시지는 유용하지만, 사용자가 문제를 더 쉽게 이해할 수 있도록 더 구체적이면 좋겠습니다. 예를 들어, "Invalid file type" 같은 메시지는 어떤 파일 타입이 허용되는지 추가하는 것이 좋습니다.
중복 코드: MIME 타입과 확장자 검증의 경우 특정 카테고리들이 중복되고 있습니다. 이를 메서드로 추출하여 중복을 줄이고 유지보수를 용이하게 할 수 있습니다.
메서드 접근 제어자: 모든 메서드가
private
로 되어 있는데, 필요한 경우public
이나protected
등으로 접근 제어자를 변경하는 것이 좋습니다.예외 처리: 현재의 예외 처리는 모두
IllegalArgumentException
을 사용하고 있는데, 다른 예외 타입을 사용하는 것도 고려해볼 수 있습니다. 예를 들어,FileValidationException
같은 커스텀 예외를 만들어 구체적인 상황을 명시하는 것이 좋습니다.이 외에도 문서화나 테스트 커버리지를 고려해보시면 좋을 것 같습니다. 전반적으로 잘 작성된 코드이나, 위의 포인트들을 참고하여 개선하시면 더욱 견고한 코드가 될 것입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 이 메소드에서는 "S3에 파일 저장" 과 "엔티티 생성", "DB에 엔티티 저장" 이렇게 3가지 일을 하고 있습니다.
엔티티 생성과 DB에 저장하는 것은 크게 한가지 책임으로 볼 수 있지만 S3에 파일을 저장하는건 다른 책임이라고 생각됩니다.
클래스 이름이
FileService
인 만큼 DB에 저장하는 로직은 다른 서비스로 분리 하는것을 추천 드립니다.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기능 구현 후 리팩토링 과정에서 기능별로 분리하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 리뷰 결과는 다음과 같습니다:
예외 처리: 현재
uploadFile
메소드에서 발생할 수 있는 모든 예외를 포괄적으로 처리하고 있습니다. 하지만 구체적인 예외를 구분하고 세분화할 필요가 있습니다. 예를 들어,FileValidator.validateFile()
에서 발생할 수 있는 예외와s3ClientService.uploadFile()
에서 발생할 수 있는 예외를 각각 다른 방법으로 처리하는 것도 고려해보세요.파일 메타데이터 생성 로직: 메타데이터 생성 시
writtenBy
,writtenAt
,createdBy
,createdAt
등의 값이 하드코딩되어 있습니다. 추후에 이 값들을 동적으로 설정할 수 있는 방법을 고려하는 것이 좋습니다. 예를 들어, 업로더 ID나 생성 일시를 인자로 받거나, AOP를 통해 자동으로 주입하는 방법도 가능합니다.파일 확장자 처리: 현재 파일 확장자를 추출하는 로직이 단순히 마지막
.
을 기준으로 하고 있습니다. 파일명이example
같은 경우에는 확장자를 얻지 못하므로, 이를 robust하게 처리할 수 있는 방법을 고민해보세요. 예를 들어, 파일명에 점이 없으면 확장자를 빈 문자열로 설정하는 방식도 생각해볼 수 있습니다.파일 크기 단위: 파일 크기를 KB로 변환할 때, 소수점 두 자리까지 출력하거나, 필요에 따라서 MB 또는 GB로 변환할 수 있는 유틸리티 메소드를 만드는 것을 추천합니다.
S3 삭제 로직: 파일 업로드 실패 시 S3에서 해당 파일을 삭제하는 로직이 포함되어 있습니다. 상황에 따라 이 파일이 필요할 수 있으므로, 로그를 통해 사용자에게 알리거나 실패한 원인에 따라 삭제 여부를 결정할 수 있는 기능을 추가하는 것도 필요합니다.
주석 및 TODO:
TODO
주석이 포함되어 있는 부분들은 향후 처리 사항을 명시하고 있지만, 별도의 이슈 트래커에 등록하여 체계적으로 관리하는 것이 좋습니다. 코드에서 TODO 항목을 지우지 말고, 그 내용을 효율적으로 관리할 수 있는 방법을 마련하는 것이 좋습니다.테스트 코드: 현재 코드에는 테스트 코드가 보이지 않으므로, 업로드 기능에 대한 단위 테스트를 작성해 검증하는 것이 좋습니다. 파일 검증, S3 업로드, 데이터베이스 저장 등의 모든 주요 기능에 대한 테스트 케이스를 포함하세요.
위의 방안들을 검토하여 개선할 수 있는 부분을 반영하면 코드를 더욱 안정적이고 유지보수하기 쉽게 만들 수 있습니다.