[BE][feat]: fcm에 대한 트랜잭션 outbox 구현 #1029
Open
Conversation
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
Contributor
|
Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | PR 설명이 템플릿 구조를 따르고 있으나 실질적 내용이 없습니다. 관련 이슈는 명시되어 있지만 '작업 내용'과 '주요 변경사항'이 모두 비워져 있습니다. | 작업 내용에 outbox 패턴 구현의 목적과 의도를 설명하고, 주요 변경사항 섹션에 구현된 컴포넌트(도메인 엔티티, 저장소, 서비스, 디스패처) 및 트랜잭션 처리 방식을 구체적으로 작성해주세요. | |
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
|
| Linked Issues check | ❓ Inconclusive | 연결된 이슈 #1018이 구현 기능, 배경, 상세 작업 내용 모두 템플릿 플레이스홀더만 포함하고 있어 구체적인 요구사항을 파악할 수 없습니다. | 이슈 #1018에서 구현할 기능, 배경, 상세 작업 체크리스트를 명확히 작성하거나, 새로운 이슈를 통해 요구사항을 명시하고 해당 이슈에 연결해주세요. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | PR 제목 '[BE][feat]: fcm에 대한 트랜잭션 outbox 구현'은 변경 사항의 핵심인 FCM 관련 트랜잭션 outbox 패턴 구현을 명확하게 나타냅니다. |
| Out of Scope Changes check | ✅ Passed | 변경 사항들이 모두 FCM 트랜잭션 outbox 패턴 구현과 직접적으로 관련되어 있으며, 벗어난 범위의 변경사항은 없습니다. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat/1018
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmEventListener.java (1)
29-35: 주석처리된 코드는 삭제하는 것을 권장합니다.Outbox 패턴 도입으로 더 이상 사용되지 않는
onTokens메서드가 주석처리되어 있습니다.문제점:
- 주석처리된 코드는 코드베이스의 가독성을 떨어뜨립니다.
- 향후 유지보수 시 혼란을 야기할 수 있습니다 (이 코드를 살려야 하는지, 왜 주석처리되었는지 등).
- Git 히스토리에 모든 변경 이력이 남아있으므로, 필요시 언제든 복구 가능합니다.
대안:
- 삭제 (권장): 완전히 제거하고 Git 히스토리에 의존
- 문서화: 삭제와 함께 PR 또는 커밋 메시지에 제거 이유를 명확히 기록
-// @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) -// public void onTokens(SendMessageByFcmTokensRequest sendMessageByFcmTokensRequest) { -// Lists.partition(sendMessageByFcmTokensRequest.allTokens(), FCM_BATCH_SIZE) -// .forEach(tokens -> { -// fcmClient.sendMulticast(sendMessageByFcmTokensRequest.withTokens(tokens)); -// }); -// }backend/src/main/java/backend/mulkkam/device/repository/DeviceRepository.java (1)
30-35: 반환 타입으로 Object[] 대신 DTO나 Projection을 고려해보세요.
findMemberIdAndTokenByMemberIdIn메서드가List<Object[]>를 반환하고 있습니다.현재 방식의 단점:
- 타입 안정성이 떨어집니다 (Object[] 인덱스 접근 시 캐스팅 필요).
- 코드 가독성이 낮습니다 (각 인덱스가 무엇을 의미하는지 명확하지 않음).
- IDE의 자동완성 및 리팩토링 지원이 제한됩니다.
대안:
Option 1: Interface Projection (권장)
interface MemberIdAndToken { Long getMemberId(); String getToken(); } @Query(""" SELECT d.member.id as memberId, d.token as token FROM Device d WHERE d.member.id IN :memberIds """) List<MemberIdAndToken> findMemberIdAndTokenByMemberIdIn(@Param("memberIds") List<Long> memberIds);Option 2: DTO Projection
record MemberIdAndTokenDto(Long memberId, String token) {} @Query(""" SELECT new backend.mulkkam.device.dto.MemberIdAndTokenDto(d.member.id, d.token) FROM Device d WHERE d.member.id IN :memberIds """) List<MemberIdAndTokenDto> findMemberIdAndTokenByMemberIdIn(@Param("memberIds") List<Long> memberIds);장점:
- 타입 안정성 향상
- 코드 가독성 개선
- 컴파일 타임에 오류 검출 가능
backend/src/test/java/backend/mulkkam/notification/service/NotificationServiceUnitTest.java (1)
301-405: Outbox 패턴 도입 후 새로운 테스트가 필요합니다.리마인더 알림 처리 관련 테스트들이
@Disabled처리되어 있지만, Outbox 패턴 기반의 새로운 동작을 검증하는 테스트가 추가되지 않았습니다.현재 상황:
- 기존 직접 발송 방식 테스트가 비활성화됨
- Outbox 큐잉 방식으로 변경되었지만 이를 검증하는 테스트 부재
- 핵심 비즈니스 로직의 테스트 커버리지 공백 발생
추가 필요한 테스트 케이스:
- Outbox 큐잉 검증 테스트:
@DisplayName("리마인더 알림 처리 시 Outbox에 정상적으로 큐잉된다") @Test void enqueuesNotificationsToOutbox_whenProcessingReminders() { // given LocalDateTime now = LocalDateTime.of(2025, 1, 15, 14, 30); List<Long> memberIds = List.of(1L, 2L); when(reminderScheduleRepository.findAllActiveMemberIdsBySchedule(...)) .thenReturn(memberIds); // when notificationService.processReminderNotifications(now); // then verify(outboxNotificationService).enqueueOutbox( eq(memberIds), eq(now), any(NotificationMessageTemplate.class) ); }
- 알림 저장과 Outbox 큐잉의 트랜잭션 원자성 테스트:
@DisplayName("Outbox 큐잉 실패 시 알림 저장도 롤백된다") @Test void rollbacksNotificationSave_whenOutboxEnqueueFails() { // Outbox 패턴의 핵심인 원자성 보장을 검증 }
- 청크 단위 처리 테스트:
@DisplayName("대량의 멤버에 대해 청크 단위로 Outbox에 큐잉한다") @Test void enqueuesInChunks_whenProcessingLargeNumberOfMembers() { // 1000개 단위 청크 처리 검증 }권장사항:
단위 테스트 외에도 OutboxDispatcher의 실제 발송 로직에 대한 통합 테스트도 고려해보세요.새로운 테스트 코드 작성을 도와드릴까요? 필요하시다면 구체적인 테스트 케이스 구현을 제안해드릴 수 있습니다.
backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmClient.java (1)
109-139: 새로운 sendMulticast 메서드 추가를 확인했습니다.Outbox 패턴을 위한 직접 파라미터 방식의
sendMulticast메서드가 추가되었습니다. 기존 DTO 기반 메서드(85-107줄)와 유사한 로직을 가지고 있습니다.코드 중복 개선 제안 (선택사항):
현재 두 메서드가 거의 동일한 로직을 수행하고 있어, 내부 로직을 공통화하면 유지보수성이 향상됩니다.
public BatchResponse sendMulticast(String title, String body, String action, List<String> tokens) { return sendMulticastInternal(title, body, action, tokens); } public void sendMulticast(SendMessageByFcmTokensRequest request) { sendMulticastInternal( request.title(), request.body(), request.action().name(), request.allTokens() ); } private BatchResponse sendMulticastInternal(String title, String body, String action, List<String> tokens) { try { MulticastMessage message = MulticastMessage.builder() .addAllTokens(tokens) .putData("title", title) .putData("body", body) .putData(ACTION, action) .build(); BatchResponse res = firebaseMessaging.sendEachForMulticast(message); log.info("[FCM MULTICAST] success={}, fail={}, total={}, action={}", res.getSuccessCount(), res.getFailureCount(), tokens.size(), action ); return res; } catch (FirebaseMessagingException e) { log.error("[FCM MULTICAST FAILED] total={}, errorCode={}, errorMessage={}, action={}", tokens.size(), e.getMessagingErrorCode(), e.getMessage(), action ); throw new AlarmException(e); } }장단점:
- 👍 중복 코드 제거로 유지보수성 향상
- 👍 버그 수정 시 한 곳만 수정하면 됨
- 👎 메서드가 하나 더 생김 (복잡도 미미하게 증가)
현재 구현도 충분히 명확하므로 급하게 리팩토링할 필요는 없지만, 향후 로직 변경이 예상된다면 고려해볼만 합니다.
backend/src/main/java/backend/mulkkam/notification/service/NotificationService.java (1)
256-266: 사용되지 않는 코드(dead code)를 제거해주세요.Outbox 패턴 도입으로
sendNotifications와readDeviceTokens메서드가 더 이상 호출되지 않습니다.문제점:
- Dead code는 코드베이스를 불필요하게 복잡하게 만듭니다.
- 다른 개발자가 이 코드를 보고 어디서 사용되는지 찾느라 시간을 낭비할 수 있습니다.
- 리팩토링 시 혼란을 야기할 수 있습니다.
제안:
- private void sendNotifications( - List<Long> memberIds, - NotificationMessageTemplate template - ) { - List<String> tokens = readDeviceTokens(memberIds); - publisher.publishEvent(template.toSendMessageByFcmTokensRequest(tokens)); - } - - private List<String> readDeviceTokens(List<Long> memberIds) { - return deviceRepository.findAllTokenByMemberIdIn(memberIds); - }Git 히스토리에 모든 코드가 보존되므로, 필요시 언제든 복구할 수 있습니다.
backend/src/main/resources/db/migration/common/V20251101_213000__add_outbox_notification_table.sql (2)
1-22: Outbox 테이블 스키마 설계에 대한 몇 가지 개선 제안입니다.전반적으로 Outbox 패턴 구현을 위한 적절한 스키마 설계이지만, 몇 가지 검토가 필요한 부분이 있습니다.
1. type 컬럼 타입 최적화 (Line 4):
type VARCHAR(50) NOT NULL,현재 구현에서는 "REMIND" 같은 고정된 값만 사용됩니다.
대안:
VARCHAR(20)으로 축소 (충분한 여유 확보하면서 공간 절약)- 또는 ENUM 타입 고려:
type ENUM('REMIND', 'NOTICE', 'SUGGESTION') NOT NULLENUM 장단점:
- 👍 타입 안정성, 스토리지 효율, 명확한 도메인 제약
- 👎 새로운 타입 추가 시 ALTER TABLE 필요 (마이그레이션)
2. token 길이 검증 필요 (Line 6):
token VARCHAR(512) NOT NULL,FCM 토큰의 실제 최대 길이를 확인해주세요. 공식 문서에 따르면 일반적으로 152-163자 정도입니다. 512는 과하게 큰 값일 수 있으며, 인덱스 크기에도 영향을 줍니다.
3. dedupe_key 관리 전략 (Line 12):
dedupe_key VARCHAR(255) NOT NULL UNIQUE,Dedupe key가 시간 기반으로 생성되는데, 오래된 레코드에 대한 정리 전략이 필요합니다. UNIQUE 제약으로 인해 같은 키는 재사용할 수 없으므로, 배치 삭제 또는 TTL 정책을 고려해주세요.
4. 정리 쿼리를 위한 인덱스 추가 고려:
CREATE INDEX idx_outbox_created_at ON outbox_notification (created_at);오래된 성공/실패 레코드를 정리하는 배치 작업이 필요할 때 유용합니다.
28-35: 인덱스 설계를 수정하세요 — 현재 불필요한 인덱스가 2개 있습니다.코드를 검토한 결과, 실제 쿼리 패턴과 인덱스가 불일치합니다.
현황:
OutboxDispatcher는 오직status = 'READY'기준으로만 조회 (fetchReadyForSend())- 조회된 레코드에서
token을 추출할 뿐, token으로 별도 조회하는 쿼리가 없음member_id로 조회하는 쿼리도 없음 (device 테이블 조회는 별도)제거 또는 재검토가 필요한 인덱스:
idx_outbox_token(Lines 34-35) — 제거 권장
- VARCHAR(512) 크기로 인덱스 용량이 매우 큼
- Outbox 패턴은 대량의 INSERT가 발생하는데, 불필요한 인덱스는 쓰기 성능 저하
- 추후 "특정 토큰의 알림 상태 조회" 같은 기능이 추가된다면 그때 고려
idx_outbox_memberId(Lines 31-32) — 재검토 권장
- 현재 코드에서
member_id로 outbox 테이블을 조회하지 않음- "회원별 발송 이력" 같은 기능이 명확히 필요한지 확인 후 추가
유지해야 할 인덱스:
idx_outbox_status_nextAttempt✅ 필수 —fetchReadyForSend()의 WHERE 절을 효율적으로 처리개선 방향:
Outbox 패턴에서는 INSERT 성능이 중요하므로, 실제 워크로드에서 필요한 인덱스만 유지하는 것이 핵심입니다. 추후 새로운 조회 요구사항이 생기면 그때 인덱스를 추가하는 것을 권장합니다.backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java (2)
16-16: 사용되지 않는 상수를 제거해주세요.private static final int FCM_BATCH_SIZE = 500;이 상수는 코드 내에서 사용되지 않습니다. FCM 배치 처리는
OutboxDispatcher에서MULTICAST_SIZE = 500으로 별도 관리되고 있습니다.제안:
- private static final int FCM_BATCH_SIZE = 500;
50-52: dedupe key 생성 로직: 실제 위험도는 낮으나 개선 권고검증 결과, 당신의 우려 중 일부는 타당하지만 실제 상황은 더 안전합니다.
검증 내용:
중복 방지는 이미 작동 중:
OutboxNotification엔티티에서dedupeKey가@Column(unique = true)로 DB 제약이 설정되어 있습니다. 따라서 같은 키로 중복 저장 시도 시 예외가 발생하여 자동으로 방지됩니다.Token 포함은 의도된 설계: Token을 포함한 것으로 보아, "같은 멤버의 같은 디바이스"에만 중복을 방지하는 디자인입니다. 같은 멤버가 여러 디바이스를 사용하면 각 디바이스마다 알림을 받게 되는데, 이것이 현재 비즈니스 요구사항과 일치하는지 확인만 하면 됩니다.
타임존 문제는 여전히 유효:
LocalDateTime에는 타임존 정보가 없어서, 서버 타임존이 변경되면 같은 시간이라도 다른 key가 생성될 수 있습니다. 현재는 크리티컬하지 않지만, 나중에 다중 타임존 배포를 고려한다면 주의가 필요합니다.개선 제안 (필수 아님, 권고):
// 현재: return type + ":" + memberId + ":" + dateTime + ":" + token; // 권고: 명시적 포맷팅으로 가독성과 유지보수성 향상 String timestamp = dateTime.format(DateTimeFormatter.ofPattern("yyyyMMddHHmmss")); return String.format("%s:%d:%s:%s", type, memberId, timestamp, token);이렇게 하면 포맷 변경 시 의도가 명확하고, 나중에 다른 개발자가 코드를 읽을 때 이해하기 쉬워집니다. 타임존이 중요해지면
ZonedDateTime도입도 고려할 수 있습니다.backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java (2)
98-111: 영구 오류 정의와 백오프 전략은 현재도 동작하지만, 향후 확장성을 염두에 두고 정리해두면 좋습니다.
isPermanentError에서 FCM 에러 코드를 하드코딩한Set.of(...)로 판단하고 있고,nextBackoffTime은 최대 60초까지 지수적(2×2^attempt) 백오프를 적용하고 있습니다.현재로서는:
- 코드량이 적고 이해하기 쉬워 유지보수에는 큰 문제가 없지만,
- 에러 코드가 늘어나거나 type별로 다른 백오프 정책을 적용해야 할 때 분기 처리가 여기저기 흩어질 수 있습니다.
개선 제안 (선택사항)
에러 코드 매핑을 별도 Enum/전략 클래스로 위임
- 장점: 신규 에러 코드 대응 시 OutboxDispatcher 수정 없이 매핑만 추가 가능.
- 단점: 지금 단계에서는 다소 과한 추상화일 수 있음.
백오프 로직을 재시도 정책 객체로 분리
- 예:
RetryPolicy인터페이스 + 구현체(고정/지수/최대 시도 횟수 등)- 장점: 다른 Outbox 타입(이메일, SMS 등)에도 재사용 가능.
- 단점: 코드 구조가 조금 더 복잡해짐.
현재 요구사항이 단순하다면 그대로 두어도 되지만, Outbox를 다른 도메인으로 확장할 계획이 있다면, 정책/전략 클래스로 한 번 감싸두는 것도 고려해볼 만합니다.
113-123: 인덱스 청킹 로직은 명확하고, MULTICAST_SIZE 상수와도 잘 맞습니다.
chunkIndices는 단순하지만:
- 인덱스를 기준으로 청크를 나누어
jobs/tokens와의 매핑을 보존하고,MULTICAST_SIZE변경에도 자연스럽게 동작하며,- 경계 조건(사이즈가 정확히 배수인 경우, 마지막 청크가 작은 경우)도 잘 처리하고 있습니다.
테스트 관점에서는:
- size가 0, 1, MULTICAST_SIZE-1, MULTICAST_SIZE, MULTICAST_SIZE+1인 케이스 정도만 단위 테스트로 잡아두면, 향후 리팩토링 시 회귀를 빠르게 잡는 데 도움이 될 것 같습니다.
backend/src/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationRepository.java (1)
77-85:markFail에서는 attemptCount를 변경하지 않는데, 의도인지 한 번 더 확인이 필요해 보입니다.현재 FAIL 처리에는 두 가지 경로가 있습니다.
markRetryOrFail에서 재시도 한도를 넘겨 FAIL로 전환markFail을 직접 호출해 즉시 FAIL로 전환이때:
markRetryOrFail에서는attemptCount += 1과 함께 상태가 FAIL이 되고,markFail에서는attemptCount는 그대로 두고 상태만 FAIL로 바뀝니다.즉, 두 FAIL 경로가
attemptCount를 서로 다르게 취급합니다.
- 만약
attemptCount를 "전송 시도 횟수"로 정의한다면, 성공이든 실패든 최종 FAIL에 도달할 때의 값이 다르게 찍힐 수 있습니다.- 모니터링/알람에서 attemptCount를 보고 디버깅할 때 혼동이 생길 수 있습니다.
선택지
markFail에서attemptCount도 증가시키도록 맞추기
- 장점: 모든 FAIL 상태는 공통적으로 "한 번 더 시도했다"는 의미를 가지게 됨.
- 단점: 즉시 FAIL을 주는 케이스(예: 영구적인 비즈니스 에러)까지 시도 횟수에 포함되는 것이 애매할 수 있음.
- 반대로,
markRetryOrFail에서 FAIL로 전환될 때도attemptCount를 건드리지 않도록 정책을 맞추기.앞서 언급한 것처럼,
attemptCount가 무엇을 의미하는지(시도 횟수 vs 실패 횟수 vs 단순 메타데이터)를 먼저 정리한 뒤, 두 FAIL 경로의 처리 방식도 통일해 두면 이후 운영/관측 시 혼동을 줄일 수 있습니다.backend/src/main/java/backend/mulkkam/outboxnotification/domain/OutboxNotification.java (1)
48-52:nextAttemptAt의 타입 import 방식은 기능상 문제는 없지만, 스타일 통일을 위해 정리할 수 있습니다.private java.time.LocalDateTime nextAttemptAt;
- 현재 파일 상단에서 다른 타입들(
Column,Entity,Id등)은 모두 import 후 단순 클래스명으로 사용하고 있는데,LocalDateTime만 FQN으로 사용 중입니다.- 기능적으로는 전혀 문제가 없지만, 팀 내 스타일을 통일한다는 관점에서는 import로 빼 두는 편이 읽기/유지보수 측면에서 조금 더 자연스럽습니다.
예시:
-import java.time.LocalDateTime; ... - private java.time.LocalDateTime nextAttemptAt; + private LocalDateTime nextAttemptAt;큰 변경은 아니지만, 같은 파일 안의 다른 타입 사용 방식과 맞춰두면 이후 코드 읽을 때 작은 혼선을 줄일 수 있습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmClient.java(2 hunks)backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmEventListener.java(1 hunks)backend/src/main/java/backend/mulkkam/device/repository/DeviceRepository.java(1 hunks)backend/src/main/java/backend/mulkkam/notification/service/NotificationService.java(3 hunks)backend/src/main/java/backend/mulkkam/notification/service/ReminderScheduleService.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/domain/OutboxNotification.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationRepository.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java(1 hunks)backend/src/main/resources/db/migration/common/V20251101_213000__add_outbox_notification_table.sql(1 hunks)backend/src/test/java/backend/mulkkam/notification/service/NotificationServiceUnitTest.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
backend/**
⚙️ CodeRabbit configuration file
backend/**: - 1. 팀 및 공식 컨벤션, 가독성, 예외처리, 테스트/확장/유지보수성, 모듈화, API/DB/보안 설계 기준을 기반으로 리뷰해주세요.
- 최신 트렌드, 불필요한 로직, 클린코드, 리팩토링, 서비스/도메인 설계, 공통 예외 처리, 확장성도 함께 확인해주세요.
- 각 피드백은 문제점·대안·장단점을 짧고 논리적으로, 예시 코드가 있다면 간결히 포함해 주세요.
- 팀 내 스타일 통일성도 확인해주세요.
- 미작성한 테스트 코드 케이스가 있다면, 어떤 테스트가 필요한지 제안해주세요. (예: 컨트롤러는 인수 테스트, 나머지는 단위 테스트)
- 리뷰 남겨주는 부분은 해당 라인 범위의 코멘트에 작성해주세요.
Files:
backend/src/test/java/backend/mulkkam/notification/service/NotificationServiceUnitTest.javabackend/src/main/java/backend/mulkkam/notification/service/NotificationService.javabackend/src/main/java/backend/mulkkam/outboxnotification/domain/OutboxNotification.javabackend/src/main/resources/db/migration/common/V20251101_213000__add_outbox_notification_table.sqlbackend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.javabackend/src/main/java/backend/mulkkam/device/repository/DeviceRepository.javabackend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmEventListener.javabackend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmClient.javabackend/src/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationRepository.javabackend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.javabackend/src/main/java/backend/mulkkam/notification/service/ReminderScheduleService.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CheChe903
Repo: woowacourse-teams/2025-mul-kkam PR: 739
File: backend/src/test/java/backend/mulkkam/auth/controller/AuthControllerTest.java:113-114
Timestamp: 2025-09-10T04:46:55.219Z
Learning: CheChe903 has instructed not to consider Android code compatibility when reviewing backend code in the woowacourse-teams/2025-mul-kkam repository. Focus only on backend code concerns and do not flag client-server DTO mismatches or Android-related issues.
🧬 Code graph analysis (4)
backend/src/test/java/backend/mulkkam/notification/service/NotificationServiceUnitTest.java (1)
backend/src/test/java/backend/mulkkam/common/infrastructure/fcm/service/FcmClientUnitTest.java (1)
Disabled(34-168)
backend/src/main/java/backend/mulkkam/notification/service/NotificationService.java (2)
backend/src/main/java/backend/mulkkam/notification/service/ReminderScheduleService.java (1)
Transactional(25-104)backend/src/main/java/backend/mulkkam/notification/service/RemindNotificationMessageTemplateProvider.java (1)
RemindNotificationMessageTemplateProvider(10-31)
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java (2)
backend/src/main/java/backend/mulkkam/notification/service/NotificationService.java (1)
Transactional(39-276)backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java (1)
Service(16-124)
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java (1)
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java (1)
Service(13-53)
🔇 Additional comments (2)
backend/src/main/java/backend/mulkkam/notification/service/ReminderScheduleService.java (1)
36-40: 트랜잭션 경계 변경이 의도적인지 확인이 필요합니다.
scheduleReminderNotification()에서@Transactional어노테이션이 제거되었는데, 이는 Outbox 패턴 도입과 관련된 중요한 아키텍처 변경입니다.현재 구조:
- 스케줄러 메서드(
scheduleReminderNotification)는 비트랜잭셔널- 실행 메서드(
executeReminderNotification)는 트랜잭셔널검토 포인트:
- Outbox 패턴에서는 알림 저장과 Outbox 큐잉이 같은 트랜잭션 내에서 일어나야 원자성이 보장됩니다.
- 스케줄러 메서드가 트랜잭션을 시작하지 않으면, 하위 메서드(
executeReminderNotification)에서 새로운 트랜잭션이 시작되는데, 이것이 의도한 동작인지 확인이 필요합니다.- 만약 스케줄러 레벨에서 트랜잭션을 관리하지 않는 것이 의도라면, 이는 각 청크 처리마다 독립적인 트랜잭션을 사용한다는 의미입니다.
제안:
설계 의도가 명확하다면 문제없지만, Outbox 패턴의 원자성 보장을 위해 트랜잭션 범위가 적절한지 재검토해보시길 권장합니다.backend/src/main/java/backend/mulkkam/notification/service/NotificationService.java (1)
237-244: Outbox 패턴 통합이 적절하게 이루어졌습니다.알림 발송 로직이 직접 전송에서 Outbox 큐잉 방식으로 변경되었습니다.
LocalDateTime now파라미터를 추가하여 dedupe key 생성에 활용하는 것은 좋은 설계입니다.검증 포인트:
savedNotifications()와outboxNotificationService.enqueueOutbox()가 같은 트랜잭션 내에서 실행되어 원자성이 보장되는지 확인이 필요합니다.- 현재
@Transactional이processReminderNotifications에는 없고saveAndSendNotifications에도 없는 것으로 보입니다. 호출 체인을 따라가면ReminderScheduleService.executeReminderNotification()에서 트랜잭션이 시작되는데, 이것이 의도한 범위인지 확인해주세요.
backend/src/main/java/backend/mulkkam/device/repository/DeviceRepository.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/backend/mulkkam/outboxnotification/domain/OutboxNotification.java
Show resolved
Hide resolved
...rc/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationRepository.java
Outdated
Show resolved
Hide resolved
...rc/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationRepository.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java
Show resolved
Hide resolved
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java
Show resolved
Hide resolved
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java
Show resolved
Hide resolved
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@coderabbitai 현재 상황까지의 PR을 검토해줘 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java (1)
28-38: 이전 트랜잭션 경계 이슈는@Transactional추가로 잘 해결된 것 같습니다.
dispatch()에@Transactional이 붙으면서,fetchReadyForSend와markSending가 같은 트랜잭션 안에서 실행되어 SKIP LOCKED 의도가 잘 살아납니다. 멀티 인스턴스 환경에서도 동일 Outbox row를 중복 처리할 가능성이 크게 줄어든 구조라 좋습니다.다만 아래 코멘트에서 언급하듯, FCM 호출까지 같은 트랜잭션에 묶는 것이 최선인지에 대해선 선택지가 있으니, 운영/성능 요구사항을 보면서 한 번 더 고민해보시면 좋겠습니다.
backend/src/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationRepository.java (1)
13-69: RETRY 조회/재시도 횟수 로직은 잘 정리되었고, 상수/enum 활용 여지만 조금 보입니다.이전 리뷰에서 지적되었던 두 포인트가 모두 개선된 상태라 흐름이 꽤 깔끔해졌습니다.
fetchReadyForSend가READY뿐 아니라RETRY도 함께 조회하고,next_attempt_at <= NOW()조건까지 포함되어 실제 재시도 로직이 동작하게 된 점 👍markSending에서만attemptCount를 증가시키고,markRetryOrFail에서는attemptCount < 3조건만 보고 상태를 정하는 방식으로 바뀌어,
attemptCount를 "총 전송 시도 횟수"로 해석하기가 훨씬 자연스러워졌습니다.다만, 향후 유지보수를 위해 아래 정도만 추가로 고민해볼 수 있습니다.
매직 넘버 3 → 상수화
attemptCount < 3은 사실상 "최대 3번까지 재시도"라는 정책을 의미하므로,- 도메인 상수(예:
MAX_RETRY_ATTEMPTS)로 빼두면 정책 변경 시 한 곳만 수정하면 되고, 의도도 더 분명해집니다.상태 문자열을 enum 상수로 통일
- 현재 JPQL은
'SENDING','SENT','RETRY','FAIL'을 문자열 리터럴로 사용하고 있습니다.- 길게 보면:
- 오타가 나도 컴파일 단계에서 잡히지 않고,
- Status enum에 새 값을 추가/이름을 변경할 때 동기화가 깨질 수 있습니다.
- 선택지:
- (A) 그대로 문자열을 쓰되, Status enum과 SQL/JPQL을 맞춰야 한다는 점을 주석으로 명확히 남기기.
- (B) JPQL에서 enum 상수를 직접 참조:
SET o.status = backend.mulkkam.outboxnotification.domain.OutboxNotification.Status.SENDING
- 장점: 컴파일 타임에 enum 이름 변경 시 바로 깨짐.
- 단점: 쿼리가 다소 길어지고, native 쿼리(fetchReadyForSend)에서는 여전히 문자열을 쓸 수밖에 없음.
전체적으로는 이미 동작 측면에서 큰 문제는 없어 보이고, 위 두 부분은 "정책/의도를 더 분명하게 드러내는 리팩터링" 정도로 보시면 될 것 같습니다.
🧹 Nitpick comments (4)
backend/src/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationJdbcRepository.java (1)
17-35: 대량 배치 시 batchSize, 빈 리스트 처리까지 한 번 점검하면 좋겠습니다.현재 구현은
JdbcTemplate.batchUpdate(sql, outboxList, outboxList.size(), ...)로 전체 리스트를 한 번의 배치로 처리하고 있어서, 기능적으로는 문제 없지만 두 가지 관점에서 살짝 아쉬운 부분이 있습니다.
빈 리스트 처리
outboxList가 비어 있으면 굳이 DB까지 내려갈 필요가 없습니다.- 사소하지만 스케줄러가 자주 도는 구조라면 불필요한 호출을 줄이는 게 좋습니다.
- 예)
if (outboxList.isEmpty()) { return; }배치 크기를 고정 상수로 두는 선택지
- 지금처럼
outboxList.size()를 그대로 넘기면, 상위에서 실수로 너무 큰 리스트를 넘겼을 때 한 번에 꽤 큰 배치가 나갈 수 있습니다.- 선택지:
- (A) 현재처럼 그대로 두고, 상위 레이어에서 이미 CHUNK_SIZE로 잘라서 넘긴다는 전제를 문서/주석에 명시
- (B) 로컬 상수(예:
private static final int BATCH_SIZE = 1_000;)를 두고,Math.min(outboxList.size(), BATCH_SIZE)로 제한- (A)는 현재 코드 변경이 거의 없어 간단하지만, 전제를 모르는 사람이 쓰면 실수 여지가 있습니다.
(B)는 더 안전하지만, 상위 CHUNK와 중복되는 느낌이 있을 수 있습니다.기능 관점에선 그대로 가도 되지만, 운영/확장성을 생각하면 위 두 포인트를 한 번 고민해보시면 좋겠습니다.
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java (2)
22-52: 토큰 조회 및 Outbox 생성 로직은 좋아 보이지만, 타입 안정성과 빈 결과 처리에 약간 여지가 있습니다.전반적으로 이전 리뷰에서 지적되었던 N+1, 개별 저장 문제는 잘 해소된 구조입니다. 다만 유지보수성과 안정성을 위해 몇 부분만 더 다듬을 수 있습니다.
List<Object[]>대신 Projection/DTO 사용 권장
- 현재:
List<Object[]> memberTokenPairs = deviceRepository.findMemberIdAndTokenByMemberIdIn(memberIds); Long memberId = (Long) pair[0]; String token = (String) pair[1];- 문제점
- 인덱스와 캐스팅에 의존해서, Repository 쿼리나 select 순서가 바뀌면 런타임
ClassCastException가능성이 있습니다.- 읽을 때 의도가 바로 드러나지 않아, 나중에 코드를 접하는 사람이 구조를 다시 추적해야 합니다.
- 대안 (선택지)
- (A) 인터페이스 기반 Projection
interface MemberTokenView { Long getMemberId(); String getToken(); }List<MemberTokenView> pairs = deviceRepository.findMemberIdAndTokenByMemberIdIn(memberIds); pairs.forEach(pair -> { pair.getMemberId(); pair.getToken(); });
- 장점: 타입 안전, 구조 변경 시 컴파일 타임에 깨짐.
- 단점: Projection 인터페이스 추가 필요.
- (B) 단순 DTO
record MemberToken(Long memberId, String token) {}
- 장점: 순수 도메인/인프라 레이어에 명시적인 타입을 남길 수 있음.
- 단점: DTO 생성/매핑 코드가 약간 늘어남.
빈 결과/빈 리스트 처리
memberTokenPairs가 비어 있을 수 있는 상황(디바이스가 하나도 없는 멤버들만 포함된 경우 등)을 고려하면,- 아래와 같이 조기에 반환해 주면 불필요한 DB 배치 호출을 피할 수 있습니다.
if (memberTokenPairs.isEmpty()) { return; }- 큰 문제는 아니지만, Outbox 테이블을 쓸데없이 두드리지 않는다는 점에서 운영 환경에는 조금 더 친절해집니다.
테스트 관점에서 추가로 있으면 좋을 케이스
memberIds에 디바이스가 없는 멤버/있는 멤버가 섞여 있을 때,- 동일
memberId + now + token조합으로 두 번 호출해도 중복 Row가 생기지 않는지(ON DUPLICATE KEY와 dedupeKey가 잘 맞는지),- 위 두 케이스에 대한 단위 테스트를 OutboxNotificationService 수준에서 한 번 챙겨두시면 회귀 방지에 도움이 됩니다.
54-61: dedupeKey 포맷(특히 LocalDateTime 사용 범위)이 의도한 중복 기준과 맞는지 한 번만 확인해주세요.현재 dedupeKey는 다음 형태입니다.
return type + ":" + memberId + ":" + dateTime + ":" + token;여기서
dateTime의 기본toString()은 초/나노초까지 포함한 ISO 문자열이기 때문에:
- 장점
- 같은 멤버/토큰이라도 호출 시각이 조금이라도 다르면(초 단위) 서로 다른 dedupeKey로 취급되어, 실질 중복만 막고 최대한 많이 보내는 쪽에 가깝습니다.
- 단점
- 만약 비즈니스 의도가 "동일 시간 슬롯(예: 특정 리마인더 시간대) 내에 한 번만 보내자"라면, 현재는
now가 조금만 달라도(몇 초 차이) 서로 다른 키가 되어 dedupe 효과가 약할 수 있습니다.선택지로는 예를 들면:
- (A) 지금처럼 정확한 LocalDateTime 전체를 쓰되, "트랜잭션 단위 중복 방지"에 초점을 맞춘다.
- (B)
dateTime을truncatedTo(ChronoUnit.MINUTES)등으로 잘라서, "같은 분 단위에서는 하나만" 같은 정책을 구현한다.- (C) 날짜/슬롯을 별도 컬럼 또는 별도 필드(예:
remind_slot)로 설계해서, dedupeKey를 더 명시적으로 구성한다.현재 비즈니스 요구가 (A)에 가깝다면 그대로 두셔도 괜찮고, (B)/(C)에 가깝다면 포맷 조정이 필요할 수 있습니다. 요구사항과 맞는지 한 번만 팀 내에서 정합성 체크해보시면 좋겠습니다.
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java (1)
28-60: 트랜잭션 범위를 FCM 호출까지 묶을지, fetch/상태 업데이트만 묶을지 설계 선택지가 있습니다.현재
dispatch()전체가@Transactional이라서:
fetchReadyForSend(FOR UPDATE SKIP LOCKED)markSending- FCM
sendMulticast호출markSent/markRetryOrFail/markFail이 네 단계가 하나의 긴 DB 트랜잭션으로 묶입니다. 이 구조는 중복 처리를 막는다는 점에서 직관적이지만, 다음과 같은 trade-off가 있습니다.
- 장점
- 한 트랜잭션 안에서 상태 전이가 일어나므로, 중간에 예외가 터지면 전체가 롤백되어 상태가 꼬일 여지가 적습니다.
- 단점
- FCM 네트워크 호출이 느려지면 그만큼 DB 락 유지 시간이 늘어나고, 다른 디스패처나 트랜잭션과 경합할 수 있습니다.
- 외부 시스템 장애로 FCM이 계속 지연될 때, Outbox 테이블에 대한 락 잡힌 트랜잭션이 길게 유지될 수 있습니다.
선택지 예시:
- (A) 현재처럼 단일 트랜잭션 유지 (간단, 일관성↑)
- 지금 구조 그대로 유지.
- 운영 환경에서 FCM 지연이 크지 않다면 실용적으로 충분히 괜찮은 선택입니다.
- (B) fetch + markSending까지만 트랜잭션으로 묶고, FCM + 결과 업데이트는 별도 트랜잭션
- 예시 구조:
- 트랜잭션1: READY/RETRY → SENDING으로 상태 변경까지 커밋.
- FCM 호출 (트랜잭션 밖).
- 트랜잭션2: 결과에 따라 SENT/RETRY/FAIL로 갱신.
- 장점: DB 락이 외부 호출 시간만큼 길게 유지되지 않습니다.
- 단점: 코드 구조가 한 단계 복잡해지고, 중간 예외 처리(FCM 호출 실패 시 SENDING만 남는 상황 등)에 대한 추가 정책이 필요합니다.
지금 단계에서는 (A)도 충분히 현실적인 선택이라, 꼭 바꿔야 하는 상황은 아닙니다. 다만 트래픽 증가나 FCM 지연에 민감해질 수 있는 서비스라면, 장기적으로 (B) 같은 분리 설계도 한 번 고려해보시면 좋겠습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/main/java/backend/mulkkam/device/repository/DeviceRepository.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/domain/OutboxNotification.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationJdbcRepository.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationRepository.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/main/java/backend/mulkkam/device/repository/DeviceRepository.java
- backend/src/main/java/backend/mulkkam/outboxnotification/domain/OutboxNotification.java
🧰 Additional context used
📓 Path-based instructions (1)
backend/**
⚙️ CodeRabbit configuration file
backend/**: - 1. 팀 및 공식 컨벤션, 가독성, 예외처리, 테스트/확장/유지보수성, 모듈화, API/DB/보안 설계 기준을 기반으로 리뷰해주세요.
- 최신 트렌드, 불필요한 로직, 클린코드, 리팩토링, 서비스/도메인 설계, 공통 예외 처리, 확장성도 함께 확인해주세요.
- 각 피드백은 문제점·대안·장단점을 짧고 논리적으로, 예시 코드가 있다면 간결히 포함해 주세요.
- 팀 내 스타일 통일성도 확인해주세요.
- 미작성한 테스트 코드 케이스가 있다면, 어떤 테스트가 필요한지 제안해주세요. (예: 컨트롤러는 인수 테스트, 나머지는 단위 테스트)
- 리뷰 남겨주는 부분은 해당 라인 범위의 코멘트에 작성해주세요.
Files:
backend/src/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationRepository.javabackend/src/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationJdbcRepository.javabackend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.javabackend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java
🧠 Learnings (3)
📓 Common learnings
Learnt from: CheChe903
Repo: woowacourse-teams/2025-mul-kkam PR: 739
File: backend/src/test/java/backend/mulkkam/auth/controller/AuthControllerTest.java:113-114
Timestamp: 2025-09-10T04:46:55.219Z
Learning: CheChe903 has instructed not to consider Android code compatibility when reviewing backend code in the woowacourse-teams/2025-mul-kkam repository. Focus only on backend code concerns and do not flag client-server DTO mismatches or Android-related issues.
📚 Learning: 2025-09-08T07:30:35.444Z
Learnt from: Jin409
Repo: woowacourse-teams/2025-mul-kkam PR: 725
File: backend/src/main/java/backend/mulkkam/auth/domain/AccountRefreshToken.java:29-34
Timestamp: 2025-09-08T07:30:35.444Z
Learning: Flyway를 사용하는 환경에서는 DDL을 마이그레이션 스크립트로만 관리하고, JPA 엔티티에서는 제약조건을 중복 선언하지 않는 것이 권장된다. 단일 진실의 원천, 환경 일관성, 충돌 방지의 장점이 있다.
Applied to files:
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java
📚 Learning: 2025-10-08T05:09:28.559Z
Learnt from: 2Jin1031
Repo: woowacourse-teams/2025-mul-kkam PR: 896
File: backend/src/test/java/backend/mulkkam/intake/service/IntakeHistoryCrudIntegrationServiceTest.java:73-79
Timestamp: 2025-10-08T05:09:28.559Z
Learning: Spring Data JPA의 save() 메서드는 엔티티를 영속화하며, 원본 객체에도 ID가 할당된다. save() 호출 후 원본 객체와 반환된 객체는 동일한 인스턴스이므로, 원본 객체를 사용해도 ID 기반 조회가 정상 작동한다.
Applied to files:
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java
🧬 Code graph analysis (2)
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java (2)
backend/src/main/java/backend/mulkkam/notification/service/NotificationService.java (1)
Transactional(39-276)backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java (1)
Service(15-62)
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java (2)
backend/src/main/java/backend/mulkkam/notification/service/NotificationService.java (1)
Transactional(39-276)backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java (1)
Service(18-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java
Show resolved
Hide resolved
Contributor
Author
|
@coderabbitai 우리 테스트 컨벤션을 확인하고, 현재 PR의 통합 테스트를 작성해줘 |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 205 🏁 Script executed: Length of output: 2653 🏁 Script executed: Length of output: 11138 🏁 Script executed: Length of output: 16580 🏁 Script executed: Length of output: 470 🏁 Script executed: Length of output: 723 🏁 Script executed: Length of output: 39 🏁 Script executed: Length of output: 2719 🏁 Script executed: Length of output: 19267 🏁 Script executed: Length of output: 2174 🏁 Script executed: Length of output: 4264 🏁 Script executed: Length of output: 6288 🏁 Script executed: Length of output: 1623 🏁 Script executed: Length of output: 44 🏁 Script executed: Length of output: 237 🏁 Script executed: Length of output: 6692 🏁 Script executed: Length of output: 1656 🏁 Script executed: Length of output: 2014 🏁 Script executed: Length of output: 2359 🏁 Script executed: Length of output: 1406 🏁 Script executed: Length of output: 913 🏁 Script executed: Length of output: 1878 @CheChe903, 테스트 컨벤션을 확인했습니다. 현재 PR에서 구현한 Outbox 패턴에 대한 통합 테스트를 작성해드리겠습니다. 작성할 통합 테스트주요 변경사항을 분석한 결과, 다음 테스트들이 필요합니다:
1. OutboxNotificationServiceIntegrationTestpackage backend.mulkkam.outboxnotification.service;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.SoftAssertions.assertSoftly;
import backend.mulkkam.common.infrastructure.fcm.domain.Action;
import backend.mulkkam.device.domain.Device;
import backend.mulkkam.device.repository.DeviceRepository;
import backend.mulkkam.member.domain.Member;
import backend.mulkkam.member.repository.MemberRepository;
import backend.mulkkam.notification.domain.NotificationType;
import backend.mulkkam.notification.dto.NotificationMessageTemplate;
import backend.mulkkam.outboxnotification.domain.OutboxNotification;
import backend.mulkkam.outboxnotification.domain.OutboxNotification.Status;
import backend.mulkkam.outboxnotification.repository.OutboxNotificationRepository;
import backend.mulkkam.support.fixture.DeviceFixtureBuilder;
import backend.mulkkam.support.fixture.member.MemberFixtureBuilder;
import backend.mulkkam.support.service.ServiceIntegrationTest;
import java.time.LocalDateTime;
import java.util.List;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
class OutboxNotificationServiceIntegrationTest extends ServiceIntegrationTest {
@Autowired
private OutboxNotificationService outboxNotificationService;
@Autowired
private OutboxNotificationRepository outboxNotificationRepository;
@Autowired
private MemberRepository memberRepository;
@Autowired
private DeviceRepository deviceRepository;
private Member member1;
private Member member2;
private Member member3;
private Device device1;
private Device device2;
private Device device3;
private Device device4;
@BeforeEach
void setUp() {
member1 = memberRepository.save(MemberFixtureBuilder.builder().build());
member2 = memberRepository.save(MemberFixtureBuilder.builder().build());
member3 = memberRepository.save(MemberFixtureBuilder.builder().build());
device1 = deviceRepository.save(
DeviceFixtureBuilder.withMember(member1).token("token-1").build()
);
device2 = deviceRepository.save(
DeviceFixtureBuilder.withMember(member1).token("token-2").build()
);
device3 = deviceRepository.save(
DeviceFixtureBuilder.withMember(member2).token("token-3").build()
);
device4 = deviceRepository.save(
DeviceFixtureBuilder.withMember(member3).token("token-4").build()
);
}
@DisplayName("Outbox에 알림을 enqueue할 때")
@Nested
class EnqueueOutbox {
@DisplayName("success_여러 멤버의 디바이스 토큰으로 outbox 레코드가 생성된다")
@Test
void success_createsOutboxRecordsForMultipleMemberDevices() {
// given
List<Long> memberIds = List.of(member1.getId(), member2.getId(), member3.getId());
LocalDateTime now = LocalDateTime.of(2025, 11, 18, 14, 30);
NotificationMessageTemplate template = new NotificationMessageTemplate(
"물 마실 시간이에요",
"오늘도 건강한 하루 되세요!",
Action.REMIND,
NotificationType.REMIND
);
// when
outboxNotificationService.enqueueOutbox(memberIds, now, template);
// then
List<OutboxNotification> savedOutboxes = outboxNotificationRepository.findAll();
assertSoftly(softly -> {
softly.assertThat(savedOutboxes).hasSize(4);
softly.assertThat(savedOutboxes)
.allMatch(outbox -> outbox.getType().equals("REMIND"));
softly.assertThat(savedOutboxes)
.allMatch(outbox -> outbox.getStatus() == Status.READY);
softly.assertThat(savedOutboxes)
.allMatch(outbox -> outbox.getTitle().equals("물 마실 시간이에요"));
softly.assertThat(savedOutboxes)
.allMatch(outbox -> outbox.getBody().equals("오늘도 건강한 하루 되세요!"));
softly.assertThat(savedOutboxes)
.allMatch(outbox -> outbox.getAttemptCount() == 0);
softly.assertThat(savedOutboxes)
.allMatch(outbox -> outbox.getNextAttemptAt() == null);
});
}
@DisplayName("success_한 멤버가 여러 디바이스를 가진 경우 모든 토큰에 대해 outbox가 생성된다")
@Test
void success_createsOutboxForAllDevicesOfSingleMember() {
// given
List<Long> memberIds = List.of(member1.getId());
LocalDateTime now = LocalDateTime.of(2025, 11, 18, 14, 30);
NotificationMessageTemplate template = new NotificationMessageTemplate(
"테스트 제목",
"테스트 내용",
Action.REMIND,
NotificationType.REMIND
);
// when
outboxNotificationService.enqueueOutbox(memberIds, now, template);
// then
List<OutboxNotification> savedOutboxes = outboxNotificationRepository.findAll();
assertSoftly(softly -> {
softly.assertThat(savedOutboxes).hasSize(2);
softly.assertThat(savedOutboxes)
.extracting(OutboxNotification::getMemberId)
.containsOnly(member1.getId());
softly.assertThat(savedOutboxes)
.extracting(OutboxNotification::getToken)
.containsExactlyInAnyOrder("token-1", "token-2");
});
}
@DisplayName("success_동일한 dedupe_key로 중복 enqueue 시 무시된다")
@Test
void success_ignoresDuplicateDedupeKey() {
// given
List<Long> memberIds = List.of(member1.getId());
LocalDateTime now = LocalDateTime.of(2025, 11, 18, 14, 30);
NotificationMessageTemplate template = new NotificationMessageTemplate(
"테스트 제목",
"테스트 내용",
Action.REMIND,
NotificationType.REMIND
);
// when
outboxNotificationService.enqueueOutbox(memberIds, now, template);
outboxNotificationService.enqueueOutbox(memberIds, now, template);
// then
List<OutboxNotification> savedOutboxes = outboxNotificationRepository.findAll();
assertThat(savedOutboxes).hasSize(2);
}
@DisplayName("success_멤버에게 등록된 디바이스가 없으면 outbox가 생성되지 않는다")
@Test
void success_noOutboxCreatedWhenMemberHasNoDevices() {
// given
Member memberWithoutDevice = memberRepository.save(
MemberFixtureBuilder.builder().build()
);
List<Long> memberIds = List.of(memberWithoutDevice.getId());
LocalDateTime now = LocalDateTime.of(2025, 11, 18, 14, 30);
NotificationMessageTemplate template = new NotificationMessageTemplate(
"테스트 제목",
"테스트 내용",
Action.REMIND,
NotificationType.REMIND
);
// when
outboxNotificationService.enqueueOutbox(memberIds, now, template);
// then
List<OutboxNotification> savedOutboxes = outboxNotificationRepository.findAll();
assertThat(savedOutboxes).isEmpty();
}
@DisplayName("success_dedupe_key가 올바른 형식으로 생성된다")
@Test
void success_dedupeKeyGeneratedCorrectly() {
// given
List<Long> memberIds = List.of(member1.getId());
LocalDateTime now = LocalDateTime.of(2025, 11, 18, 14, 30);
NotificationMessageTemplate template = new NotificationMessageTemplate(
"테스트 제목",
"테스트 내용",
Action.REMIND,
NotificationType.REMIND
);
// when
outboxNotificationService.enqueueOutbox(memberIds, now, template);
// then
List<OutboxNotification> savedOutboxes = outboxNotificationRepository.findAll();
assertSoftly(softly -> {
softly.assertThat(savedOutboxes).hasSize(2);
savedOutboxes.forEach(outbox -> {
String expectedDedupeKey = "REMIND:" + outbox.getMemberId() + ":" + now;
softly.assertThat(outbox.getDedupeKey()).isEqualTo(expectedDedupeKey);
});
});
}
@DisplayName("success_다른 시간으로 enqueue 시 별도의 outbox가 생성된다")
@Test
void success_separateOutboxForDifferentTime() {
// given
List<Long> memberIds = List.of(member1.getId());
LocalDateTime time1 = LocalDateTime.of(2025, 11, 18, 14, 30);
LocalDateTime time2 = LocalDateTime.of(2025, 11, 18, 15, 30);
NotificationMessageTemplate template = new NotificationMessageTemplate(
"테스트 제목",
"테스트 내용",
Action.REMIND,
NotificationType.REMIND
);
// when
outboxNotificationService.enqueueOutbox(memberIds, time1, template);
outboxNotificationService.enqueueOutbox(memberIds, time2, template);
// then
List<OutboxNotification> savedOutboxes = outboxNotificationRepository.findAll();
assertSoftly(softly -> {
softly.assertThat(savedOutboxes).hasSize(4);
softly.assertThat(savedOutboxes)
.extracting(OutboxNotification::getDedupeKey)
.containsExactlyInAnyOrder(
"REMIND:" + member1.getId() + ":" + time1,
"REMIND:" + member1.getId() + ":" + time1,
"REMIND:" + member1.getId() + ":" + time2,
"REMIND:" + member1.getId() + ":" + time2
);
});
}
}
@DisplayName("Dedupe key를 생성할 때")
@Nested
class BuildDedupeKey {
@DisplayName("success_올바른 형식의 dedupe key가 생성된다")
@Test
void success_generatesCorrectFormat() {
// given
String type = "REMIND";
Long memberId = 123L;
LocalDateTime time = LocalDateTime.of(2025, 11, 18, 14, 30);
// when
String dedupeKey = outboxNotificationService.buildDedupeKey(type, memberId, time);
// then
assertThat(dedupeKey).isEqualTo("REMIND:123:2025-11-18T14:30");
}
}
}2. OutboxNotificationFixtureBuilderpackage backend.mulkkam.support.fixture.outbox;
import backend.mulkkam.outboxnotification.domain.OutboxNotification;
import backend.mulkkam.outboxnotification.domain.OutboxNotification.Status;
import java.time.LocalDateTime;
public class OutboxNotificationFixtureBuilder {
private String type = "REMIND";
private Long memberId = 1L;
private String token = "test-fcm-token";
private String title = "물 마실 시간이에요";
private String body = "오늘도 건강한 하루 되세요!";
private Status status = Status.READY;
private String dedupeKey = "REMIND:1:" + LocalDateTime.now();
private int attemptCount = 0;
private String lastError = null;
private LocalDateTime nextAttemptAt = null;
private OutboxNotificationFixtureBuilder() {
}
public static OutboxNotificationFixtureBuilder builder() {
return new OutboxNotificationFixtureBuilder();
}
public OutboxNotificationFixtureBuilder type(String type) {
this.type = type;
return this;
}
public OutboxNotificationFixtureBuilder memberId(Long memberId) {
this.memberId = memberId;
return this;
}
public OutboxNotificationFixtureBuilder token(String token) {
this.token = token;
return this;
}
public OutboxNotificationFixtureBuilder title(String title) {
this.title = title;
return this;
}
public OutboxNotificationFixtureBuilder body(String body) {
this.body = body;
return this;
}
public OutboxNotificationFixtureBuilder status(Status status) {
this.status = status;
return this;
}
public OutboxNotificationFixtureBuilder dedupeKey(String dedupeKey) {
this.dedupeKey = dedupeKey;
return this;
}
public OutboxNotificationFixtureBuilder attemptCount(int attemptCount) {
this.attemptCount = attemptCount;
return this;
}
public OutboxNotificationFixtureBuilder lastError(String lastError) {
this.lastError = lastError;
return this;
}
public OutboxNotificationFixtureBuilder nextAttemptAt(LocalDateTime nextAttemptAt) {
this.nextAttemptAt = nextAttemptAt;
return this;
}
public OutboxNotification build() {
return new OutboxNotification(
type,
memberId,
token,
title,
body,
status,
dedupeKey,
attemptCount,
lastError,
nextAttemptAt
);
}
}3. DeviceRepository 메서드 테스트 (선택사항)package backend.mulkkam.device.repository;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.SoftAssertions.assertSoftly;
import backend.mulkkam.device.domain.Device;
import backend.mulkkam.member.domain.Member;
import backend.mulkkam.member.repository.MemberRepository;
import backend.mulkkam.support.fixture.DeviceFixtureBuilder;
import backend.mulkkam.support.fixture.member.MemberFixtureBuilder;
import backend.mulkkam.support.service.ServiceIntegrationTest;
import java.util.List;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
class DeviceRepositoryTest extends ServiceIntegrationTest {
@Autowired
private DeviceRepository deviceRepository;
@Autowired
private MemberRepository memberRepository;
private Member member1;
private Member member2;
private Member member3;
@BeforeEach
void setUp() {
member1 = memberRepository.save(MemberFixtureBuilder.builder().build());
member2 = memberRepository.save(MemberFixtureBuilder.builder().build());
member3 = memberRepository.save(MemberFixtureBuilder.builder().build());
deviceRepository.save(
DeviceFixtureBuilder.withMember(member1).token("token-1").build()
);
deviceRepository.save(
DeviceFixtureBuilder.withMember(member1).token("token-2").build()
);
deviceRepository.save(
DeviceFixtureBuilder.withMember(member2).token("token-3").build()
);
}
@DisplayName("여러 멤버 ID로 토큰을 조회할 때")
@Nested
class FindAllTokenByMemberIdIn {
@DisplayName("success_해당하는 모든 토큰을 반환한다")
@Test
void success_returnsAllTokensForGivenMemberIds() {
// given
List<Long> memberIds = List.of(member1.getId(), member2.getId());
// when
List<String> tokens = deviceRepository.findAllTokenByMemberIdIn(memberIds);
// then
assertSoftly(softly -> {
softly.assertThat(tokens).hasSize(3);
softly.assertThat(tokens).containsExactlyInAnyOrder("token-1", "token-2", "token-3");
});
}
@DisplayName("success_디바이스가 없는 멤버는 결과에 포함되지 않는다")
@Test
void success_excludesMembersWithoutDevices() {
// given
List<Long> memberIds = List.of(member1.getId(), member3.getId());
// when
List<String> tokens = deviceRepository.findAllTokenByMemberIdIn(memberIds);
// then
assertSoftly(softly -> {
softly.assertThat(tokens).hasSize(2);
softly.assertThat(tokens).containsExactlyInAnyOrder("token-1", "token-2");
});
}
@DisplayName("success_빈 리스트로 조회 시 빈 리스트를 반환한다")
@Test
void success_returnsEmptyListForEmptyInput() {
// given
List<Long> memberIds = List.of();
// when
List<String> tokens = deviceRepository.findAllTokenByMemberIdIn(memberIds);
// then
assertThat(tokens).isEmpty();
}
}
@DisplayName("단일 멤버 ID로 토큰을 조회할 때")
@Nested
class FindAllTokenByMemberId {
@DisplayName("success_해당 멤버의 모든 토큰을 반환한다")
@Test
void success_returnsAllTokensForSingleMember() {
// when
List<String> tokens = deviceRepository.findAllTokenByMemberId(member1.getId());
// then
assertSoftly(softly -> {
softly.assertThat(tokens).hasSize(2);
softly.assertThat(tokens).containsExactlyInAnyOrder("token-1", "token-2");
});
}
@DisplayName("success_디바이스가 없는 멤버는 빈 리스트를 반환한다")
@Test
void success_returnsEmptyListForMemberWithoutDevices() {
// when
List<String> tokens = deviceRepository.findAllTokenByMemberId(member3.getId());
// then
assertThat(tokens).isEmpty();
}
}
@DisplayName("여러 멤버 ID로 memberId와 token 쌍을 조회할 때")
@Nested
class FindMemberIdAndTokenByMemberIdIn {
@DisplayName("success_memberId와 token 쌍을 반환한다")
@Test
void success_returnsMemberIdTokenPairs() {
// given
List<Long> memberIds = List.of(member1.getId(), member2.getId());
// when
List<Object[]> pairs = deviceRepository.findMemberIdAndTokenByMemberIdIn(memberIds);
// then
assertSoftly(softly -> {
softly.assertThat(pairs).hasSize(3);
Object[] pair1 = pairs.stream()
.filter(pair -> pair[1].equals("token-1"))
.findFirst()
.orElseThrow();
softly.assertThat(pair1[0]).isEqualTo(member1.getId());
Object[] pair2 = pairs.stream()
.filter(pair -> pair[1].equals("token-2"))
.findFirst()
.orElseThrow();
softly.assertThat(pair2[0]).isEqualTo(member1.getId());
Object[] pair3 = pairs.stream()
.filter(pair -> pair[1].equals("token-3"))
.findFirst()
.orElseThrow();
softly.assertThat(pair3[0]).isEqualTo(member2.getId());
});
}
@DisplayName("success_빈 리스트로 조회 시 빈 리스트를 반환한다")
@Test
void success_returnsEmptyListForEmptyInput() {
// given
List<Long> memberIds = List.of();
// when
List<Object[]> pairs = deviceRepository.findMemberIdAndTokenByMemberIdIn(memberIds);
// then
assertThat(pairs).isEmpty();
}
}
}구현한 테스트의 특징
주의사항OutboxDispatcher 통합 테스트는 제외했습니다. 이유는:
필요하다면 OutboxDispatcher에 대한 단위 테스트나 별도의 통합 테스트 전략에 대해서도 안내드릴 수 있습니다! 🧠 Learnings used |
Contributor
Author
|
@coderabbitai 아웃박스디스패처도 테스트 해볼래 ? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. @CheChe903, OutboxDispatcher 통합 테스트를 작성해드리겠습니다! FcmClient는 실제 Firebase 의존성이 있으므로 OutboxDispatcherIntegrationTestpackage backend.mulkkam.outboxnotification.service;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.SoftAssertions.assertSoftly;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyList;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import backend.mulkkam.common.infrastructure.fcm.service.FcmClient;
import backend.mulkkam.outboxnotification.domain.OutboxNotification;
import backend.mulkkam.outboxnotification.domain.OutboxNotification.Status;
import backend.mulkkam.outboxnotification.repository.OutboxNotificationRepository;
import backend.mulkkam.support.fixture.outbox.OutboxNotificationFixtureBuilder;
import backend.mulkkam.support.service.ServiceIntegrationTest;
import com.google.firebase.messaging.BatchResponse;
import com.google.firebase.messaging.FirebaseMessagingException;
import com.google.firebase.messaging.MessagingErrorCode;
import com.google.firebase.messaging.SendResponse;
import java.time.LocalDateTime;
import java.util.List;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.mock.mockito.MockBean;
class OutboxDispatcherIntegrationTest extends ServiceIntegrationTest {
@Autowired
private OutboxDispatcher outboxDispatcher;
@Autowired
private OutboxNotificationRepository outboxNotificationRepository;
@MockBean
private FcmClient fcmClient;
private OutboxNotification outbox1;
private OutboxNotification outbox2;
private OutboxNotification outbox3;
@BeforeEach
void setUp() {
outbox1 = outboxNotificationRepository.save(
OutboxNotificationFixtureBuilder.builder()
.type("REMIND")
.memberId(1L)
.token("token-1")
.title("물 마실 시간이에요")
.body("오늘도 건강한 하루 되세요!")
.status(Status.READY)
.dedupeKey("REMIND:1:" + LocalDateTime.now())
.build()
);
outbox2 = outboxNotificationRepository.save(
OutboxNotificationFixtureBuilder.builder()
.type("REMIND")
.memberId(2L)
.token("token-2")
.title("물 마실 시간이에요")
.body("오늘도 건강한 하루 되세요!")
.status(Status.READY)
.dedupeKey("REMIND:2:" + LocalDateTime.now())
.build()
);
outbox3 = outboxNotificationRepository.save(
OutboxNotificationFixtureBuilder.builder()
.type("REMIND")
.memberId(3L)
.token("token-3")
.title("물 마실 시간이에요")
.body("오늘도 건강한 하루 되세요!")
.status(Status.READY)
.dedupeKey("REMIND:3:" + LocalDateTime.now())
.build()
);
}
@DisplayName("Outbox를 dispatch할 때")
@Nested
class Dispatch {
@DisplayName("success_FCM 전송 성공 시 모든 outbox가 SENT 상태로 변경된다")
@Test
void success_allOutboxesMarkedAsSentWhenFcmSucceeds() {
// given
BatchResponse batchResponse = createSuccessfulBatchResponse(3);
when(fcmClient.sendMulticast(anyString(), anyString(), anyString(), anyList()))
.thenReturn(batchResponse);
// when
outboxDispatcher.dispatch();
// then
List<OutboxNotification> updatedOutboxes = outboxNotificationRepository.findAll();
assertSoftly(softly -> {
softly.assertThat(updatedOutboxes).hasSize(3);
softly.assertThat(updatedOutboxes)
.allMatch(outbox -> outbox.getStatus() == Status.SENT);
softly.assertThat(updatedOutboxes)
.allMatch(outbox -> outbox.getAttemptCount() == 1);
});
verify(fcmClient, times(1)).sendMulticast(
eq("물 마실 시간이에요"),
eq("오늘도 건강한 하루 되세요!"),
anyString(),
anyList()
);
}
@DisplayName("success_일부 전송 실패 시 영구 실패는 FAIL, 일시 실패는 RETRY로 변경된다")
@Test
void success_mixedResultsHandledCorrectly() {
// given
// token-1: 성공, token-2: 일시적 실패(UNAVAILABLE), token-3: 영구 실패(INVALID_ARGUMENT)
BatchResponse batchResponse = createMixedBatchResponse();
when(fcmClient.sendMulticast(anyString(), anyString(), anyString(), anyList()))
.thenReturn(batchResponse);
// when
outboxDispatcher.dispatch();
// then
OutboxNotification updated1 = outboxNotificationRepository.findById(outbox1.getId()).orElseThrow();
OutboxNotification updated2 = outboxNotificationRepository.findById(outbox2.getId()).orElseThrow();
OutboxNotification updated3 = outboxNotificationRepository.findById(outbox3.getId()).orElseThrow();
assertSoftly(softly -> {
// 성공
softly.assertThat(updated1.getStatus()).isEqualTo(Status.SENT);
softly.assertThat(updated1.getAttemptCount()).isEqualTo(1);
// 일시적 실패 -> RETRY
softly.assertThat(updated2.getStatus()).isEqualTo(Status.RETRY);
softly.assertThat(updated2.getAttemptCount()).isEqualTo(1);
softly.assertThat(updated2.getNextAttemptAt()).isNotNull();
softly.assertThat(updated2.getLastError()).contains("UNAVAILABLE");
// 영구 실패 -> FAIL
softly.assertThat(updated3.getStatus()).isEqualTo(Status.FAIL);
softly.assertThat(updated3.getAttemptCount()).isEqualTo(1);
softly.assertThat(updated3.getLastError()).contains("INVALID_ARGUMENT");
});
}
@DisplayName("success_재시도 시도 횟수가 3회 이상이면 FAIL로 변경된다")
@Test
void success_maxRetriesExceededMarkedAsFail() {
// given
outboxNotificationRepository.deleteAll();
OutboxNotification retryOutbox = outboxNotificationRepository.save(
OutboxNotificationFixtureBuilder.builder()
.type("REMIND")
.memberId(1L)
.token("token-retry")
.title("물 마실 시간이에요")
.body("오늘도 건강한 하루 되세요!")
.status(Status.RETRY)
.attemptCount(2)
.nextAttemptAt(LocalDateTime.now().minusMinutes(1))
.dedupeKey("REMIND:1:retry")
.build()
);
BatchResponse batchResponse = createFailedBatchResponse(1, MessagingErrorCode.UNAVAILABLE);
when(fcmClient.sendMulticast(anyString(), anyString(), anyString(), anyList()))
.thenReturn(batchResponse);
// when
outboxDispatcher.dispatch();
// then
OutboxNotification updated = outboxNotificationRepository.findById(retryOutbox.getId()).orElseThrow();
assertSoftly(softly -> {
softly.assertThat(updated.getStatus()).isEqualTo(Status.FAIL);
softly.assertThat(updated.getAttemptCount()).isEqualTo(3);
softly.assertThat(updated.getLastError()).isNotNull();
});
}
@DisplayName("success_RETRY 상태이면서 nextAttemptAt이 아직 도래하지 않은 경우 dispatch하지 않는다")
@Test
void success_doesNotDispatchWhenNextAttemptNotReached() {
// given
outboxNotificationRepository.deleteAll();
outboxNotificationRepository.save(
OutboxNotificationFixtureBuilder.builder()
.type("REMIND")
.memberId(1L)
.token("token-future")
.title("물 마실 시간이에요")
.body("오늘도 건강한 하루 되세요!")
.status(Status.RETRY)
.attemptCount(1)
.nextAttemptAt(LocalDateTime.now().plusHours(1))
.dedupeKey("REMIND:1:future")
.build()
);
// when
outboxDispatcher.dispatch();
// then
verify(fcmClient, never()).sendMulticast(anyString(), anyString(), anyString(), anyList());
}
@DisplayName("success_SENDING 상태의 outbox는 재처리되지 않는다")
@Test
void success_doesNotReprocessSendingStatus() {
// given
outboxNotificationRepository.deleteAll();
outboxNotificationRepository.save(
OutboxNotificationFixtureBuilder.builder()
.type("REMIND")
.memberId(1L)
.token("token-sending")
.title("물 마실 시간이에요")
.body("오늘도 건강한 하루 되세요!")
.status(Status.SENDING)
.attemptCount(1)
.dedupeKey("REMIND:1:sending")
.build()
);
// when
outboxDispatcher.dispatch();
// then
verify(fcmClient, never()).sendMulticast(anyString(), anyString(), anyString(), anyList());
}
@DisplayName("success_대량의 outbox를 배치 단위로 처리한다")
@Test
void success_processesBatchesCorrectly() {
// given
outboxNotificationRepository.deleteAll();
// 600개의 outbox 생성 (multicast는 보통 500개 단위)
for (int i = 1; i <= 600; i++) {
outboxNotificationRepository.save(
OutboxNotificationFixtureBuilder.builder()
.type("REMIND")
.memberId((long) i)
.token("token-" + i)
.title("물 마실 시간이에요")
.body("오늘도 건강한 하루 되세요!")
.status(Status.READY)
.dedupeKey("REMIND:" + i + ":batch")
.build()
);
}
BatchResponse batchResponse = createSuccessfulBatchResponse(500);
when(fcmClient.sendMulticast(anyString(), anyString(), anyString(), anyList()))
.thenReturn(batchResponse);
// when
outboxDispatcher.dispatch();
// then
// multicast는 500개 단위이므로 최소 2번 호출되어야 함
verify(fcmClient, times(2)).sendMulticast(
anyString(),
anyString(),
anyString(),
anyList()
);
List<OutboxNotification> processedOutboxes = outboxNotificationRepository.findAll();
assertThat(processedOutboxes)
.allMatch(outbox -> outbox.getStatus() == Status.SENT);
}
@DisplayName("success_빈 outbox 큐에서 dispatch 시 아무 동작도 하지 않는다")
@Test
void success_noActionWhenQueueIsEmpty() {
// given
outboxNotificationRepository.deleteAll();
// when
outboxDispatcher.dispatch();
// then
verify(fcmClient, never()).sendMulticast(anyString(), anyString(), anyString(), anyList());
}
@DisplayName("success_첫 번째 outbox의 title과 body를 사용하여 multicast를 전송한다")
@Test
void success_usesTitleAndBodyFromFirstOutbox() {
// given
BatchResponse batchResponse = createSuccessfulBatchResponse(3);
when(fcmClient.sendMulticast(anyString(), anyString(), anyString(), anyList()))
.thenReturn(batchResponse);
// when
outboxDispatcher.dispatch();
// then
verify(fcmClient).sendMulticast(
eq("물 마실 시간이에요"),
eq("오늘도 건강한 하루 되세요!"),
anyString(),
anyList()
);
}
}
// Helper methods for creating BatchResponse
private BatchResponse createSuccessfulBatchResponse(int count) {
List<SendResponse> responses = java.util.stream.IntStream.range(0, count)
.mapToObj(i -> Mockito.mock(SendResponse.class))
.peek(response -> when(response.isSuccessful()).thenReturn(true))
.toList();
BatchResponse batchResponse = Mockito.mock(BatchResponse.class);
when(batchResponse.getResponses()).thenReturn(responses);
when(batchResponse.getSuccessCount()).thenReturn(count);
when(batchResponse.getFailureCount()).thenReturn(0);
return batchResponse;
}
private BatchResponse createMixedBatchResponse() {
// token-1: 성공
SendResponse success = Mockito.mock(SendResponse.class);
when(success.isSuccessful()).thenReturn(true);
// token-2: 일시적 실패 (UNAVAILABLE)
SendResponse transientFailure = Mockito.mock(SendResponse.class);
when(transientFailure.isSuccessful()).thenReturn(false);
FirebaseMessagingException transientException = Mockito.mock(FirebaseMessagingException.class);
when(transientException.getMessagingErrorCode()).thenReturn(MessagingErrorCode.UNAVAILABLE);
when(transientException.getMessage()).thenReturn("Service unavailable");
when(transientFailure.getException()).thenReturn(transientException);
// token-3: 영구 실패 (INVALID_ARGUMENT)
SendResponse permanentFailure = Mockito.mock(SendResponse.class);
when(permanentFailure.isSuccessful()).thenReturn(false);
FirebaseMessagingException permanentException = Mockito.mock(FirebaseMessagingException.class);
when(permanentException.getMessagingErrorCode()).thenReturn(MessagingErrorCode.INVALID_ARGUMENT);
when(permanentException.getMessage()).thenReturn("Invalid token");
when(permanentFailure.getException()).thenReturn(permanentException);
List<SendResponse> responses = List.of(success, transientFailure, permanentFailure);
BatchResponse batchResponse = Mockito.mock(BatchResponse.class);
when(batchResponse.getResponses()).thenReturn(responses);
when(batchResponse.getSuccessCount()).thenReturn(1);
when(batchResponse.getFailureCount()).thenReturn(2);
return batchResponse;
}
private BatchResponse createFailedBatchResponse(int count, MessagingErrorCode errorCode) {
List<SendResponse> responses = java.util.stream.IntStream.range(0, count)
.mapToObj(i -> {
SendResponse response = Mockito.mock(SendResponse.class);
when(response.isSuccessful()).thenReturn(false);
FirebaseMessagingException exception = Mockito.mock(FirebaseMessagingException.class);
when(exception.getMessagingErrorCode()).thenReturn(errorCode);
when(exception.getMessage()).thenReturn("Error: " + errorCode);
when(response.getException()).thenReturn(exception);
return response;
})
.toList();
BatchResponse batchResponse = Mockito.mock(BatchResponse.class);
when(batchResponse.getResponses()).thenReturn(responses);
when(batchResponse.getSuccessCount()).thenReturn(0);
when(batchResponse.getFailureCount()).thenReturn(count);
return batchResponse;
}
}테스트가 검증하는 핵심 시나리오1. 성공 케이스
2. 혼합 결과 처리
3. 재시도 로직
4. 배치 처리
5. 상태 관리
테스트의 특징
이 테스트로 OutboxDispatcher의 핵심 로직인 배치 처리, 상태 변화, 재시도, 에러 처리를 모두 검증할 수 있습니다! 🧠 Learnings used |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
backend/src/main/java/backend/mulkkam/common/exception/errorCode/FirebaseErrorCode.java (1)
38-42: 예외 처리 전략을 명확히 하거나, 방어 코드 추가를 고려해보세요.현재
isPermanentError메서드는 내부적으로findByName을 호출하는데, 이 메서드는 매칭되는 enum 값이 없으면CommonException을 던집니다(Line 35).잠재적 문제:
- OutboxDispatcher의 에러 처리 흐름(Line 82)에서 호출되므로, Firebase가 예상치 못한 에러 코드를 반환하면 예외가 전파되어 해당 outbox 항목이 SENDING 상태에 갇힐 수 있습니다.
- 특히 Firebase SDK 버전이 업그레이드되면서 새로운 에러 코드가 추가되는 경우, 기존 코드가 처리하지 못하는 상황이 발생할 수 있습니다.
대안 제안:
보수적 접근: 알 수 없는 에러는 일시적 오류로 간주 (권장)
- 장점: 시스템이 예외 없이 계속 작동하며, 알 수 없는 에러는 재시도 로직을 탑니다.
- 단점: 실제로는 영구 오류인데 재시도될 수 있으나, 최대 재시도 횟수 제한으로 결국 FAIL 처리됩니다.
public static boolean isPermanentError(String error) { try { FirebaseErrorCode errorCode = findByName(error); return errorCode == UNREGISTERED || errorCode == INVALID_ARGUMENT || errorCode == SENDER_ID_MISMATCH || errorCode == THIRD_PARTY_AUTH_ERROR; } catch (CommonException e) { // 알 수 없는 에러 코드는 일시적 오류로 간주하여 재시도 return false; } }명시적 접근: Optional 기반 안전한 조회 메서드 추가
- 기존
findByName은 유지하고, null-safe variant를 별도로 만듭니다.- 장점: 기존 코드 영향 없이 새로운 안전한 API 제공.
- 단점: 메서드가 하나 더 늘어남.
현재는 OutboxDispatcher가 "UNKNOWN" 폴백(Line 80)을 제공하므로 실제로는 예외가 발생하지 않지만, 향후 다른 곳에서 이 메서드를 재사용할 때를 대비해 방어 코드를 추가하는 것이 안전합니다.
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java (1)
97-100: 백오프 계산 공식에 작은 개선 여지가 있습니다.현재 구현:
long sec = Math.min(60, (long) (2 * Math.pow(2, attempt)));동작 방식:
- attempt=0: 2초
- attempt=1: 4초
- attempt=2: 8초
- attempt=3: 16초
- attempt=4: 32초
- attempt=5 이상: 60초 (cap)
개선 제안 (선택사항):
초기 대기 시간을 더 짧게 시작
- 일시적 네트워크 지연은 수 초 내에 복구되는 경우가 많으므로, 첫 재시도를 좀 더 빠르게 하는 것도 고려할 수 있습니다.
// 1초부터 시작 (1, 2, 4, 8, 16, 32, 60...) long sec = Math.min(60, (long) Math.pow(2, attempt));jitter 추가로 재시도 충돌 방지
- 여러 outbox 항목이 동시에 실패하면 모두 같은 시간에 재시도되어 "thundering herd" 문제가 발생할 수 있습니다.
long baseSec = Math.min(60, (long) Math.pow(2, attempt)); long jitter = ThreadLocalRandom.current().nextLong(0, baseSec / 4 + 1); return LocalDateTime.now().plusSeconds(baseSec + jitter);
- 장점: 재시도가 시간적으로 분산되어 시스템 부하 완화
- 단점: 구현 복잡도 약간 증가
현재 구현도 충분히 합리적이지만, 프로덕션 환경에서 재시도 패턴을 모니터링한 후 필요시 조정하는 것을 권장합니다.
backend/src/test/java/backend/mulkkam/outboxnotification/service/OutboxNotificationServiceIntegrationTest.java (3)
49-74: setUp 구조가 깔끔하고, BeforeEach 활용이 잘 되어 있습니다! 👍테스트마다 재사용되는 Member와 Device fixture를 BeforeEach에서 초기화하여 가독성과 유지보수성을 높였습니다. 특히
outboxNotificationRepository.deleteAll()을 먼저 호출하여 테스트 간 격리도 잘 보장하고 있습니다.작은 개선 제안:
현재 member3는 생성되지만 device를 할당받지 않는데, "디바이스가 없는 멤버" 테스트(Lines 168-190)에서는 별도의
memberWithoutDevice를 새로 생성하고 있습니다. 일관성을 위해 다음 중 하나를 고려해보세요:
member3를 device 없는 멤버로 활용 (약간 더 효율적)
// member3에는 device를 할당하지 않음 // Line 171-174를 아래로 교체: List<Long> memberIds = List.of(member3.getId());member3에도 device4를 할당 (현재 패턴 유지)
- 모든 setUp 데이터에 일관되게 device를 할당하고
- 특수한 케이스는 각 테스트에서 별도로 생성 (현재 방식)
현재 방식도 명확하지만, member3가 setUp에서 생성되었는데 실제로는 사용되지 않는 점이 살짝 혼란스러울 수 있습니다.
Based on learnings
80-114: 테스트가 핵심 시나리오를 잘 검증하고 있습니다.여러 멤버와 디바이스를 조합한 시나리오에서 outbox 생성을 검증하며, assertSoftly를 활용해 여러 조건을 한 번에 확인하는 구조가 좋습니다.
추가 고려사항:
현재 테스트는 "3개의 outbox가 생성된다"는 것을 검증하지만, 어느 멤버의 어느 토큰으로 생성되었는지는 검증하지 않습니다.
예를 들어:
- member1 → device1 (token-1), device2 (token-2)
- member2 → device3 (token-3)
- member3 → (device 없음)
결과적으로 3개의 outbox가 생성되는데, 그 토큰이 정확히 "token-1", "token-2", "token-3"인지 확인하면 더욱 견고한 테스트가 됩니다.
softly.assertThat(savedOutboxes) .extracting(OutboxNotification::getToken) .containsExactlyInAnyOrder("token-1", "token-2", "token-3");Lines 116-144의 두 번째 테스트에서는 이미 토큰을 검증하고 있으므로, 일관성을 위해 여기서도 추가하는 것을 고려해보세요.
168-190: DisplayName의 접두사 "success_"를 제거하는 것이 더 자연스럽습니다.Line 168:
@DisplayName("success_멤버에게 등록된 디바이스가 없으면 outbox가 생성되지 않는다")문제점:
- DisplayName은 테스트 실행 시 사람이 읽는 설명이므로, 메서드명 컨벤션(
success_,fail_등)을 그대로 가져올 필요가 없습니다.- "success_"가 있으면 테스트 리포트에서 가독성이 떨어집니다.
개선:
@DisplayName("멤버에게 등록된 디바이스가 없으면 outbox가 생성되지 않는다") @Test void success_noOutboxCreatedWhenMemberHasNoDevices() {이렇게 하면:
- 메서드명은 코드 컨벤션을 따르고
- DisplayName은 자연스러운 한국어 문장으로 표시됩니다
같은 패턴이 여러 테스트에 적용되어 있으니, 프로젝트 전체의 테스트 DisplayName 스타일을 통일하는 것을 권장합니다.
backend/src/test/java/backend/mulkkam/outboxnotification/service/OutboxDispatcherIntegrationTest.java (2)
216-240: SENDING 상태 재처리 방지 테스트가 중요한 엣지 케이스를 다룹니다.디스패처가 처리 중 중단되면 SENDING 상태로 남을 수 있는데, 이를 재처리하지 않는지 확인하는 테스트입니다.
하지만 실제 프로덕션 고려사항:
SENDING 상태로 오랫동안 남아있는 항목은 "stuck" 상태일 가능성이 높습니다. 예를 들어:
- 디스패처가 FCM 호출 직후 크래시
- DB 업데이트 전 프로세스 종료
권장 보완 로직:
Repository의
fetchReadyForSend에서:WHERE (status = 'READY' OR status = 'RETRY') OR (status = 'SENDING' AND updated_at < NOW() - INTERVAL 10 MINUTE)이렇게 하면:
- 정상적으로 처리 중인 SENDING은 제외
- 10분 이상 SENDING인 "멈춘" 항목은 복구
테스트 추가 제안:
@DisplayName("SENDING 상태가 오래된 경우 재처리된다 (stuck 복구)") @Test void success_reprocessesStuckSendingStatus() { outboxNotificationRepository.deleteAll(); OutboxNotification stuckOutbox = outboxNotificationRepository.save( OutboxNotificationFixtureBuilder.builder() .status(Status.SENDING) .attemptCount(1) // createdAt을 10분 전으로 설정하는 방법 필요 .build() ); // 시간을 앞당기는 로직이 필요하거나, Repository에 테스트용 메서드 추가 BatchResponse batchResponse = createSuccessfulBatchResponse(1); when(fcmClient.sendMulticast(...)).thenReturn(batchResponse); outboxDispatcher.dispatch(); OutboxNotification updated = outboxNotificationRepository.findById(stuckOutbox.getId()).orElseThrow(); assertThat(updated.getStatus()).isEqualTo(Status.SENT); }현재는 SENDING을 완전히 건너뛰는데, 실제 운영에서는 stuck 복구 로직이 있는지 확인하고, 있다면 테스트도 추가하는 것을 권장합니다.
276-288: Mock 응답 생성 헬퍼 메서드가 중복을 잘 제거했습니다.여러 테스트에서 재사용 가능한 깔끔한 구조입니다.
작은 개선 제안:
Line 277-280의 스트림 체이닝을 좀 더 간결하게:
private BatchResponse createSuccessfulBatchResponse(int count) { List<SendResponse> responses = IntStream.range(0, count) .mapToObj(i -> { SendResponse response = Mockito.mock(SendResponse.class); when(response.isSuccessful()).thenReturn(true); return response; }) .toList(); // ... 나머지 동일 }현재도 충분히 읽기 쉽지만, peek를 사용하는 것보다 mapToObj 내부에서 설정하는 것이 의도가 더 명확합니다.
backend/src/main/resources/db/migration/common/V20251101_213000__add_outbox_notification_table.sql (1)
28-35: 인덱스 전략이 조회 패턴을 잘 반영하고 있습니다.분석:
idx_outbox_status_nextAttempt(status, next_attempt_at) (Lines 28-29)
- OutboxDispatcher의
fetchReadyForSend쿼리에 최적화WHERE status IN ('READY', 'RETRY') AND (next_attempt_at IS NULL OR next_attempt_at <= NOW())- 복합 인덱스로 효율적인 필터링 가능 ✓
idx_outbox_memberId(member_id) (Lines 31-32)
- 특정 회원의 알림 이력 조회 시 사용
- 관리자 기능이나 디버깅 용도로 유용 ✓
idx_outbox_token(token) (Lines 34-35)
- 특정 토큰의 발송 이력 조회 시 사용
- 토큰별 통계나 문제 추적 용도로 유용 ✓
추가 고려사항:
시간이 지나면 outbox 테이블이 계속 커집니다. SENT/FAIL 상태의 오래된 레코드를 정리하는 전략이 필요합니다:
옵션 1: 주기적 삭제 (Purge) 작업
-- 30일 이상 된 완료/실패 항목 삭제 DELETE FROM outbox_notification WHERE status IN ('SENT', 'FAIL') AND created_at < NOW() - INTERVAL 30 DAY LIMIT 1000;옵션 2: Partitioning
- created_at 기준으로 월별 파티션 생성
- 오래된 파티션을 통째로 DROP
옵션 3: 아카이빙
- SENT/FAIL 항목을 별도 아카이브 테이블로 이동
- 메인 테이블은 READY/SENDING/RETRY만 유지하여 가볍게
이런 정리 전략을 초기부터 계획해두면, 나중에 성능 이슈를 겪지 않습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/src/main/java/backend/mulkkam/common/exception/errorCode/FirebaseErrorCode.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java(1 hunks)backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java(1 hunks)backend/src/main/resources/db/migration/common/V20251101_213000__add_outbox_notification_table.sql(1 hunks)backend/src/test/java/backend/mulkkam/outboxnotification/service/OutboxDispatcherIntegrationTest.java(1 hunks)backend/src/test/java/backend/mulkkam/outboxnotification/service/OutboxNotificationServiceIntegrationTest.java(1 hunks)backend/src/test/java/backend/mulkkam/support/fixture/OutboxNotificationFixtureBuilder.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java
🧰 Additional context used
📓 Path-based instructions (1)
backend/**
⚙️ CodeRabbit configuration file
backend/**: - 1. 팀 및 공식 컨벤션, 가독성, 예외처리, 테스트/확장/유지보수성, 모듈화, API/DB/보안 설계 기준을 기반으로 리뷰해주세요.
- 최신 트렌드, 불필요한 로직, 클린코드, 리팩토링, 서비스/도메인 설계, 공통 예외 처리, 확장성도 함께 확인해주세요.
- 각 피드백은 문제점·대안·장단점을 짧고 논리적으로, 예시 코드가 있다면 간결히 포함해 주세요.
- 팀 내 스타일 통일성도 확인해주세요.
- 미작성한 테스트 코드 케이스가 있다면, 어떤 테스트가 필요한지 제안해주세요. (예: 컨트롤러는 인수 테스트, 나머지는 단위 테스트)
- 리뷰 남겨주는 부분은 해당 라인 범위의 코멘트에 작성해주세요.
Files:
backend/src/main/java/backend/mulkkam/common/exception/errorCode/FirebaseErrorCode.javabackend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.javabackend/src/test/java/backend/mulkkam/outboxnotification/service/OutboxDispatcherIntegrationTest.javabackend/src/test/java/backend/mulkkam/support/fixture/OutboxNotificationFixtureBuilder.javabackend/src/test/java/backend/mulkkam/outboxnotification/service/OutboxNotificationServiceIntegrationTest.javabackend/src/main/resources/db/migration/common/V20251101_213000__add_outbox_notification_table.sql
🧠 Learnings (1)
📓 Common learnings
Learnt from: CheChe903
Repo: woowacourse-teams/2025-mul-kkam PR: 739
File: backend/src/main/java/backend/mulkkam/auth/infrastructure/OauthJwtTokenHandler.java:56-64
Timestamp: 2025-09-10T04:51:38.314Z
Learning: CheChe903 사용자는 테스트 코드에서 BeforeEach를 활용해서 가독성을 향상시킬 수 있는 부분이 있으면 리뷰해달라고 요청했습니다. 앞으로 테스트 코드 리뷰 시 중복되는 객체 생성, 테스트 데이터 셋업, Mock 설정 등을 BeforeEach로 추출할 수 있는 리팩터링 기회를 적극적으로 제안해야 합니다.
Learnt from: CheChe903
Repo: woowacourse-teams/2025-mul-kkam PR: 739
File: backend/src/test/java/backend/mulkkam/auth/controller/AuthControllerTest.java:113-114
Timestamp: 2025-09-10T04:46:55.219Z
Learning: CheChe903 has instructed not to consider Android code compatibility when reviewing backend code in the woowacourse-teams/2025-mul-kkam repository. Focus only on backend code concerns and do not flag client-server DTO mismatches or Android-related issues.
🧬 Code graph analysis (3)
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java (2)
backend/src/main/java/backend/mulkkam/notification/service/NotificationService.java (1)
Transactional(39-276)backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxNotificationService.java (1)
Service(15-61)
backend/src/test/java/backend/mulkkam/outboxnotification/service/OutboxDispatcherIntegrationTest.java (1)
backend/src/test/java/backend/mulkkam/support/fixture/OutboxNotificationFixtureBuilder.java (1)
OutboxNotificationFixtureBuilder(7-91)
backend/src/test/java/backend/mulkkam/outboxnotification/service/OutboxNotificationServiceIntegrationTest.java (2)
backend/src/test/java/backend/mulkkam/support/fixture/DeviceFixtureBuilder.java (1)
DeviceFixtureBuilder(6-46)backend/src/test/java/backend/mulkkam/support/fixture/member/MemberFixtureBuilder.java (1)
MemberFixtureBuilder(9-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (7)
backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java (1)
28-37:@Transactional추가로 동시성 문제가 해결되었습니다! 👍이전 리뷰에서 지적된 SKIP LOCKED 트랜잭션 경계 문제가
@Transactional애노테이션 추가로 해결되었습니다. 이제fetchReadyForSend의SELECT ... FOR UPDATE SKIP LOCKED와markSending호출이 동일한 트랜잭션 내에서 실행되어, 멀티 인스턴스 환경에서도 중복 처리를 효과적으로 방지할 수 있습니다.추가 고려사항:
트랜잭션이 최대 500건(MULTICAST_SIZE)을 한 번에 처리하므로, FCM 호출이 느리거나 실패 시 전체 배치가 롤백될 수 있습니다. 현재 구조에서는:
- 장점: 구현이 단순하고 데이터 일관성이 보장됩니다.
- 단점: 부분 실패 시 전체 배치를 다시 처리해야 합니다.
대부분의 outbox 패턴에서는 이 정도 배치 크기가 적절하지만, 향후 처리량이 급증하면 배치 크기를 줄이거나 내부적으로 더 작은 단위로 나누는 것을 고려해볼 수 있습니다.
backend/src/test/java/backend/mulkkam/outboxnotification/service/OutboxNotificationServiceIntegrationTest.java (1)
192-226: 시간 기반 dedupe 동작을 검증하는 좋은 테스트입니다.다른 시간에 enqueue하면 별도의 outbox가 생성되는 것을 확인하고, dedupe key 포맷까지 검증하여 테스트 커버리지가 탄탄합니다.
한 가지 의문:
Lines 220, 222에서 dedupe key를 검증할 때:
softly.assertThat(dedupeKeys) .contains("REMIND:" + member1.getId() + ":" + time1 + ":" + device1.getToken()); softly.assertThat(dedupeKeys) .contains("REMIND:" + member1.getId() + ":" + time2 + ":" + device2.getToken());의문점:
- Line 220은 time1 + device1.getToken()
- Line 222는 time2 + device2.getToken()
member1은 device1과 device2를 모두 가지고 있으므로, time1에도 device1, device2 토큰으로 각각 outbox가 생성되어야 합니다. 현재 검증은:
- time1 + device1 ✓
- time2 + device2 ✓
- member2 + time1 ✓
하지만 누락된 조합들:
- time1 + device2
- time2 + device1
이 부분이 의도적으로 일부만 샘플링한 것인지, 아니면 모든 조합을 검증해야 하는지 확인해보세요. 완전한 검증을 원한다면:
assertSoftly(softly -> { softly.assertThat(dedupeKeys).hasSize(6); // (2 members × 2 times) + (1 member × 2 times) = 6 // member1 × time1 × device1, device2 softly.assertThat(dedupeKeys).contains( "REMIND:" + member1.getId() + ":" + time1 + ":" + device1.getToken(), "REMIND:" + member1.getId() + ":" + time1 + ":" + device2.getToken() ); // member1 × time2 × device1, device2 softly.assertThat(dedupeKeys).contains( "REMIND:" + member1.getId() + ":" + time2 + ":" + device1.getToken(), "REMIND:" + member1.getId() + ":" + time2 + ":" + device2.getToken() ); // member2 × time1, time2 × device3 softly.assertThat(dedupeKeys).contains( "REMIND:" + member2.getId() + ":" + time1 + ":" + device3.getToken(), "REMIND:" + member2.getId() + ":" + time2 + ":" + device3.getToken() ); });backend/src/test/java/backend/mulkkam/support/fixture/OutboxNotificationFixtureBuilder.java (1)
7-25: Fixture Builder 구조가 깔끔하고 재사용성이 좋습니다! 👍빌더 패턴을 활용해 테스트에서 필요한 필드만 오버라이드하고 나머지는 합리적인 기본값을 사용하도록 설계되어 있습니다. private 생성자와 public static factory 메서드를 사용한 것도 좋은 관례입니다.
backend/src/test/java/backend/mulkkam/outboxnotification/service/OutboxDispatcherIntegrationTest.java (3)
40-83: Mock 기반 통합 테스트 설계가 훌륭합니다! 👍
@MockitoBean을 활용해 Firebase 의존성을 제거하면서도, 실제 Repository와 Service 레이어를 통합하여 테스트하는 구조가 적절합니다.장점:
- 외부 서비스(Firebase) 없이도 빠르고 안정적으로 테스트 가능
- 다양한 FCM 응답 시나리오(성공/일시 실패/영구 실패)를 쉽게 시뮬레이션
- setUp에서 테스트 데이터를 명확히 준비하여 가독성 향상
추가 고려사항:
현재 setUp에서 dedupe key에
LocalDateTime.now()를 사용하고 있는데, 이는 OutboxNotificationFixtureBuilder의 기본값 이슈와 연결됩니다(이전 코멘트 참조). 여기서는 명시적으로 값을 설정하고 있으므로 문제는 없지만, 일관성을 위해 고정된 시간을 사용하는 것도 고려해보세요:LocalDateTime fixedTime = LocalDateTime.of(2025, 11, 18, 14, 30); .dedupeKey("REMIND:1:" + fixedTime + ":token-1")Based on learnings
119-151: 혼합 결과 시나리오 테스트가 핵심 로직을 잘 검증합니다.성공, 일시 실패(UNAVAILABLE), 영구 실패(INVALID_ARGUMENT)를 모두 포함한 시나리오로, 실제 프로덕션에서 발생할 수 있는 상황을 잘 반영하고 있습니다.
검증 항목이 완벽합니다:
- 각 outbox의 status 변화 ✓
- attemptCount 증가 ✓
- RETRY의 경우 nextAttemptAt 설정 확인 ✓
- lastError 메시지 저장 확인 ✓
추가 검증 제안 (선택사항):
Line 144에서
lastError가 "UNAVAILABLE"을 포함하는지만 확인하는데, OutboxDispatcher의 현재 구현(Line 85)을 보면error값이 그대로 저장됩니다. 따라서:softly.assertThat(updated2.getLastError()).isEqualTo("UNAVAILABLE");정확한 일치를 검증하는 것이 더 명확할 수 있습니다. (메시지 포맷이 변경될 경우를 대비한다면
contains도 합리적이긴 합니다.)
153-187: 원본 리뷰 코멘트는 재시도 횟수 제한에 대한 잘못된 가정에 기반했습니다.실제 Repository 코드를 확인한 결과,
attemptCount < 3 THEN 'RETRY'로 설정되어 있습니다. 즉, 최대 재시도 횟수는 3회입니다.현재 테스트는 이 경계값을 정확하게 검증하고 있습니다:
attemptCount=2→ 한 번 더 실패하면 3으로 증가 →Status.FAIL로 전환원본 리뷰에서 제안한 "attemptCount=7 → 8"은 실제 로직과 맞지 않습니다. 현재 테스트는 올바르게 경계 조건을 테스트하고 있으므로 변경할 필요가 없습니다.
Likely an incorrect or invalid review comment.
backend/src/main/resources/db/migration/common/V20251101_213000__add_outbox_notification_table.sql (1)
24-26: CHECK 제약조건으로 데이터 무결성을 강제하는 것이 좋습니다! 👍CHECK (status IN ('READY', 'SENDING', 'SENT', 'RETRY', 'FAIL'))애플리케이션 레벨의 enum과 DB 레벨의 제약을 모두 두어 이중으로 보호하는 전략이 훌륭합니다.
추가 고려사항:
향후 상태 값을 추가/변경할 때는:
- 마이그레이션으로 CHECK 제약조건 수정
- Java enum 업데이트
- 기존 데이터 마이그레이션(필요 시)
세 가지를 모두 동기화해야 한다는 점을 기억하세요. 팀 내에서 이런 변경 프로세스를 문서화해두면 좋습니다.
Comment on lines
+42
to
+54
| String title = jobs.getFirst().getTitle(); | ||
| String body = jobs.getFirst().getBody(); | ||
|
|
||
| List<List<Integer>> chunks = chunkIndices(tokens.size(), MULTICAST_SIZE); | ||
|
|
||
| for (List<Integer> chunk : chunks) { | ||
|
|
||
| List<String> chunkTokens = chunk.stream() | ||
| .map(tokens::get) | ||
| .toList(); | ||
|
|
||
| BatchResponse response = | ||
| fcmClient.sendMulticast(title, body, "REMIND", chunkTokens); |
Contributor
There was a problem hiding this comment.
서로 다른 알림 타입/템플릿이 섞일 경우 잘못된 메시지가 발송될 수 있습니다.
현재 구조는 다음과 같이 작동합니다:
String title = jobs.getFirst().getTitle(); // 첫 번째 job의 title 사용
String body = jobs.getFirst().getBody(); // 첫 번째 job의 body 사용
...
fcmClient.sendMulticast(title, body, "REMIND", chunkTokens); // type도 "REMIND"로 고정문제 상황:
fetchReadyForSend가 READY/RETRY 상태의 항목을 시간순으로 가져오는데, 만약:
- 1번 job: "물 마실 시간이에요" (REMIND)
- 2번 job: "새로운 기능이 추가되었습니다" (MARKETING)
- 3번 job: "시스템 점검 안내" (SYSTEM)
이렇게 섞여 있으면, 2번과 3번 사용자도 모두 "물 마실 시간이에요"라는 메시지를 받게 됩니다.
현재는 괜찮지만, 향후 확장 시 구조적 문제:
지금은 REMIND 알림만 outbox를 사용하고 있지만, 이후 다른 알림 타입도 outbox를 공유하게 되면 심각한 버그가 됩니다.
대안 설계:
-
단기: 쿼리 레벨에서 타입 제한 (최소 변경)
fetchReadyForSend에 type 파라미터를 추가하고, 쿼리에WHERE type = ?조건 추가- 장점: 코드 변경이 적고, 현재 전제를 명확히 보장
- 단점: 타입별로 디스패처를 여러 번 실행해야 하고, 배치 효율성이 떨어질 수 있음
-
장기: (type, title, body) 기준으로 그룹핑 후 FCM 호출 (권장)
jobs를Map<TemplateKey, List<OutboxNotification>>형태로 그룹핑- 각 그룹마다 별도의
sendMulticast호출 - 장점:
- 여러 타입/템플릿이 공존해도 안전하게 처리
- 멀티캐스트 이점(동일 메시지를 여러 토큰에)을 유지하면서 정확성 보장
- 단점: 구현 복잡도 증가, FCM 호출 횟수 증가 가능성
Map<TemplateKey, List<OutboxNotification>> grouped = jobs.stream() .collect(Collectors.groupingBy(job -> new TemplateKey(job.getType(), job.getTitle(), job.getBody()))); for (Map.Entry<TemplateKey, List<OutboxNotification>> entry : grouped.entrySet()) { TemplateKey key = entry.getKey(); List<String> tokens = entry.getValue().stream() .map(OutboxNotification::getToken) .toList(); // tokens를 MULTICAST_SIZE로 청킹하여 FCM 호출 List<List<Integer>> chunks = chunkIndices(tokens.size(), MULTICAST_SIZE); for (List<Integer> chunk : chunks) { fcmClient.sendMulticast(key.title, key.body, key.type, ...); } }
권장사항:
최소한 Repository의 fetchReadyForSend 메서드가 어떤 전제로 데이터를 가져오는지 명확히 문서화하거나, 향후 확장을 고려한다면 지금 그룹핑 기반 설계를 도입하는 것이 안전합니다.
Comment on lines
+60
to
+95
| private void handleFcmResponse( | ||
| List<OutboxNotification> jobs, | ||
| List<Integer> chunkIndexes, | ||
| BatchResponse response | ||
| ) { | ||
| List<SendResponse> results = response.getResponses(); | ||
|
|
||
| for (int j = 0; j < chunkIndexes.size(); j++) { | ||
|
|
||
| int idx = chunkIndexes.get(j); | ||
| OutboxNotification job = jobs.get(idx); | ||
|
|
||
| SendResponse result = results.get(j); | ||
|
|
||
| if (result.isSuccessful()) { | ||
| outboxRepository.markSent(job.getId()); | ||
| continue; | ||
| } | ||
|
|
||
| MessagingErrorCode errorCode = result.getException().getMessagingErrorCode(); | ||
| String error = (errorCode != null) ? errorCode.name() : "UNKNOWN"; | ||
|
|
||
| if (isPermanentError(error)) { | ||
| outboxRepository.markFail( | ||
| job.getId(), | ||
| error | ||
| ); | ||
| } else { | ||
| outboxRepository.markRetryOrFail( | ||
| job.getId(), | ||
| nextBackoffTime(job.getAttemptCount()), | ||
| error | ||
| ); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
에러 처리 로직이 이전보다 개선되었지만, 백오프 계산 기준을 확인해주세요.
개선된 부분:
- Line 79-80에서
MessagingErrorCode가 null일 경우 "UNKNOWN"으로 폴백 - NPE 방지 ✓ - Line 82에서
isPermanentError(error)로 구조화된 에러 코드 사용 - 이전reason사용 문제 해결 ✓
확인 필요:
Line 90에서 nextBackoffTime(job.getAttemptCount())를 호출하는데, 이 시점의 attemptCount와 DB의 값이 정확히 일치하는지 확인이 필요합니다.
흐름 분석:
dispatch()시작 시:job.getAttemptCount()는 DB에서 읽어온 값 (예: 0)- Line 37:
markSending(job.getId())에서 DB의attemptCount증가 (예: 0 → 1) - 하지만 메모리의
job객체는 여전히 0을 유지 - Line 90:
nextBackoffTime(0)으로 계산 - DB는 1이지만 백오프는 0 기준
잠재적 문제:
- 백오프 간격이 실제 시도 횟수보다 한 단계 짧게 계산될 수 있습니다.
- 예: 첫 번째 실패 시 DB는 attemptCount=1이지만 백오프는 attempt=0 기준으로 2초, 두 번째 실패 시 DB는 attemptCount=2이지만 백오프는 attempt=1 기준으로 4초
확인 방법:
Repository의 markSending, markRetryOrFail 구현을 확인하여:
markSending에서 attemptCount를 증가시키는지?markRetryOrFail에서도 추가로 증가시키는지?
만약 두 곳에서 모두 증가시킨다면, 백오프 계산 로직을 다음 중 하나로 조정해야 합니다:
대안 1: 증가된 attemptCount 반영
// markSending이 증가된 값을 반환하도록 수정하거나
int currentAttempt = job.getAttemptCount() + 1;
outboxRepository.markRetryOrFail(
job.getId(),
nextBackoffTime(currentAttempt),
error
);대안 2: Repository에서 증분 전략 정리
markSending에서는 attemptCount를 증가시키지 않고markRetryOrFail또는markSent에서만 증가시키는 방식으로 통일
어느 쪽이든, DB의 attemptCount 증분 시점과 백오프 계산 기준이 명확히 일치하도록 맞춰주는 것이 중요합니다.
다음 스크립트로 Repository의 증분 로직을 확인해보세요:
🏁 Script executed:
#!/bin/bash
# Repository의 attemptCount 증분 로직 확인
rg -nA10 "markSending|markRetryOrFail|markSent|markFail" backend/src/main/java/backend/mulkkam/outboxnotification/repository/Length of output: 4468
Repository 메서드의 전체 구현을 확인하기 위해 추가 정보가 필요합니다.
추가 스크립트가 필요합니다. Repository 파일의 전체 SQL 쿼리를 확인해야 합니다.
---
🏁 Script executed:
```shell
#!/bin/bash
# OutboxNotificationRepository의 전체 내용 확인
cat backend/src/main/java/backend/mulkkam/outboxnotification/repository/OutboxNotificationRepository.java
Length of output: 2314
OutboxDispatcher의 전체 로직 및 nextBackoffTime 메서드를 확인하겠습니다.
---
🏁 Script executed:
```shell
#!/bin/bash
# OutboxDispatcher의 dispatch와 nextBackoffTime 확인
rg -n "dispatch|nextBackoffTime" backend/src/main/java/backend/mulkkam/outboxnotification/service/OutboxDispatcher.java -A5
Length of output: 650
백오프 계산이 메모리-DB 불일치로 인해 의도와 다르게 작동합니다.
확인 결과:
Repository 검증으로 리뷰어의 우려가 정확함이 확인되었습니다:
markSending(): DB에서만attemptCount + 1실행 (메모리는 미반영)handleFcmResponseline 90: 메모리의 stale 값job.getAttemptCount()사용nextBackoffTime(): 지수 백오프 (2 × 2^attempt)
구체적인 문제:
실제 흐름:
1. dispatch() 시작: job.attemptCount = 0 (메모리)
2. markSending() 실행: DB.attemptCount = 0 → 1 (메모리는 여전히 0)
3. 첫 실패: nextBackoffTime(0) = 2초 계산
↳ 실제 DB.attemptCount는 이미 1이지만, 백오프는 0 기준으로 계산
4. 다음 사이클에서 fetchReadyForSend로 attemptCount=1인 레코드 재조회
5. 두 번째 실패: nextBackoffTime(1) = 4초 계산
↳ 이제 DB.attemptCount는 2이지만, 1 기준으로 계산
결과: 백오프 간격이 실제 재시도 횟수보다 한 단계 낮게 계산됩니다.
개선 방안:
두 가지 접근이 가능합니다:
1안: Repository에서 증가된 값 반환 (권장)
// markSending: Long 반환
Long markSendingAndGetAttempt(@Param("id") Long id);
// handleFcmResponse에서
int currentAttempt = markSendingAndGetAttempt(job.getId());
outboxRepository.markRetryOrFail(
job.getId(),
nextBackoffTime(currentAttempt - 1), // 전 시도 기준으로 다음 백오프 계산
error
);장점: DB와 메모리 동기화, 명확한 의도
단점: Repository 메서드 변경 필요
2안: 메모리에서 직접 증가 (단순)
// handleFcmResponse에서
int nextAttempt = job.getAttemptCount() + 1;
outboxRepository.markRetryOrFail(
job.getId(),
nextBackoffTime(nextAttempt - 1),
error
);장점: 변경 최소
단점: markSending의 DB 증가와 중복, 일관성 유지 필요
권장: 1안으로 진행하여 메모리-DB를 명확히 동기화하는 것이 장기적으로 유지보수성을 높입니다.
Comment on lines
+1
to
+22
| CREATE TABLE outbox_notification ( | ||
| id BIGINT PRIMARY KEY AUTO_INCREMENT, | ||
|
|
||
| type VARCHAR(50) NOT NULL, | ||
| member_id BIGINT NOT NULL, | ||
| token VARCHAR(512) NOT NULL, | ||
| title VARCHAR(255) NOT NULL, | ||
| body VARCHAR(1000) NOT NULL, | ||
|
|
||
| status VARCHAR(20) NOT NULL, | ||
|
|
||
| dedupe_key VARCHAR(255) NOT NULL UNIQUE, | ||
|
|
||
| attempt_count INT NOT NULL DEFAULT 0, | ||
|
|
||
| last_error VARCHAR(255), | ||
|
|
||
| next_attempt_at TIMESTAMP NULL, | ||
|
|
||
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| deleted_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP | ||
| ); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
테이블 스키마가 Outbox 패턴의 핵심 요구사항을 잘 반영하고 있습니다.
잘 설계된 부분:
dedupe_key UNIQUE(Line 12): 중복 메시지 방지 - Outbox 패턴의 핵심 ✓attempt_count,next_attempt_at,last_error: 재시도 메커니즘 지원 ✓status필드로 상태 관리 (READY/SENDING/SENT/RETRY/FAIL) ✓
개선 제안:
1. deleted_at 컬럼명과 동작 불일치 (Line 21)
deleted_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP- 문제: 컬럼명은
deleted_at인데 실제로는updated_at역할을 합니다. INSERT 시 현재 시간이 들어가고, UPDATE 시마다 자동 갱신되므로 "삭제 시각"이 아니라 "수정 시각"입니다. - 영향: 코드에서
deleted_at을 soft delete 용도로 사용하려 할 때 혼란을 초래하고, 실제로는updated_at으로 사용되어 의미가 불명확합니다.
해결 방법:
-- 옵션 1: updated_at으로 컬럼명 변경 (권장)
updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP
-- 옵션 2: 실제 soft delete 용도로 사용
deleted_at TIMESTAMP NULL DEFAULT NULL
-- (soft delete는 별도 로직에서 UPDATE deleted_at = NOW() 수행)2. token 컬럼 크기 (Line 6)
token VARCHAR(512) NOT NULLFCM 토큰은 보통 150-200자 정도지만, 512는 충분히 여유있는 크기입니다. 향후 다른 푸시 서비스(APNs 등)를 지원할 경우를 대비한다면 적절합니다.
3. last_error 크기 제한 (Line 16)
last_error VARCHAR(255)Firebase 에러 메시지가 길어질 수 있습니다. 255자로 잘릴 경우 디버깅이 어려울 수 있으니:
VARCHAR(500)또는TEXT로 확장 고려- 또는 애플리케이션 레벨에서 255자로 truncate 후 저장하되, 전체 로그는 별도 로깅 시스템에 남기기
| private String title = "물 마실 시간이에요"; | ||
| private String body = "오늘도 건강한 하루 되세요!"; | ||
| private Status status = Status.READY; | ||
| private String dedupeKey = "REMIND:1:" + LocalDateTime.now() + ":" + token; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
dedupeKey 기본값 생성 방식이 테스트 안정성을 해칠 수 있습니다.
현재 구현:
private String dedupeKey = "REMIND:1:" + LocalDateTime.now() + ":" + token;문제점:
-
매번 다른 시간이 생성됨
- 빌더 인스턴스를 생성할 때마다
LocalDateTime.now()가 호출되므로, 같은 조건의 fixture를 여러 개 만들어도 dedupe key가 달라집니다. - dedupe 동작을 테스트할 때 의도치 않게 항상 unique한 키가 생성되어, 중복 검증이 제대로 되지 않을 수 있습니다.
- 빌더 인스턴스를 생성할 때마다
-
token 필드 참조 타이밍
- 필드 초기화 순서 상
token이 먼저 초기화되고 나서dedupeKey가 초기화되므로 현재는 문제없지만,.token(...)을 나중에 호출하면 dedupe key에 반영되지 않습니다.
- 필드 초기화 순서 상
개선 방안:
방안 1: build() 시점에 동적 생성 (권장)
private String dedupeKey = null; // 기본값을 null로
public OutboxNotification build() {
String finalDedupeKey = (dedupeKey != null)
? dedupeKey
: "REMIND:" + memberId + ":" + LocalDateTime.now() + ":" + token;
return new OutboxNotification(
type, memberId, token, title, body, status,
finalDedupeKey,
attemptCount, lastError, nextAttemptAt
);
}- 장점: 항상 최신 memberId, token 반영. 명시적으로 dedupeKey를 설정하지 않은 경우에만 생성.
- 단점: build()가 살짝 복잡해짐.
방안 2: 고정된 기본값 사용 (더 단순)
private String dedupeKey = "REMIND:1:2025-11-18T14:30:00:test-fcm-token";- 장점: 예측 가능하고 일관된 기본값. 대부분의 테스트에서는 dedupe key가 중요하지 않으므로 충분.
- 단점: memberId나 token을 변경했을 때 dedupe key가 실제 값과 안 맞을 수 있음 (하지만 테스트에서 명시적으로 설정 가능).
방안 3: 테스트마다 명시적으로 설정 (현재 방식 유지)
- 현재처럼 두고, 테스트에서 항상
.dedupeKey(...)를 명시적으로 호출하도록 가이드 - 단점: 실수로 누락하면 예측 불가능한 동작
중복 검증이 중요한 outbox 패턴의 특성상, 방안 1 (build 시 동적 생성) 또는 **방안 2 (고정 기본값)**를 권장합니다.
🤖 Prompt for AI Agents
In
backend/src/test/java/backend/mulkkam/support/fixture/OutboxNotificationFixtureBuilder.java
around line 15, the dedupeKey is initialized using LocalDateTime.now() and token
at field declaration which makes tests non-deterministic and ignores later
.token(...) updates; change to either leave dedupeKey null and compute a
finalDedupeKey in build() (use provided dedupeKey if set, otherwise construct
"REMIND:" + memberId + ":" + LocalDateTime.now() + ":" + token) or replace the
field with a fixed, deterministic default string; ensure build() passes the
finalDedupeKey into the OutboxNotification so tests are stable and respect any
later setter calls.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🔗 관련 이슈
📝 작업 내용
주요 변경사항
📸 스크린샷 (Optional)
Summary by CodeRabbit
릴리스 노트
새로운 기능
동작 변경
데이터베이스
테스트