Conversation
WalkthroughIntroduces category color support across domain, DTOs, services, and repositories; adds “today remind” retrieval with read/unread filtering and count DTOs plus a new API endpoint; adjusts article update flow to accept “now”; modifies auth token endpoint; enables JPA auditing; updates CI, Gradle test config, settings, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant AC as ArticleController
participant GU as GetArticleUsecase
participant GS as ArticleGetService
participant AR as ArticleRepository
Note over C,AC: GET /articles/remind?now&read-status&page&size
C->>AC: Request
AC->>GU: getRemindArticles(user, now, readStatus, pageQuery)
GU->>GS: findTodayRemindWithCount(user, now, pageable, readStatus)
GS->>AR: findTodayRemindWithCount(userId, pageable, startAt, endAt, isRead)
AR-->>GS: RemindArticlesWithCount(readCount, unreadCount, Page<Article>)
GS-->>GU: RemindArticlesWithCountDto(...)
GU-->>AC: TodayRemindResponse(readCount, unreadCount, articles)
AC-->>C: 200 OK (ResponseDto)
sequenceDiagram
participant C as Client
participant CC as CreateCategoryUsecase
participant CR as CategoryRepository (Custom)
participant CS as CategorySaveService
C->>CC: createCategory(user, name)
CC->>CR: findColorsByUser(user)
CR-->>CC: Set<CategoryColor>
CC->>CC: pick first unused CategoryColor
CC->>CS: save(Category.create(name, user, color))
CS-->>CC: Category(id, name, color)
CC-->>C: CreateCategoryResponse(id, name, color)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
domain/src/test/java/com/pinback/domain/article/ArticleDomainTest.java (1)
44-57: Fix broken test: assert exception for memo > 300The display name says an exception should be thrown, but the test creates the entity and asserts read-status. Assert the exception instead.
@DisplayName("Article 생성시 메모의 길이가 300개 초과하면 예외가 발생한다.") @Test void testMaxLength() { //given User user = TestFixture.user(); Category category = category(user); String longMemo = "a".repeat(301); - //when & then - Article article = Article.create("test", longMemo, user, category, - LocalDateTime.of(2025, 7, 7, 12, 0, 0)); - assertThat(article.isRead()).isFalse(); + // when & then + assertThatThrownBy(() -> Article.create( + "test", longMemo, user, category, LocalDateTime.of(2025, 7, 7, 12, 0, 0) + )).isInstanceOf(IllegalArgumentException.class); }
🧹 Nitpick comments (51)
api/src/main/java/com/pinback/api/auth/controller/AuthController.java (3)
40-40: Rename method to reflect behavior.Method name
signInno longer matches the operation (“토큰 재발급”). UseissueToken(as in the diff) orreissueTokenfor clarity.
3-3: If switching to POST, drop unused GetMapping import.After adopting POST in the token endpoint, remove this import to avoid an unused import warning.
- import org.springframework.web.bind.annotation.GetMapping;
7-7: Prefer request body for PII to reduce logging of query strings.If you must keep simple email input, a small DTO via @RequestBody avoids email in URLs/access logs and documents the schema in OpenAPI better.
Example DTO:
public record TokenIssueRequest(@NotBlank @Email String email) {}Controller signature:
public ResponseDto<TokenResponse> issueToken(@Valid @RequestBody TokenIssueRequest req) { ... }.github/workflows/dev.yml (2)
10-13: Matrix looks fine; consider adding Java 17 for broader LTS coverage.If you expect consumers on 17, a 17+21 matrix prevents accidental regressions. If not, this is fine as-is.
46-46: Avoid hard-coding --max-workers=4 on 2-vCPU GitHub runners.This can oversubscribe CPUs (you also fork tests in Gradle). Let Gradle auto-tune or parameterize per runner.
Apply:
- run: ./gradlew clean build --no-daemon --build-cache --parallel --max-workers=4 + run: ./gradlew clean build --no-daemon --build-cache --parallelinfrastructure/src/test/java/com/pinback/infrastructure/fixture/CustomRepository.java (1)
16-17: Flush before clear to avoid stale persistence state during cleanup.If there are pending writes in the same transaction, flushing first prevents surprises before native deletes.
Apply:
- entityManager.clear(); + entityManager.flush(); + entityManager.clear();If IDs for users/push_subscription are relied on in tests, consider resetting their identity/sequence as well.
build.gradle (1)
58-83: Don’t overwrite Test.jvmArgs; append instead.Using “=” can drop previously configured args (from plugins or project-specific settings).
Apply:
- // 테스트 성능 최적화 - jvmArgs = ['-XX:+UseG1GC', '-Xmx1g'] + // 테스트 성능 최적화 + jvmArgs += ['-XX:+UseG1GC', '-Xmx1g']Optional: ensure memory fits forks on CI (e.g., smaller Xmx or lower maxParallelForks) to avoid OOM.
domain/src/test/java/com/pinback/domain/fixture/TestFixture.java (2)
23-24: Fixture update looks good; consider a helper that accepts color.Adding
categoryWithColor(User, CategoryColor)can help target color-assignment edge cases in tests.
27-28: Same as above for name-specific factory.Providing a
categoryWithNameAndColor(User, String, CategoryColor)keeps tests flexible as enum grows.domain/src/test/java/com/pinback/domain/article/ArticleDomainTest.java (2)
3-3: Unify TestFixture usage (avoid mixed static/class calls)Use the existing static import and drop the class import for consistency.
-import com.pinback.domain.fixture.TestFixture; +// (remove; static imports already cover usages)- User user = TestFixture.user(); - Article article = TestFixture.article(user); + User user = user(); + Article article = article(user);Also applies to: 13-13, 22-24
40-41: Prefer a valid URL in test dataMinor: use an actual URL to avoid future validation surprises.
- Article article = Article.create("test", "testmemo", user, category, LocalDateTime.of(2025, 7, 7, 12, 0, 0)); + Article article = Article.create("https://test.com", "testmemo", user, category, LocalDateTime.of(2025, 7, 7, 12, 0, 0));infrastructure/src/main/java/com/pinback/infrastructure/category/repository/CategoryRepository.java (1)
4-4: Remove unused imports
SetandCategoryColoraren’t used in this interface. Clean them up.-import java.util.Set; @@ -import com.pinback.domain.category.enums.CategoryColor;Also applies to: 9-9
infrastructure/src/main/java/com/pinback/infrastructure/category/repository/CategoryRepositoryCustom.java (2)
16-18: Align user parameter style across custom repository APIsExisting custom methods take
UUID userId, while the new one takesUser. Consider standardizing (either allUUIDor allUser) to reduce cognitive load and adapter code.If you want, I can draft the minimal cascading changes to standardize on one style.
16-18: Replace HashSet with EnumSet in findColorsByUser
In CategoryRepositoryCustomImpl (around line 66), swapreturn new HashSet<>(colors);for an EnumSet-based build to leverage compact storage and speed while handling empty results safely:
if (colors.isEmpty()) { return EnumSet.noneOf(CategoryColor.class); } return EnumSet.copyOf(colors);domain/src/main/java/com/pinback/domain/category/enums/CategoryColor.java (1)
3-14: Add Javadoc to CategoryColor enum documenting persistence and UI mapping.Include a brief comment above the enum noting that JPA persists the values as STRING (to guard against ordering changes) and that the UI maps COLOR1…COLOR10 to the corresponding theme tokens.
application/src/test/java/com/pinback/application/article/usecase/command/UpdateArticleUsecaseTest.java (1)
68-69: DRY the repeated ‘now’ timestampDefine a class-level constant to cut duplication.
class UpdateArticleUsecaseTest extends ApplicationTestBase { + private static final LocalDateTime NOW = LocalDateTime.of(2025, 9, 1, 13, 0); @@ - LocalDateTime now = LocalDateTime.of(2025, 9, 1, 13, 0); + LocalDateTime now = NOW;Also applies to: 97-98, 118-119, 143-144, 171-172, 199-200, 226-227
application/src/main/java/com/pinback/application/category/usecase/command/UpdateCategoryUsecase.java (1)
26-29: Remove redundant try/catch
Replacetry { category.updateName(command.categoryName()); } catch (CategoryNameLengthOverException e) { throw new CategoryNameLengthOverException(); }with
category.updateName(command.categoryName());to preserve the original exception stack.
Duplicate‐name handling currently lives only in
CreateCategoryUsecase(viacheckExistsByCategoryNameAndUser) and there’s no DB‐level unique constraint on the category name. The update use case bypasses this check—consider adding application-level validation or a database constraint on name to prevent duplicates when renaming.infrastructure/src/main/java/com/pinback/infrastructure/category/repository/CategoryRepositoryCustomImpl.java (1)
59-67: Prefer DB-side distinct and EnumSet; avoid entity-instance equality in predicate.
- Use
.distinct()so the DB de-duplicates.- Compare by
user.idto avoid relying on entity instance identity.- Return
EnumSetfor better perf with enums (handle empty safely).Consider an index on
category(user_id, color)if this query is hot.- public Set<CategoryColor> findColorsByUser(User user) { - List<CategoryColor> colors = queryFactory - .select(category.color) - .from(category) - .where(category.user.eq(user)) - .fetch(); - - return new HashSet<>(colors); - } + public Set<CategoryColor> findColorsByUser(User user) { + List<CategoryColor> colors = queryFactory + .select(category.color).distinct() + .from(category) + .where(category.user.id.eq(user.getId())) + .fetch(); + + return colors.isEmpty() + ? java.util.EnumSet.noneOf(CategoryColor.class) + : java.util.EnumSet.copyOf(colors); + }Add import outside this hunk:
import java.util.EnumSet;application/src/test/java/com/pinback/application/TestFixture.java (1)
22-28: Fixture hardcodes COLOR1; add overloads to vary color to avoid conflicts in tests.Helps when creating multiple categories for the same user and when validating color-assignment logic.
public static Category category(User user) { - return Category.create("테스트카테고리", user, CategoryColor.COLOR1); + return Category.create("테스트카테고리", user, CategoryColor.COLOR1); } public static Category categoryWithName(User user, String name) { - return Category.create(name, user, CategoryColor.COLOR1); + return Category.create(name, user, CategoryColor.COLOR1); } + +// Overloads to explicitly control color in tests +public static Category category(User user, CategoryColor color) { + return Category.create("테스트카테고리", user, color); +} + +public static Category categoryWithName(User user, String name, CategoryColor color) { + return Category.create(name, user, color); +}application/src/main/java/com/pinback/application/category/dto/response/CategoryResponse.java (1)
7-12: Use enum name() instead of toString() in CategoryResponse.from
Category.color is already persisted with@Enumerated(EnumType.STRING).- return new CategoryResponse(category.getId(), category.getName(), category.getColor().toString()); + return new CategoryResponse(category.getId(), category.getName(), category.getColor().name());application/src/main/java/com/pinback/application/article/dto/command/ArticleUpdateCommand.java (1)
8-9: If you must keep it, don’t use LocalDateTime for “now”.Use
InstantorOffsetDateTimeto avoid timezone/DST ambiguity.application/src/main/java/com/pinback/application/article/dto/RemindArticlesWithCountDto.java (1)
at the application DTO boundary.
7-11: Don’t expose PageThis couples to Spring Data and leaks domain entities. Consider mapping to a lightweight response DTO and pagination metadata.
-public record RemindArticlesWithCountDto( - long readCount, - long unreadCount, - Page<Article> articles -) { -} +public record RemindArticlesWithCountDto( + long readCount, + long unreadCount, + List<RemindArticleDto> items, + int pageNumber, + int pageSize, + long totalElements, + int totalPages +) {}application/src/main/java/com/pinback/application/category/port/out/CategoryColorServicePort.java (1)
10-11: Prefer EnumSet for enum collections.Return
EnumSet<CategoryColor>(or at least ensure implementations return anEnumSet) for better performance/memory.-Set<CategoryColor> getUsedColorsByUser(User user); +EnumSet<CategoryColor> getUsedColorsByUser(User user);Clarify semantics: do “used colors” include soft-deleted/archived categories?
application/src/main/java/com/pinback/application/article/usecase/command/UpdateArticleUsecase.java (2)
50-54: Null-safety and off-by-one on memo length.Current check throws when length == 500 and NPEs on null. If the spec is “max 500 chars allowed”, use
>and guard nulls.- if (TextUtil.countGraphemeClusters(memo) >= MEMO_LIMIT_LENGTH) { + if (memo != null && TextUtil.countGraphemeClusters(memo) > MEMO_LIMIT_LENGTH) { throw new MemoLengthLimitException(); }If
memomust be non-null, validate earlier and fail fast with a clear exception.
61-61: LocalDateTime comparison may be timezone-ambiguous.If users operate across timezones/DST, comparing
LocalDateTimecan misbehave. Consider persisting/comparing inInstant(viaZonedDateTime/OffsetDateTime).application/src/test/java/com/pinback/application/category/usecase/command/UpdateCategoryUsecaseTest.java (1)
59-76: Add a failing-case test for name validation (moved to domain).Given name-length validation moved into the domain, add a test that updating to an invalid name propagates the domain exception.
application/src/main/java/com/pinback/application/article/port/in/GetArticlePort.java (2)
8-10: Remove unused import.RemindArticlesResponse appears unused after the signature change.
-import com.pinback.application.article.dto.response.RemindArticlesResponse;
23-23: Consider tri-state read filtering to support “all/read/unread”.Primitive boolean can’t represent “all”. Align with infra Boolean (nullable) or an enum (ALL, READ, UNREAD) before this API solidifies.
- TodayRemindResponse getRemindArticles(User user, LocalDateTime now, boolean readStatus, PageQuery query); + TodayRemindResponse getRemindArticles(User user, LocalDateTime now, Boolean isRead, PageQuery query);Also document that null means “no filter”.
infrastructure/src/test/java/com/pinback/infrastructure/fixture/TestFixture.java (1)
22-24: Fixture updates align with colorized Category.Defaulting to COLOR1 is fine for tests; consider an overload to choose color when needed.
application/src/main/java/com/pinback/application/article/usecase/query/GetArticleUsecase.java (2)
100-105: Allow optional read filter: preferBooleanoverbooleanIf “all” (no filter) should be supported, accept
Boolean readStatusand pass-through null to the port; primitivebooleancan’t represent “no preference”.- public TodayRemindResponse getRemindArticles(User user, LocalDateTime now, boolean readStatus, PageQuery query) { + public TodayRemindResponse getRemindArticles(User user, LocalDateTime now, Boolean readStatus, PageQuery query) {If the product explicitly requires a forced filter, ignore this.
117-125: Timezone/DST sensitivity for “today remind” computation
LocalDateTime+ user’s default time ignores timezone/DST edges. Consider deriving a zoned instant using the user’s timezone (e.g., store zone with the user, buildZonedDateTimeat that zone, then convert for persistence) and add tests around DST transitions.api/src/main/java/com/pinback/api/article/controller/ArticleController.java (1)
136-148: Makenowparsing robust and optional; default to server time.Explicitly mark the date format and allow omission to reduce client coupling and parsing errors.
Apply within this method:
- public ResponseDto<TodayRemindResponse> getRemindArticles( + public ResponseDto<TodayRemindResponse> getRemindArticles( @Parameter(hidden = true) @CurrentUser User user, - @Parameter(description = "현재 시간", example = "2025-09-03T10:00:00") @RequestParam LocalDateTime now, + @Parameter(description = "현재 시간", example = "2025-09-03T10:00:00") + @DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME) + @RequestParam(required = false) LocalDateTime now, @Parameter(description = "읽음 상태 (true: 읽음, false: 안읽음)", example = "true") @RequestParam(name = "read-status") boolean readStatus, @Parameter(description = "페이지 번호 (0부터 시작)") @RequestParam(defaultValue = "0") int page, @Parameter(description = "페이지 크기") @RequestParam(defaultValue = "8") int size ) { PageQuery query = new PageQuery(page, size); - TodayRemindResponse response = getArticlePort.getRemindArticles(user, now, readStatus, query); + LocalDateTime effectiveNow = (now != null) ? now : LocalDateTime.now(); + TodayRemindResponse response = getArticlePort.getRemindArticles(user, effectiveNow, readStatus, query); return ResponseDto.ok(response); }Add import:
import org.springframework.format.annotation.DateTimeFormat;application/src/main/java/com/pinback/application/article/dto/response/TodayRemindResponse.java (1)
10-12: Defensive copy of articles to keep the DTO immutable.Wrap with List.copyOf to avoid accidental external mutation.
- return new TodayRemindResponse(readArticleCount, unreadArticleCount, articles); + return new TodayRemindResponse(readArticleCount, unreadArticleCount, List.copyOf(articles));infrastructure/src/main/java/com/pinback/infrastructure/category/service/CategoryColorService.java (1)
20-23: Make read path transactional, null-safe, and return an unmodifiable Set.Avoid NPEs, hint read-only to JPA, and prevent callers from mutating the returned set.
@Override - public Set<CategoryColor> getUsedColorsByUser(User user) { - return categoryRepository.findColorsByUser(user); - } + @org.springframework.transaction.annotation.Transactional(readOnly = true) + public Set<CategoryColor> getUsedColorsByUser(User user) { + java.util.Objects.requireNonNull(user, "user must not be null"); + return java.util.Set.copyOf(categoryRepository.findColorsByUser(user)); + }Optionally add imports:
import java.util.Objects; import org.springframework.transaction.annotation.Transactional;application/src/main/java/com/pinback/application/article/dto/response/RemindArticleResponse.java (1)
17-27: Null-safe category mapping
Article.category is optional by JPA default (nonullable=falseon the@JoinColumn), soarticle.getCategory()can be null and lead to an NPE. Guard the mapping:article.getCategory() != null ? CategoryResponse.from(article.getCategory()) : nullinfrastructure/src/test/java/com/pinback/infrastructure/category/service/CategoryColorServiceTest.java (2)
65-80: Display name implies constraint enforcement rather than result set uniquenessThis test verifies that the returned Set has unique elements, not that the system enforces uniqueness. Consider renaming the display name to avoid confusion.
- @DisplayName("한 사용자는 각 색상을 한 번만 사용할 수 있다") + @DisplayName("한 사용자의 사용 색상 집합은 중복 없이 조회된다")
108-125: Minor loop simplification for readabilityUse enhanced for-loop over CategoryColor.values() to reduce index math.
- for (int i = 0; i < allColors.length; i++) { - categoryRepository.save(Category.create("카테고리" + (i + 1), user, allColors[i])); - } + int i = 1; + for (CategoryColor color : allColors) { + categoryRepository.save(Category.create("카테고리" + i++, user, color)); + }infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustom.java (1)
18-21: Clarify tri-state isRead semantics and annotate as nullableMake it explicit that null means “no filter,” and annotate for nullability. Also add the import.
import com.pinback.domain.article.entity.Article; import com.pinback.infrastructure.article.repository.dto.ArticlesWithUnreadCount; import com.pinback.infrastructure.article.repository.dto.RemindArticlesWithCount; +import org.springframework.lang.Nullable; @@ - Page<Article> findTodayRemind(UUID userId, Pageable pageable, LocalDateTime startAt, LocalDateTime endAt, Boolean isRead); + /** + * Find reminder articles within [startAt, endAt]. When isRead is null, do not filter by read status. + */ + Page<Article> findTodayRemind(UUID userId, Pageable pageable, LocalDateTime startAt, LocalDateTime endAt, @Nullable Boolean isRead); - RemindArticlesWithCount findTodayRemindWithCount(UUID userId, Pageable pageable, LocalDateTime startAt, LocalDateTime endAt, Boolean isRead); + /** + * Same as findTodayRemind but returns page + count metadata. When isRead is null, do not filter by read status. + */ + RemindArticlesWithCount findTodayRemindWithCount(UUID userId, Pageable pageable, LocalDateTime startAt, LocalDateTime endAt, @Nullable Boolean isRead);application/src/main/java/com/pinback/application/article/port/out/ArticleGetServicePort.java (1)
33-36: Document and annotate nullable isRead; align semantics with repositoryMake null = “no filter” explicit and annotate. Consider consistent nullability across layers.
import com.pinback.application.article.dto.RemindArticlesWithCountDto; +import org.springframework.lang.Nullable; @@ - Page<Article> findTodayRemind(User user, LocalDateTime remindDateTime, Pageable pageable, Boolean isRead); + /** + * Retrieves reminder articles for the day of remindDateTime. When isRead is null, do not filter by read status. + */ + Page<Article> findTodayRemind(User user, LocalDateTime remindDateTime, Pageable pageable, @Nullable Boolean isRead); - RemindArticlesWithCountDto findTodayRemindWithCount(User user, LocalDateTime remindDateTime, Pageable pageable, Boolean isRead); + /** + * Same as findTodayRemind but returns page + count metadata. When isRead is null, do not filter by read status. + */ + RemindArticlesWithCountDto findTodayRemindWithCount(User user, LocalDateTime remindDateTime, Pageable pageable, @Nullable Boolean isRead);api/src/main/java/com/pinback/api/article/dto/request/ArticleUpdateRequest.java (2)
24-24: Korean message spacing nitAdd a space: “현재 시간은”.
- @NotNull(message = "현재시간은 필수입니다") + @NotNull(message = "현재 시간은 필수입니다")
27-29: Prefer time with offset or validate future/presentLocalDateTime is timezone-agnostic; for APIs, OffsetDateTime/Instant is safer. At minimum, ensure remindTime is not in the past if provided.
- @Schema(description = "리마인드 시간", example = "2025-12-31T23:59:00") - LocalDateTime remindTime + @Schema(description = "리마인드 시간", example = "2025-12-31T23:59:00Z") + @jakarta.validation.constraints.FutureOrPresent(message = "리마인드 시간은 과거일 수 없습니다") + java.time.OffsetDateTime remindTimeinfrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java (1)
99-106: Prefer sorting by remindAt and prefetch category to avoid N+1 in projections that include categoryFor “today remind” UX, ordering by remindAt (then createdAt) is typically expected. Also, category is often rendered with the article; add a fetch join to avoid N+1.
- .join(article.user, user).fetchJoin() + .join(article.user, user).fetchJoin() + .leftJoin(article.category, category).fetchJoin() .where(conditions) .offset(pageable.getOffset()) .limit(pageable.getPageSize()) - .orderBy(article.createdAt.desc()) + .orderBy(article.remindAt.asc(), article.createdAt.desc())Apply to both queries in findTodayRemind and findTodayRemindWithCount.
Also applies to: 152-159
application/src/test/java/com/pinback/application/category/usecase/command/CreateCategoryUsecaseTest.java (2)
46-53: Fixture choice may confuse expectationssavedCategory’s name/color differ from the command and computed color. Either build a fresh Category in test or avoid relying on savedCategory when stubbing save().
115-134: Nice coverage for color auto-assignment; consider also asserting response colorYou already verify the argument color is COLOR2. If CreateCategoryUsecase returns the persisted entity’s color, assert it in the response too to catch mapping regressions.
-// when -createCategoryUsecase.createCategory(user, command); +// when +CreateCategoryResponse resp = createCategoryUsecase.createCategory(user, command); @@ verify(categorySaveService).save(argThat(category -> category.getColor().equals(CategoryColor.COLOR2) )); +assertThat(resp.categoryColor()).isEqualTo("COLOR2");infrastructure/src/test/java/com/pinback/infrastructure/article/service/ArticleGetServiceTest.java (3)
155-155: Confirm null read-filter semantics.
Passing null for isRead should mean “no read filter.” If the service doesn’t explicitly handle null, this test may be brittle. Consider overloading (without isRead) or using Optional to make intent explicit.
244-273: Solid coverage for “read” branch with total counts.
Nice separation of read/unread and exclusion of yesterday. Add one pagination-invariance assertion to ensure counts reflect all of “today,” not just the current page.Example (additional test):
+ @Test + void findTodayRemindWithCount_ReadStatus_PagingInvariant_Test() { + User user = userRepository.save(user()); + Category category = categoryRepository.save(category(user)); + LocalDateTime today = LocalDateTime.of(2025, 8, 18, 12, 0, 0); + articleRepository.save(readArticleWithDate(user, "r1", category, today)); + articleRepository.save(readArticleWithDate(user, "r2", category, today)); + articleRepository.save(articleWithDate(user, "u1", category, today)); + articleRepository.save(articleWithDate(user, "u2", category, today)); + articleRepository.save(articleWithDate(user, "u3", category, today)); + PageRequest smallPage = PageRequest.of(0, 1); + RemindArticlesWithCountDto result = + articleGetService.findTodayRemindWithCount(user, today, smallPage, true); + assertThat(result.readCount()).isEqualTo(2L); + assertThat(result.unreadCount()).isEqualTo(3L); + assertThat(result.articles().getContent()).hasSize(1); + }
302-317: Good empty-state coverage.
Optionally duplicate with the unread flag to cover both booleans.domain/src/main/java/com/pinback/domain/category/entity/Category.java (1)
29-34: Unique (user, color) constraint is appropriate; plan migration.
Before enabling in production, ensure a data migration checks and resolves duplicates, then adds the unique index/constraint to avoid deploy-time failures.application/src/main/java/com/pinback/application/category/usecase/command/CreateCategoryUsecase.java (1)
56-63: Tighten fallback when all colors are used.
Defaulting to COLOR1 can collide under races or if limits drift. Prefer throwing to surface logic drift; optionally handle retries on unique constraint.Apply:
- return Arrays.stream(CategoryColor.values()) - .filter(color -> !usedColors.contains(color)) - .findFirst() - .orElse(CategoryColor.COLOR1); + return Arrays.stream(CategoryColor.values()) + .filter(color -> !usedColors.contains(color)) + .findFirst() + .orElseThrow(() -> new IllegalStateException("No available category color"));Operational note: consider a single retry on DataIntegrityViolationException (unique (user, color)) by re-fetching used colors and picking the next available to mitigate race conditions under concurrent category creation.
application/src/test/java/com/pinback/application/article/usecase/query/GetArticleUsecaseTest.java (2)
202-228: Unread-case mirrors read-case; consider parameterizing to remove duplication.
Both tests differ only by isRead flag and expected sizes. A parameterized test would be DRY and easier to extend.Example (for reference):
@ParameterizedTest(name = "오늘 리마인드 {0} 아티클을 조회할 수 있다") @CsvSource({ "true, 2, 2, 3", "false, 3, 2, 3" }) void getRemindArticles_ByReadStatus_Success(boolean isRead, int pageSize, long readCount, long unreadCount) { User userWithRemindTime = User.create("test@example.com", LocalTime.of(14, 0)); ReflectionTestUtils.setField(userWithRemindTime, "id", java.util.UUID.randomUUID()); LocalDateTime now = LocalDateTime.of(2025, 8, 20, 15, 0); LocalDateTime todayRemindTime = LocalDateTime.of(now.toLocalDate(), userWithRemindTime.getRemindTime()); PageQuery pageQuery = new PageQuery(0, 10); PageRequest pageRequest = PageRequest.of(0, 10); List<Article> articles = java.util.stream.IntStream.range(0, pageSize).mapToObj(i -> article).toList(); Page<Article> articlePage = new PageImpl<>(articles, pageRequest, pageSize); RemindArticlesWithCountDto dto = new RemindArticlesWithCountDto(readCount, unreadCount, articlePage); when(articleGetServicePort.findTodayRemindWithCount(userWithRemindTime, todayRemindTime, pageRequest, isRead)).thenReturn(dto); TodayRemindResponse response = getArticleUsecase.getRemindArticles(userWithRemindTime, now, isRead, pageQuery); assertThat(response.articles()).hasSize(pageSize); assertThat(response.readArticleCount()).isEqualTo(readCount); assertThat(response.unreadArticleCount()).isEqualTo(unreadCount); verify(articleGetServicePort).findTodayRemindWithCount(userWithRemindTime, todayRemindTime, pageRequest, isRead); }Also apply the same todayRemindTime derivation tweak here:
- LocalDateTime todayRemindTime = LocalDateTime.of(2025, 8, 20, 14, 0); + LocalDateTime todayRemindTime = LocalDateTime.of(now.toLocalDate(), userWithRemindTime.getRemindTime());
174-201: Derive todayRemindTime dynamically, add before-remind boundary test, and assert category fields
- Replace the hard-coded remind time with a derivation from the inputs:
- LocalDateTime todayRemindTime = LocalDateTime.of(2025, 8, 20, 14, 0); + LocalDateTime todayRemindTime = LocalDateTime.of(now.toLocalDate(), userWithRemindTime.getRemindTime());- Add a test for when
now.isBefore(user.getRemindTime())to verify whether the logic rolls back to the previous day’s remindTime or keeps today’s.- Since
RemindArticleResponseincludes aCategoryResponse categoryfield, assert its contents in the test, e.g.:RemindArticleResponse first = response.articles().get(0); assertThat(first.category().id()).isEqualTo(expectedCategoryId); assertThat(first.category().name()).isEqualTo(expectedCategoryName); assertThat(first.category().color()).isEqualTo(expectedCategoryColor);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (48)
.github/workflows/dev.yml(2 hunks)api/src/main/java/com/pinback/api/PinbackApiApplication.java(2 hunks)api/src/main/java/com/pinback/api/article/controller/ArticleController.java(3 hunks)api/src/main/java/com/pinback/api/article/dto/request/ArticleUpdateRequest.java(1 hunks)api/src/main/java/com/pinback/api/auth/controller/AuthController.java(2 hunks)application/src/main/java/com/pinback/application/article/dto/RemindArticlesWithCountDto.java(1 hunks)application/src/main/java/com/pinback/application/article/dto/command/ArticleUpdateCommand.java(1 hunks)application/src/main/java/com/pinback/application/article/dto/response/ArticleResponse.java(1 hunks)application/src/main/java/com/pinback/application/article/dto/response/RemindArticleResponse.java(1 hunks)application/src/main/java/com/pinback/application/article/dto/response/RemindArticlesResponse.java(1 hunks)application/src/main/java/com/pinback/application/article/dto/response/TodayRemindResponse.java(1 hunks)application/src/main/java/com/pinback/application/article/port/in/GetArticlePort.java(2 hunks)application/src/main/java/com/pinback/application/article/port/out/ArticleGetServicePort.java(2 hunks)application/src/main/java/com/pinback/application/article/usecase/command/UpdateArticleUsecase.java(2 hunks)application/src/main/java/com/pinback/application/article/usecase/query/GetArticleUsecase.java(2 hunks)application/src/main/java/com/pinback/application/category/dto/response/CategoryResponse.java(1 hunks)application/src/main/java/com/pinback/application/category/dto/response/CreateCategoryResponse.java(1 hunks)application/src/main/java/com/pinback/application/category/port/out/CategoryColorServicePort.java(1 hunks)application/src/main/java/com/pinback/application/category/usecase/command/CreateCategoryUsecase.java(2 hunks)application/src/main/java/com/pinback/application/category/usecase/command/UpdateCategoryUsecase.java(2 hunks)application/src/test/java/com/pinback/application/TestFixture.java(2 hunks)application/src/test/java/com/pinback/application/article/usecase/command/UpdateArticleUsecaseTest.java(8 hunks)application/src/test/java/com/pinback/application/article/usecase/query/GetArticleUsecaseTest.java(3 hunks)application/src/test/java/com/pinback/application/category/usecase/command/CreateCategoryUsecaseTest.java(1 hunks)application/src/test/java/com/pinback/application/category/usecase/command/DeleteCategoryUsecaseTest.java(1 hunks)application/src/test/java/com/pinback/application/category/usecase/command/UpdateCategoryUsecaseTest.java(1 hunks)application/src/test/java/com/pinback/application/category/usecase/query/GetCategoryUsecaseTest.java(1 hunks)build.gradle(1 hunks)domain/src/main/java/com/pinback/domain/category/entity/Category.java(3 hunks)domain/src/main/java/com/pinback/domain/category/enums/CategoryColor.java(1 hunks)domain/src/main/java/com/pinback/domain/category/exception/CategoryNameLengthOverException.java(1 hunks)domain/src/test/java/com/pinback/domain/article/ArticleDomainTest.java(2 hunks)domain/src/test/java/com/pinback/domain/fixture/TestFixture.java(2 hunks)domain/src/test/java/com/pinback/fixture/TestFixture.java(0 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(3 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/repository/dto/RemindArticlesWithCount.java(1 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/service/ArticleGetService.java(2 hunks)infrastructure/src/main/java/com/pinback/infrastructure/category/repository/CategoryRepository.java(1 hunks)infrastructure/src/main/java/com/pinback/infrastructure/category/repository/CategoryRepositoryCustom.java(1 hunks)infrastructure/src/main/java/com/pinback/infrastructure/category/repository/CategoryRepositoryCustomImpl.java(2 hunks)infrastructure/src/main/java/com/pinback/infrastructure/category/service/CategoryColorService.java(1 hunks)infrastructure/src/test/java/com/pinback/infrastructure/article/service/ArticleGetServiceTest.java(5 hunks)infrastructure/src/test/java/com/pinback/infrastructure/category/service/CategoryColorServiceTest.java(1 hunks)infrastructure/src/test/java/com/pinback/infrastructure/category/service/CategorySaveServiceTest.java(1 hunks)infrastructure/src/test/java/com/pinback/infrastructure/fixture/CustomRepository.java(1 hunks)infrastructure/src/test/java/com/pinback/infrastructure/fixture/TestFixture.java(3 hunks)settings.gradle(0 hunks)
💤 Files with no reviewable changes (2)
- settings.gradle
- domain/src/test/java/com/pinback/fixture/TestFixture.java
🧰 Additional context used
🧬 Code graph analysis (7)
application/src/test/java/com/pinback/application/category/usecase/command/DeleteCategoryUsecaseTest.java (1)
application/src/main/java/com/pinback/application/common/exception/CategoryNotOwnedException.java (1)
CategoryNotOwnedException(6-10)
infrastructure/src/test/java/com/pinback/infrastructure/category/service/CategorySaveServiceTest.java (1)
infrastructure/src/test/java/com/pinback/infrastructure/category/service/CategoryColorServiceTest.java (1)
Import(21-173)
infrastructure/src/test/java/com/pinback/infrastructure/category/service/CategoryColorServiceTest.java (1)
infrastructure/src/test/java/com/pinback/infrastructure/category/service/CategorySaveServiceTest.java (1)
Import(19-128)
domain/src/test/java/com/pinback/domain/article/ArticleDomainTest.java (1)
domain/src/test/java/com/pinback/domain/fixture/TestFixture.java (1)
TestFixture(12-60)
application/src/main/java/com/pinback/application/category/usecase/command/UpdateCategoryUsecase.java (1)
domain/src/main/java/com/pinback/domain/category/exception/CategoryNameLengthOverException.java (1)
CategoryNameLengthOverException(7-11)
domain/src/main/java/com/pinback/domain/category/entity/Category.java (2)
domain/src/main/java/com/pinback/domain/category/exception/CategoryNameLengthOverException.java (1)
CategoryNameLengthOverException(7-11)shared/src/main/java/com/pinback/shared/util/TextUtil.java (1)
TextUtil(6-32)
application/src/test/java/com/pinback/application/category/usecase/command/CreateCategoryUsecaseTest.java (2)
application/src/main/java/com/pinback/application/common/exception/CategoryAlreadyExistException.java (1)
CategoryAlreadyExistException(6-10)application/src/main/java/com/pinback/application/common/exception/CategoryLimitOverException.java (1)
CategoryLimitOverException(6-10)
🔇 Additional comments (36)
api/src/main/java/com/pinback/api/PinbackApiApplication.java (2)
6-6: Import looks correct for Spring Data JPA auditing.
The packageorg.springframework.data.jpa.repository.config.EnableJpaAuditingis the right one.
17-17: Auditing date stamping is correctly configured; no AuditorAware bean needed without user auditing
- BaseEntity (domain/src/main/java/com/pinback/domain/common/BaseEntity.java) is annotated with @MappedSuperclass, @EntityListeners(AuditingEntityListener.class) and declares @CreatedDate/@LastModifiedDate fields.
- There are no @CreatedBy/@LastModifiedBy annotations, so no AuditorAware implementation or bean is required at this time. If you introduce user auditing, supply an AuditorAware bean.
- Optionally extract @EnableJpaAuditing from PinbackApiApplication into a dedicated @configuration class for clearer separation (e.g., com.pinback.infrastructure.config.AuditingConfig).
api/src/main/java/com/pinback/api/auth/controller/AuthController.java (1)
38-45: Legacy/signinendpoint removed—no remaining mappings or callers found.domain/src/main/java/com/pinback/domain/category/exception/CategoryNameLengthOverException.java (1)
1-11: LGTM — exception aligns with shared error code usage.infrastructure/src/main/java/com/pinback/infrastructure/category/repository/CategoryRepositoryCustom.java (1)
16-18: Explicit fallback for exhausted colors
getNextAvailableColor() currently falls back to COLOR1 when a user has used all available colors. Confirm this reuse strategy matches requirements, document it, and add a unit test for the “all colors exhausted” scenario (or adjust to error/LRU reassignment if preferred).domain/src/main/java/com/pinback/domain/category/enums/CategoryColor.java (1)
3-14: LGTM: bounded enum for palette slotsEnum values look good and map cleanly to a fixed palette.
application/src/test/java/com/pinback/application/article/usecase/command/UpdateArticleUsecaseTest.java (1)
62-89: LGTM: ‘now’ plumbed through update flowHappy path verifies cancel + reschedule with token; matches the new 4-arg command.
application/src/main/java/com/pinback/application/article/dto/response/RemindArticlesResponse.java (1)
11-12: LGTM on formatting; RemindArticlesResponse is still referenced in GetArticlePort (import at GetArticlePort.java:8), so do not remove it unless you update the port signature and its implementations.Likely an incorrect or invalid review comment.
application/src/main/java/com/pinback/application/category/usecase/command/UpdateCategoryUsecase.java (1)
11-11: Domain exception import looks correct.infrastructure/src/main/java/com/pinback/infrastructure/article/repository/dto/RemindArticlesWithCount.java (1)
7-11: LGTM; repository-layer carrier is fine.Keep infra record internal, but ensure mapping tests protect shape parity with the application DTO.
Consider adding/expanding a test that asserts the mapper from
RemindArticlesWithCountto the application DTO covers counts and page metadata.application/src/main/java/com/pinback/application/article/usecase/command/UpdateArticleUsecase.java (1)
42-42: Dismiss remindAt type‐equality concern
Both article.getRemindAt() and command.remindTime() are java.time.LocalDateTime (zone-less), so Objects.equals correctly compares them.Likely an incorrect or invalid review comment.
infrastructure/src/main/java/com/pinback/infrastructure/article/service/ArticleGetService.java (2)
12-21: Imports look correct and purposeful.New DTO/infra imports align with the added methods below; nothing stray.
79-86: The search to verify the repository definition is running; I’ll inspect its SQL and parameter usage next.application/src/test/java/com/pinback/application/category/usecase/command/UpdateCategoryUsecaseTest.java (2)
32-38: Ensure Mockito initialization is happening.Since you use @Mock/@Injectmocks without @ExtendWith(MockitoExtension.class), confirm ApplicationTestBase sets up Mockito; otherwise tests may NPE.
40-57: Solid happy-path coverage.Verifies mapping and side effects on aggregate; interaction verified.
infrastructure/src/test/java/com/pinback/infrastructure/fixture/TestFixture.java (1)
49-53: Helpful read-article builder.Keeps tests concise and explicit about remindAt.
infrastructure/src/test/java/com/pinback/infrastructure/category/service/CategorySaveServiceTest.java (1)
32-52: Good persistence assertion coverage.Confirms id, fields, and round-trip via repository.
application/src/main/java/com/pinback/application/article/dto/response/ArticleResponse.java (1)
8-15: API surface changed: newcategoryfield in ArticleResponseConfirm FE and any consumers tolerate the additional
categoryfield name and shape (especially JSON property naming forisReadremains as expected).application/src/main/java/com/pinback/application/article/usecase/query/GetArticleUsecase.java (1)
103-115: Confirm semantics after removing end-of-day boundEarlier logic used an end boundary; now only a single
remindDateTimeis passed to the service. Verify the repository/service still scopes results to the intended “today” window and that counts match previous behavior.application/src/test/java/com/pinback/application/category/usecase/command/DeleteCategoryUsecaseTest.java (1)
22-141: Solid test coverage and ordering checks.Happy path, ownership, not-found, and call order are all covered and precise.
infrastructure/src/test/java/com/pinback/infrastructure/category/service/CategoryColorServiceTest.java (2)
34-50: Solid happy-path coverage for used-colors queryScenario reads well and asserts the exact colors. No issues.
127-172: Good cross-user isolation scenarioNicely demonstrates per-user isolation of color usage. No changes needed.
application/src/main/java/com/pinback/application/article/port/out/ArticleGetServicePort.java (1)
33-36: Confirm reminder-window boundaries
The application buildsremindDateTimeat the user’s configured hour/minute viagetRemindDateTime(now, remindDefault)and the infrastructure layer then queries[remindDateTime, remindDateTime.plusDays(1)]without anyatStartOfDayorLocalTime.MIN/MAXadjustments. Verify this matches the intended semantics (e.g., a rolling 24 h window from the user’s reminder time vs. calendar-day boundaries).infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java (1)
89-97: Approve changes All findTodayRemind invocations—including in ArticleGetServicePort, ArticleGetService, ArticleRepositoryCustom(Impl), and the service tests—are updated to the new signature with the optional isRead parameter.application/src/test/java/com/pinback/application/category/usecase/query/GetCategoryUsecaseTest.java (3)
60-80: Extension list flow test reads clean and preciseGood use of Optional recent article and ordered assertions. Interactions verified.
102-127: Dashboard mapping assertions are clearDTO→Response mapping checks for id/name/unreadCount look correct and focused.
164-178: Exception propagation path validatedGood negative test for CategoryNotFoundException with precise interaction verification.
infrastructure/src/test/java/com/pinback/infrastructure/article/service/ArticleGetServiceTest.java (4)
3-3: Static import aligns with new color enum usage.
Works well with the updated Category.create API.
19-19: DTO import looks correct.
Matches the new findTodayRemindWithCount return type.
127-127: Good: explicit color for the second category.
This validates category filtering across distinct colors.
275-300: Good: “unread” branch mirrored cleanly.
Assertions accurately reflect filtering and counts.domain/src/main/java/com/pinback/domain/category/entity/Category.java (2)
52-55: Color column mapping LGTM.
EnumType.STRING is the right choice for forward compatibility.
72-75: updateName correctly reuses validation.
Once validateName includes null/blank checks, this is complete.application/src/main/java/com/pinback/application/category/usecase/command/CreateCategoryUsecase.java (1)
31-31: New dependency injection for color service looks right.
Port aligns with the selection logic.application/src/test/java/com/pinback/application/article/usecase/query/GetArticleUsecaseTest.java (2)
24-24: LGTM: Import of RemindArticlesWithCountDto aligns with new service port.
This matches the updated API surface and is used correctly in the new tests.
28-28: LGTM: TodayRemindResponse adoption.
The response type switch looks consistent with the new use case contract.
| @Parameter(description = "페이지 번호 (0부터 시작)") @RequestParam(defaultValue = "0") int page, | ||
| @Parameter(description = "페이지 크기") @RequestParam(defaultValue = "8") int size |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard page/size bounds (min/max) at the edge.
If PageQuery doesn’t validate, add @Min/@max to prevent heavy queries and negative indexes.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect PageQuery for bounds/validation
fd -a PageQuery.java
rg -n -C3 -g '!**/build/**' $'class\\s+PageQuery\\b|@Min|@Max|@Positive|@PositiveOrZero|Math\\.(min|max)|requireNonNull|clamp' $(fd -a PageQuery.java)Length of output: 395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd -a PageQuery.java)
echo "Inspecting $file"
sed -n '1,200p' "$file"Length of output: 728
Constrain page/size inputs with bean validation. Add @PositiveOrZero to pageNumber and @Min(1)/@Max(100) to pageSize on the PageQuery record, and enable validation in the controller (e.g. @Validated on the class and @Valid on the PageQuery parameter).
🤖 Prompt for AI Agents
In api/src/main/java/com/pinback/api/article/controller/ArticleController.java
around lines 142-143, the request page and size parameters are not validated;
update the PageQuery record to annotate the page number with @PositiveOrZero and
the page size with @Min(1) and @Max(100), then enable validation in the
controller by adding @Validated on the controller class and annotating the
PageQuery parameter with @Valid so incoming page/size values are constrained and
validated automatically.
| @Schema(description = "현재 시간", example = "2025-12-31T23:59:00") | ||
| @NotNull(message = "현재시간은 필수입니다") | ||
| LocalDateTime now, | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Client-supplied “now” is a trust boundary risk
Allowing the client to dictate “now” enables back/forward-dating and inconsistent reminder behavior. Prefer server-generated time (via Clock) in the application layer; if you must accept it, strictly bound acceptable drift.
Option A — remove now from the API and compute server-side:
- @Schema(description = "현재 시간", example = "2025-12-31T23:59:00")
- @NotNull(message = "현재시간은 필수입니다")
- LocalDateTime now,And adjust toCommand below.
Option B — keep now but validate drift (example 5 minutes) in controller/usecase and ignore if out-of-bounds. I can propose a concrete implementation if you choose this path.
📝 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.
| @Schema(description = "현재 시간", example = "2025-12-31T23:59:00") | |
| @NotNull(message = "현재시간은 필수입니다") | |
| LocalDateTime now, |
🤖 Prompt for AI Agents
In
api/src/main/java/com/pinback/api/article/dto/request/ArticleUpdateRequest.java
around lines 23-26, the DTO exposes a client-supplied "now" which is a
trust-boundary risk; remove the now field from the API and compute current time
server-side (inject java.time.Clock into controller/usecase), then update the
toCommand mapping to use Instant/LocalDateTime from Clock; if you must keep the
field instead, add strict drift validation in the controller/usecase (e.g.,
parse client now, compare to server Clock.now(), reject or ignore the request if
drift > 5 minutes, or clamp to server time) and document the behavior.
| return new ArticleUpdateCommand(categoryId, memo, now, remindTime); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust toCommand if “now” is removed
If you adopt Option A, update the command mapping accordingly.
- return new ArticleUpdateCommand(categoryId, memo, now, remindTime);
+ return new ArticleUpdateCommand(categoryId, memo, remindTime);📝 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.
| return new ArticleUpdateCommand(categoryId, memo, now, remindTime); | |
| } | |
| return new ArticleUpdateCommand(categoryId, memo, remindTime); | |
| } |
🤖 Prompt for AI Agents
In
api/src/main/java/com/pinback/api/article/dto/request/ArticleUpdateRequest.java
around lines 32-33, the toCommand() call still passes the "now" parameter but
the ArticleUpdateCommand no longer expects it (Option A); update the mapping to
match the new constructor by removing the now argument and either (a) call the
constructor without now, or (b) if the command requires a timestamp, instantiate
the command using Instant.now() inside the command constructor or the service
layer—ensure the toCommand signature and the ArticleUpdateCommand constructor
parameters align.
| @Operation(summary = "토큰 재발급", description = "이메일로 토큰을 발급합니다.") | ||
| @GetMapping("/token") | ||
| public ResponseDto<TokenResponse> signIn( | ||
| @Valid @RequestBody SignUpRequest request | ||
| @Valid @RequestParam String email | ||
| ) { | ||
| TokenResponse response = authUsecase.getToken(request.email()); | ||
| TokenResponse response = authUsecase.getToken(email); | ||
| return ResponseDto.ok(response); | ||
| } |
There was a problem hiding this comment.
Blocker: GET + email-only token issuance is insecure (account takeover, user enumeration, cacheable).
Issuing tokens with just an email via GET enables:
- User enumeration by response differences.
- Token delivery without proof-of-identity (no password/refresh-token/OTP).
- Query-string PII leakage in logs/proxies and potential caching of responses.
Action:
- Use POST.
- Require a credential: valid refresh token, password, or a one-time code delivered out-of-band. If this is a “magic link” flow, don’t return the token here—send the link and return 202.
- Add input constraints and normalize email.
- Add rate limiting and ensure Cache-Control: no-store on responses.
Apply within this hunk:
- @Operation(summary = "토큰 재발급", description = "이메일로 토큰을 발급합니다.")
- @GetMapping("/token")
- public ResponseDto<TokenResponse> signIn(
- @Valid @RequestParam String email
- ) {
- TokenResponse response = authUsecase.getToken(email);
+ @Operation(summary = "토큰 재발급", description = "이메일로 토큰을 발급합니다.")
+ @PostMapping("/token")
+ public ResponseDto<TokenResponse> issueToken(
+ @RequestParam @NotBlank @Email String email
+ ) {
+ TokenResponse response = authUsecase.getToken(email.trim().toLowerCase());
return ResponseDto.ok(response);
}Also ensure global/security layer sets Cache-Control: no-store and adds rate limiting (e.g., IP+email). If this endpoint replaces a prior signin flow, consider a deprecation window or a compatibility shim.
📝 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.
| @Operation(summary = "토큰 재발급", description = "이메일로 토큰을 발급합니다.") | |
| @GetMapping("/token") | |
| public ResponseDto<TokenResponse> signIn( | |
| @Valid @RequestBody SignUpRequest request | |
| @Valid @RequestParam String email | |
| ) { | |
| TokenResponse response = authUsecase.getToken(request.email()); | |
| TokenResponse response = authUsecase.getToken(email); | |
| return ResponseDto.ok(response); | |
| } | |
| @Operation(summary = "토큰 재발급", description = "이메일로 토큰을 발급합니다.") | |
| - @GetMapping("/token") | |
| - public ResponseDto<TokenResponse> signIn( | |
| - @Valid @RequestParam String email | |
| - ) { | |
| @PostMapping("/token") | |
| public ResponseDto<TokenResponse> issueToken( | |
| @RequestParam @NotBlank @Email String email | |
| ) { | |
| // Normalize email to prevent case‐sensitive duplicates | |
| String normalizedEmail = email.trim().toLowerCase(); | |
| TokenResponse response = authUsecase.getToken(normalizedEmail); | |
| return ResponseDto.ok(response); | |
| } |
🤖 Prompt for AI Agents
In api/src/main/java/com/pinback/api/auth/controller/AuthController.java around
lines 38-45, the current GET /token endpoint accepts only an email and returns a
token which is insecure; change this to a POST endpoint, validate and normalize
the email (trim, lowercase, length and pattern checks), and require an
authentication credential (either a valid refresh token in Authorization/cookie,
a password field, or a one-time code) — if implementing a magic-link flow, do
not return a token: send the link and return 202 Accepted. Ensure the controller
sets Cache-Control: no-store on responses, add input constraints (DTO validation
annotations) and server-side rate limiting (IP+email) for this route, and
coordinate with the global/security layer to enforce no-store and rate limiting
policy; if compatibility is needed, implement a documented deprecation shim.
| @GetMapping("/token") | ||
| public ResponseDto<TokenResponse> signIn( | ||
| @Valid @RequestBody SignUpRequest request | ||
| @Valid @RequestParam String email |
There was a problem hiding this comment.
🛠️ Refactor suggestion
@Valid on String is a no-op; add concrete constraints.
Add @Email and @notblank to enforce format and presence. Keep normalization server-side.
Add imports outside this hunk:
import jakarta.validation.constraints.Email;
import jakarta.validation.constraints.NotBlank;Diff within this line is included in the main change above.
🤖 Prompt for AI Agents
In api/src/main/java/com/pinback/api/auth/controller/AuthController.java around
line 41, the @Valid on a String is ineffective—replace it with concrete
validation annotations by adding @Email and @NotBlank to the email parameter and
ensure you add the corresponding imports (jakarta.validation.constraints.Email
and jakarta.validation.constraints.NotBlank) at the top of the file; keep any
server-side normalization logic unchanged.
| Page<Article> findTodayRemind(UUID userId, Pageable pageable, LocalDateTime startAt, LocalDateTime endAt, Boolean isRead); | ||
|
|
||
| RemindArticlesWithCount findTodayRemindWithCount(UUID userId, Pageable pageable, LocalDateTime startAt, LocalDateTime endAt, Boolean isRead); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify implementors updated to new signatures
Since this is an interface change, ensure all implementations compile and tests cover both null and non-null isRead.
🏁 Script executed:
#!/bin/bash
# Find all implementors and method signatures affected by the change
rg -nP 'class\s+\w+\s+implements\s+ArticleRepositoryCustom'
rg -nP 'findTodayRemind\('
rg -nP 'findTodayRemindWithCount\('
# Look for call sites passing null or booleans to isRead
rg -nP --type=java -C2 '\bfindTodayRemind\([^)]*?,\s*[^)]*?,\s*[^)]*?,\s*[^)]*?,\s*(null|true|false)\)'Length of output: 4620
Add tests for non-null isRead in findTodayRemind
ArticleGetServiceTest only covers the null case for findTodayRemind; add scenarios passing both true and false to fully validate the new signature.
🤖 Prompt for AI Agents
in
infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustom.java
lines 18-21 the interface now has a non-null Boolean isRead parameter for
findTodayRemind (and its WithCount variant) but tests only cover the null case;
add unit tests to ArticleGetServiceTest that call the service/repository with
isRead = true and isRead = false (in addition to the existing null case), set up
test data for read and unread articles within the date window, invoke
findTodayRemind (and findTodayRemindWithCount if exercised) with appropriate
Pageable and start/end datetimes, and assert that results only contain articles
matching the isRead filter and that counts reflect the filtered set.
| @Override | ||
| public RemindArticlesWithCount findTodayRemindWithCount(UUID userId, Pageable pageable, LocalDateTime startAt, | ||
| LocalDateTime endAt, Boolean isRead) { | ||
| BooleanExpression baseConditions = article.user.id.eq(userId) | ||
| .and(article.remindAt.goe(startAt).and(article.remindAt.loe(endAt))); | ||
|
|
||
| BooleanExpression conditions = baseConditions.and(article.isRead.eq(isRead)); | ||
|
|
||
| List<Article> articles = queryFactory |
There was a problem hiding this comment.
Null handling bug: eq(isRead) will NPE when isRead == null
Unlike the method above, this one unconditionally does article.isRead.eq(isRead). If isRead is null, QueryDSL will throw at runtime. Mirror the null-guard pattern.
Apply:
-BooleanExpression conditions = baseConditions.and(article.isRead.eq(isRead));
+BooleanExpression conditions = baseConditions;
+if (isRead != null) {
+ conditions = conditions.and(article.isRead.eq(isRead));
+}📝 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.
| @Override | |
| public RemindArticlesWithCount findTodayRemindWithCount(UUID userId, Pageable pageable, LocalDateTime startAt, | |
| LocalDateTime endAt, Boolean isRead) { | |
| BooleanExpression baseConditions = article.user.id.eq(userId) | |
| .and(article.remindAt.goe(startAt).and(article.remindAt.loe(endAt))); | |
| BooleanExpression conditions = baseConditions.and(article.isRead.eq(isRead)); | |
| List<Article> articles = queryFactory | |
| @Override | |
| public RemindArticlesWithCount findTodayRemindWithCount(UUID userId, Pageable pageable, LocalDateTime startAt, | |
| LocalDateTime endAt, Boolean isRead) { | |
| BooleanExpression baseConditions = article.user.id.eq(userId) | |
| .and(article.remindAt.goe(startAt).and(article.remindAt.loe(endAt))); | |
| BooleanExpression conditions = baseConditions; | |
| if (isRead != null) { | |
| conditions = conditions.and(article.isRead.eq(isRead)); | |
| } | |
| List<Article> articles = queryFactory | |
| // … rest of the query construction … |
🤖 Prompt for AI Agents
in
infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java
around lines 144-152, the code unconditionally calls article.isRead.eq(isRead)
which will NPE when isRead is null; create a nullable BooleanExpression
isReadCondition = (isRead != null) ? article.isRead.eq(isRead) : null and then
combine it with baseConditions (e.g., BooleanExpression conditions =
baseConditions.and(isReadCondition)) so the predicate building mirrors the
null-guard pattern used elsewhere and avoids calling eq(null).
| public Page<Article> findTodayRemind(User user, LocalDateTime remindDateTime, Pageable pageable, Boolean isRead) { | ||
| return articleRepository.findTodayRemind(user.getId(), pageable, remindDateTime, remindDateTime.plusDays(1), isRead); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify “today” window; fix boundary to end-of-day.
Using now.plusDays(1) creates a 24h window, not “until midnight.” Prefer [start = now, endExclusive = startOfTomorrow] to avoid leaking into tomorrow.
Apply:
- return articleRepository.findTodayRemind(user.getId(), pageable, remindDateTime, remindDateTime.plusDays(1), isRead);
+ LocalDateTime endExclusive = remindDateTime.toLocalDate().plusDays(1).atStartOfDay();
+ return articleRepository.findTodayRemind(user.getId(), pageable, remindDateTime, endExclusive, isRead);Optionally extract a helper to centralize the window:
private static LocalDateTime endOfDayExclusive(LocalDateTime t) {
return t.toLocalDate().plusDays(1).atStartOfDay();
}🤖 Prompt for AI Agents
in
infrastructure/src/main/java/com/pinback/infrastructure/article/service/ArticleGetService.java
around lines 75 to 77, the code uses remindDateTime.plusDays(1) which creates a
24-hour window instead of an end-of-day boundary; replace the end boundary with
the start of the next day (i.e., endExclusive =
remindDateTime.toLocalDate().plusDays(1).atStartOfDay()) and pass that to
articleRepository.findTodayRemind; optionally extract a private helper like
endOfDayExclusive(LocalDateTime t) returning
t.toLocalDate().plusDays(1).atStartOfDay() and use it here for clarity and
reuse.
| // when & then | ||
| assertThatThrownBy(() -> categorySaveService.save(category2)) | ||
| .isInstanceOf(Exception.class); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Force constraint evaluation and assert a specific exception.
Without flush, the exception may surface on transaction commit, outside the assertion. Also prefer a concrete type.
- assertThatThrownBy(() -> categorySaveService.save(category2))
- .isInstanceOf(Exception.class);
+ assertThatThrownBy(() -> {
+ categorySaveService.save(category2);
+ categoryRepository.flush(); // force DB constraint check now
+ }).isInstanceOfAny(
+ org.springframework.dao.DataIntegrityViolationException.class,
+ jakarta.persistence.PersistenceException.class
+ );📝 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.
| // when & then | |
| assertThatThrownBy(() -> categorySaveService.save(category2)) | |
| .isInstanceOf(Exception.class); | |
| } | |
| // when & then | |
| assertThatThrownBy(() -> { | |
| categorySaveService.save(category2); | |
| categoryRepository.flush(); // force DB constraint check now | |
| }).isInstanceOfAny( | |
| org.springframework.dao.DataIntegrityViolationException.class, | |
| jakarta.persistence.PersistenceException.class | |
| ); | |
| } |
🤖 Prompt for AI Agents
in
infrastructure/src/test/java/com/pinback/infrastructure/category/service/CategorySaveServiceTest.java
around lines 112-115, the test currently asserts a generic Exception and doesn't
force constraint evaluation; change the assertion to call save and then flush
the persistence context inside the asserted lambda (e.g. call
entityManager.flush() or repository.flush() after saving) so the DB constraint
is evaluated within the assertion, and replace Exception.class with a concrete
exception type expected on constraint violation (e.g.
DataIntegrityViolationException or ConstraintViolationException) and import that
class.
| void save_NullColor_ThrowsException() { | ||
| // given | ||
| User user = userRepository.save(user()); | ||
| Category category = Category.create("테스트카테고리", user, null); | ||
|
|
||
| // when & then | ||
| assertThatThrownBy(() -> categorySaveService.save(category)) | ||
| .isInstanceOf(Exception.class); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same here: flush and assert specific type.
If null color is guarded only by DB, you need flush to observe the violation.
- assertThatThrownBy(() -> categorySaveService.save(category))
- .isInstanceOf(Exception.class);
+ assertThatThrownBy(() -> {
+ categorySaveService.save(category);
+ categoryRepository.flush();
+ }).isInstanceOfAny(
+ org.springframework.dao.DataIntegrityViolationException.class,
+ jakarta.persistence.PersistenceException.class
+ );If the domain should forbid null earlier, change this test to expect the domain exception at creation time instead.
📝 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.
| void save_NullColor_ThrowsException() { | |
| // given | |
| User user = userRepository.save(user()); | |
| Category category = Category.create("테스트카테고리", user, null); | |
| // when & then | |
| assertThatThrownBy(() -> categorySaveService.save(category)) | |
| .isInstanceOf(Exception.class); | |
| } | |
| void save_NullColor_ThrowsException() { | |
| // given | |
| User user = userRepository.save(user()); | |
| Category category = Category.create("테스트카테고리", user, null); | |
| // when & then | |
| assertThatThrownBy(() -> { | |
| categorySaveService.save(category); | |
| categoryRepository.flush(); | |
| }).isInstanceOfAny( | |
| org.springframework.dao.DataIntegrityViolationException.class, | |
| jakarta.persistence.PersistenceException.class | |
| ); | |
| } |
🤖 Prompt for AI Agents
in
infrastructure/src/test/java/com/pinback/infrastructure/category/service/CategorySaveServiceTest.java
around lines 119 to 127, the test only asserts a generic Exception and doesn't
flush the persistence context so DB-level null constraints may not be observed;
update the test to either (A) if null color is enforced by the DB, call save and
then flush the EntityManager (or use saveAndFlush) and assert the specific
persistence exception (e.g., DataIntegrityViolationException or
ConstraintViolationException) is thrown, or (B) if the domain should prevent
null earlier, change the test to expect the domain-level exception (e.g.,
IllegalArgumentException) at Category.create(...) time instead of relying on DB
errors.
2 participants
🚀 PR 요약
카테고리의 색상을 추가 하는 기능을 구현했습니다.
close #88
✨ PR 상세 내용
Category
Article
🚨 주의 사항
없습니다.
✅ 체크 리스트
Summary by CodeRabbit
New Features
Changes
Chores