Conversation
- BinaryContent 메타데이터 구조로 변경 - 파일 다운로드 엔드포인트 추가 - StorageException 적용
page 공통 응답, 요청 dto 추가 message 목록 조회에 Pagination 적용 ChannelMessageController를 MessageController에 병합
불필요한 join, leave 메서드 제거 readstatus 생성시 channel의 생성 시점을 주입하도록 변경
MessageMapper의 Mapper uses에 BinaryMapper 추가
* refactor: page dto를 커서 기반으로 리팩토링 * refactor: JPQL 추가 * refactor: controller, service에 cursor 적용
joonfluence
left a comment
joonfluence
left a comment
There was a problem hiding this comment.
PR 리뷰 요약
File/JCF 기반 저장소를 Spring Data JPA + PostgreSQL로 마이그레이션하고, MapStruct를 활용한 DTO/Mapper 패턴, 커서 기반 페이지네이션, feature 기반 패키지 구조를 적용한 PR입니다. 전반적으로 JPA 엔티티 설계와 서비스 계층 구조가 잘 잡혀 있으나, N+1 문제와 일부 설계 개선이 필요합니다.
| } | ||
| } | ||
| @Mapping(target = "lastMessageAt", expression = "java(getLastMessageAt(channel.getId()))") | ||
| @Mapping(target = "participants", expression = "java(getParticipants(channel.getId()))") |
There was a problem hiding this comment.
[P2] toDto()가 호출될 때마다 messageRepository와 readStatusRepository를 각각 호출합니다. 채널 목록을 조회하면 채널 수 × 2만큼 추가 쿼리가 발생하는 N+1 문제입니다.
Mapper는 단순 변환 역할만 맡기고, lastMessageAt과 participants는 Service 계층에서 JOIN FETCH나 별도 배치 쿼리로 미리 조회한 뒤 전달하는 것이 좋습니다.
| return channelRepository.findAll() | ||
| .stream() | ||
| .filter(channel -> channel.getType() == ChannelType.PUBLIC | ||
| || readStatusRepository.existsByUserIdAndChannelId(userId, channel.getId())) |
There was a problem hiding this comment.
[P2] 전체 채널을 로드한 후 PRIVATE 채널마다 existsByUserIdAndChannelId()를 호출하므로 채널 수만큼 쿼리가 발생합니다.
JPQL로 SELECT c FROM Channel c WHERE c.type = 'PUBLIC' OR EXISTS (SELECT rs FROM ReadStatus rs WHERE rs.user.id = :userId AND rs.channel.id = c.id) 같은 단일 쿼리를 사용하세요.
| public List<UserDto> findAllByChannelId(UUID channelId) { | ||
| return userRepository.findAll() | ||
| .stream() | ||
| .filter(user -> readStatusRepository.existsByUserIdAndChannelId(user.getId(), channelId)) |
There was a problem hiding this comment.
[P2] 위 BasicChannelService.findAllByUserId와 동일한 패턴입니다. 전체 User를 로드한 후 유저마다 existsByUserIdAndChannelId()를 호출합니다.
ReadStatusRepository에 findAllByChannelId → User 목록을 반환하는 JPQL 쿼리를 만들거나, JOIN 기반 쿼리로 대체해 주세요.
| userStatus.getId() | ||
| ); | ||
| } | ||
| @Mapping(target = "online", expression = "java(user.getUserStatus().isOnline())") |
There was a problem hiding this comment.
[P2] User 엔티티에서 userStatus는 mappedBy 관계로 선언되어 있어 null일 수 있습니다 (예: 방금 생성된 사용자, 데이터 불일치 등).
user.getUserStatus() != null && user.getUserStatus().isOnline() 같은 null-safe 처리가 필요합니다.
| public UserDto login(LoginRequest loginRequest) { | ||
| User findUser = userRepository.findByUsername(loginRequest.username()) | ||
| .orElseThrow(UserNotFoundException::new); | ||
| if (!findUser.getPassword().equals(loginRequest.password())) { |
There was a problem hiding this comment.
[P2] 비밀번호를 평문으로 비교하고 있습니다. 현재 스프린트 범위가 아닐 수 있지만, 향후 PasswordEncoder(BCrypt 등)를 도입하여 해싱된 비밀번호와 비교하는 방식으로 전환을 고려해 주세요.
| public int hashCode() { | ||
| return id.hashCode(); | ||
| } | ||
| public BinaryContent(String fileName, Long size, String contentType, byte[] bytes) { |
There was a problem hiding this comment.
[P3] bytes 파라미터를 받지만 필드에 저장하지 않고 완전히 무시합니다. 엔티티에서 bytes 필드를 제거한 것은 좋은데, 생성자 시그니처에서도 제거하여 혼동을 방지해 주세요.
| public BinaryContent(String fileName, Long size, String contentType, byte[] bytes) { | |
| public BinaryContent(String fileName, Long size, String contentType) { |
| return profileId != null; | ||
| } | ||
| @Setter | ||
| @NoArgsConstructor |
There was a problem hiding this comment.
[P3] @Setter를 클래스 레벨에 적용하면 password, username, email 등 모든 필드가 외부에서 자유롭게 변경 가능해집니다. JPA 엔티티에서는 꼭 필요한 필드(예: profile, userStatus)에만 @Setter를 개별 적용하고, 나머지는 비즈니스 메서드로 변경을 제어하는 것이 안전합니다.
또한 @NoArgsConstructor에 access = AccessLevel.PROTECTED를 추가하여 외부에서 빈 객체 생성을 방지하세요.
|
|
||
| public void update(String newContent) { | ||
| this.content = newContent; | ||
| this.updatedAt = Instant.now(); |
There was a problem hiding this comment.
[P3] BaseUpdatableEntity에서 @LastModifiedDate로 updatedAt을 관리하고 있으므로, update() 메서드에서 수동으로 설정하면 중복입니다. JPA Auditing이 트랜잭션 커밋 시 자동으로 갱신해 주므로 수동 설정을 제거하세요. (ReadStatus.update()도 동일)
| @@ -0,0 +1,73 @@ | |||
| CREATE TABLE users | |||
There was a problem hiding this comment.
[P3] IF NOT EXISTS가 없어 애플리케이션 재시작 시 이미 테이블이 있으면 에러가 발생합니다. application.yaml에 sql.init.mode 설정이 없으므로, ddl-auto: none과 함께 사용할 경우를 대비해 방어적으로 CREATE TABLE IF NOT EXISTS를 추가하는 것이 좋습니다.
| ADD CONSTRAINT fk_users_binary_contents FOREIGN KEY (profile_id) | ||
| REFERENCES binary_contents (id) ON DELETE SET NULL; | ||
| ALTER TABLE binary_contents | ||
| DROP COLUMN bytes; No newline at end of file |
There was a problem hiding this comment.
[P4] binary_contents 테이블에 bytes bytea NOT NULL 컬럼을 만든 뒤 마지막에 DROP COLUMN으로 제거하고 있습니다. 처음부터 bytes 컬럼 없이 테이블을 정의하면 DDL이 더 깔끔합니다.
2 participants
기본 요구사항
API 명세
데이터베이스
아래와 같이 데이터베이스 환경을 설정하세요.
discodeitdiscodeit_userdiscodeit1234아래와 같이 데이터베이스 환경을 설정하세요.
Spring Data JPA 적용하기
application.yaml파일에 작성하세요.application.yaml파일에 작성하세요.Spring Data JPA 적용하기
com.sprint.mission.discodeit.entity.basecreatedAt,updatedAt속성이 자동으로 설정되도록 구현하세요.@CreatedDate,@LastModifiedDate@Entity,@Table@Column,@Enumerated@OneToMany,@OneToOne,@ManyToOne@JoinColumn,@JoinTablecascade,orphanRemoval레포지토리와 서비스에 JPA 도입하기
트랜잭션,영속성 전이,변경 감지,지연로딩DTO 적극 도입하기
Entity를 Controller 까지 그대로 노출했을 때 발생할 수 있는 문제점에 대해 정리해보세요. DTO를 적극 도입했을 때 보일러플레이트 코드가 많아지지만, 그럼에도 불구하고 어떤 이점이 있는지 알 수 있을거에요.(이 내용은 PR에 첨부해주세요.)
또한 사용자 엔티티에서 비밀번호와 같은 민감한 정보가 그대로 응답에 담기는 문제도 발생할 수 있습니다.
그리고 또 응답 시 엔티티 객체가 그대로 반환된다면 양방향 연관관계에서 순환 참조가 발생할 수 있습니다. 두 객체가 서로를 참조하고 있는 양방향 관계의 경우 한 객체를 반환하면 그 객체가 참조하고 있는 다른 객체도 반환하게 되고 이 과정이 반복되는 문제가 발생합니다.
다음의 클래스 다이어그램을 참고하여 DTO를 정의하세요.
com.sprint.mission.discodeit.mapperBinaryContent 저장 로직 고도화
데이터베이스에 이미지와 같은 파일을 저장하면 성능 상 불리한 점이 많습니다. 따라서 실제 바이너리 데이터는 별도의 공간에 저장하고, 데이터베이스에는 바이너리 데이터에 대한 메타 정보(파일명, 크기, 유형 등)만 저장하는 것이 좋습니다.
BinaryContent 엔티티는 파일의 메타 정보(
fileName,size,contentType)만 표현하도록bytes속성을 제거하세요.BinaryContent의
byte[]데이터 저장을 담당하는 인터페이스를 설계하세요.com.sprint.mission.discodeit.storageUUID put(UUID, byte[])byte[]데이터를 저장합니다.InputStream get(UUID)byte[]데이터를 읽어 InputStream 타입으로 반환합니다.ResponseEntity<?> download(BinaryContentDto)서비스 레이어에서 기존에 BinaryContent를 저장하던 로직을 BinaryContentStorage를 활용하도록 리팩토링하세요.
BinaryContentController에 파일을 다운로드하는 API를 추가하고, BinaryContentStorage에 로직을 위임하세요.
/api/binaryContents/{binaryContentId}/downloadResponseEntity<?>로컬 디스크 저장 방식으로 BinaryContentStorage 구현체를 구현하세요.
discodeit.storage.type값이local인 경우에만 Bean으로 등록되어야 합니다.Path rootdiscodeit.storage.local.root-path설정값을 정의하고, 이 값을 통해 주입합니다.void init()Path resolvePath(UUID){root}/{UUID}put,get메소드에서 호출해 일관된 파일 경로 규칙을 유지합니다.ResponseEntity<Resource> donwload(BinaryContentDto)ResponseEntity<Resource>응답을 생성 후 반환합니다.페이징과 정렬
메시지 목록을 조회할 때 다음의 조건에 따라 페이지네이션 처리를 해보세요.
일관된 페이지네이션 응답을 위해 제네릭을 활용해 DTO로 구현하세요.
com.sprint.mission.discodeit.dto.responsecontent: 실제 데이터입니다.number: 페이지 번호입니다.size: 페이지의 크기입니다.totalElements: T 데이터의 총 갯수를 의미하며, null일 수 있습니다.Slice 또는 Page 객체로부터 DTO를 생성하는 Mapper를 구현하세요.
com.sprint.mission.discodeit.mapper심화 요구사항
N+1 문제
읽기전용 트랜잭션 활용
페이지네이션 최적화
오프셋 페이지네이션과 커서 페이지네이션 방식의 차이에 대해 정리해보세요.
커서 페이지네이션 방식은 커서 값을 두어 마지막으로 조회한 데이터 값을 기준으로 페이징하는 방식입니다. 현재 프로젝트의 created_at을 기준으로 클라이언트가 커서로 마지막으로 읽은 페이지의 created_at 값을 지정하여 요청하면 created_at 값을 기준으로 조회된 값들을 커서 기반으로 조회합니다. OFFSET으로 인해 발생하는 성능 저하가 없지만, 커서 값이 unique하지 않으면 데이터 누락 문제가 생길 수 있습니다.
기존에 구현한 오프셋 페이지네이션을 커서 페이지네이션으로 리팩토링하세요.
MapStruct 적용