refactor: 독서태그 N+1문제를 해결합니다. #114
Conversation
Walkthrough읽기 기록 조회를 per-record VO 생성에서 배치 로딩으로 변경하여 읽기기록, 태그, 유저북을 일괄 조회하고 그룹핑해 ReadingRecordInfoVO를 생성하도록 흐름을 수정했습니다. ReadingRecordTag의 다건 조회 메서드도 도메인/인프라 계층에 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as ReadingRecordDomainService
participant RR as ReadingRecordRepository
participant RRT as ReadingRecordTagRepository
participant UR as UserBookRepository
participant T as TagRepository
C->>S: findReadingRecordsByDynamicCondition(pageable, filters)
S->>RR: findReadingRecordsByDynamicCondition(...)
RR-->>S: Page<ReadingRecord>
alt empty page
S-->>C: Page.empty(pageable)
else has content
S->>RRT: findByReadingRecordIdIn(recordIds)
RRT-->>S: List<ReadingRecordTag>
S->>T: findByIdIn(tagIds)
T-->>S: List<Tag>
S->>UR: findById(userBookId)
UR-->>S: UserBook?
S-->>C: Page<ReadingRecordInfoVO>
end
rect rgba(200,230,255,0.25)
note over S: 변경: N+1 회피를 위한 배치 로딩 및 그룹핑
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/readingrecordtag/ReadingRecordTagRepository.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/readingrecordtag/repository/JpaReadingRecordTagRepository.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/readingrecordtag/repository/impl/ReadingRecordTagRepositoryImpl.kt(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI - Pull Request
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
[error] 113-113: Kotlin compilation error: Unresolved reference 'readingRecordTags' and 'it' on line 113.
[error] 117-117: Kotlin compilation error: Unresolved reference 'readingRecordTags' and 'it' on line 117.
[error] 120-120: Kotlin compilation error: Not enough information to infer type variable T on line 120.
[error] 121-121: Kotlin compilation error: Unresolved reference 'it' on line 121.
[error] 125-125: Kotlin compilation error: Unresolved reference 'it' on line 125.
🔇 Additional comments (1)
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt (1)
105-111: 빈 페이지 처리 로직은 적절합니다.
readingRecords.isEmpty()시Page.empty(pageable)반환은 N+1 최적화 플로우와 잘 맞습니다. 그대로 가시죠.
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
Show resolved
Hide resolved
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
Outdated
Show resolved
Hide resolved
| interface ReadingRecordTagRepository { | ||
| fun saveAll(readingRecordTags: List<ReadingRecordTag>): List<ReadingRecordTag> | ||
| fun findByReadingRecordId(readingRecordId: UUID): List<ReadingRecordTag> | ||
| fun findByReadingRecordIdIn(readingRecordIds: List<UUID>): List<ReadingRecordTag> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
배치 조회 메서드 추가는 방향 OK. 입력/결과 계약을 명시해 주세요.
빈 리스트 입력 시 반환 값(빈 리스트 권장), 중복 ID 처리, 결과 정렬 보장 여부를 KDoc에 명확히 남겨주세요. 서비스/인프라에서 이미 안전 가드를 둘 예정이라도 계약을 확실히 해두면 유지보수 비용이 줄어듭니다.
🤖 Prompt for AI Agents
In
domain/src/main/kotlin/org/yapp/domain/readingrecordtag/ReadingRecordTagRepository.kt
around line 8, the new batch query method lacks a contract in KDoc; add a
concise KDoc above the method that states: (1) empty input returns an empty
list, (2) duplicate IDs in the input are allowed but do not cause duplicate
entities in the result (results correspond to matching entities once per stored
entity), (3) callers should not rely on any ordering unless explicitly requested
— if deterministic ordering is desired specify it (e.g., sorted by
readingRecordId then tag id) and implement accordingly, and (4) whether nulls
are allowed (reject nulls in the list). Keep the KDoc short and precise so
service/infrastructure callers know the expected behavior.
...ain/kotlin/org/yapp/infra/readingrecordtag/repository/impl/ReadingRecordTagRepositoryImpl.kt
Show resolved
Hide resolved
|
|
||
| interface JpaReadingRecordTagRepository : JpaRepository<ReadingRecordTagEntity, UUID>, JpaReadingRecordTagQuerydslRepository { | ||
| fun findByReadingRecordId(readingRecordId: UUID): List<ReadingRecordTagEntity> | ||
| fun findByReadingRecordIdIn(readingRecordIds: List<UUID>): List<ReadingRecordTagEntity> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
빈 컬렉션 IN 처리 주의.
Spring Data JPA에서 ...In(emptyList())는 DB/벤더에 따라 예외가 날 수 있습니다. 호출부 또는 구현체에서 빈 리스트 조기 반환 가드를 두는 것을 전제로 합의해 주세요(아래 Impl 코멘트 참고).
🤖 Prompt for AI Agents
infra/src/main/kotlin/org/yapp/infra/readingrecordtag/repository/JpaReadingRecordTagRepository.kt
around line 9: the repository method findByReadingRecordIdIn(readingRecordIds:
List<UUID>) may throw on an empty list depending on the JPA provider; change
callers or the repo implementation to guard against empty input by returning an
empty list early instead of calling the generated query. Add a null/empty check
at call sites to return emptyList() when readingRecordIds.isEmpty(), or provide
a custom repository impl method that checks for empty collection and returns
emptyList() before invoking the JPA query.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
infra/src/main/kotlin/org/yapp/infra/readingrecordtag/repository/impl/ReadingRecordTagRepositoryImpl.kt (1)
23-28: 빈 리스트 가드 추가: 잘 처리되었습니다.
IN ()회피 및 불필요한 호출 방지에 유효합니다. 지난 코멘트 반영 확인했습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/readingrecordtag/repository/impl/ReadingRecordTagRepositoryImpl.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-validation
🔇 Additional comments (2)
infra/src/main/kotlin/org/yapp/infra/readingrecordtag/repository/impl/ReadingRecordTagRepositoryImpl.kt (1)
30-34: 서명 포매팅 변경만 존재: 문제 없습니다.domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt (1)
105-108: 빈 페이지 fast-return: 적절합니다.불필요한 후속 조회를 막아 성능 이점이 있습니다.
| val readingRecordTags = readingRecordTagRepository.findByReadingRecordIdIn(readingRecordIds) | ||
| val tagIds = readingRecordTags.map { it.tagId.value } | ||
| val tagsById = tagRepository.findByIds(tagIds).associateBy { it.id.value } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
빈 태그일 때 DB 호출 방지 + 중복 tagIds 제거
태그가 없으면 findByIds를 건너뛰고, 중복 tagId를 제거해 바인딩 수를 줄이세요.
- val tagIds = readingRecordTags.map { it.tagId.value }
- val tagsById = tagRepository.findByIds(tagIds).associateBy { it.id.value }
+ val tagIds = readingRecordTags.map { it.tagId.value }.distinct()
+ val tagsById: Map<UUID, Tag> =
+ if (tagIds.isEmpty()) emptyMap()
+ else tagRepository.findByIds(tagIds).associateBy { it.id.value }📝 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.
| val readingRecordTags = readingRecordTagRepository.findByReadingRecordIdIn(readingRecordIds) | |
| val tagIds = readingRecordTags.map { it.tagId.value } | |
| val tagsById = tagRepository.findByIds(tagIds).associateBy { it.id.value } | |
| val readingRecordTags = readingRecordTagRepository.findByReadingRecordIdIn(readingRecordIds) | |
| val tagIds = readingRecordTags.map { it.tagId.value }.distinct() | |
| val tagsById: Map<UUID, Tag> = | |
| if (tagIds.isEmpty()) emptyMap() | |
| else tagRepository.findByIds(tagIds).associateBy { it.id.value } |
🤖 Prompt for AI Agents
In
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
around lines 113 to 116, avoid calling tagRepository.findByIds when there are no
tag IDs and reduce duplicate DB bindings by deduplicating tagIds before the
query; specifically, build tagIds as a distinct collection (e.g.,
toSet()/distinct()) and if that collection is empty return or set tagsById to an
emptyMap(), otherwise call findByIds with the deduped list and associateBy id as
before.
| val tagsByReadingRecordId = readingRecordTags | ||
| .groupBy { it.readingRecordId.value } | ||
| .mapValues { (_, tags) -> | ||
| tags.mapNotNull { tagsById[it.tagId.value] } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
가독성 니트픽: 람다 변수명 충돌 방지
tagsById와 동일 이름 사용을 피하면 읽기성이 좋아집니다.
- val tagsByReadingRecordId = readingRecordTags
- .groupBy { it.readingRecordId.value }
- .mapValues { (_, tags) ->
- tags.mapNotNull { tagsById[it.tagId.value] }
- }
+ val tagsByReadingRecordId = readingRecordTags
+ .groupBy { it.readingRecordId.value }
+ .mapValues { (_, tagLinks) ->
+ tagLinks.mapNotNull { tagsById[it.tagId.value] }
+ }📝 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.
| val tagsByReadingRecordId = readingRecordTags | |
| .groupBy { it.readingRecordId.value } | |
| .mapValues { (_, tags) -> | |
| tags.mapNotNull { tagsById[it.tagId.value] } | |
| } | |
| val tagsByReadingRecordId = readingRecordTags | |
| .groupBy { it.readingRecordId.value } | |
| .mapValues { (_, tagLinks) -> | |
| tagLinks.mapNotNull { tagsById[it.tagId.value] } | |
| } |
🤖 Prompt for AI Agents
In
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
around lines 117 to 121, avoid reusing the name "tags" in the mapValues lambda
which reduces readability and risks confusion with tagsById; rename the lambda
variable (for example to "rrTags" or "readingRecordTags") and update the inner
mapping to use that new name (e.g., rrTags.mapNotNull { tagsById[it.tagId.value]
}) so the code clearly distinguishes the grouped list from the tagsById lookup.
| return readingRecordPage.map { readingRecord -> | ||
| ReadingRecordInfoVO.newInstance( | ||
| readingRecord = readingRecord, | ||
| emotionTags = tagsByReadingRecordId[readingRecord.id.value]?.map { it.name } ?: emptyList(), | ||
| bookTitle = userBook?.title, | ||
| bookPublisher = userBook?.publisher, | ||
| bookCoverImageUrl = userBook?.coverImageUrl, | ||
| author = userBook?.author | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
제네릭 추론 보조(필요 시만): 람다 파라미터 타입 명시
환경에 따라 드물게 Page.map 제네릭 추론 경고가 남을 수 있습니다. 발생 시 아래처럼 파라미터 타입을 명시하세요.
- return readingRecordPage.map { readingRecord ->
+ return readingRecordPage.map { readingRecord: ReadingRecord ->
ReadingRecordInfoVO.newInstance(
readingRecord = readingRecord,
emotionTags = tagsByReadingRecordId[readingRecord.id.value]?.map { it.name } ?: emptyList(),
bookTitle = userBook?.title,
bookPublisher = userBook?.publisher,
bookCoverImageUrl = userBook?.coverImageUrl,
author = userBook?.author
)
}📝 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.
| return readingRecordPage.map { readingRecord -> | |
| ReadingRecordInfoVO.newInstance( | |
| readingRecord = readingRecord, | |
| emotionTags = tagsByReadingRecordId[readingRecord.id.value]?.map { it.name } ?: emptyList(), | |
| bookTitle = userBook?.title, | |
| bookPublisher = userBook?.publisher, | |
| bookCoverImageUrl = userBook?.coverImageUrl, | |
| author = userBook?.author | |
| ) | |
| } | |
| return readingRecordPage.map { readingRecord: ReadingRecord -> | |
| ReadingRecordInfoVO.newInstance( | |
| readingRecord = readingRecord, | |
| emotionTags = tagsByReadingRecordId[readingRecord.id.value]?.map { it.name } ?: emptyList(), | |
| bookTitle = userBook?.title, | |
| bookPublisher = userBook?.publisher, | |
| bookCoverImageUrl = userBook?.coverImageUrl, | |
| author = userBook?.author | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
around lines 125 to 134, the Page.map lambda relies on implicit generic
inference which can emit warnings in some environments; explicitly declare the
lambda parameter type for readingRecord (for example: readingRecord:
ReadingRecord) in the map call to help the compiler infer generics correctly —
update the map invocation to use a typed parameter and keep the inner
construction unchanged.
| override fun findByReadingRecordIdIn(readingRecordIds: List<UUID>): List<ReadingRecordTag> { | ||
| if (readingRecordIds.isEmpty()) return emptyList() | ||
| return jpaReadingRecordTagRepository | ||
| .findByReadingRecordIdIn(readingRecordIds) | ||
| .map { it.toDomain() } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
대규모 IN 파라미터 대비: 중복 제거 + 청크 처리 제안
대량 ID 입력 시 DB 바인드 한도/플랜 비대화 대비를 권장합니다. 안전하게 distinct() 후 chunked()로 나눠 조회하세요.
- override fun findByReadingRecordIdIn(readingRecordIds: List<UUID>): List<ReadingRecordTag> {
- if (readingRecordIds.isEmpty()) return emptyList()
- return jpaReadingRecordTagRepository
- .findByReadingRecordIdIn(readingRecordIds)
- .map { it.toDomain() }
- }
+ override fun findByReadingRecordIdIn(readingRecordIds: List<UUID>): List<ReadingRecordTag> {
+ if (readingRecordIds.isEmpty()) return emptyList()
+ return readingRecordIds
+ .distinct()
+ .chunked(1_000)
+ .flatMap { chunk ->
+ jpaReadingRecordTagRepository.findByReadingRecordIdIn(chunk)
+ }
+ .map { it.toDomain() }
+ }📝 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.
| override fun findByReadingRecordIdIn(readingRecordIds: List<UUID>): List<ReadingRecordTag> { | |
| if (readingRecordIds.isEmpty()) return emptyList() | |
| return jpaReadingRecordTagRepository | |
| .findByReadingRecordIdIn(readingRecordIds) | |
| .map { it.toDomain() } | |
| } | |
| override fun findByReadingRecordIdIn(readingRecordIds: List<UUID>): List<ReadingRecordTag> { | |
| if (readingRecordIds.isEmpty()) return emptyList() | |
| return readingRecordIds | |
| .distinct() | |
| .chunked(1_000) | |
| .flatMap { chunk -> | |
| jpaReadingRecordTagRepository.findByReadingRecordIdIn(chunk) | |
| } | |
| .map { it.toDomain() } | |
| } |
📘 작업 유형
📙 작업 내역
ReadingRecordTagRepository인터페이스에findByReadingRecordIdIn메서드 추가JpaReadingRecordTagRepository에findByReadingRecordIdIn메서드 추가ReadingRecordTagRepositoryImpl구현체에findByReadingRecordIdIn메서드 구현ReadingRecordService내getReadingRecords로직 개선readingRecord들의 ID를 기반으로 태그를 일괄 조회하도록 변경readingRecordTags를 미리 그룹핑 후 매핑하는 방식으로 수정🧪 테스트 내역
readingRecord조회 시 태그가 정상적으로 매핑되는지 확인Page.empty(pageable)정상 반환 확인🎨 스크린샷 또는 시연 영상 (선택)
해당 없음
✅ PR 체크리스트
💬 추가 설명 or 리뷰 포인트
readingRecordId기반으로 태그를 조회할 수 있는 기능이 추가되어 향후 API 성능 최적화에 활용될 수 있습니다.Summary by CodeRabbit