[Notification][Refactor] 알림 재시도 전략 고도화 #204
Conversation
- LocalDateTime 포함되었을 시 파라미터로 받
- And 각 클래스 책임 분리
Summary of ChangesHello @sunwon12, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 알림 시스템의 견고성과 유지보수성을 크게 향상시키는 데 중점을 둡니다. 특히 FCM 전송 실패 시의 재시도 메커니즘을 기존의 알림 단위에서 기기 단위로 전환하여 더욱 정교한 오류 처리와 추적을 가능하게 했습니다. Resilience4j 도입으로 외부 API 호출의 안정성을 확보하고, 각 컴포넌트의 역할을 명확히 분리하여 시스템의 복잡도를 낮추고 확장성을 높였습니다. 또한, 개발자들이 더 효과적인 테스트 코드를 작성할 수 있도록 테스트 가이드라인을 보강했습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Test Results103 files 103 suites 23s ⏱️ Results for commit be12a6c. ♻️ This comment has been updated with latest results. |
🌻 테스트 커버리지 리포트
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
알림 재시도 전략을 고도화하는 이번 리팩토링은 매우 인상적입니다. NotificationDeviceLog를 도입하여 기기별로 전송 상태와 재시도 이력을 관리하고, Resilience4j를 활용한 재시도 및 서킷 브레이커 패턴을 적용하여 시스템의 안정성을 크게 향상시켰습니다. 책임 분리 원칙에 따라 NotificationFacade, NotificationService, FCMService 등으로 역할을 명확히 나눈 점도 코드의 유지보수성을 높이는 좋은 변화입니다. 테스트 가이드라인을 추가하여 팀의 테스트 품질을 높이려는 노력도 훌륭합니다. 몇 가지 개선점을 제안드렸으며, 특히 NotificationDeviceLog의 unique 제약 조건 관련 내용은 시스템의 핵심 기능에 영향을 줄 수 있으니 확인이 필요해 보입니다. 전반적으로 매우 완성도 높은 Pull Request입니다.
| @Column(nullable = false, unique = true) | ||
| private Long fcmTokenId; |
There was a problem hiding this comment.
fcmTokenId 필드에 설정된 unique = true 제약 조건은 잘못된 것으로 보입니다. 이 제약 조건은 각 FCM 토큰이 notification_device_log 테이블에 단 하나의 항목만 가질 수 있음을 의미합니다. 하지만, 단일 기기(즉, 단일 FCM 토큰)는 시간이 지남에 따라 여러 알림을 수신할 수 있습니다. 만약 사용자가 두 개의 다른 알림을 받게 되면, 동일한 기기에 대한 두 번째 알림 로그를 저장하려 할 때 DataIntegrityViolationException이 발생할 것입니다.
고유 제약 조건은 특정 알림 이벤트가 특정 기기에 한 번만 전송되도록 보장하기 위해 notification_id와 fcm_token_id의 조합에 대해 설정되어야 합니다. 다음과 같이 복합 고유 제약 조건으로 변경하는 것을 고려해 보세요.
@Table(name = "notification_device_log", uniqueConstraints = {
@UniqueConstraint(columnNames = {"notification_id", "fcmTokenId"})
})그리고 @Column(name = "fcmTokenId")에서 unique=true를 제거해야 합니다.
| package book.book.notification.dto; | ||
|
|
||
| public record FcmSendResult( | ||
| int successCount, | ||
| int totalCount, | ||
| String errorLog) { | ||
| public boolean isAnySuccess() { | ||
| return successCount > 0; | ||
| } | ||
|
|
||
| public boolean isAllFailed() { | ||
| return totalCount > 0 && successCount == 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
새로 추가된 FcmSendResult 레코드가 코드베이스 전체에서 사용되지 않는 것으로 보입니다. 만약 가까운 미래에 사용할 계획이 없다면, 코드를 깔끔하게 유지하고 혼동을 피하기 위해 제거하는 것이 좋겠습니다.
| fcmTokenRepository.save(FCMTokenFixture.builderWithoutId().userId(receiverId).build()); | ||
|
|
There was a problem hiding this comment.
FCM_전송_실패_시_알림은_DB에_저장되고_상태가_FAILED로_변경된다 테스트의 then 블록 안에 fcmTokenRepository.save() 호출이 잘못 위치해 있습니다. 테스트 설정 코드는 given 블록에 있어야 합니다. 이미 테스트 시작 부분(113번 줄)에 유사한 설정 코드가 있습니다. 이 중복되고 잘못 배치된 코드는 테스트를 읽고 이해하기 어렵게 만듭니다. 이 줄을 제거하고 모든 설정이 given 블록에서 이루어지도록 해주세요.
| java.time.LocalDateTime pendingThreshold = now.plusHours(1); | ||
|
|
||
| java.util.List<NotificationDto> retryMessages = notificationService.findRetryMessages(cutoffTime, | ||
| pendingThreshold); | ||
|
|
||
| // then | ||
| Notification updated = notificationRepository.findById(notificationId).orElseThrow(); | ||
| assertThat(updated.isRead()).isTrue(); | ||
| assertThat(retryMessages).hasSize(1); | ||
| NotificationDto retryMessage = retryMessages.get(0); | ||
| assertThat(retryMessage.getNotificationDeviceLogId()).isEqualTo(logId); | ||
|
|
||
| // Verify retry count incremented | ||
| book.book.notification.domain.NotificationDeviceLog updatedLog = notificationDeviceLogRepository | ||
| .findByIdOrElseThrow(logId); | ||
| assertThat(updatedLog.getRetryCount()).isEqualTo(1); | ||
| } |
There was a problem hiding this comment.
markAsRead 메소드에 대한 테스트(알림_읽음_처리가_올바르게_동작한다)가 제거된 것으로 보입니다. 하지만 NotificationService 클래스에는 markAsRead 메소드가 여전히 존재합니다. 향후 발생할 수 있는 기능 회귀(regression)를 방지하기 위해 public 메소드에 대한 테스트 커버리지를 유지하는 것이 중요합니다. markAsRead 기능에 대한 테스트 케이스를 다시 추가하는 것을 고려해 주세요.
Change Details
🔔 Notification (알림)
NotificationDeviceLog엔티티를 도입하여 기기 단위의 재시도 관리 및 이력 추적을 구현했습니다📄 Documentation (문서)
TESTING_GUIDELINES.md를 추가하여 테스트 작성 시LocalDateTime처리 등 공통 규칙을 정의했습니다