feat: 도서 등록 및 도서 목록 조회 #31
Conversation
📝 WalkthroughWalkthrough내 서재(유저 도서 등록/조회) 기능이 도메인, 인프라, API 계층에 걸쳐 전면적으로 추가되었습니다. 도메인에는 UserBook 엔티티 및 관련 서비스가 신설되고, API 및 서비스 계층에는 도서 등록/조회 엔드포인트와 DTO, 예외처리, 서비스 구현이 반영되었습니다. 기존 인증 방식은 @AuthenticationPrincipal 기반으로 일원화되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BookController
participant BookUseCase
participant UserAuthService
participant BookQueryService
participant BookManagementService
participant UserBookService
User->>BookController: PUT /upsert (UserBookRegisterRequest)
BookController->>BookUseCase: upsertBookToMyLibrary(userId, request)
BookUseCase->>UserAuthService: validateUserExists(userId)
BookUseCase->>BookQueryService: getBookDetail(isbn)
BookUseCase->>BookManagementService: findOrCreateBook(bookDetail)
BookUseCase->>UserBookService: upsertUserBook(userId, book, status)
UserBookService-->>BookUseCase: UserBook
BookUseCase-->>BookController: UserBookResponse
BookController-->>User: UserBookResponse
User->>BookController: GET /my-library
BookController->>BookUseCase: getUserLibraryBooks(userId)
BookUseCase->>UserAuthService: validateUserExists(userId)
BookUseCase->>UserBookService: findAllUserBooks(userId)
UserBookService-->>BookUseCase: List<UserBook>
BookUseCase-->>BookController: List<UserBookResponse>
BookController-->>User: List<UserBookResponse>
Assessment against linked issues
Suggested reviewers
✨ Finishing Touches
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: 26
🔭 Outside diff range comments (1)
apis/src/main/kotlin/org/yapp/apis/book/dto/response/BookDetailResponse.kt (1)
40-40: 빈 문자열 기본값 처리를 재검토해주세요.null 값을 빈 문자열로 대체하는 것은 데이터 무결성에 영향을 줄 수 있습니다. 실제 null 정보가 손실될 수 있으니, 의미있는 기본값을 사용하거나 명시적으로 null을 처리하는 것을 고려해보세요.
-author = bookItem.author ?: "", +author = bookItem.author ?: "저자 미상", -cover = bookItem.cover ?: "", +cover = bookItem.cover ?: "표지 없음", -publisher = bookItem.publisher ?: "", +publisher = bookItem.publisher ?: "출판사 미상",Also applies to: 51-51, 54-54
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
apis/src/main/kotlin/org/yapp/apis/book/controller/BookController.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookDetailRequest.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/dto/request/UserBookRegisterRequest.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/dto/response/BookDetailResponse.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/book/dto/response/UserBookResponse.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/exception/BookErrorCode.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/exception/BookException.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/helper/AladinApiHelper.kt(0 hunks)apis/src/main/kotlin/org/yapp/apis/book/service/BookService.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/service/UserBookService.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/usecase/BookUseCase.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/book/Book.kt(2 hunks)domain/src/main/kotlin/org/yapp/domain/book/BookRepository.kt(0 hunks)domain/src/main/kotlin/org/yapp/domain/book/BookStatus.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/book/UserBook.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/book/UserBookRepository.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/service/domain/BookDomainService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/service/domain/UserBookDomainService.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/book/entity/BookEntity.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/user/entity/UserBookEntity.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/user/repository/JpaUserBookRepository.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/user/repository/impl/UserBookRepositoryImpl.kt(1 hunks)
💤 Files with no reviewable changes (2)
- apis/src/main/kotlin/org/yapp/apis/book/helper/AladinApiHelper.kt
- domain/src/main/kotlin/org/yapp/domain/book/BookRepository.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 (31)
domain/src/main/kotlin/org/yapp/domain/book/BookStatus.kt (1)
3-7: 깔끔한 도서 상태 enum 구현입니다.도서 읽기 상태를 명확하게 구분하고 한국어 주석으로 가독성을 높였습니다. 네이밍과 구조가 적절합니다.
apis/src/main/kotlin/org/yapp/apis/book/exception/BookException.kt (1)
6-9: 예외 클래스 구조가 적절합니다.CommonException을 상속하고 BookErrorCode를 활용하는 구조가 일관성 있게 설계되었습니다.
apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookDetailRequest.kt (1)
26-34: 편리한 팩토리 메소드 추가입니다.ISBN 기반 도서 상세 요청을 쉽게 생성할 수 있는 유틸리티 메소드가 잘 구현되었습니다. itemIdType을 "ISBN"으로 고정한 것도 현재 사용 사례에 적합해 보입니다.
apis/src/main/kotlin/org/yapp/apis/book/dto/response/BookDetailResponse.kt (1)
14-14: Non-null 속성 변경을 검토해주세요.
author,cover,publisher속성을 non-nullable로 변경한 것은 일관성 측면에서 좋습니다.Also applies to: 25-25, 28-28
apis/src/main/kotlin/org/yapp/apis/book/exception/BookErrorCode.kt (1)
7-23: 에러 코드 정의가 적절합니다.도서 관련 에러 코드들이 명확하게 정의되어 있고, HTTP 상태 코드와 메시지가 적절합니다. BaseErrorCode 인터페이스 구현도 올바릅니다.
infra/src/main/kotlin/org/yapp/infra/user/repository/JpaUserBookRepository.kt (1)
7-10: JPA 저장소 인터페이스가 올바르게 정의되었습니다.메서드 시그니처가 JPA 네이밍 규칙을 준수하고, 반환 타입이 적절합니다. 도메인 요구사항에 필요한 쿼리 메서드들이 명확하게 정의되어 있습니다.
domain/src/main/kotlin/org/yapp/domain/book/UserBookRepository.kt (1)
7-14: 저장소 인터페이스가 적절하게 정의되었습니다.도메인 저장소 인터페이스의 메서드 시그니처가 명확하고, 필요한 CRUD 연산들이 잘 정의되어 있습니다. DDD 아키텍처에서 도메인 계층의 저장소 추상화가 올바르게 구현되었습니다.
domain/src/main/kotlin/org/yapp/domain/service/domain/BookDomainService.kt (1)
7-23: 도메인 서비스 구조가 적절합니다.BookDomainService가 DDD 원칙에 따라 잘 구현되었습니다. 팩토리 패턴을 통한 Book 생성 위임과 저장소 추상화가 올바르게 적용되어 있습니다.
infra/src/main/kotlin/org/yapp/infra/book/entity/BookEntity.kt (2)
40-67: 엔티티 매핑 메서드들이 올바르게 구현되었습니다.
toDomain()과fromDomain()메서드가 도메인 모델과 엔티티 간의 변환을 적절히 처리하고 있습니다. BaseTimeEntity의 필드들도 올바르게 매핑되어 있습니다.
25-34: DB 마이그레이션 파일 및 외부 API 매핑에서 non-nullable 필드 확인 필요BookEntity의
author,publisher,coverImageUrl를 nullable→non-nullable로 변경하면서 아래 부분을 반드시 검토해야 합니다:
- infra/src/main/resources/db/changelog/** 에 있는 마이그레이션 파일에서
–author,publisher,cover_image_url컬럼의 nullable 속성이false로 설정되었는지 확인
– 기존 데이터에 null 값이 남아있다면 적절한 마이그레이션(기본값 부여 혹은 데이터 정제) 수행- apis/src/main/kotlin/org/yapp/apis/** (Aladin 연동 매퍼 등)에서
– 외부 API 응답에author/publisher/coverImageUrl값이 항상 제공되는지 검증
– null이 올 경우 예외 처리 혹은 기본값 할당 로직이 필요한지 검토- domain/src/main/kotlin/org/yapp/domain/book/Book.kt 의
create/reconstruct메서드에서
– non-nullable 파라미터로 바로 전달되기 때문에 null 유입 시 NPE 발생 여부 확인위 사항들을 직접 검증 후 스키마·마이그레이션과 매퍼 코드를 업데이트 해주시기 바랍니다.
apis/src/main/kotlin/org/yapp/apis/book/service/UserBookService.kt (1)
9-18: 애플리케이션 서비스 구조가 DDD 원칙에 맞습니다.도메인 서비스를 래핑하는 얇은 애플리케이션 서비스 레이어가 올바르게 구현되었습니다. 계층 간 책임 분리가 명확하게 이루어져 있습니다.
domain/src/main/kotlin/org/yapp/domain/service/domain/UserBookDomainService.kt (3)
10-13: 올바른 DDD 패턴 구현
@DomainService어노테이션을 사용하여 도메인 서비스임을 명확히 표시하고, 리포지토리 의존성을 생성자 주입으로 받는 구조가 DDD 원칙에 잘 맞습니다.
15-24: 효과적인 upsert 로직 구현기존 데이터 조회 후 업데이트 또는 신규 생성하는 upsert 패턴이 명확하고 효율적으로 구현되어 있습니다. 도메인 로직이 잘 캡슐화되어 있습니다.
26-28: 간결한 조회 메서드사용자별 도서 목록 조회 로직이 리포지토리에 적절히 위임되어 있어 단순하고 명확합니다.
apis/src/main/kotlin/org/yapp/apis/book/dto/response/UserBookResponse.kt (1)
7-18: 적절한 DTO 구조 설계private constructor와 companion object factory 패턴을 사용하여 객체 생성을 제어하는 구조가 잘 설계되었습니다.
apis/src/main/kotlin/org/yapp/apis/book/controller/BookController.kt (3)
6-7: 적절한 의존성 추가사용자 인증을 위한
AuthUseCase의존성과AuthUtils임포트가 새로운 인증 기능에 맞게 적절히 추가되었습니다.Also applies to: 14-14, 21-21
38-46: 도서 등록/수정 API 적절한 구현Authorization 헤더를 통한 사용자 인증, 요청 데이터 검증, 그리고 use case 호출이 올바른 순서로 구현되어 있습니다. REST 규약을 잘 따르고 있습니다.
48-55: 사용자 도서 목록 조회 API 올바른 구현사용자 인증 후 개인 도서 목록을 조회하는 로직이 명확하고 간결하게 구현되어 있습니다.
apis/src/main/kotlin/org/yapp/apis/book/service/BookService.kt (3)
3-4: 적절한 로깅 설정KotlinLogging을 사용하여 로깅을 설정하고
@Service어노테이션으로 스프링 컴포넌트임을 명시한 것이 좋습니다.Also applies to: 17-17
40-47: 적절한 외부 API 에러 핸들링외부 API 호출 실패에 대한 예외 처리와 로깅이 잘 구현되어 있습니다. 에러 메시지도 명확하고 디버깅에 도움이 됩니다.
22-23: ISBN 검증 로직 확인알라딘 API 응답에서 ISBN을 가져오는 로직이 적절하게 구현되어 있고, ISBN이 없는 경우 명확한 에러 메시지와 함께 예외를 발생시키는 것이 좋습니다.
infra/src/main/kotlin/org/yapp/infra/user/repository/impl/UserBookRepositoryImpl.kt (3)
1-13: 리포지토리 구현체 구조가 올바르게 설계되었습니다.DDD 원칙에 따라 도메인 인터페이스를 구현하고 인프라스트럭처 계층에서 JPA 리포지토리를 활용하는 구조가 적절합니다.
15-18: 널 안전성 처리가 적절합니다.엘비스 연산자를 사용하여 nullable 엔티티를 안전하게 도메인 객체로 변환하고 있습니다.
20-22: UserBookEntity 매핑 로직 검증 완료
toDomain과 fromDomain 메서드가 생성자 필드(id,userId,bookIsbn,coverImageUrl,publisher,title,author,status)와 타임스탬프(createdAt,updatedAt,deletedAt)를 모두 올바르게 매핑하고 있어 데이터 일관성에 문제가 없습니다.domain/src/main/kotlin/org/yapp/domain/book/Book.kt (1)
11-14: Author, Publisher, CoverImageUrl non-null 변경에 따른 매핑 코드 널 처리 확인 완료API 레이어(
BookDetailResponse)에서 이미 다음과 같이?: ""로 널 처리가 적용되어 있어 non-null 도메인 모델로 안전하게 전달됩니다.
- apis/src/main/kotlin/org/yapp/apis/book/dto/response/BookDetailResponse.kt
• author = bookItem.author ?: ""
• publisher= bookItem.publisher ?: ""
• cover = bookItem.cover ?: ""해당 변경은 데이터 무결성 관점에서 적절하며, 더 이상의 수정이 필요 없습니다.
apis/src/main/kotlin/org/yapp/apis/book/usecase/BookUseCase.kt (2)
19-24: 의존성 주입 구조가 적절합니다.유스케이스에서 필요한 서비스들을 적절히 주입받고 있으며, 계층 간 의존성 방향이 DDD 원칙에 부합합니다.
34-42: 트랜잭션 경계 설정이 올바릅니다.도서 생성과 사용자 도서 등록을 하나의 트랜잭션으로 처리하는 것이 적절합니다. 사용자 인증 확인도 포함되어 있어 보안성이 확보되었습니다.
infra/src/main/kotlin/org/yapp/infra/user/entity/UserBookEntity.kt (3)
12-14: 소프트 삭제 구현이 올바릅니다.
@SQLDelete어노테이션을 사용하여 물리적 삭제 대신 논리적 삭제를 구현한 것이 적절합니다.
46-58: 도메인 변환 메서드 구현이 적절합니다.모든 필드를 올바르게 매핑하여 도메인 객체로 변환하고 있습니다.
60-66: equals/hashCode 구현이 올바릅니다.JPA 엔티티에서 ID 기반으로 동등성을 비교하는 것이 적절합니다.
domain/src/main/kotlin/org/yapp/domain/book/UserBook.kt (1)
1-1: 패키지 구조가 DDD 원칙에 부합합니다.UserBook 도메인 모델을 별도의 패키지로 분리한 것이 적절합니다.
apis/src/main/kotlin/org/yapp/apis/book/exception/BookException.kt
Outdated
Show resolved
Hide resolved
apis/src/main/kotlin/org/yapp/apis/book/exception/BookErrorCode.kt
Outdated
Show resolved
Hide resolved
| package org.yapp.domain.book | ||
|
|
||
| import org.yapp.domain.userbook.UserBook |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
패키지 구조 일관성을 확인해주세요.
도메인 저장소 인터페이스가 org.yapp.domain.book 패키지에 위치하는데, UserBook 도메인 모델을 org.yapp.domain.userbook 패키지에서 import하고 있습니다. PR 요약에서 언급된 패키지 재구성과 일치하는지 확인이 필요합니다.
🏁 Script executed:
#!/bin/bash
# 설명: UserBook 도메인 모델과 관련 클래스들의 패키지 구조 일관성 확인
echo "=== UserBook 도메인 모델 위치 확인 ==="
fd -t f -e kt "UserBook" domain/src/main/kotlin/
echo "=== UserBook 관련 클래스들의 import 구조 확인 ==="
rg -A 3 "import.*UserBook" domain/src/main/kotlin/
echo "=== domain.book vs domain.userbook 패키지 구조 확인 ==="
find domain/src/main/kotlin/org/yapp/domain -type d -name "*book*" 2>/dev/null || echo "패키지 구조를 찾을 수 없습니다."Length of output: 1563
패키지 경로 수정 필요
UserBook 도메인 모델이 실제로는 org.yapp.domain.book 패키지에 위치하므로, 아래처럼 import 경로를 수정해주세요.
- 파일:
domain/src/main/kotlin/org/yapp/domain/book/UserBookRepository.kt- import org.yapp.domain.userbook.UserBook + import org.yapp.domain.book.UserBook
📝 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.
| package org.yapp.domain.book | |
| import org.yapp.domain.userbook.UserBook | |
| package org.yapp.domain.book | |
| -import org.yapp.domain.userbook.UserBook | |
| +import org.yapp.domain.book.UserBook |
🤖 Prompt for AI Agents
In domain/src/main/kotlin/org/yapp/domain/book/UserBookRepository.kt at the top
import statements, the import path for UserBook is incorrect. Change the import
from org.yapp.domain.userbook.UserBook to org.yapp.domain.book.UserBook to match
the actual package location of the UserBook domain model.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| fun findOrCreateBook(isbn: String, bookFactory: (String) -> Book): Book { | ||
| return bookRepository.findByIsbn(isbn) ?: run { | ||
| val newBook = bookFactory(isbn) | ||
| bookRepository.save(newBook) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
불필요한 run 블록을 제거하여 코드를 간소화하세요.
Elvis 연산자 우측에 run 블록이 불필요합니다. 직접 블록을 사용하는 것이 더 간결합니다.
- fun findOrCreateBook(isbn: String, bookFactory: (String) -> Book): Book {
- return bookRepository.findByIsbn(isbn) ?: run {
- val newBook = bookFactory(isbn)
- bookRepository.save(newBook)
- }
- }
+ fun findOrCreateBook(isbn: String, bookFactory: (String) -> Book): Book {
+ return bookRepository.findByIsbn(isbn) ?: bookFactory(isbn).also {
+ bookRepository.save(it)
+ }
+ }또는 더 명확하게:
- fun findOrCreateBook(isbn: String, bookFactory: (String) -> Book): Book {
- return bookRepository.findByIsbn(isbn) ?: run {
- val newBook = bookFactory(isbn)
- bookRepository.save(newBook)
- }
- }
+ fun findOrCreateBook(isbn: String, bookFactory: (String) -> Book): Book {
+ return bookRepository.findByIsbn(isbn) ?: {
+ val newBook = bookFactory(isbn)
+ bookRepository.save(newBook)
+ }()
+ }📝 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.
| fun findOrCreateBook(isbn: String, bookFactory: (String) -> Book): Book { | |
| return bookRepository.findByIsbn(isbn) ?: run { | |
| val newBook = bookFactory(isbn) | |
| bookRepository.save(newBook) | |
| } | |
| } | |
| fun findOrCreateBook(isbn: String, bookFactory: (String) -> Book): Book { | |
| return bookRepository.findByIsbn(isbn) ?: bookFactory(isbn).also { | |
| bookRepository.save(it) | |
| } | |
| } |
🤖 Prompt for AI Agents
In domain/src/main/kotlin/org/yapp/domain/service/domain/BookDomainService.kt
lines 12 to 17, remove the unnecessary run block used on the right side of the
Elvis operator. Instead of using ?: run { ... }, directly return the result of
bookRepository.save(bookFactory(isbn)) to simplify and clarify the code.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
현재 로직상으로는 사실 아래처럼 run 없이 간단하게 써도 큰 문제는 없어요.
return bookRepository.findByIsbn(isbn)
?: bookRepository.save(bookFactory(isbn))
다만 bookFactory로 만든 객체를 변수로 받아야 했기 때문에 run을 써서
중간에 연산을 통해 저장가능하도록 했어요!
There was a problem hiding this comment.
fun findOrCreateBook(isbn: String): Book {
return bookRepository.findByIsbn(isbn)
?: bookRepository.save(Book.create(.....))
}- 저는 사실 bookFactory라는 전략 함수?에 대해서 익숙하지가 않아서 위 코드가 더 쉽게 느껴지는데, 혹시 bookFactory로 만들었을 때 장점에는 어떤게 있을까요?
- 추가적으로 isbn만 인자로 받는데 없을 경우 새로운 Book 객체를 만든다는 흐름이 논리적으로 잘 이해가 안됩니다. create 메서드로 호출할 때만 해도 총 7개의 인자를 받아야지만 Book 객체가 생성이 되더라고요....
re: bookFactory.createBook()는 이미 조립된 키트를 받아오는 느낌으로 생각해주시면 되는데
책임 분리라고 생각해주시면 될 것 같아요! findOrCreateBook()는 조회 + 조건부 저장에만 집중하고 어떻게 생성할지는 외부에서 결정 수 있게 되는데
사실 밑에 말씀해주신 service에서 create 인자 하나하나 넣어서 하는 방식이 아닌 VO로 감싸서 하는 방식이면 book인지 하나만 넘기면 되서 factory부분은 안쓸 것 같네요!
apis/src/main/kotlin/org/yapp/apis/book/service/UserBookService.kt
Outdated
Show resolved
Hide resolved
| @Operation( | ||
| summary = "서재에 책 등록 또는 상태 업데이트 (Upsert)", // 요약 변경 | ||
| description = "사용자의 서재에 책을 등록하거나, 이미 등록된 책의 상태를 업데이트합니다." // 설명 변경 | ||
| ) | ||
| @ApiResponses( | ||
| value = [ | ||
| ApiResponse( | ||
| responseCode = "201", | ||
| description = "책이 서재에 성공적으로 등록되었습니다.", | ||
| content = [Content(schema = Schema(implementation = UserBookResponse::class))] | ||
| ), | ||
| ApiResponse( | ||
| responseCode = "200", // 업데이트 시 200 OK 반환 가능 | ||
| description = "책 상태가 성공적으로 업데이트되었습니다.", | ||
| content = [Content(schema = Schema(implementation = UserBookResponse::class))] | ||
| ), | ||
| ApiResponse( | ||
| responseCode = "400", | ||
| description = "잘못된 요청 파라미터" | ||
| ), | ||
| ApiResponse( | ||
| responseCode = "404", | ||
| description = "존재하지 않는 책 (ISBN 오류)" | ||
| ) | ||
| ] | ||
| ) | ||
| @PostMapping("/upsert") // 경로를 /upsert로 변경 | ||
| fun upsertBookToMyLibrary( // 메서드 이름 변경 | ||
| @RequestHeader("Authorization") authorization: String, | ||
| @Valid @RequestBody request: UserBookRegisterRequest | ||
| ): ResponseEntity<UserBookResponse> | ||
|
|
||
| @Operation( | ||
| summary = "사용자 서재 조회", | ||
| description = "현재 사용자의 서재에 등록된 모든 책을 조회합니다." | ||
| ) | ||
| @ApiResponses( | ||
| value = [ | ||
| ApiResponse( | ||
| responseCode = "200", | ||
| description = "서재 조회 성공", | ||
| content = [Content(schema = Schema(implementation = Array<UserBookResponse>::class))] | ||
| ), | ||
| ApiResponse( | ||
| responseCode = "404", | ||
| description = "사용자를 찾을 수 없습니다." | ||
| ) | ||
| ] | ||
| ) | ||
| @GetMapping("/my-library") | ||
| fun getUserLibraryBooks( | ||
| @RequestHeader("Authorization") authorization: String | ||
| ): ResponseEntity<List<UserBookResponse>> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
API 설계 관점에서 컨트롤러의 책임 분리를 고려해보세요.
현재 BookControllerApi는 도서 검색/상세 조회와 사용자 서재 관리라는 두 가지 다른 도메인을 담당하고 있습니다. 단일 책임 원칙(SRP)에 따라 사용자 서재 관련 기능을 별도의 컨트롤러(UserLibraryControllerApi)로 분리하는 것을 고려해보세요.
새로운 컨트롤러를 생성하는 예시:
@Tag(name = "User Library", description = "사용자 서재 관리 API")
@RequestMapping("/api/v1/user-library")
interface UserLibraryControllerApi {
// 사용자 서재 관련 엔드포인트들
}🤖 Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
between lines 113 and 165, the BookControllerApi currently handles both book
search/detail and user library management, violating the single responsibility
principle. To fix this, create a new controller interface named
UserLibraryControllerApi dedicated to user library-related endpoints like
upsertBookToMyLibrary and getUserLibraryBooks. Move these methods from
BookControllerApi to UserLibraryControllerApi, annotate it with @Tag(name =
"User Library", description = "사용자 서재 관리 API") and
@RequestMapping("/api/v1/user-library"), and update routing accordingly to
separate concerns clearly.
apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
Outdated
Show resolved
Hide resolved
| @PostMapping("/upsert") // 경로를 /upsert로 변경 | ||
| fun upsertBookToMyLibrary( // 메서드 이름 변경 | ||
| @RequestHeader("Authorization") authorization: String, | ||
| @Valid @RequestBody request: UserBookRegisterRequest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
RESTful API 설계 원칙을 고려해보세요.
현재 /upsert 경로는 RESTful하지 않습니다. HTTP 메서드의 의미를 명확히 하는 것이 좋습니다:
- 새로운 리소스 생성:
POST /my-library - 기존 리소스 업데이트:
PUT /my-library/{bookId}
또한 Authorization 헤더를 String으로 직접 처리하는 것보다 Spring Security의 @AuthenticationPrincipal 어노테이션을 사용하는 것을 고려해보세요.
-@PostMapping("/upsert")
-fun upsertBookToMyLibrary(
- @RequestHeader("Authorization") authorization: String,
- @Valid @RequestBody request: UserBookRegisterRequest
-): ResponseEntity<UserBookResponse>
+@PostMapping("/my-library")
+fun registerBookToMyLibrary(
+ @AuthenticationPrincipal userId: Long,
+ @Valid @RequestBody request: UserBookRegisterRequest
+): ResponseEntity<UserBookResponse>📝 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.
| @PostMapping("/upsert") // 경로를 /upsert로 변경 | |
| fun upsertBookToMyLibrary( // 메서드 이름 변경 | |
| @RequestHeader("Authorization") authorization: String, | |
| @Valid @RequestBody request: UserBookRegisterRequest | |
| @PostMapping("/my-library") | |
| fun registerBookToMyLibrary( | |
| @AuthenticationPrincipal userId: Long, | |
| @Valid @RequestBody request: UserBookRegisterRequest | |
| ): ResponseEntity<UserBookResponse> |
🤖 Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
around lines 139 to 142, the endpoint path /upsert is not RESTful. Refactor the
controller to use POST /my-library for creating new resources and PUT
/my-library/{bookId} for updating existing ones. Also, replace the manual
extraction of the Authorization header String with Spring Security's
@AuthenticationPrincipal annotation to handle authentication more cleanly and
securely.
There was a problem hiding this comment.
Create와 Update를 한번에 같은 API로 처리하신 이유가 궁금합니다!
하나의 메서드가 두 가지 일을 하게 되면, 단일 책임 원칙을 위반했다고 생각합니다.
저는 @PostMapping(), @PatchMapping or @PutMapping 방식으로 별도로 분리하고 api/v1/books와 api/v1/books/{isbn}로 경로를 지정하는게 좀더 RESTful 한 설계라고 생각됩니다.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
좋은 질문 감사합니다!
말씀해주신 것처럼 @PostMapping(생성)과 @PutMapping 또는 @PatchMapping(수정)을 RESTful하게 분리하는 설계는 원칙적으로 올바른 방향입니다.
다만, 이번에는 다음과 같은 이유로 Create와 Update를 하나의 API (Upsert)로 통합하여 처리했습니다.
Upsert로 통합한 이유
- 도메인 상 "존재 여부"를 판단하는 로직이 비즈니스 로직에 가까움
클라이언트가 책을 등록할 때, 해당 ISBN이 이미 등록된 것인지 매번 선조회 → 분기 처리하는 것은 불필요한 round-trip을 야기할 수 있다고 판단했어요!
따라서 서버 쪽에서 존재 여부 판단 → 생성 or 수정을 한번에 처리하는 것이 효율적이고, 비즈니스에 더 적합하다고 판단했습니다.
-
클라이언트 단 구현 복잡도 감소
만약 등록과 수정을 각각 다른 API로 만들면, 클라이언트는 다음의 흐름을 가져야 합니다: -
ISBN 존재 여부 확인 API 호출
-
존재하면 → 수정 API 호출
존재하지 않으면 → 등록 API 호출
이는 불필요한 네트워크 요청과 분기 로직을 유발하고, 특히 모바일처럼 네트워크 비용이 중요한 환경에서 불필요한 통신이 오고간다고 판단했습니다. -
백엔드 복잡도는 오히려 줄었습니다
하나의 UseCase (findOrCreateBookByIsbn)에서 "있으면 수정 / 없으면 생성"을 도메인 규칙에 맞춰 처리하기 때문에,
Controller, Service 모두 하나의 흐름으로 응집력 있게 구성할 수 있고
오히려 API가 나뉘면 create, update 양쪽에서 중복 로직이 생길 수도 있습니다. -
단일 책임 원칙(SRP)을 위반하지 않는 범위 내에서 처리
단일 책임 원칙을 고려해도, 이 API는 ISBN으로 책 데이터를 동기화하는 책임을 가집니다.
"무조건 존재하도록 보장"하는 동작이 도메인의 핵심 요구사항이기 때문에, 이는 하나의 책임으로 볼 수 있지 않을까 생각했어요!
- 실제 많은 API에서도 Upsert는 흔히 사용되는 패턴
예: PUT /users/{id}가 없으면 생성, 있으면 수정 동작하는 경우.
해당 의견을 듣고 동훈님의 생각 남겨주세요!
There was a problem hiding this comment.
오! 사실 이번에 upsert라는 방식을 처음 접하게 되었는데 흥미롭네요 ㅎㅎ
민우님 말씀처럼, ISBN으로 책 데이터를 동기화한다는 것을 한 가지 책임으로 볼 수 있을 것 같습니다.
존재하면 → 수정 API 호출, 존재하지 않으면 → 등록 API 호출
이는 불필요한 네트워크 요청과 분기 로직을 유발하고, 특히 모바일처럼 네트워크 비용이 중요한 환경에서 불필요한 통신이 오고간다고 판단했습니다.
특히 이 부분이 공감이 많이 되었습니다. 결국 데이터는 서버에서 관리하기 때문에 서버에서 판단을 하는 방식이 자연스러운 흐름 같기도 합니다!!
다만 한 가지 궁금한 점은, 현재 사용하고 있는 @PostMapping 방식은 일반적으로 **멱등성(idempotency)**을 보장하지 않는 것으로 알고 있습니다.
멱등성: 동일한 요청을 한 번 보내는 것과 여러 번 연속으로 보내는 것이 같은 효과를 지님.
현재 Upsert 로직은 동일한 요청을 한 번 보내는 것과 여러 번 연속으로 보내는 것이 같은 response를 보내주는 것 같아 @PostMapping보다는 @PutMapping을 사용하는 게 더 적절하지 않을까 하는 생각이 들었습니다.
이에 대해 의견부탁드립니다!!
re: 좋아요 PUT으로 변경할게요~
apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
Outdated
Show resolved
Hide resolved
apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
Outdated
Show resolved
Hide resolved
move-hoon
left a comment
move-hoon
left a comment
There was a problem hiding this comment.
너무 고생많으셨습니다 민우님!
코멘트 남겼으니 한 번 확인 부탁드리겠습니다 ㅎㅎ
| private val bookUseCase: BookUseCase, | ||
| private val authUseCase: AuthUseCase |
There was a problem hiding this comment.
BookController에서 두 도메인의 유즈케이스를 함께 의존하고 있는 부분이 있는데요,
저희가 논의했던 방향과는 조금 다른 설계 방식인 것 같아서 공유드립니다!
아래 블로그 기준으로 보면 현재는 1번 방식에 가까운 것 같고, 저희는 3번 방식으로 가기로 했던 걸로 기억하고 있어요 😊
There was a problem hiding this comment.
그리고 액세스 토큰으로 부터 userId를 빼내는 방식은 AuthUtil 대신, ArgumentResolver에 등록을 해두는 방식 or @AuthenticationPrincipal 어노테이션 방식을 사용하는게 더 좋을 것 같습니다!
- Gemini 기준 @AuthenticationPrincipal를 더 추천하긴 하네요 😊
There was a problem hiding this comment.
BookController에서 두 도메인의 유즈케이스를 함께 의존하고 있는 부분이 있는데요, 저희가 논의했던 방향과는 조금 다른 설계 방식인 것 같아서 공유드립니다! 아래 블로그 기준으로 보면 현재는 1번 방식에 가까운 것 같고, 저희는 3번 방식으로 가기로 했던 걸로 기억하고 있어요 😊
앗 그런가요?? 제가 착각했나보네요 블로그 글 읽어보니 테스트의 용이성을 위해서라도 3번이 좋아보이긴하네요 좋습니다~
There was a problem hiding this comment.
ArgumentResolver는 처음봣는데 신기하네요 저도 동감 @AuthenticationPrincipal 이 방향으로 가시죠!
| @PostMapping("/upsert") | ||
| override fun upsertBookToMyLibrary( | ||
| @RequestHeader("Authorization") authorization: String, | ||
| @Valid @RequestBody request: UserBookRegisterRequest | ||
| ): ResponseEntity<UserBookResponse> { | ||
| val currentUserId = AuthUtils.extractUserIdFromAuthHeader(authorization, authUseCase::getUserIdFromAccessToken) | ||
| val response = bookUseCase.upsertBookToMyLibrary(currentUserId, request) | ||
| return ResponseEntity.ok(response) | ||
| } | ||
|
|
||
| @GetMapping("/my-library") | ||
| override fun getUserLibraryBooks( | ||
| @RequestHeader("Authorization") authorization: String | ||
| ): ResponseEntity<List<UserBookResponse>> { | ||
| val currentUserId = AuthUtils.extractUserIdFromAuthHeader(authorization, authUseCase::getUserIdFromAccessToken) | ||
| val response = bookUseCase.getUserLibraryBooks(currentUserId) | ||
| return ResponseEntity.ok(response) | ||
| } |
There was a problem hiding this comment.
그리고 액세스 토큰으로 부터 userId를 빼내는 방식은 AuthUtil 대신, ArgumentResolver에 등록을 해두는 방식 or @AuthenticationPrincipal 어노테이션 방식을 사용하는게 더 좋을 것 같습니다!
해당 부분 의견도 위 리뷰와 의견 일치합니다!
apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
Outdated
Show resolved
Hide resolved
| @PostMapping("/upsert") // 경로를 /upsert로 변경 | ||
| fun upsertBookToMyLibrary( // 메서드 이름 변경 | ||
| @RequestHeader("Authorization") authorization: String, | ||
| @Valid @RequestBody request: UserBookRegisterRequest |
There was a problem hiding this comment.
Create와 Update를 한번에 같은 API로 처리하신 이유가 궁금합니다!
하나의 메서드가 두 가지 일을 하게 되면, 단일 책임 원칙을 위반했다고 생각합니다.
저는 @PostMapping(), @PatchMapping or @PutMapping 방식으로 별도로 분리하고 api/v1/books와 api/v1/books/{isbn}로 경로를 지정하는게 좀더 RESTful 한 설계라고 생각됩니다.
apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
Outdated
Show resolved
Hide resolved
| fun findOrCreateBook(isbn: String, bookFactory: (String) -> Book): Book { | ||
| return bookRepository.findByIsbn(isbn) ?: run { | ||
| val newBook = bookFactory(isbn) | ||
| bookRepository.save(newBook) | ||
| } | ||
| } |
There was a problem hiding this comment.
해당 서비스도 userbook 패키지에 있는게 좋을 것 같습니다!
| fun upsertUserBook(userId: UUID, book: Book, status: BookStatus): UserBook { | ||
| val existing = userBookRepository.findByUserIdAndBookIsbn(userId, book.isbn) | ||
| return if (existing != null) { | ||
| val updated = existing.updateStatus(status) | ||
| userBookRepository.save(updated) | ||
| } else { | ||
| val created = UserBook.create(userId, book, status) | ||
| userBookRepository.save(created) | ||
| } | ||
| } |
There was a problem hiding this comment.
Create와 Update를 한번에 같은 API로 처리하신 이유가 궁금합니다!
하나의 메서드가 두 가지 일을 하게 되면, 단일 책임 원칙을 위반했다고 생각합니다.저는 @PostMapping(), @PatchMapping or @PutMapping 방식으로 별도로 분리하고 api/v1/books와 api/v1/books/{isbn}로 경로를 지정하는게 좀더 RESTful 한 설계라고 생각됩니다.
해당 리뷰와 의견 동일합니다.
API 분리와, 메서드 분리를 동시해 해야 할 것 같아요
There was a problem hiding this comment.
이건 제가 전에 리뷰하면서 놓친 포인트인데, 상태 변경이 필요한 필드를 var + protected set으로 바꾸는게 좋아보입니다!
isbn을 제외하고는 모두 변경 가능성이 있을 것 같긴합니다!
There was a problem hiding this comment.
이것도 userbook 패키지로 빼면 좋지 않을까 라는 생각이 듭니다!
There was a problem hiding this comment.
추가적으로 상태 변경이 필요한 필드를 var + protected set으로 바꾸는게 좋아보입니다!
|
추가적으로, 스웨거에 ErrorResponse에 대한 명세가 없습니다!
|
minwoo1999
left a comment
minwoo1999
left a comment
There was a problem hiding this comment.
좋은 의견 감사합니다.
질문 하신 내용 답변드렸습니다
고생하셨어요!!
| private val bookUseCase: BookUseCase, | ||
| private val authUseCase: AuthUseCase |
There was a problem hiding this comment.
BookController에서 두 도메인의 유즈케이스를 함께 의존하고 있는 부분이 있는데요, 저희가 논의했던 방향과는 조금 다른 설계 방식인 것 같아서 공유드립니다! 아래 블로그 기준으로 보면 현재는 1번 방식에 가까운 것 같고, 저희는 3번 방식으로 가기로 했던 걸로 기억하고 있어요 😊
앗 그런가요?? 제가 착각했나보네요 블로그 글 읽어보니 테스트의 용이성을 위해서라도 3번이 좋아보이긴하네요 좋습니다~
| private val bookUseCase: BookUseCase, | ||
| private val authUseCase: AuthUseCase |
There was a problem hiding this comment.
ArgumentResolver는 처음봣는데 신기하네요 저도 동감 @AuthenticationPrincipal 이 방향으로 가시죠!
apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
Outdated
Show resolved
Hide resolved
apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
Outdated
Show resolved
Hide resolved
| @PostMapping("/upsert") // 경로를 /upsert로 변경 | ||
| fun upsertBookToMyLibrary( // 메서드 이름 변경 | ||
| @RequestHeader("Authorization") authorization: String, | ||
| @Valid @RequestBody request: UserBookRegisterRequest |
There was a problem hiding this comment.
좋은 질문 감사합니다!
말씀해주신 것처럼 @PostMapping(생성)과 @PutMapping 또는 @PatchMapping(수정)을 RESTful하게 분리하는 설계는 원칙적으로 올바른 방향입니다.
다만, 이번에는 다음과 같은 이유로 Create와 Update를 하나의 API (Upsert)로 통합하여 처리했습니다.
Upsert로 통합한 이유
- 도메인 상 "존재 여부"를 판단하는 로직이 비즈니스 로직에 가까움
클라이언트가 책을 등록할 때, 해당 ISBN이 이미 등록된 것인지 매번 선조회 → 분기 처리하는 것은 불필요한 round-trip을 야기할 수 있다고 판단했어요!
따라서 서버 쪽에서 존재 여부 판단 → 생성 or 수정을 한번에 처리하는 것이 효율적이고, 비즈니스에 더 적합하다고 판단했습니다.
-
클라이언트 단 구현 복잡도 감소
만약 등록과 수정을 각각 다른 API로 만들면, 클라이언트는 다음의 흐름을 가져야 합니다: -
ISBN 존재 여부 확인 API 호출
-
존재하면 → 수정 API 호출
존재하지 않으면 → 등록 API 호출
이는 불필요한 네트워크 요청과 분기 로직을 유발하고, 특히 모바일처럼 네트워크 비용이 중요한 환경에서 불필요한 통신이 오고간다고 판단했습니다. -
백엔드 복잡도는 오히려 줄었습니다
하나의 UseCase (findOrCreateBookByIsbn)에서 "있으면 수정 / 없으면 생성"을 도메인 규칙에 맞춰 처리하기 때문에,
Controller, Service 모두 하나의 흐름으로 응집력 있게 구성할 수 있고
오히려 API가 나뉘면 create, update 양쪽에서 중복 로직이 생길 수도 있습니다. -
단일 책임 원칙(SRP)을 위반하지 않는 범위 내에서 처리
단일 책임 원칙을 고려해도, 이 API는 ISBN으로 책 데이터를 동기화하는 책임을 가집니다.
"무조건 존재하도록 보장"하는 동작이 도메인의 핵심 요구사항이기 때문에, 이는 하나의 책임으로 볼 수 있지 않을까 생각했어요!
- 실제 많은 API에서도 Upsert는 흔히 사용되는 패턴
예: PUT /users/{id}가 없으면 생성, 있으면 수정 동작하는 경우.
해당 의견을 듣고 동훈님의 생각 남겨주세요!
There was a problem hiding this comment.
아아 그러네요 저거 분리안해놨었네요 확인!
| var createdAt: LocalDateTime, | ||
| var updatedAt: LocalDateTime, | ||
| var deletedAt: LocalDateTime? = null, |
There was a problem hiding this comment.
오 좋은데요 ? 도메인모델에서는 전부 val로 처리하고 var처럼 변경이 필요한 부분들을 copy로 처리한다 좋은 것 같아요 굿굿!
| createdAt: LocalDateTime = LocalDateTime.now(), | ||
| updatedAt: LocalDateTime = LocalDateTime.now(), |
There was a problem hiding this comment.
이부분은 이해가 안가는데 다시 한번 설명 부탁드릴게요~
| fun findOrCreateBook(isbn: String, bookFactory: (String) -> Book): Book { | ||
| return bookRepository.findByIsbn(isbn) ?: run { | ||
| val newBook = bookFactory(isbn) | ||
| bookRepository.save(newBook) | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 로직상으로는 사실 아래처럼 run 없이 간단하게 써도 큰 문제는 없어요.
return bookRepository.findByIsbn(isbn)
?: bookRepository.save(bookFactory(isbn))
다만 bookFactory로 만든 객체를 변수로 받아야 했기 때문에 run을 써서
중간에 연산을 통해 저장가능하도록 했어요!
minwoo1999
left a comment
minwoo1999
left a comment
There was a problem hiding this comment.
추가적으로 코멘트 남겼어요 @hoonyworld
| fun updateStatus(newStatus: BookStatus): UserBook { | ||
| this.status = newStatus | ||
| this.updatedAt = LocalDateTime.now() | ||
| return this | ||
| } |
There was a problem hiding this comment.
require로 이미 동일한 상태일 경우 예외 메세지를 띄워주는게 맞을까요?
| @PostMapping("/upsert") // 경로를 /upsert로 변경 | ||
| fun upsertBookToMyLibrary( // 메서드 이름 변경 | ||
| @RequestHeader("Authorization") authorization: String, | ||
| @Valid @RequestBody request: UserBookRegisterRequest |
There was a problem hiding this comment.
오! 사실 이번에 upsert라는 방식을 처음 접하게 되었는데 흥미롭네요 ㅎㅎ
민우님 말씀처럼, ISBN으로 책 데이터를 동기화한다는 것을 한 가지 책임으로 볼 수 있을 것 같습니다.
존재하면 → 수정 API 호출, 존재하지 않으면 → 등록 API 호출
이는 불필요한 네트워크 요청과 분기 로직을 유발하고, 특히 모바일처럼 네트워크 비용이 중요한 환경에서 불필요한 통신이 오고간다고 판단했습니다.
특히 이 부분이 공감이 많이 되었습니다. 결국 데이터는 서버에서 관리하기 때문에 서버에서 판단을 하는 방식이 자연스러운 흐름 같기도 합니다!!
다만 한 가지 궁금한 점은, 현재 사용하고 있는 @PostMapping 방식은 일반적으로 **멱등성(idempotency)**을 보장하지 않는 것으로 알고 있습니다.
멱등성: 동일한 요청을 한 번 보내는 것과 여러 번 연속으로 보내는 것이 같은 효과를 지님.
현재 Upsert 로직은 동일한 요청을 한 번 보내는 것과 여러 번 연속으로 보내는 것이 같은 response를 보내주는 것 같아 @PostMapping보다는 @PutMapping을 사용하는 게 더 적절하지 않을까 하는 생각이 들었습니다.
이에 대해 의견부탁드립니다!!
re: 좋아요 PUT으로 변경할게요~
| fun updateStatus(newStatus: BookStatus): UserBook { | ||
| this.status = newStatus | ||
| this.updatedAt = LocalDateTime.now() | ||
| return this | ||
| } |
There was a problem hiding this comment.
enum으로 들어오니 타입 안정성은 보장이 된 것 같아서, 중복 상태를 예시로 들었었습니다 ㅎㅎ
추후 비즈니스 로직이 추가되었을 때, 보장하기 위해 require을 도입해도 될 것 같습니다!
re: 일단 현재로직에서는 require가 필요없을거라고 판단이 들어서 지금 로직에는 추가하지 않는방향으로 가는건 어떨까요
현재상황에서 읽는 중 -> 읽는 중으로 동일 상태로 변경하였다고 해서 사용자에게 예외를 던져주는 건 아니라고 판단들어서요!
| createdAt: LocalDateTime = LocalDateTime.now(), | ||
| updatedAt: LocalDateTime = LocalDateTime.now(), |
There was a problem hiding this comment.
https://github.com/YAPP-Github/Reed-Server/pull/31/files#r2190365664
여기서 기본값을 제거하는 부분에 대한 얘기였습니다~
| fun findOrCreateBook(isbn: String, bookFactory: (String) -> Book): Book { | ||
| return bookRepository.findByIsbn(isbn) ?: run { | ||
| val newBook = bookFactory(isbn) | ||
| bookRepository.save(newBook) | ||
| } | ||
| } |
There was a problem hiding this comment.
fun findOrCreateBook(isbn: String): Book {
return bookRepository.findByIsbn(isbn)
?: bookRepository.save(Book.create(.....))
}- 저는 사실 bookFactory라는 전략 함수?에 대해서 익숙하지가 않아서 위 코드가 더 쉽게 느껴지는데, 혹시 bookFactory로 만들었을 때 장점에는 어떤게 있을까요?
- 추가적으로 isbn만 인자로 받는데 없을 경우 새로운 Book 객체를 만든다는 흐름이 논리적으로 잘 이해가 안됩니다. create 메서드로 호출할 때만 해도 총 7개의 인자를 받아야지만 Book 객체가 생성이 되더라고요....
re: bookFactory.createBook()는 이미 조립된 키트를 받아오는 느낌으로 생각해주시면 되는데
책임 분리라고 생각해주시면 될 것 같아요! findOrCreateBook()는 조회 + 조건부 저장에만 집중하고 어떻게 생성할지는 외부에서 결정 수 있게 되는데
사실 밑에 말씀해주신 service에서 create 인자 하나하나 넣어서 하는 방식이 아닌 VO로 감싸서 하는 방식이면 book인지 하나만 넘기면 되서 factory부분은 안쓸 것 같네요!
| Book.create( | ||
| isbn = finalIsbn, | ||
| title = bookDetail.title, | ||
| author = bookDetail.author, | ||
| publisher = bookDetail.publisher, | ||
| publicationYear = bookDetail.pubDate | ||
| ?.takeIf { it.length >= 4 && it.substring(0, 4).all { ch -> ch.isDigit() } } | ||
| ?.substring(0, 4) | ||
| ?.toIntOrNull(), |
There was a problem hiding this comment.
현재 Book 도메인을 Domain 서비스에서 create를 하는게 아닌, 애플리케이션 서비스에서 create를 진행하고 있는데 이 부분은 유효한 데이터를 VO로 감싸서 vo 객체를 생성한 후 bookDomainService.create(Vo vo)로 넘겨서 해당 메서드 내부에서 Book.create를 하면 더 좋을 것 같습니다!!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
apis/src/main/kotlin/org/yapp/apis/book/dto/request/UserBookRegisterRequest.kt (3)
8-9: nullable 필드에 @notblank 검증 어노테이션 사용 문제이전 리뷰에서 지적된 문제가 여전히 해결되지 않았습니다.
bookIsbn필드가 nullable로 선언되어 있으면서@NotBlank검증 어노테이션을 사용하는 것은 일관성이 없습니다.
13-13: 불필요한 null 단언 연산자 사용이전 리뷰에서 지적된 문제가 여전히 해결되지 않았습니다.
validBookIsbn()메서드에서!!연산자를 사용하는 것은 NPE 위험성이 있습니다.
15-19: 팩토리 메서드의 파라미터 검증 부재이전 리뷰에서 지적된 문제가 여전히 해결되지 않았습니다.
create메서드에서bookIsbn파라미터에 대한 검증이 없습니다.domain/src/main/kotlin/org/yapp/domain/service/domain/UserBookDomainService.kt (1)
15-24: 생성과 업데이트 로직을 분리하세요.하나의 메서드에서 생성과 업데이트를 모두 처리하는 것은 단일 책임 원칙을 위반합니다. 테스트와 유지보수가 어려워질 수 있습니다.
infra/src/main/kotlin/org/yapp/infra/user/repository/impl/UserBookRepositoryImpl.kt (1)
24-26: 대용량 데이터 처리를 위한 성능 최적화를 고려하세요.모든 결과를 한번에 변환하는 방식은 대용량 데이터의 경우 메모리 사용량 문제를 야기할 수 있습니다. 스트리밍 처리나 페이징 처리를 검토해보세요.
infra/src/main/kotlin/org/yapp/infra/user/entity/UserBookEntity.kt (3)
47-49: author 필드의 nullable 설정 검토 필요
author필드가 nullable로 설정되어 있지만, 도메인 모델에서는 non-nullable로 되어 있습니다. 데이터베이스 스키마와 도메인 모델 간의 일관성을 확인해야 합니다.
1-1: 패키지 구조 개선 제안이전 리뷰에서 언급된 바와 같이, UserBookEntity를 userbook 패키지로 이동하는 것이 도메인 구조에 더 적합할 것 같습니다.
78-94: fromDomain 메서드 구현 방식 개선이전 리뷰에서 언급된 바와 같이,
apply블록에서 타임스탬프 필드를 설정하는 것보다 더 명확한 방식을 사용하는 것이 좋겠습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
apis/src/main/kotlin/org/yapp/apis/auth/controller/AuthController.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/auth/controller/AuthControllerApi.kt(3 hunks)apis/src/main/kotlin/org/yapp/apis/book/controller/BookController.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookCreateRequest.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookDetailRequest.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/dto/request/UserBookRegisterRequest.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/dto/response/UserBookResponse.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/exception/BookErrorCode.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/exception/BookException.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/service/BookManagementService.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/service/BookQueryService.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/service/UserBookService.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/usecase/BookUseCase.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/book/Book.kt(2 hunks)domain/src/main/kotlin/org/yapp/domain/service/domain/BookDomainService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/service/domain/UserBookDomainService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/userbook/BookStatus.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/userbook/UserBook.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/userbook/UserBookRepository.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/book/entity/BookEntity.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/user/entity/UserBookEntity.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/user/repository/impl/UserBookRepositoryImpl.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt (2)
Learnt from: hoonyworld
PR: YAPP-Github/Reed-Server#31
File: apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt:13-13
Timestamp: 2025-07-08T14:24:44.340Z
Learning: IntelliJ IDEA에서 와일드카드 임포트를 방지하는 설정 방법: File > Settings > Editor > Code Style > Java > Imports 탭에서 "Class count to use import with '*'" 값을 높게 설정(예: 500)하면 해당 임계값 이하에서는 와일드카드 임포트가 자동으로 사용되지 않음. 와일드카드 임포트 관련 리뷰 시 이 IDE 설정 방법도 함께 안내하면 좋음.
Learnt from: hoonyworld
PR: YAPP-Github/Reed-Server#31
File: apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt:13-13
Timestamp: 2025-07-08T14:24:44.340Z
Learning: hoonyworld가 와일드카드 임포트 관련하여 IDE 설정을 통한 자동 최적화 방법을 추천했습니다. 향후 임포트 관련 리뷰 시 IDE 설정 방법도 함께 안내하는 것이 좋겠습니다.
⏰ 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 (37)
domain/src/main/kotlin/org/yapp/domain/userbook/BookStatus.kt (1)
3-7: 읽기 상태 enum이 잘 정의되어 있습니다.도서 읽기 상태를 명확하게 표현하는 enum이 잘 구현되었습니다. 한국어 주석도 이해하기 쉽게 작성되어 있어 좋습니다.
apis/src/main/kotlin/org/yapp/apis/book/exception/BookException.kt (1)
5-8: 예외 클래스가 올바르게 구현되었습니다.
CommonException을 상속하여 도서 관련 예외를 처리하는 구조가 잘 구현되었습니다. 에러 코드와 선택적 메시지를 받는 구조도 적절합니다.apis/src/main/kotlin/org/yapp/apis/book/exception/BookErrorCode.kt (1)
7-23: 에러 코드가 적절하게 정의되었습니다.도서 관련 에러 코드가 잘 구조화되어 있습니다. HTTP 상태 코드와 에러 메시지가 적절하게 매핑되어 있고,
BaseErrorCode인터페이스를 올바르게 구현했습니다.domain/src/main/kotlin/org/yapp/domain/service/domain/BookDomainService.kt (2)
12-15: 도메인 서비스가 깔끔하게 구현되었습니다.
findOrCreateBook메서드가 이전 리뷰 피드백을 반영하여 간단하고 명확하게 구현되었습니다. Elvis 연산자를 활용한 구현이 가독성이 좋습니다.
17-19: 저장 메서드가 적절하게 구현되었습니다.
save메서드가 리포지토리에 대한 단순한 위임으로 구현되어 있어 적절합니다. 도메인 서비스의 역할에 맞게 구현되었습니다.domain/src/main/kotlin/org/yapp/domain/userbook/UserBookRepository.kt (3)
8-8: 조회 메서드가 적절하게 정의되었습니다.사용자 ID와 도서 ISBN으로 조회하는 메서드가 nullable 반환 타입으로 잘 정의되었습니다. 조회 결과가 없을 수 있음을 명확히 표현했습니다.
10-10: 저장 메서드가 올바르게 정의되었습니다.
UserBook엔티티를 저장하고 저장된 객체를 반환하는 표준적인 리포지토리 패턴을 따르고 있습니다.
12-12: 사용자별 조회 메서드가 적절하게 정의되었습니다.사용자 ID로 모든 도서를 조회하는 메서드가 List 반환 타입으로 잘 정의되었습니다. 메서드명도 명확하게 의도를 표현하고 있습니다.
apis/src/main/kotlin/org/yapp/apis/book/service/UserBookService.kt (1)
9-18: 수정 완료: 불필요한 BookStatus 변환 제거됨이전 리뷰에서 지적된
BookStatus.valueOf(status.name)변환 코드가 올바르게 수정되었습니다. 현재 코드는 깔끔하고 효율적입니다.apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookDetailRequest.kt (1)
30-38: 좋은 팩토리 메서드 패턴companion object의
of메서드는 객체 생성을 명확하게 제어하는 좋은 패턴입니다.apis/src/main/kotlin/org/yapp/apis/book/service/BookQueryService.kt (1)
10-22: 좋은 파사드 패턴 구현외부 서비스를 캡슐화하는 파사드 패턴으로 잘 구현되었습니다. 의존성 방향이 올바르고 책임이 명확하게 분리되어 있습니다.
apis/src/main/kotlin/org/yapp/apis/book/service/BookManagementService.kt (1)
15-20: 중복 변환 로직 확인 필요
BookCreateRequest.create(detail)로 생성한 후 바로BookCreateRequest.from(request)로 다시 변환하는 것이 의도된 것인지 확인이 필요합니다. 불필요한 변환이 아닌지 검토해주세요.domain/src/main/kotlin/org/yapp/domain/service/domain/UserBookDomainService.kt (1)
26-28: 도메인 서비스 구현이 적절합니다.유저 도서 조회 로직이 명확하게 구현되어 있습니다. 리포지토리 위임 패턴도 올바르게 적용되었습니다.
apis/src/main/kotlin/org/yapp/apis/book/dto/response/UserBookResponse.kt (2)
34-35: 타임스탬프 포맷팅이 올바르게 구현되었습니다.
DateTimeFormatter.ISO_LOCAL_DATE_TIME을 사용하여 일관된 타임스탬프 포맷을 제공하고 있습니다. 이전 리뷰 피드백이 잘 반영되었습니다.
8-19: DTO 구조가 잘 설계되었습니다.private constructor와 companion object factory method 패턴을 사용하여 도메인 객체로부터 안전하게 DTO를 생성하고 있습니다.
infra/src/main/kotlin/org/yapp/infra/user/repository/impl/UserBookRepositoryImpl.kt (2)
15-17: 도메인-엔티티 변환이 적절하게 구현되었습니다.nullable 처리와 도메인 객체 변환 로직이 올바르게 구현되어 있습니다.
20-22: 저장 로직이 올바르게 구현되었습니다.도메인 객체를 엔티티로 변환 후 저장하고 다시 도메인 객체로 변환하는 패턴이 적절합니다.
apis/src/main/kotlin/org/yapp/apis/auth/controller/AuthController.kt (2)
37-40: 인증 보안이 향상되었습니다.수동 헤더 파싱 대신
@AuthenticationPrincipal을 사용하여 Spring Security의 인증 컨텍스트를 안전하게 활용하고 있습니다.
43-46: 일관된 인증 처리 방식이 적용되었습니다.사용자 프로필 조회에서도 동일한 보안 개선 사항이 적용되어 코드 일관성이 향상되었습니다.
domain/src/main/kotlin/org/yapp/domain/book/Book.kt (2)
11-14: 도메인 모델의 non-nullable 속성 변경이 적절합니다.저자, 출판사, 표지 이미지 URL을 필수 속성으로 변경하여 도메인 모델의 무결성이 향상되었습니다.
23-31: 팩토리 메서드 매개변수 순서가 일관성 있게 정렬되었습니다.이전 리뷰 피드백이 반영되어
create메서드의 매개변수 순서가reconstruct메서드와 일치하도록 수정되었습니다.apis/src/main/kotlin/org/yapp/apis/book/controller/BookController.kt (2)
37-39: getBookDetail 메서드 변경 사항 확인
bookUseCase.getBookDetail이 이제request.validIsbn()을 받도록 변경되었습니다. 이는 요청 객체에서 ISBN 검증 로직이 분리되었음을 의미합니다. 이러한 변경이 의도된 것인지 확인하시기 바랍니다.
41-57: 사용자 서재 관리 기능 구현 잘 되었습니다
@AuthenticationPrincipal을 사용하여 사용자 인증을 처리하는 방식이 이전 리뷰 피드백을 잘 반영했습니다. 두 새로운 엔드포인트가 RESTful 설계 원칙을 따르고 있으며, 비즈니스 로직을 UseCase로 적절히 위임하고 있습니다.infra/src/main/kotlin/org/yapp/infra/user/entity/UserBookEntity.kt (1)
12-55: 엔티티 구조 및 소프트 삭제 구현이 잘 되었습니다JPA 엔티티 구조가 잘 설계되어 있고,
@SQLDelete를 사용한 소프트 삭제 기능이 적절히 구현되었습니다.var+protected set을 사용한 필드 보호도 좋은 설계입니다.apis/src/main/kotlin/org/yapp/apis/auth/controller/AuthControllerApi.kt (2)
94-94: @AuthenticationPrincipal 사용으로 개선되었습니다이전 리뷰에서 제안된 바와 같이
@RequestHeader("Authorization")대신@AuthenticationPrincipal을 사용하도록 변경되었습니다. 이는 더 안전하고 Spring Security와 일관된 방식입니다.
115-117: getUserProfile 메서드도 일관되게 업데이트
getUserProfile메서드도 동일하게@AuthenticationPrincipal을 사용하도록 업데이트되어 일관성을 유지하고 있습니다.infra/src/main/kotlin/org/yapp/infra/book/entity/BookEntity.kt (2)
22-53: 필드 보호 설정이 잘 개선되었습니다이전 리뷰에서 제안된 바와 같이 상태 변경이 필요한 필드들을
var+protected set으로 변경하여 적절한 캡슐화를 제공하고 있습니다.isbn은 불변으로 유지하는 것도 적절합니다.
16-28: 생성자 매개변수 확장이 적절합니다생성자에서 모든 주요 도서 속성을 받도록 변경되어 엔티티 초기화가 더 명확해졌습니다. 이는 도메인 모델의 non-nullable 요구사항과도 잘 맞습니다.
domain/src/main/kotlin/org/yapp/domain/userbook/UserBook.kt (2)
9-24: 도메인 모델 설계가 잘 되었습니다불변성을 유지하면서 private constructor와 factory 메서드를 사용하는 DDD 원칙에 맞는 설계입니다.
updateStatus메서드가 새로운 인스턴스를 반환하여 불변성을 유지하는 것도 좋습니다.
27-76: 팩토리 메서드 구현이 적절합니다
create와reconstruct팩토리 메서드가 각각의 역할을 명확히 하고 있습니다. 새로운 엔티티 생성과 기존 엔티티 재구성을 분리하여 도메인 로직이 명확합니다.apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt (3)
119-146: Upsert 엔드포인트의 RESTful 설계 및 응답 코드 재검토 필요
/upsert경로는 작업 의도를 표현하지 못합니다.PUT /my-library/{isbn}(업데이트) 또는POST /my-library(등록)처럼 자원 중심 URI로 분리하는 것이 REST 원칙에 더 적합합니다.@PutMapping에서 생성 성공 시201 Created를 반환하려면 Location 헤더 등 추가 메타가 필요합니다. 그렇지 않다면200 OK또는204 No Content가 일반적입니다.
[ suggest_essential_refactor ]
124-144: Swagger 오류 응답 스키마 누락모든 실패 케이스(
400,404)에 대해ErrorResponse스키마를 명시해달라는 이전 피드백이 아직 반영되지 않았습니다.ApiResponse( responseCode = "400", description = "잘못된 요청 파라미터", content = [Content(schema = Schema(implementation = ErrorResponse::class))] )[ duplicate_comment ]
160-164: 리스트 반환 시 OpenAPI 스키마 표기가 부정확함
List<UserBookResponse>를 반환하지만 단일 객체 스키마로 기재되어 있습니다. 배열 형태로 지정하거나array = Schema(...)옵션을 사용해야 문서/스펙 일치가 보장됩니다.
[ suggest_nitpick ]apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookCreateRequest.kt (2)
23-25: 표지 URL 필드에 공백 검증 추가 고려
coverImageUrl은 필수(non-null)인데@NotBlank없이 길이만 검증합니다. 빈 문자열도 허용되지 않도록@NotBlank를 함께 붙이면 의도 명확성이 높아집니다.
[ suggest_nitpick ]
28-31:!!사용 대신 명시적 검증으로 NPE 방지
validIsbn()등에서 바로!!를 사용하면 DTO 생성 과정에서 누락된 값이 있을 때 원인 파악이 어려운 NPE가 발생합니다.fun validIsbn(): String = requireNotNull(isbn) { "ISBN은 필수입니다." }와 같이
requireNotNull로 메시지를 제공해 주세요.
[ suggest_optional_refactor ]apis/src/main/kotlin/org/yapp/apis/book/usecase/BookUseCase.kt (2)
34-35: 조회 결과 미사용
userAuthService.findUserById(userId)의 반환 값을 사용하지 않고 있습니다. 검증 목적이라면 존재하지 않을 때 예외가 발생하도록 메서드를 명확히 하거나, 결과 객체를 이후 로직에서 활용해 주세요.
[ duplicate_comment ]
42-47: 읽기 전용 메서드에@Transactional(readOnly = true)명시 권장
getUserLibraryBooks는 조회 전용이므로 메서드 레벨에@Transactional(readOnly = true)를 추가하면 JPA 플러시·락 비용을 줄일 수 있습니다.
[ duplicate_comment ]
| val optResult: List<String>? = null | ||
| ) { | ||
|
|
||
| fun validIsbn(): String = itemId!! |
There was a problem hiding this comment.
🛠️ Refactor suggestion
잠재적 NPE 위험: null 단언 연산자 사용
itemId!! 사용은 NPE 위험이 있습니다. 검증 어노테이션이 있더라도 런타임에서 null이 될 수 있는 상황을 고려해야 합니다.
- fun validIsbn(): String = itemId!!
+ fun validIsbn(): String = itemId ?: throw IllegalStateException("ISBN이 유효하지 않습니다.")📝 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.
| fun validIsbn(): String = itemId!! | |
| fun validIsbn(): String = itemId ?: throw IllegalStateException("ISBN이 유효하지 않습니다.") |
🤖 Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookDetailRequest.kt at
line 19, the use of the null assertion operator (!!) on itemId can cause a
potential NullPointerException at runtime. To fix this, replace the null
assertion with a safe null check or handle the null case explicitly by either
returning a default value, throwing a custom exception, or using Kotlin's safe
call operators to avoid runtime crashes.
apis/src/main/kotlin/org/yapp/apis/book/service/BookManagementService.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt (1)
27-129: 컨트롤러 책임 분리를 고려해보세요.현재
BookControllerApi가 도서 검색/상세 조회와 사용자 서재 관리라는 서로 다른 도메인을 담당하고 있습니다. 단일 책임 원칙에 따라 사용자 서재 관련 기능을 별도의 컨트롤러로 분리하는 것을 권장합니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apis/src/main/kotlin/org/yapp/apis/auth/controller/AuthControllerApi.kt(5 hunks)apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt(3 hunks)apis/src/main/kotlin/org/yapp/apis/book/service/BookManagementService.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt (2)
Learnt from: hoonyworld
PR: YAPP-Github/Reed-Server#31
File: apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt:13-13
Timestamp: 2025-07-08T14:24:44.340Z
Learning: IntelliJ IDEA에서 와일드카드 임포트를 방지하는 설정 방법: File > Settings > Editor > Code Style > Java > Imports 탭에서 "Class count to use import with '*'" 값을 높게 설정(예: 500)하면 해당 임계값 이하에서는 와일드카드 임포트가 자동으로 사용되지 않음. 와일드카드 임포트 관련 리뷰 시 이 IDE 설정 방법도 함께 안내하면 좋음.
Learnt from: hoonyworld
PR: YAPP-Github/Reed-Server#31
File: apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt:13-13
Timestamp: 2025-07-08T14:24:44.340Z
Learning: hoonyworld가 와일드카드 임포트 관련하여 IDE 설정을 통한 자동 최적화 방법을 추천했습니다. 향후 임포트 관련 리뷰 시 IDE 설정 방법도 함께 안내하는 것이 좋겠습니다.
⏰ 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 (7)
apis/src/main/kotlin/org/yapp/apis/book/service/BookManagementService.kt (1)
13-13: 컴파일 에러: BookQueryService import 누락
BookQueryService를 사용하고 있지만 import 문이 누락되어 있습니다. 이는 컴파일 에러를 발생시킵니다.다음과 같이 import를 추가하세요:
import org.yapp.domain.book.Book import org.yapp.domain.service.domain.BookDomainService +import org.yapp.apis.book.service.BookQueryServiceLikely an incorrect or invalid review comment.
apis/src/main/kotlin/org/yapp/apis/auth/controller/AuthControllerApi.kt (4)
11-15: 임포트 개선사항 승인구체적인 Spring Web 어노테이션 임포트와
AuthenticationPrincipal,UUID,ErrorResponse임포트 추가가 적절합니다. 와일드카드 임포트 대신 필요한 클래스만 명시적으로 임포트하여 코드 가독성이 향상되었습니다.Also applies to: 20-21
40-42: Swagger 문서화 개선사항 승인PR 피드백에서 지적된
ErrorResponse명세 누락 문제를 정확히 해결했습니다. 모든 에러 응답에content = @Content(schema = @Schema(implementation = ErrorResponse::class))추가로 API 문서의 완성도가 크게 향상되었습니다.Also applies to: 45-47, 66-68, 71-73, 85-87, 103-105
79-79: API 문서 설명 개선사항 승인operation summary와 description의 포맷팅 및 내용 개선이 적절합니다. 더 간결하고 명확한 API 문서가 되었습니다.
Also applies to: 82-82, 85-85, 93-93
91-91: 컨트롤러 시그니처 일관성 확인됨 – 인증 Principal(UUID) 설정 검증 필요컨트롤러(및 API) 전반에서
@AuthenticationPrincipal userId: UUID사용이 일치합니다. 이제 실제 인증 시스템에서Authentication.getPrincipal()이UUID를 반환하도록 설정되어 있는지 아래 위치들을 확인해주세요.• apis/src/main/kotlin/org/yapp/apis/auth/controller/AuthControllerApi.kt
–fun signOut(@AuthenticationPrincipal userId: UUID)
–fun getUserProfile(@AuthenticationPrincipal userId: UUID)
• apis/src/main/kotlin/org/yapp/apis/auth/controller/AuthController.kt
–override fun signOut(@AuthenticationPrincipal userId: UUID)
–override fun getUserProfile(@AuthenticationPrincipal userId: UUID)
• apis/src/main/kotlin/org/yapp/apis/book/controller/BookController*.kt 및 BookControllerApi.kt
–@AuthenticationPrincipal userId: UUID사용 메서드 전반
• Security 설정 (예:SecurityConfig, JWT 필터 등)
–AuthenticationManager/AuthenticationConverter에서principal에UUID값을 할당하는지 검증apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt (2)
12-24: Import 구문 개선이 잘 되었습니다.과거 리뷰에서 지적된 와일드카드 임포트 문제가 해결되었고, 새로운 기능을 위한 필요한 클래스들이 명시적으로 임포트되었습니다. 특히
ErrorResponse임포트 추가로 API 문서화가 개선될 것으로 보입니다.
31-70: API 문서화 개선이 훌륭합니다.기존 엔드포인트들에
ErrorResponse스키마가 추가되어 에러 응답의 구조가 명확해졌습니다. PR 목표에서 언급된 Swagger API 문서화 개선이 잘 반영되었습니다.
apis/src/main/kotlin/org/yapp/apis/book/service/BookManagementService.kt
Outdated
Show resolved
Hide resolved
apis/src/main/kotlin/org/yapp/apis/book/service/BookManagementService.kt
Outdated
Show resolved
Hide resolved
| @Operation(summary = "서재에 책 등록 또는 상태 업데이트 (Upsert)", description = "사용자의 서재에 책을 등록하거나, 이미 등록된 책의 상태를 업데이트합니다.") | ||
| @ApiResponses( | ||
| value = [ | ||
| ApiResponse( | ||
| responseCode = "201", | ||
| description = "책이 서재에 성공적으로 등록되었습니다.", | ||
| content = [Content(schema = Schema(implementation = UserBookResponse::class))] | ||
| ), | ||
| ApiResponse( | ||
| responseCode = "200", | ||
| description = "책 상태가 성공적으로 업데이트되었습니다.", | ||
| content = [Content(schema = Schema(implementation = UserBookResponse::class))] | ||
| ), | ||
| ApiResponse( | ||
| responseCode = "400", | ||
| description = "잘못된 요청 파라미터", | ||
| content = [Content(schema = Schema(implementation = ErrorResponse::class))] | ||
| ), | ||
| ApiResponse( | ||
| responseCode = "404", | ||
| description = "존재하지 않는 책 (ISBN 오류)", | ||
| content = [Content(schema = Schema(implementation = ErrorResponse::class))] | ||
| ) | ||
| ] | ||
| ) | ||
| @PutMapping("/upsert") | ||
| fun upsertBookToMyLibrary( | ||
| @AuthenticationPrincipal userId: UUID, | ||
| @Valid @RequestBody request: UserBookRegisterRequest | ||
| ): ResponseEntity<UserBookResponse> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
인증 처리와 HTTP 메서드 선택이 개선되었습니다.
과거 리뷰에서 제안된 사항들이 잘 반영되었습니다:
@PutMapping사용으로 멱등성 보장@AuthenticationPrincipal사용으로 인증 처리 개선- 적절한 HTTP 상태 코드와
ErrorResponse스키마 사용
다만 /upsert 경로명은 여전히 RESTful하지 않습니다. /my-library 또는 /library/books와 같은 리소스 중심의 경로명을 고려해보세요.
🤖 Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
around lines 72 to 101, the endpoint path "/upsert" is not RESTful. Rename the
path to a resource-oriented name such as "/my-library" or "/library/books" to
better align with RESTful API design principles while keeping the existing
method, annotations, and functionality intact.
| @Operation(summary = "사용자 서재 조회", description = "현재 사용자의 서재에 등록된 모든 책을 조회합니다.") | ||
| @ApiResponses( | ||
| value = [ | ||
| ApiResponse( | ||
| responseCode = "200", | ||
| description = "서재 조회 성공", | ||
| content = [Content(schema = Schema(implementation = UserBookResponse::class))] | ||
| ), | ||
| ApiResponse( | ||
| responseCode = "400", | ||
| description = "Invalid request or credentials", | ||
| content = [Content(schema = Schema(implementation = ErrorResponse::class))] | ||
| ), | ||
| ApiResponse( | ||
| responseCode = "409", | ||
| description = "Email already in use with a different account", | ||
| content = [Content(schema = Schema(implementation = ErrorResponse::class))] | ||
| ), | ||
| ApiResponse( | ||
| responseCode = "404", | ||
| description = "사용자를 찾을 수 없습니다.", | ||
| content = [Content(schema = Schema(implementation = ErrorResponse::class))] | ||
| ) | ||
| ] | ||
| ) | ||
| @GetMapping("/my-library") | ||
| fun getUserLibraryBooks(@AuthenticationPrincipal userId: UUID): ResponseEntity<List<UserBookResponse>> |
There was a problem hiding this comment.
ApiResponses 설명이 현재 API와 맞지 않습니다.
라인 113, 118-119의 description들이 다른 API (인증 관련)에서 복사된 것으로 보입니다. 사용자 서재 조회 API에 맞게 수정해주세요.
- responseCode = "400",
- description = "Invalid request or credentials",
+ responseCode = "400",
+ description = "잘못된 요청",
content = [Content(schema = Schema(implementation = ErrorResponse::class))]
),
ApiResponse(
- responseCode = "409",
- description = "Email already in use with a different account",
- content = [Content(schema = Schema(implementation = ErrorResponse::class))]
- ),
- ApiResponse(
responseCode = "404",
description = "사용자를 찾을 수 없습니다.",
content = [Content(schema = Schema(implementation = ErrorResponse::class))]또한 409 상태 코드는 이 API에서 발생하지 않을 것 같으니 제거하는 것이 좋겠습니다.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/book/controller/BookControllerApi.kt
between lines 103 and 129, the ApiResponses descriptions for status codes 400,
404, and 409 do not match the user library books retrieval API and seem copied
from an authentication API. Update the descriptions for 400 and 404 to reflect
errors relevant to this API, such as invalid request parameters or user not
found in the library context, and remove the 409 response entirely since it is
not applicable here.
move-hoon
left a comment
move-hoon
left a comment
There was a problem hiding this comment.
제가 좀 길게 코멘트를 남겼는데, 얼라인을 맞춰보면 좋을 것 같아 의견 남깁니다!!
There was a problem hiding this comment.
(사소) userAuthService.findUserById(userId);를 호출해서 존재하지 않는 userId에 대해서 validate 처리를 하고 있는데, user 객체를 가져올 경우도 동일한 메서드를 호출하기 때문에 validate용 메서드를 하나 만들면 좋을 것 같습니다 ㅎㅎ
fun validateUserExists(userId: UUID) {
if (!userDomainService.existsById(userId)) {
throw AuthException(AuthErrorCode.USER_NOT_FOUND, "User not found: $userId")
}
}
// 사용
@Transactional
fun upsertBookToMyLibrary(userId: UUID, request: UserBookRegisterRequest): UserBookResponse {
userAuthService.validateUserExists(userId) // 반환값 없음
val book = bookManagementService.findOrCreateBookByIsbn(request.validBookIsbn())
val userBook = userBookService.upsertUserBook(userId, book, request.bookStatus)
return UserBookResponse.from(userBook)
}
|
아키텍처 정리본 |
|
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
domain/src/main/kotlin/org/yapp/domain/user/ProviderType.kt (1)
1-8:ProviderType도메인 이동 적합성 점검
ProviderType가 user 도메인 루트로 이동하면서 인증·사용자 모듈 간 의존성이 단순화됐습니다.
다만 인증 전략·토큰 검증 등 auth 전용 로직에서도 동일 enum 을 사용하므로, user 모듈이 auth 모듈에 역으로 의존하지 않도록 레이어 의존성을 한 번 더 확인해 주세요. 현재 구조상 순환 의존은 보이지 않으나, 향후 모듈 분할 시 주의가 필요합니다.
♻️ Duplicate comments (1)
apis/src/main/kotlin/org/yapp/apis/book/usecase/BookUseCase.kt (1)
51-56: 읽기 전용 트랜잭션 어노테이션 추가 필요이 메서드는 읽기 전용 작업을 수행하므로 성능 최적화를 위해
@Transactional(readOnly = true)어노테이션을 추가해야 합니다.+ @Transactional(readOnly = true) fun getUserLibraryBooks(userId: UUID): List<UserBookResponse> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (29)
apis/src/main/kotlin/org/yapp/apis/auth/dto/AuthCredentials.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/auth/dto/UserCreateInfo.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/auth/dto/request/SocialLoginRequest.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/auth/dto/response/UserProfileResponse.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/auth/service/TokenService.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/auth/service/UserAuthService.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/auth/strategy/AppleAuthStrategy.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/auth/strategy/AuthStrategy.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/auth/strategy/KakaoAuthStrategy.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/constant/BookQueryServiceQualifier.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/controller/BookController.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookCreateRequest.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/service/BookManagementService.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/service/BookQueryService.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/service/UserBookService.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/service/impl/AladinBookQueryService.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/book/usecase/BookUseCase.kt(1 hunks)apis/src/main/kotlin/org/yapp/apis/util/AuthUtils.kt(0 hunks)domain/src/main/kotlin/org/yapp/domain/book/BookDomainService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/token/TokenDomainRedisService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/user/ProviderType.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/user/User.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/user/UserDomainService.kt(2 hunks)domain/src/main/kotlin/org/yapp/domain/user/UserRepository.kt(2 hunks)domain/src/main/kotlin/org/yapp/domain/user/vo/SocialUserProfile.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/userbook/UserBookDomainService.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/user/entity/UserEntity.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/user/repository/JpaUserRepository.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/user/repository/impl/UserRepositoryImpl.kt(2 hunks)
💤 Files with no reviewable changes (1)
- apis/src/main/kotlin/org/yapp/apis/util/AuthUtils.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 (38)
domain/src/main/kotlin/org/yapp/domain/token/TokenDomainRedisService.kt (1)
5-5: 와일드카드 import 제거 👍
java.util.*→java.util.UUID로 명시적 import 변경이 가독성과 빌드 성능 모두에 긍정적입니다.apis/src/main/kotlin/org/yapp/apis/auth/service/TokenService.kt (1)
6-6: 레거시 경로org.yapp.domain.service.redis.TokenDomainRedisService미사용 확인됨
PR 전체에서 해당 경로를 참조하는 코드가 더 이상 존재하지 않음을 검증했습니다. 안전하게 코드 변경을 승인합니다.domain/src/main/kotlin/org/yapp/domain/user/vo/SocialUserProfile.kt (1)
3-3: 패키지 위치 수정 검증 – 잔존 레퍼런스 수동 확인 권장
org.yapp.domain.auth.ProviderType경로를 검색한 결과(rg 실행) 잔여 참조는 발견되지 않았습니다.
자동 검색만으로는 누락 가능성이 있으니, 아래와 같이 전역 검색을 한 번 더 수행해 수동으로 확인해 주세요.
- 검색 예시
rg -n 'org.yapp.domain.auth.ProviderType'apis/src/main/kotlin/org/yapp/apis/auth/dto/AuthCredentials.kt (1)
3-3: 패키지 경로 정리 완료import 경로 수정이 일관성 있게 반영되었습니다. 별도 이슈 없습니다.
apis/src/main/kotlin/org/yapp/apis/auth/strategy/AppleAuthStrategy.kt (1)
12-12: import 경로 갱신 확인
ProviderType패키지 변경이 정상 반영되었습니다. 추가 조치 사항 없습니다.apis/src/main/kotlin/org/yapp/apis/auth/strategy/AuthStrategy.kt (1)
5-5: ProviderType 패키지 경로 수정 반영 완료 확인
레거시 import(org.yapp.domain.auth.ProviderType)가 코드베이스 전역에서 제거된 것을 확인했습니다.
- apis/src/main/kotlin/org/yapp/apis/auth/strategy/AuthStrategy.kt
import org.yapp.domain.user.ProviderTypeinfra/src/main/kotlin/org/yapp/infra/user/entity/UserEntity.kt (1)
6-6: ProviderType 패키지 경로 업데이트 완료엔티티에서도 동일한 패키지 경로로 통일되었습니다. 도메인 ‑ 인프라 간 타입 불일치 위험이 제거되었습니다.
domain/src/main/kotlin/org/yapp/domain/user/User.kt (1)
3-3: 도메인 모델 import 정정 완료도메인 내부 참조 역시
user패키지로 일관성 있게 수정되었습니다.
추가 조치 필요 없습니다.apis/src/main/kotlin/org/yapp/apis/auth/dto/UserCreateInfo.kt (1)
3-3: DTO import 경로 통일DTO 계층에서도 패키지 이동이 반영되어 타입 충돌 가능성이 사라졌습니다.
infra/src/main/kotlin/org/yapp/infra/user/repository/JpaUserRepository.kt (1)
5-5: 리포지토리 import 경로 수정리포지토리에서도 최신
ProviderType경로로 맞춰졌습니다.
JPA 쿼리 메서드 시그니처와 일치함을 확인했습니다.apis/src/main/kotlin/org/yapp/apis/auth/dto/response/UserProfileResponse.kt (1)
4-4: DDD 패키지 구조 개선 승인
ProviderType을auth도메인에서user도메인으로 이동한 것은 DDD 원칙에 부합하는 올바른 설계입니다.ProviderType은 인증 로직보다는 User 엔티티의 속성에 더 가깝기 때문입니다.apis/src/main/kotlin/org/yapp/apis/auth/strategy/KakaoAuthStrategy.kt (1)
12-12: 일관된 패키지 구조 개선다른 인증 관련 파일들과 일관되게
ProviderTypeimport를user도메인으로 변경한 것이 좋습니다. 인증 전략 로직은 그대로 유지되면서 패키지 구조만 개선되었습니다.apis/src/main/kotlin/org/yapp/apis/auth/dto/request/SocialLoginRequest.kt (1)
10-10: 소셜 로그인 DTO의 패키지 구조 개선
SocialLoginRequest에서도ProviderTypeimport를 일관되게 변경한 것이 좋습니다.valueOf()및when문에서의 사용 패턴이 user 도메인에 더 적합합니다.domain/src/main/kotlin/org/yapp/domain/user/UserRepository.kt (2)
3-3: 도메인 레포지토리 패키지 구조 개선도메인 레포지토리에서도
ProviderTypeimport를 user 도메인으로 변경한 것이 DDD 원칙에 부합합니다.
21-21: 사용자 존재 확인 메서드 추가 승인
existsById메서드 추가는 표준 repository 패턴을 따르며 성능상 이점이 있습니다. 전체 User 엔티티를 조회하지 않고 존재 여부만 확인할 수 있어 불필요한 데이터 조회를 방지합니다.infra/src/main/kotlin/org/yapp/infra/user/repository/impl/UserRepositoryImpl.kt (2)
4-4: 인프라 레이어 패키지 구조 개선인프라 레이어에서도
ProviderTypeimport를 일관되게 변경한 것이 좋습니다.
36-38: 사용자 존재 확인 메서드 구현 승인
existsById메서드의 구현이 올바릅니다. JPA repository의existsById에 적절히 위임하여 성능 최적화된 존재 확인을 제공합니다.apis/src/main/kotlin/org/yapp/apis/auth/service/UserAuthService.kt (2)
8-8: 임포트 패키지 경로 업데이트 확인됨UserDomainService의 임포트 경로가 올바르게 업데이트되었습니다.
60-64: 사용자 존재 검증 로직이 잘 구현되었습니다새로 추가된
validateUserExists메서드는 DDD 패턴을 잘 따르고 있으며, 도메인 서비스에 적절히 위임하고 있습니다. 예외 처리도 명확하고 일관성 있게 구현되었습니다.domain/src/main/kotlin/org/yapp/domain/user/UserDomainService.kt (2)
1-1: 패키지 구조 개선이 적절합니다도메인 서비스를
org.yapp.domain.user패키지로 이동한 것은 DDD의 도메인 모듈 구조에 잘 맞습니다.
49-51: 효율적인 존재 확인 메서드 구현
existsById메서드는 전체 엔티티를 로드하지 않고 존재 여부만 확인하는 효율적인 구현입니다. 리포지토리 계층에 적절히 위임하고 있어 좋습니다.apis/src/main/kotlin/org/yapp/apis/book/service/BookQueryService.kt (1)
9-12: 도서 조회 인터페이스가 잘 설계되었습니다
BookQueryService인터페이스는 책임이 명확하게 분리되어 있고, 전략 패턴을 활용해 다양한 도서 데이터 소스 구현을 가능하게 합니다. 메서드 시그니처도 일관성 있게 설계되었습니다.apis/src/main/kotlin/org/yapp/apis/book/service/BookManagementService.kt (2)
8-11: 서비스 클래스 구조가 적절합니다
BookManagementService는 도서 엔티티 관리 책임을 명확히 하고 있으며, 도메인 서비스에 적절히 위임하는 구조입니다.
12-25: findOrCreate 패턴이 잘 구현되었습니다ISBN으로 기존 도서를 조회하고, 없으면 새로 생성하는 로직이 명확하게 구현되어 있습니다. 요청 객체에서 검증된 필드들을 추출하여 사용하는 것도 좋은 접근입니다.
apis/src/main/kotlin/org/yapp/apis/book/service/UserBookService.kt (2)
9-12: 서비스 래퍼 구조가 깔끔합니다
UserBookService는 도메인 서비스를 감싸는 깔끔한 래퍼 역할을 하고 있으며, API 계층과 도메인 계층 간의 명확한 분리를 제공합니다.
13-17: 도메인 서비스 위임이 적절합니다
upsertUserBook과findAllUserBooks메서드 모두 도메인 서비스에 적절히 위임하고 있으며, 이전 리뷰에서 지적된 불필요한 BookStatus 변환 문제도 해결되었습니다.apis/src/main/kotlin/org/yapp/apis/book/controller/BookController.kt (2)
5-5: 이전 리뷰 피드백이 잘 반영되었습니다!
@AuthenticationPrincipal어노테이션을 통한 사용자 인증 처리가 적절하게 구현되었고, AuthUtil 대신 Spring Security의 표준 방식을 사용한 점이 좋습니다.Also applies to: 43-43, 52-52
23-25: 단일 의존성 구조가 잘 구현되었습니다.BookController가 BookUseCase 하나만 의존하고 있어서 이전 리뷰에서 합의한 아키텍처 방향(블로그의 3번 방식)과 일치합니다. 테스트 용이성과 결합도 측면에서 좋은 구조입니다.
domain/src/main/kotlin/org/yapp/domain/userbook/UserBookDomainService.kt (2)
7-10: DDD 도메인 서비스가 잘 설계되었습니다.
@DomainService어노테이션과 함께 UserBook 도메인의 핵심 비즈니스 로직을 적절히 캡슐화하고 있습니다.
12-21: upsert 로직이 명확하고 안전합니다.기존 사용자 도서 레코드 존재 여부를 확인한 후 업데이트 또는 생성으로 분기하는 로직이 명확하며, 도메인 객체의 팩토리 메서드를 활용한 점도 좋습니다.
apis/src/main/kotlin/org/yapp/apis/book/service/impl/AladinBookQueryService.kt (2)
1-1: 인터페이스 기반 리팩토링이 잘 수행되었습니다.패키지 구조 개선(
service.impl로 이동)과@Qualifier를 통한 구현체 구분으로 더 나은 추상화와 확장성을 제공합니다.Also applies to: 17-17, 20-20
31-31: 메서드명 통일과 명시적 호출이 개선되었습니다.
getBookDetail로 메서드명을 통일하고Companion.from을 명시적으로 호출하여 코드 가독성이 향상되었습니다.Also applies to: 35-35
domain/src/main/kotlin/org/yapp/domain/book/BookDomainService.kt (2)
5-8: 도메인 서비스 구조가 적절합니다.
@DomainService어노테이션과 함께 Book 도메인의 핵심 로직을 잘 캡슐화하고 있습니다.
22-24: 중복 생성 방지 로직이 효율적입니다.기존 도서가 있는 경우 바로 반환하여 불필요한 중복 생성을 방지하는 로직이 좋습니다.
apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookCreateRequest.kt (1)
50-55: 출간년도 파싱 로직이 안전하게 구현되었습니다.null 체크와 숫자 검증을 통한 안전한 파싱 로직이 잘 구현되어 있습니다.
apis/src/main/kotlin/org/yapp/apis/book/usecase/BookUseCase.kt (3)
3-18: 새로운 기능을 위한 임포트 추가 확인사용자 도서 관리 기능을 위한 필요한 임포트들이 적절히 추가되었습니다.
29-31: 인터페이스 사용으로 개선됨기존 구체 클래스 대신 BookQueryService 인터페이스를 사용하도록 개선되었습니다.
33-35: BookDetailRequest 사용으로 개선됨메서드 시그니처가 BookDetailRequest 객체를 받도록 개선되어 타입 안전성이 향상되었습니다.
| @@ -1,10 +1,8 @@ | |||
| package org.yapp.domain.service.redis | |||
| package org.yapp.domain.token | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
도메인 계층에서 Redis 명시는 의존성 방향을 흐릴 수 있습니다
패키지를 org.yapp.domain.token으로 이동하면서도 클래스명이 TokenDomainRedisService(Redis)로 남아 있습니다. 도메인 레이어는 기술 세부사항(저장소 구현)을 모르도록 설계하는 것이 DDD 원칙에 부합합니다.
TokenDomainService 등으로 네이밍을 단순화하고, 실제 Redis 구현체는 인프라 레이어로 분리해 스프링 빈으로 주입받도록 리팩터링을 고려해 주세요.
🤖 Prompt for AI Agents
In domain/src/main/kotlin/org/yapp/domain/token/TokenDomainRedisService.kt at
line 1, the class name includes "Redis," which exposes a technical detail in the
domain layer, violating DDD principles. Rename the class to a more generic name
like TokenDomainService to remove the Redis reference. Then, move the actual
Redis implementation to the infrastructure layer and inject it as a Spring bean
into the domain service to keep the domain layer technology-agnostic.
| object BookQueryServiceQualifier { | ||
| const val ALADIN = "aladinBookQueryService" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
문자열 Qualifier 대신 전용 애노테이션 고려
상수 문자열로 Qualifier 를 관리하면 오타로 인한 런타임 오류 가능성이 남아 있습니다.
전용 애노테이션을 정의하면 컴파일타임 안전성을 확보할 수 있으니 검토해 보세요.
-package org.yapp.apis.book.constant
-
-object BookQueryServiceQualifier {
- const val ALADIN = "aladinBookQueryService"
-}
+package org.yapp.apis.book.constant
+
+import org.springframework.beans.factory.annotation.Qualifier
+
+@Qualifier
+@Target(
+ AnnotationTarget.FUNCTION,
+ AnnotationTarget.CLASS,
+ AnnotationTarget.VALUE_PARAMETER,
+ AnnotationTarget.FIELD
+)
+@Retention(AnnotationRetention.RUNTIME)
+annotation class AladinBookQuery서비스/주입부에서는 @AladinBookQuery 로 대체하면 됩니다.
📝 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.
| object BookQueryServiceQualifier { | |
| const val ALADIN = "aladinBookQueryService" | |
| } | |
| package org.yapp.apis.book.constant | |
| import org.springframework.beans.factory.annotation.Qualifier | |
| @Qualifier | |
| @Target( | |
| AnnotationTarget.FUNCTION, | |
| AnnotationTarget.CLASS, | |
| AnnotationTarget.VALUE_PARAMETER, | |
| AnnotationTarget.FIELD | |
| ) | |
| @Retention(AnnotationRetention.RUNTIME) | |
| annotation class AladinBookQuery |
🤖 Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/book/constant/BookQueryServiceQualifier.kt
around lines 3 to 5, replace the string constant qualifier with a dedicated
annotation to improve compile-time safety and avoid runtime errors from typos.
Define a new annotation class named AladinBookQuery and use it as a qualifier in
service and injection points instead of the string "aladinBookQueryService".
| fun validIsbn(): String = isbn!! | ||
| fun validTitle(): String = title!! | ||
| fun validAuthor(): String = author!! | ||
| fun validPublisher(): String = publisher!! |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
validation 헬퍼 메서드의 안전성을 개선해 주세요.
!! 연산자를 사용한 validation 헬퍼 메서드들이 validation이 실패했을 때 NPE를 발생시킬 수 있습니다. requireNotNull()이나 Elvis 연산자를 사용하여 더 명확한 에러 메시지를 제공하는 것을 고려해 보세요.
- fun validIsbn(): String = isbn!!
- fun validTitle(): String = title!!
- fun validAuthor(): String = author!!
- fun validPublisher(): String = publisher!!
+ fun validIsbn(): String = requireNotNull(isbn) { "ISBN이 null입니다." }
+ fun validTitle(): String = requireNotNull(title) { "제목이 null입니다." }
+ fun validAuthor(): String = requireNotNull(author) { "저자가 null입니다." }
+ fun validPublisher(): String = requireNotNull(publisher) { "출판사가 null입니다." }🤖 Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookCreateRequest.kt
around lines 28 to 31, the validation helper methods use the !! operator which
can cause NullPointerExceptions without clear error messages. Replace the !!
operator with requireNotNull() or use the Elvis operator to provide explicit
error messages when the values are null, improving safety and clarity of
validation failures.
| val publicationYear: Int? = null, | ||
|
|
||
| @field:Size(max = 2048, message = "표지 URL은 2048자 이내여야 합니다.") | ||
| val coverImageUrl: String, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
coverImageUrl validation annotation이 누락되었습니다.
coverImageUrl이 non-null 필드임에도 @NotBlank validation annotation이 누락되어 있습니다. 일관성을 위해 추가해 주세요.
+ @field:NotBlank(message = "표지 URL은 필수입니다.")
@field:Size(max = 2048, message = "표지 URL은 2048자 이내여야 합니다.")
val coverImageUrl: String,📝 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 coverImageUrl: String, | |
| @field:NotBlank(message = "표지 URL은 필수입니다.") | |
| @field:Size(max = 2048, message = "표지 URL은 2048자 이내여야 합니다.") | |
| val coverImageUrl: String, |
🤖 Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/book/dto/request/BookCreateRequest.kt at
line 24, the coverImageUrl field lacks the @NotBlank validation annotation
despite being non-null. Add the @NotBlank annotation above the coverImageUrl
declaration to ensure consistent validation and enforce that the field is not
blank.
| @Transactional | ||
| fun upsertBookToMyLibrary(userId: UUID, request: UserBookRegisterRequest): UserBookResponse { | ||
| userAuthService.validateUserExists(userId) | ||
|
|
||
| val detail = bookQueryService.getBookDetail(BookDetailRequest.of(request.validBookIsbn())) | ||
|
|
||
| val book = bookManagementService.findOrCreateBook(BookCreateRequest.create(detail)) | ||
|
|
||
| val userBook = userBookService.upsertUserBook(userId, book, request.bookStatus) | ||
|
|
||
| return UserBookResponse.from(userBook) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
UseCase 오케스트레이션 로직 개선 필요
메서드 구현은 올바른 트랜잭션 처리와 적절한 단계별 로직을 보여줍니다. 하지만 이전 리뷰에서 논의된 아키텍처 개선사항을 반영하면 더 좋을 것 같습니다.
현재 BookManagementService.findOrCreateBook() 내부에서 여전히 오케스트레이션이 발생하고 있을 가능성이 있습니다. UseCase에서 완전한 오케스트레이션을 담당하도록 리팩터링을 고려해보세요.
이전 리뷰에서 제안된 구조:
private fun findOrCreateBookByIsbn(validIsbn: String): Book {
val existingBook = bookManagementService.findBookByIsbn(validIsbn)
if (existingBook != null) return existingBook
val detail = bookQueryService.getBookDetail(BookDetailRequest.of(validIsbn))
return bookManagementService.createBook(detail)
}🤖 Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/book/usecase/BookUseCase.kt around lines
37 to 48, refactor the upsertBookToMyLibrary method to move the orchestration
logic out of BookManagementService.findOrCreateBook and into the UseCase layer.
Implement a private method in the UseCase that first tries to find the book by
ISBN using bookManagementService.findBookByIsbn, and if not found, fetches the
book detail from bookQueryService and then creates the book via
bookManagementService.createBook. Replace the call to findOrCreateBook with this
new method to ensure the UseCase fully controls the orchestration flow.
| class BookUseCase( | ||
| private val aladinBookQueryService: AladinBookQueryService | ||
| private val userAuthService: UserAuthService, | ||
| private val userBookService: UserBookService, | ||
| @Qualifier(BookQueryServiceQualifier.ALADIN) | ||
| private val bookQueryService: BookQueryService, | ||
| private val bookManagementService: BookManagementService | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
의존성 주입 개선 및 아키텍처 검토 필요
BookQueryService 인터페이스와 @qualifier 사용은 좋은 개선사항입니다. 하지만 이전 리뷰에서 논의된 Application Service 간 직접 호출 이슈가 여전히 남아있습니다.
현재 UseCase가 4개의 Application Service에 직접 의존하고 있어 오케스트레이션 책임이 분산되어 있습니다.
🤖 Prompt for AI Agents
In apis/src/main/kotlin/org/yapp/apis/book/usecase/BookUseCase.kt around lines
22 to 28, the BookUseCase class directly depends on four Application Services,
causing dispersed orchestration responsibility. Refactor to reduce direct
dependencies by introducing a dedicated orchestration layer or service that
coordinates interactions between these Application Services. Adjust BookUseCase
to depend on this new orchestrator instead of multiple services, improving
separation of concerns and maintainability.
|
어제 회의에서 논의한 방향에 따라, UseCase에서는 DTO와 필드 값을 혼용해서 넘기는 형태로 구성했습니다. 필요한 경우에는 DTO 전체를 넘기고, 가공이 필요한 경우에는 필드 단위로 분리해서 넘기는 방식이에요. 결국 우리의 목적은 유연한 유스케이스 작성 가능 Domain Layer는 항상 순수하게 유지 혹시 더 나은 방향이나 우려되는 부분 있으면 말씀 주세요! |
🔗 관련 이슈
📘 작업 유형
📙 작업 내역
🏗️ DDD 구조 개선 및 UserBook 도메인 리팩토링
📦 Domain 모듈
UserBook 도메인 모델 개선
registeredAt,lastStatusUpdateAt→createdAt,updatedAt,deletedAt으로 표준화reconstruct메서드 완성으로 도메인 객체 생성 로직 안정화org.yapp.domain.userbook패키지로 통합도메인 서비스 추가
UserBookDomainService: UserBook 관련 핵심 비즈니스 로직 처리BookDomainService: Book 조회/생성 로직 처리🔧 APIs 모듈
서비스 계층 재구성
UserBookService: 도메인 서비스를 활용한 애플리케이션 서비스BookService: 알라딘 API 연동 및 Book 생성 로직 처리UserBookCommandQueryService삭제 후 새로운 구조로 대체UseCase 리팩토링
BookUseCase: 새로운 서비스 구조에 맞게 의존성 변경🗄️ Infra 모듈
BaseTimeEntity활용으로 공통 필드 관리fromDomain/toDomain메서드 개선🔍 주요 수정사항
🧪 테스트 내역
🎨 스크린샷 또는 시연 영상 (선택)
✅ PR 체크리스트
💬 추가 설명 or 리뷰 포인트 (선택)
🔍 리뷰 포인트
다시 해당 데이터를 반환해주는 방향으로 구현하였는데 더 좋은 방향이 있으면 의견주세요~ 동훈님의 생각이 궁금합니다
🚨 주의사항
🔄 마이그레이션 영향도
Summary by CodeRabbit
신규 기능
버그 수정
개선 및 리팩터링
기타