feat: 독서기록상세조회 기능을 개발합니다 #68
Merged
Conversation
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
📝 WalkthroughWalkthrough독서기록 상세 조회 기능이 도입되었습니다. 컨트롤러, API 인터페이스, 서비스, 유스케이스, 도메인 서비스에 상세 조회 메서드가 추가되었으며, 관련 예외 및 에러 코드도 신설되었습니다. 새로운 GET 엔드포인트( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant UseCase
participant Service
participant DomainService
participant Repository
Client->>Controller: GET /detail/{readingRecordId}
Controller->>UseCase: getReadingRecordDetail(userId, readingRecordId)
UseCase->>Service: getReadingRecordDetail(userId, readingRecordId)
Service->>DomainService: findReadingRecordById(readingRecordId)
DomainService->>Repository: findById(readingRecordId)
DomainService->>Repository: findTagsByReadingRecordId
DomainService->>Repository: findUserBookById
DomainService-->>Service: ReadingRecordInfoVO
Service->>Service: validateUserBookExists(userId, userBookId)
Service-->>UseCase: ReadingRecordResponse
UseCase-->>Controller: ReadingRecordResponse
Controller-->>Client: 200 OK + ReadingRecordResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(해당 사항 없음) Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordController.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApi.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/readingrecord/exception/ReadingRecordErrorCode.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/readingrecord/exception/ReadingRecordNotFoundException.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/readingrecord/service/ReadingRecordService.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/readingrecord/usecase/ReadingRecordUseCase.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.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 (6)
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt (1)
69-69: UserBook 조회 시 예외 처리 필요성 검토domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt:69
- 현재
userBookRepository.findById(readingRecord.userBookId.value)호출 결과를userBook?.title등으로 null-safe하게 처리하고 있는데,
비즈니스 로직 상 해당 UserBook이 항상 존재해야 한다면 null 반환 시 예외를 던지거나 별도의 처리(flow)를 명시적으로 구현하는 것이 좋습니다.- 예를 들어,
• 존재하지 않을 때UserBookNotFoundException등을 던지거나
•findByIdAndUserId또는userBookService.validateUserBookExists(userId, userBookId)호출로 소유권(유효성) 검증 로직을 추가하는 방안을 검토해주세요.apis/src/main/kotlin/org/yapp/apis/readingrecord/exception/ReadingRecordNotFoundException.kt (1)
1-8: 잘 구현된 예외 클래스입니다.
CommonException을 상속받아 일관된 예외 처리 패턴을 따르고 있으며, 타입 안전한 에러 코드와 선택적 메시지를 지원하는 깔끔한 구현입니다.apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordController.kt (1)
44-54: REST API 엔드포인트가 잘 구현되었습니다.Spring Security의
@AuthenticationPrincipal을 통한 사용자 인증, 적절한 HTTP 메서드 사용, 그리고 Use Case로의 위임이 올바르게 구현되어 있습니다. REST 규칙을 잘 따르고 있습니다.apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApi.kt (1)
63-85: OpenAPI 문서화가 훌륭하게 작성되었습니다.API 명세가 완전하고 명확하며, 한국어 설명과 적절한 HTTP 응답 코드가 잘 정의되어 있습니다. 기존 코드베이스의 패턴을 일관되게 따르고 있습니다.
apis/src/main/kotlin/org/yapp/apis/readingrecord/service/ReadingRecordService.kt (1)
9-10: 새로운 예외 처리를 위한 import 추가가 적절합니다.독서 기록 상세 조회 기능을 위한 예외 클래스들이 올바르게 import되었습니다.
apis/src/main/kotlin/org/yapp/apis/readingrecord/exception/ReadingRecordErrorCode.kt (1)
1-16: 독서 기록 에러 코드 구현이 우수합니다.새로운
ReadingRecordErrorCodeenum 클래스가 잘 구현되었습니다:
BaseErrorCode인터페이스를 올바르게 구현- HTTP 상태 코드(404)가 적절함
- 에러 코드 네이밍 규칙("READING_RECORD_001")을 준수
- 한국어 에러 메시지가 사용자 친화적
- Kotlin enum 모범 사례를 따름
| ) | ||
| ] | ||
| ) | ||
| @GetMapping("/detail/{readingRecordId}") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
API 경로 구조에 대한 제안사항입니다.
현재 /detail/{readingRecordId} 경로는 기능적으로 올바르지만, RESTful 설계 관점에서 /{readingRecordId}만으로도 충분할 수 있습니다. 다른 리소스와의 일관성을 고려해보세요.
-@GetMapping("/detail/{readingRecordId}")
+@GetMapping("/{readingRecordId}")하지만 기존 /userBookId 엔드포인트와의 충돌을 피하기 위해 현재 구조가 더 명확할 수 있습니다.
📝 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
| @GetMapping("/detail/{readingRecordId}") | |
| @GetMapping("/{readingRecordId}") |
🤖 Prompt for AI Agents
In
apis/src/main/kotlin/org/yapp/apis/readingrecord/controller/ReadingRecordControllerApi.kt
at line 81, the API endpoint uses the path "/detail/{readingRecordId}" which is
functionally correct but could be simplified to "/{readingRecordId}" for better
RESTful design consistency. Review other endpoints like "/userBookId" to ensure
no conflicts arise, and if safe, rename the mapping to just "/{readingRecordId}"
to align with RESTful conventions and improve path clarity.
Comment on lines
+43
to
+56
| fun getReadingRecordDetail( | ||
| userId: UUID, | ||
| readingRecordId: UUID | ||
| ): ReadingRecordResponse { | ||
| val readingRecordInfoVO = readingRecordDomainService.findReadingRecordById(readingRecordId) | ||
| ?: throw ReadingRecordNotFoundException( | ||
| ReadingRecordErrorCode.READING_RECORD_NOT_FOUND, | ||
| "Reading record not found with id: $readingRecordId" | ||
| ) | ||
|
|
||
| userBookService.validateUserBookExists(userId, readingRecordInfoVO.userBookId.value) | ||
|
|
||
| return ReadingRecordResponse.from(readingRecordInfoVO) | ||
| } |
There was a problem hiding this comment.
보안 취약점: 인가되지 않은 사용자에게 정보 누출 가능성이 있습니다.
현재 구현에서는 독서 기록 존재 여부를 먼저 확인한 후 사용자 권한을 검증하는데, 이는 권한이 없는 사용자에게도 특정 ID의 독서 기록 존재 여부를 알려줄 수 있습니다.
다음과 같이 수정하여 보안을 강화하는 것을 권장합니다:
fun getReadingRecordDetail(
userId: UUID,
readingRecordId: UUID
): ReadingRecordResponse {
val readingRecordInfoVO = readingRecordDomainService.findReadingRecordById(readingRecordId)
?: throw ReadingRecordNotFoundException(
ReadingRecordErrorCode.READING_RECORD_NOT_FOUND,
"Reading record not found with id: $readingRecordId"
)
- userBookService.validateUserBookExists(userId, readingRecordInfoVO.userBookId.value)
+ // 사용자 권한 검증을 먼저 수행하여 정보 누출 방지
+ try {
+ userBookService.validateUserBookExists(userId, readingRecordInfoVO.userBookId.value)
+ } catch (e: Exception) {
+ // 권한이 없는 경우에도 동일한 예외를 던져 정보 누출 방지
+ throw ReadingRecordNotFoundException(
+ ReadingRecordErrorCode.READING_RECORD_NOT_FOUND,
+ "Reading record not found with id: $readingRecordId"
+ )
+ }
return ReadingRecordResponse.from(readingRecordInfoVO)
}🤖 Prompt for AI Agents
In
apis/src/main/kotlin/org/yapp/apis/readingrecord/service/ReadingRecordService.kt
around lines 43 to 56, the current code checks for the existence of the reading
record before validating user authorization, which can leak information about
the record's existence to unauthorized users. To fix this, first call
userBookService.validateUserBookExists with the userId and userBookId to verify
the user's permission, then retrieve the reading record by ID. This way,
unauthorized users cannot infer the existence of records they shouldn't access.
Comment on lines
+45
to
+55
| fun getReadingRecordDetail( | ||
| userId: UUID, | ||
| readingRecordId: UUID | ||
| ): ReadingRecordResponse { | ||
| userAuthService.validateUserExists(userId) | ||
|
|
||
| return readingRecordService.getReadingRecordDetail( | ||
| userId = userId, | ||
| readingRecordId = readingRecordId | ||
| ) | ||
| } |
There was a problem hiding this comment.
보안 취약점: 사용자 권한 검증이 누락되었습니다.
현재 구현에서는 사용자 존재 여부만 확인하고 있어, 다른 사용자의 독서기록에 접근할 수 있는 보안 취약점이 있습니다. 독서기록 소유권 검증이 필요합니다.
다음과 같이 수정하여 보안을 강화하세요:
fun getReadingRecordDetail(
userId: UUID,
readingRecordId: UUID
): ReadingRecordResponse {
userAuthService.validateUserExists(userId)
+
+ // 독서기록 소유권 검증 추가 필요
+ // readingRecordService에서 userId와 readingRecord의 소유자가 일치하는지 확인
return readingRecordService.getReadingRecordDetail(
userId = userId,
readingRecordId = readingRecordId
)
}또는 service layer에서 소유권 검증이 이루어지고 있는지 확인해주세요.
📝 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
| fun getReadingRecordDetail( | |
| userId: UUID, | |
| readingRecordId: UUID | |
| ): ReadingRecordResponse { | |
| userAuthService.validateUserExists(userId) | |
| return readingRecordService.getReadingRecordDetail( | |
| userId = userId, | |
| readingRecordId = readingRecordId | |
| ) | |
| } | |
| fun getReadingRecordDetail( | |
| userId: UUID, | |
| readingRecordId: UUID | |
| ): ReadingRecordResponse { | |
| userAuthService.validateUserExists(userId) | |
| // 독서기록 소유권 검증 추가 필요 | |
| // readingRecordService에서 userId와 readingRecord의 소유자가 일치하는지 확인 | |
| return readingRecordService.getReadingRecordDetail( | |
| userId = userId, | |
| readingRecordId = readingRecordId | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
apis/src/main/kotlin/org/yapp/apis/readingrecord/usecase/ReadingRecordUseCase.kt
between lines 45 and 55, the method getReadingRecordDetail only validates that
the user exists but does not verify if the user owns the requested reading
record, creating a security vulnerability. To fix this, add a check to confirm
that the reading record belongs to the user before returning the detail, either
by adding ownership validation in this use case method or ensuring that the
service layer method getReadingRecordDetail performs this ownership verification
and throws an error if the user is unauthorized.
Comment on lines
62
to
78
| fun findReadingRecordById(readingRecordId: UUID): ReadingRecordInfoVO? { | ||
| val readingRecord = readingRecordRepository.findById(readingRecordId) ?: return null | ||
|
|
||
| val readingRecordTags = readingRecordTagRepository.findByReadingRecordId(readingRecord.id.value) | ||
| val tagIds = readingRecordTags.map { it.tagId.value } | ||
| val tags = tagRepository.findByIds(tagIds) | ||
|
|
||
| val userBook = userBookRepository.findById(readingRecord.userBookId.value) | ||
|
|
||
| return ReadingRecordInfoVO.newInstance( | ||
| readingRecord = readingRecord, | ||
| emotionTags = tags.map { it.name }, | ||
| bookTitle = userBook?.title, | ||
| bookPublisher = userBook?.publisher, | ||
| bookCoverImageUrl = userBook?.coverImageUrl | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
성능 최적화를 위한 쿼리 개선이 필요합니다.
현재 구현에서 여러 개의 개별 데이터베이스 쿼리가 순차적으로 실행되어 N+1 문제가 발생할 수 있습니다. 또한 기존 findReadingRecordsByDynamicCondition 메서드와 유사한 로직이 중복되고 있습니다.
다음과 같은 개선을 제안합니다:
fun findReadingRecordById(readingRecordId: UUID): ReadingRecordInfoVO? {
val readingRecord = readingRecordRepository.findById(readingRecordId) ?: return null
- val readingRecordTags = readingRecordTagRepository.findByReadingRecordId(readingRecord.id.value)
- val tagIds = readingRecordTags.map { it.tagId.value }
- val tags = tagRepository.findByIds(tagIds)
-
- val userBook = userBookRepository.findById(readingRecord.userBookId.value)
-
- return ReadingRecordInfoVO.newInstance(
- readingRecord = readingRecord,
- emotionTags = tags.map { it.name },
- bookTitle = userBook?.title,
- bookPublisher = userBook?.publisher,
- bookCoverImageUrl = userBook?.coverImageUrl
- )
+ return buildReadingRecordInfoVO(readingRecord)
+ }
+
+ private fun buildReadingRecordInfoVO(readingRecord: ReadingRecord): ReadingRecordInfoVO {
+ val readingRecordTags = readingRecordTagRepository.findByReadingRecordId(readingRecord.id.value)
+ val tagIds = readingRecordTags.map { it.tagId.value }
+ val tags = tagRepository.findByIds(tagIds)
+ val userBook = userBookRepository.findById(readingRecord.userBookId.value)
+
+ return ReadingRecordInfoVO.newInstance(
+ readingRecord = readingRecord,
+ emotionTags = tags.map { it.name },
+ bookTitle = userBook?.title,
+ bookPublisher = userBook?.publisher,
+ bookCoverImageUrl = userBook?.coverImageUrl
+ )
}📝 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
| fun findReadingRecordById(readingRecordId: UUID): ReadingRecordInfoVO? { | |
| val readingRecord = readingRecordRepository.findById(readingRecordId) ?: return null | |
| val readingRecordTags = readingRecordTagRepository.findByReadingRecordId(readingRecord.id.value) | |
| val tagIds = readingRecordTags.map { it.tagId.value } | |
| val tags = tagRepository.findByIds(tagIds) | |
| val userBook = userBookRepository.findById(readingRecord.userBookId.value) | |
| return ReadingRecordInfoVO.newInstance( | |
| readingRecord = readingRecord, | |
| emotionTags = tags.map { it.name }, | |
| bookTitle = userBook?.title, | |
| bookPublisher = userBook?.publisher, | |
| bookCoverImageUrl = userBook?.coverImageUrl | |
| ) | |
| } | |
| fun findReadingRecordById(readingRecordId: UUID): ReadingRecordInfoVO? { | |
| val readingRecord = readingRecordRepository.findById(readingRecordId) ?: return null | |
| return buildReadingRecordInfoVO(readingRecord) | |
| } | |
| private fun buildReadingRecordInfoVO(readingRecord: ReadingRecord): ReadingRecordInfoVO { | |
| val readingRecordTags = readingRecordTagRepository.findByReadingRecordId(readingRecord.id.value) | |
| val tagIds = readingRecordTags.map { it.tagId.value } | |
| val tags = tagRepository.findByIds(tagIds) | |
| val userBook = userBookRepository.findById(readingRecord.userBookId.value) | |
| return ReadingRecordInfoVO.newInstance( | |
| readingRecord = readingRecord, | |
| emotionTags = tags.map { it.name }, | |
| bookTitle = userBook?.title, | |
| bookPublisher = userBook?.publisher, | |
| bookCoverImageUrl = userBook?.coverImageUrl | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
around lines 62 to 78, the method findReadingRecordById executes multiple
sequential database queries causing potential N+1 query issues and duplicates
logic similar to findReadingRecordsByDynamicCondition. To fix this, refactor the
method to fetch all necessary data with fewer queries by using joins or batch
fetching strategies, and consolidate overlapping logic with existing methods to
avoid redundancy and improve performance.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🔗 관련 이슈
📘 작업 유형
📙 작업 내역
GET /detail/{readingRecordId})ReadingRecordService에 상세 조회 로직 추가ReadingRecordDomainService에findReadingRecordById()메서드 구현ReadingRecordNotFoundException,ReadingRecordErrorCode정의 및 예외 처리 적용🧪 테스트 내역
🎨 스크린샷 또는 시연 영상 (선택)
✅ PR 체크리스트
💬 추가 설명 or 리뷰 포인트
Summary by CodeRabbit