Repository navigation
feat: 코스 일정 푸시 알림 기능 구현 - #48
Conversation
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (43)
📝 WalkthroughWalkthroughAdded Firebase Cloud Messaging integration, FCM token APIs, scheduled D-1 and D-0 course reminders, notification delivery tracking, retry handling, and related tests and configuration. ChangesCourse notification delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant ReminderService
participant NotificationRepository
participant DeliveryWorker
participant FirebaseGateway
Scheduler->>ReminderService: Create daily course reminders
ReminderService->>NotificationRepository: Save unique pending notifications
Scheduler->>DeliveryWorker: Dispatch retryable notifications
DeliveryWorker->>FirebaseGateway: Send notification to FCM token
FirebaseGateway-->>DeliveryWorker: Return delivery result
DeliveryWorker->>NotificationRepository: Mark sent or schedule retry
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: 8
🧹 Nitpick comments (7)
src/test/java/com/daytodo/global/config/TargetApiSecurityTest.java (1)
60-60: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd security tests for the FCM token endpoints.
Add unauthenticated POST and DELETE requests for
/users/fcm-token. Expect401 Unauthorizedand the standard error payload.The controller unit tests set
SecurityContextHolderdirectly. They do not verify the SecurityConfig filter chain for these new state-changing endpoints.🤖 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/test/java/com/daytodo/global/config/TargetApiSecurityTest.java` at line 60, Add integration security coverage in TargetApiSecurityTest for unauthenticated POST and DELETE requests to /users/fcm-token, exercising the configured filter chain rather than setting SecurityContextHolder directly. Assert each request returns 401 Unauthorized and the standard error payload, using the existing fcmTokenService mock where required.src/test/java/com/daytodo/domain/user/service/CourseReminderServiceTest.java (1)
45-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd D-0 reminder coverage.
This test verifies only
COURSE_D1. It stubs the D-0 candidate query as empty, so it cannot detect a regression increateReminders(today, COURSE_D0).Add a case with a course dated
TODAYand verifyNotificationType.COURSE_D0.🤖 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/test/java/com/daytodo/domain/user/service/CourseReminderServiceTest.java` around lines 45 - 64, Add a separate D-0 test alongside createsD1ReminderForUserWithDefaultSettingsAndToken using a course dated TODAY, stub the corresponding candidate query to return its member, and verify eventCreator.create is called with NotificationType.COURSE_D0.src/test/java/com/daytodo/domain/user/service/NotificationDeliveryWorkerTest.java (1)
57-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the retry limit.
This test verifies that the first failed delivery gets a retry time. It does not verify the required maximum of five attempts.
Add boundary cases for the final allowed attempt and the next delivery attempt. Verify that the terminal failure does not schedule another retry.
🤖 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/test/java/com/daytodo/domain/user/service/NotificationDeliveryWorkerTest.java` around lines 57 - 70, Extend removesInvalidTokenAndSchedulesRetry and related worker tests to cover the retry boundary: verify the fifth failed delivery remains retryable with a retry time, then verify the sixth delivery becomes terminal and does not schedule another retry. Assert the delivery status and nextRetryAt for both boundary cases while preserving the existing invalid-token removal assertions.src/main/java/com/daytodo/domain/user/service/CourseNotificationScheduler.java (1)
18-30: 🩺 Stability & Availability | 🔵 TrivialPlan for multi-instance scheduling and scheduler capacity.
Both jobs run on every application instance. Reminder creation stays idempotent through the duplicate check, and dispatch is serialized by the pessimistic lock, so correctness holds. Throughput and log noise still multiply with instance count.
Two operational points for deployment:
- If you run more than one instance, gate these jobs with a distributed lock (for example ShedLock) or a leader election.
- The default Spring scheduler pool holds one thread. The daily job now performs reminder creation and dispatch in sequence, so a slow dispatch delays other scheduled tasks. Size the task scheduler pool explicitly.
🤖 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/com/daytodo/domain/user/service/CourseNotificationScheduler.java` around lines 18 - 30, The scheduling configuration around CourseNotificationScheduler must support multi-instance deployments and avoid serializing unrelated scheduled work. Configure a distributed lock or leader-election guard for createDailyReminders and retryFailedNotifications, and explicitly size the Spring task scheduler pool so dispatch work cannot block other scheduled tasks.src/main/java/com/daytodo/domain/course/repository/CourseMemberRepository.java (1)
24-39: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider paginating the reminder candidate query.
findReminderCandidatesreturns an unbounded list. The daily job loads every joined member of every course on the target date into memory at once, andCourseReminderService.createRemindersthen builds three additional collections from that list. As course volume grows, this becomes a large single fetch on a scheduled thread.Add a
Pageableparameter and process candidates in slices, or drive the loop with keyset pagination on(course.courseId, user.id), which the existingorder byalready supports.🤖 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/com/daytodo/domain/course/repository/CourseMemberRepository.java` around lines 24 - 39, Update findReminderCandidates to avoid returning all reminder candidates in one unbounded fetch by adding pagination support, preferably a Pageable parameter with a stable page order matching course.courseId and user.id. Update CourseReminderService.createReminders and its scheduled processing loop to fetch and process candidates in slices until exhausted, preserving the existing filtering and reminder behavior.src/main/java/com/daytodo/domain/user/push/DisabledPushNotificationGateway.java (1)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisabled Firebase produces five failed delivery attempts per notification.
This gateway always returns a retryable failure. With Firebase disabled,
NotificationDispatchServicepicks each notification up on every cycle untilattemptCountreaches 5, and each notification finally persists asFAILEDwith an error message. Local and CI environments accumulate this state without value.Consider marking these notifications as terminal instead of retryable. One option is a
skippedoutcome onPushResultthat the worker maps to a non-retryable status. Add a single warn-level log at startup so the disabled state stays visible.🤖 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/com/daytodo/domain/user/push/DisabledPushNotificationGateway.java` around lines 12 - 15, Update DisabledPushNotificationGateway.send and the PushResult/NotificationDispatchService outcome flow so disabled Firebase notifications are treated as terminal skipped results rather than retryable failures, preventing repeated dispatch attempts and FAILED persistence; also add one warn-level startup log indicating Firebase push notifications are disabled.src/main/java/com/daytodo/domain/user/repository/NotificationRepository.java (1)
39-41: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNarrow the pessimistic lock to the notification-only delivery path.
findByIdForDelivery()currently lock-fetchesnotification.user, so the delivery transaction can hold locks on the user row while sending FCM requests. Keep the pessimistic write on the notification result and avoid the fetch join unless the worker needs the user entity before theFCM_TOKENquery;notification.getUser().getId()can be read from the managed FK association.🤖 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/com/daytodo/domain/user/repository/NotificationRepository.java` around lines 39 - 41, Update findByIdForDelivery in NotificationRepository to remove the join fetch of notification.user while preserving the PESSIMISTIC_WRITE lock on the notification result. Ensure delivery can obtain notification.getUser().getId() from the managed association before the FCM_TOKEN query without locking or eagerly fetching the user row.
🤖 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 @.env.example:
- Line 13: Quote the COURSE_REMINDER_CRON value in the dotenv example so the
six-field expression, including its spaces, is preserved when parsed by
shell-compatible dotenv tooling.
In `@build.gradle`:
- Around line 60-61: Update FirebasePushNotificationGateway’s message
construction to replace Message.builder().setToken(...) with the supported
setFid(...) API, preserving the existing recipient value and payload
configuration unless the client contract explicitly requires server-provided FCM
tokens.
In `@src/main/java/com/daytodo/domain/user/entity/FcmToken.java`:
- Line 35: The FcmTokenService.register flow must atomically handle a globally
unique token across different users. Replace the separate token
lookup-and-insert sequence with a database upsert, or catch the
unique-constraint conflict, reload the existing token, and reassign it in a new
transaction while preserving the intended token ownership.
In `@src/main/java/com/daytodo/domain/user/entity/Notification.java`:
- Around line 26-32: Add a checked-in Flyway/Liquibase migration for the
notification schema matching the entity fields, including delivery_status and
attempt_count with appropriate non-null defaults, backfill existing rows
deterministically, resolve duplicate user/course/type records before creating
uk_notification_user_course_type, and add the unique constraint after cleanup.
In `@src/main/java/com/daytodo/domain/user/service/FcmTokenService.java`:
- Around line 33-36: Update the token registration flow in FcmTokenService
around fcmTokenRepository.findByToken so concurrent inserts for the unique token
are handled safely: use a database-native upsert or catch the save unique-key
conflict, re-read the existing FcmToken, and register it for the requested user
and platform. Preserve the existing behavior for tokens already found.
In
`@src/main/java/com/daytodo/domain/user/service/NotificationDeliveryWorker.java`:
- Around line 29-70: Refactor NotificationDeliveryWorker.deliver so the
pessimistic notification lock is held only during short claim and result-update
transactions: load/claim the retryable notification, perform token sends via
pushGateway.send outside any transaction, then persist the final sent/failed
state in a separate transaction. Update FirebaseConfig to configure explicit
connectTimeout and readTimeout for the Firebase HTTP client used by
FirebaseMessaging.send.
In
`@src/main/java/com/daytodo/domain/user/service/NotificationDispatchService.java`:
- Around line 16-34: Remove the class-level `@Transactional`(readOnly = true) from
NotificationDispatchService so dispatchRetryable does not run
deliveryWorker.deliver within one read-only transaction. Ensure
NotificationDeliveryWorker.deliver executes in an independent writable
transaction, using Propagation.REQUIRES_NEW if needed to avoid retaining batch
locks during FCM calls.
In `@src/main/resources/application.yml`:
- Around line 62-64: Update FirebaseConfig startup validation to require a
non-empty resolved firebase.project-id whenever firebase.enabled is true, and
fail initialization immediately when it is missing. Ensure validation occurs
before Firebase initialization or FirebasePushNotificationGateway delivery can
proceed, while leaving disabled Firebase behavior unchanged.
---
Nitpick comments:
In
`@src/main/java/com/daytodo/domain/course/repository/CourseMemberRepository.java`:
- Around line 24-39: Update findReminderCandidates to avoid returning all
reminder candidates in one unbounded fetch by adding pagination support,
preferably a Pageable parameter with a stable page order matching
course.courseId and user.id. Update CourseReminderService.createReminders and
its scheduled processing loop to fetch and process candidates in slices until
exhausted, preserving the existing filtering and reminder behavior.
In
`@src/main/java/com/daytodo/domain/user/push/DisabledPushNotificationGateway.java`:
- Around line 12-15: Update DisabledPushNotificationGateway.send and the
PushResult/NotificationDispatchService outcome flow so disabled Firebase
notifications are treated as terminal skipped results rather than retryable
failures, preventing repeated dispatch attempts and FAILED persistence; also add
one warn-level startup log indicating Firebase push notifications are disabled.
In
`@src/main/java/com/daytodo/domain/user/repository/NotificationRepository.java`:
- Around line 39-41: Update findByIdForDelivery in NotificationRepository to
remove the join fetch of notification.user while preserving the
PESSIMISTIC_WRITE lock on the notification result. Ensure delivery can obtain
notification.getUser().getId() from the managed association before the FCM_TOKEN
query without locking or eagerly fetching the user row.
In
`@src/main/java/com/daytodo/domain/user/service/CourseNotificationScheduler.java`:
- Around line 18-30: The scheduling configuration around
CourseNotificationScheduler must support multi-instance deployments and avoid
serializing unrelated scheduled work. Configure a distributed lock or
leader-election guard for createDailyReminders and retryFailedNotifications, and
explicitly size the Spring task scheduler pool so dispatch work cannot block
other scheduled tasks.
In
`@src/test/java/com/daytodo/domain/user/service/CourseReminderServiceTest.java`:
- Around line 45-64: Add a separate D-0 test alongside
createsD1ReminderForUserWithDefaultSettingsAndToken using a course dated TODAY,
stub the corresponding candidate query to return its member, and verify
eventCreator.create is called with NotificationType.COURSE_D0.
In
`@src/test/java/com/daytodo/domain/user/service/NotificationDeliveryWorkerTest.java`:
- Around line 57-70: Extend removesInvalidTokenAndSchedulesRetry and related
worker tests to cover the retry boundary: verify the fifth failed delivery
remains retryable with a retry time, then verify the sixth delivery becomes
terminal and does not schedule another retry. Assert the delivery status and
nextRetryAt for both boundary cases while preserving the existing invalid-token
removal assertions.
In `@src/test/java/com/daytodo/global/config/TargetApiSecurityTest.java`:
- Line 60: Add integration security coverage in TargetApiSecurityTest for
unauthenticated POST and DELETE requests to /users/fcm-token, exercising the
configured filter chain rather than setting SecurityContextHolder directly.
Assert each request returns 401 Unauthorized and the standard error payload,
using the existing fcmTokenService mock where required.
🪄 Autofix
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 Plus
Run ID: 325ec330-69c0-4259-bfa7-c34321da01c9
📒 Files selected for processing (40)
.env.exampleREADME.mdbuild.gradlesrc/main/java/com/daytodo/domain/auth/service/AuthService.javasrc/main/java/com/daytodo/domain/course/repository/CourseMemberRepository.javasrc/main/java/com/daytodo/domain/user/controller/UserController.javasrc/main/java/com/daytodo/domain/user/dto/UserRequest.javasrc/main/java/com/daytodo/domain/user/dto/UserResponse.javasrc/main/java/com/daytodo/domain/user/entity/FcmToken.javasrc/main/java/com/daytodo/domain/user/entity/Notification.javasrc/main/java/com/daytodo/domain/user/entity/UserNotificationSetting.javasrc/main/java/com/daytodo/domain/user/enums/DevicePlatform.javasrc/main/java/com/daytodo/domain/user/enums/NotificationDeliveryStatus.javasrc/main/java/com/daytodo/domain/user/push/DisabledPushNotificationGateway.javasrc/main/java/com/daytodo/domain/user/push/FirebasePushNotificationGateway.javasrc/main/java/com/daytodo/domain/user/push/PushNotificationGateway.javasrc/main/java/com/daytodo/domain/user/repository/FcmTokenRepository.javasrc/main/java/com/daytodo/domain/user/repository/NotificationRepository.javasrc/main/java/com/daytodo/domain/user/repository/UserNotificationSettingRepository.javasrc/main/java/com/daytodo/domain/user/repository/WithdrawnUserCleanupRepository.javasrc/main/java/com/daytodo/domain/user/service/CourseNotificationScheduler.javasrc/main/java/com/daytodo/domain/user/service/CourseReminderService.javasrc/main/java/com/daytodo/domain/user/service/FcmTokenService.javasrc/main/java/com/daytodo/domain/user/service/NotificationDeliveryWorker.javasrc/main/java/com/daytodo/domain/user/service/NotificationDispatchService.javasrc/main/java/com/daytodo/domain/user/service/NotificationEventCreator.javasrc/main/java/com/daytodo/domain/user/service/UserNotificationService.javasrc/main/java/com/daytodo/global/config/FirebaseConfig.javasrc/main/java/com/daytodo/global/config/SecurityConfig.javasrc/main/java/com/daytodo/global/config/TimeConfig.javasrc/main/resources/application.ymlsrc/test/java/com/daytodo/DayTodoApplicationTests.javasrc/test/java/com/daytodo/domain/course/service/CourseServiceTest.javasrc/test/java/com/daytodo/domain/user/controller/UserControllerTest.javasrc/test/java/com/daytodo/domain/user/service/CourseReminderServiceTest.javasrc/test/java/com/daytodo/domain/user/service/FcmTokenServiceTest.javasrc/test/java/com/daytodo/domain/user/service/NotificationDeliveryWorkerTest.javasrc/test/java/com/daytodo/domain/user/service/UserNotificationServiceTest.javasrc/test/java/com/daytodo/domain/user/service/WithdrawnUserDeletionIntegrationTest.javasrc/test/java/com/daytodo/global/config/TargetApiSecurityTest.java
| FIREBASE_PROJECT_ID= | ||
| GOOGLE_APPLICATION_CREDENTIALS= | ||
| NOTIFICATION_ZONE=Asia/Seoul | ||
| COURSE_REMINDER_CRON=0 0 9 * * * |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote the cron expression.
Line 13 contains spaces. If deployment or local tooling loads this file as a shell-compatible dotenv file, the unquoted assignment is parsed as multiple words. Quote the value to preserve the six-field cron expression.
Proposed fix
-COURSE_REMINDER_CRON=0 0 9 * * *
+COURSE_REMINDER_CRON="0 0 9 * * *"The supplied dotenv-linter warning also identifies this value.
📝 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.
| COURSE_REMINDER_CRON=0 0 9 * * * | |
| COURSE_REMINDER_CRON="0 0 9 * * *" |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 13-13: [UnorderedKey] The COURSE_REMINDER_CRON key should go before the DB_PW key
(UnorderedKey)
[warning] 13-13: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 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 @.env.example at line 13, Quote the COURSE_REMINDER_CRON value in the dotenv
example so the six-field expression, including its spaces, is preserved when
parsed by shell-compatible dotenv tooling.
Source: Linters/SAST tools
| // Firebase Cloud Messaging | ||
| implementation 'com.google.firebase:firebase-admin:9.10.0' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="src/main/java/com/daytodo/domain/user/push/FirebasePushNotificationGateway.java"
if rg -n -C 4 'setToken\(|addToken\(|addAllTokens\(' "$file"; then
echo "Deprecated token-builder usage found. Verify the gateway implementation."
else
echo "No deprecated token-builder usage found."
fiRepository: DayTodo/DayTodo_BE
Length of output: 644
🌐 Web query:
Firebase Admin Java 9.10.0 release notes message token MulticastMessage token deprecations
💡 Result:
The Firebase Admin Java SDK version 9.10.0, released on July 2, 2026, introduces a deprecation of the token-based messaging approach in favor of Firebase Installation IDs (FIDs) [1][2][3]. Key changes in version 9.10.0 include: Deprecation of Token Fields and Methods: - The token field in the Message class is deprecated [1]. - The tokens field in the MulticastMessage class is deprecated [1]. - The setToken method in Message.Builder is deprecated [1]. - The addToken and addAllTokens methods in MulticastMessage.Builder are deprecated [1]. Introduction of FID Support: - The SDK now supports sending messages using Firebase Installation IDs (FIDs) [1]. - New fields have been added: the fid field in the Message class and the fids field in the MulticastMessage class [1]. - New methods have been introduced: setFid in Message.Builder, and addFid and addAllFids in MulticastMessage.Builder [1]. While the MulticastMessage class continues to support both tokens and FIDs, developers are advised to migrate to the new FID-based fields, as the token-based fields are now officially deprecated [1]. [1][2]
Citations:
- 1: https://firebase.google.com/support/release-notes/admin/java
- 2: https://github.com/firebase/firebase-admin-java/releases/tag/v9.10.0
- 3: https://github.com/firebase/firebase-admin-java/releases
Replace the Message.setToken() call with the supported FID API.
FirebasePushNotificationGateway still uses Message.builder().setToken(...), which is deprecated in Firebase Admin Java 9.10.0. Use Message.builder().setFid(...) unless the client contract already requires server-provided FCM tokens.
🤖 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 `@build.gradle` around lines 60 - 61, Update FirebasePushNotificationGateway’s
message construction to replace Message.builder().setToken(...) with the
supported setFid(...) API, preserving the existing recipient value and payload
configuration unless the client contract explicitly requires server-provided FCM
tokens.
| @JoinColumn(name = "user_id", nullable = false) | ||
| private User user; | ||
|
|
||
| @Column(name = "token", nullable = false, unique = true, length = 512) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make token registration atomic across users.
Line 35 enforces global token uniqueness. FcmTokenService.register first reads by token and then inserts. Its user-row lock does not serialize requests for different users. Two concurrent registrations of the same new token can both see no row, and one request will fail with a unique-constraint error.
Use a database upsert, or handle the constraint conflict by reloading and reassigning the token in a new transaction.
🤖 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/com/daytodo/domain/user/entity/FcmToken.java` at line 35, The
FcmTokenService.register flow must atomically handle a globally unique token
across different users. Replace the separate token lookup-and-insert sequence
with a database upsert, or catch the unique-constraint conflict, reload the
existing token, and reassign it in a new transaction while preserving the
intended token ownership.
| @Table( | ||
| name = "notification", | ||
| uniqueConstraints = @UniqueConstraint( | ||
| name = "uk_notification_user_course_type", | ||
| columnNames = {"user_id", "course_id", "notification_type"} | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate schema-management configuration and migrations.
fd -t f -i '(flyway|liquibase|migration)|^V[0-9].*__.*\.(sql|xml|yaml|yml)$' .
rg -n -i -C 3 \
'notification|delivery_status|attempt_count|uk_notification_user_course_type|flyway|liquibase|ddl-auto' \
srcRepository: DayTodo/DayTodo_BE
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant files:\n'
fd -t f 'Notification.java|application.yml|pom.xml|build.gradle|settings.gradle|flyway|liquibase|migration.sql|M[0-9]|db|schema' .
printf '\nNotification entity:\n'
cat -n src/main/java/com/daytodo/domain/user/entity/Notification.java
printf '\nApplication schema config:\n'
cat -n src/main/resources/application.yml | sed -n '1,80p'
printf '\nBuild schema-management deps:\n'
if [ -f pom.xml ]; then
rg -n -C 2 '(flyway|liquibase|maven-fmp|spring-boot-maven-plugin)' pom.xml
fi
printf '\nFind migration scripts only:\n'
fd -t f 'migration|\.sql$|flyway|liquibase' . | sed -n '1,120p'
printf '\nSearch db-related tables/corresponding scripts:\n'
rg -n -i -C 2 'create table.*notification|alter table.*notification|notification_table|delivery_status|attempt_count|uk_notification_user_course_type|unique constraint|constraint' . \
-g '*.sql' -g '*db*' -g 'src/main/resources/**' | sed -n '1,220p'Repository: DayTodo/DayTodo_BE
Length of output: 7173
Add a migration for the notification schema changes.
The live profile uses hibernate.ddl-auto: update, but there are no checked-in Flyway/Liquibase migrations for delivery_status, attempt_count, or uk_notification_user_course_type. Existing rows need backfill values for non-null columns and deterministic duplicate-resolution before the unique constraint is added.
🤖 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/com/daytodo/domain/user/entity/Notification.java` around lines
26 - 32, Add a checked-in Flyway/Liquibase migration for the notification schema
matching the entity fields, including delivery_status and attempt_count with
appropriate non-null defaults, backfill existing rows deterministically, resolve
duplicate user/course/type records before creating
uk_notification_user_course_type, and add the unique constraint after cleanup.
| @Transactional | ||
| public void deliver(Long notificationId) { | ||
| Notification notification = notificationRepository.findByIdForDelivery(notificationId) | ||
| .orElse(null); | ||
| if (!isRetryable(notification)) { | ||
| return; | ||
| } | ||
|
|
||
| var tokens = fcmTokenRepository.findAllByUser_Id(notification.getUser().getId()); | ||
| if (tokens.isEmpty()) { | ||
| notification.markFailed("등록된 FCM 토큰이 없습니다.", nextRetryAt(notification)); | ||
| return; | ||
| } | ||
|
|
||
| boolean sent = false; | ||
| var errors = new ArrayList<String>(); | ||
| for (FcmToken token : tokens) { | ||
| PushNotificationGateway.PushResult result = pushGateway.send( | ||
| token.getToken(), | ||
| notification.getTitle(), | ||
| notification.getBody(), | ||
| Map.of( | ||
| "courseId", String.valueOf(notification.getCourseId()), | ||
| "notificationType", notification.getNotificationType().name() | ||
| ) | ||
| ); | ||
| if (result.success()) { | ||
| sent = true; | ||
| } else { | ||
| if (result.invalidToken()) { | ||
| fcmTokenRepository.delete(token); | ||
| } | ||
| errors.add(result.errorMessage() == null ? "FCM 발송 실패" : result.errorMessage()); | ||
| } | ||
| } | ||
|
|
||
| if (sent) { | ||
| notification.markSent(LocalDateTime.now(clock)); | ||
| } else { | ||
| notification.markFailed(String.join(" | ", errors), nextRetryAt(notification)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the lastError column definition and any Firebase transport timeout configuration.
set -euo pipefail
fd -t f 'Notification.java' --exec rg -n -C 3 'lastError|columnDefinition|`@Column`' {}
fd -t f 'FirebaseConfig.java' --exec cat -n {}Repository: DayTodo/DayTodo_BE
Length of output: 3613
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -t f 'NotificationDeliveryWorker.java|FirebasePushNotificationGateway.java|PushNotificationGateway.java|Repository.java|NotificationRepository.java' .
echo
echo "== NotificationDeliveryWorker =="
file="$(fd -t f 'NotificationDeliveryWorker.java' . | head -n 1)"
sed -n '1,140p' "$file" | cat -n
echo
echo "== Gateway files =="
for f in $(fd -t f 'FirebasePushNotificationGateway.java|PushNotificationGateway.java' .); do
echo "--- $f"
sed -n '1,180p' "$f" | cat -n
done
echo
echo "== repository methods =="
for f in $(fd -t f 'Repository.java|NotificationRepository.java' src/main/java | head -n 20); do
if rg -q "findByIdForDelivery|PessimisticLock|PESSIMISTIC" "$f"; then
echo "--- $f"
rg -n -C 4 "findByIdForDelivery|`@Lock`|`@Query`|SetSessionTimeout|`@Modifying`|`@Transactional`" "$f"
fi
done
echo
echo "== dependency versions mentioning firebase/http client (from lockfiles/pom/build scripts) =="
for f in pom.xml build.gradle build.gradle.kts; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n -C 2 'firebase|httpclient|okhttp|firebase-admin|google-messaging|spring-boot' "$f" || true
fi
doneRepository: DayTodo/DayTodo_BE
Length of output: 14937
🌐 Web query:
Google Firebase Admin Java Messaging send synchronously send method timeout FirebaseMessagingOptions default sendTimeout AsyncMessaging transport
💡 Result:
In the Firebase Admin SDK for Java, the synchronous send method does not have a built-in, configurable timeout parameter directly on the method call [1][2]. Instead, the SDK relies on timeout settings configured globally via FirebaseOptions during the initialization of your FirebaseApp instance [3][4]. Configuration of Timeouts You can set connection, read, and write timeouts in milliseconds using the FirebaseOptions.Builder [3][5]: FirebaseOptions options = FirebaseOptions.builder.setCredentials(GoogleCredentials.getApplicationDefault).setConnectTimeout(10000) // 10 seconds.setReadTimeout(10000) // 10 seconds.setWriteTimeout(10000) // 10 seconds.build; FirebaseApp.initializeApp(options); Important Considerations: 1. Default Value is Zero: By default, these timeouts are initialized to 0, which signifies an infinite timeout [6][3]. In scenarios where the FCM server or network becomes unresponsive, this can lead to threads hanging indefinitely [6][7]. It is highly recommended to explicitly set non-zero, meaningful values [6]. 2. Scope of Settings: These timeout settings apply to most outgoing REST calls made by the SDK, but they do not affect Firebase Realtime Database or Cloud Firestore APIs [4][5]. 3. Asynchronous Alternative: If you need to enforce a specific timeout for a single request rather than a global configuration, use the sendAsync method [1][2]. This returns an ApiFuture, which allows you to call the.get(long timeout, TimeUnit unit) method to wait for the result with a specific deadline [8]: try { ApiFuture future = FirebaseMessaging.getInstance.sendAsync(message); String messageId = future.get(5, TimeUnit.SECONDS); // Wait max 5 seconds } catch (TimeoutException e) { // Handle specific request timeout } 4. Credential Resolution: Note that initial token fetching via GoogleCredentials may not strictly honor these FirebaseOptions timeouts in all edge cases, as some underlying credential libraries have their own retry logic [9]. For best results in production, ensure your infrastructure layer also enforces its own timeouts where appropriate [10].
Citations:
- 1: https://firebase.google.com/docs/reference/admin/java/reference/com/google/firebase/messaging/FirebaseMessaging
- 2: https://github.com/firebase/firebase-admin-java/blob/master/src/main/java/com/google/firebase/messaging/FirebaseMessaging.java
- 3: https://firebase.google.com/docs/reference/admin/java/reference/com/google/firebase/FirebaseOptions
- 4: https://firebase.google.com/support/release-notes/admin/java
- 5: https://github.com/firebase/firebase-admin-java/blob/main/src/main/java/com/google/firebase/FirebaseOptions.java
- 6: Not meaningful timeout default values leading to hanging requests firebase/firebase-admin-java#901
- 7: HttpRequest with 0 write timeout may make program hangs firebase/firebase-admin-java#850
- 8: https://stackoverflow.com/questions/52796068/how-to-configure-connection-timeout-on-firebaseapp-nethttptransport
- 9: connectTimeout not working if network connection is not available. firebase/firebase-admin-java#317
- 10: https://firebase.google.com/docs/cloud-messaging/scale-fcm
Shorten the transaction that holds the Notification lock.
deliver opens a transaction, locks notification with PESSIMISTIC_WRITE, then calls synchronous FirebaseMessaging.send inside that transaction. A slow or unresponsive FCM HTTP call keeps the lock and database connection held. Split this into short claim/update transactions and call pushGateway.send between them. Also set explicit Firebase connectTimeout and readTimeout in FirebaseConfig; the current FirebaseMessaging.send path does not provide per-call timeouts.
🤖 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/com/daytodo/domain/user/service/NotificationDeliveryWorker.java`
around lines 29 - 70, Refactor NotificationDeliveryWorker.deliver so the
pessimistic notification lock is held only during short claim and result-update
transactions: load/claim the retryable notification, perform token sends via
pushGateway.send outside any transaction, then persist the final sent/failed
state in a separate transaction. Update FirebaseConfig to configure explicit
connectTimeout and readTimeout for the Firebase HTTP client used by
FirebaseMessaging.send.
| firebase: | ||
| enabled: ${FIREBASE_ENABLED:false} | ||
| project-id: ${FIREBASE_PROJECT_ID:} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
ast-grep outline src/main/java/com/daytodo/global/config/FirebaseConfig.java --items all
rg -n -C 6 \
'firebase\.enabled|project-id|GOOGLE_APPLICATION_CREDENTIALS|FirebaseOptions|GoogleCredentials|FirebaseApp|DisabledPushNotificationGateway' \
src/main/java/com/daytodo/global/config/FirebaseConfig.java \
src/main/java/com/daytodo/domain/user/push \
src/main/resources/application.yml \
.env.exampleRepository: DayTodo/DayTodo_BE
Length of output: 6777
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\nPush notification gateway implementations:\n'
fd -a '.*\.java' src/main/java/com/daytodo/domain/user/push | sort
printf '\nPush config/bean wiring references:\n'
rg -n -C 5 \
'PushNotificationGateway|PushResult|firebaseMessaging|FirebaseMessaging|DisabledPushNotificationGateway|FirebaseConfig' \
src/main/java || true
printf '\nFirebase config file with line numbers:\n'
cat -n src/main/java/com/daytodo/global/config/FirebaseConfig.java
printf '\nApplication config and env example:\n'
cat -n src/main/resources/application.yml | sed -n '55,70p'
cat -n .env.example | sed -n '1,16p'Repository: DayTodo/DayTodo_BE
Length of output: 18062
🌐 Web query:
FirebaseAdmin Java FirebaseOptions projectId optional setProjectId null service account credentials get application default
💡 Result:
In the Firebase Admin SDK for Java, the projectId field within FirebaseOptions is technically optional when building your options, but your application must still be able to resolve a project ID for many Firebase features to work correctly [1][2]. If you do not explicitly call setProjectId in your FirebaseOptions.Builder, the FirebaseApp will attempt to resolve the project ID automatically during initialization [2]. It follows this lookup order [2]: 1. User-provided options: If setProjectId was called, that value is used [2]. 2. Service Account Credentials: If you provided ServiceAccountCredentials via setCredentials, the SDK attempts to extract the project ID from those credentials [2]. 3. Environment Variables: The SDK checks for the GOOGLE_CLOUD_PROJECT or GCLOUD_PROJECT environment variables [2]. When using GoogleCredentials.getApplicationDefault, if the project ID is not explicitly set in FirebaseOptions, it may not be automatically resolved unless it is present in the environment or the underlying credentials provide it [2]. While you can omit setProjectId, it is considered best practice to set it explicitly if your environment does not guarantee the presence of these specific environment variables or if you want to avoid ambiguity [3][2]. Note that while projectId is optional, setCredentials is mandatory [4]. You must initialize your FirebaseOptions with a valid credentials object (such as GoogleCredentials.getApplicationDefault) or the SDK will throw an exception [4][5]. Top results: [3][1][2][4]
Citations:
- 1: https://github.com/firebase/firebase-admin-java/blob/main/src/main/java/com/google/firebase/FirebaseOptions.java
- 2: https://github.com/firebase/firebase-admin-java/blob/master/src/main/java/com/google/firebase/FirebaseApp.java
- 3: https://firebase.google.com/docs/reference/admin/java/reference/com/google/firebase/FirebaseOptions.Builder
- 4: setCredentials is required but not mentioned in documentation when using setServiceAccountId firebase/firebase-admin-java#234
- 5: Why do I get this error? firebase/firebase-admin-java#520
Validate required Firebase project ID at startup.
With firebase.enabled=true and an empty firebase.project-id, FirebaseConfig can initialize Firebase without calling setProjectId. Add an explicit startup check that fails fast when project ID resolution is missing, so deliveries do not go through FirebasePushNotificationGateway and arrive in the retry/failure path.
🤖 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/application.yml` around lines 62 - 64, Update
FirebaseConfig startup validation to require a non-empty resolved
firebase.project-id whenever firebase.enabled is true, and fail initialization
immediately when it is missing. Ensure validation occurs before Firebase
initialization or FirebasePushNotificationGateway delivery can proceed, while
leaving disabled Firebase behavior unchanged.
nariming
left a comment
nariming
left a comment
There was a problem hiding this comment.
수고하셨습니다!!
코드래빗도 이 부분 짚었는데, NotificationDispatchService의 @transactional(readOnly = true) 확인 부탁드립니다! 안에서 호출되는 deliver()가 상태를 변경하는데 readOnly라 flush가 안 될 수 있어서 알림 재발송으로 이어질 수도 있을 것 같아요
cc9852d to
1dad57c
Compare
2 participants
📌 관련 이슈
🔎 What is this PR?
코스 일정에 맞춰 사용자에게 D-1 및 D-Day 푸시 알림을 전송하는 기능을 구현했
습니다.
POST /api/users/fcm-tokenDELETE /api/users/fcm-token✨ Changes
Asia/Seoul기준 매일 오전 9시에 알림이 생성되도록 설정했습니다.
록 구현했습니다.
PENDING,SENT,FAILED로 관리하도록 개선했습니다.삭제하도록 구현했습니다.
성했습니다.
습니다.
할 예정입니다.
📷 Result
백엔드 푸시 알림 기능으로 별도의 화면 결과는 없습니다.
💬 To. Reviewer
주세요.
에 반영할 예정입니다.
Summary by CodeRabbit