refactor: 독서 기록 목록 API에 대표 감정 응답 추가 및 버그 개선 #148
Conversation
|
Warning Rate limit exceeded@move-hoon has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Walkthrough독서 기록 목록 응답에 최다(대표) 감정 정보를 포함하는 복합 응답 DTO와 관련 변환 로직을 추가하고, 서비스·도메인·레포지토리 계층에 최다 감정 조회 및 매핑 로직을 구현했습니다. 컨트롤러·유스케이스 반환 타입이 복합 응답으로 변경되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
Client->>Controller: GET /reading-records?userBookId
Controller->>UseCase: getReadingRecordsByUserBookId(userBookId, sort, pageable)
UseCase->>Service: getReadingRecordsByDynamicCondition(userBookId, sort, pageable)
Service->>ReadingRecordRepository: findReadingRecordsByDynamicCondition(...)
ReadingRecordRepository-->>Service: Page<ReadingRecord>
Service->>ReadingRecordRepository: findMostFrequentPrimaryEmotion(userBookId)
ReadingRecordRepository->>JpaQuerydslRepo: findMostFrequentPrimaryEmotion(userBookId)
JpaQuerydslRepo-->>ReadingRecordRepository: PrimaryEmotion?
ReadingRecordRepository-->>Service: PrimaryEmotion?
Service->>UserBookDomainService: findById(userBookId)
UserBookDomainService-->>Service: UserBookInfoVO?
Service->>Service: buildDetailTagsMap(readingRecordIds)
Service->>Service: toResponsePage(page, detailTagsMap)
Service->>Service: toPrimaryEmotionDto(primaryEmotion)
Service-->>UseCase: ReadingRecordsWithPrimaryEmotionResponse
UseCase-->>Controller: ReadingRecordsWithPrimaryEmotionResponse
Controller-->>Client: ResponseEntity<ReadingRecordsWithPrimaryEmotionResponse>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApiV2.kt (1)
94-120: API 문서에 응답 필드 설명 추가 필요현재 94-97번 라인의
description에서 응답에 포함된primaryEmotion필드(대표 감정)에 대한 설명이 누락되었습니다.권장사항: 설명을 다음과 같이 업데이트하세요:
- 현재: "사용자의 책에 대한 독서 기록을 페이징하여 조회합니다. sort 파라미터가 지정된 경우 해당 정렬이 우선 적용되며, 지정하지 않으면 기본 정렬(updatedAt DESC)이 적용됩니다."
- 제안: "사용자의 책에 대한 독서 기록을 페이징하여 조회하며, 해당 책의 대표(최다 기록) 감정 정보를 함께 반환합니다. sort 파라미터가 지정된 경우 해당 정렬이 우선 적용되며, 지정하지 않으면 기본 정렬(updatedAt DESC)이 적용됩니다."
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/JpaReadingRecordQuerydslRepository.kt (1)
11-18: 반환 타입이String?이면 매핑 실패가 조용히 발생할 수 있어, 타입/계약을 더 강하게 가져가세요.가능하면 여기서부터
PrimaryEmotion?(또는 엔티티 enum 타입)을 반환해서 문자열 매핑을 제거하거나, 최소한 “반환 문자열은 enumname과 동일” 같은 규칙을 KDoc로 고정하는 걸 권장합니다.
🤖 Fix all issues with AI agents
In @apis/src/main/kotlin/org/yapp/apis/emotion/dto/response/EmotionDetailDto.kt:
- Around line 14-18: The companion object factory EmotionDetailDto.of currently
just constructs an instance; add simple validation for the name parameter (e.g.,
non-null/non-blank) inside of and throw a clear exception
(IllegalArgumentException) when invalid, or if you want to keep parity with
PrimaryEmotionDto leave the factory as-is and ensure the same validation is
implemented at the service layer where objects are created; update either
EmotionDetailDto.of or the service creation path to enforce consistent name
validation.
In
@apis/src/main/kotlin/org/yapp/apis/readingrecord/service/ReadingRecordServiceV2.kt:
- Around line 103-118: The implementation of buildDetailTagsMap safely filters
out missing detail tags but the reviewer suggests optionally logging when a tag
referenced by readingRecordDetailTagDomainService has no corresponding entry in
tagLookup; update buildDetailTagsMap to, inside the tags.mapNotNull block, when
tagLookup[tag.detailTagId.value] is null, call a logger.warn (e.g.,
logger.warn("DetailTag not found for ID: {}" , tag.detailTagId.value)) before
returning null, and ensure a logger is available in the ReadingRecordServiceV2
class (add a companion object or injected logger) so missing tag lookups are
recorded for monitoring.
In
@domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt:
- Around line 113-135: Replace the in-memory aggregation in
findPrimaryEmotionByUserBookId with a direct delegation to
readingRecordRepository.findMostFrequentPrimaryEmotion(userBookId); call and
return that result from findPrimaryEmotionByUserBookId, and if you need to
preserve the original tie-with-all-createdAt-null behavior ensure the repository
method is adjusted to return null in that specific edge case (or add a small
repository-side check to detect tied emotions with all null createdAt and return
null) so domain service remains a thin delegator.
In
@domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordRepository.kt:
- Around line 38-40: Add KDoc to the
ReadingRecordRepository.findMostFrequentPrimaryEmotion method that documents the
contract: state whether soft-deleted records (deletedAt) are excluded, specify
the tie-breaker rule used when multiple emotions have the same frequency (e.g.,
choose the record with the latest createdAt), and confirm that the method
returns null when there are no matching records; reference the method name
findMostFrequentPrimaryEmotion and ensure the three points are clearly and
concisely described so implementers/callers know expected behavior.
In
@infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt:
- Around line 48-62: The query in findMostFrequentPrimaryEmotion uses ambiguous
stringValue() and count() expressions; change the aggregation to use an explicit
count on the identifier (e.g. readingRecord.id.count()) and select the enum
value itself (readingRecord.primaryEmotion) rather than stringValue(), then
convert that enum to a stable name string before returning (or change the method
to return the enum type) while keeping the ordering by count and latest
createdAt (readingRecord.createdAt.max().desc()) intact.
In
@infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/ReadingRecordRepositoryImpl.kt:
- Around line 71-74: The mapping of the DB string to enum in
findMostFrequentPrimaryEmotion currently uses PrimaryEmotion.entries.find {
it.name == emotionName } which returns null on mismatch; replace that mapping
with PrimaryEmotion.fromCode(emotionName) so invalid DB values safely fall back
to OTHER; update the expression to use
jpaReadingRecordRepository.findMostFrequentPrimaryEmotion(userBookId)?.let {
PrimaryEmotion.fromCode(it) } referencing the existing
findMostFrequentPrimaryEmotion method and PrimaryEmotion.fromCode.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
apis/src/main/kotlin/org/yapp/apis/emotion/dto/response/EmotionDetailDto.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApiV2.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerV2.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/dto/response/PrimaryEmotionDto.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/dto/response/ReadingRecordsWithPrimaryEmotionResponse.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/service/ReadingRecordServiceV2.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/usecase/ReadingRecordUseCaseV2.ktdomain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.ktdomain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordRepository.ktinfra/src/main/kotlin/org/yapp/infra/readingrecord/repository/JpaReadingRecordQuerydslRepository.ktinfra/src/main/kotlin/org/yapp/infra/readingrecord/repository/JpaReadingRecordRepository.ktinfra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.ktinfra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/ReadingRecordRepositoryImpl.kt
⏰ 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 (16)
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/JpaReadingRecordRepository.kt (2)
25-25: Trailing blank line 추가 확인됨파일 끝에 추가된 blank line입니다. 이는 일반적인 코드 스타일 개선사항입니다.
1-24: 주 감정(Primary Emotion) 조회 메서드가 올바르게 구현되어 있습니다.
JpaReadingRecordQuerydslRepository인터페이스에findMostFrequentPrimaryEmotion(userBookId: UUID): String?메서드가 추가되었으며,JpaReadingRecordQuerydslRepositoryImpl에서 Querydsl aggregation으로 올바르게 구현되어 있습니다. 그룹화와 정렬(count() DESC,createdAt.max() DESC)을 통해 가장 빈번한 감정을 반환하고 있습니다.Likely an incorrect or invalid review comment.
apis/src/main/kotlin/org/yapp/apis/emotion/dto/response/EmotionDetailDto.kt (1)
6-13: LGTM! 깔끔한 DTO 구조와 적절한 문서화데이터 클래스 구조가 명확하고, Swagger 어노테이션이 적절히 작성되어 있습니다. 불변 속성(val)과 private 생성자 패턴이
PrimaryEmotionDto와 일관성 있게 적용되었습니다.apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApiV2.kt (1)
21-21: LGTM - 필요한 import 추가새로운 응답 타입을 사용하기 위한 적절한 import입니다.
apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerV2.kt (2)
14-14: LGTM - 새로운 응답 타입 import 추가새로운 복합 응답 타입 사용을 위한 적절한 import입니다.
51-66: LGTM - 인터페이스와 일치하는 구현컨트롤러 구현이 API 인터페이스의 변경사항과 일치하며, use case 레이어에 적절히 위임하고 있습니다.
apis/src/main/kotlin/org/yapp/apis/readingrecord/dto/response/ReadingRecordsWithPrimaryEmotionResponse.kt (2)
17-27: LGTM - 일관된 팩토리 패턴 적용Companion object의 팩토리 메서드 패턴이 프로젝트의 다른 DTO들과 일관되게 적용되어 있습니다.
10-16: 코드 분석 결과: 신경 쓸 필요가 없는 코드입니다
primaryEmotion필드의 null 처리가 일관되게 구현되어 있습니다. 도메인 서비스의findPrimaryEmotionByUserBookId메서드는 다음과 같이 동작합니다:
- 독서 기록이 없으면 즉시 null을 반환 (line 115)
- 독서 기록이 있으면 가장 빈번한 감정을 반환하고, 동일한 경우 가장 최신 기록 기준으로 정렬
개별 독서 기록은 항상 primaryEmotion을 가져야 하므로 (entity/domain에서 non-nullable), "감정 정보 없이 기록이 존재하는 경우"는 설계상 발생하지 않습니다.
서비스에서 이 값을
PrimaryEmotionDto?로 변환하고 응답에 전달하는 로직은 모든 케이스에서 일관되고 올바릅니다.apis/src/main/kotlin/org/yapp/apis/readingrecord/usecase/ReadingRecordUseCaseV2.kt (2)
8-8: LGTM - 필요한 import 추가새로운 복합 응답 타입 사용을 위한 적절한 import입니다.
49-63: LGTM - 서비스 레이어 응답 타입 변경 전파Use case 레이어가 서비스 레이어의 새로운 복합 응답 타입을 적절히 전파하고 있습니다. 검증 로직과 위임 로직은 그대로 유지되어 기존 동작이 보존됩니다.
apis/src/main/kotlin/org/yapp/apis/readingrecord/service/ReadingRecordServiceV2.kt (6)
8-10: LGTM!새로운 응답 구조를 위한 필수 import가 정확하게 추가되었습니다.
83-88: LGTM!빈 페이지 케이스에서도 일관된 응답 구조를 유지하는 것이 좋습니다.
90-98: 성능 개선이 잘 적용되었습니다!읽기 기록 ID 목록을 한 번에 추출하고
buildDetailTagsMap을 사용하여 배치로 태그를 조회하는 방식으로 리팩토링되어 N+1 쿼리 문제를 해결했습니다.
100-101: LGTM!null 안전성이 잘 처리된 간단하고 명확한 변환 로직입니다.
120-130: LGTM!페이지 변환 로직이 깔끔하게 구현되었고, 태그가 없는 레코드에 대한 기본값 처리(
?: emptyList())도 적절합니다.
74-78: API 호출자 업데이트 확인 완료메서드의 반환 타입이
Page<ReadingRecordResponseV2>에서ReadingRecordsWithPrimaryEmotionResponse로 변경되었습니다. 모든 호출자가 올바르게 업데이트되었습니다:
ReadingRecordUseCaseV2.getReadingRecordsByUserBookId(): 새로운 응답 타입 처리 ✓ReadingRecordControllerV2.getReadingRecords(): 엔드포인트가 올바른 응답 타입 반환 ✓- V1 API는 별도로 유지되어 영향 없음 ✓
apis/src/main/kotlin/org/yapp/apis/emotion/dto/response/EmotionDetailDto.kt
Show resolved
Hide resolved
apis/src/main/kotlin/org/yapp/apis/readingrecord/dto/response/PrimaryEmotionDto.kt
Show resolved
Hide resolved
apis/src/main/kotlin/org/yapp/apis/readingrecord/service/ReadingRecordServiceV2.kt
Show resolved
Hide resolved
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
Outdated
Show resolved
Hide resolved
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordRepository.kt
Show resolved
Hide resolved
...otlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/ReadingRecordRepositoryImpl.kt
Show resolved
Hide resolved
move-hoon
force-pushed
the
BOOK-486-refactor/#147
branch
from
0751895 to
84899b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApiV2.kt (1)
97-109: API 문서화 일관성을 위해 응답 스키마 추가 권장다른 엔드포인트들은
@ApiResponse에content와schema를 명시적으로 지정하고 있지만,getReadingRecords엔드포인트의 200 응답에는 스키마가 누락되어 있습니다. Swagger 문서의 일관성을 위해 추가하는 것이 좋습니다.♻️ 제안된 수정
@ApiResponses( value = [ ApiResponse( responseCode = "200", - description = "독서 기록 목록 조회 성공" + description = "독서 기록 목록 조회 성공", + content = [Content(schema = Schema(implementation = ReadingRecordsWithPrimaryEmotionResponse::class))] ),
🤖 Fix all issues with AI agents
In @apis/src/main/kotlin/org/yapp/apis/emotion/dto/response/EmotionDetailDto.kt:
- Around line 1-19: Duplicate EmotionDetailDto definitions exist: one as a
top-level data class EmotionDetailDto and another nested inside
EmotionListResponse; remove the duplication by choosing one canonical
definition. Option A: delete this EmotionDetailDto.kt file and update code to
import/use the nested EmotionListResponse.EmotionDetailDto where referenced;
Option B: move the nested class out of EmotionListResponse into this
EmotionDetailDto.kt (make it public) and update EmotionListResponse to
import/use EmotionDetailDto. Ensure all references are updated to the chosen
symbol (EmotionDetailDto or EmotionListResponse.EmotionDetailDto) and remove the
unused class to avoid name collisions.
In
@domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt:
- Line 162: Remove the unnecessary .toList() call: assign the Page content
directly to readingRecords by replacing "val readingRecords =
readingRecordPage.content.toList()" with "val readingRecords =
readingRecordPage.content" inside ReadingRecordDomainService so you use the
existing List<T> returned by readingRecordPage.content.
- Around line 246-249: findOrCreateTags currently issues a query per tag via
tagRepository.findByName and may call tagRepository.save for each missing tag;
change it to batch-fetch existing tags using a new
TagRepository.findByNames(names: List<String>) method, compute which names are
missing, create Tag.create(...) for those, persist them in bulk (e.g. saveAll or
equivalent), and return the combined list of existing + newly saved Tag entities
so findOrCreateTags avoids the N+1 query pattern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
apis/src/main/kotlin/org/yapp/apis/emotion/dto/response/EmotionDetailDto.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApiV2.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerV2.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/dto/response/PrimaryEmotionDto.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/dto/response/ReadingRecordsWithPrimaryEmotionResponse.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/service/ReadingRecordServiceV2.ktapis/src/main/kotlin/org/yapp/apis/readingrecord/usecase/ReadingRecordUseCaseV2.ktdomain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
⏰ 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 (11)
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt (2)
230-256: 헬퍼 메서드 추출 및 리팩토링이 잘 되었습니다.
findReadingRecordOrThrow,findUserBookOrThrow,findOrCreateTags,saveReadingRecordTags헬퍼 메서드 추출을 통해 코드 중복이 줄어들고 가독성이 향상되었습니다. 에러 메시지에 ID를 포함시켜 디버깅에도 도움이 됩니다.
88-89: LGTM!
findPrimaryEmotionByUserBookId메서드가 레포지토리에 깔끔하게 위임되어 있습니다. nullable 반환 타입은 독서 기록이 없는 경우를 적절히 처리합니다.apis/src/main/kotlin/org/yapp/apis/readingrecord/usecase/ReadingRecordUseCaseV2.kt (1)
49-63: LGTM!
getReadingRecordsByUserBookId메서드의 반환 타입이ReadingRecordsWithPrimaryEmotionResponse로 적절히 변경되었습니다. 유스케이스 레이어에서 서비스로의 위임이 깔끔하게 유지되고 있습니다.apis/src/main/kotlin/org/yapp/apis/readingrecord/dto/response/ReadingRecordsWithPrimaryEmotionResponse.kt (1)
1-28: LGTM!복합 응답 DTO가 PR 목표에 맞게 잘 설계되었습니다.
primaryEmotion의 nullable 처리로 독서 기록이 없는 경우도 적절히 대응됩니다.
Page<ReadingRecordResponseV2>가 JSON으로 직렬화될 때 예상대로 동작하는지 확인해 주세요. Spring의 기본PageImpl직렬화는content,pageable,totalElements등의 필드를 포함합니다. 클라이언트 측에서 기대하는 응답 구조와 일치하는지 Swagger/Postman 테스트 결과를 확인하시기 바랍니다.apis/src/main/kotlin/org/yapp/apis/readingrecord/dto/response/PrimaryEmotionDto.kt (1)
1-18: LGTM!
PrimaryEmotionDto가 일관된 패턴으로 잘 구현되었습니다. Swagger 어노테이션의 예시 값("JOY","즐거움")이 API 문서화에 도움이 됩니다.apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerV2.kt (1)
51-66: LGTM!컨트롤러의
getReadingRecords메서드가 인터페이스와 유스케이스의 반환 타입 변경에 맞게 적절히 업데이트되었습니다. 구현이 깔끔하고 일관성 있습니다.apis/src/main/kotlin/org/yapp/apis/readingrecord/service/ReadingRecordServiceV2.kt (5)
8-10: LGTM!새로운 응답 타입을 위한 필수 import가 올바르게 추가되었습니다.
100-101: LGTM!도메인 모델을 DTO로 변환하는 로직이 간결하고 명확합니다. null-safe 처리도 적절합니다.
103-118: N+1 쿼리 방지를 위한 훌륭한 리팩토링!이 헬퍼 메서드는 다음과 같은 장점이 있습니다:
- 벌크 조회를 통해 N+1 쿼리 문제를 효과적으로 방지
associateBy를 사용한 효율적인 룩업 맵 구성distinct()로 중복 쿼리 방지 (106번 라인)mapNotNull을 통한 방어적 프로그래밍으로 누락된 태그 처리PR 목표에 명시된 성능 개선 목적과 잘 부합합니다.
120-130: LGTM!페이지 변환 로직이 깔끔하게 분리되었습니다:
- 미리 구성된 룩업 맵을 효율적으로 활용
- Elvis 연산자로 누락된 태그를 안전하게 처리 (127번 라인)
- 코드 가독성과 유지보수성 향상
79-88: DB 레벨 집계 쿼리 구현 확인됨
findMostFrequentPrimaryEmotion의 구현이 QueryDSL을 통해 DB 레벨에서 GROUP BY와 COUNT 집계를 사용하도록 이미 올바르게 구현되어 있습니다.구현 확인 (JpaReadingRecordQuerydslRepositoryImpl.kt 48-62)
override fun findMostFrequentPrimaryEmotion(userBookId: UUID): String? { return queryFactory .select(readingRecord.primaryEmotion.stringValue()) .from(readingRecord) .where( readingRecord.userBookId.eq(userBookId) .and(readingRecord.deletedAt.isNull()) ) .groupBy(readingRecord.primaryEmotion) .orderBy( readingRecord.count().desc(), readingRecord.createdAt.max().desc() ) .fetchFirst() }대표 감정 조회 순서는 의도된 설계입니다
79번 라인에서 페이지 검증 전에 대표 감정을 조회하는 것은 문제가 아닙니다.
ReadingRecordsWithPrimaryEmotionResponse는 조회 결과가 비어있는 경우(84-87)와 그렇지 않은 경우(94-97) 모두에서primaryEmotion을 항상 포함하는 구조이므로, 이 쿼리는 필수적입니다.
apis/src/main/kotlin/org/yapp/apis/emotion/dto/response/EmotionDetailDto.kt
Show resolved
Hide resolved
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
Show resolved
Hide resolved
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
Show resolved
Hide resolved
move-hoon
force-pushed
the
BOOK-486-refactor/#147
branch
from
aaaa423 to
a1faab4
Compare
move-hoon
force-pushed
the
BOOK-486-refactor/#147
branch
from
a1faab4 to
a0cb5fd
Compare
move-hoon
changed the title
move-hoon
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecord.kt (1)
67-82: PATCH 의미론의 불명확성 - 명시적인 업데이트 패턴 고려 필요현재 구현에서 null 입력값이 필드마다 다른 의미를 가집니다:
pageNumber,review: null 입력 시 필드를 null로 초기화 (Line 75, 77)quote,primaryEmotion,emotionTags: null 입력 시 기존 값 유지이로 인해 두 가지 문제가 발생합니다:
- API 불일치: 동일한 메서드 내에서 null이 "업데이트 안 함"과 "필드 삭제"라는 서로 다른 의미로 사용됩니다.
- 의미 모호성: 클라이언트가 "이 필드를 변경하지 않음"과 "이 필드를 null로 초기화"를 구분할 수 없습니다.
더 명시적인 API 설계를 권장합니다. 예를 들어 sealed class나 wrapper 타입을 사용하여 의도를 명확히 할 수 있습니다:
sealed class UpdateField<T> { class NoChange<T> : UpdateField<T>() data class SetValue<T>(val value: T) : UpdateField<T>() class ClearValue<T> : UpdateField<T>() }또는 별도의 DTO를 사용하여 업데이트 의도를 명확하게 전달할 수 있습니다.
apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApiV2.kt (1)
100-119: ApiResponse 200 응답에 스키마 지정 필요현재
ApiResponse(responseCode = "200")에content와schema속성이 누락되어 있어, OpenAPI/Swagger 문서에 응답 구조가 정확히 표시되지 않습니다. 404 응답처럼 200 응답도 스키마를 명시하여 API 문서의 일관성을 개선해주세요:ApiResponse( responseCode = "200", - description = "독서 기록 목록 조회 성공" + description = "독서 기록 목록 조회 성공", + content = [Content(schema = Schema(implementation = ReadingRecordsWithPrimaryEmotionResponse::class))] ),참고:
ReadingRecordsWithPrimaryEmotionResponse의records필드가Page<ReadingRecordResponseV2>타입으로 유지되고 있어 페이지네이션 메타데이터(totalElements, totalPages 등)는 모두 보존됩니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApiV2.ktdomain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecord.ktdomain/src/main/kotlin/org/yapp/domain/userbook/UserBookDomainService.ktinfra/src/main/kotlin/org/yapp/infra/readingrecorddetailtag/repository/JpaReadingRecordDetailTagRepository.kt
⏰ 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 (3)
apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApiV2.kt (2)
20-20: LGTM - 필요한 import 추가새로운 응답 타입
ReadingRecordsWithPrimaryEmotionResponse를 위한 import가 정확히 추가되었습니다.
152-152: V2 버전 표시 추가로 일관성 개선삭제 작업의 summary에 "(V2)" 표시를 추가하여 다른 작업들과 일관성을 유지했습니다. PR의 주요 목적과는 무관하지만 API 문서의 명확성을 높이는 좋은 개선입니다.
domain/src/main/kotlin/org/yapp/domain/userbook/UserBookDomainService.kt (1)
84-87: 보안 검증이 이미 상위 계층에서 수행되고 있습니다.이 메서드의 보안 우려는 실제로는 타당하지 않습니다.
findById는ReadingRecordServiceV2.buildResponse()에서만 호출되며, 이 메서드는ReadingRecordUseCase.getReadingRecordDetail()등의 UseCase 메서드에서 호출됩니다.
ReadingRecordUseCase에서는buildResponse()를 호출하기 전에 다음과 같이 권한을 검증합니다:
userService.validateUserExists(userId)- 사용자 존재 확인userBookService.validateUserBookExists(userBookId, userId)- 사용자가 해당 UserBook을 소유하는지 확인 (내부적으로existsByUserBookIdAndUserId사용)따라서
findById에 전달되는userBookId는 이미 현재 사용자에게 속함이 확인된 후의 값입니다. 도메인 서비스 레이어에서 추가 userId 검증이 없어도 상위 서비스 레이어의 권한 검증으로 인해 보안이 보장됩니다. 이는 계층별 책임을 명확히 하는 아키텍처 패턴입니다.Likely an incorrect or invalid review comment.
...tlin/org/yapp/infra/readingrecorddetailtag/repository/JpaReadingRecordDetailTagRepository.kt
Show resolved
Hide resolved
중요
|
|
🔗 관련 이슈
📘 작업 유형
📙 작업 내역
1. 독서 기록 목록 API 응답에 대표 감정 필드 추가
Page<ReadingRecordResponseV2>→ReadingRecordsWithPrimaryEmotionResponseReadingRecordsWithPrimaryEmotionResponse구조:{ "primaryEmotion": { "code": "JOY", "displayName": "기쁨" }, "records": { ... } // 기존 페이징 응답 }2. 대표 감정 계산 성능 최적화 (N+1/메모리 이슈 해결)
3. 코드 품질 개선 및 리팩토링
findReadingRecordOrThrow등), 불필요한 V1/V2 중복 로직 통합🧪 테스트 내역
🎨 스크린샷 또는 시연 영상 (선택)
✅ PR 체크리스트
💬 추가 설명 or 리뷰 포인트 (선택)
apis모듈의ReadingRecordControllerApiV2반환 타입 변경에 따른 영향도를 확인해주세요.ReadingRecordDomainService리팩토링 과정에서 기존 로직과의 동일성을 신경 썼습니다.Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Behavior
✏️ Tip: You can customize this high-level summary in your review settings.