feat: 사용자별 다중 디바이스 알림 전송 기능 추가 및 리팩토링 #128
Conversation
Walkthrough단일 FCM 토큰 기반에서 다중 디바이스 기반 알림으로 전환: Device 도메인·엔티티·리포지토리·서비스와 관련 API/DTO/유스케이스/배치 로직을 추가·조정하고 User/Notification의 fcmToken 의존성을 제거했습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as UserController
participant UseCase as UserUseCase
participant Service as UserService
participant DeviceDomain as DeviceDomainService
participant DeviceRepo as DeviceRepository
participant UserDomain as UserDomainService
participant DB
Client->>Controller: PUT /me/devices (DeviceRequest)
Controller->>UseCase: registerDevice(userId, request)
UseCase->>Service: registerDevice(userId, request)
Service->>DeviceDomain: findOrCreateDevice(userId, deviceId, fcmToken)
DeviceDomain->>DeviceRepo: findByDeviceId(deviceId)
alt exists
DeviceRepo-->>DeviceDomain: Device
else not exists
DeviceDomain->>DeviceRepo: save(new Device)
DeviceRepo->>DB: INSERT Device
DB-->>DeviceRepo: DeviceEntity
DeviceRepo-->>DeviceDomain: Device
end
DeviceDomain-->>Service: done
Service->>UserDomain: findUserProfile(userId)
UserDomain->>DB: SELECT profile
DB-->>UserDomain: UserProfileVO
UserDomain-->>Service: UserProfileVO
Service-->>UseCase: UserProfileResponse
UseCase-->>Controller: ResponseEntity(200)
Controller-->>Client: 200 OK
sequenceDiagram
participant Scheduler as FcmNotificationJobConfig
participant NotifSvc as NotificationService
participant UserDomain as UserDomainService
participant DeviceDomain as DeviceDomainService
participant FCM as FcmService
participant NotifDomain as NotificationDomainService
participant DB
Scheduler->>NotifSvc: sendUnrecordedNotifications(daysThreshold)
NotifSvc->>UserDomain: findUnrecordedUsers(daysThreshold)
UserDomain-->>NotifSvc: List<NotificationTargetUserVO>
loop for each user
NotifSvc->>NotifDomain: hasActiveNotification(userId, type)
alt no active
NotifSvc->>DeviceDomain: findDevicesByUserId(userId)
DeviceDomain-->>NotifSvc: List<Device>
NotifSvc->>FCM: multicast(validTokens)
alt success
NotifSvc->>NotifDomain: createAndSaveNotification(...)
else failure
NotifSvc-->>NotifSvc: log error
end
else active
NotifSvc-->>NotifSvc: skip
end
end
NotifSvc-->>Scheduler: summary counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
apis/src/main/kotlin/org/yapp/apis/user/controller/UserController.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/user/controller/UserControllerApi.kt(3 hunks)apis/src/main/kotlin/org/yapp/apis/user/dto/request/DeviceRequest.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/user/service/UserService.kt(2 hunks)apis/src/main/kotlin/org/yapp/apis/user/usecase/UserUseCase.kt(2 hunks)batch/src/main/kotlin/org/yapp/batch/job/fcm/FcmNotificationJobConfig.kt(5 hunks)domain/src/main/kotlin/org/yapp/domain/device/Device.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/device/DeviceDomainService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/device/DeviceRepository.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/notification/Notification.kt(0 hunks)domain/src/main/kotlin/org/yapp/domain/user/User.kt(0 hunks)domain/src/main/kotlin/org/yapp/domain/user/UserDomainService.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/device/DeviceJpaRepository.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/device/DeviceRepositoryImpl.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/device/entity/DeviceEntity.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/notification/entity/NotificationEntity.kt(0 hunks)infra/src/main/kotlin/org/yapp/infra/user/entity/UserEntity.kt(0 hunks)
💤 Files with no reviewable changes (4)
- domain/src/main/kotlin/org/yapp/domain/notification/Notification.kt
- infra/src/main/kotlin/org/yapp/infra/user/entity/UserEntity.kt
- domain/src/main/kotlin/org/yapp/domain/user/User.kt
- infra/src/main/kotlin/org/yapp/infra/notification/entity/NotificationEntity.kt
🧰 Additional context used
🪛 detekt (1.23.8)
batch/src/main/kotlin/org/yapp/batch/job/fcm/FcmNotificationJobConfig.kt
[warning] 167-167: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
⏰ 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: build-validation
🔇 Additional comments (4)
apis/src/main/kotlin/org/yapp/apis/user/dto/request/DeviceRequest.kt (1)
10-29: LGTM! 디바이스 요청 DTO 구조가 적절합니다deviceId와 fcmToken 필드 모두 적절한 검증(NotBlank)이 적용되어 있고, 헬퍼 메소드를 통해 trim된 값을 제공하는 구조가 깔끔합니다.
apis/src/main/kotlin/org/yapp/apis/user/usecase/UserUseCase.kt (1)
32-34: LGTM! 메소드명이 의도를 명확하게 표현합니다
updateFcmToken에서registerDevice로의 이름 변경이 디바이스 기반 모델의 의도를 더 명확하게 전달합니다.infra/src/main/kotlin/org/yapp/infra/device/DeviceRepositoryImpl.kt (1)
9-24: LGTM! 표준 리포지토리 패턴이 올바르게 구현되었습니다도메인-엔티티 변환이 적절하게 처리되고 있으며, JPA 리포지토리에 대한 위임이 깔끔합니다.
domain/src/main/kotlin/org/yapp/domain/device/DeviceRepository.kt (1)
5-9: LGTM! 도메인 리포지토리 인터페이스가 명확합니다필요한 쿼리 메소드들이 적절하게 정의되어 있습니다.
batch/src/main/kotlin/org/yapp/batch/job/fcm/FcmNotificationJobConfig.kt
Outdated
Show resolved
Hide resolved
| data class Device private constructor( | ||
| val id: Id, | ||
| val user: User, | ||
| val deviceId: String, | ||
| val fcmToken: String, | ||
| val createdAt: LocalDateTime? = null, | ||
| val updatedAt: LocalDateTime? = null | ||
| ) { | ||
| @JvmInline | ||
| value class Id(val value: UUID) { | ||
| companion object { | ||
| fun newInstance(value: UUID) = Id(value) | ||
| } | ||
| } | ||
|
|
||
| companion object { | ||
| fun create( | ||
| user: User, | ||
| deviceId: String, | ||
| fcmToken: String | ||
| ): Device { | ||
| return Device( | ||
| id = Id.newInstance(UuidGenerator.create()), | ||
| user = user, | ||
| deviceId = deviceId, | ||
| fcmToken = fcmToken | ||
| ) | ||
| } | ||
|
|
||
| fun reconstruct( | ||
| id: Id, | ||
| user: User, | ||
| deviceId: String, | ||
| fcmToken: String, | ||
| createdAt: LocalDateTime?, | ||
| updatedAt: LocalDateTime? | ||
| ): Device { | ||
| return Device( | ||
| id = id, | ||
| user = user, | ||
| deviceId = deviceId, | ||
| fcmToken = fcmToken, | ||
| createdAt = createdAt, | ||
| updatedAt = updatedAt | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
FCM 토큰 업데이트 메소드 추가 고려
도메인 엔티티 구조는 잘 설계되었습니다. 다만 FCM 토큰은 만료되거나 변경될 수 있으므로, 기존 디바이스의 FCM 토큰을 업데이트하는 메소드를 추가하는 것을 고려해보세요.
다음과 같은 메소드 추가를 제안합니다:
fun updateFcmToken(newFcmToken: String): Device {
return this.copy(
fcmToken = newFcmToken,
updatedAt = LocalDateTime.now()
)
}🤖 Prompt for AI Agents
In domain/src/main/kotlin/org/yapp/domain/device/Device.kt around lines 8 to 55,
add a method to the Device data class that returns an updated Device with a new
fcmToken and refreshed updatedAt to handle token rotation; implement it as an
instance function that uses the data class copy(...) to set fcmToken =
newFcmToken and updatedAt = LocalDateTime.now() so immutability is preserved and
existing fields (id, user, deviceId, createdAt) remain unchanged.
| fun findOrCreateDevice(user: User, deviceId: String, fcmToken: String): Device { | ||
| val device = deviceRepository.findByDeviceId(deviceId) | ||
| return if (device != null) { | ||
| device | ||
| } else { | ||
| val newDevice = Device.create(user, deviceId, fcmToken) | ||
| deviceRepository.save(newDevice) | ||
| } | ||
| } |
There was a problem hiding this comment.
FCM 토큰 변경 시 업데이트 로직 누락
현재 로직은 기존 디바이스가 존재하면 그대로 반환하여 FCM 토큰 변경사항을 반영하지 않습니다. FCM 토큰은 앱 재설치, 토큰 만료, 사용자 로그아웃 후 재로그인 등의 상황에서 변경될 수 있으며, 이를 업데이트하지 않으면 알림이 전송되지 않는 심각한 문제가 발생합니다.
다음 diff를 적용하여 FCM 토큰을 항상 최신 상태로 유지하세요:
fun findOrCreateDevice(user: User, deviceId: String, fcmToken: String): Device {
val device = deviceRepository.findByDeviceId(deviceId)
return if (device != null) {
- device
+ if (device.fcmToken != fcmToken) {
+ val updatedDevice = device.copy(fcmToken = fcmToken, updatedAt = java.time.LocalDateTime.now())
+ deviceRepository.save(updatedDevice)
+ } else {
+ device
+ }
} else {
val newDevice = Device.create(user, deviceId, fcmToken)
deviceRepository.save(newDevice)
}
}🤖 Prompt for AI Agents
In domain/src/main/kotlin/org/yapp/domain/device/DeviceDomainService.kt around
lines 10 to 18, the current findOrCreateDevice returns an existing Device
without updating its FCM token; change the logic so that when a device is found
you compare the stored fcmToken with the provided fcmToken, and if they differ
update the device's fcmToken (via its setter or update method) and persist the
change with deviceRepository.save(device) before returning; otherwise return the
existing device unchanged.
domain/src/main/kotlin/org/yapp/domain/user/UserDomainService.kt
Outdated
Show resolved
Hide resolved
| interface DeviceJpaRepository : JpaRepository<DeviceEntity, UUID> { | ||
| fun findByDeviceId(deviceId: String): DeviceEntity? | ||
| fun findByUserId(userId: UUID): List<DeviceEntity> | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
deviceId 유니크 제약조건 확인 필요
JPA 리포지토리 인터페이스는 적절하게 정의되었습니다. 다만 deviceId가 디바이스를 식별하는 주요 키이므로, DeviceEntity에 유니크 제약조건이 설정되어 있는지 확인이 필요합니다.
다음 스크립트로 DeviceEntity의 제약조건을 확인하세요:
🏁 Script executed:
#!/bin/bash
# Description: DeviceEntity에서 deviceId 유니크 제약조건 확인
# deviceId 필드의 유니크 제약조건 찾기
rg -n -A5 -B5 "class DeviceEntity|@Column.*deviceId|@UniqueConstraint" --type=kt infra/src/main/kotlin/org/yapp/infra/device/entity/Length of output: 26
🏁 Script executed:
#!/bin/bash
# Find DeviceEntity.kt file and examine its contents
# First, find the file
echo "=== Finding DeviceEntity.kt ==="
fd -t f "DeviceEntity\.kt" infra/
# If found, display its contents
if [ -f "infra/src/main/kotlin/org/yapp/infra/device/entity/DeviceEntity.kt" ]; then
echo ""
echo "=== DeviceEntity.kt contents ==="
cat "infra/src/main/kotlin/org/yapp/infra/device/entity/DeviceEntity.kt"
fiLength of output: 1538
deviceId에 유니크 제약조건 추가 필요
DeviceEntity.kt(22-23줄)에서 deviceId 필드는 현재 유니크 제약조건 없이 정의되어 있습니다. findByDeviceId() 쿼리가 단일 엔티티를 반환하는 것으로 보아 deviceId는 고유해야 하므로, @Column 애노테이션에 unique=true를 추가하거나 @Table에 @UniqueConstraint를 추가하여 데이터베이스 레벨에서 유니크 제약조건을 보장해야 합니다.
🤖 Prompt for AI Agents
In infra/src/main/kotlin/org/yapp/infra/device/DeviceJpaRepository.kt around
lines 7 to 10, the repository exposes findByDeviceId() which returns a single
DeviceEntity but the underlying DeviceEntity.deviceId (lines ~22-23 in
DeviceEntity.kt) lacks a database-level unique constraint; update
DeviceEntity.kt to declare deviceId as unique (either add unique = true on the
@Column for deviceId or add a @UniqueConstraint on the @Table for the deviceId
column) and create/apply a corresponding migration to enforce the uniqueness at
the database level so the repository's single-entity assumption is guaranteed.
…ation 모델 수정 및 알림 관리를 위한 NotificationDomainService 도입
…cationService로 리팩토링하고, 알림 전송 로직을 분리하여 코드 가독성 및 유지보수성 향상
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
domain/src/main/kotlin/org/yapp/domain/device/DeviceDomainService.kt (1)
11-17: 기존 디바이스 FCM 토큰 갱신 누락으로 알림 실패 위험 있음기존에 등록된 deviceId를 찾아도 fcmToken을 비교·갱신하지 않아, 토큰이 재발급된 상황(앱 재설치, 만료 등)에서는 계속 오래된 토큰을 사용하게 됩니다. 그 결과 해당 기기로는 더 이상 알림이 전송되지 않는 치명적인 장애가 발생합니다. 동일한 문제가 이전 리뷰에서도 지적되었으므로 반드시 수정이 필요합니다.
@@ fun findOrCreateDevice(userId: UUID, deviceId: String, fcmToken: String) { val device = deviceRepository.findByDeviceId(deviceId) - if (device == null) { - val newDevice = Device.create(userId, deviceId, fcmToken) - deviceRepository.save(newDevice) - } + if (device == null) { + val newDevice = Device.create(userId, deviceId, fcmToken) + deviceRepository.save(newDevice) + return + } + + if (device.fcmToken != fcmToken) { + val updatedDevice = device.updateFcmToken(fcmToken) + deviceRepository.save(updatedDevice) + } }※
updateFcmToken은 도메인 구현에 맞게 적절한 갱신 메서드를 사용/추가해 주세요.domain/src/main/kotlin/org/yapp/domain/device/Device.kt (1)
15-45: 토큰 로테이션을 위한 갱신 메소드가 필요합니다.FCM 토큰은 주기적으로 변경되기 때문에 기존 디바이스 인스턴스에 대해 안전하게 갱신할 수 있는 도메인 메소드가 없으면 서비스 단에서
reconstruct로 전필드를 직접 채워야 하고, 실수로 createdAt/updatedAt을 누락하거나 잘못 넣을 위험이 큽니다.copy가 private이라 외부에서 바로 쓰기도 어려운 구조이므로, 토큰과updatedAt을 최신화하는 전용 메소드를 엔티티 내부에 두는 편이 안전합니다.data class Device private constructor( val id: Id, val userId: User.Id, val deviceId: String, val fcmToken: String, val createdAt: LocalDateTime? = null, val updatedAt: LocalDateTime? = null ) { + fun updateFcmToken(newFcmToken: String): Device { + return this.copy( + fcmToken = newFcmToken, + updatedAt = LocalDateTime.now() + ) + } + companion object { fun create( userId: UUID,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
batch/src/main/kotlin/org/yapp/batch/job/fcm/FcmNotificationJobConfig.kt(1 hunks)batch/src/main/kotlin/org/yapp/batch/service/NotificationService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/device/Device.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/device/DeviceDomainService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/device/vo/DeviceVO.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/notification/Notification.kt(6 hunks)domain/src/main/kotlin/org/yapp/domain/notification/NotificationDomainService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/user/UserDomainService.kt(3 hunks)domain/src/main/kotlin/org/yapp/domain/user/vo/NotificationTargetUserVO.kt(1 hunks)infra/src/main/kotlin/org/yapp/infra/notification/entity/NotificationEntity.kt(4 hunks)
🧰 Additional context used
🪛 detekt (1.23.8)
batch/src/main/kotlin/org/yapp/batch/service/NotificationService.kt
[warning] 134-134: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 155-155: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
⏰ 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: build-validation
🔇 Additional comments (2)
domain/src/main/kotlin/org/yapp/domain/user/vo/NotificationTargetUserVO.kt (1)
7-22: NotificationTargetUserVO 도입으로 알림 대상 데이터가 깔끔하게 분리되었습니다User 엔티티에서 필요한 속성만 안전하게 꺼내 immutable VO로 캡슐화한 구조가 명료하고 재사용에 유리해 보입니다. 👍
batch/src/main/kotlin/org/yapp/batch/job/fcm/FcmNotificationJobConfig.kt (1)
21-34: NotificationService로 책임을 위임한 구조가 좋습니다.배치가 순수 오케스트레이션에 집중하도록 정리되어 다중 디바이스 전송 흐름을 추적하기 훨씬 수월해졌습니다. 요약 로그도 운영 관점에서 도움이 될 것 같습니다.
| fun reset(): Notification { | ||
| return this.copy( | ||
| isSent = false, | ||
| sentAt = null | ||
| ) |
There was a problem hiding this comment.
reset 시점에 updatedAt도 갱신해야 합니다.
reset()이 isSent와 sentAt만 되돌리고 updatedAt은 그대로 두면, “마지막 갱신 시점”이 과거에 머물러 있어 다음 배치 주기에서 곧바로 Dormant/Unrecorded 대상으로 다시 잡히는 문제가 생깁니다. 결과적으로 동일 알림이 여러 번 재발송될 수 있으니, 초기화와 동시에 updatedAt을 현재 시각으로 갱신해야 합니다.
fun reset(): Notification {
return this.copy(
isSent = false,
- sentAt = null
+ sentAt = null,
+ updatedAt = LocalDateTime.now()
)
}📝 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.
| fun reset(): Notification { | |
| return this.copy( | |
| isSent = false, | |
| sentAt = null | |
| ) | |
| fun reset(): Notification { | |
| return this.copy( | |
| isSent = false, | |
| sentAt = null, | |
| updatedAt = LocalDateTime.now() | |
| ) | |
| } |
🤖 Prompt for AI Agents
In domain/src/main/kotlin/org/yapp/domain/notification/Notification.kt around
lines 20 to 24, the reset() function only reverts isSent and sentAt but leaves
updatedAt unchanged; update reset() to also set updatedAt to the current
timestamp (e.g. Instant.now() or the project’s time provider) so the entity’s
last-modified time reflects the reset and prevents it from being immediately
reprocessed by the next batch.
|
|
||
| @DomainService | ||
| class ReadingRecordDomainService( | ||
| class ReadingRecordDomainService( // TODO: readingRecordRepository만 남기고 제거 |
There was a problem hiding this comment.
🧩 Analysis chain
TODO 주석이 이 PR의 범위 및 목표와 맞지 않습니다.
라인 20의 TODO 주석("readingRecordRepository만 남기고 제거")은 현재 PR의 목표인 다중 디바이스 알림 기능(BOOK-408/feat)과 직접적인 연관이 없어 보입니다. 또한 현재 코드에서는 모든 주입된 의존성(tagRepository, readingRecordTagRepository, userBookRepository)이 적극적으로 사용되고 있습니다.
다음 중 하나를 고려하세요:
- 이 TODO가 별도의 리팩토링 작업 항목이라면, 별도 이슈로 분리하고 이 PR에서는 제거하기
- 이 주석이 정말 필요하다면, PR 범위에 명확히 포함시키고 작업 완료 후 제거하기
---
🏁 Script executed:
```shell
#!/bin/bash
# ReadingRecordDomainService.kt 파일의 완전한 내용 확인
cat domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
Length of output: 7656
🏁 Script executed:
#!/bin/bash
# 파일 존재 여부 확인 및 라인 수 확인
wc -l domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.ktLength of output: 86
🏁 Script executed:
#!/bin/bash
# 라인 15-30 범위 확인 (라인 20 주변)
sed -n '15,30p' domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.ktLength of output: 566
TODO 주석을 PR에서 제거하세요.
라인 20의 TODO 주석("readingRecordRepository만 남기고 제거")은 현재 코드와 맞지 않습니다. 실제로 네 개의 모든 의존성이 여러 메서드에서 활발하게 사용되고 있습니다:
tagRepository: createReadingRecord, buildReadingRecordInfoVO, findReadingRecordsByDynamicCondition, modifyReadingRecord에서 호출readingRecordTagRepository: createReadingRecord, buildReadingRecordInfoVO, findReadingRecordsByDynamicCondition, modifyReadingRecord에서 호출userBookRepository: createReadingRecord, buildReadingRecordInfoVO, findReadingRecordsByDynamicCondition, deleteReadingRecord 등에서 호출
이 주석은 이 PR의 범위(다중 디바이스 알림 기능)와도 무관하므로, 별도 이슈로 분리하거나 이 PR에서 제거해야 합니다.
🤖 Prompt for AI Agents
In
domain/src/main/kotlin/org/yapp/domain/readingrecord/ReadingRecordDomainService.kt
around line 20, remove the misleading TODO comment ("readingRecordRepository만
남기고 제거") because all four dependencies are actively used across multiple
methods; either delete the TODO from this PR or convert it into a new
issue/ticket and reference that issue ID here, then update the commit message to
reflect the cleanup (or issue creation) so the PR no longer contains the
unrelated TODO.
| val allUsers = userRepository.findByLastActivityBeforeAndNotificationEnabled( | ||
| LocalDateTime.now().plusDays(1), | ||
| true | ||
| ) | ||
|
|
||
| return allUsers.filter { user -> | ||
| val userId = user.id.value | ||
| val recentBooks = userBookRepository.findByUserIdAndCreatedAtAfter(userId, targetDate) | ||
|
|
||
| if (recentBooks.isEmpty()) { | ||
| false | ||
| } else { | ||
| val userBooks = userBookRepository.findAllByUserId(userId) | ||
| val userBookIds = userBooks.map { it.id.value } | ||
|
|
||
| val recentRecords = if (userBookIds.isNotEmpty()) { | ||
| readingRecordRepository.findByUserBookIdInAndCreatedAtAfter(userBookIds, targetDate) | ||
| } else { | ||
| emptyList() | ||
| } | ||
|
|
||
| recentRecords.isEmpty() | ||
| } | ||
| }.map { NotificationTargetUserVO.from(it) } |
There was a problem hiding this comment.
N+1 쿼리로 인해 배치 실행 시 심각한 성능 저하가 우려됩니다
findByLastActivityBeforeAndNotificationEnabled(...)로 모든 활성 사용자를 메모리에 올린 뒤, 각 사용자마다 userBookRepository.findByUserIdAndCreatedAtAfter(...), userBookRepository.findAllByUserId(...), readingRecordRepository.findByUserBookIdInAndCreatedAtAfter(...)를 순차 호출하고 있어 쿼리 수가 1 + 3N 형태로 폭증합니다. 사용자 수가 수천 명만 되어도 배치 푸시나 API 호출이 급격히 느려지고 커넥션 고갈 위험도 존재합니다. DB 단계에서 조건을 처리하도록 커스텀 리포지토리 쿼리(예: 서브쿼리/EXISTS 조합)로 재작성하거나, 최소한 대상 사용자 ID를 한 번의 질의로 추려내는 형태로 리팩터링해 주세요.
🤖 Prompt for AI Agents
domain/src/main/kotlin/org/yapp/domain/user/UserDomainService.kt around lines
141-164: the current implementation eagerly loads all users and then runs
per-user repository calls (userBookRepository.findByUserIdAndCreatedAtAfter,
userBookRepository.findAllByUserId,
readingRecordRepository.findByUserBookIdInAndCreatedAtAfter), causing an N+1
query problem; replace this with a single DB-driven selection by adding a custom
repository method (or JPQL/SQL with EXISTS/subquery) that returns only user IDs
(or users) satisfying: lastActivity before target, notification enabled, has at
least one user_book created after targetDate, and has no reading_record for
those user_books after targetDate; alternatively, fetch candidate user IDs in
two batched queries (1) select user IDs meeting lastActivity/notification, (2)
select user IDs that have recent user_books and exclude those with recent
reading_records using IN/subquery/grouping) and then map results to
NotificationTargetUserVO without per-user repository calls.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
domain/src/main/kotlin/org/yapp/domain/device/Device.kt (1)
8-46: 이전 리뷰 제안이 아직 반영되지 않았습니다 - FCM 토큰 업데이트 메소드 추가 필요이전 리뷰에서 제안된 FCM 토큰 업데이트 메소드가 아직 구현되지 않았습니다. FCM 토큰은 만료되거나 클라이언트에서 재발급될 수 있으므로, 기존 디바이스의 토큰을 갱신하는 도메인 메소드가 필요합니다. 현재
updatedAt필드가 존재하지만 이를 활용하는 업데이트 로직이 없는 상태입니다.다음과 같은 메소드 추가를 권장합니다:
+ fun updateFcmToken(newFcmToken: String): Device { + return this.copy( + fcmToken = newFcmToken, + updatedAt = LocalDateTime.now() + ) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
batch/src/main/kotlin/org/yapp/batch/config/InfraConfig.kt(1 hunks)domain/src/main/kotlin/org/yapp/domain/device/Device.kt(1 hunks)infra/src/main/resources/application-persistence.yml(1 hunks)
⏰ 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: build-validation
🔇 Additional comments (2)
domain/src/main/kotlin/org/yapp/domain/device/Device.kt (1)
49-54: LGTM! 타입 안전성을 위한 훌륭한 설계
@JvmInlinevalue class를 사용하여 런타임 오버헤드 없이 타입 안전성을 제공하는 좋은 설계입니다. UUID를 래핑하여 Device.Id와 다른 Id 타입들을 컴파일 타임에 구분할 수 있습니다.batch/src/main/kotlin/org/yapp/batch/config/InfraConfig.kt (1)
10-12: Now searching for more information about the Sentry configuration and whether it's needed for batch operations:AOP 설정은 필수입니다. SENTRY는 검토 후 진행하세요.
배치 모듈의 @transactional 애노테이션을 처리하기 위해 Spring AOP가 필요합니다. NotificationService에서 세 개의 메서드가 @transactional로 표시되어 있으므로 AOP 구성은 선택사항이 아니라 필수입니다.
반면, SENTRY 설정은 정기적으로 실행되는 배치 작업의 실패 이유를 추적하는 데 유용하지만 필수는 아닙니다. 프로덕션 환경에서 에러 모니터링이 필요한 경우에만 활성화하세요.
| fun create( | ||
| userId: UUID, | ||
| deviceId: String, | ||
| fcmToken: String | ||
| ): Device { | ||
| return Device( | ||
| id = Id.newInstance(UuidGenerator.create()), | ||
| userId = User.Id.newInstance(userId), | ||
| deviceId = deviceId, | ||
| fcmToken = fcmToken | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
입력 파라미터 검증 추가 고려
create 메소드에서 deviceId와 fcmToken에 대한 검증이 없습니다. 빈 문자열이나 잘못된 형식의 토큰이 전달될 경우 문제가 발생할 수 있습니다. 도메인 계층에서 불변 조건(invariant)을 강제하는 것이 DDD 관점에서 권장됩니다.
다음과 같은 검증 추가를 고려해보세요:
fun create(
userId: UUID,
deviceId: String,
fcmToken: String
): Device {
+ require(deviceId.isNotBlank()) { "deviceId는 비어있을 수 없습니다" }
+ require(fcmToken.isNotBlank()) { "fcmToken은 비어있을 수 없습니다" }
return Device(
id = Id.newInstance(UuidGenerator.create()),
userId = User.Id.newInstance(userId),
deviceId = deviceId,
fcmToken = fcmToken
)
}🤖 Prompt for AI Agents
In domain/src/main/kotlin/org/yapp/domain/device/Device.kt around lines 17 to
28, add input validation for deviceId and fcmToken inside the create(...)
factory: ensure deviceId and fcmToken are not null/blank (trim and check
isNotBlank), optionally enforce sensible length/format constraints (e.g.,
min/max length or a regex for token characters) and throw a clear domain-level
exception (IllegalArgumentException or a custom DomainException) when validation
fails; keep construction of Device only after checks pass and update/cover unit
tests accordingly.
| database-platform: org.hibernate.dialect.MySQL8Dialect | ||
| hibernate: | ||
| ddl-auto: update | ||
| ddl-auto: validate |
There was a problem hiding this comment.
🧩 Analysis chain
Flyway 마이그레이션 스크립트 존재 여부를 확인해주세요.
ddl-auto를 validate로 변경하고 Flyway를 활성화한 것은 프로덕션 환경에서 스키마 관리를 위한 좋은 방향입니다. 그러나 baseline-on-migrate: false 설정과 함께 사용되므로, Device 엔티티 및 관련 스키마 변경사항을 커버하는 Flyway 마이그레이션 스크립트가 db/migration/mysql 경로에 반드시 존재해야 합니다. 마이그레이션 스크립트가 없으면 애플리케이션 시작 시 실패할 수 있습니다.
다음 스크립트를 실행하여 Flyway 마이그레이션 스크립트의 존재 여부와 Device 관련 스키마 변경사항이 포함되어 있는지 확인해주세요:
Also applies to: 19-19
🏁 Script executed:
#!/bin/bash
# Description: Verify Flyway migration scripts exist and cover Device entity changes
# Check for migration scripts in the specified location
echo "=== Checking for Flyway migration scripts ==="
fd -t f -e sql . infra/src/main/resources/db/migration/mysql
echo -e "\n=== Checking for Device-related schema changes in migrations ==="
rg -i "device" infra/src/main/resources/db/migration/mysql --type sql -C 3Length of output: 364
마이그레이션 스크립트 분석 결과를 바탕으로 추가 검증이 필요합니다. Device 엔티티 관련 스키마가 마이그레이션 스크립트에 없는 것으로 보이므로, Device 엔티티의 존재 여부와 초기 마이그레이션에 포함 여부를 확인하겠습니다.
🏁 Script executed:
#!/bin/bash
# Verify Device entity exists and check initial migration content
echo "=== Searching for Device entity class definition ==="
fd -t f \( -name "*.java" -o -name "*.kt" \) . | xargs rg -l "class Device" -g '!**/test/**' 2>/dev/null || echo "No Device entity found"
echo -e "\n=== Checking V1__init.sql for Device table ==="
grep -i "device" infra/src/main/resources/db/migration/mysql/V1__init.sql || echo "No 'device' table in V1__init.sql"
echo -e "\n=== Displaying first 100 lines of V1__init.sql to see structure ==="
head -100 infra/src/main/resources/db/migration/mysql/V1__init.sql
echo -e "\n=== Checking git log for Device entity recent changes ==="
git log --oneline --all -- "*Device*" 2>/dev/null | head -20 || echo "Git history unavailable"Length of output: 4530
DeviceEntity가 JPA 엔티티로 정의되어 있는지 확인하겠습니다.
🏁 Script executed:
#!/bin/bash
# Verify DeviceEntity is a JPA entity mapped to database table
echo "=== Inspecting DeviceEntity.kt ==="
cat infra/src/main/kotlin/org/yapp/infra/device/entity/DeviceEntity.kt
echo -e "\n=== Checking for Device table creation migration ==="
ls -la infra/src/main/resources/db/migration/mysql/ | grep -i device || echo "No device migration found"
echo -e "\n=== List all migration files in order ==="
ls -1 infra/src/main/resources/db/migration/mysql/Length of output: 1672
DeviceEntity용 Flyway 마이그레이션 스크립트 필수 생성
치명적 문제가 확인되었습니다. DeviceEntity는 @Entity 및 @Table(name = "device")로 매핑된 JPA 엔티티이지만, 데이터베이스 마이그레이션에서 device 테이블 생성 스크립트가 완전히 누락되어 있습니다.
현재 설정(ddl-auto: validate, flyway enabled: true, baseline-on-migrate: false)에서 애플리케이션은 시작 시 다음 오류로 실패합니다:
- Hibernate가
device테이블을 데이터베이스에서 찾으려 함 - 테이블이 존재하지 않으므로 SchemaValidationException 발생
- 애플리케이션 시작 불가
필수 조치:
infra/src/main/resources/db/migration/mysql/ 경로에 다음 버전의 Flyway 마이그레이션 스크립트를 생성해야 합니다 (예: V2__create_device_table.sql):
CREATE TABLE device (
id VARCHAR(36) NOT NULL,
created_at datetime(6) NOT NULL,
updated_at datetime(6) NOT NULL,
deleted_at datetime(6) NULL,
user_id VARCHAR(36) NOT NULL,
device_id VARCHAR(255) NOT NULL,
fcm_token VARCHAR(1024) NOT NULL,
CONSTRAINT pk_device PRIMARY KEY (id)
);이 마이그레이션 없이는 병합할 수 없습니다.
🤖 Prompt for AI Agents
infra/src/main/resources/application-persistence.yml around line 11: Hibernate
validation fails because the JPA DeviceEntity maps to table "device" but no
Flyway migration creates that table; add a new Flyway SQL migration file under
infra/src/main/resources/db/migration/mysql/ (e.g. V2__create_device_table.sql)
that creates the device table with columns id, created_at, updated_at,
deleted_at, user_id, device_id, fcm_token and a primary key on id, ensuring
column types and nullability match the entity; commit the migration file so
Flyway will run it before Hibernate validation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
infra/src/main/kotlin/org/yapp/infra/notification/repository/JpaNotificationRepository.kt(0 hunks)infra/src/main/kotlin/org/yapp/infra/notification/repository/impl/NotificationRepositoryImpl.kt(1 hunks)
💤 Files with no reviewable changes (1)
- infra/src/main/kotlin/org/yapp/infra/notification/repository/JpaNotificationRepository.kt
⏰ 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: build-validation
| override fun findByUser(user: User): Notification? { | ||
| val userEntity = UserEntity.fromDomain(user) | ||
| return jpaNotificationRepository.findByUser(userEntity)?.toDomain() | ||
| return jpaNotificationRepository.findByUserId(user.id.value).firstOrNull()?.toDomain() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
.firstOrNull()의 임의 선택 문제 및 데이터 모델 검증 필요
jpaNotificationRepository.findByUserId()가 List<NotificationEntity>를 반환하는데 .firstOrNull()로 첫 번째 요소만 선택하고 있습니다. 이는 다음과 같은 문제를 야기할 수 있습니다:
- 비결정적 동작: 정렬 기준(ORDER BY)이 명시되지 않아 어떤 알림이 반환될지 예측할 수 없습니다.
- 의도 불명확: 한 사용자가 여러 개의
Notification을 가질 수 있다면 왜 첫 번째만 반환하나요? 반대로 한 사용자당 하나의 알림 설정만 존재해야 한다면 왜 리스트를 반환하는 쿼리를 사용하나요? - API 혼란:
findByUser(단일 반환)와findByUserId(리스트 반환) 메소드가 모두 존재하여 혼란을 야기합니다.
다음 사항을 검증해 주세요:
권장사항:
- 사용자당 하나의 알림 설정만 존재해야 한다면: DB에 unique constraint 추가 및 단일 결과를 반환하는 전용 쿼리 사용
- 여러 알림이 존재할 수 있다면: 명시적인 정렬 기준 추가 또는 메소드명과 반환 타입 재검토 필요
🏁 Script executed:
#!/bin/bash
# Description: Notification과 User 간의 관계가 1:1인지 1:N인지 확인
# 1. NotificationEntity와 User 간의 관계 확인
echo "=== Checking Notification-User relationship ==="
ast-grep --pattern 'class NotificationEntity {
$$$
}'
# 2. findByUserId 쿼리 정의 확인 (정렬 여부 포함)
echo "=== Checking JpaNotificationRepository query definition ==="
rg -A 5 "fun findByUserId" --type kotlin
# 3. findByUser 메소드 사용처 확인
echo "=== Checking usage of findByUser method ==="
rg -n "\.findByUser\(" --type kotlin -C 3Length of output: 9075
🏁 Script executed:
#!/bin/bash
# More comprehensive verification of Notification data model and relationships
# 1. Find NotificationEntity class definition (broader search)
echo "=== NotificationEntity class definition ==="
rg -n "class NotificationEntity" --type kotlin -A 15
# 2. Find all @ManyToOne or relationship annotations in notification context
echo "=== Relationship annotations in NotificationEntity ==="
rg -B 2 -A 2 "@ManyToOne|@OneToOne|@OneToMany" infra/src/main/kotlin/org/yapp/infra/notification --type kotlin
# 3. Full NotificationRepositoryImpl to see all method signatures
echo "=== NotificationRepositoryImpl full content ==="
cat infra/src/main/kotlin/org/yapp/infra/notification/repository/impl/NotificationRepositoryImpl.kt
# 4. Check domain NotificationRepository interface
echo "=== Domain NotificationRepository interface ==="
rg -n "interface NotificationRepository|fun find" domain/src/main/kotlin/org/yapp/domain/notification --type kotlin -A 2
# 5. Broader search for findByUser usage (excluding test comments)
echo "=== Broader search for findByUser usage ==="
rg -n "findByUser" --type kotlin -C 2Length of output: 22696
🏁 Script executed:
#!/bin/bash
# Final verification: Is findByUser() actually used anywhere in codebase?
# 1. Search for actual calls to findByUser (different from definition)
echo "=== Calls to findByUser method ==="
rg -rn "\.findByUser\s*\(" --type kotlin -C 2
# 2. Check if it's in tests
echo "=== findByUser in test files ==="
rg -rn "findByUser" --type kotlin | grep -i test
# 3. Search in application/api layer
echo "=== findByUser usage in service/api layers ==="
rg -rn "findByUser" apis/ domain/ --type kotlin -C 1Length of output: 386
.firstOrNull()로 인한 비결정적 동작 및 설계 불일치 문제 확인됨
검증 결과, 원래 지적한 문제가 확실합니다:
- 데이터 모델: NotificationEntity는 userId를 plain
@Column으로 정의하여 사용자당 여러 알림을 허용합니다. - 쿼리 설계:
findByUserId()는List<NotificationEntity>를 반환하도록 설계되었습니다. - 실제 사용 사례:
NotificationDomainService.hasActiveNotification()에서findByUserId()를 호출해.any()로 여러 알림을 순회하고 있어, 사용자당 여러 알림이 존재할 수 있음을 확인했습니다. - 문제:
findByUser()메소드는 정렬 기준 없이.firstOrNull()을 사용하여 리스트에서 임의의 첫 번째 요소를 반환합니다.
필요한 조치:
- 사용자당 정확히 하나의 알림만 존재해야 한다면: 데이터베이스에 UNIQUE 제약조건을 추가하고 단일 결과를 반환하는 전용 쿼리 사용
- 여러 알림이 존재할 수 있다면: 명시적인
ORDER BY절 추가 (예:orderByCreatedAtDesc) 또는findByUser()메소드의 의도 재검토
override fun findByUser(user: User): Notification? {
return jpaNotificationRepository.findByUserId(user.id.value)
.sortedByDescending { it.createdAt } // 또는 적절한 정렬 기준
.firstOrNull()
?.toDomain()
}🤖 Prompt for AI Agents
In
infra/src/main/kotlin/org/yapp/infra/notification/repository/impl/NotificationRepositoryImpl.kt
around lines 23–25, the method uses .firstOrNull() on an unordered list returned
by findByUserId(), causing nondeterministic behavior; either enforce
single-notification-per-user by adding a UNIQUE constraint to the DB and replace
the repository query with a single-result method (e.g., findByUserId returning
Optional/nullable entity) or, if multiple notifications per user are valid,
change the repository query to include an explicit ORDER BY (for example by
createdAt DESC) and then pick the first element, or alternatively change
findByUser to return a list of domain notifications so callers handle multiple
items — implement the chosen option and update repository signature and mapping
accordingly.
|
|
LGTM |
🔗 관련 이슈
Close [BOOK-408/feat] 멀터디바이스를 고려한 알림기능 설계 및 기능개발 #127
📘 작업 유형
✨ Feature (기능 추가)
🐞 Bugfix (버그 수정)
🔧 Refactor (코드 리팩토링)
⚙️ Chore (환경 설정)
📝 Docs (문서 작성 및 수정)
✅ Test (기능 테스트)
🎨 style (코드 스타일 수정)
📙 작업 내역
deviceId기반의 알림 전송 기능 추가사용자별 다중 디바이스 지원 기능 구현
관련 API 엔드포인트 및 메소드명 변경
코드 구조 개선 및 중복 로직 리팩토링
✅ PR 체크리스트
커밋 메시지가 명확합니다
PR 제목이 컨벤션에 맞습니다
관련 이슈 번호를 작성했습니다
기능이 정상적으로 작동합니다
불필요한 코드를 제거했습니다
💬 추가 설명 or 리뷰 포인트
유저별로 여러 디바이스를 등록하고, 각 디바이스에 알림을 전송할 수 있도록 기능을 추가했습니다.
기존 단일 디바이스 구조를 다중 디바이스로 리팩토링하면서, API 구조를 단순화했습니다.
리뷰 시 디바이스 식별 및 알림 로직의 확장성에 중점을 두고 확인 부탁드립니다.
Summary by CodeRabbit
새로운 기능
기능 개선