구민주 Sprint6#141
Conversation
There was a problem hiding this comment.
전체 요약
Spring Boot + JPA로의 전면 마이그레이션이 잘 수행되었습니다. 계층 분리, MapStruct 활용, 커서 기반 페이징 등 구조적으로 좋은 설계입니다.
다만 비밀번호 업데이트 시 암호화 누락(P1)과 채널 삭제 시 ReadStatus FK 제약 위반(P1) 이슈가 있어 수정이 필요합니다.
라인 코멘트에 포함하지 못한 항목
P4. .gitignore 중복 항목: 루트 .gitignore와 discodeit/.gitignore 모두에서 .idea/, *.iml, out/, build/ 항목이 파일 내에서 두 번 선언되어 있습니다. 정리하면 관리가 편해집니다.
P5. BaseEntity.java 포매팅: 모든 import, 어노테이션, 필드 사이에 빈 줄이 삽입되어 있어 일반적인 Java 컨벤션과 다릅니다. 정리를 권장합니다.
|
|
||
| @Override | ||
| public List<User> findAll() { return userRepository.findAll(); } | ||
| user.update(request.newUsername(), request.newEmail(), request.newPassword(), newProfile); |
There was a problem hiding this comment.
P1. 비밀번호 업데이트 시 암호화 누락 🔴
회원가입(create) 시에는 passwordEncoder.encode()로 암호화하여 저장하지만, update() 시에는 request.newPassword()를 평문 그대로 전달하고 있습니다.
이후 로그인 시 passwordEncoder.matches()로 비교하므로, 비밀번호 변경 후 로그인이 불가능해지는 버그가 발생합니다.
| user.update(request.newUsername(), request.newEmail(), request.newPassword(), newProfile); | |
| String encodedPassword = request.newPassword() != null | |
| ? passwordEncoder.encode(request.newPassword()) | |
| : null; | |
| user.update(request.newUsername(), request.newEmail(), encodedPassword, newProfile); |
| @Override | ||
| @Transactional | ||
| public void delete(UUID channelId) { | ||
| channelRepository.deleteById(channelId); |
There was a problem hiding this comment.
P1. 채널 삭제 시 ReadStatus 미정리로 FK 제약 위반 🔴
ReadStatus 엔티티가 channel_id를 FK로 참조하고 있지만, Channel 엔티티에서 ReadStatus로의 cascade 설정이 없습니다.
채널 삭제 시 read_statuses 테이블의 FK 제약 조건 위반으로 DataIntegrityViolationException이 발생합니다.
ReadStatusRepository.deleteAllByChannelId()를 먼저 호출해야 합니다.
| channelRepository.deleteById(channelId); | |
| readStatusRepository.deleteAllByChannelId(channelId); | |
| channelRepository.deleteById(channelId); |
| @JoinColumn(name = "channel_id", nullable = false) | ||
| private Channel channel; | ||
|
|
||
| @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE, CascadeType.REMOVE}) |
There was a problem hiding this comment.
P2. @manytomany에 CascadeType.REMOVE 사용 주의
@ManyToMany 관계에서 CascadeType.REMOVE는 위험합니다. 메시지 삭제 시 연결된 BinaryContent가 다른 곳에서도 참조될 수 있는데, cascade로 인해 의도치 않게 삭제될 수 있습니다.
현재 BasicMessageService.delete()에서 이미 수동으로 첨부파일을 삭제하고 있으므로, cascade에서 REMOVE를 제거하는 것이 안전합니다.
| @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE, CascadeType.REMOVE}) | |
| @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}) |
|
|
||
| void delete(UUID id); | ||
|
|
||
| ResponseEntity<Resource> download(UUID binaryContentId); |
There was a problem hiding this comment.
P2. Service 인터페이스에서 ResponseEntity 반환
서비스 레이어가 ResponseEntity를 반환하면 HTTP 관심사가 비즈니스 레이어에 침투합니다.
서비스는 Resource(또는 InputStream + 메타데이터)만 반환하고, ResponseEntity 구성은 컨트롤러에서 담당하는 것이 계층 분리 원칙에 부합합니다.
현재 동작에는 문제 없으나, 테스트 작성이나 다른 클라이언트(배치 등)에서 재사용하기 어려워질 수 있습니다.
| log.error("서버 내부 오류 발생", e); | ||
|
|
||
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) | ||
| .body("서버 내부 오류가 발생했습니다: " + e.getMessage()); |
There was a problem hiding this comment.
P3. 500 에러 시 내부 예외 메시지 노출
e.getMessage()를 클라이언트에 직접 반환하면 내부 구현 정보(SQL 에러, 스택 정보 등)가 노출될 수 있습니다.
프로덕션 환경에서는 고정 메시지만 반환하고, 상세 정보는 로그(log.error)에만 남기는 것이 좋습니다.
| .body("서버 내부 오류가 발생했습니다: " + e.getMessage()); | |
| .body("서버 내부 오류가 발생했습니다."); |
| @Query("SELECT DISTINCT m FROM Message m " + | ||
| "JOIN FETCH m.author " + | ||
| "WHERE m.channel.id = :channelId " + | ||
| "AND (:lastMessageId IS NULL OR m.createdAt < (SELECT m2.createdAt FROM Message m2 WHERE m2.id = :lastMessageId)) " |
There was a problem hiding this comment.
P3. 커서 기반 페이징에서 createdAt 동일 시 메시지 누락 가능
createdAt이 동일한 메시지가 여러 개일 경우(같은 밀리초에 생성), 일부 메시지가 누락될 수 있습니다.
createdAt과 id를 복합 조건으로 사용하면 정확한 커서 기반 페이징이 가능합니다:
AND (:lastMessageId IS NULL OR
(m.createdAt < (SELECT m2.createdAt FROM Message m2 WHERE m2.id = :lastMessageId))
OR (m.createdAt = (SELECT m2.createdAt FROM Message m2 WHERE m2.id = :lastMessageId) AND m.id < :lastMessageId))현재 트래픽 수준에서는 문제가 없을 수 있지만, 질문 2에서 언급하신 대로 데이터가 방대해질 경우를 대비하면 좋습니다.
2 participants
요구사항
기본
심화
엔티티 연관 관계 매핑 정리
주요 변경사항
1. 5차 과제 코드 리뷰 피드백 반영
2. 6차 과제 기술적 특이점
3. 성능 최적화 및 페이지네이션
멘토에게