Conversation
|
Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | PR 설명에서 '작업 내용'과 '주요 변경사항' 섹션이 완전히 비어있어 변경사항에 대한 설명이 부족합니다. | 작업 내용과 주요 변경사항 섹션을 채워 구현된 기능과 변경 사항을 명확하게 설명해주세요. | |
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | PR 제목이 FCM 전송을 위한 데이터베이스 큐 구현의 핵심 내용을 명확하게 전달하고 있습니다. |
| Linked Issues check | ✅ Passed | PR의 코드 변경사항들이 FCM 데이터베이스 큐 구현의 기본 요구사항을 충족하고 있습니다. |
| Out of Scope Changes check | ✅ Passed | 모든 변경사항이 FCM 큐 기능 구현 범위 내에 있으며 범위 밖의 변경사항은 없습니다. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueScheduler.java`:
- Around line 13-16: FcmQueueScheduler.pollAndSend() currently calls
fcmQueueService.processPendingQueue() without exception handling; wrap that call
in a try-catch that catches Exception (or specific checked/runtime exceptions
you expect), log the error with a proper Logger (e.g., private static final
Logger log = LoggerFactory.getLogger(FcmQueueScheduler.class)) using a clear
message like "Failed to process FCM queue" and include the exception, and
optionally emit a metric or alert inside the catch so failures of
fcmQueueService.processPendingQueue() are tracked; do not rethrow so the
`@Scheduled` loop continues.
In
`@backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueService.java`:
- Around line 98-115: processPendingQueue currently runs the entire batch in one
transaction; refactor so each outbox is processed in its own transaction: remove
`@Transactional` from processPendingQueue, keep the fetch using
notificationOutboxRepository.findPendingForUpdate, iterate pendingQueues and
call a new `@Transactional` method processSingleOutbox(NotificationOutbox outbox)
which invokes send(outbox), outbox.markSuccess(...), and on exception calls
handleFailure(outbox, ..., e); alternatively, if you prefer programmatic
transactions use TransactionTemplate inside the loop to execute the per-item
logic (send/markSuccess/handleFailure) in its own transaction.
🧹 Nitpick comments (15)
backend/src/main/resources/db/migration/common/V20260115_120000__add_notification_outbox.sql (1)
23-24:deleted_at활용 시 인덱스 고려가 필요합니다.Soft delete 패턴을 사용하는 경우, 대부분의 조회 쿼리에
WHERE deleted_at IS NULL조건이 추가됩니다. 현재 인덱스(status, next_attempt_at)에는deleted_at이 포함되어 있지 않습니다.만약 soft delete를 실제로 활용할 계획이라면:
- 기존 인덱스를
(status, next_attempt_at, deleted_at)으로 확장하거나- partial index를 지원하는 DB라면
WHERE deleted_at IS NULL조건부 인덱스 고려Soft delete를 사용하지 않을 계획이라면
deleted_at컬럼 자체가 불필요할 수 있습니다.backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/repository/NotificationOutboxRepository.java (1)
24-28: 메서드 네이밍 및 파라미터 타입 개선 고려코딩 가이드라인에 따르면 여러 결과를 반환하는 경우
read접두사를 사용하도록 되어 있습니다.findPendingForUpdate→readPendingForUpdate변경을 고려해 보세요.또한,
status파라미터를String대신NotificationOutboxStatusenum으로 받으면 타입 안전성이 높아집니다. 현재 방식은 호출부에서.name()변환이 필요하고, 오타 가능성이 있습니다.단, native query에서 enum을 직접 사용하려면
@Enumerated(EnumType.STRING)설정이 엔티티에 되어 있어야 합니다. SpEL을 활용하거나 JPQL로 전환하는 방법도 있습니다.backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueService.java (2)
50-61: 테스트 용이성을 위한 Clock 주입 고려
LocalDateTime.now()를 직접 호출하면 단위 테스트에서 시간 관련 로직을 검증하기 어렵습니다.java.time.Clock을 주입받아 사용하면 테스트에서 고정된 시간으로 검증할 수 있습니다.♻️ Clock 주입 예시
public class FcmQueueService { + private final Clock clock; public FcmQueueService( NotificationOutboxRepository notificationOutboxRepository, DeviceRepository deviceRepository, FcmClient fcmClient, + Clock clock, `@Value`("${fcm.queue.batch-size:100}") int batchSize, ... ) { + this.clock = clock; ... } `@Transactional` public void enqueueTopic(SendMessageByFcmTopicRequest request) { NotificationOutbox outbox = NotificationOutbox.forTopic( ... maxAttempts, - LocalDateTime.now() + LocalDateTime.now(clock) );
76-96: 빈 리스트 체크 후 early return 패턴 적절함빈 토큰 리스트에 대한 방어 코드가 잘 작성되어 있습니다.
Lists.partition을 사용한 배치 처리도 FCM의 500개 제한을 고려한 적절한 구현입니다.다만, 현재 코드에서는 각 파티션마다
saveAll을 호출하여 여러 번의 DB 왕복이 발생합니다. 전체 리스트를 한 번에 수집 후 단일saveAll로 변경하면 성능이 개선될 수 있습니다:♻️ 단일 saveAll로 개선
`@Transactional` public void enqueueTokens(SendMessageByFcmTokensRequest request) { if (request.allTokens().isEmpty()) { return; } LocalDateTime now = LocalDateTime.now(); - Lists.partition(request.allTokens(), FCM_BATCH_SIZE) - .forEach(tokens -> { - List<NotificationOutbox> outboxes = tokens.stream() - .map(token -> NotificationOutbox.forToken( - request.title(), - request.body(), - token, - request.action(), - maxAttempts, - now - )) - .toList(); - notificationOutboxRepository.saveAll(outboxes); - }); + List<NotificationOutbox> outboxes = request.allTokens().stream() + .map(token -> NotificationOutbox.forToken( + request.title(), + request.body(), + token, + request.action(), + maxAttempts, + now + )) + .toList(); + notificationOutboxRepository.saveAll(outboxes); }참고로
FCM_BATCH_SIZE는 현재 enqueue 시점이 아닌 실제 FCM 전송 시점에서 의미가 있는 값이므로, 여기서는 파티션이 불필요해 보입니다.backend/src/test/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueServiceUnitTest.java (3)
41-68: @DisplayName 형식 및 테스트 구조 개선 필요코딩 가이드라인에 따르면
@DisplayName은methodName_successOrError_condition형식을 사용해야 합니다. 또한@Nested를 사용하여 테스트 시나리오를 구성하도록 되어 있습니다.현재:
"토큰 리스트 요청은 토큰당 1건의 아웃박스로 저장된다"
권장:"enqueueTokens_success_savesOutboxPerToken"♻️ 테스트 구조 개선 예시
`@ExtendWith`(MockitoExtension.class) class FcmQueueServiceUnitTest { `@Nested` `@DisplayName`("enqueueTokens 메서드") class EnqueueTokens { `@DisplayName`("enqueueTokens_success_savesOutboxPerToken") `@Test` void savesOutboxPerToken() { // ... } } `@Nested` `@DisplayName`("processPendingQueue 메서드") class ProcessPendingQueue { `@DisplayName`("processPendingQueue_success_marksSuccess") `@Test` void marksSuccess() { // ... } `@DisplayName`("processPendingQueue_failure_marksPendingWithBackoff") `@Test` void marksPendingWithBackoff() { // ... } } }코딩 가이드라인에 따른 권장사항입니다.
89-93: assertSoftly 사용 권장코딩 가이드라인에 따르면 여러 assertion을
assertSoftly로 감싸서 모든 검증 실패를 한 번에 확인할 수 있도록 해야 합니다.♻️ assertSoftly 적용 예시
+import static org.assertj.core.api.SoftAssertions.assertSoftly; // then - assertThat(outbox.getStatus()).isEqualTo(NotificationOutboxStatus.SUCCESS); - assertThat(outbox.getAttemptCount()).isEqualTo(1); - assertThat(outbox.getSentAt()).isNotNull(); + assertSoftly(softly -> { + softly.assertThat(outbox.getStatus()).isEqualTo(NotificationOutboxStatus.SUCCESS); + softly.assertThat(outbox.getAttemptCount()).isEqualTo(1); + softly.assertThat(outbox.getSentAt()).isNotNull(); + });이 패턴을 다른 테스트 메서드들에도 일관되게 적용해 주세요. 코딩 가이드라인 기반 권장사항입니다.
70-93: enqueueTopic, enqueueToken 메서드 테스트 누락현재
enqueueTokens만 테스트되어 있고,enqueueTopic과enqueueToken메서드에 대한 테스트가 없습니다. 단위 테스트 커버리지를 위해 다음 케이스들을 추가하는 것이 좋습니다:
enqueueTopic_success_savesTopicOutboxenqueueToken_success_savesTokenOutbox이 테스트 케이스들을 생성해 드릴까요?
backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmEventListener.java (1)
12-15: 어노테이션 순서 조정 필요코딩 가이드라인에 따르면 어노테이션 순서는 "logs, Lombok, Spring meta, Spring components (가장 마지막)" 입니다.
현재:
@Async,@RequiredArgsConstructor,@Component
권장:@RequiredArgsConstructor,@Async,@Component♻️ 어노테이션 순서 수정
-@Async `@RequiredArgsConstructor` +@Async `@Component` public class FcmEventListener {코딩 가이드라인 기반 권장사항입니다.
backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/domain/NotificationOutbox.java (7)
16-18:@NoArgsConstructor의 접근 제어자를protected로 지정하세요.JPA 엔티티에서 기본 생성자는 프록시 생성을 위해 필요하지만, 외부에서 직접 호출되는 것은 의도된 사용 방식이 아닙니다.
public대신protected로 제한하면 의도치 않은 객체 생성을 방지할 수 있습니다.♻️ 개선 제안
`@Getter` -@NoArgsConstructor +@NoArgsConstructor(access = AccessLevel.PROTECTED) `@Entity` public class NotificationOutbox extends BaseEntity {
AccessLevel.PROTECTEDimport 추가:import lombok.AccessLevel;
86-124: 코딩 가이드라인 위반: 도메인에서 정적 팩토리 메서드 사용을 지양해야 합니다.현재
forTopic과forToken정적 팩토리 메서드가 사용되고 있으나, 팀 코딩 가이드라인에서는backend/**/domain/**/*.java경로의 파일에 대해 "Do not use static factory methods; use new operator for domain and DTO creation"을 명시하고 있습니다.대안 1: 생성자를 public으로 변경
- 장점: 가이드라인 준수, 단순한 구조
- 단점: 타입별 생성 의도가 명시적이지 않음
대안 2: 팀과 협의 후 예외 허용
- 이 파일은
domain패키지가 아닌infrastructure/fcm/domain에 위치하여 인프라 레이어로 볼 수 있습니다- 인프라 레이어의 엔티티는 도메인 규칙에서 예외로 둘 수 있는지 팀과 논의가 필요합니다
♻️ 대안 1: 생성자 public으로 변경
- private NotificationOutbox( + public NotificationOutbox( NotificationOutboxTargetType targetType, String title, String body, Action action, String topic, String token, int maxAttempts, LocalDateTime now ) { ... } - public static NotificationOutbox forTopic( - String title, - String body, - String topic, - Action action, - int maxAttempts, - LocalDateTime now - ) { - return new NotificationOutbox( - NotificationOutboxTargetType.TOPIC, - title, - body, - action, - topic, - null, - maxAttempts, - now - ); - } - - public static NotificationOutbox forToken( - String title, - String body, - String token, - Action action, - int maxAttempts, - LocalDateTime now - ) { - return new NotificationOutbox( - NotificationOutboxTargetType.TOKEN, - title, - body, - action, - null, - token, - maxAttempts, - now - ); - }호출부에서 직접 생성자 사용:
new NotificationOutbox(NotificationOutboxTargetType.TOPIC, title, body, action, topic, null, maxAttempts, now); new NotificationOutbox(NotificationOutboxTargetType.TOKEN, title, body, action, null, token, maxAttempts, now);코딩 가이드라인에 따르면, 도메인 파일에서는 static factory method 대신 new 연산자를 사용해야 합니다.
63-84: 필수 필드에 대한 유효성 검증 추가를 권장합니다.생성자에서
title,body,action등nullable = false로 지정된 필드들에 대한 null 검증이 없습니다. DB 제약 조건에만 의존하면 런타임에 예외가 발생할 때 원인 파악이 어려울 수 있습니다.선택지:
- 생성자에서 직접 검증: 즉시 피드백 제공, 단순한 구현
- Value Object 활용: 가이드라인에서 권장하는 방식, 재사용성 향상
- Bean Validation 활용:
@NotNull,@NotBlank애노테이션 활용♻️ 선택지 1: 생성자에서 직접 검증 예시
private NotificationOutbox( NotificationOutboxTargetType targetType, String title, String body, Action action, String topic, String token, int maxAttempts, LocalDateTime now ) { super(); + validateNotNull(targetType, "targetType"); + validateNotBlank(title, "title"); + validateNotBlank(body, "body"); + validateNotNull(action, "action"); + validateNotNull(now, "now"); this.targetType = targetType; ... } + +private static void validateNotNull(Object value, String fieldName) { + if (value == null) { + throw new IllegalArgumentException(fieldName + " must not be null"); + } +} + +private static void validateNotBlank(String value, String fieldName) { + if (value == null || value.isBlank()) { + throw new IllegalArgumentException(fieldName + " must not be blank"); + } +}코딩 가이드라인에서 "Use Value Objects (VO) for domains that require validation"을 권장하고 있으니, 장기적으로는 VO 도입도 고려해 보세요.
135-152: 지수 백오프 계산 시 오버플로우 가능성을 확인하세요.Line 150의
1L << (this.attemptCount - 1)계산에서,attemptCount가 64 이상이면 비트 시프트 결과가 예상치 못한 값이 될 수 있습니다. 현재maxAttempts설정값이 작다면 실제 문제가 되지 않겠지만, 설정 변경에 대비한 방어 로직이 있으면 더 안전합니다.또한
baseBackoffSeconds * multiplier곱셈 결과가long범위를 초과할 수 있으므로,plusSeconds에 전달되는 값에 상한선을 두는 것이 좋습니다.♻️ 안전한 백오프 계산 예시
this.status = NotificationOutboxStatus.PENDING; - long multiplier = 1L << (this.attemptCount - 1); - this.nextAttemptAt = now.plusSeconds(baseBackoffSeconds * multiplier); + int shiftAmount = Math.min(this.attemptCount - 1, 30); // 최대 ~34년으로 제한 + long multiplier = 1L << shiftAmount; + long backoffSeconds = Math.min(baseBackoffSeconds * multiplier, 86400L); // 최대 1일 + this.nextAttemptAt = now.plusSeconds(backoffSeconds);
126-133:markSuccess에서nextAttemptAt설정 의도를 명확히 해주세요.성공 시 재시도가 필요 없으므로
nextAttemptAt을now로 설정하는 것이 논리적으로 불필요해 보입니다. 쿼리 조건(status = PENDING AND nextAttemptAt <= now)에서 SUCCESS 상태는 이미 제외되므로 기능상 문제는 없지만, 코드 의도가 명확하지 않습니다.선택지:
nextAttemptAt = null로 설정하여 "더 이상 재시도 없음"을 명시 (컬럼이 nullable이면)- 현재 로직 유지하되 주석으로 의도 명시
- 그대로 유지 (동작에는 영향 없음)
43-45:topic과token의 상호 배타성을 검증하는 것이 좋습니다.
targetType이TOPIC이면topic만,TOKEN이면token만 값이 있어야 합니다. 현재는 이 불변 조건이 팩토리 메서드에서만 암묵적으로 보장되고 있어, 향후 생성자가 직접 호출될 경우 잘못된 상태의 객체가 생성될 수 있습니다.♻️ 생성자에서 상호 배타성 검증 예시
private NotificationOutbox(...) { super(); // ... 기존 할당 로직 ... if (targetType == NotificationOutboxTargetType.TOPIC && topic == null) { throw new IllegalArgumentException("topic must not be null for TOPIC type"); } if (targetType == NotificationOutboxTargetType.TOKEN && token == null) { throw new IllegalArgumentException("token must not be null for TOKEN type"); } }
1-165: 테스트 코드 작성 제안아웃박스 엔티티의 상태 전이 로직이 핵심이므로, 다음 단위 테스트 케이스를 권장합니다:
생성 테스트
forTopic호출 시 초기 상태(PENDING,attemptCount=0) 검증forToken호출 시 초기 상태 검증markSuccess 테스트
- 성공 마킹 후
status=SUCCESS,sentAt설정, 에러 정보 초기화 검증markFailure 테스트
- 첫 실패 후
PENDING유지,nextAttemptAt백오프 적용 검증- 최대 재시도 도달 시
FAILED전환 검증- 지수 백오프 계산 정확성 검증 (1초, 2초, 4초, ...)
markFailedNow 테스트
- 즉시
FAILED전환 및 에러 정보 저장 검증AI 요약에 따르면
FcmQueueServiceUnitTest가 존재하는 것으로 보이나, 엔티티 자체의 단위 테스트도 별도로 있으면 상태 전이 로직의 안정성을 더 확보할 수 있습니다.
backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueScheduler.java
Show resolved
Hide resolved
backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/domain/FcmErrorHandlingStrategy.java`:
- Around line 39-41: Add null guarding to FcmErrorHandlingStrategy.from:
currently STRATEGIES.getOrDefault(errorCode, INTERNAL) will throw if errorCode
is null because the EnumMap (STRATEGIES) doesn't accept null keys; update the
method to first check if errorCode is null and immediately return INTERNAL, then
lookup STRATEGIES.getOrDefault(errorCode, INTERNAL). This references the
FcmErrorHandlingStrategy.from method, the STRATEGIES EnumMap and the
FirebaseErrorCode enum.
In
`@backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueOutboxProcessor.java`:
- Around line 61-75: The send method in FcmQueueOutboxProcessor currently
switches on outbox.getTargetType() without a default path, so unknown or null
TargetType becomes a silent no-op; update the send(NotificationOutbox outbox)
method to handle unexpected values by adding a default branch that throws an
IllegalStateException (e.g., throw new IllegalStateException("Unsupported
TargetType: " + outbox.getTargetType())) to fail-fast, and also guard against
null by checking outbox.getTargetType() == null and throwing a clear exception;
reference the send method, NotificationOutbox.getTargetType(), and the switch
over TargetType when making this change.
In
`@backend/src/test/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueOutboxProcessorUnitTest.java`:
- Line 38: Update the `@DisplayName` values in FcmQueueOutboxProcessorUnitTest to
follow the guide's methodName_successOrError_condition format (e.g., change "전송
성공 시 아웃박스 상태가 성공으로 갱신된다" to something like
"processOutbox_sendSuccess_updatesOutboxStatusSuccess"); apply the same pattern
for the other `@DisplayName` annotations in this file (the occurrences around
lines shown in the review), and if you need to preserve the longer Korean
description, move it into a single-line method-level comment or expand the test
method name while keeping the DisplayName strictly formatted; ensure the unique
annotation tokens `@DisplayName` in this class and the corresponding test method
names (e.g., the test methods that reference FcmQueueOutboxProcessor) are
updated consistently.
🧹 Nitpick comments (5)
backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueScheduler.java (2)
19-23: 예외 로깅 시 스택 트레이스 누락현재 예외 발생 시
e.getMessage()와e.getClass().getSimpleName()만 로깅하고 있어, 실제 예외가 발생한 위치(스택 트레이스)가 기록되지 않습니다.문제점:
- 프로덕션에서 오류 발생 시 근본 원인 파악이 어려워집니다
- 특히 중첩된 예외(nested exception)의 경우 원인 추적이 불가능합니다
대안:
SLF4J에서는 마지막 인자로Throwable을 전달하면 자동으로 스택 트레이스가 기록됩니다.♻️ 스택 트레이스를 포함한 로깅 제안
} catch (Exception e) { - log.error("[FCM QUEUE SCHEDULER ERROR] message={}, type={}", - e.getMessage(), - e.getClass().getSimpleName()); + log.error("[FCM QUEUE SCHEDULER ERROR] 큐 처리 중 오류 발생", e); }이렇게 하면 메시지와 함께 전체 스택 트레이스가 기록되어 디버깅이 훨씬 수월해집니다.
1-25: FcmQueueScheduler 단위 테스트 필요AI 요약에 따르면
FcmQueueServiceUnitTest와FcmQueueOutboxProcessorUnitTest는 존재하지만,FcmQueueScheduler자체에 대한 테스트는 언급되지 않았습니다.테스트 케이스 제안:
pollAndSend()호출 시fcmQueueService.processPendingQueue()가 정상 호출되는지 검증processPendingQueue()에서 예외 발생 시 스케줄러가 중단되지 않고 에러 로그만 남기는지 검증- 예외 발생 후 다음 스케줄링 주기에 다시 정상 실행되는지 검증
`@ExtendWith`(MockitoExtension.class) class FcmQueueSchedulerUnitTest { `@Mock` private FcmQueueService fcmQueueService; `@InjectMocks` private FcmQueueScheduler fcmQueueScheduler; `@Test` void pollAndSend_정상_처리시_큐_서비스_호출() { fcmQueueScheduler.pollAndSend(); verify(fcmQueueService, times(1)).processPendingQueue(); } `@Test` void pollAndSend_예외_발생시_스케줄러_중단되지_않음() { doThrow(new RuntimeException("테스트 예외")) .when(fcmQueueService).processPendingQueue(); assertDoesNotThrow(() -> fcmQueueScheduler.pollAndSend()); } }코딩 가이드라인에 따르면 스케줄러와 같은 컴포넌트는 단위 테스트로 커버하는 것이 권장됩니다.
backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/domain/FcmErrorHandlingStrategy.java (1)
16-31: 필드final사용이 팀 가이드와 충돌합니다.현재
STRATEGIES와 플래그 필드가final로 선언돼 있습니다.
선택지 A)final제거로 가이드 준수 (장점: 스타일 일관성, 단점: 불변성 의도 약화)
선택지 B) 불변성이 핵심이면final유지 + 가이드 예외 문서화/주석 추가 (장점: 안전성 유지, 단점: 규칙 예외 관리 필요)코딩 가이드 기준입니다.
수정 예시 (선택지 A)
- private static final Map<FirebaseErrorCode, FcmErrorHandlingStrategy> STRATEGIES = + private static Map<FirebaseErrorCode, FcmErrorHandlingStrategy> STRATEGIES = new EnumMap<>(FirebaseErrorCode.class); - private final boolean retryable; - private final boolean removeToken; - private final boolean failFast; + private boolean retryable; + private boolean removeToken; + private boolean failFast;backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueService.java (1)
37-83: 생성성 메서드 네이밍을 가이드와 맞추는 편이 좋겠습니다.
enqueue*가 의미는 좋지만, 가이드는 생성 작업을 create/save/register/add로 제한합니다. 로직 자체는 명확합니다.
선택지 A)saveTopicOutbox,saveTokenOutbox,saveTokenOutboxes등으로 변경 (장점: 규칙/검색성 향상, 단점: 호출부 일괄 수정 필요)
선택지 B) 외부 API 의미를 유지해야 한다면enqueue*는 퍼사드로 두고 내부 구현을save*로 분리 (장점: 의미+규칙 모두 충족, 단점: 메서드 수 증가)코딩 가이드 기준입니다.
backend/src/test/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueOutboxProcessorUnitTest.java (1)
26-27: 시나리오별@Nested구성이 있으면 가독성이 좋아집니다.선택지 A) 성공/실패/에러코드 별
@Nested분리 (장점: 구조화/탐색 용이, 단점: 파일 길이 증가)
선택지 B) 현 구조 유지 + 접두어/섹션 주석으로 그룹화 (장점: 단순, 단점: 시나리오 경계가 약함)코딩 가이드 기준입니다.
| public static FcmErrorHandlingStrategy from(FirebaseErrorCode errorCode) { | ||
| return STRATEGIES.getOrDefault(errorCode, INTERNAL); | ||
| } |
There was a problem hiding this comment.
from 메서드에 null 방어가 있으면 더 안전합니다.
EnumMap은 null 키를 허용하지 않아 errorCode가 null이면 예외가 날 수 있습니다. 기본값 반환으로 방어하는 편이 안전합니다.
수정 예시
public static FcmErrorHandlingStrategy from(FirebaseErrorCode errorCode) {
+ if (errorCode == null) {
+ return INTERNAL;
+ }
return STRATEGIES.getOrDefault(errorCode, INTERNAL);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static FcmErrorHandlingStrategy from(FirebaseErrorCode errorCode) { | |
| return STRATEGIES.getOrDefault(errorCode, INTERNAL); | |
| } | |
| public static FcmErrorHandlingStrategy from(FirebaseErrorCode errorCode) { | |
| if (errorCode == null) { | |
| return INTERNAL; | |
| } | |
| return STRATEGIES.getOrDefault(errorCode, INTERNAL); | |
| } |
🤖 Prompt for AI Agents
In
`@backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/domain/FcmErrorHandlingStrategy.java`
around lines 39 - 41, Add null guarding to FcmErrorHandlingStrategy.from:
currently STRATEGIES.getOrDefault(errorCode, INTERNAL) will throw if errorCode
is null because the EnumMap (STRATEGIES) doesn't accept null keys; update the
method to first check if errorCode is null and immediately return INTERNAL, then
lookup STRATEGIES.getOrDefault(errorCode, INTERNAL). This references the
FcmErrorHandlingStrategy.from method, the STRATEGIES EnumMap and the
FirebaseErrorCode enum.
| private void send(NotificationOutbox outbox) { | ||
| switch (outbox.getTargetType()) { | ||
| case TOPIC -> fcmClient.sendMessageByTopic(new SendMessageByFcmTopicRequest( | ||
| outbox.getTitle(), | ||
| outbox.getBody(), | ||
| outbox.getTopic(), | ||
| outbox.getAction() | ||
| )); | ||
| case TOKEN -> fcmClient.sendMessageByToken(new SendMessageByFcmTokenRequest( | ||
| outbox.getTitle(), | ||
| outbox.getBody(), | ||
| outbox.getToken(), | ||
| outbox.getAction() | ||
| )); | ||
| } |
There was a problem hiding this comment.
알 수 없는 TargetType 처리 누락으로 ‘무전송 성공’ 가능성이 있습니다.
default가 없어 새로운 enum 값/NULL일 때 send가 no-op인데도 성공 처리될 수 있습니다.
선택지 A) default에서 IllegalStateException 던져 fail-fast (장점: 조기 감지, 단점: 예외 증가)
선택지 B) 명시적으로 실패 상태로 마킹 (장점: 재처리/추적 가능, 단점: 실패 정책 설계 필요)
수정 예시 (선택지 A)
private void send(NotificationOutbox outbox) {
switch (outbox.getTargetType()) {
case TOPIC -> fcmClient.sendMessageByTopic(new SendMessageByFcmTopicRequest(
outbox.getTitle(),
outbox.getBody(),
outbox.getTopic(),
outbox.getAction()
));
case TOKEN -> fcmClient.sendMessageByToken(new SendMessageByFcmTokenRequest(
outbox.getTitle(),
outbox.getBody(),
outbox.getToken(),
outbox.getAction()
));
+ default -> throw new IllegalStateException("Unsupported target type: " + outbox.getTargetType());
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void send(NotificationOutbox outbox) { | |
| switch (outbox.getTargetType()) { | |
| case TOPIC -> fcmClient.sendMessageByTopic(new SendMessageByFcmTopicRequest( | |
| outbox.getTitle(), | |
| outbox.getBody(), | |
| outbox.getTopic(), | |
| outbox.getAction() | |
| )); | |
| case TOKEN -> fcmClient.sendMessageByToken(new SendMessageByFcmTokenRequest( | |
| outbox.getTitle(), | |
| outbox.getBody(), | |
| outbox.getToken(), | |
| outbox.getAction() | |
| )); | |
| } | |
| private void send(NotificationOutbox outbox) { | |
| switch (outbox.getTargetType()) { | |
| case TOPIC -> fcmClient.sendMessageByTopic(new SendMessageByFcmTopicRequest( | |
| outbox.getTitle(), | |
| outbox.getBody(), | |
| outbox.getTopic(), | |
| outbox.getAction() | |
| )); | |
| case TOKEN -> fcmClient.sendMessageByToken(new SendMessageByFcmTokenRequest( | |
| outbox.getTitle(), | |
| outbox.getBody(), | |
| outbox.getToken(), | |
| outbox.getAction() | |
| )); | |
| default -> throw new IllegalStateException("Unsupported target type: " + outbox.getTargetType()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@backend/src/main/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueOutboxProcessor.java`
around lines 61 - 75, The send method in FcmQueueOutboxProcessor currently
switches on outbox.getTargetType() without a default path, so unknown or null
TargetType becomes a silent no-op; update the send(NotificationOutbox outbox)
method to handle unexpected values by adding a default branch that throws an
IllegalStateException (e.g., throw new IllegalStateException("Unsupported
TargetType: " + outbox.getTargetType())) to fail-fast, and also guard against
null by checking outbox.getTargetType() == null and throwing a clear exception;
reference the send method, NotificationOutbox.getTargetType(), and the switch
over TargetType when making this change.
| @Mock | ||
| private FcmClient fcmClient; | ||
|
|
||
| @DisplayName("전송 성공 시 아웃박스 상태가 성공으로 갱신된다") |
There was a problem hiding this comment.
@DisplayName 포맷이 가이드와 다릅니다.
가이드는 methodName_successOrError_condition 형식을 요구합니다.
선택지 A) 가이드 형식으로 변경 (장점: 규칙 준수/검색성, 단점: 설명성이 줄어듦)
선택지 B) DisplayName은 형식 준수, 상세 설명은 주석/메서드명으로 보완 (장점: 설명 유지, 단점: 위치 분산)
코딩 가이드 기준입니다.
수정 예시 (다른 테스트에도 동일 패턴 적용)
- `@DisplayName`("전송 성공 시 아웃박스 상태가 성공으로 갱신된다")
+ `@DisplayName`("processNext_success_whenSendOk")Also applies to: 69-69, 106-106, 138-138, 170-170
🤖 Prompt for AI Agents
In
`@backend/src/test/java/backend/mulkkam/common/infrastructure/fcm/service/FcmQueueOutboxProcessorUnitTest.java`
at line 38, Update the `@DisplayName` values in FcmQueueOutboxProcessorUnitTest to
follow the guide's methodName_successOrError_condition format (e.g., change "전송
성공 시 아웃박스 상태가 성공으로 갱신된다" to something like
"processOutbox_sendSuccess_updatesOutboxStatusSuccess"); apply the same pattern
for the other `@DisplayName` annotations in this file (the occurrences around
lines shown in the review), and if you need to preserve the longer Korean
description, move it into a single-line method-level comment or expand the test
method name while keeping the DisplayName strictly formatted; ensure the unique
annotation tokens `@DisplayName` in this class and the corresponding test method
names (e.g., the test methods that reference FcmQueueOutboxProcessor) are
updated consistently.
1 participant
🔗 관련 이슈
📝 작업 내용
주요 변경사항
📸 스크린샷 (Optional)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.