feat: 알림 서버 구축#135
Open
dongho108 wants to merge 19 commits into
Open
Conversation
- 디바이스 토큰 등록/해제 API (POST/DELETE /api/v1/device-tokens) - 알림 이력 조회/읽음 처리 API (GET/PATCH /api/v1/notifications) - 가용시간 등록 시 방장에게 FCM 알림 발송 (이벤트 기반, 비동기) - FCM 토글 지원 (fcm.enabled=false → NoOpNotificationSender) - 만료 토큰 자동 정리 (UNREGISTERED/INVALID_ARGUMENT 감지) - CORS PATCH 메서드 허용 추가 - SQL 마이그레이션 파일 및 CLAUDE.md DB 마이그레이션 가이드 추가 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- `가용시간_등록_알림` 메서드명을 `sendReplaceMessage`로 변경 - 관련 테스트 및 이벤트 핸들러 코드에 반영 - 불필요한 import 제거 (e.g., `LocalDateTime`, `NotificationQueryRepository
생성자를 private으로 변경하고 Notification.of()를 통해 객체를 생성하도록 수정. CLAUDE.md에 정적 팩터리 메서드 컨벤션(from/of) 문서화. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
NotificationSender.send()의 반환타입을 void에서 NotificationSendResult로 변경하여 발송 성공/실패 건수와 실패 토큰을 추적할 수 있도록 개선. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
알림 생성, FCM 발송, 발송 결과 반영 로직을 NotificationProcessor로 분리하여 NotificationService의 책임을 축소하고 재사용성을 높임. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
notification 테이블에 sent, sent_at 컬럼 추가 마이그레이션 작성. application-fcm.yaml을 local/prod 프로필로 분리. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- sent, sent_at 컬럼 제거 후 관련 마이그레이션 파일 삭제. - CREATE_NOTIFICATION.sql 파일에 sent 컬럼과 관련 정의 통합.
roomUuid, senderName 컬럼을 제거하고 Map<String, String> data 필드로 대체하여 알림 타입별 유연한 컨텍스트 데이터를 저장할 수 있도록 변경 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 설계 상세 문서(notification-system.md) 제거 - 프로젝트에서 불필요해진 문서로 판단하여 정리.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughFCM 기반 푸시 알림 서브시스템이 추가되었습니다. 기기 토큰 관리, 알림 엔티티·저장소, 알림 생성·전송·조회·읽음 처리 서비스, FCM/No‑Op 전송 구현, 비동기 실행 빈 및 DB 마이그레이션이 포함됩니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant Repo as DeviceTokenRepo
participant DB as Database
participant Sender as FCM/NoOp
Client->>Controller: POST /api/v1/device-tokens (token)
Controller->>Service: register(command)
Service->>Repo: findByToken(token)
alt exists && different user
Repo-->>Service: existing token (other user)
Service->>Repo: deleteByToken(token)
Service->>Repo: save(new DeviceToken)
else exists && same user
Repo-->>Service: existing token (same user)
else not exists
Service->>Repo: save(new DeviceToken)
Repo->>DB: INSERT device_token
end
Service-->>Controller: HTTP 응답 (201/200)
Controller-->>Client: 응답 전송
sequenceDiagram
participant Event as TimeBlockReplaceEvent
participant Handler as NotificationEventHandler
participant Service as NotificationService
participant Processor as NotificationProcessor
participant TokenRepo as DeviceTokenQueryRepository
participant Sender as FCM/NoOp
participant FCM as Firebase
participant DB as Database
Event->>Handler: AFTER_COMMIT (async)
Handler->>Service: sendReplaceMessage(roomUuid, participantName)
Service->>Service: resolve target userIds, build payload
Service->>Processor: process(userIds, type, title, message, data)
Processor->>TokenRepo: findByUserIdIn(userIds)
TokenRepo-->>Processor: List<DeviceToken>
Processor->>Sender: send(tokens, title, message, data)
Sender->>FCM: multicast request (if enabled)
FCM-->>Sender: BatchResponse
Sender-->>Processor: NotificationSendResult
Processor->>Processor: mark notifications sent if success
Processor->>DB: saveAll(notifications)
DB-->>Processor: OK
Processor-->>Service: 완료
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 사용자 경험을 향상시키기 위해 Firebase Cloud Messaging(FCM) 기반의 알림 시스템을 통합합니다. 주요 목표는 사용자가 모임의 가용 시간을 업데이트할 때 다른 참여자들에게 실시간으로 알림을 제공하는 것입니다. 이 시스템은 디바이스 토큰 관리, 알림 발송 및 이력 저장 기능을 포함하며, 비동기 이벤트 처리를 통해 핵심 서비스의 성능에 영향을 주지 않도록 설계되었습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces a notification server using FCM, which is well-structured with domain models, services, infrastructure, API documentation, and test code, utilizing event-driven asynchronous processing and ConditionalOnProperty. No significant security vulnerabilities were identified; the the implementation is robust, handles authentication/authorization, and database queries are safe from SQL injection. There are a few areas for improvement noted in the specific comments.
| activate: | ||
| on-profile: prod | ||
| fcm: | ||
| enabled: false |
There was a problem hiding this comment.
prod 프로필에서 fcm.enabled가 false로 설정되어 있습니다. 이는 기능 플래그로서 의도된 설정일 수 있으나, 프로덕션 환경에서 FCM 알림 기능이 동작하지 않을 수 있습니다. 배포 시점에 이 설정이 올바른지 다시 한번 확인해 보시는 것을 권장합니다.
| @@ -0,0 +1,38 @@ | |||
| -- 디바이스 토큰 테이블 생성 (FCM 푸시 알림용) | |||
There was a problem hiding this comment.
파일명이 V20260311_CREATE_NOTIFICATION.sql로 되어 있는데, 2026년은 오타인 것 같습니다. 컨벤션에 맞게 현재 날짜로 수정하는 것을 고려해 보세요.
| is_read tinyint(1) NOT NULL DEFAULT 0 COMMENT '읽음 여부', | ||
| read_at datetime(6) NULL COMMENT '읽은 시각', | ||
| sent tinyint(1) NOT NULL DEFAULT 0 COMMENT '발송 여부', | ||
| sent_at datetime NULL COMMENT '발송 시각', |
There was a problem hiding this comment.
notification 테이블에서 created_at과 read_at은 datetime(6)으로 정의되어 있는데, sent_at은 datetime으로 되어 있어 정밀도가 다릅니다. 스키마의 일관성을 위해 sent_at도 datetime(6)으로 맞추는 것이 좋겠습니다.
sent_at datetime(6) NULL COMMENT '발송 시각',
Comment on lines
+44
to
+45
| var room = roomRepository.findByUuid(roomUuid).orElse(null); | ||
| var roomTitle = room != null ? room.getTitle() : ""; |
There was a problem hiding this comment.
roomRepository.findByUuid(roomUuid).orElse(null)을 사용하여 방을 찾지 못했을 때 null을 반환하고 roomTitle을 빈 문자열로 처리하고 있습니다. TimeBlockReplaceEvent가 발생했다면 해당 방은 반드시 존재해야 합니다. 방을 찾지 못하는 경우는 예외적인 상황이므로, null을 반환하고 조용히 넘어가는 대신 orElseThrow()를 사용하여 예외를 발생시키거나 명확한 에러 로그를 남겨서 문제를 빠르게 인지할 수 있도록 하는 것이 더 안전한 방법입니다. 코드를 한 줄로 더 간결하게 만들 수도 있습니다.
Suggested change
| var room = roomRepository.findByUuid(roomUuid).orElse(null); | |
| var roomTitle = room != null ? room.getTitle() : ""; | |
| var roomTitle = roomRepository.findByUuid(roomUuid) | |
| .map(Room::getTitle) | |
| .orElseThrow(() -> new IllegalArgumentException("Room not found for uuid: " + roomUuid)); |
| if (deviceTokenQueryRepository.existsByToken(command.token())) { | ||
| return false; | ||
| } | ||
| var deviceToken = new DeviceToken(command.token(), command.userId()); |
There was a problem hiding this comment.
DeviceToken 엔티티에 정적 팩토리 메서드를 도입하는 것을 제안했습니다. 해당 변경이 적용되면 이 부분도 new DeviceToken(...) 대신 정적 팩토리 메서드를 사용하도록 수정해야 합니다.
Suggested change
| var deviceToken = new DeviceToken(command.token(), command.userId()); | |
| var deviceToken = DeviceToken.of(command.token(), command.userId()); |
Comment on lines
+42
to
+51
| public DeviceToken(String token, Long userId) { | ||
| if (token == null || token.isBlank()) { | ||
| throw new IllegalArgumentException("토큰은 빈 값일 수 없습니다."); | ||
| } | ||
| if (userId == null) { | ||
| throw new IllegalArgumentException("userId는 null일 수 없습니다."); | ||
| } | ||
| this.token = token; | ||
| this.userId = userId; | ||
| } |
There was a problem hiding this comment.
.claude/CLAUDE.md에 새로 추가된 가이드라인에 따르면 엔티티 생성 시 생성자 대신 정적 팩토리 메서드(of, from)를 사용하고 생성자는 private으로 선언하기로 했습니다. DeviceToken 엔티티도 이 컨벤션을 따르도록 수정하는 것이 좋겠습니다.
Suggested change
| public DeviceToken(String token, Long userId) { | |
| if (token == null || token.isBlank()) { | |
| throw new IllegalArgumentException("토큰은 빈 값일 수 없습니다."); | |
| } | |
| if (userId == null) { | |
| throw new IllegalArgumentException("userId는 null일 수 없습니다."); | |
| } | |
| this.token = token; | |
| this.userId = userId; | |
| } | |
| private DeviceToken(String token, Long userId) { | |
| if (token == null || token.isBlank()) { | |
| throw new IllegalArgumentException("토큰은 빈 값일 수 없습니다."); | |
| } | |
| if (userId == null) { | |
| throw new IllegalArgumentException("userId는 null일 수 없습니다."); | |
| } | |
| this.token = token; | |
| this.userId = userId; | |
| } | |
| public static DeviceToken of(String token, Long userId) { | |
| return new DeviceToken(token, userId); | |
| } |
- 시간 블록 변경 알림 기능 주석 처리 - TODO 주석 추가 (알림 기능 임시 비활성화 목적 명시)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
src/main/java/com/dnd/modutime/config/AsyncConfig.java (1)
6-9: 커스텀TaskExecutor빈 추가를 권장합니다.현재 설정은 Spring의 기본
SimpleAsyncTaskExecutor를 사용하게 되어,@Async호출마다 새 스레드가 생성됩니다. 알림 발송 볼륨이 증가하면 무제한 스레드 생성으로 인한 리소스 고갈 위험이 있습니다.♻️ ThreadPoolTaskExecutor 설정 추가 제안
package com.dnd.modutime.config; +import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.scheduling.annotation.EnableAsync; +import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; + +import java.util.concurrent.Executor; `@EnableAsync` `@Configuration` public class AsyncConfig { + + `@Bean`(name = "notificationExecutor") + public Executor notificationExecutor() { + ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); + executor.setCorePoolSize(2); + executor.setMaxPoolSize(5); + executor.setQueueCapacity(100); + executor.setThreadNamePrefix("notification-"); + executor.initialize(); + return executor; + } }그리고
NotificationEventHandler의@Async어노테이션에 executor 이름을 지정:`@Async`("notificationExecutor") `@TransactionalEventListener`(phase = TransactionPhase.AFTER_COMMIT) public void handle(TimeBlockReplaceEvent event) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/modutime/config/AsyncConfig.java` around lines 6 - 9, Add a bounded ThreadPoolTaskExecutor bean in AsyncConfig to replace Spring's unbounded SimpleAsyncTaskExecutor: define a bean method (e.g., public TaskExecutor notificationExecutor()) inside the AsyncConfig class that returns a configured ThreadPoolTaskExecutor with sensible corePoolSize, maxPoolSize, queueCapacity and threadNamePrefix, and mark it as `@Bean`; then update NotificationEventHandler.handle by specifying `@Async`("notificationExecutor") on the method so `@Async` uses the new executor instead of creating unbounded threads.src/main/java/com/dnd/modutime/core/notification/domain/NotificationType.java (1)
3-4: 열거형 값은 지역화 문구 대신 안정적인 코드로 분리해 주세요.
NotificationType가 응답 DTO까지 흘러가면 이 값이 그대로 API/저장 계약이 됩니다. 지금처럼 한국어 식별자를 상수로 두면 문구 수정이 곧 breaking change가 되니,AVAILABILITY_REGISTERED같은 고정 코드로 두고 표시 문구는 별도 매핑으로 분리하는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/modutime/core/notification/domain/NotificationType.java` around lines 3 - 4, The enum NotificationType currently uses a localized identifier (가용시간_등록); change it to a stable code-style constant (e.g., AVAILABILITY_REGISTERED) and move any display/localized text into a separate mapping layer (e.g., a localization map or method that maps NotificationType to display strings). Update usages of NotificationType (constructor, serializers, DTOs) to rely on the new enum name and ensure API/serialization returns the enum code while presentation layers call the new mapping to get the Korean label.src/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationJpaQueryRepository.java (1)
20-22:@Modifying에clearAutomatically = true추가 권장벌크 UPDATE 쿼리 실행 후 영속성 컨텍스트가 자동으로 클리어되지 않으면, 같은 트랜잭션 내에서 조회 시 이전 상태의 캐시된 엔티티가 반환될 수 있습니다.
♻️ 영속성 컨텍스트 동기화 제안
-@Modifying +@Modifying(clearAutomatically = true) `@Query`("UPDATE Notification n SET n.read = true, n.readAt = CURRENT_TIMESTAMP WHERE n.recipientId = :recipientId AND n.read = false") void markAllAsReadByRecipientId(`@Param`("recipientId") Long recipientId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationJpaQueryRepository.java` around lines 20 - 22, The bulk UPDATE method markAllAsReadByRecipientId in NotificationJpaQueryRepository uses `@Modifying` but doesn't clear the persistence context, so entities cached in the same transaction may be stale; update the `@Modifying` annotation on markAllAsReadByRecipientId to include clearAutomatically = true (i.e., `@Modifying`(clearAutomatically = true)) so the persistence context is cleared automatically after the bulk update.src/main/java/com/dnd/modutime/core/notification/domain/NotificationSendResult.java (1)
12-16: 불변성 보장을 위한 방어적 복사 권장
failedTokens리스트가 외부에서 전달된 참조를 그대로 저장하므로 외부에서 수정 시 내부 상태가 변경될 수 있습니다. 값 객체의 불변성을 보장하려면 방어적 복사를 권장합니다.♻️ 불변성 보장 제안
private NotificationSendResult(int successCount, int failureCount, List<String> failedTokens) { this.successCount = successCount; this.failureCount = failureCount; - this.failedTokens = failedTokens; + this.failedTokens = failedTokens != null ? List.copyOf(failedTokens) : Collections.emptyList(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/modutime/core/notification/domain/NotificationSendResult.java` around lines 12 - 16, The constructor in NotificationSendResult currently stores the passed List<String> failedTokens by reference, risking external mutation; update the constructor (NotificationSendResult(int successCount, int failureCount, List<String> failedTokens)) to defensively copy and/or wrap the list (e.g., assign Collections.unmodifiableList(new ArrayList<>(Optional.ofNullable(failedTokens).orElse(Collections.emptyList())))) so the internal failedTokens field is immutable and null-safe.src/main/java/com/dnd/modutime/core/notification/domain/Notification.java (1)
85-88:markAsSent()에 멱등성 체크 추가 권장
markAsRead()는 이미 읽음 상태인 경우 재처리를 방지하는 체크가 있지만,markAsSent()는 동일한 보호가 없습니다. 중복 호출 시sentAt타임스탬프가 덮어쓰여질 수 있습니다.♻️ 멱등성 체크 추가 제안
public void markAsSent(LocalDateTime now) { + if (!this.sent) { this.sent = true; this.sentAt = now; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/modutime/core/notification/domain/Notification.java` around lines 85 - 88, The markAsSent(LocalDateTime now) method lacks an idempotency check and can overwrite sentAt on repeated calls; update Notification.markAsSent to first check the sent flag (similar to markAsRead) and return immediately if already sent, otherwise set sent = true and sentAt = now so sentAt is only set on the first transition to sent.src/main/java/com/dnd/modutime/core/notification/controller/DeviceTokenController.java (1)
36-40: DELETE 요청 본문의 호환성 문제를 개선하세요.
DELETE /api/v1/device-tokens가 요청 본문에만 의존하면 중간 프록시, 게이트웨이, WAF 같은 인프라 계층에서 본문이 제거되어 요청이 실패할 수 있습니다 (RFC 9110에서 DELETE 본문의 "일반적으로 정의된 의미가 없음"을 경고). 다음 중 하나로 개선하세요:
DELETE /api/v1/device-tokens/{token}– 토큰을 경로 매개변수로 이동DELETE /api/v1/device-tokens?token=xxx– 토큰을 쿼리 매개변수로 이동POST /api/v1/device-tokens:unregister– 명시적으로 요청 본문 의미를 정의하는 POST 엔드포인트 사용🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/modutime/core/notification/controller/DeviceTokenController.java` around lines 36 - 40, Change the DELETE endpoint to avoid relying on request body: update the mapping on the unregister method from `@DeleteMapping`("/api/v1/device-tokens") to include the token as a path (e.g. `@DeleteMapping`("/api/v1/device-tokens/{token}")), remove the `@RequestBody` DeviceTokenRequest parameter, add a `@PathVariable` String token parameter, and call deviceTokenService.unregister(token, oAuth2User.user().getId()) inside unregister; adjust any callers, tests and API docs (or alternatively use a query param or a POST /device-tokens:unregister if you prefer) so the endpoint no longer depends on a DELETE body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/CLAUDE.md:
- Around line 107-111: The docs require making constructors private and using
static factory methods (`from()` for single-param, `of()` for multiple) but many
domain classes still expose public constructors (e.g., Participant.java exposes
a public constructor while also providing of()), and 16+ domain classes lack
factory methods; update the classes to match the guideline: change public
constructors to private, add appropriate static factory methods (`from`/`of`)
for each domain entity that currently lacks them, and for classes like
Participant replace or deprecate the public constructor in favor of its existing
`of()` method (or forward the factory to the private constructor) so all domain
classes consistently use the static factory pattern.
In `@build.gradle`:
- Around line 47-48: The Gradle dependency currently pins the Firebase Admin SDK
to 'com.google.firebase:firebase-admin:9.2.0'; update that implementation
coordinate to 'com.google.firebase:firebase-admin:9.8.0' to use the recommended
release, then re-sync/refresh Gradle (and run a build) to ensure the new
artifact is resolved and tests compile against the updated SDK.
In
`@src/main/java/com/dnd/modutime/core/notification/application/DeviceTokenService.java`:
- Around line 23-29: DeviceTokenService.register에서
deviceTokenQueryRepository.existsByToken(...) 후 save하는 현재 흐름은 동시성 예외와 다른 사용자로의
재등록 누락 문제를 일으키므로, register(DeviceTokenRegisterCommand) 내부를 트랜잭션으로 만든
뒤(command.token() 사용) deviceTokenQueryRepository.findByToken(token)로 조회하고, 조회
결과가 없으면 새 DeviceToken(command.token(), command.userId())를 저장하고 조회 결과가 있으면 조회된
DeviceToken의 소유자(userId)와 command.userId()를 비교하여 같으면 no-op(기존 동작처럼 false 반환 또는
명세에 맞게 처리), 다르면 DeviceToken#rebindTo(command.userId()) 또는 setUserId(...)로 소유자를
갱신하고 deviceTokenRepository.save(updated)로 재저장하도록 변경해 동작을 원자적으로 처리하고 동시성 문제를
방지하세요; 필요하면 findByToken을 잠금 조회(findByTokenForUpdate)나 `@Transactional로` 감싸세요.
In
`@src/main/java/com/dnd/modutime/core/notification/application/NotificationEventHandler.java`:
- Around line 28-30: The warning log in NotificationEventHandler currently logs
participant name and only e.getMessage(); remove the participant identifier from
the log and change the log.warn call to pass the exception object as the final
argument so the full stack trace is recorded (i.e., stop using
event.getParticipantName() in the message and replace e.getMessage() with the
exception variable e), updating the log message parameters that include
event.getRoomUuid() accordingly to ensure safe, useful logging.
In
`@src/main/java/com/dnd/modutime/core/notification/application/NotificationProcessor.java`:
- Around line 42-45: The code flattens device tokens
(deviceTokenQueryRepository.findByUserIdIn -> DeviceToken::getToken) and then
uses a single hasSuccess() to mark all notifications as sent, which incorrectly
marks recipients as sent if any token succeeded; instead preserve
recipient->tokens grouping (e.g., group device tokens by DeviceToken::getUserId)
and, when evaluating send results, compute success per recipient (check
hasSuccess() for that recipient's token set) and only call markAsSent on the
Notification objects corresponding to recipients that had at least one
successful token; update the logic around hasSuccess(), notifications
processing, and markAsSent to use the per-recipient success decision.
In
`@src/main/java/com/dnd/modutime/core/notification/application/NotificationQueryService.java`:
- Around line 46-49: The current flow in NotificationQueryService exposes
whether a notification exists vs. is owned by the user by first calling
notificationQueryRepository.findById(notificationId) and then checking recipient
ownership; change this to a single repository lookup that includes ownership
(e.g., notificationQueryRepository.findByIdAndRecipientId(notificationId,
userId)) and throw the same generic exception (e.g., "존재하지 않는 알림입니다.") when the
combined lookup returns empty so existence and ownership are not
distinguishable.
In `@src/main/java/com/dnd/modutime/infrastructure/fcm/FcmConfig.java`:
- Around line 24-28: The FileInputStream created for serviceAccountPath in
FcmConfig (inside the constructor/initializer that accepts
DeviceTokenRepository) is not closed; wrap the FileInputStream creation in a
try-with-resources block and pass that stream into
GoogleCredentials.fromStream(...) so the stream is auto-closed, then build the
FirebaseOptions as before (adjust exception handling if needed). Ensure the
variable names (serviceAccountPath, serviceAccount/FileInputStream) and the
FirebaseOptions.builder()/GoogleCredentials.fromStream(...) call are updated
accordingly.
In
`@src/main/java/com/dnd/modutime/infrastructure/fcm/FcmNotificationSender.java`:
- Around line 70-80: The FcmNotificationSender currently logs raw device tokens
in the two log.warn calls; instead mask or hash tokens before logging to avoid
exposing identifiers. Locate the log statements inside FcmNotificationSender
that reference tokens.get(i) and invalidToken (and the
deviceTokenRepository.deleteByToken call) and replace logging of the plain token
with a masked representation (e.g., first 4 + "..." + last 4) or a deterministic
hash (SHA-256) stored in a local variable like maskedToken, then log maskedToken
along with the existing exception.getMessage()/errorCode; keep deleteByToken
using the real token but never log it in plain form. Ensure the masking/hashing
logic is implemented once and reused for both logging sites.
- Around line 26-34: The send method currently ignores the title/body parameters
when building the MulticastMessage; update the MulticastMessage.builder() usage
in FcmNotificationSender.send(List<String> tokens, String title, String body,
Map<String, String> data) to include a Notification payload (e.g. call
.setNotification(Notification.builder().setTitle(title).setBody(body).build()) )
so the title/body are sent to FCM; keep the existing .putAllData(data) and
.addAllTokens(tokens) and handle null/empty title or body as appropriate (skip
or set empty strings) before building the MulticastMessage and returning
NotificationSendResult.
In
`@src/main/java/com/dnd/modutime/infrastructure/fcm/NoOpNotificationSender.java`:
- Around line 14-16: In NoOpNotificationSender.send, avoid logging sensitive
text: remove title and body from the debug log and only log non-identifying
metadata (e.g., tokens.size()); update the log.debug call in the send method to
not interpolate title or body, leaving the method behavior returning
NotificationSendResult.success(tokens.size()) unchanged so only the count or
other safe info is logged.
In
`@src/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationQueryRepositoryImpl.java`:
- Around line 22-24: The method findByRecipientIdOrderByCreatedAtDesc currently
treats the second/third parameters as offset/limit but computes page as offset /
limit (and will throw on limit==0); change the signature to (Long recipientId,
int page, int size) to reflect page/size semantics, replace
PageRequest.of(offset / limit, limit) with PageRequest.of(page, size), and call
jpaQueryRepository.findByRecipientIdOrderByCreatedAtDesc(recipientId, pageable);
if you truly need offset semantics instead, validate size>0 and implement a
proper offset-based query (or use a repository method that supports offset)
rather than dividing offset by limit.
In
`@src/test/java/com/dnd/modutime/controller/notification/NotificationControllerDocsTest.java`:
- Around line 135-145: The generated API docs are missing the paging query
params; update the MockMvcRestDocumentationWrapper call that builds the
ResourceSnippetParameters (inside ResourceDocumentation.resource) to include the
requestParameters by adding .requestParameters(queryParameters) to the
ResourceSnippetParameters.builder() chain so the page/size params
(queryParameters) are included in the OpenAPI output; locate the call that
constructs ResourceSnippetParameters in NotificationControllerDocsTest and
append .requestParameters(queryParameters) before .build().
---
Nitpick comments:
In `@src/main/java/com/dnd/modutime/config/AsyncConfig.java`:
- Around line 6-9: Add a bounded ThreadPoolTaskExecutor bean in AsyncConfig to
replace Spring's unbounded SimpleAsyncTaskExecutor: define a bean method (e.g.,
public TaskExecutor notificationExecutor()) inside the AsyncConfig class that
returns a configured ThreadPoolTaskExecutor with sensible corePoolSize,
maxPoolSize, queueCapacity and threadNamePrefix, and mark it as `@Bean`; then
update NotificationEventHandler.handle by specifying
`@Async`("notificationExecutor") on the method so `@Async` uses the new executor
instead of creating unbounded threads.
In
`@src/main/java/com/dnd/modutime/core/notification/controller/DeviceTokenController.java`:
- Around line 36-40: Change the DELETE endpoint to avoid relying on request
body: update the mapping on the unregister method from
`@DeleteMapping`("/api/v1/device-tokens") to include the token as a path (e.g.
`@DeleteMapping`("/api/v1/device-tokens/{token}")), remove the `@RequestBody`
DeviceTokenRequest parameter, add a `@PathVariable` String token parameter, and
call deviceTokenService.unregister(token, oAuth2User.user().getId()) inside
unregister; adjust any callers, tests and API docs (or alternatively use a query
param or a POST /device-tokens:unregister if you prefer) so the endpoint no
longer depends on a DELETE body.
In `@src/main/java/com/dnd/modutime/core/notification/domain/Notification.java`:
- Around line 85-88: The markAsSent(LocalDateTime now) method lacks an
idempotency check and can overwrite sentAt on repeated calls; update
Notification.markAsSent to first check the sent flag (similar to markAsRead) and
return immediately if already sent, otherwise set sent = true and sentAt = now
so sentAt is only set on the first transition to sent.
In
`@src/main/java/com/dnd/modutime/core/notification/domain/NotificationSendResult.java`:
- Around line 12-16: The constructor in NotificationSendResult currently stores
the passed List<String> failedTokens by reference, risking external mutation;
update the constructor (NotificationSendResult(int successCount, int
failureCount, List<String> failedTokens)) to defensively copy and/or wrap the
list (e.g., assign Collections.unmodifiableList(new
ArrayList<>(Optional.ofNullable(failedTokens).orElse(Collections.emptyList()))))
so the internal failedTokens field is immutable and null-safe.
In
`@src/main/java/com/dnd/modutime/core/notification/domain/NotificationType.java`:
- Around line 3-4: The enum NotificationType currently uses a localized
identifier (가용시간_등록); change it to a stable code-style constant (e.g.,
AVAILABILITY_REGISTERED) and move any display/localized text into a separate
mapping layer (e.g., a localization map or method that maps NotificationType to
display strings). Update usages of NotificationType (constructor, serializers,
DTOs) to rely on the new enum name and ensure API/serialization returns the enum
code while presentation layers call the new mapping to get the Korean label.
In
`@src/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationJpaQueryRepository.java`:
- Around line 20-22: The bulk UPDATE method markAllAsReadByRecipientId in
NotificationJpaQueryRepository uses `@Modifying` but doesn't clear the persistence
context, so entities cached in the same transaction may be stale; update the
`@Modifying` annotation on markAllAsReadByRecipientId to include
clearAutomatically = true (i.e., `@Modifying`(clearAutomatically = true)) so the
persistence context is cleared automatically after the bulk update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 778c739a-4ec0-4454-a9a3-479504ad9c2a
📒 Files selected for processing (43)
.claude/CLAUDE.md.claude/plans/notification-system.mdbuild.gradledb/migrations/V20260311_CREATE_NOTIFICATION.sqlsrc/main/java/com/dnd/modutime/config/AsyncConfig.javasrc/main/java/com/dnd/modutime/core/auth/oauth/OAuth2SecurityConfig.javasrc/main/java/com/dnd/modutime/core/notification/application/DeviceTokenService.javasrc/main/java/com/dnd/modutime/core/notification/application/NotificationEventHandler.javasrc/main/java/com/dnd/modutime/core/notification/application/NotificationProcessor.javasrc/main/java/com/dnd/modutime/core/notification/application/NotificationQueryService.javasrc/main/java/com/dnd/modutime/core/notification/application/NotificationService.javasrc/main/java/com/dnd/modutime/core/notification/application/command/DeviceTokenRegisterCommand.javasrc/main/java/com/dnd/modutime/core/notification/application/response/NotificationResponse.javasrc/main/java/com/dnd/modutime/core/notification/application/response/UnreadCountResponse.javasrc/main/java/com/dnd/modutime/core/notification/controller/DeviceTokenController.javasrc/main/java/com/dnd/modutime/core/notification/controller/NotificationController.javasrc/main/java/com/dnd/modutime/core/notification/controller/dto/DeviceTokenRequest.javasrc/main/java/com/dnd/modutime/core/notification/domain/DeviceToken.javasrc/main/java/com/dnd/modutime/core/notification/domain/DeviceTokenQueryRepository.javasrc/main/java/com/dnd/modutime/core/notification/domain/DeviceTokenRepository.javasrc/main/java/com/dnd/modutime/core/notification/domain/MapToJsonConverter.javasrc/main/java/com/dnd/modutime/core/notification/domain/Notification.javasrc/main/java/com/dnd/modutime/core/notification/domain/NotificationQueryRepository.javasrc/main/java/com/dnd/modutime/core/notification/domain/NotificationRepository.javasrc/main/java/com/dnd/modutime/core/notification/domain/NotificationSendResult.javasrc/main/java/com/dnd/modutime/core/notification/domain/NotificationSender.javasrc/main/java/com/dnd/modutime/core/notification/domain/NotificationType.javasrc/main/java/com/dnd/modutime/infrastructure/fcm/FcmConfig.javasrc/main/java/com/dnd/modutime/infrastructure/fcm/FcmNotificationSender.javasrc/main/java/com/dnd/modutime/infrastructure/fcm/NoOpNotificationSender.javasrc/main/java/com/dnd/modutime/infrastructure/persistence/notification/DeviceTokenJpaQueryRepository.javasrc/main/java/com/dnd/modutime/infrastructure/persistence/notification/DeviceTokenJpaRepository.javasrc/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationJpaQueryRepository.javasrc/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationJpaRepository.javasrc/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationQueryRepositoryImpl.javasrc/main/resources/application-fcm.yamlsrc/main/resources/application.yamlsrc/test/java/com/dnd/modutime/controller/notification/DeviceTokenControllerDocsTest.javasrc/test/java/com/dnd/modutime/controller/notification/NotificationControllerDocsTest.javasrc/test/java/com/dnd/modutime/core/notification/domain/DeviceTokenTest.javasrc/test/java/com/dnd/modutime/core/notification/domain/NotificationTest.javasrc/test/java/com/dnd/modutime/core/notification/integration/NotificationQueryServiceTest.javasrc/test/java/com/dnd/modutime/core/notification/integration/NotificationServiceTest.java
💤 Files with no reviewable changes (1)
- .claude/plans/notification-system.md
Comment on lines
+107
to
+111
| **Static Factory Method:** | ||
| - 엔티티/도메인 객체 생성 시 생성자 대신 정적 팩터리 메서드를 사용한다 | ||
| - `from()` - 파라미터가 1개일 때 | ||
| - `of()` - 파라미터가 여러 개일 때 | ||
| - 생성자는 `private`으로 선언한다 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 도메인 클래스에서 public 생성자 사용 현황 확인
echo "=== public 생성자를 사용하는 도메인 클래스 ==="
rg -n "public\s+\w+\s*\(" --type=java -g '**/domain/*.java' | head -30Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 2066
🏁 Script executed:
cat .claude/CLAUDE.md | head -120 | tail -30Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 1286
🏁 Script executed:
# Search for existing static factory methods (from/of methods) in domain classes
rg -n "public static.*from\(|public static.*of\(" --type=java -g '**/domain/*.java'Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 491
🏁 Script executed:
# Check Participant.java for both constructor and factory method
sed -n '53,75p' src/main/java/com/dnd/modutime/core/participant/domain/Participant.javaRepository: dnd-side-project/dnd-8th-5-backend
Length of output: 815
🏁 Script executed:
# Get a summary of which classes have factory methods vs public constructors
echo "=== Classes with static factory methods (from/of) ==="
rg -B 5 "public static.*from\(|public static.*of\(" --type=java -g '**/domain/*.java' | grep -E "class |public static" | head -20Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 536
정적 팩터리 메서드 패턴 구현의 불일치를 명확히 해주세요.
문서는 생성자를 private으로 선언하고 정적 팩터리 메서드를 사용할 것을 권장하지만, 현재 도메인 클래스들은 여전히 public 생성자를 노출하고 있습니다. 더 구체적으로, Participant.java처럼 이미 of() 팩터리 메서드를 가진 클래스도 public 생성자를 유지하고 있으며, 대다수의 도메인 클래스(16개+)는 아직 팩터리 메서드를 구현하지 않았습니다. 이 가이드라인이 향후 새로운 코드에만 적용되는 관례인지, 아니면 기존 코드도 리팩터링할 계획이 있는지 명확히 해주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/CLAUDE.md around lines 107 - 111, The docs require making
constructors private and using static factory methods (`from()` for
single-param, `of()` for multiple) but many domain classes still expose public
constructors (e.g., Participant.java exposes a public constructor while also
providing of()), and 16+ domain classes lack factory methods; update the classes
to match the guideline: change public constructors to private, add appropriate
static factory methods (`from`/`of`) for each domain entity that currently lacks
them, and for classes like Participant replace or deprecate the public
constructor in favor of its existing `of()` method (or forward the factory to
the private constructor) so all domain classes consistently use the static
factory pattern.
Comment on lines
+22
to
+24
| public List<Notification> findByRecipientIdOrderByCreatedAtDesc(Long recipientId, int offset, int limit) { | ||
| var pageable = PageRequest.of(offset / limit, limit); | ||
| return jpaQueryRepository.findByRecipientIdOrderByCreatedAtDesc(recipientId, pageable); |
There was a problem hiding this comment.
offset/limit 계약을 실제로 지키지 못합니다.
Line 23은 offset / limit로 페이지 번호를 역산해서 offset=25, limit=20이면 25번째가 아니라 20번째부터 조회합니다. 또한 limit=0이면 여기서 바로 ArithmeticException이 납니다. 현재 호출부가 page/size만 넘기는 구조라면 시그니처도 그 의미로 맞추고, 정말 offset이 필요하면 별도 쿼리로 처리하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationQueryRepositoryImpl.java`
around lines 22 - 24, The method findByRecipientIdOrderByCreatedAtDesc currently
treats the second/third parameters as offset/limit but computes page as offset /
limit (and will throw on limit==0); change the signature to (Long recipientId,
int page, int size) to reflect page/size semantics, replace
PageRequest.of(offset / limit, limit) with PageRequest.of(page, size), and call
jpaQueryRepository.findByRecipientIdOrderByCreatedAtDesc(recipientId, pageable);
if you truly need offset semantics instead, validate size>0 and implement a
proper offset-based query (or use a repository method that supports offset)
rather than dividing offset by limit.
Comment on lines
+135
to
+145
| MockMvcRestDocumentationWrapper.document( | ||
| operationIdentifier, | ||
| DocumentUtils.getDocumentRequest(), | ||
| DocumentUtils.getDocumentResponse(), | ||
| ResourceDocumentation.resource( | ||
| ResourceSnippetParameters.builder() | ||
| .description("알림 목록 조회 API") | ||
| .tag("Notification") | ||
| .responseFields(responseFields) | ||
| .build()) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "NotificationControllerDocsTest.java" -type fRepository: dnd-side-project/dnd-8th-5-backend
Length of output: 170
🏁 Script executed:
cd . && git ls-files | grep -i notification | head -20Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 1776
🏁 Script executed:
fd "NotificationControllerDocsTest" --type fRepository: dnd-side-project/dnd-8th-5-backend
Length of output: 168
🏁 Script executed:
wc -l ./src/test/java/com/dnd/modutime/controller/notification/NotificationControllerDocsTest.javaRepository: dnd-side-project/dnd-8th-5-backend
Length of output: 174
🏁 Script executed:
cat -n ./src/test/java/com/dnd/modutime/controller/notification/NotificationControllerDocsTest.java | head -200Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 11876
🏁 Script executed:
cat -n ./src/test/java/com/dnd/modutime/controller/notification/NotificationControllerDocsTest.java | sed -n '200,289p'Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 5031
🏁 Script executed:
rg "ResourceSnippetParameters" --type java -A 10 | head -100Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 12813
🏁 Script executed:
rg "queryParameters" --type java -B 2 -A 2 | grep -A 5 "ResourceSnippetParameters"Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 60
🏁 Script executed:
web_search
restdocs-apispec ResourceSnippetParameters queryParameters methodRepository: dnd-side-project/dnd-8th-5-backend
Length of output: 183
🌐 Web query:
restdocs-apispec ResourceSnippetParameters queryParameters documentation
💡 Result:
To document query parameters with restdocs-api-spec, you generally document them using Spring REST Docs’ queryParameters(...) snippet, and restdocs-api-spec will pick them up into the generated resource.json / OpenAPI output.
Spring REST Docs (3.x/4.x): use queryParameters(...)
import static org.springframework.restdocs.request.RequestDocumentation.*;
andDo(document("users",
queryParameters(
parameterWithName("page").description("The page to retrieve"),
parameterWithName("per_page").description("Entries per page")
)));Spring REST Docs documents query params via RequestDocumentation.queryParameters(...) (separate from form params). [1]
restdocs-api-spec integration (wrapper approach)
If you use the restdocs-api-spec wrappers (e.g., WebTestClientRestDocumentationWrapper / MockMvcRestDocumentationWrapper), include the parameters snippet in the document(...) call; the wrapper adds the ResourceSnippet and incorporates the parameter descriptors. The project README shows this pattern (older example uses requestParameters(...)). [2]
Version note (why you may see requestParameters(...) in examples)
Spring REST Docs 3.0 split the old “request parameters” snippet into query vs form parameters; restdocs-api-spec calls this out in its compatibility notes. So on newer Spring REST Docs, prefer queryParameters(...) for URL query params. [2]
Sources:
[1] Spring REST Docs reference – “Query Parameters” section (uses queryParameters(...)) (docs.spring.io)
[2] ePages restdocs-api-spec README (usage + version compatibility notes) (github.com)
Citations:
- 1: https://docs.spring.io/spring-restdocs/docs/current/reference/htmlsingle/
- 2: https://github.com/ePages-de/restdocs-api-spec
🏁 Script executed:
rg "queryParameters\(" src/test/java --type java -B 2 -A 2 | head -50Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 60
🏁 Script executed:
fd "pom.xml\|build.gradle\|build.gradle.kts" -type fRepository: dnd-side-project/dnd-8th-5-backend
Length of output: 248
🏁 Script executed:
rg "restdocs-apispec\|restdocs.apispec" --type xml -B 2 -A 2 | head -50Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 60
🏁 Script executed:
find . -name "pom.xml" -o -name "build.gradle*" | head -5Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 92
🏁 Script executed:
cat ./pom.xml | head -50Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 119
🏁 Script executed:
cat ./build.gradle | grep -A 5 -B 5 "restdocs"Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 1292
🏁 Script executed:
grep "springRestDocsApiSpecVersion" ./build.gradleRepository: dnd-side-project/dnd-8th-5-backend
Length of output: 299
🏁 Script executed:
rg "queryParameters\|pathParameters" src/test/java --type java -B 3 -A 3 | grep -A 10 "ResourceSnippetParameters"Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 60
🏁 Script executed:
rg "ResourceSnippetParameters" src/test/java --type java -B 5 -A 10 | head -100Repository: dnd-side-project/dnd-8th-5-backend
Length of output: 13952
ResourceSnippetParameters에 쿼리 파라미터 추가 필요
queryParameters 배열(페이지 번호, 크기)이 표준 REST Docs 문서화에는 포함되어 있지만, MockMvcRestDocumentationWrapper의 ResourceSnippetParameters에는 빠져 있습니다. 생성된 OpenAPI 스펙에 page/size 파라미터가 누락되지 않도록 .requestParameters(queryParameters)를 추가하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/dnd/modutime/controller/notification/NotificationControllerDocsTest.java`
around lines 135 - 145, The generated API docs are missing the paging query
params; update the MockMvcRestDocumentationWrapper call that builds the
ResourceSnippetParameters (inside ResourceDocumentation.resource) to include the
requestParameters by adding .requestParameters(queryParameters) to the
ResourceSnippetParameters.builder() chain so the page/size params
(queryParameters) are included in the OpenAPI output; locate the call that
constructs ResourceSnippetParameters in NotificationControllerDocsTest and
append .requestParameters(queryParameters) before .build().
기존에는 토큰을 flatten 후 단일 hasSuccess()로 전체 알림을 sent 처리하여, 일부 수신자의 토큰만 성공해도 모든 알림이 sent로 마킹되는 버그가 있었다. 토큰을 userId별로 그룹핑하고, 수신자별로 실패 토큰 여부를 확인하여 해당 수신자의 알림만 markAsSent 처리하도록 수정한다. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
existsByToken() + save() 사이의 TOCTOU 경합 조건을 제거하기 위해 findByToken()으로 변경한다. 동일 토큰이 다른 유저로 재등록될 경우 기존 토큰을 삭제하고 새 소유자로 재생성하는 로직을 추가한다. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
findById 후 소유자 체크하는 방식은 존재 여부와 소유권을 분리 노출하여 공격자가 알림 존재 여부를 유추할 수 있었다. findByIdAndRecipientId 단일 조회로 통합하여 정보 노출을 방지한다. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- FcmConfig: FileInputStream을 try-with-resources로 감싸 리소스 누수 수정 - FcmNotificationSender: FCM 토큰 로그 마스킹 처리 (앞4...뒤4) - FcmNotificationSender: MulticastMessage에 Notification payload 추가 - NoOpNotificationSender: 로그에서 title/body 제거 - NotificationEventHandler: 참여자 이름 로그 제거, exception 객체 직접 전달 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
무한 스레드 생성을 방지하기 위해 notificationExecutor 빈을 추가한다. (core=2, max=5, queue=100) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- DeviceToken: 정적 팩토리 메서드 of() 도입, 생성자 private - Notification.markAsSent(): 멱등성 체크 추가 - NotificationSendResult: failedTokens 방어적 복사 (List.copyOf) - NotificationService: orElse(null) → orElseThrow()로 명확한 에러 처리 - @Modifying(clearAutomatically=true) 추가 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DELETE 요청에서 request body 대신 query parameter로 토큰을 전달하도록 변경한다. REST 관례에 맞게 개선하고 API docs 테스트를 수정한다. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- NotificationType: 가용시간_등록 → AVAILABILITY_REGISTERED + displayName - Firebase Admin SDK: 9.2.0 → 9.8.0 업그레이드 - SDK 업그레이드로 인한 XML serialization 이슈에 Accept 헤더 추가 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/main/java/com/dnd/modutime/core/notification/application/DeviceTokenService.java (1)
22-34:⚠️ Potential issue | 🟠 Major신규 등록 경로는 아직 유니크 충돌에 취약합니다.
findByToken()뒤에 바로save()하는 구조라서, 같은 토큰으로 동시 요청 2건이 들어오면 둘 다 Line 33까지 내려와 한쪽이 unique constraint 예외로 끝날 수 있습니다. 지금 변경으로 “이미 있는 토큰” 케이스는 개선됐지만, “없는 토큰을 동시에 등록”하는 케이스는 아직 idempotent하지 않습니다. 토큰 기준 잠금 조회, DB upsert, 또는DataIntegrityViolationException재조회 중 하나는 필요해 보입니다.잠금/재시도 처리가 이미 들어가 있는지 먼저 확인해 주세요. 기대 결과는
findByToken경로에@Lock/pessimistic lock, upsert, 또는 unique 충돌 재처리 중 하나가 있어야 합니다.#!/bin/bash set -euo pipefail printf '\n# DeviceTokenService register flow\n' rg -n -C3 'class DeviceTokenService|findByToken\(|deleteByToken\(|save\(' src/main/java printf '\n# Repository/query definitions related to token lookup or locking\n' rg -n -C3 '@Lock|PESSIMISTIC|findByToken|deleteByToken|DataIntegrityViolationException|DuplicateKeyException|ConstraintViolationException' src/main/java🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/modutime/core/notification/application/DeviceTokenService.java` around lines 22 - 34, The register method in DeviceTokenService is vulnerable to race conditions because it does findByToken(...) then save(...) without protection; update register(DeviceTokenRegisterCommand) to make insertion idempotent by either using a database-level upsert or adding a retry-on-unique-constraint path: attempt to save(DeviceToken.of(...)) and if a DataIntegrityViolationException (or the DB-specific duplicate key exception) is thrown, re-query with deviceTokenQueryRepository.findByToken(command.token()) and return false if present (or reconcile userId if needed); alternatively, use a pessimistic lock on the token lookup (e.g., `@Lock/PESSIMISTIC` on the repository query) before deciding to insert to prevent concurrent inserts. Ensure you reference and modify DeviceTokenService.register, deviceTokenQueryRepository.findByToken, and deviceTokenRepository.save to implement the chosen strategy.src/test/java/com/dnd/modutime/controller/notification/NotificationControllerDocsTest.java (1)
140-145:⚠️ Potential issue | 🟡 MinorOpenAPI 스펙에
page/size가 빠집니다.
requestParameters(queryParameters)는 REST Docs 스니펫에만 적용되고,ResourceSnippetParameters에는 같은 descriptor가 없어 생성된 OpenAPI에는 쿼리 파라미터가 누락됩니다.restdocs-api-spec에서 OpenAPI에 쿼리 파라미터를 포함하려면 `ResourceSnippetParameters.requestParameters(...)` 설정이 필요한가?수정 예시
ResourceSnippetParameters.builder() .description("알림 목록 조회 API") .tag("Notification") + .requestParameters(queryParameters) .responseFields(responseFields) .build())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/modutime/controller/notification/NotificationControllerDocsTest.java` around lines 140 - 145, The OpenAPI output is missing the query params because ResourceSnippetParameters.builder() is not being given the request parameter descriptors; update the ResourceDocumentation.resource call that builds ResourceSnippetParameters (using ResourceSnippetParameters.builder()) to include .requestParameters(queryParameters) (the same queryDescriptors used for the REST Docs snippet) so page/size are included in the generated OpenAPI; ensure the queryParameters variable passed matches the descriptors used elsewhere in the test (e.g., the existing requestParameters(queryParameters) REST Docs invocation) and rebuild the ResourceSnippetParameters with that requestParameters(...) call.src/main/java/com/dnd/modutime/core/notification/application/NotificationProcessor.java (1)
55-64:⚠️ Potential issue | 🟠 Major수신자별 sent 판정이 여전히 너무 엄격합니다.
지금 로직은 같은 수신자의 토큰 중 하나라도 실패하면
sent=false로 남깁니다. 멀티디바이스 사용자에게 한 기기라도 전달됐으면 해당 알림은 sent로 기록해야 해서, 현재 조건이면 성공 이력이 과소 집계됩니다.수정 예시
var result = notificationSender.send(tokens, title, message, data); - var failedTokens = result.getFailedTokens(); + var failedTokenSet = result.getFailedTokens().stream() + .collect(Collectors.toSet()); var now = timeProvider.getCurrentLocalDateTime(); notifications.forEach(n -> { var userTokens = tokensByUserId.get(n.getRecipientId()); if (userTokens == null || userTokens.isEmpty()) { return; } - boolean allSucceeded = userTokens.stream().noneMatch(failedTokens::contains); - if (allSucceeded) { + boolean anySucceeded = userTokens.stream() + .anyMatch(token -> !failedTokenSet.contains(token)); + if (anySucceeded) { n.markAsSent(now); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/modutime/core/notification/application/NotificationProcessor.java` around lines 55 - 64, The current logic in NotificationProcessor marks a notification as sent only if none of a recipient's tokens appear in failedTokens, which incorrectly treats any single-device failure as overall failure; change the check inside notifications.forEach (where you fetch userTokens from tokensByUserId and call n.markAsSent(now)) to mark sent if at least one token succeeded — i.e., detect existence of a token NOT in failedTokens (use userTokens.stream().anyMatch(t -> !failedTokens.contains(t)) or equivalently negate allMatch) and call n.markAsSent(now) when that condition is true, leaving timeProvider.getCurrentLocalDateTime() and other flows untouched.
🧹 Nitpick comments (3)
src/test/java/com/dnd/modutime/core/notification/domain/NotificationTest.java (1)
53-68:markAsSent()멱등성도 테스트로 고정해 두는 편이 좋겠습니다.이번 PR에서 sent 처리도 재호출 방어가 들어갔는데, 현재 테스트는
markAsRead()만 최초 timestamp 보존을 검증합니다. 전송 재시도나 중복 이벤트에서sentAt이 덮어써지지 않는 케이스도 같이 묶어두면 회귀를 막기 좋습니다.테스트 추가 예시
`@Test` void 알림을_발송_완료_처리한다() { var notification = Notification.of( NotificationType.AVAILABILITY_REGISTERED, "가용시간 등록", @@ assertThat(notification.isSent()).isTrue(); assertThat(notification.getSentAt()).isEqualTo(now); } + + `@Test` + void 이미_발송한_알림을_다시_발송_완료_처리하면_변경되지_않는다() { + var notification = Notification.of( + NotificationType.AVAILABILITY_REGISTERED, + "가용시간 등록", + "테스트 메시지", + 1L, + Map.of() + ); + var firstSentTime = LocalDateTime.of(2026, 3, 11, 14, 0, 0); + var secondSentTime = LocalDateTime.of(2026, 3, 11, 15, 0, 0); + + notification.markAsSent(firstSentTime); + notification.markAsSent(secondSentTime); + + assertThat(notification.getSentAt()).isEqualTo(firstSentTime); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/modutime/core/notification/domain/NotificationTest.java` around lines 53 - 68, Add idempotency checks for markAsSent(): after creating the Notification via Notification.of(...) and calling notification.markAsSent(now) assert isSent() is true and getSentAt() equals now, then call notification.markAsSent(now.plusSeconds(10)) (or another timestamp) a second time and assert that isSent() remains true and getSentAt() still equals the original now (i.e., sentAt is not overwritten). Keep the assertions alongside the existing ones for markAsSent() to ensure re-call protection.src/main/java/com/dnd/modutime/config/AsyncConfig.java (1)
10-23: 이 executor는 아직 실제 알림 경로에 연결되지 않았습니다.지금 PR 기준으로는
src/main/java/com/dnd/modutime/core/notification/application/NotificationEventHandler.javaLine 20-32의@Async("notificationExecutor")가 주석 처리돼 있어서, 이 설정을 추가해도 알림 처리는 계속 동기 경로로 남습니다. 이번 변경의 목적이 비동기화라면 핸들러 연결까지 같이 마무리하거나, 아니면 실제 사용 시점에 맞춰 설정을 넣는 편이 낫습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/modutime/config/AsyncConfig.java` around lines 10 - 23, 현재 추가한 ThreadPoolTaskExecutor 빈(AsyncConfig.notificationExecutor)은 NotificationEventHandler의 `@Async`("notificationExecutor")가 주석 처리되어 있어 사용되지 않습니다; 비동기화 목적이라면 src/main/java/com/dnd/modutime/core/notification/application/NotificationEventHandler.java에서 해당 `@Async`("notificationExecutor") 어노테이션을 주석 해제해 핸들러(해당 메서드 또는 클래스)에 연결하거나, 아직 핸들러를 활성화할 준비가 안됐다면 AsyncConfig 클래스의 notificationExecutor 빈을 제거하거나 주석 처리해 실제 사용 시점에 추가되도록 정리하세요.src/main/java/com/dnd/modutime/core/notification/domain/Notification.java (1)
71-80:data맵은 방어 복사가 필요합니다.지금은 참조를 그대로 저장해서, 생성 경로가 같은
data객체를 여러Notification이 공유할 수 있습니다. 이후 한 곳에서라도 값을 바꾸면 다른 알림 payload까지 같이 오염됩니다.수정 예시
- this.data = data; + this.data = data == null ? null : Collections.unmodifiableMap(new HashMap<>(data));추가 import:
import java.util.Collections; import java.util.HashMap;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/modutime/core/notification/domain/Notification.java` around lines 71 - 80, 생성자 Notification(NotificationType type, String title, String message, Long recipientId, Map<String, String> data)에서 전달된 data를 그대로 할당하지 말고 방어 복사하여 내부 상태를 불변화하세요; 즉 Notification 클래스의 생성자에서 this.data = data == null ? Collections.emptyMap() : Collections.unmodifiableMap(new HashMap<>(data))처럼 복사 및 불변 래핑을 적용하고 필요 시 java.util.HashMap와 java.util.Collections를 import하여 외부에서 변경되어도 내부 payload가 오염되지 않도록 변경하세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/dnd/modutime/core/notification/domain/DeviceToken.java`:
- Around line 46-55: The constructor DeviceToken(String token, Long userId)
currently only checks null/blank and can allow tokens >512 chars which will
later cause DB constraint failures; update the validation in the DeviceToken
constructor to reject tokens longer than 512 characters (matching
db/migrations/V20260311_CREATE_NOTIFICATION.sql's token varchar(512)) by
throwing an IllegalArgumentException with a clear message (e.g., "토큰은 512자 이하여야
합니다."), keeping the existing null/blank and userId null checks and assigning to
the token and userId fields only after validation.
In
`@src/main/java/com/dnd/modutime/infrastructure/fcm/NoOpNotificationSender.java`:
- Around line 14-16: The NoOpNotificationSender.send currently returns
NotificationSendResult.success(tokens.size()), which causes
NotificationProcessor.markAsSent to record non-sent pushes as delivered; change
send in NoOpNotificationSender to return a non-success result (e.g.,
NotificationSendResult.empty() or a failure result) so messages are not marked
sent, and additionally register NoOpNotificationSender only for non-production
profiles (e.g., local/test) instead of a global bean when fcm.enabled is false;
update references in NoOpNotificationSender.send, NotificationSendResult usage,
and bean configuration to enforce profile-restricted instantiation.
---
Duplicate comments:
In
`@src/main/java/com/dnd/modutime/core/notification/application/DeviceTokenService.java`:
- Around line 22-34: The register method in DeviceTokenService is vulnerable to
race conditions because it does findByToken(...) then save(...) without
protection; update register(DeviceTokenRegisterCommand) to make insertion
idempotent by either using a database-level upsert or adding a
retry-on-unique-constraint path: attempt to save(DeviceToken.of(...)) and if a
DataIntegrityViolationException (or the DB-specific duplicate key exception) is
thrown, re-query with deviceTokenQueryRepository.findByToken(command.token())
and return false if present (or reconcile userId if needed); alternatively, use
a pessimistic lock on the token lookup (e.g., `@Lock/PESSIMISTIC` on the
repository query) before deciding to insert to prevent concurrent inserts.
Ensure you reference and modify DeviceTokenService.register,
deviceTokenQueryRepository.findByToken, and deviceTokenRepository.save to
implement the chosen strategy.
In
`@src/main/java/com/dnd/modutime/core/notification/application/NotificationProcessor.java`:
- Around line 55-64: The current logic in NotificationProcessor marks a
notification as sent only if none of a recipient's tokens appear in
failedTokens, which incorrectly treats any single-device failure as overall
failure; change the check inside notifications.forEach (where you fetch
userTokens from tokensByUserId and call n.markAsSent(now)) to mark sent if at
least one token succeeded — i.e., detect existence of a token NOT in
failedTokens (use userTokens.stream().anyMatch(t -> !failedTokens.contains(t))
or equivalently negate allMatch) and call n.markAsSent(now) when that condition
is true, leaving timeProvider.getCurrentLocalDateTime() and other flows
untouched.
In
`@src/test/java/com/dnd/modutime/controller/notification/NotificationControllerDocsTest.java`:
- Around line 140-145: The OpenAPI output is missing the query params because
ResourceSnippetParameters.builder() is not being given the request parameter
descriptors; update the ResourceDocumentation.resource call that builds
ResourceSnippetParameters (using ResourceSnippetParameters.builder()) to include
.requestParameters(queryParameters) (the same queryDescriptors used for the REST
Docs snippet) so page/size are included in the generated OpenAPI; ensure the
queryParameters variable passed matches the descriptors used elsewhere in the
test (e.g., the existing requestParameters(queryParameters) REST Docs
invocation) and rebuild the ResourceSnippetParameters with that
requestParameters(...) call.
---
Nitpick comments:
In `@src/main/java/com/dnd/modutime/config/AsyncConfig.java`:
- Around line 10-23: 현재 추가한 ThreadPoolTaskExecutor
빈(AsyncConfig.notificationExecutor)은 NotificationEventHandler의
`@Async`("notificationExecutor")가 주석 처리되어 있어 사용되지 않습니다; 비동기화 목적이라면
src/main/java/com/dnd/modutime/core/notification/application/NotificationEventHandler.java에서
해당 `@Async`("notificationExecutor") 어노테이션을 주석 해제해 핸들러(해당 메서드 또는 클래스)에 연결하거나, 아직
핸들러를 활성화할 준비가 안됐다면 AsyncConfig 클래스의 notificationExecutor 빈을 제거하거나 주석 처리해 실제 사용
시점에 추가되도록 정리하세요.
In `@src/main/java/com/dnd/modutime/core/notification/domain/Notification.java`:
- Around line 71-80: 생성자 Notification(NotificationType type, String title,
String message, Long recipientId, Map<String, String> data)에서 전달된 data를 그대로 할당하지
말고 방어 복사하여 내부 상태를 불변화하세요; 즉 Notification 클래스의 생성자에서 this.data = data == null ?
Collections.emptyMap() : Collections.unmodifiableMap(new HashMap<>(data))처럼 복사 및
불변 래핑을 적용하고 필요 시 java.util.HashMap와 java.util.Collections를 import하여 외부에서 변경되어도
내부 payload가 오염되지 않도록 변경하세요.
In
`@src/test/java/com/dnd/modutime/core/notification/domain/NotificationTest.java`:
- Around line 53-68: Add idempotency checks for markAsSent(): after creating the
Notification via Notification.of(...) and calling notification.markAsSent(now)
assert isSent() is true and getSentAt() equals now, then call
notification.markAsSent(now.plusSeconds(10)) (or another timestamp) a second
time and assert that isSent() remains true and getSentAt() still equals the
original now (i.e., sentAt is not overwritten). Keep the assertions alongside
the existing ones for markAsSent() to ensure re-call protection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 326e16d5-0ace-4b1b-b370-8e5345e03a1c
📒 Files selected for processing (24)
build.gradlesrc/main/java/com/dnd/modutime/config/AsyncConfig.javasrc/main/java/com/dnd/modutime/core/notification/application/DeviceTokenService.javasrc/main/java/com/dnd/modutime/core/notification/application/NotificationEventHandler.javasrc/main/java/com/dnd/modutime/core/notification/application/NotificationProcessor.javasrc/main/java/com/dnd/modutime/core/notification/application/NotificationQueryService.javasrc/main/java/com/dnd/modutime/core/notification/application/NotificationService.javasrc/main/java/com/dnd/modutime/core/notification/controller/DeviceTokenController.javasrc/main/java/com/dnd/modutime/core/notification/domain/DeviceToken.javasrc/main/java/com/dnd/modutime/core/notification/domain/Notification.javasrc/main/java/com/dnd/modutime/core/notification/domain/NotificationQueryRepository.javasrc/main/java/com/dnd/modutime/core/notification/domain/NotificationSendResult.javasrc/main/java/com/dnd/modutime/core/notification/domain/NotificationType.javasrc/main/java/com/dnd/modutime/infrastructure/fcm/FcmConfig.javasrc/main/java/com/dnd/modutime/infrastructure/fcm/FcmNotificationSender.javasrc/main/java/com/dnd/modutime/infrastructure/fcm/NoOpNotificationSender.javasrc/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationJpaQueryRepository.javasrc/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationQueryRepositoryImpl.javasrc/test/java/com/dnd/modutime/controller/notification/DeviceTokenControllerDocsTest.javasrc/test/java/com/dnd/modutime/controller/notification/NotificationControllerDocsTest.javasrc/test/java/com/dnd/modutime/core/notification/domain/DeviceTokenTest.javasrc/test/java/com/dnd/modutime/core/notification/domain/NotificationTest.javasrc/test/java/com/dnd/modutime/core/notification/integration/NotificationQueryServiceTest.javasrc/test/java/com/dnd/modutime/core/notification/integration/NotificationServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (8)
- src/test/java/com/dnd/modutime/core/notification/integration/NotificationServiceTest.java
- src/main/java/com/dnd/modutime/core/notification/application/NotificationQueryService.java
- src/main/java/com/dnd/modutime/infrastructure/persistence/notification/NotificationJpaQueryRepository.java
- src/main/java/com/dnd/modutime/core/notification/controller/DeviceTokenController.java
- src/main/java/com/dnd/modutime/core/notification/application/NotificationEventHandler.java
- src/main/java/com/dnd/modutime/core/notification/domain/NotificationQueryRepository.java
- src/main/java/com/dnd/modutime/core/notification/application/NotificationService.java
- build.gradle
Comment on lines
+14
to
+16
| public NotificationSendResult send(List<String> tokens, String title, String body, Map<String, String> data) { | ||
| log.debug("NoOp 알림 발송 - tokens: {}", tokens.size()); | ||
| return NotificationSendResult.success(tokens.size()); |
There was a problem hiding this comment.
NoOp가 성공을 반환하면 실제 미발송을 영구히 숨깁니다.
여기서 success(tokens.size())를 반환하면 NotificationProcessor가 이 알림들을 모두 markAsSent() 처리합니다. 즉 fcm.enabled=false나 설정 누락이 운영 계열 환경에 섞이면, 푸시는 한 건도 안 나갔는데 이력은 전부 발송 성공으로 남습니다. NoOp 경로는 최소한 empty()/failure()로 sent 상태를 올리지 않게 하거나, 아예 로컬·테스트 프로필 전용 빈으로 분리하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/dnd/modutime/infrastructure/fcm/NoOpNotificationSender.java`
around lines 14 - 16, The NoOpNotificationSender.send currently returns
NotificationSendResult.success(tokens.size()), which causes
NotificationProcessor.markAsSent to record non-sent pushes as delivered; change
send in NoOpNotificationSender to return a non-success result (e.g.,
NotificationSendResult.empty() or a failure result) so messages are not marked
sent, and additionally register NoOpNotificationSender only for non-production
profiles (e.g., local/test) instead of a global bean when fcm.enabled is false;
update references in NoOpNotificationSender.send, NotificationSendResult usage,
and bean configuration to enforce profile-restricted instantiation.
- 토큰 길이가 512자를 초과할 경우 IllegalArgumentException 발생하도록 수정.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closed
어떤 기능을 개발했나요?
어떻게 해결했나요?
어떤 부분에 집중하여 리뷰해야 할까요?
(option) 이 부분은 주의해 주세요.
(option) 참고자료
(option) 결과
RCA rule
Summary by CodeRabbit
New Features
Chores
Tests