feat: FCM 404 에러 핸들링 추가#2283
Open
Soundbar91 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR refactors FCM notification delivery to capture per-message send results and persist them on notification records, then introduces a scheduled cleanup job to remove device tokens from users with repeated unregistered delivery failures. ChangesFCM Push Result Persistence
Device Token Cleanup for Unregistered Push Failures
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java (1)
70-80: 💤 Low valueConsider adding a defensive size check for list alignment.
The method assumes
notificationsandresponseshave identical sizes. While this is true given the currentFcmClient.sendMessagesimplementation, a defensive check would preventIndexOutOfBoundsExceptionif the contract is accidentally violated.🛡️ Optional defensive check
private void markPushResults(List<Notification> notifications, List<FcmSendResponse> responses) { + if (notifications.size() != responses.size()) { + log.warn("알림과 FCM 응답 수 불일치. notifications={}, responses={}", notifications.size(), responses.size()); + return; + } for (int index = 0; index < notifications.size(); index++) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java` around lines 70 - 80, The markPushResults in NotificationService assumes notifications and responses are the same length and can throw IndexOutOfBoundsException; add a defensive size check and only iterate up to Math.min(notifications.size(), responses.size()) (or throw a clear IllegalStateException if sizes differ) so Notification.markPushSuccess()/markPushFailure(...) are only called with matching FcmSendResponse entries; reference the method markPushResults and the types Notification and FcmSendResponse when making the change.src/main/resources/db/migration/V236__add_fcm_result_to_notification.sql (1)
1-4: 💤 Low valueConsider adding an index for the cleanup scheduler query.
The cleanup scheduler will query notifications by
is_push_success,fcm_messaging_error_code, and time range. If thenotificationtable is large, consider adding a composite index to optimize these queries:CREATE INDEX idx_notification_push_failure ON notification (is_push_success, fcm_messaging_error_code, created_at);This is not blocking since the scheduler runs daily and the query volume is low.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/db/migration/V236__add_fcm_result_to_notification.sql` around lines 1 - 4, The migration adds is_push_success, fcm_error_code and fcm_messaging_error_code to the notification table but doesn’t add an index for the cleanup scheduler; add a composite index (e.g. idx_notification_push_failure) on (is_push_success, fcm_messaging_error_code, created_at) so queries filtering by is_push_success and fcm_messaging_error_code and a time range are optimized; modify V236__add_fcm_result_to_notification.sql to create that index after adding the columns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/repository/NotificationRepository.java`:
- Around line 29-42: The query in
NotificationRepository.findUnregisteredPushFailureUserIds relies on
User.updatedAt which is a coarse entity-wide audit field and can falsely skip
stale device tokens; change the predicate to use a token-specific timestamp or
persisted token on the Notification entity instead (e.g. add
Notification.deviceToken and/or Notification.deviceTokenUpdatedAt and persist
the token/timestamp when sending), then update the JPQL in
findUnregisteredPushFailureUserIds to compare n.deviceToken (or
n.deviceTokenUpdatedAt) against the sent token/timestamp rather than
n.user.updatedAt so only notifications referencing the same stale token are
considered for cleanup.
---
Nitpick comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 70-80: The markPushResults in NotificationService assumes
notifications and responses are the same length and can throw
IndexOutOfBoundsException; add a defensive size check and only iterate up to
Math.min(notifications.size(), responses.size()) (or throw a clear
IllegalStateException if sizes differ) so
Notification.markPushSuccess()/markPushFailure(...) are only called with
matching FcmSendResponse entries; reference the method markPushResults and the
types Notification and FcmSendResponse when making the change.
In `@src/main/resources/db/migration/V236__add_fcm_result_to_notification.sql`:
- Around line 1-4: The migration adds is_push_success, fcm_error_code and
fcm_messaging_error_code to the notification table but doesn’t add an index for
the cleanup scheduler; add a composite index (e.g.
idx_notification_push_failure) on (is_push_success, fcm_messaging_error_code,
created_at) so queries filtering by is_push_success and fcm_messaging_error_code
and a time range are optimized; modify V236__add_fcm_result_to_notification.sql
to create that index after adding the columns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e675769-b3bb-4d65-a39f-efeeac94904b
📒 Files selected for processing (11)
src/main/java/in/koreatech/koin/admin/notification/service/AdminNotificationService.javasrc/main/java/in/koreatech/koin/domain/notification/model/Notification.javasrc/main/java/in/koreatech/koin/domain/notification/repository/NotificationJdbcRepository.javasrc/main/java/in/koreatech/koin/domain/notification/repository/NotificationRepository.javasrc/main/java/in/koreatech/koin/domain/notification/scheduler/NotificationDeviceTokenCleanupScheduler.javasrc/main/java/in/koreatech/koin/domain/notification/service/NotificationDeviceTokenCleanupService.javasrc/main/java/in/koreatech/koin/domain/notification/service/NotificationService.javasrc/main/java/in/koreatech/koin/domain/user/repository/UserRepository.javasrc/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.javasrc/main/java/in/koreatech/koin/infrastructure/fcm/FcmSendResponse.javasrc/main/resources/db/migration/V236__add_fcm_result_to_notification.sql
Comment on lines
+29
to
+42
| @Query(""" | ||
| SELECT DISTINCT n.user.id | ||
| FROM Notification n | ||
| WHERE n.pushSuccess = false | ||
| AND n.fcmMessagingErrorCode = 'UNREGISTERED' | ||
| AND n.createdAt >= :start | ||
| AND n.createdAt < :end | ||
| AND n.user.deviceToken IS NOT NULL | ||
| AND n.user.updatedAt <= n.createdAt | ||
| """) | ||
| List<Integer> findUnregisteredPushFailureUserIds( | ||
| @Param("start") LocalDateTime start, | ||
| @Param("end") LocalDateTime end | ||
| ); |
There was a problem hiding this comment.
updatedAt is too coarse to guard token freshness.
User.updatedAt is the entity-wide audit field from BaseEntity, so any later user-row update can make this predicate false even when the same invalid deviceToken is still stored. That means the cleanup job can silently skip exactly the stale tokens this PR is trying to remove. Please base this guard on token-specific state instead, e.g. persist the token used for the send on Notification or compare against a dedicated deviceTokenUpdatedAt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/in/koreatech/koin/domain/notification/repository/NotificationRepository.java`
around lines 29 - 42, The query in
NotificationRepository.findUnregisteredPushFailureUserIds relies on
User.updatedAt which is a coarse entity-wide audit field and can falsely skip
stale device tokens; change the predicate to use a token-specific timestamp or
persisted token on the Notification entity instead (e.g. add
Notification.deviceToken and/or Notification.deviceTokenUpdatedAt and persist
the token/timestamp when sending), then update the JPQL in
findUnregisteredPushFailureUserIds to compare n.deviceToken (or
n.deviceTokenUpdatedAt) against the sent token/timestamp rather than
n.user.updatedAt so only notifications referencing the same stale token are
considered for cleanup.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔍 개요
🚀 주요 변경 내용
FcmSendResponse로 반환하도록 변경했습니다.NotificationService경로를 사용하도록 변경해 전송 결과가 저장되도록 했습니다.UNREGISTERED실패 알림을 기준으로 유저device_token을 정리하는 스케줄러를 추가했습니다.notification테이블에 FCM 전송 결과 저장 컬럼을 추가했습니다.💬 참고 사항
✅ Checklist (완료 조건)
Summary by CodeRabbit
New Features
Improvements
Infrastructure