[FIX] 모바일 테스트를 위한 ip허용#125
Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive state management for reading progress tracking by adding entity-level state transition methods to UserBooks, updating repositories to use user-scoped delete operations instead of global ones, extending query capabilities, refactoring service logic to leverage entity methods, and adding validation error statuses. CORS configuration is updated to support local network testing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserBooksService
participant UserBooks
participant ReadingLogsService
participant ReadingLogsRepository
Client->>UserBooksService: applyReadingProgress(currentPage, readDate)
UserBooksService->>ReadingLogsService: apply(userBook, currentPage, readDate)
ReadingLogsService->>ReadingLogsRepository: save/update reading log
ReadingLogsRepository-->>ReadingLogsService: persisted log
ReadingLogsService->>ReadingLogsService: recalcLogsAndUserBook()
ReadingLogsService->>UserBooks: setCurrentPage(computed)
UserBooks->>UserBooks: recalcProgressPercent()
ReadingLogsService->>UserBooks: changeStatus(status)
UserBooks->>UserBooks: apply state side effects
UserBooks-->>ReadingLogsService: updated
ReadingLogsService-->>UserBooksService: updated state
UserBooksService-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🤖 Fix all issues with AI agents
In
`@booklog/src/main/java/com/example/booklog/domain/library/shelves/repository/BookshelfItemsRepository.java`:
- Around line 65-73: The deleteByUserIdAndBookIds method in
BookshelfItemsRepository will throw at runtime if called with an empty bookIds
list; like the fix applied for
ReadingLogsRepository.deleteByUserIdAndUserBookIds, ensure callers verify the
list before invoking the repository: add a guard (e.g., if bookIds == null ||
bookIds.isEmpty()) return 0 (or no-op) in the service/handler that calls
BookshelfItemsRepository.deleteByUserIdAndBookIds so the repository is never
invoked with an empty collection.
In
`@booklog/src/main/java/com/example/booklog/domain/library/shelves/service/UserBooksService.java`:
- Around line 214-228: The inline comment in UserBooksService that suggests
adding a repository method is stale and misleading because
userBooksRepository.findUserBookIdsByUserIdAndStatus already exists; remove or
update that comment (the lines above the call to
findUserBookIdsByUserIdAndStatus) so the block that deletes
readingLogsRepository.deleteByUserIdAndUserBookIds,
bookshelfItemsRepository.deleteByUserIdAndBookIds, and the final
userBooksRepository.deleteByUserIdAndStatus call is accurate and clean, keeping
the surrounding logic intact.
🧹 Nitpick comments (7)
booklog/.gitignore (1)
8-10: Avoid ignoring allsrc/test/sources unless tests are intentionally excluded.This rule hides all tests from version control, which can reduce CI coverage and code review visibility. If the goal is to ignore generated test artifacts only, narrow the pattern (e.g.,
**/build/,**/out/) or add a rationale in repo docs.booklog/src/main/java/com/example/booklog/global/common/apiPayload/code/status/ErrorStatus.java (1)
71-72: Inconsistent naming pattern and missing punctuation.
- Naming inconsistency: Line 71 uses
TOTAL_PAGE_INVALIDwhile line 72 usesINVALID_TOTAL_PAGE. Consider using a consistent pattern (eitherX_INVALIDorINVALID_X).- The message for
INVALID_TOTAL_PAGEis missing a period at the end, unlike other messages.✏️ Suggested fix for consistency
- INVALID_TOTAL_PAGE(HttpStatus.BAD_REQUEST, "LB004", "총 페이지를 입력해야합니다"), + TOTAL_PAGE_REQUIRED(HttpStatus.BAD_REQUEST, "LB004", "총 페이지를 입력해야합니다."),booklog/src/main/java/com/example/booklog/global/config/WebConfig.java (1)
22-23: Consider restricting these patterns to development/test environments only.The
192.168.*.*:*patterns are very broad, allowing CORS requests from any device on any 192.168.x.x subnet. Since production domains are configured in the same file, this pattern will likely be active in production as well.While 192.168.x.x is a private IP range, best practice is to use Spring profiles to conditionally apply such permissive patterns only in non-production environments:
♻️ Suggested approach using profiles
// Option 1: Use `@Profile` annotation with separate config classes `@Configuration` `@Profile`("dev") public class DevWebConfig implements WebMvcConfigurer { // Include 192.168.*.* patterns here } `@Configuration` `@Profile`("prod") public class ProdWebConfig implements WebMvcConfigurer { // Production-only origins }Alternatively, externalize the allowed origins to
application-{profile}.ymland inject them via@Value.booklog/src/main/java/com/example/booklog/domain/library/shelves/service/ReadingLogsService.java (1)
76-81: Minor redundancy:log.getCurrentPage()will be overwritten.Line 79 passes
log.getCurrentPage()as the third argument, butrecalcLogsAndUserBookat line 81 will immediately recalculate and overwrite all logs'currentPagevalues. Consider passing0or documenting that this value is intentionally ignored.booklog/src/main/java/com/example/booklog/domain/library/shelves/entity/UserBooks.java (2)
79-100: Inconsistent null handling: silent return vs. throwing exception.
changeStatus(null)silently returns (line 80), whilesetTotalPages(null)andsetCurrentPage(null)throwGeneralException. Consider throwing an exception here too for consistent behavior and to help surface potential bugs at call sites.♻️ Suggested fix for consistent null handling
public void changeStatus(ReadingStatus newStatus) { - if (newStatus == null) return; + if (newStatus == null) { + throw new GeneralException(ErrorStatus.STATUS_INVALID); // or similar + } this.status = newStatus;
151-162: Avoid reassigning method parameters.Line 157 reassigns the
currentPageparameter, which can reduce clarity. Consider using a local variable instead.♻️ Suggested refactor
public void setCurrentPage(Integer currentPage) { if (currentPage == null || currentPage < 0) { throw new GeneralException(ErrorStatus.CURRENT_PAGE_INVALID); } + int clamped = currentPage; if (this.pageCountSnapshot != null && currentPage > this.pageCountSnapshot) { - currentPage = this.pageCountSnapshot; // clamp 정책 + clamped = this.pageCountSnapshot; // clamp 정책 } - this.currentPage = currentPage; + this.currentPage = clamped; recalcProgressPercent(); }booklog/src/main/java/com/example/booklog/domain/library/shelves/service/UserBooksService.java (1)
54-59: RedundantchangeStatuscalls after builder already sets status.The builder at line 51 already sets the status.
changeStatusis then called again, which while it setsstartDate/endDateas side effects, also reassignsthis.statusunnecessarily. Consider extracting only the date-setting logic or documenting why the double assignment is intentional.
| /** ✅ (필수) 내 서재들에서만 bookIds 제거 */ | ||
| @Modifying(clearAutomatically = true, flushAutomatically = true) | ||
| @Query(""" | ||
| delete from BookshelfItems bi | ||
| where bi.book.id in :bookIds | ||
| and bi.shelf.user.id = :userId | ||
| """) | ||
| int deleteByUserIdAndBookIds(@Param("userId") Long userId, | ||
| @Param("bookIds") List<Long> bookIds); |
There was a problem hiding this comment.
Same empty-list concern applies here.
Similar to ReadingLogsRepository.deleteByUserIdAndUserBookIds, passing an empty bookIds list will cause a runtime exception. Ensure callers check for emptiness before calling.
🤖 Prompt for AI Agents
In
`@booklog/src/main/java/com/example/booklog/domain/library/shelves/repository/BookshelfItemsRepository.java`
around lines 65 - 73, The deleteByUserIdAndBookIds method in
BookshelfItemsRepository will throw at runtime if called with an empty bookIds
list; like the fix applied for
ReadingLogsRepository.deleteByUserIdAndUserBookIds, ensure callers verify the
list before invoking the repository: add a guard (e.g., if bookIds == null ||
bookIds.isEmpty()) return 0 (or no-op) in the service/handler that calls
BookshelfItemsRepository.deleteByUserIdAndBookIds so the repository is never
invoked with an empty collection.
| if (status != null) { | ||
| List<Long> bookIds = userBooksRepository.findBookIdsByUserIdAndStatus(userId, status); | ||
| if (!bookIds.isEmpty()) { | ||
| // ✅ 모든 서재에서 제거 | ||
| bookshelfItemsRepository.deleteByBookIds(bookIds); | ||
| // ✅ 4-1) status에 해당하는 userBookIds를 먼저 구해서 로그 삭제 | ||
| // (지금 repo에 userBookIds 조회가 없으니 간단히 하나 추가하는 걸 추천) | ||
| // 아래는 "추가할 repo 메서드" 참고 | ||
|
|
||
| List<Long> userBookIds = userBooksRepository.findUserBookIdsByUserIdAndStatus(userId, status); | ||
| if (!userBookIds.isEmpty()) { | ||
| readingLogsRepository.deleteByUserIdAndUserBookIds(userId, userBookIds); | ||
|
|
||
| List<Long> bookIds = userBooksRepository.findBookIdsByUserIdAndStatus(userId, status); | ||
| if (!bookIds.isEmpty()) { | ||
| bookshelfItemsRepository.deleteByUserIdAndBookIds(userId, bookIds); | ||
| } | ||
| } | ||
| return userBooksRepository.deleteByUserIdAndStatus(userId, status); |
There was a problem hiding this comment.
Stale comment: the repository method already exists.
Lines 216-217 suggest adding a repo method, but findUserBookIdsByUserIdAndStatus is already defined and used at line 219. Remove the misleading comment.
🧹 Suggested cleanup
// 4) [상태 기준 완전 삭제] status 있는 경우
if (status != null) {
- // ✅ 4-1) status에 해당하는 userBookIds를 먼저 구해서 로그 삭제
- // (지금 repo에 userBookIds 조회가 없으니 간단히 하나 추가하는 걸 추천)
- // 아래는 "추가할 repo 메서드" 참고
-
List<Long> userBookIds = userBooksRepository.findUserBookIdsByUserIdAndStatus(userId, status);📝 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.
| if (status != null) { | |
| List<Long> bookIds = userBooksRepository.findBookIdsByUserIdAndStatus(userId, status); | |
| if (!bookIds.isEmpty()) { | |
| // ✅ 모든 서재에서 제거 | |
| bookshelfItemsRepository.deleteByBookIds(bookIds); | |
| // ✅ 4-1) status에 해당하는 userBookIds를 먼저 구해서 로그 삭제 | |
| // (지금 repo에 userBookIds 조회가 없으니 간단히 하나 추가하는 걸 추천) | |
| // 아래는 "추가할 repo 메서드" 참고 | |
| List<Long> userBookIds = userBooksRepository.findUserBookIdsByUserIdAndStatus(userId, status); | |
| if (!userBookIds.isEmpty()) { | |
| readingLogsRepository.deleteByUserIdAndUserBookIds(userId, userBookIds); | |
| List<Long> bookIds = userBooksRepository.findBookIdsByUserIdAndStatus(userId, status); | |
| if (!bookIds.isEmpty()) { | |
| bookshelfItemsRepository.deleteByUserIdAndBookIds(userId, bookIds); | |
| } | |
| } | |
| return userBooksRepository.deleteByUserIdAndStatus(userId, status); | |
| if (status != null) { | |
| List<Long> userBookIds = userBooksRepository.findUserBookIdsByUserIdAndStatus(userId, status); | |
| if (!userBookIds.isEmpty()) { | |
| readingLogsRepository.deleteByUserIdAndUserBookIds(userId, userBookIds); | |
| List<Long> bookIds = userBooksRepository.findBookIdsByUserIdAndStatus(userId, status); | |
| if (!bookIds.isEmpty()) { | |
| bookshelfItemsRepository.deleteByUserIdAndBookIds(userId, bookIds); | |
| } | |
| } | |
| return userBooksRepository.deleteByUserIdAndStatus(userId, status); |
🤖 Prompt for AI Agents
In
`@booklog/src/main/java/com/example/booklog/domain/library/shelves/service/UserBooksService.java`
around lines 214 - 228, The inline comment in UserBooksService that suggests
adding a repository method is stale and misleading because
userBooksRepository.findUserBookIdsByUserIdAndStatus already exists; remove or
update that comment (the lines above the call to
findUserBookIdsByUserIdAndStatus) so the block that deletes
readingLogsRepository.deleteByUserIdAndUserBookIds,
bookshelfItemsRepository.deleteByUserIdAndBookIds, and the final
userBooksRepository.deleteByUserIdAndStatus call is accurate and clean, keeping
the surrounding logic intact.
1 participant
#124
Summary by CodeRabbit
New Features
Improvements
Bug Fixes