Conversation
There was a problem hiding this comment.
총평
전반적으로 계층 구조와 Swagger 문서화가 잘 설계되어 있으나, 런타임에 치명적 영향을 주는 버그가 발견되어 수정이 필요합니다.
P1. file-data-map/*.ser, out/ 디렉토리가 Git에 커밋됨
직렬화된 .ser 파일(수십 개)과 빌드 산출물(out/)이 Git에 포함되어 있습니다. .gitignore에 추가하고 git rm --cached로 tracking을 제거해 주세요. discodeit.iml IDE 파일도 마찬가지입니다.
P4. UserResponse.java 패키지 위치
UserResponse가 dto.request 패키지에 위치해 있습니다. 응답 DTO이므로 dto.response 패키지로 이동을 권장합니다.
P5. 주석 처리된 코드 정리
ChannelController 등에 주석 처리된 코드 블록이 남아 있습니다. 불필요한 코드는 삭제하고, 필요시 Git 히스토리에서 복구하는 것을 권장합니다.
| @Override | ||
| public void deleteByUserId(UUID userId) { | ||
| this.findByUserId(userId) | ||
| .ifPresent(userStatus -> this.deleteByUserId(userStatus.getId())); |
There was a problem hiding this comment.
P1. 무한 재귀 — StackOverflowError 발생
deleteByUserId() 내부에서 this.deleteById()를 호출해야 하는데 this.deleteByUserId()를 재귀 호출하고 있습니다.
이 메서드가 호출되면 무조건 StackOverflowError가 발생합니다.
| .ifPresent(userStatus -> this.deleteByUserId(userStatus.getId())); | |
| .ifPresent(userStatus -> this.deleteById(userStatus.getId())); |
| BinaryContent binaryContentNullable = null; | ||
| Path path = resolvePath(id); | ||
| ReentrantLock lock = fileLockProvider.getLock(path); | ||
| lock.lock(); |
There was a problem hiding this comment.
P1. 데드락 위험 — 파일 미존재 시 lock 미해제
lock.lock() 이후 Files.exists(path) 조건이 false일 때 lock.unlock()이 호출되지 않습니다.
이 패턴이 모든 File*Repository(FileChannelRepository, FileMessageRepository, FileReadStatusRepository, FileUserRepository, FileUserStatusRepository)의 findById() 메서드에 동일하게 존재합니다.
try-finally를 if 블록 바깥으로 이동해야 합니다:
lock.lock();
try {
if (Files.exists(path)) {
// read logic
}
} finally {
lock.unlock();
}| .orElseThrow( | ||
| () -> new NoSuchElementException("User with username " + username + " not found")); | ||
|
|
||
| if (!user.getPassword().equals(password)) { |
There was a problem hiding this comment.
P2. 비밀번호 평문 비교 및 API 노출
비밀번호를 평문으로 비교하고 있으며, login() 반환값인 User 엔티티가 LoginController에서 그대로 API 응답으로 반환되어 비밀번호가 클라이언트에 노출됩니다.
최소한:
- 응답은
UserResponseDTO로 변환해서 반환 - 향후
PasswordEncoder도입을 권장합니다.
|
|
||
| String newUsername = userUpdateRequest.newUsername(); | ||
| String newEmail = userUpdateRequest.newEmail(); | ||
| if (userRepository.existsByEmail(newEmail)) { |
There was a problem hiding this comment.
P2. update() 유니크 체크에서 자기 자신 미제외
email/username 중복 체크가 현재 사용자를 제외하지 않습니다. email이나 username을 변경하지 않고 다른 필드만 수정하려 해도 중복 오류가 발생합니다.
// 예시: 자기 자신 제외
if (!user.getEmail().equals(newEmail) && userRepository.existsByEmail(newEmail)) {
throw new IllegalArgumentException("User with email " + newEmail + " already exists");
}| } | ||
|
|
||
| @ExceptionHandler(Exception.class) | ||
| public ResponseEntity<String> handlerException(Exception e) { |
There was a problem hiding this comment.
[P3] Exception catch-all이 400 BAD_REQUEST 반환
예상치 못한 서버 예외(NPE, DB 오류 등)까지 400으로 반환하면 클라이언트가 원인을 오해할 수 있습니다.
| public ResponseEntity<String> handlerException(Exception e) { | |
| public ResponseEntity<String> handlerException(Exception e) { | |
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(e.getMessage()); |
|
|
||
| @Override | ||
| @PatchMapping("/{userId}/userStatus") | ||
| public ResponseEntity<UserStatus> StatusUpdateByUserId( |
There was a problem hiding this comment.
[P4] 메서드/파라미터 네이밍 컨벤션 위반
메서드명 StatusUpdateByUserId가 대문자로 시작하며, 파라미터 Request도 대문자입니다.
Java 컨벤션에 따라 statusUpdateByUserId, request로 변경을 권장합니다.
2 participants
요구사항
기본
심화
주요 변경사항
멘토에게