Conversation
- PushTestRequest: FCM 푸시 테스트용 요청 DTO - CategoriesTestResponse: 카테고리 생성 테스트용 응답 DTO
- TestPort: Test API 비즈니스 로직 인터페이스 - FcmServicePort: FCM 서비스 추상화 포트
- FCM 푸시 알림 테스트 - 카테고리별 아티클 5개 생성 - 디폴트 카테고리 10개 생성 (사용하지 않는 색상 자동 할당) - 카테고리별 아티클 삭제
- FcmServicePort 구현체 - 기존 FcmService를 포트 패턴으로 연결
- POST /api/v1/test/push: FCM 푸시 테스트
- POST /api/v1/test/articles: 카테고리별 아티클 생성
- POST /api/v1/test/categories: 카테고리 10개 생성
- DELETE /api/v1/test/articles/{categoryId}: 카테고리별 아티클 삭제
WalkthroughAdds a new test REST API (controller, DTOs, ports, usecase) with FCM integration and an infrastructure adapter; creates sample article/category flows and deletes by category; and updates the Article entity to set URL column length to 700. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as TestController
participant Usecase as TestUsecase (implements TestPort)
participant FCMPort as FcmServicePort
participant FCM as FcmService
Client->>Controller: POST /api/v1/test/push {fcmToken, message}
Controller->>Usecase: pushTest(PushTestRequest)
Usecase->>FCMPort: sendNotification(token, message)
FCMPort->>FCM: sendNotification(token, message)
FCM-->>FCMPort: ack
FCMPort-->>Usecase: void
Usecase-->>Controller: void
Controller-->>Client: ResponseDto.ok()
sequenceDiagram
autonumber
actor Client
participant Controller as TestController
participant Usecase as TestUsecase
participant UserSvc as UserGetServicePort
participant CatGet as CategoryGetServicePort
participant ArtSave as ArticleSaveServicePort
participant CatSave as CategorySaveServicePort
participant Color as CategoryColorServicePort
participant ArtDel as ArticleDeleteServicePort
rect rgb(245,248,255)
note over Client,Usecase: Create articles by category
Client->>Controller: POST /api/v1/test/articles?categoryId
Controller->>Usecase: createArticlesByCategory(user, categoryId)
Usecase->>UserSvc: fetch user
Usecase->>CatGet: fetch category
loop 5 times
Usecase->>ArtSave: save(new Article(...))
end
Usecase-->>Controller: void
Controller-->>Client: ResponseDto.ok()
end
rect rgb(245,255,245)
note over Client,Usecase: Create categories
Client->>Controller: POST /api/v1/test/categories
Controller->>Usecase: createCategories(user)
Usecase->>UserSvc: fetch user
Usecase->>Color: get used colors
loop names (up to 10)
Usecase->>CatSave: save(new Category(name, nextColor))
end
Usecase-->>Controller: CategoriesTestResponse
Controller-->>Client: ResponseDto.ok(response)
end
rect rgb(255,245,245)
note over Client,Usecase: Delete articles by category
Client->>Controller: DELETE /api/v1/test/articles/{categoryId}
Controller->>Usecase: deleteArticlesByCategory(user, categoryId)
Usecase->>UserSvc: fetch user
Usecase->>CatGet: fetch category
Usecase->>ArtDel: deleteByCategory(user, category)
Usecase-->>Controller: void
Controller-->>Client: ResponseDto.ok()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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: 3
🧹 Nitpick comments (9)
domain/src/main/java/com/pinback/domain/article/entity/Article.java (1)
43-44: Add Bean Validation to fail fast before hitting DB constraintsApplying @SiZe keeps validation consistent with the JPA length and avoids runtime SQL errors.
- @Column(name = "url", length = 700, nullable = false) + @jakarta.validation.constraints.Size(max = 700) + @Column(name = "url", length = 700, nullable = false) private String url;If you prefer imports, add:
import jakarta.validation.constraints.Size;application/src/main/java/com/pinback/application/test/port/out/FcmServicePort.java (1)
3-5: Consider returning a send result/messageId for observabilityA void signature hides delivery outcomes. Returning a result (messageId/status) improves tracing and testability. Keep as-is if intentionally minimal for test endpoints.
application/src/main/java/com/pinback/application/test/dto/request/PushTestRequest.java (1)
3-7: Validate inputs (token/message) to avoid useless push attemptsAdd basic Bean Validation. Pair with @Valid in the controller.
-public record PushTestRequest( - String fcmToken, - String message -) { +public record PushTestRequest( + @jakarta.validation.constraints.NotBlank String fcmToken, + @jakarta.validation.constraints.NotBlank + @jakarta.validation.constraints.Size(max = 2000) String message +) { }Optionally import:
import jakarta.validation.constraints.NotBlank; import jakarta.validation.constraints.Size;application/src/main/java/com/pinback/application/test/port/in/TestPort.java (1)
7-15: API shape is fine; consider passing ids instead of User to reduce couplingNot required for test flows, but passing userId can simplify serialization boundaries and mocking.
api/src/main/java/com/pinback/api/test/controller/TestController.java (1)
50-57: Nit: rename method for clarity
deleteTest->deleteArticlesbetter reflects behavior.- public ResponseDto<Void> deleteTest( + public ResponseDto<Void> deleteArticles(application/src/main/java/com/pinback/application/test/usecase/TestUsecase.java (4)
62-97: Use CATEGORY_LIMIT constant and improve color/name handling
- Replace magic number 10 with CATEGORY_LIMIT.
- Safer color fallback when all colors used (cycle instead of always COLOR1).
@Override public CategoriesTestResponse createCategories(User user) { @@ - for (int i = 0; i < 10; i++) { + for (int i = 0; i < CATEGORY_LIMIT; i++) { String categoryName = defaultCategoryNames.get(i % defaultCategoryNames.size()); if (i >= defaultCategoryNames.size()) { categoryName = categoryName + "_" + (i - defaultCategoryNames.size() + 1); } - CategoryColor availableColor = getNextAvailableColor(usedColors); + CategoryColor availableColor = getNextAvailableColor(usedColors); Category category = Category.create(categoryName, getUser, availableColor); Category savedCategory = categorySaveServicePort.save(category); createdCategoryNames.add(savedCategory.getName()); // 사용된 색상 업데이트 usedColors.add(availableColor); }And adjust helper below (see next comment) to cycle colors.
99-104: Cycle colors when all are used, don’t always fall back to COLOR1Prevents overuse of COLOR1.
private CategoryColor getNextAvailableColor(Set<CategoryColor> usedColors) { - 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() + .orElseGet(() -> { + CategoryColor[] values = CategoryColor.values(); + return values[usedColors.size() % values.length]; + }); }
46-59: Optional: validate category ownership before creating articlesAvoids creating user articles in another user’s category if misused.
public void createArticlesByCategory(User user, Long categoryId) { String format = "%s:%s"; Category category = categoryGetServicePort.findById(categoryId); + // if (!category.getUser().getId().equals(user.getId())) { throw new AccessDeniedException("Category not owned by user"); }
106-111: Transactional already handled; add explicit ownership check in TestUsecase (optional)
- ArticleDeleteService is annotated @transactional and calls articleRepository.deleteArticlesByUserIdAndCategoryId(user.getId(), categoryId) — operation is transactional and scoped by user+category. (infrastructure/src/main/java/com/pinback/infrastructure/article/service/ArticleDeleteService.java)
- Category exposes isOwnedBy(User) and DeleteCategoryUsecase enforces ownership (domain/src/main/java/com/pinback/domain/category/entity/Category.java, application/src/main/java/com/pinback/application/category/usecase/command/DeleteCategoryUsecase.java)
- Recommendation: for consistency/clearer error semantics, add an ownership check in TestUsecase.deleteArticlesByCategory (throw CategoryNotOwnedException) or add a short comment noting deleteByCategory is already scoped by user. This is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/src/main/java/com/pinback/api/test/controller/TestController.java(1 hunks)application/src/main/java/com/pinback/application/test/dto/request/PushTestRequest.java(1 hunks)application/src/main/java/com/pinback/application/test/dto/response/CategoriesTestResponse.java(1 hunks)application/src/main/java/com/pinback/application/test/port/in/TestPort.java(1 hunks)application/src/main/java/com/pinback/application/test/port/out/FcmServicePort.java(1 hunks)application/src/main/java/com/pinback/application/test/usecase/TestUsecase.java(1 hunks)domain/src/main/java/com/pinback/domain/article/entity/Article.java(1 hunks)infrastructure/src/main/java/com/pinback/infrastructure/firebase/FcmServiceAdapter.java(1 hunks)
🔇 Additional comments (3)
domain/src/main/java/com/pinback/domain/article/entity/Article.java (1)
43-44: Confirm DB migration & index-size for url VARCHAR(700)domain/src/main/java/com/pinback/domain/article/entity/Article.java: @column(name = "url", length = 700) with @UniqueConstraint(columnNames = {"user_id","url"}) (lines ~43-44). No DB migration (Flyway/Liquibase) found in repo; tests run against H2 (MySQL mode).
- Add/confirm a schema migration that alters article.url to VARCHAR(700) for your production DB.
- Verify production DB/version and InnoDB settings can support the index size — utf8mb4 700 chars ≈ 2800 bytes and can exceed legacy MySQL index limits; if not, enable large-prefix/Barracuda/DYNAMIC row format or move to MySQL 8+.
- Alternatives: add url_hash (e.g., SHA‑256) and unique(user_id, url_hash), or use a shorter indexed prefix for url.
infrastructure/src/main/java/com/pinback/infrastructure/firebase/FcmServiceAdapter.java (1)
11-17: LGTM: clean adapter, proper wiring through the portStraight delegation keeps infra thin. Ensure FcmService is a Spring bean in the same package scan.
application/src/main/java/com/pinback/application/test/dto/response/CategoriesTestResponse.java (1)
5-11: LGTM: small, focused DTO with factoryRecord + of(...) is clear.
| @RestController | ||
| @RequestMapping("/api/v1/test") | ||
| @RequiredArgsConstructor | ||
| public class TestController { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate “test” endpoints to non-prod (profile/feature flag or auth role)
These endpoints create/delete data and send push. Restrict to local/dev or admin.
-@RestController
-@RequestMapping("/api/v1/test")
-@RequiredArgsConstructor
-public class TestController {
+@org.springframework.context.annotation.Profile({"local","dev"})
+@org.springframework.validation.annotation.Validated
+@RestController
+@RequestMapping("/api/v1/test")
+@RequiredArgsConstructor
+public class TestController {If profiles aren’t feasible, add @PreAuthorize("hasRole('ADMIN')") or guard via a property (e.g., @ConditionalOnProperty("pinback.test.enabled", havingValue="true")).
📝 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.
| @RestController | |
| @RequestMapping("/api/v1/test") | |
| @RequiredArgsConstructor | |
| public class TestController { | |
| @org.springframework.context.annotation.Profile({"local","dev"}) | |
| @org.springframework.validation.annotation.Validated | |
| @RestController | |
| @RequestMapping("/api/v1/test") | |
| @RequiredArgsConstructor | |
| public class TestController { |
🤖 Prompt for AI Agents
In api/src/main/java/com/pinback/api/test/controller/TestController.java around
lines 21 to 24, the controller exposing test endpoints is unguarded and must be
restricted to non-prod or admin use; update the class to be conditionally
enabled or authorized by either (a) adding a profile/conditional property such
as @Profile("local|dev") or @ConditionalOnProperty(name="pinback.test.enabled",
havingValue="true") to disable in prod, or (b) add method/class-level security
like @PreAuthorize("hasRole('ADMIN')") and ensure Spring Security configuration
enables global method security; apply one of these guards consistently to all
test endpoints and wire the corresponding property/profile or role so only
intended environments/users can access them.
| @PostMapping("/push") | ||
| public ResponseDto<Void> pushTest(@RequestBody PushTestRequest pushTestRequest) { | ||
| testPort.pushTest(pushTestRequest); | ||
| return ResponseDto.ok(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enable request validation on body
- public ResponseDto<Void> pushTest(@RequestBody PushTestRequest pushTestRequest) {
+ public ResponseDto<Void> pushTest(@jakarta.validation.Valid @RequestBody PushTestRequest pushTestRequest) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PostMapping("/push") | |
| public ResponseDto<Void> pushTest(@RequestBody PushTestRequest pushTestRequest) { | |
| testPort.pushTest(pushTestRequest); | |
| return ResponseDto.ok(); | |
| } | |
| @PostMapping("/push") | |
| public ResponseDto<Void> pushTest(@jakarta.validation.Valid @RequestBody PushTestRequest pushTestRequest) { | |
| testPort.pushTest(pushTestRequest); | |
| return ResponseDto.ok(); | |
| } |
🤖 Prompt for AI Agents
In api/src/main/java/com/pinback/api/test/controller/TestController.java around
lines 27 to 31, the request body is not validated; add request validation by
annotating the @RequestBody parameter with @Valid, ensure the controller class
is annotated with @Validated (or validation is enabled globally), and add
JSR-303 annotations (e.g., @NotNull, @NotBlank, @Size) to the fields in
PushTestRequest; also ensure a MethodArgumentNotValidException handler (or
global @ControllerAdvice) exists to return proper validation error responses.
| @Override | ||
| public void createArticlesByCategory(User user, Long categoryId) { | ||
| String format = "%s:%s"; | ||
| Category category = categoryGetServicePort.findById(categoryId); | ||
|
|
||
| for (int i = 0; i < 5; i++) { | ||
| Article article = Article.create( | ||
| String.format(format, user.getEmail(), UUID.randomUUID()), | ||
| "testMemo", | ||
| user, | ||
| category, | ||
| null | ||
| ); | ||
| articleSaveServicePort.save(article); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap write operations in transactions
Prevents partial writes on mid-loop failures.
@Override
- public void createArticlesByCategory(User user, Long categoryId) {
+ @org.springframework.transaction.annotation.Transactional
+ public void createArticlesByCategory(User user, Long categoryId) {Also add (outside diff range) the import:
import org.springframework.transaction.annotation.Transactional;I can annotate other write methods similarly in a follow-up diff below.
🤖 Prompt for AI Agents
In
application/src/main/java/com/pinback/application/test/usecase/TestUsecase.java
around lines 45 to 61, the createArticlesByCategory method performs multiple
writes without a transaction; annotate the method with @Transactional to ensure
the loop is executed within a single transaction so partial writes are rolled
back on failure, and add the import line import
org.springframework.transaction.annotation.Transactional; at the top of the
file; keep the annotation on the method signature and do not change method
visibility.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java (2)
150-151: Potential empty result when isRead is null (mismatch with counts).
article.isRead.eq(isRead)evaluates toIS NULLifisReadis null, likely filtering out all rows while read/unread counts still reflect both states.- BooleanExpression conditions = baseConditions.and(article.isRead.eq(isRead)); + BooleanExpression conditions = baseConditions; + if (isRead != null) { + conditions = conditions.and(article.isRead.eq(isRead)); + }
61-87: User scoping missing in findAllByCategory → possible cross-tenant data leak.
userIdparameter is unused; queries are scoped only bycategoryId. Align with other methods to enforce per-user isolation.- BooleanExpression conditions = article.category.id.eq(categoryId); + BooleanExpression conditions = article.user.id.eq(userId) + .and(article.category.id.eq(categoryId));
🧹 Nitpick comments (2)
infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java (2)
146-176: Add supporting index for the new sort/filter pattern.Queries filter by (user_id, remind_at [range], is_read) and sort by remind_at. Add a composite btree index to keep scans and sorts efficient.
Example (PostgreSQL):
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_article_user_remind_isread ON article (user_id, remind_at, is_read);If MySQL, create an equivalent composite index.
105-106: Verify intentional sort divergence between similar endpoints.findTodayRemind uses createdAt DESC; findTodayRemindWithCount uses remindAt ASC. If UX expects consistent ordering, align them; otherwise add a comment explaining the difference.
Also applies to: 158-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java(1 hunks)
| .orderBy(article.remindAt.asc()) | ||
| .fetch(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Switch to remindAt ASC is sensible, but add a deterministic tie-breaker for stable pagination.
Without a secondary key, rows sharing the same remindAt can shuffle between pages.
- .orderBy(article.remindAt.asc())
+ .orderBy(article.remindAt.asc(), article.id.asc())📝 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.
| .orderBy(article.remindAt.asc()) | |
| .fetch(); | |
| .orderBy(article.remindAt.asc(), article.id.asc()) | |
| .fetch(); |
🤖 Prompt for AI Agents
in
infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java
around lines 158-159 the query currently orders only by remindAt which can lead
to non-deterministic result ordering for rows with identical remindAt; add a
deterministic tie-breaker by appending a secondary order by a unique, stable
column such as article.id (or another unique timestamp) so the orderBy becomes
remindAt.asc() followed by id.asc() to guarantee stable pagination across pages.
2 participants
🚀 PR 요약
test api를 재생성하고 url 최대길이를 증가시켰습니다.
close #97
✨ PR 상세 내용
🚨 주의 사항
없습니다
✅ 체크 리스트
Summary by CodeRabbit
New Features
Improvements