Conversation
WalkthroughMakes the read-status filter optional by changing Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as ArticleController
participant Usecase as GetArticleUsecase
participant Service as ArticleGetService
participant Repo as ArticleRepositoryCustom
Client->>Controller: GET /articles?categoryId&read-status?
Controller->>Usecase: getAllArticlesByCategory(user, categoryId, isRead?, page)
Usecase->>Service: findAllByCategory(user, category, isRead?, pageReq)
Service->>Repo: findAllByCategory(userId, categoryId, isRead?, pageable)
rect rgb(230,245,255)
note over Repo: Build base predicates (user + category)
alt isRead != null
Repo-->>Repo: Add predicate article.isRead == isRead
else isRead == null
Repo-->>Repo: No isRead filter (all articles)
end
Repo-->>Repo: Compute unreadCount (isRead = false) and totalCategoryArticleCount
end
Repo-->>Service: Articles + unreadCount + totalCategoryArticleCount
Service-->>Usecase: ArticlesWithUnreadCountDto
Usecase-->>Controller: ArticlesPageResponse
Controller-->>Client: 200 OK (paged result)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. 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: 0
🧹 Nitpick comments (2)
infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustom.java (1)
16-16: Rename parameter to categoryId to match semantics and impl.Interface uses
articleIdbut impl and usage are category-scoped. Rename for clarity.Apply:
- ArticlesWithUnreadCount findAllByCategory(UUID userId, long articleId, Boolean isRead, Pageable pageable); + ArticlesWithUnreadCount findAllByCategory(UUID userId, long categoryId, Boolean isRead, Pageable pageable);api/src/main/java/com/pinback/api/article/controller/ArticleController.java (1)
4-4: Remove unused import.
java.util.Optionalis unused.Apply:
-import java.util.Optional;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/src/main/java/com/pinback/api/article/controller/ArticleController.java(2 hunks)application/src/main/java/com/pinback/application/article/port/in/GetArticlePort.java(1 hunks)application/src/main/java/com/pinback/application/article/port/out/ArticleGetServicePort.java(1 hunks)application/src/main/java/com/pinback/application/article/usecase/query/GetArticleUsecase.java(1 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustom.java(1 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java(2 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/service/ArticleGetService.java(1 hunks)
🔇 Additional comments (9)
application/src/main/java/com/pinback/application/article/port/out/ArticleGetServicePort.java (1)
29-30: LGTM: Nullable read filter on category fetch.Signature widening to
Boolean isReadis consistent with the PR goal.Please confirm if Javadoc should clarify: null ⇒ no filter, true/false ⇒ filtered.
application/src/main/java/com/pinback/application/article/usecase/query/GetArticleUsecase.java (1)
70-75: LGTM: Propagates nullable read filter end-to-end.Pass-through to service is correct.
application/src/main/java/com/pinback/application/article/port/in/GetArticlePort.java (1)
19-20: LGTM: Public API supports optional read-status.Matches controller and service ports.
api/src/main/java/com/pinback/api/article/controller/ArticleController.java (1)
97-103: LGTM: Optional read-status for category endpoint.
required = falsewithBooleanaligns with repository behavior.infrastructure/src/main/java/com/pinback/infrastructure/article/service/ArticleGetService.java (1)
57-63: LGTM: Service port and repository wiring for nullable isRead.Pass-through parameters are correct.
infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java (4)
61-69: LGTM: Apply isRead predicate only when provided.Conditional predicate construction is correct.
72-74: LGTM: Add fetch joins for user and category.Fixes prior over-broad result set and prevents N+1 on to-one relations.
80-85: Verify count semantics when isRead is provided.
countQueryignoresisRead, so totalElements = all in category, not filtered subset. If UI expects total for the filtered set, use the sameconditions.Apply if filtered count is desired:
- JPAQuery<Long> countQuery = queryFactory - .select(article.count()) - .from(article) - .where(article.user.id.eq(userId).and(article.category.id.eq(categoryId))); + JPAQuery<Long> countQuery = queryFactory + .select(article.count()) + .from(article) + .where(conditions);If the product wants “category total regardless of isRead,” keep current code.
85-91: LGTM: Unread count scoped by user+category with isRead=false.Matches the requirement to report unread independently of the current filter.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java (1)
168-183: Guard isRead == null in findTodayRemindWithCount — infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java:168–183article.isRead.eq(isRead) will apply an "IS NULL" filter when isRead == null; mirror the null-guard used in other methods and add tests.
- BooleanExpression conditions = baseConditions.and(article.isRead.eq(isRead)); + BooleanExpression conditions = baseConditions; + if (isRead != null) { + conditions = conditions.and(article.isRead.eq(isRead)); + }Add unit tests covering isRead = null for the remind path to ensure parity with category/all-articles queries.
🧹 Nitpick comments (7)
application/src/main/java/com/pinback/application/article/dto/ArticlesWithUnreadCountDto.java (2)
7-11: Document the semantics and nullability of totalCategoryArticleCount.Clarify that it represents the total number of the user's articles in the category, ignoring any isRead filter, and may be null when not category-scoped.
Suggested doc:
package com.pinback.application.article.dto; import org.springframework.data.domain.Page; import com.pinback.domain.article.entity.Article; +/** + * unReadCount: count of unread articles for the user (scope may vary by query). + * article: paged articles for the current query. + * totalCategoryArticleCount: total number of the user's articles in the category, + * ignoring any read filter; may be null when not category-scoped. + */ public record ArticlesWithUnreadCountDto( Long unReadCount, Page<Article> article, Long totalCategoryArticleCount ) { }
9-9: Nit: Use plural name for a page of items.Prefer
articlesoverarticlefor clarity and consistency.- Page<Article> article, + Page<Article> articles,application/src/test/java/com/pinback/application/article/usecase/query/GetArticleUsecaseTest.java (2)
140-154: Assert totalCategoryArticleCount is surfaced (if API exposes it).You pass
5Lbut don’t verify it. IfArticlesPageResponseincludes this field, add an assertion; otherwise confirm it’s intentionally not part of the API response.Sample assertion (adjust to your response type):
assertThat(response.totalCategoryArticleCount()).isEqualTo(5L);Also applies to: 142-143
144-149: Add tests for the new tri-state isRead (null/true/false) on category query.Current test covers
trueonly. Add cases fornull(no filter) andfalse(unread) to prevent regressions.Example (additional tests):
@DisplayName("카테고리별 조회시 isRead=null이면 읽음 필터를 적용하지 않는다") @Test void getAllArticlesByCategory_IsReadNull() { Long categoryId = 1L; PageQuery pageQuery = new PageQuery(0, 10); PageRequest pageRequest = PageRequest.of(0, 10); List<Article> articles = List.of(article, article, article, article, article); Page<Article> articlePage = new PageImpl<>(articles, pageRequest, 5); ArticlesWithUnreadCountDto dto = new ArticlesWithUnreadCountDto(2L, articlePage, 5L); when(getCategoryPort.getCategoryAndUser(categoryId, user)).thenReturn(category); when(articleGetServicePort.findAllByCategory(user, category, null, pageRequest)).thenReturn(dto); ArticlesPageResponse response = getArticleUsecase.getAllArticlesByCategory(user, categoryId, null, pageQuery); assertThat(response.articles()).hasSize(5); verify(articleGetServicePort).findAllByCategory(user, category, null, pageRequest); } @DisplayName("카테고리별 조회시 isRead=false이면 안읽은 아티클만 조회한다") @Test void getAllArticlesByCategory_IsReadFalse() { Long categoryId = 1L; PageQuery pageQuery = new PageQuery(0, 10); PageRequest pageRequest = PageRequest.of(0, 10); List<Article> articles = List.of(article, article, article); Page<Article> articlePage = new PageImpl<>(articles, pageRequest, 3); ArticlesWithUnreadCountDto dto = new ArticlesWithUnreadCountDto(3L, articlePage, 5L); when(getCategoryPort.getCategoryAndUser(categoryId, user)).thenReturn(category); when(articleGetServicePort.findAllByCategory(user, category, false, pageRequest)).thenReturn(dto); ArticlesPageResponse response = getArticleUsecase.getAllArticlesByCategory(user, categoryId, false, pageQuery); assertThat(response.articles()).hasSize(3); verify(articleGetServicePort).findAllByCategory(user, category, false, pageRequest); }infrastructure/src/main/java/com/pinback/infrastructure/article/repository/dto/ArticlesWithUnreadCount.java (2)
7-11: Mirror the semantics doc here as well.Document that totalCategoryArticleCount is category-scoped, ignores isRead, and may be null.
Suggested doc:
package com.pinback.infrastructure.article.repository.dto; import org.springframework.data.domain.Page; import com.pinback.domain.article.entity.Article; +/** + * unReadCount: unread count for the user in the current scope. + * article: page of articles for the current query. + * totalCategoryArticleCount: total (read+unread) for the user within the category; may be null. + */ public record ArticlesWithUnreadCount( Long unReadCount, Page<Article> article, Long totalCategoryArticleCount) { }
9-9: Nit: PreferarticlesoverarticleforPage<Article>.Aligns with plural semantics and the application DTO.
- Page<Article> article, + Page<Article> articles,infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java (1)
95-104: Consider lazy count for consistency and performance.Other methods use
PageableExecutionUtils. Keep it here too to avoid unnecessary count queries on the last page.- return new ArticlesWithUnreadCount(unReadCount, - new PageImpl<>(articles, pageable, countQuery.fetchOne()), - totalCategoryArticleCount); + return new ArticlesWithUnreadCount( + unReadCount, + PageableExecutionUtils.getPage(articles, pageable, countQuery::fetchOne), + totalCategoryArticleCount + );Also applies to: 102-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
application/src/main/java/com/pinback/application/article/dto/ArticlesWithUnreadCountDto.java(1 hunks)application/src/main/java/com/pinback/application/article/usecase/query/GetArticleUsecase.java(2 hunks)application/src/test/java/com/pinback/application/article/usecase/query/GetArticleUsecaseTest.java(3 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java(4 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/repository/dto/ArticlesWithUnreadCount.java(1 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/service/ArticleGetService.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- infrastructure/src/main/java/com/pinback/infrastructure/article/service/ArticleGetService.java
- application/src/main/java/com/pinback/application/article/usecase/query/GetArticleUsecase.java
🔇 Additional comments (4)
application/src/test/java/com/pinback/application/article/usecase/query/GetArticleUsecaseTest.java (2)
117-123: Constructor update looks good; add a null-handling assertion.Since totalCategoryArticleCount can be null outside category-scope, add an assertion that the mapping path doesn’t NPE and behaves as expected (e.g., absent/ignored in response).
Also applies to: 119-120
163-167: Constructor update for unread-articles path looks good.Null third argument is appropriate outside category scope. Consider adding an assertion guarding null handling in the mapping, similar to the all-articles test.
Also applies to: 165-166
infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java (2)
63-76: Good: category+user scoping and optional isRead filter fixed.Adding user/category predicates and the nullable isRead condition addresses the over-fetching bug and the tri-state requirement.
Also applies to: 65-71, 74-75
87-99: Counts semantics LGTM.Unread count ignores the
isReadfilter (by design) andtotalCategoryArticleCountignores it as well; matches the PR objectives.
1 participant
🚀 PR 요약
아티클 조회시 조회조건을 수정했습니다.
✨ PR 상세 내용
🚨 주의 사항
주의할 부분이 무엇인가요? - 지우고 작성
✅ 체크 리스트
Summary by CodeRabbit