Skip to content

refactor: 독서태그 N+1문제를 해결합니다. #114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
minwoo1999 merged 3 commits into from
Sep 8, 2025
Merged

refactor: 독서태그 N+1문제를 해결합니다. #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ebb13f5
[BOOK-175] refactor: domain - N+1문제 개선 (#54)
minwoo1999 Aug 27, 2025
24eec00
[BOOK-175] refactor: infra - N+1문제 개선 (#54)
minwoo1999 Aug 27, 2025
194dd96
[BOOK-175] refactor: domain,infra - 코드레빗 코드리뷰 반영(#54)
minwoo1999 Aug 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,36 @@ class ReadingRecordDomainService(
sort: ReadingRecordSortType?,
pageable: Pageable
): Page<ReadingRecordInfoVO> {
return readingRecordRepository.findReadingRecordsByDynamicCondition(userBookId, sort, pageable)
.map { buildReadingRecordInfoVO(it) }
val readingRecordPage = readingRecordRepository.findReadingRecordsByDynamicCondition(userBookId, sort, pageable)
if (readingRecordPage.isEmpty) {
return Page.empty(pageable)
}

val readingRecords = readingRecordPage.content
val readingRecordIds = readingRecords.map { it.id.value }

val readingRecordTags = readingRecordTagRepository.findByReadingRecordIdIn(readingRecordIds)
val tagIds = readingRecordTags.map { it.tagId.value }
val tagsById = tagRepository.findByIds(tagIds).associateBy { it.id.value }

Comment on lines +113 to +116
Copy link

@coderabbitai coderabbitai bot Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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] }
}
Comment on lines +117 to +121
Copy link

@coderabbitai coderabbitai bot Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.


val userBook = userBookRepository.findById(userBookId)

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
)
}
Comment on lines +125 to +134
Copy link

@coderabbitai coderabbitai bot Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.

}

fun modifyReadingRecord(
Expand Down
1 change: 1 addition & 0 deletions domain/src/main/kotlin/org/yapp/domain/readingrecordtag/ReadingRecordTagRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import java.util.UUID
interface ReadingRecordTagRepository {
fun saveAll(readingRecordTags: List<ReadingRecordTag>): List<ReadingRecordTag>
fun findByReadingRecordId(readingRecordId: UUID): List<ReadingRecordTag>
fun findByReadingRecordIdIn(readingRecordIds: List<UUID>): List<ReadingRecordTag>
Copy link

@coderabbitai coderabbitai bot Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

fun deleteAllByReadingRecordId(readingRecordId: UUID)
fun countTagsByUserIdAndUserBookIdAndCategories(userId: UUID, userBookId: UUID, categories: List<String>): Map<String, Int>
}
1 change: 1 addition & 0 deletions ...c/main/kotlin/org/yapp/infra/readingrecordtag/repository/JpaReadingRecordTagRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ import java.util.*

interface JpaReadingRecordTagRepository : JpaRepository<ReadingRecordTagEntity, UUID>, JpaReadingRecordTagQuerydslRepository {
fun findByReadingRecordId(readingRecordId: UUID): List<ReadingRecordTagEntity>
fun findByReadingRecordIdIn(readingRecordIds: List<UUID>): List<ReadingRecordTagEntity>
Copy link

@coderabbitai coderabbitai bot Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

fun deleteAllByReadingRecordId(readingRecordId: UUID)
}
13 changes: 12 additions & 1 deletion .../kotlin/org/yapp/infra/readingrecordtag/repository/impl/ReadingRecordTagRepositoryImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,18 @@ class ReadingRecordTagRepositoryImpl(
return jpaReadingRecordTagRepository.findByReadingRecordId(readingRecordId).map { it.toDomain() }
}

override fun countTagsByUserIdAndUserBookIdAndCategories(userId: UUID, userBookId: UUID, categories: List<String>): Map<String, Int> {
override fun findByReadingRecordIdIn(readingRecordIds: List<UUID>): List<ReadingRecordTag> {
if (readingRecordIds.isEmpty()) return emptyList()
return jpaReadingRecordTagRepository
.findByReadingRecordIdIn(readingRecordIds)
.map { it.toDomain() }
}
Comment on lines +23 to +28
Copy link

@coderabbitai coderabbitai bot Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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() }
}


override fun countTagsByUserIdAndUserBookIdAndCategories(
userId: UUID,
userBookId: UUID,
categories: List<String>
): Map<String, Int> {
return jpaReadingRecordTagRepository.countTagsByUserIdAndUserBookIdAndCategories(userId, userBookId, categories)
}

Expand Down