feat: 알림 API 구현#71
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 14 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough알림 API 기능을 완성하기 위한 타입 계약부터 REST 엔드포인트, 데이터베이스 마이그레이션, 통합 테스트까지 전체 스택을 추가했습니다. 단순 푸시 발송에 의존하지 않고 서버 기반 알림 보관함으로 누락된 알림 동기화를 지원합니다. Changes알림 API 구현
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/gachi/be/domain/notification/api/controller/NotificationController.java`:
- Around line 56-60: The controller exposes the size query param without a
Swagger default; update the NotificationController's size parameter to declare
the OpenAPI default so docs match runtime: add defaultValue="20" to the
`@RequestParam` on the Integer size parameter (keep the `@Min`(1)/@Max(100) and the
existing description and required=false as-is) — this aligns the controller spec
with NotificationService#normalizePageSize and the DEFAULT_PAGE_SIZE(20)
behavior.
In
`@src/main/java/com/gachi/be/domain/notification/dto/request/PushTokenRegisterRequest.java`:
- Line 11: The request indicates ambiguity about deviceId being optional and its
overwrite behavior: in PushTokenRegisterRequest (deviceId) either make it
required by adding `@NotBlank` to the field to force callers to always supply a
deviceId, or change the refresh(...) logic that sets PushDeviceToken.deviceId
(which currently uses normalizeOptional(request.deviceId())) to only overwrite
when the normalized value is non-null/non-empty so existing deviceId is
preserved; update the code paths that create/update tokens (registerPushToken,
deletePushToken, and the refresh method) to follow the chosen policy and ensure
normalizeOptional(request.deviceId()) is only used to assign when you intend to
replace the stored deviceId.
In `@src/main/java/com/gachi/be/domain/notification/entity/PushDeviceToken.java`:
- Around line 82-90: The refresh method updates token but not tokenHash, leaving
a stale hash; update refresh(PushPlatform platform, String token, String
deviceId, String appVersion) to also set tokenHash (either by recalculating from
the new token inside PushDeviceToken.refresh or by adding a tokenHash parameter
and assigning it) so that the field tokenHash is kept in sync whenever token is
updated; modify any callers if you choose the parameter approach and ensure
tokenHash is consistently used for token-based lookups.
In
`@src/main/java/com/gachi/be/domain/notification/service/NotificationService.java`:
- Around line 104-106: The code currently normalizes the push token but does not
validate emptiness, causing blank tokens to be hashed and stored/deleted; update
registerPushToken (and the corresponding unregister/delete method that also
calls normalizeRequired/sha256Hex) to trim and then reject empty tokens by
throwing a validation exception (e.g., IllegalArgumentException or your
service's ValidationException) with a clear message before computing
sha256Hex(token) or performing persistence/soft-delete so blank tokens cannot be
recorded or removed accidentally.
In
`@src/test/java/com/gachi/be/domain/notification/api/controller/NotificationControllerIntegrationTest.java`:
- Around line 40-60: Add integration tests to
NotificationControllerIntegrationTest to cover the suggested edge cases: create
tests (e.g., shouldReturnEmptyListWhenNoNotifications,
shouldHandleInvalidCursor, shouldIgnoreMarkReadForAlreadyReadNotification,
shouldValidateSizeBoundaries, shouldReturnEmptyWhenUnreadOnlyAndNone) that use
mockMvc (built in setUp), objectMapper, and jwtTokenProvider to call the
notification endpoints; seed/cleanup data via userRepository,
notificationRepository, and pushDeviceTokenRepository and assert proper HTTP
status and response bodies for empty lists, invalid cursor values, re-marking
read notifications, size values (0, 101, negative), and unreadOnly=true
returning empty results.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4fcd890a-bb9d-4082-880f-7693a60a78b3
📒 Files selected for processing (25)
src/main/java/com/gachi/be/domain/notification/api/controller/NotificationController.javasrc/main/java/com/gachi/be/domain/notification/dto/request/NotificationReadRequest.javasrc/main/java/com/gachi/be/domain/notification/dto/request/PushTokenDeleteRequest.javasrc/main/java/com/gachi/be/domain/notification/dto/request/PushTokenRegisterRequest.javasrc/main/java/com/gachi/be/domain/notification/dto/response/NotificationListResponse.javasrc/main/java/com/gachi/be/domain/notification/dto/response/NotificationReadResponse.javasrc/main/java/com/gachi/be/domain/notification/dto/response/NotificationResponse.javasrc/main/java/com/gachi/be/domain/notification/dto/response/NotificationUnreadCountResponse.javasrc/main/java/com/gachi/be/domain/notification/dto/response/PushTokenResponse.javasrc/main/java/com/gachi/be/domain/notification/entity/Notification.javasrc/main/java/com/gachi/be/domain/notification/entity/NotificationDeliveryLog.javasrc/main/java/com/gachi/be/domain/notification/entity/PushDeviceToken.javasrc/main/java/com/gachi/be/domain/notification/entity/enums/NotificationDeliveryStatus.javasrc/main/java/com/gachi/be/domain/notification/entity/enums/NotificationType.javasrc/main/java/com/gachi/be/domain/notification/entity/enums/PushPlatform.javasrc/main/java/com/gachi/be/domain/notification/repository/NotificationDeliveryLogRepository.javasrc/main/java/com/gachi/be/domain/notification/repository/NotificationRepository.javasrc/main/java/com/gachi/be/domain/notification/repository/PushDeviceTokenRepository.javasrc/main/java/com/gachi/be/domain/notification/service/NotificationCreateCommand.javasrc/main/java/com/gachi/be/domain/notification/service/NotificationDeliveryResultCommand.javasrc/main/java/com/gachi/be/domain/notification/service/NotificationService.javasrc/main/java/com/gachi/be/global/code/ErrorCode.javasrc/main/java/com/gachi/be/global/code/SuccessCode.javasrc/main/resources/db/migration/V14__notification_api.sqlsrc/test/java/com/gachi/be/domain/notification/api/controller/NotificationControllerIntegrationTest.java
Comment on lines
+40
to
+60
| @SpringBootTest | ||
| @ActiveProfiles("test") | ||
| @Transactional | ||
| class NotificationControllerIntegrationTest { | ||
| private static final AtomicInteger PHONE_SEQUENCE = new AtomicInteger(7000); | ||
|
|
||
| private final ObjectMapper objectMapper = new ObjectMapper(); | ||
| private MockMvc mockMvc; | ||
|
|
||
| @Autowired private WebApplicationContext webApplicationContext; | ||
| @Autowired private UserRepository userRepository; | ||
| @Autowired private JwtTokenProvider jwtTokenProvider; | ||
| @Autowired private NotificationService notificationService; | ||
| @Autowired private NotificationRepository notificationRepository; | ||
| @Autowired private PushDeviceTokenRepository pushDeviceTokenRepository; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| mockMvc = | ||
| MockMvcBuilders.webAppContextSetup(webApplicationContext).apply(springSecurity()).build(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
추가 엣지 케이스 테스트를 고려해보세요.
현재 테스트는 주요 시나리오를 잘 커버하고 있습니다. 다음과 같은 엣지 케이스를 추가하면 더욱 견고해질 수 있습니다:
- 빈 알림 목록 조회
- 잘못된 cursor 값 처리
- 이미 읽은 알림을 다시 읽기
- size 경계값 (0, 101, 음수)
- unreadOnly=true일 때 빈 결과
현재 구현도 충분히 좋지만, 프로덕션 안정성을 높이려면 이러한 케이스들을 추가로 검증하는 것이 도움이 될 수 있습니다.
🤖 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/gachi/be/domain/notification/api/controller/NotificationControllerIntegrationTest.java`
around lines 40 - 60, Add integration tests to
NotificationControllerIntegrationTest to cover the suggested edge cases: create
tests (e.g., shouldReturnEmptyListWhenNoNotifications,
shouldHandleInvalidCursor, shouldIgnoreMarkReadForAlreadyReadNotification,
shouldValidateSizeBoundaries, shouldReturnEmptyWhenUnreadOnlyAndNone) that use
mockMvc (built in setUp), objectMapper, and jwtTokenProvider to call the
notification endpoints; seed/cleanup data via userRepository,
notificationRepository, and pushDeviceTokenRepository and assert proper HTTP
status and response bodies for empty lists, invalid cursor values, re-marking
read notifications, size values (0, 101, negative), and unreadOnly=true
returning empty results.
Hminkyung
approved these changes
May 26, 2026
Collaborator
Hminkyung
left a comment
Hminkyung
left a comment
There was a problem hiding this comment.
확인했씁니다!!!!! 고생하셨씁니다아아아아ㅏ
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/gachi/be/domain/notification/service/NotificationService.java (1)
110-131:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift토큰 refresh 시 기존 레코드가 갱신되지 않습니다.
현재 등록 경로는
userId + tokenHash로만 기존 토큰을 찾습니다. 푸시 토큰이 새 값으로 갱신되면 해시도 바뀌므로 기존 행을 절대 찾지 못하고 새 레코드를 추가하게 됩니다. 그 결과 이전 토큰이enabled=true로 남아 이후 발송에서 중복 알림이나 불필요한 실패 로그를 만들 수 있습니다.deviceId같은 안정적인 식별자로 기존 행을 먼저 찾아refresh(...)하거나, 동일 기기의 이전 활성 토큰을 비활성화하는 경로가 필요합니다.🤖 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/gachi/be/domain/notification/service/NotificationService.java` around lines 110 - 131, The current lookup in NotificationService that uses pushDeviceTokenRepository.findByUserIdAndTokenHash(...) fails to match when the token (and its hash) changes; update the logic to first try finding an existing PushDeviceToken by stable identifiers (e.g., userId + deviceId using request.deviceId() via preserveExistingIfBlank/normalizeOptional) and call existing.refresh(...) if found, otherwise fall back to the tokenHash lookup and then to creation; additionally, ensure you deactivate or set enabled=false on any other active tokens for the same userId+deviceId (use pushDeviceTokenRepository to query and update other rows) so only the current token remains active, while still using PushDeviceToken.builder() when creating a truly new record.
🤖 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 @.github/workflows/docker-be.yml:
- Line 43: 현재 워크플로에서 사용중인 actions/setup-java@v4 태그는 공급망 리스크가 있으니 태그 대신 해당 태그의 커밋
SHA c1e323688fd81a25caa38c78aa6df2d33d3e20d9으로 교체하세요; 구체적으로
`.github/workflows/docker-be.yml`의 uses: actions/setup-java@v4 항목을 SHA 핀으로 바꾸고,
동일한 파일의 uses: actions/checkout@v6 및 uses: gradle/actions/setup-gradle@v5 항목도 가능한
경우 각 태그의 대응 커밋 SHA로 핀하도록 업데이트하세요.
---
Outside diff comments:
In
`@src/main/java/com/gachi/be/domain/notification/service/NotificationService.java`:
- Around line 110-131: The current lookup in NotificationService that uses
pushDeviceTokenRepository.findByUserIdAndTokenHash(...) fails to match when the
token (and its hash) changes; update the logic to first try finding an existing
PushDeviceToken by stable identifiers (e.g., userId + deviceId using
request.deviceId() via preserveExistingIfBlank/normalizeOptional) and call
existing.refresh(...) if found, otherwise fall back to the tokenHash lookup and
then to creation; additionally, ensure you deactivate or set enabled=false on
any other active tokens for the same userId+deviceId (use
pushDeviceTokenRepository to query and update other rows) so only the current
token remains active, while still using PushDeviceToken.builder() when creating
a truly new record.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec77a4d0-a4a0-4432-8655-5628b1dad6ab
📒 Files selected for processing (6)
.github/workflows/docker-be.ymldeploy/docker-compose.ymlsrc/main/java/com/gachi/be/domain/notification/api/controller/NotificationController.javasrc/main/java/com/gachi/be/domain/notification/entity/PushDeviceToken.javasrc/main/java/com/gachi/be/domain/notification/service/NotificationService.javasrc/test/java/com/gachi/be/domain/notification/api/controller/NotificationControllerIntegrationTest.java
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
2 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.
📌 작업 요약
dedupeKey와 향후 FCM/Expo 발송 결과 추적을 위한 delivery log 구조를 추가NOTI4041(HTTP 404 Not Found): notificationId에 해당하는 알림이 없거나 소유권이 일치하지 않음🌿 브랜치 정보
feat/#70-notification-apidevelop(기본)✅ 체크리스트
feat/refac/hotfix/chore/design/bugfix)feat/fix/refactor/docs/style/chore)🧪 테스트 결과
Summary by CodeRabbit
New Features
Tests