Skip to content

fix: 독서 기록 pagenumber로 order by 시 페이징처리 동일한 id가 내려오는 상황 #97

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 1 commit into from
Aug 16, 2025
Merged

fix: 독서 기록 pagenumber로 order by 시 페이징처리 동일한 id가 내려오는 상황 #97

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 15 additions & 7 deletions ...in/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class JpaReadingRecordQuerydslRepositoryImpl(
val results = queryFactory
.selectFrom(readingRecord)
.where(whereCondition)
.orderBy(createOrderSpecifier(sort))
.orderBy(*createOrderSpecifiers(sort))
Copy link

@coderabbitai coderabbitai bot Aug 16, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

스프레드 연산자 사용(detekt 경고) — 영향 미미하나 억제 또는 구조 변경을 고려하세요

.orderBy(*createOrderSpecifiers(sort))는 detekt의 SpreadOperator 경고 대상입니다. 현재 배열 길이가 1~2 수준이라 성능 영향은 사실상 미미합니다. 유지보수를 위해 그대로 두되 경고 억제 또는 호출부 구조 변경 중 하나를 권장합니다.

대안 1: 함수 레벨에서 경고 억제

@Suppress("SpreadOperator")
override fun findReadingRecordsByDynamicCondition(
    userBookId: UUID,
    sort: ReadingRecordSortType?,
    pageable: Pageable
): Page<ReadingRecordEntity> {
    // ...
}

대안 2: 스프레드 없이 체이닝

  • createOrderSpecifiers를 사용하지 않고 when 분기 내에서 .orderBy(a, b) 형태로 직접 지정
  • 변경폭이 커지므로 본 PR 범위를 넘는다면 대안 1이 현실적입니다.
🧰 Tools
🪛 detekt (1.23.8)

[warning] 34-34: In most cases using a spread operator causes a full copy of the array to be created before calling a method. This may result in a performance penalty.

(detekt.performance.SpreadOperator)

🤖 Prompt for AI Agents 
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
around line 34: Detekt flags the spread operator in
`.orderBy(*createOrderSpecifiers(sort))`; to address this either suppress the
warning on the function with @Suppress("SpreadOperator") or remove the spread by
inlining orderBy arguments (use explicit chaining or branch on sort and call
.orderBy(a, b) directly); pick suppression if you want minimal changes, or
refactor the ordering logic to avoid creating and spreading an array if you
prefer to eliminate the warning fully.

.offset(pageable.offset)
.limit(pageable.pageSize.toLong())
.fetch()
Expand All @@ -45,13 +45,21 @@ class JpaReadingRecordQuerydslRepositoryImpl(
return PageImpl(results, pageable, total)
}

private fun createOrderSpecifier(sort: ReadingRecordSortType?): OrderSpecifier<*> {
private fun createOrderSpecifiers(sort: ReadingRecordSortType?): Array<OrderSpecifier<*>> {
return when (sort) {
ReadingRecordSortType.PAGE_NUMBER_ASC -> readingRecord.pageNumber.asc()
ReadingRecordSortType.PAGE_NUMBER_DESC -> readingRecord.pageNumber.desc()
ReadingRecordSortType.CREATED_DATE_ASC -> readingRecord.createdAt.asc()
ReadingRecordSortType.CREATED_DATE_DESC -> readingRecord.createdAt.desc()
null -> readingRecord.createdAt.desc()
ReadingRecordSortType.PAGE_NUMBER_ASC -> arrayOf(
readingRecord.pageNumber.asc(),
readingRecord.updatedAt.desc()
)
Comment on lines +50 to +53
Copy link

@coderabbitai coderabbitai bot Aug 16, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

updatedAt을 보조 정렬로 사용한 선택은 타당합니다

동일 pageNumber 내에서 “최근 수정 기준”을 기대한다면 updatedAt.desc()는 도메인 의도에 부합합니다. 위 코멘트대로 최종 타이브레이커(id)만 추가해 주시면 더 이상 중복 위험이 없습니다.

🤖 Prompt for AI Agents 
In
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
around lines 50 to 53, the sort array uses pageNumber and updatedAt but lacks a
final tiebreaker; append the entity id comparator as the last sort key (e.g.,
readingRecord.id.asc() or .desc() depending on desired stable order) so results
are deterministic and duplicate-key risks are eliminated.


ReadingRecordSortType.PAGE_NUMBER_DESC -> arrayOf(
readingRecord.pageNumber.desc(),
readingRecord.updatedAt.desc()
)

Comment on lines +48 to +59
Copy link

@coderabbitai coderabbitai bot Aug 16, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

🛠️ Refactor suggestion

타이브레이커 부족 — id로 최종 정렬 기준을 추가해 페이징 안정성을 보장하세요

updatedAt.desc()를 보조 정렬로 추가한 것은 합리적입니다. 다만 동일 pageNumber와 동일 updatedAt이 동시에 존재할 경우(타임스탬프 해상도/동시 업데이트 등) 여전히 정렬이 비결정적이어서 오프셋 페이징에서 중복/누락이 재현될 수 있습니다. 최종 타이브레이커로 유일 키(일반적으로 id)를 추가해 결정적 정렬을 보장하세요.

권장 변경(diff):

             ReadingRecordSortType.PAGE_NUMBER_ASC -> arrayOf(
                 readingRecord.pageNumber.asc(),
-                readingRecord.updatedAt.desc()
+                readingRecord.updatedAt.desc(),
+                readingRecord.id.desc()
             )

             ReadingRecordSortType.PAGE_NUMBER_DESC -> arrayOf(
                 readingRecord.pageNumber.desc(),
-                readingRecord.updatedAt.desc()
+                readingRecord.updatedAt.desc(),
+                readingRecord.id.desc()
             )
  • updatedAt vs createdAt에 대한 질문에 답변: “페이지 번호 내에서 최근에 수정된 기록을 우선”이라는 제품 의도가 있다면 updatedAt.desc()가 createdAt보다 더 적절합니다. 단, “등록 순서” 의미가 더 중요하다면 createdAt을 택할 수 있습니다. 어떤 의미가 UX/도메인에 맞는지 팀 합의가 필요합니다.

성능(인덱스) 관점:

  • 현재 where: (userBookId, deletedAt IS NULL) + order: (pageNumber [asc/desc], updatedAt desc, id desc) 조합을 자주 사용한다면 해당 컬럼 순서로의 복합 인덱스 추가를 고려하세요.
    • 예: (user_book_id, deleted_at, page_number, updated_at, id)
  • createdAt 정렬 경로도 빈번하다면 (user_book_id, deleted_at, created_at, id) 인덱스가 유효합니다.

오프셋 페이징의 본질적 한계:

  • updatedAt이 자주 변하는 도메인이라면 페이지를 넘기는 사이에 레코드가 이동하여 중복/누락이 발생할 수 있습니다. 장기적으로는 keyset 페이징(커서 기반) 전환을 검토하세요.
📝 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 fun createOrderSpecifiers(sort: ReadingRecordSortType?): Array<OrderSpecifier<*>> {
return when (sort) {
ReadingRecordSortType.PAGE_NUMBER_ASC -> readingRecord.pageNumber.asc()
ReadingRecordSortType.PAGE_NUMBER_DESC -> readingRecord.pageNumber.desc()
ReadingRecordSortType.CREATED_DATE_ASC -> readingRecord.createdAt.asc()
ReadingRecordSortType.CREATED_DATE_DESC -> readingRecord.createdAt.desc()
null -> readingRecord.createdAt.desc()
ReadingRecordSortType.PAGE_NUMBER_ASC -> arrayOf(
readingRecord.pageNumber.asc(),
readingRecord.updatedAt.desc()
)
ReadingRecordSortType.PAGE_NUMBER_DESC -> arrayOf(
readingRecord.pageNumber.desc(),
readingRecord.updatedAt.desc()
)
private fun createOrderSpecifiers(sort: ReadingRecordSortType?): Array<OrderSpecifier<*>> {
return when (sort) {
ReadingRecordSortType.PAGE_NUMBER_ASC -> arrayOf(
readingRecord.pageNumber.asc(),
readingRecord.updatedAt.desc(),
readingRecord.id.desc()
)
ReadingRecordSortType.PAGE_NUMBER_DESC -> arrayOf(
readingRecord.pageNumber.desc(),
readingRecord.updatedAt.desc(),
readingRecord.id.desc()
)
🤖 Prompt for AI Agents 
In
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
around lines 48–59, the order specifiers lack a final deterministic tiebreaker
which can cause nondeterministic paging when pageNumber and updatedAt are equal;
add the primary key (id) as the last OrderSpecifier (descending or ascending
consistent with desired newest-first/oldest-first tie-breaking) to both
PAGE_NUMBER_ASC and PAGE_NUMBER_DESC branches so sorting is fully deterministic;
optionally confirm whether updatedAt or createdAt is the intended second-level
sort and, if this sort pattern is frequent, consider adding a composite DB index
(e.g., user_book_id, deleted_at, page_number, updated_at, id) to support the new
ordering.

ReadingRecordSortType.CREATED_DATE_ASC -> arrayOf(readingRecord.createdAt.asc())
ReadingRecordSortType.CREATED_DATE_DESC -> arrayOf(readingRecord.createdAt.desc())
null -> arrayOf(readingRecord.createdAt.desc())
}
Comment on lines +60 to 63
Copy link

@coderabbitai coderabbitai bot Aug 16, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

createdAt 정렬에도 유일 타이브레이커(id) 추가를 고려

본 PR 범위를 넘는 제안이지만, CREATED_DATE_ASC/DESC에서도 동일 타임스탬프가 가능하므로 최종적으로 id를 추가하면 전역적으로 페이징 안정성이 향상됩니다.

권장 변경(diff):

-            ReadingRecordSortType.CREATED_DATE_ASC -> arrayOf(readingRecord.createdAt.asc())
-            ReadingRecordSortType.CREATED_DATE_DESC -> arrayOf(readingRecord.createdAt.desc())
-            null -> arrayOf(readingRecord.createdAt.desc())
+            ReadingRecordSortType.CREATED_DATE_ASC -> arrayOf(
+                readingRecord.createdAt.asc(),
+                readingRecord.id.desc()
+            )
+            ReadingRecordSortType.CREATED_DATE_DESC -> arrayOf(
+                readingRecord.createdAt.desc(),
+                readingRecord.id.desc()
+            )
+            null -> arrayOf(
+                readingRecord.createdAt.desc(),
+                readingRecord.id.desc()
+            )

단위 테스트 보강 제안:

  • 동일 createdAt/updatedAt를 가진 샘플 데이터로 오프셋 페이징 시 중복/누락이 없는지 검증하는 테스트를 추가하면 회귀 방지에 도움이 됩니다.
📝 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
ReadingRecordSortType.CREATED_DATE_ASC -> arrayOf(readingRecord.createdAt.asc())
ReadingRecordSortType.CREATED_DATE_DESC -> arrayOf(readingRecord.createdAt.desc())
null -> arrayOf(readingRecord.createdAt.desc())
}
ReadingRecordSortType.CREATED_DATE_ASC -> arrayOf(
readingRecord.createdAt.asc(),
readingRecord.id.desc()
)
ReadingRecordSortType.CREATED_DATE_DESC -> arrayOf(
readingRecord.createdAt.desc(),
readingRecord.id.desc()
)
null -> arrayOf(
readingRecord.createdAt.desc(),
readingRecord.id.desc()
)
}
🤖 Prompt for AI Agents 
In
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
around lines 60 to 63, the sorting for CREATED_DATE_ASC/DESC currently only uses
createdAt which can produce non-deterministic ordering when timestamps are
identical; update the sort arrays to add id as a deterministic tie-breaker (for
CREATED_DATE_ASC use createdAt.asc(), id.asc(); for CREATED_DATE_DESC use
createdAt.desc(), id.desc(); keep the null default as createdAt.desc(),
id.desc()). Also add unit tests that create multiple records with identical
createdAt/updatedAt values and assert offset-based paging returns stable,
non-duplicated/omitted results.

}

Expand Down