Conversation
요약MongoDB 기반의 Dropping, Playlist 엔티티와 관련 서비스를 JPA/Hibernate 기반 관계형 데이터베이스(PostgreSQL)로 마이그레이션하고, 모든 ID 타입을 String에서 Long으로 변경했습니다. 또한 docker-compose에서 MongoDB를 Elasticsearch로 교체했습니다. 변경사항
예상 코드 리뷰 난이도🎯 4 (복잡함) | ⏱️ ~60분 관련 PR
제안 리뷰어
시
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/org/example/remedy/domain/comment/application/service/CommentService.java`:
- Around line 118-124: parseDroppingId currently calls Long.valueOf(droppingId)
which throws NPE for null and bypasses the NumberFormatException mapping; update
parseDroppingId to first check for null or blank (e.g., droppingId == null ||
droppingId.trim().isEmpty()) and throw DroppingNotFoundException.EXCEPTION for
those cases, and keep the existing try/catch for NumberFormatException to also
throw DroppingNotFoundException.EXCEPTION.
In `@src/main/java/org/example/remedy/domain/playlist/domain/Playlist.java`:
- Around line 26-28: The songIds JSONB field can be null when loading existing
rows causing NPEs in methods like addSong, removeSong, hasSong, and
getSongIdList; fix by ensuring songIds is initialized after JPA loads the
entity—either make the field default to an empty list (private List<String>
songIds = new ArrayList<>();) or add a `@PostLoad` lifecycle method in Playlist
(e.g., postLoad()) that checks if songIds is null and sets it to new
ArrayList<>(), so all usages of songIds in
addSong/removeSong/hasSong/getSongIdList are safe.
🧹 Nitpick comments (10)
docker-compose.yaml (1)
65-78: Elasticsearch JVM 메모리 설정 및 헬스체크 추가 권장Elasticsearch 8.x는 메모리 사용량이 많아 명시적인 JVM 힙 설정 없이는 리소스 제한 환경에서 OOM 문제가 발생할 수 있습니다. 또한,
service_started조건은 Elasticsearch가 실제로 요청을 받을 준비가 되었는지 보장하지 않으므로, 헬스체크 추가를 권장합니다.♻️ 권장 설정 추가
elasticsearch: image: docker.elastic.co/elasticsearch/elasticsearch:8.11.0 container_name: elasticsearch environment: - discovery.type=single-node - xpack.security.enabled=true - ELASTIC_PASSWORD=${ELASTIC_PASSWORD} + - ES_JAVA_OPTS=-Xms512m -Xmx512m ports: - "9200:9200" - "9300:9300" volumes: - elasticsearch_data:/usr/share/elasticsearch/data networks: - remedy-network + healthcheck: + test: ["CMD-SHELL", "curl -s -u elastic:${ELASTIC_PASSWORD} http://localhost:9200/_cluster/health | grep -q '\"status\":\"green\"\\|\"status\":\"yellow\"'"] + interval: 30s + timeout: 10s + retries: 5spring-boot 서비스의 depends_on도 함께 업데이트:
elasticsearch: - condition: service_started + condition: service_healthysrc/main/java/org/example/remedy/domain/dropping/application/dto/request/DroppingCreateRequest.java (1)
21-23: playlistId 양수 제약 추가를 권장합니다Line 22의
playlistId가 Long으로 바뀌면서 0/음수 값이 유입될 수 있습니다. 선택 값이라도 유효한 ID 범위를 보장하려면@Positive같은 제약을 추가하는 편이 안전합니다.♻️ 제안 변경
- Long playlistId, + `@Positive`(message = "playlistId는 양수여야 합니다") + Long playlistId,src/main/java/org/example/remedy/domain/like/application/dto/request/LikeRequest.java (1)
3-6: 유효성 강화가 필요하면 양수 검증도 고려해 주세요.
현재@NotNull만으로는0/음수를 막지 못합니다. 도메인에서 음수가 불가라면@Positive또는@Min(1) 추가를 고려해볼 수 있습니다.src/main/java/org/example/remedy/domain/dropping/application/service/DroppingServiceFacade.java (1)
33-41: switch 문의 일관성 확인 필요
createDropping메서드(Line 21-27)에서는default케이스를 통해InvalidDroppingTypeException을 던지고 있지만,getDropping메서드의 switch 표현식(Line 36-40)에는default케이스가 없습니다.Java의 switch 표현식은 enum이 exhaustive할 경우 컴파일러가 이를 처리하지만, 향후 새로운
DroppingType이 추가될 경우 런타임 오류가 발생할 수 있습니다. 일관성을 위해default케이스 추가를 권장합니다.♻️ 제안된 수정
return switch (dropping.getDroppingType()) { case MUSIC -> musicDroppingService.getMusicDropping(droppingId); case VOTE -> voteDroppingService.getVoteDropping(droppingId, userId); case PLAYLIST -> playlistDroppingService.getPlaylistDropping(droppingId); + default -> throw InvalidDroppingTypeException.EXCEPTION; };src/main/java/org/example/remedy/domain/playlist/application/service/PlaylistService.java (1)
51-62: N+1 쿼리 최적화 고려
getPlaylist메서드에서 각songId마다 개별 DB 조회가 발생하여 N+1 쿼리 문제가 있습니다.getMyPlaylists의createAlbumImageMap메서드(Line 71-79)처럼findAllById를 사용한 배치 조회로 개선할 수 있습니다.♻️ 제안된 수정
public PlaylistDetailResponse getPlaylist(Long playlistId) { Playlist playlist = playlistRepository.findById(playlistId) .orElseThrow(() -> PlaylistNotFoundException.EXCEPTION); - List<SongResponse> songs = playlist.getSongIdList().stream() - .map(songId -> songRepository.findById(songId) - .map(SongMapper::toSongResponse) - .orElseThrow(() -> SongNotFoundException.EXCEPTION)) - .collect(Collectors.toList()); + List<String> songIdList = playlist.getSongIdList(); + List<Song> foundSongs = songRepository.findAllById(songIdList); + + if (foundSongs.size() != songIdList.size()) { + throw SongNotFoundException.EXCEPTION; + } + + Map<String, Song> songMap = foundSongs.stream() + .collect(Collectors.toMap(Song::getId, song -> song)); + + List<SongResponse> songs = songIdList.stream() + .map(songMap::get) + .map(SongMapper::toSongResponse) + .collect(Collectors.toList()); return PlaylistMapper.toPlaylistDetailResponse(playlist, songs); }src/main/java/org/example/remedy/domain/playlist/domain/Playlist.java (1)
45-48: songId null 체크 추가 권장
addSong메서드에서songId가null인 경우에 대한 방어 코드가 없습니다.null이 전달되면 리스트에null이 추가될 수 있습니다.♻️ 제안된 수정
public void addSong(String songId) { + if (songId == null) return; if (hasSong(songId)) return; this.songIds.add(songId); }src/main/java/org/example/remedy/domain/dropping/repository/DroppingRepositoryImpl.java (2)
15-24:@PersistenceContext와@RequiredArgsConstructor혼용 주의
@RequiredArgsConstructor는final필드에 대한 생성자를 생성하지만,@PersistenceContext로 주입되는EntityManager는final이 아니므로 생성자 주입 대상에서 제외됩니다. 이는 의도된 동작이지만, 일관성을 위해 명시적으로 주석을 추가하거나EntityManager도 생성자 주입 방식으로 통일하는 것을 고려해 주세요.
86-117:LocalDateTime.now()외부 주입 권장Line 111에서
LocalDateTime.now()를 쿼리 내부에서 직접 호출하고 있습니다. 이는 테스트 시 시간 제어가 어렵고, 메서드 호출 시점과 쿼리 실행 시점 사이에 미세한 시간 차이가 발생할 수 있습니다. 파라미터로LocalDateTime을 받거나Clock을 주입받는 방식을 고려해 주세요.♻️ 제안된 수정
- private List<Dropping> findDroppingsByAroundRadius(double longitude, double latitude, double distance) { + private List<Dropping> findDroppingsByAroundRadius(double longitude, double latitude, double distance, LocalDateTime now) { // 대략적인 위도/경도 범위 계산 (1도 ≈ 111km) double latOffset = distance / 111.0; double lonOffset = distance / (111.0 * Math.cos(Math.toRadians(latitude))); double minLat = latitude - latOffset; double maxLat = latitude + latOffset; double minLon = longitude - lonOffset; double maxLon = longitude + lonOffset; // JPQL을 사용한 범위 검색 String jpql = """ SELECT d FROM Dropping d WHERE d.isDeleted = false AND d.expiryDate > :now AND d.latitude BETWEEN :minLat AND :maxLat AND d.longitude BETWEEN :minLon AND :maxLon """; return entityManager.createQuery(jpql, Dropping.class) - .setParameter("now", LocalDateTime.now()) + .setParameter("now", now) .setParameter("minLat", minLat) .setParameter("maxLat", maxLat) .setParameter("minLon", minLon) .setParameter("maxLon", maxLon) .getResultList(); }호출부도 함께 수정:
public void createDropping(Dropping dropping) { List<Dropping> nearbyDroppings = findDroppingsByAroundRadius( dropping.getLongitude(), dropping.getLatitude(), - DROPPING_CONSTRAINT_DISTANCE + DROPPING_CONSTRAINT_DISTANCE, + LocalDateTime.now() ); // ... }src/main/java/org/example/remedy/domain/dropping/domain/Dropping.java (2)
34-34:content필드에@Column어노테이션 추가를 고려해주세요.다른 필드들은 모두 명시적으로
@Column을 지정했는데,content만 빠져있어 일관성이 부족합니다. 의도적으로 nullable을 허용하는 것이라면 주석으로 명시하거나, 다른 필드들과 동일하게 어노테이션을 추가하면 코드 가독성이 향상됩니다.♻️ 일관성을 위한 수정 제안
+ `@Column`(name = "content") private String content;
54-67: 생성자 파라미터가 많아 Builder 패턴 적용을 고려해보세요.10개의 파라미터를 받는 생성자는 호출 시 인자 순서 실수가 발생하기 쉽습니다. Lombok의
@Builder를 활용하면 가독성과 안전성이 향상됩니다.♻️ Builder 패턴 적용 제안
`@Entity` `@Table`(name = "droppings") `@Getter` `@NoArgsConstructor`(access = AccessLevel.PROTECTED) +@Builder public class Dropping {Builder 적용 시
createdAt기본값 로직은@Builder.Default나@PrePersist로 처리할 수 있습니다:`@PrePersist` protected void onCreate() { if (this.createdAt == null) { this.createdAt = LocalDateTime.now(); } }
Comment on lines
+118
to
+124
| private Long parseDroppingId(String droppingId) { | ||
| try { | ||
| return Long.valueOf(droppingId); | ||
| } catch (NumberFormatException ex) { | ||
| throw DroppingNotFoundException.EXCEPTION; | ||
| } | ||
| } |
There was a problem hiding this comment.
null/blank droppingId 처리 누락으로 NPE 가능 (Line 118-123).
Long.valueOf(null)은 NullPointerException을 발생시키므로, 현재 예외 매핑이 동작하지 않습니다. null/blank를 먼저 차단하는 것이 안전합니다.
🛠️ 수정 제안
private Long parseDroppingId(String droppingId) {
+ if (droppingId == null || droppingId.isBlank()) {
+ throw DroppingNotFoundException.EXCEPTION;
+ }
try {
return Long.valueOf(droppingId);
} catch (NumberFormatException ex) {
throw DroppingNotFoundException.EXCEPTION;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private Long parseDroppingId(String droppingId) { | |
| try { | |
| return Long.valueOf(droppingId); | |
| } catch (NumberFormatException ex) { | |
| throw DroppingNotFoundException.EXCEPTION; | |
| } | |
| } | |
| private Long parseDroppingId(String droppingId) { | |
| if (droppingId == null || droppingId.isBlank()) { | |
| throw DroppingNotFoundException.EXCEPTION; | |
| } | |
| try { | |
| return Long.valueOf(droppingId); | |
| } catch (NumberFormatException ex) { | |
| throw DroppingNotFoundException.EXCEPTION; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/java/org/example/remedy/domain/comment/application/service/CommentService.java`
around lines 118 - 124, parseDroppingId currently calls Long.valueOf(droppingId)
which throws NPE for null and bypasses the NumberFormatException mapping; update
parseDroppingId to first check for null or blank (e.g., droppingId == null ||
droppingId.trim().isEmpty()) and throw DroppingNotFoundException.EXCEPTION for
those cases, and keep the existing try/catch for NumberFormatException to also
throw DroppingNotFoundException.EXCEPTION.
Comment on lines
+26
to
28
| @Type(JsonBinaryType.class) | ||
| @Column(name = "song_ids", columnDefinition = "jsonb") | ||
| private List<String> songIds; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for songIds field usage and null checks
rg -n "songIds" --type java -C 3 src/main/java/org/example/remedy/domain/playlist/Repository: TEAM-TERA/REMEDY-BACK
Length of output: 7365
🏁 Script executed:
# Look at the Playlist.java file structure
find . -name "Playlist.java" -path "*/playlist/domain/*" | head -1 | xargs wc -lRepository: TEAM-TERA/REMEDY-BACK
Length of output: 139
🏁 Script executed:
# Read the Playlist.java file
find . -name "Playlist.java" -path "*/playlist/domain/*" | head -1 | xargs cat -nRepository: TEAM-TERA/REMEDY-BACK
Length of output: 2212
JSONB 필드 null 값으로 인한 NullPointerException 위험
songIds 필드가 JSONB로 저장될 때, DB에 이미 null로 저장된 데이터가 있거나 직접 DB에 삽입된 경우 엔티티 로드 시 null 상태가 됩니다. 생성자의 new ArrayList<>() 초기화는 새로운 엔티티 생성 시에만 적용되며, JPA는 기존 데이터 로드 시 no-arg 생성자를 사용하므로 이 초기화가 발생하지 않습니다.
현재 addSong(), removeSong(), hasSong(), getSongIdList() 등의 메서드들이 null 체크 없이 songIds를 직접 사용하고 있어, null 값이 있는 데이터를 로드하면 NullPointerException이 발생합니다.
해결책: 필드를 기본값으로 초기화하거나(private List<String> songIds = new ArrayList<>();), @PostLoad 메서드를 추가하여 로드 후 null 값을 초기화하세요.
🤖 Prompt for AI Agents
In `@src/main/java/org/example/remedy/domain/playlist/domain/Playlist.java` around
lines 26 - 28, The songIds JSONB field can be null when loading existing rows
causing NPEs in methods like addSong, removeSong, hasSong, and getSongIdList;
fix by ensuring songIds is initialized after JPA loads the entity—either make
the field default to an empty list (private List<String> songIds = new
ArrayList<>();) or add a `@PostLoad` lifecycle method in Playlist (e.g.,
postLoad()) that checks if songIds is null and sets it to new ArrayList<>(), so
all usages of songIds in addSong/removeSong/hasSong/getSongIdList are safe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
릴리스 노트
✏️ Tip: You can customize this high-level summary in your review settings.