FCM 토큰 등록 API 구현 - #47
Open
jminkkk wants to merge 26 commits into
Open
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFirebase Admin SDK 추가 후 FCM 토큰 등록(동시성 처리), 비동기 FCM 푸시, 알림 엔드포인트(목록/단건 읽음/전체 읽음), 엔티티·리포지토리·서비스·컨트롤러 및 관련 테스트와 환경 구성을 추가합니다. ChangesFCM 토큰 및 알림 관리
Sequence Diagram(s)sequenceDiagram
participant Client
participant FcmTokenController
participant FcmTokenService
participant FcmTokenRepository
participant FirebaseMessaging
Client->>FcmTokenController: POST /fcm-tokens (X-User-Id, token)
FcmTokenController->>FcmTokenService: registerToken(userId, token)
FcmTokenService->>FcmTokenRepository: findByToken(token) / saveAndFlush(...)
FcmTokenRepository-->>FcmTokenService: FcmToken?
FcmTokenService->>FirebaseMessaging: (optionally) send for user tokens
FirebaseMessaging-->>FcmTokenService: success / FirebaseMessagingException
FcmTokenService-->>FcmTokenController: RegisterFcmTokenResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/main/kotlin/depromeet/hotsix/obrit/notification/controller/NotificationController.kt (1)
19-22: ⚡ Quick win단건 읽음 API가
null을 반환해 갱신 결과 DTO 계약이 살아있지 않습니다.
MarkReadNotificationResponse를 실제 응답으로 내려주도록 서비스/컨트롤러 시그니처를 맞춰 주세요.As per coding guidelines, "Controller classes should use `@RestController` annotation and return only DTOs (never expose JPA entities directly)".수정 예시
- ): ApiResponse<Nothing?> { - notificationService.markAsRead(userId, notificationId) - return ApiResponse.ok(null) + ): ApiResponse<MarkReadNotificationResponse> { + val response = notificationService.markAsRead(userId, notificationId) + return ApiResponse.ok(response) }🤖 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/kotlin/depromeet/hotsix/obrit/notification/controller/NotificationController.kt` around lines 19 - 22, The controller currently returns ApiResponse.ok(null) after calling notificationService.markAsRead(userId, notificationId); change both the controller and service signature to return a MarkReadNotificationResponse DTO: update NotificationController endpoint to return ApiResponse<MarkReadNotificationResponse> and have notificationService.markAsRead(...) return a MarkReadNotificationResponse (or map the service result to that DTO) so the controller returns ApiResponse.ok(markReadResponse) instead of null; ensure you do not return any JPA entities and map domain objects to the MarkReadNotificationResponse DTO within the service or a mapper.src/main/kotlin/depromeet/hotsix/obrit/notification/service/NotificationService.kt (1)
31-33: ⚡ Quick win
markAsReadAll은 대량 데이터에서 메모리/쿼리 비용이 커질 수 있습니다.읽지 않은 알림을 벌크 업데이트로 처리하면 훨씬 효율적입니다.
리팩터링 예시
- notificationRepository.findAllByUserId(userId) - .filter { !it.isRead } - .forEach { it.markAsRead() } + notificationRepository.markAllAsReadByUserId(userId, java.time.LocalDateTime.now())// NotificationRepository.kt `@org.springframework.data.jpa.repository.Modifying` `@org.springframework.data.jpa.repository.Query`( "update Notification n set n.isRead = true, n.readAt = :readAt where n.userId = :userId and n.isRead = false" ) fun markAllAsReadByUserId(userId: Long, readAt: java.time.LocalDateTime): Int🤖 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/kotlin/depromeet/hotsix/obrit/notification/service/NotificationService.kt` around lines 31 - 33, The current mark-as-read implementation loads all notifications via notificationRepository.findAllByUserId(...).filter { !it.isRead }.forEach { it.markAsRead() } which causes unnecessary memory and query overhead for large datasets; replace this with a repository-level bulk update (e.g., add a `@Modifying` `@Query` method like markAllAsReadByUserId(userId, readAt)) and call that from NotificationService.markAsReadAll so unread notifications are updated in a single SQL update rather than iterating and updating each entity in memory.
🤖 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 `@build.gradle`:
- Line 35: Replace the invalid test dependency declaration testImplementation
'org.springframework.boot:spring-boot-webmvc-test' with the correct Spring Boot
testing starter; update the dependency to use
org.springframework.boot:spring-boot-starter-test so MockMvc and other test
utilities are available (i.e., change the artifact coordinate referenced in the
testImplementation entry).
In `@src/main/kotlin/depromeet/hotsix/obrit/global/config/FirebaseConfig.kt`:
- Line 3: FirebaseConfig 클래스가 비어 있고 사용되지 않으므로 해당 클래스를 소스에서 제거하세요: 파일에 선언된 class
FirebaseConfig를 삭제하고 프로젝트 전반에서 이 클래스에 대한 참조가 있는지 검색하여 모두 제거하거나 불필요한 import를
클린업하세요; 빌드 및 테스트를 실행하여 삭제로 인한 영향이 없는지 확인하세요.
In `@src/main/kotlin/depromeet/hotsix/obrit/notification/entity/Notification.kt`:
- Around line 37-39: The markAsRead() method currently overwrites readAt on
every call; change markAsRead() (which sets isRead and readAt using
LocalDateTime.now()) to first check if isRead is already true and return early
if so, preserving the original readAt; only when isRead is false set isRead =
true and set readAt = LocalDateTime.now().
- Around line 19-20: The userId field on Notification is marked nullable which
weakens data integrity and authorization checks; change the Notification.userId
property to be non-nullable (remove the nullable type and default null), update
its JPA mapping to enforce NOT NULL (e.g., `@Column`(name = "user_id", nullable =
false)), and adjust any constructors, builders, or places that instantiate
Notification to supply a userId; also add a DB migration to alter the user_id
column to NOT NULL (and backfill existing rows if needed) and update any
service/validation logic that currently assumes userId can be null.
In
`@src/main/kotlin/depromeet/hotsix/obrit/notification/service/FcmTokenService.kt`:
- Around line 13-20: The current sequence in FcmTokenService that calls
fcmTokenRepository.findByToken(token) then fcmTokenRepository.save(...) is not
atomic and can cause unique-constraint races on concurrent token registrations;
make this operation transactionally safe by wrapping the logic in a
`@Transactional` method and handling concurrent insert conflicts: attempt the
save, catch the DataIntegrityViolationException (or DuplicateKeyException)
thrown on unique constraint violation, then load the existing FcmToken via
fcmTokenRepository.findByToken(token) and call existing.reassignOwner(userId)
(or update owner and return it); alternatively implement a repository-level
upsert/merge or use a SELECT ... FOR UPDATE variant if supported. Ensure you
reference and modify the code around FcmTokenService, the save/findByToken calls
and the reassignOwner flow so concurrent requests resolve to the existing token
instead of failing.
In
`@src/main/kotlin/depromeet/hotsix/obrit/notification/service/NotificationService.kt`:
- Around line 19-24: The current logic first fetches Notification by
notificationId then checks ownership, which can leak existence of other users'
notifications; change the retrieval in NotificationService to call a repository
method that filters by both id and userId (e.g., add
NotificationRepository.findByIdAndUserId(id: Long, userId: Long):
Optional<Notification>) and use its result.orElseThrow {
ResourceNotFoundException("존재하지 않는 알림입니다.") } so missing/unauthorized cases are
handled identically; remove the subsequent isOwnedBy(userId) check in the
service since the repository-level query enforces ownership.
In
`@src/test/kotlin/depromeet/hotsix/obrit/notification/FcmTokenAcceptanceTest.kt`:
- Around line 50-54: The test is missing assertions that each FCM registration
call returned HTTP 200, so add status checks after every invocation of the test
helper `FCM 토큰을 등록한다` (the calls shown at the two identical lines and the other
occurrences referenced) — update each call site to assert the response status
(e.g., chain a `andExpect(status().isOk())` or equivalent on the MockMvc result)
so every successful registration path verifies HTTP OK and fails the test on
regressions.
---
Nitpick comments:
In
`@src/main/kotlin/depromeet/hotsix/obrit/notification/controller/NotificationController.kt`:
- Around line 19-22: The controller currently returns ApiResponse.ok(null) after
calling notificationService.markAsRead(userId, notificationId); change both the
controller and service signature to return a MarkReadNotificationResponse DTO:
update NotificationController endpoint to return
ApiResponse<MarkReadNotificationResponse> and have
notificationService.markAsRead(...) return a MarkReadNotificationResponse (or
map the service result to that DTO) so the controller returns
ApiResponse.ok(markReadResponse) instead of null; ensure you do not return any
JPA entities and map domain objects to the MarkReadNotificationResponse DTO
within the service or a mapper.
In
`@src/main/kotlin/depromeet/hotsix/obrit/notification/service/NotificationService.kt`:
- Around line 31-33: The current mark-as-read implementation loads all
notifications via notificationRepository.findAllByUserId(...).filter {
!it.isRead }.forEach { it.markAsRead() } which causes unnecessary memory and
query overhead for large datasets; replace this with a repository-level bulk
update (e.g., add a `@Modifying` `@Query` method like markAllAsReadByUserId(userId,
readAt)) and call that from NotificationService.markAsReadAll so unread
notifications are updated in a single SQL update rather than iterating and
updating each entity in memory.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2bec8a8-686a-477e-b80f-6f8524af458f
📒 Files selected for processing (16)
build.gradlesrc/main/kotlin/depromeet/hotsix/obrit/global/config/FirebaseConfig.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/controller/FcmTokenController.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/controller/NotificationController.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/controller/docs/FcmTokenControllerApi.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/dto/request/RegisterFcmTokenRequest.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/dto/response/MarkReadNotificationResponse.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/dto/response/RegisterFcmTokenResponse.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/entity/FcmToken.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/entity/Notification.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/repository/FcmTokenRepository.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/repository/NotificationRepository.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/service/FcmTokenService.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/service/NotificationService.ktsrc/test/kotlin/depromeet/hotsix/obrit/notification/FcmTokenAcceptanceTest.ktsrc/test/kotlin/depromeet/hotsix/obrit/notification/FcmTokenFixture.kt
| runtimeOnly 'com.h2database:h2' | ||
| runtimeOnly 'com.mysql:mysql-connector-j' | ||
| testImplementation 'org.springframework.boot:spring-boot-starter-test' | ||
| testImplementation 'org.springframework.boot:spring-boot-webmvc-test' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] spring-boot-webmvc-test 존재 여부 확인"
curl -s 'https://search.maven.org/solrsearch/select?q=g:%22org.springframework.boot%22+AND+a:%22spring-boot-webmvc-test%22&rows=20&wt=json' \
| jq '.response.numFound, .response.docs'
echo "[2] 비교용: spring-boot-starter-test 최신 버전 확인"
curl -s 'https://search.maven.org/solrsearch/select?q=g:%22org.springframework.boot%22+AND+a:%22spring-boot-starter-test%22&rows=1&wt=json' \
| jq '.response.docs[0] | {id, latestVersion}'Repository: depromeet/18th-team6-server
Length of output: 248
spring-boot-webmvc-test 의존성 좌표가 존재하지 않습니다. 수정이 필요합니다.
Maven Central Repository에서 org.springframework.boot:spring-boot-webmvc-test는 존재하지 않는 아티팩트입니다. 이 의존성으로는 빌드가 즉시 실패합니다. spring-boot-starter-test를 사용하십시오. 이미 MockMvc 테스트 구성이 포함되어 있습니다.
🤖 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` at line 35, Replace the invalid test dependency declaration
testImplementation 'org.springframework.boot:spring-boot-webmvc-test' with the
correct Spring Boot testing starter; update the dependency to use
org.springframework.boot:spring-boot-starter-test so MockMvc and other test
utilities are available (i.e., change the artifact coordinate referenced in the
testImplementation entry).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ziweek
requested changes
May 20, 2026
ziweek
left a comment
ziweek
left a comment
Collaborator
There was a problem hiding this comment.
고생많으셨습니다!!
토끼 리뷰만 챙겨주세용! 👍
| @@ -0,0 +1,3 @@ | |||
| package depromeet.hotsix.obrit.global.config | |||
|
|
|||
| class FirebaseConfig | |||
Collaborator
There was a problem hiding this comment.
q;
firebase 설정이 아직 없어서 요기 아직 비어 있는 것 맞나용?
eclipse021
approved these changes
May 21, 2026
eclipse021
left a comment
eclipse021
left a comment
Collaborator
There was a problem hiding this comment.
덕분에 코드 리뷰 하면서 FCM 부분 같이 공부한 것 같아~
코드 작성하느라 고생했음 !!
Collaborator
There was a problem hiding this comment.
요건 안 쓰는거 같은데 만든 의도가 어떻게 돼??
Collaborator
Author
There was a problem hiding this comment.
알림 읽기처리 api에서 쓰이는 파일인데 커밋이 같이 들어갔나봐
| fcmTokenRepository.save(FcmToken(userId = userId, token = token)) | ||
| } else { | ||
| existing.reassignOwner(userId) | ||
| existing |
Collaborator
There was a problem hiding this comment.
여기서 existing 한번 더 호출하는데 어떤 의도인지 궁금해!
Collaborator
Author
There was a problem hiding this comment.
fcmToken 변수에 할당하려고 선언했어~!
| var id: Long? = null, | ||
|
|
||
| @Column(name = "user_id") | ||
| var userId: Long? = null, |
Collaborator
There was a problem hiding this comment.
알림에 user_id가 비어있으면 누구에게 간 알림인지 알 수 없어서 문제 생길 거 같은데 not null 로 바꾸는 건 어떨까 🙂
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ations 알림 목록 조회 API
…ations 알림 읽음 처리 API
Firebase를 notification 도메인에서만 사용하므로 global/config에서 notification/config/firebase로 이동 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
정리 - sendToToken을 private으로 변경 (내부 전용 메서드) - sendToToken의 @async 제거 (sendToUser에서 이미 비동기 처리) - sendToUser의 @transactional 제거 (외부 API 호출과 트랜잭션 분리) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
notification/config/firebase 패키지 구조 지원을 위해 레이어 패키지 허용 목록에 config 추가 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- FirebaseConfig 빈 클래스 제거 - Notification.userId non-nullable 변경 및 NOT NULL 제약 추가 - Notification.markAsRead() 멱등성 보장 (이미 읽은 경우 early return) - FcmTokenService.registerToken() race condition 처리 (saveAndFlush + DataIntegrityViolationException catch) - FcmTokenService.findOrCreateToken() 분리로 가독성 개선 - NotificationRepository.findByIdAndUserId() 추가로 ownership 체크 단일 쿼리 처리 - NotificationRepository.markAllAsReadByUserId() bulk update 쿼리 추가 - NotificationService.markAsRead() MarkReadNotificationResponse 반환 - NotificationService.markAsReadAll() bulk update로 교체 - NotificationController.markAsRead() 응답 타입 Nothing? → MarkReadNotificationResponse - FcmTokenAcceptanceTest Given/When 호출에 HTTP 200 assertion 추가 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/kotlin/depromeet/hotsix/obrit/notification/controller/docs/NotificationControllerApi.kt (1)
38-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwagger 헤더 파라미터
name누락으로 런타임 계약과 불일치
NotificationController.kt는@RequestHeader("X-User-Id")로 헤더 키를 고정 요구NotificationControllerApi.kt의 Swagger@Parameter(description=..., in=ParameterIn.HEADER)는name이 지정되지 않아 문서가userId로 노출될 수 있어 클라이언트 연동 실패 위험이 있음 (listNotification,markAsRead,markAsReadAll모두)수정 예시
fun listNotification( - `@Parameter`(description = "사용자 ID", required = true, example = "1", `in` = ParameterIn.HEADER) + `@Parameter`(name = "X-User-Id", description = "사용자 ID", required = true, example = "1", `in` = ParameterIn.HEADER) userId: Long, ): ApiResponse<List<ListNotificationResponse>> fun markAsRead( - `@Parameter`(description = "사용자 ID", required = true, example = "1", `in` = ParameterIn.HEADER) + `@Parameter`(name = "X-User-Id", description = "사용자 ID", required = true, example = "1", `in` = ParameterIn.HEADER) userId: Long, `@Parameter`(description = "알림 ID", required = true, example = "1") notificationId: Long, ): ApiResponse<MarkReadNotificationResponse> fun markAsReadAll( - `@Parameter`(description = "사용자 ID", required = true, example = "1", `in` = ParameterIn.HEADER) + `@Parameter`(name = "X-User-Id", description = "사용자 ID", required = true, example = "1", `in` = ParameterIn.HEADER) userId: Long, ): ApiResponse<Nothing?>🤖 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/kotlin/depromeet/hotsix/obrit/notification/controller/docs/NotificationControllerApi.kt` around lines 38 - 39, The Swagger header parameters in NotificationControllerApi.kt are missing the header key name, causing docs to expose `userId` instead of the actual header expected by the controller; update the `@Parameter` annotations for the header parameter used by listNotification, markAsRead, and markAsReadAll to include name = "X-User-Id" and keep required=true and in=ParameterIn.HEADER so the generated contract matches the controller's `@RequestHeader`("X-User-Id") expectation.
🤖 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/kotlin/depromeet/hotsix/obrit/notification/dto/response/MarkReadNotificationResponse.kt`:
- Line 5: MarkReadNotificationResponse DTO is missing per-field Schema
descriptions; add `@field`:Schema(description = "...") annotations (in Korean) to
each constructor property: id (Long) — e.g. "알림 ID", isRead (Boolean) — e.g. "읽음
여부", readAt (LocalDateTime) — e.g. "읽음 시각"; import the Swagger Schema annotation
(io.swagger.v3.oas.annotations.media.Schema) if not present and annotate each
field in the data class signature so it complies with the DTO documentation
rule.
In
`@src/main/kotlin/depromeet/hotsix/obrit/notification/service/FcmTokenService.kt`:
- Around line 31-33: In FcmTokenService's catch(DataIntegrityViolationException
e) block avoid force-unwrapping the optional from
fcmTokenRepository.findByToken(token)!!; instead, perform a safe null-check on
the result of findByToken(token) and call reassignOwner(userId) only if the
token entity is present, otherwise rethrow the original exception e so the
original cause isn't lost; ensure you reference the same repository call
(fcmTokenRepository.findByToken(token)) and the entity method (reassignOwner) in
your fix.
---
Outside diff comments:
In
`@src/main/kotlin/depromeet/hotsix/obrit/notification/controller/docs/NotificationControllerApi.kt`:
- Around line 38-39: The Swagger header parameters in
NotificationControllerApi.kt are missing the header key name, causing docs to
expose `userId` instead of the actual header expected by the controller; update
the `@Parameter` annotations for the header parameter used by listNotification,
markAsRead, and markAsReadAll to include name = "X-User-Id" and keep
required=true and in=ParameterIn.HEADER so the generated contract matches the
controller's `@RequestHeader`("X-User-Id") expectation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1aefc3c0-a7f4-459e-9a9b-0e90f0839ffe
📒 Files selected for processing (8)
src/main/kotlin/depromeet/hotsix/obrit/notification/controller/NotificationController.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/controller/docs/NotificationControllerApi.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/dto/response/MarkReadNotificationResponse.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/entity/Notification.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/repository/NotificationRepository.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/service/FcmTokenService.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/service/NotificationService.ktsrc/test/kotlin/depromeet/hotsix/obrit/notification/FcmTokenAcceptanceTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/kotlin/depromeet/hotsix/obrit/notification/FcmTokenAcceptanceTest.kt
- src/main/kotlin/depromeet/hotsix/obrit/notification/entity/Notification.kt
- FcmTokenService: catch 블록 !! 제거, null 시 원본 예외 rethrow - FcmTokenService: findOrCreateToken ?.also 패턴 적용 - FcmTokenService: createToken 메서드 분리로 race condition 처리 격리 - MarkReadNotificationResponse: @field:Schema 한국어 설명 추가 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- spring-boot-starter-security 의존성 추가 - AdminSecurityConfig 추가: /fcm-tokens/**, /notifications/** 화이트리스트 등록 - application-test.yml: obrit.admin 테스트용 설정 추가 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ture/50-set-fcm-notification
eclipse021
approved these changes
Jun 4, 2026
eclipse021
left a comment
eclipse021
left a comment
Collaborator
There was a problem hiding this comment.
충성! 이병 임준현입니다.
팀장님 PR에 리뷰 달았는데 확인 부탁드립니다! 충성!
| @Service | ||
| class FcmTokenService(private val fcmTokenRepository: FcmTokenRepository) { | ||
| @Transactional | ||
| @Transactional(noRollbackFor = [DataIntegrityViolationException::class]) |
Collaborator
There was a problem hiding this comment.
내부 함수에서 try-catch 문으로 DataIntegrityCiolationException 예외 처리해서 해당 부분 없어도 될 거 같아~
Comment on lines
28
to
+39
| @Transactional | ||
| fun markAsRead(userId: Long, notificationId: Long) { | ||
| val notification = notificationRepository.findById(notificationId) | ||
| .orElseThrow { ResourceNotFoundException("존재하지 않는 알림입니다.") } | ||
|
|
||
| if (!notification.isOwnedBy(userId)) { | ||
| throw ForbiddenException("알림을 읽을 권한이 없습니다.") | ||
| } | ||
| fun markAsRead(userId: Long, notificationId: Long): MarkReadNotificationResponse { | ||
| val notification = notificationRepository.findByIdAndUserId(notificationId, userId) | ||
| ?: throw ResourceNotFoundException("존재하지 않는 알림입니다.") | ||
|
|
||
| notification.markAsRead() | ||
|
|
||
| return MarkReadNotificationResponse( | ||
| id = notification.id!!, | ||
| isRead = notification.isRead, | ||
| readAt = notification.readAt!!, | ||
| ) |
Collaborator
There was a problem hiding this comment.
알림 읽을 권한에 대한 유효성 검사 유지해도 될거 같아!
Collaborator
Author
There was a problem hiding this comment.
이거 기존에는 그냥 알림 id로만 조회해와서 검증이 있었는데, 아예 레포 메서드를 findByIdAndUserId 유저 id로 가지고 와서 뺀 거긴 한데 헷갈리는 부분이 있으려나 ?!
Collaborator
There was a problem hiding this comment.
이거 기존에는 그냥 알림 id로만 조회해와서 검증이 있었는데, 아예 레포 메서드를 findByIdAndUserId 유저 id로 가지고 와서 뺀 거긴 한데 헷갈리는 부분이 있으려나 ?!
아냐아냐 내가 잘못봤다 지금이 나은 거 같아 !
…fication FCM push 호출 구현
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/kotlin/depromeet/hotsix/obrit/architecture/LayerDependencyTest.kt (1)
28-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win오류 메시지가 실제 검증 규칙과 일치하지 않습니다.
Line 18에서 허용 레이어 패턴에
config를 추가했지만, Line 30의 오류 메시지에는 여전히 "controller/service/dto/entity/repository packages"만 나열되어 있습니다. 오류 메시지에도config를 포함시켜야 개발자가 혼란을 겪지 않습니다.제안하는 수정
assertTrue( violations.isEmpty(), - "Production domain classes must live under controller/service/dto/entity/repository packages: $violations", + "Production domain classes must live under controller/service/dto/entity/repository/config packages: $violations", )🤖 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/kotlin/depromeet/hotsix/obrit/architecture/LayerDependencyTest.kt` around lines 28 - 31, The assertion message for the LayerDependencyTest's assertTrue(violations.isEmpty, "...") is out of sync with the allowed layer pattern (you added "config" to allowed layers); update the error string used in the assertTrue call to include "config" alongside controller/service/dto/entity/repository so the message matches the actual rule and helps developers identify the allowed packages when violations are reported.
🤖 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/kotlin/depromeet/hotsix/obrit/notification/config/firebase/FirebaseConfig.kt`:
- Around line 19-21: The FileInputStream created when building FirebaseOptions
in FirebaseConfig is not closed; wrap the stream in Kotlin's use to auto-close
it and pass the opened stream to GoogleCredentials.fromStream, e.g. open the
FileInputStream for firebaseProperties.credentialsPath with use and build the
FirebaseOptions using the credentials obtained inside that block so the stream
is always closed; update the code around FirebaseOptions.builder() and the
options variable in FirebaseConfig to use this pattern.
In
`@src/main/kotlin/depromeet/hotsix/obrit/notification/service/FcmPushService.kt`:
- Around line 48-51: The logs in FcmPushService expose the full device token
(variable token) in both the info and error messages; replace raw token logging
with a deterministic masked or hashed representation (e.g., SHA-256 or mask all
but last 4 chars) before calling log.info/log.error so the original token value
is not written to logs, but still use the original token when calling
fcmTokenRepository.findByToken(token) and when deleting; update references
around the log statements where token and e are used (log.info, log.error) to
log the masked/hashedToken instead of token.
---
Outside diff comments:
In `@src/test/kotlin/depromeet/hotsix/obrit/architecture/LayerDependencyTest.kt`:
- Around line 28-31: The assertion message for the LayerDependencyTest's
assertTrue(violations.isEmpty, "...") is out of sync with the allowed layer
pattern (you added "config" to allowed layers); update the error string used in
the assertTrue call to include "config" alongside
controller/service/dto/entity/repository so the message matches the actual rule
and helps developers identify the allowed packages when violations are reported.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bc722ef-ae6c-429d-b317-400c1ef2a6e2
📒 Files selected for processing (13)
build.gradlesrc/main/kotlin/depromeet/hotsix/obrit/global/config/AdminSecurityConfig.ktsrc/main/kotlin/depromeet/hotsix/obrit/global/config/AsyncConfig.ktsrc/main/kotlin/depromeet/hotsix/obrit/global/exception/GlobalExceptionHandler.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/config/firebase/FirebaseConfig.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/config/firebase/FirebaseProperties.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/dto/response/MarkReadNotificationResponse.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/service/FcmPushService.ktsrc/main/kotlin/depromeet/hotsix/obrit/notification/service/FcmTokenService.ktsrc/main/resources/application-prod.ymlsrc/test/kotlin/depromeet/hotsix/obrit/architecture/LayerDependencyTest.ktsrc/test/kotlin/depromeet/hotsix/obrit/notification/FcmPushServiceAsyncTest.ktsrc/test/resources/application-test.yml
✅ Files skipped from review due to trivial changes (1)
- src/test/resources/application-test.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/kotlin/depromeet/hotsix/obrit/notification/dto/response/MarkReadNotificationResponse.kt
- src/main/kotlin/depromeet/hotsix/obrit/notification/service/FcmTokenService.kt
- build.gradle
Comment on lines
+19
to
+21
| val options = FirebaseOptions.builder() | ||
| .setCredentials(GoogleCredentials.fromStream(FileInputStream(firebaseProperties.credentialsPath))) | ||
| .build() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# FileInputStream 사용 위치 점검 (use 블록 적용 여부 확인용)
rg -n --type=kt 'FileInputStream\(' src/main/kotlinRepository: depromeet/18th-team6-server
Length of output: 98
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) 해당 파일의 19~30줄을 직접 확인
FILE="src/main/kotlin/depromeet/hotsix/obrit/notification/config/firebase/FirebaseConfig.kt"
nl -ba "$FILE" | sed -n '1,120p' | sed -n '15,40p'
# 2) src/main/kotlin 전체에서 FileInputStream 사용 위치 확인 (+ kt 파일만)
rg -n --glob='*.kt' 'FileInputStream\(' src/main/kotlinRepository: depromeet/18th-team6-server
Length of output: 114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/kotlin/depromeet/hotsix/obrit/notification/config/firebase/FirebaseConfig.kt"
# 1) 해당 파일 19~30줄 전후 확인 (nl 대신 cat -n 사용)
cat -n "$FILE" | sed -n '15,40p'
# 2) src/main/kotlin에서 FileInputStream 사용 위치 전체 재검색
rg -n --glob='*.kt' 'FileInputStream\(' src/main/kotlinRepository: depromeet/18th-team6-server
Length of output: 773
자격증명 파일 스트림을 use로 닫아 리소스 누수를 방지하세요.
FirebaseConfig.kt의 20행에서 FileInputStream(firebaseProperties.credentialsPath)가 use로 처리되지 않아 스트림이 닫히지 않을 수 있습니다. 아래처럼 감싸는 방식이 안전합니다.
🔧 제안 수정안
- val options = FirebaseOptions.builder()
- .setCredentials(GoogleCredentials.fromStream(FileInputStream(firebaseProperties.credentialsPath)))
- .build()
+ val credentials = FileInputStream(firebaseProperties.credentialsPath).use { input ->
+ GoogleCredentials.fromStream(input)
+ }
+ val options = FirebaseOptions.builder()
+ .setCredentials(credentials)
+ .build()📝 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.
Suggested change
| val options = FirebaseOptions.builder() | |
| .setCredentials(GoogleCredentials.fromStream(FileInputStream(firebaseProperties.credentialsPath))) | |
| .build() | |
| val credentials = FileInputStream(firebaseProperties.credentialsPath).use { input -> | |
| GoogleCredentials.fromStream(input) | |
| } | |
| val options = FirebaseOptions.builder() | |
| .setCredentials(credentials) | |
| .build() |
🤖 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/kotlin/depromeet/hotsix/obrit/notification/config/firebase/FirebaseConfig.kt`
around lines 19 - 21, The FileInputStream created when building FirebaseOptions
in FirebaseConfig is not closed; wrap the stream in Kotlin's use to auto-close
it and pass the opened stream to GoogleCredentials.fromStream, e.g. open the
FileInputStream for firebaseProperties.credentialsPath with use and build the
FirebaseOptions using the credentials obtained inside that block so the stream
is always closed; update the code around FirebaseOptions.builder() and the
options variable in FirebaseConfig to use this pattern.
Comment on lines
+48
to
+51
| log.info("만료된 FCM 토큰 삭제. token={}", token) | ||
| fcmTokenRepository.findByToken(token)?.let { fcmTokenRepository.delete(it) } | ||
| } else { | ||
| log.error("FCM 전송 실패. token={}, error={}", token, e.messagingErrorCode, e) |
Contributor
There was a problem hiding this comment.
FCM 토큰 원문 로그 노출을 제거하세요.
Line 48, Line 51에서 디바이스 토큰 전체값을 로그에 남기고 있어 민감 식별자가 로그에 축적됩니다. 토큰은 마스킹/해시 후 로깅하세요.
🔧 제안 수정안
- log.info("만료된 FCM 토큰 삭제. token={}", token)
+ log.info("만료된 FCM 토큰 삭제. tokenPrefix={}", token.take(8))
fcmTokenRepository.findByToken(token)?.let { fcmTokenRepository.delete(it) }
} else {
- log.error("FCM 전송 실패. token={}, error={}", token, e.messagingErrorCode, e)
+ log.error("FCM 전송 실패. tokenPrefix={}, error={}", token.take(8), e.messagingErrorCode, e)
}📝 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.
Suggested change
| log.info("만료된 FCM 토큰 삭제. token={}", token) | |
| fcmTokenRepository.findByToken(token)?.let { fcmTokenRepository.delete(it) } | |
| } else { | |
| log.error("FCM 전송 실패. token={}, error={}", token, e.messagingErrorCode, e) | |
| log.info("만료된 FCM 토큰 삭제. tokenPrefix={}", token.take(8)) | |
| fcmTokenRepository.findByToken(token)?.let { fcmTokenRepository.delete(it) } | |
| } else { | |
| log.error("FCM 전송 실패. tokenPrefix={}, error={}", token.take(8), e.messagingErrorCode, e) | |
| } |
🤖 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/kotlin/depromeet/hotsix/obrit/notification/service/FcmPushService.kt`
around lines 48 - 51, The logs in FcmPushService expose the full device token
(variable token) in both the info and error messages; replace raw token logging
with a deterministic masked or hashed representation (e.g., SHA-256 or mask all
but last 4 chars) before calling log.info/log.error so the original token value
is not written to logs, but still use the original token when calling
fcmTokenRepository.findByToken(token) and when deleting; update references
around the log statements where token and e are used (log.info, log.error) to
log the masked/hashedToken instead of token.
…gister_fcm_token # Conflicts: # src/main/kotlin/depromeet/hotsix/obrit/global/exception/GlobalExceptionHandler.kt # src/test/kotlin/depromeet/hotsix/obrit/architecture/LayerDependencyTest.kt
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
3 participants
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.
Closes #37
Summary
FCM 토큰 등록 API
Changes
POST /fcm-tokens) 구현Etc
동일한 토큰 값을 다른 사용자가 재등록 시도할 경우 신규 사용자의 토큰으로 덮어지도록 처리했습니다.
Summary by CodeRabbit
새로운 기능
테스트
버그/예외 처리
설정