[SRLT-159] 알림 Redis payload 축소#98
Open
SeongHo5356 wants to merge 5 commits into
Hidden character warning
The head ref may contain hidden characters: "SRLT-159-\uc54c\ub9bc-payload-\ucd95\uc18c"
Open
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Message DTO Separation and Realtime Schema src/main/java/starlight/application/notification/required/dto/NotificationPublishMessage.java, src/main/java/starlight/application/notification/required/dto/NotificationRealtimeMessage.java |
NotificationPublishMessage를 memberId/notificationId/type으로 축소하여 routing-only 정보를 담도록 변경. NotificationRealtimeMessage 신규 추가: 구독자에게 전달할 전체 알림 상세정보(title/message/referenceId/createdAt with JSON format) 포함. 각각 from(Notification) 팩토리 메서드 제공. |
Port Interface Contracts for Realtime Flow src/main/java/starlight/application/notification/required/NotificationQueryPort.java, src/main/java/starlight/application/notification/required/NotificationRealtimePort.java, src/main/java/starlight/application/notification/provided/NotificationUseCase.java |
NotificationQueryPort: 누락 알림 조회용 findAllByMemberIdAndIdGreaterThanOrderByIdAsc(memberId, notificationId, limit) 메서드 추가. NotificationRealtimePort: subscribe(memberId, List missedMessages), send(NotificationRealtimeMessage), hasSubscriber(memberId) 계약 변경/추가. NotificationUseCase 공개 인터페이스 동일하게 업데이트. |
Notification Repository Queries for Missed Recovery src/main/java/starlight/adapter/notification/persistence/NotificationJpa.java, src/main/java/starlight/adapter/notification/persistence/NotificationRepository.java |
NotificationJpa: findAllByMemberIdAndIdGreaterThanOrderByIdAsc(memberId, notificationId, limit) 메서드 추가, PageRequest.of(0, limit)로 결과 개수 제한. NotificationRepository: @Query로 id > 조건과 오름차순 정렬 구현. |
NotificationService Missed Recovery and Realtime Send Logic src/main/java/starlight/application/notification/NotificationService.java |
subscribe(memberId, Long lastEventId) 확장: lastEventId가 유효하면 repository에서 누락 알림 조회, NotificationRealtimeMessage로 변환 후 realtime port에 전달. sendRealtime(NotificationPublishMessage) 메서드 신규: 구독자 존재 여부 확인, 미존재 시 즉시 반환(최적화), 존재 시 알림 엔티티 조회 후 변환하여 realtime port.send로 전송. sseRecoveryLimit 설정값 필드 추가(@Value default 100). |
WebAPI Last-Event-ID Header and Query Parameter Handling src/main/java/starlight/adapter/notification/webapi/NotificationController.java, src/main/java/starlight/adapter/notification/webapi/swagger/NotificationApiDoc.java |
NotificationController.subscribe: Last-Event-ID 요청 헤더, lastEventId 쿼리 파라미터 입력 추가(모두 선택). 헤더 우선, notificationUseCase.subscribe(memberId, resolvedLastEventId) 호출. NotificationApiDoc: Swagger 문서화 확장, 두 파라미터에 @Parameter 설명 추가. |
SSE Registry Missed Message Replay and Realtime Send src/main/java/starlight/adapter/notification/sse/NotificationSseRegistry.java |
subscribe(memberId, List missedMessages): 새 구독자에게 "connected" SSE 이벤트 후 누락 메시지 재전송. hasSubscriber(memberId) 메서드 추가: 활성 구독 여부 조회. send(NotificationRealtimeMessage): NotificationPublishMessage 대신 새 타입 수용, createNotificationEvent 헬퍼 활용. sendMissedMessages 프라이빗 헬퍼: memberId 필터링하여 누락 메시지만 리플레이. |
Redis Subscriber Use Case Dispatch for Realtime Send src/main/java/starlight/adapter/notification/redis/RedisNotificationSubscriber.java |
의존성: NotificationRealtimePort → NotificationUseCase로 변경. handlePayload: 파싱된 NotificationPublishMessage를 notificationUseCase.sendRealtime(...) 호출로 전달(기존 직접 port.send 대신). |
Tests for Last-Event-ID Resolution and Missed Recovery Flow src/test/java/starlight/adapter/notification/redis/RedisNotificationSubscriberTest.java, src/test/java/starlight/adapter/notification/webapi/NotificationControllerTest.java, src/test/java/starlight/application/notification/NotificationOutboxPublishServiceUnitTest.java, src/test/java/starlight/application/notification/NotificationServiceUnitTest.java |
RedisNotificationSubscriberTest: NotificationUseCase mock 사용, sendRealtime 호출 검증; ObjectMapper JavaTimeModule 제거. NotificationControllerTest 신규: Last-Event-ID 헤더 우선, 쿼리 파라미터 fallback, 미인증 예외 검증. NotificationOutboxPublishServiceUnitTest: ArgumentCaptor로 NotificationPublishMessage 필드(memberId/notificationId/type) 단언. NotificationServiceUnitTest: sseRecoveryLimit=100 주입; subscribe 다중 시나리오(lastEventId null/valid/zero) 검증; sendRealtime 구독자 없음/있음 케이스 분리 검증. |
Sequence Diagram(s)
sequenceDiagram
participant Browser
participant Controller
participant Service
participant QueryPort
participant RealtimePort
participant SseRegistry
Browser->>Controller: subscribe(Last-Event-ID or lastEventId)
Controller->>Service: subscribe(memberId, lastEventId)
alt lastEventId valid
Service->>QueryPort: findAllByMemberIdAndIdGreaterThanOrderByIdAsc
QueryPort-->>Service: List<Notification>
Service->>Service: convert to NotificationRealtimeMessage
end
Service->>RealtimePort: subscribe(memberId, missedMessages)
RealtimePort->>SseRegistry: subscribe(memberId, missedMessages)
SseRegistry->>SseRegistry: sendMissedMessages(memberId)
SseRegistry->>Browser: SSE missedMessages
SseRegistry->>Browser: SSE "connected" event
RealtimePort-->>Service: SseEmitter
Service-->>Controller: SseEmitter
Controller-->>Browser: SseEmitter
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- StartUpLight/STARLIGHT_BE#92: 이 PR에서 도입한 알림/SSE/Redis 실시간 파이프라인 기초 위에서 missed message recovery, 메시지 타입 분리, subscribe/send 계약 확장이 이루어지므로 직접적 선행 관련 작업.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | PR 제목 'SRLT-159 알림 Redis payload 축소'는 변경사항의 핵심을 명확하게 요약합니다. NotificationPublishMessage를 축소하여 Redis payload를 경량화하는 것이 주요 변경이며, 제목이 이를 정확하게 반영하고 있습니다. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
SRLT-159-알림-payload-축소
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 help to get the list of available commands and usage tips.
Test Results273 tests 273 ✅ 10s ⏱️ Results for commit b17b3f2. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/starlight/adapter/notification/redis/RedisNotificationSubscriber.java`:
- Around line 29-33: The current single catch swallows both JSON parsing and
business-processing failures in RedisNotificationSubscriber; change to first
catch JsonProcessingException (from objectMapper.readValue) and log the bad
payload and parsing error only, then separately catch exceptions thrown by
notificationUseCase.sendRealtime and route those to a retry/failure path (e.g.,
rethrow to let the message broker requeue, publish the message to a retry Redis
channel, and/or increment a failure metric) so processing failures are not lost;
reference objectMapper.readValue, NotificationPublishMessage, and
notificationUseCase.sendRealtime when making these changes.
In
`@src/main/java/starlight/adapter/notification/sse/NotificationSseRegistry.java`:
- Around line 106-110: sendMissedMessages currently iterates missedMessages
directly which causes an NPE if null; update the method (sendMissedMessages) to
defensively normalize missedMessages to an empty list at the start (or return
early) before iterating, so SseEmitter handling (and any calls that may pass
null) won't fail; reference the sendMissedMessages method and the missedMessages
parameter and ensure the loop over NotificationRealtimeMessage elements only
runs on the normalized (non-null) collection.
In `@src/main/java/starlight/application/notification/NotificationService.java`:
- Around line 35-36: The sseRecoveryLimit field in NotificationService can be
set to 0 or negative via config which will cause PageRequest.of(0, limit) to
throw; add a startup validation method (e.g., a `@PostConstruct`
validateSseRecoveryLimit()) in NotificationService that checks the injected
sseRecoveryLimit and if it's less than 1 either set it to 1 (clamp) or throw an
IllegalStateException with a clear message; also log a warning when clamping so
operators see the misconfiguration. Ensure the check references the
sseRecoveryLimit field and protects code paths that call
NotificationJpa/PageRequest.of(...).
🪄 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: CHILL
Plan: Pro
Run ID: d83028a3-7cfc-438d-838d-c6f4c6bdf8fd
📒 Files selected for processing (16)
src/main/java/starlight/adapter/notification/persistence/NotificationJpa.javasrc/main/java/starlight/adapter/notification/persistence/NotificationRepository.javasrc/main/java/starlight/adapter/notification/redis/RedisNotificationSubscriber.javasrc/main/java/starlight/adapter/notification/sse/NotificationSseRegistry.javasrc/main/java/starlight/adapter/notification/webapi/NotificationController.javasrc/main/java/starlight/adapter/notification/webapi/swagger/NotificationApiDoc.javasrc/main/java/starlight/application/notification/NotificationService.javasrc/main/java/starlight/application/notification/provided/NotificationUseCase.javasrc/main/java/starlight/application/notification/required/NotificationQueryPort.javasrc/main/java/starlight/application/notification/required/NotificationRealtimePort.javasrc/main/java/starlight/application/notification/required/dto/NotificationPublishMessage.javasrc/main/java/starlight/application/notification/required/dto/NotificationRealtimeMessage.javasrc/test/java/starlight/adapter/notification/redis/RedisNotificationSubscriberTest.javasrc/test/java/starlight/adapter/notification/webapi/NotificationControllerTest.javasrc/test/java/starlight/application/notification/NotificationOutboxPublishServiceUnitTest.javasrc/test/java/starlight/application/notification/NotificationServiceUnitTest.java
Comment on lines
29
to
33
| try { | ||
| NotificationPublishMessage publishMessage = objectMapper.readValue(payload, NotificationPublishMessage.class); | ||
| notificationRealtimePort.send(publishMessage); | ||
| notificationUseCase.sendRealtime(publishMessage); | ||
| } catch (Exception exception) { | ||
| log.warn("[NOTIFICATION] failed to consume redis payload={}", payload, exception); |
There was a problem hiding this comment.
파싱 오류와 처리 오류를 같은 catch에서 삼키면 실시간 알림 유실이 고정됩니다.
이번 변경으로 sendRealtime(...)가 DB 조회까지 포함하는데, 현재는 해당 실패도 모두 로그만 남기고 버려집니다. 최소한 JSON 파싱 실패와 비즈니스 처리 실패를 분리하고, 처리 실패는 재처리 가능한 경로(재시도/재큐잉/실패 메트릭)로 연결해야 합니다.
🤖 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/starlight/adapter/notification/redis/RedisNotificationSubscriber.java`
around lines 29 - 33, The current single catch swallows both JSON parsing and
business-processing failures in RedisNotificationSubscriber; change to first
catch JsonProcessingException (from objectMapper.readValue) and log the bad
payload and parsing error only, then separately catch exceptions thrown by
notificationUseCase.sendRealtime and route those to a retry/failure path (e.g.,
rethrow to let the message broker requeue, publish the message to a retry Redis
channel, and/or increment a failure metric) so processing failures are not lost;
reference objectMapper.readValue, NotificationPublishMessage, and
notificationUseCase.sendRealtime when making these changes.
Comment on lines
+106
to
+110
| private void sendMissedMessages( | ||
| SseEmitter emitter, | ||
| Long memberId, | ||
| List<NotificationRealtimeMessage> missedMessages | ||
| ) throws IOException { |
There was a problem hiding this comment.
missedMessages null 방어가 없어 구독이 즉시 실패할 수 있습니다.
sendMissedMessages에서 missedMessages를 바로 순회해 null 입력 시 NPE로 연결이 끊깁니다. 포트 경계에서 빈 리스트로 정규화해 안전하게 처리하는 편이 좋습니다.
🔧 제안 코드
public SseEmitter subscribe(Long memberId, List<NotificationRealtimeMessage> missedMessages) {
+ List<NotificationRealtimeMessage> safeMissedMessages =
+ missedMessages == null ? List.of() : missedMessages;
+
SseEmitter emitter = new SseEmitter(sseTimeoutMs);
addEmitter(memberId, emitter);
@@
- sendMissedMessages(emitter, memberId, missedMessages);
+ sendMissedMessages(emitter, memberId, safeMissedMessages);🤖 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/starlight/adapter/notification/sse/NotificationSseRegistry.java`
around lines 106 - 110, sendMissedMessages currently iterates missedMessages
directly which causes an NPE if null; update the method (sendMissedMessages) to
defensively normalize missedMessages to an empty list at the start (or return
early) before iterating, so SseEmitter handling (and any calls that may pass
null) won't fail; reference the sendMissedMessages method and the missedMessages
parameter and ensure the loop over NotificationRealtimeMessage elements only
runs on the normalized (non-null) collection.
Comment on lines
+35
to
+36
| @Value("${notification.sse.recovery-limit:100}") | ||
| private int sseRecoveryLimit; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# recovery-limit 검증 유무와 사용 지점을 빠르게 확인
rg -n "notification\\.sse\\.recovery-limit|sseRecoveryLimit|PageRequest\\.of\\(0,\\s*limit\\)|`@PostConstruct`|`@Min`\\(|`@Positive`\\("Repository: StartUpLight/STARLIGHT_BE
Length of output: 2153
🏁 Script executed:
cat -n src/main/java/starlight/application/notification/NotificationService.java | head -110Repository: StartUpLight/STARLIGHT_BE
Length of output: 5222
🏁 Script executed:
rg -A 10 "findMissedMessages" src/main/java/starlight/application/notification/NotificationService.javaRepository: StartUpLight/STARLIGHT_BE
Length of output: 956
🏁 Script executed:
cat -n src/main/java/starlight/adapter/notification/persistence/NotificationJpa.java | head -80Repository: StartUpLight/STARLIGHT_BE
Length of output: 2121
sseRecoveryLimit 최소값 검증이 없어 SSE 재연결 경로가 런타임에 실패할 수 있습니다.
설정된 notification.sse.recovery-limit 값이 0 이하면 NotificationJpa의 PageRequest.of(0, limit) 호출 시 예외가 발생합니다. 기본값(100)은 안전하지만, 설정으로 오버라이드될 수 있으므로 애플리케이션 시작 시점에 1 이상을 강제하는 게 안전합니다.
🔧 제안 수정
+import jakarta.annotation.PostConstruct;
...
`@Service`
`@RequiredArgsConstructor`
public class NotificationService implements NotificationUseCase {
...
`@Value`("${notification.sse.recovery-limit:100}")
private int sseRecoveryLimit;
+
+ `@PostConstruct`
+ void validateSseRecoveryLimit() {
+ if (sseRecoveryLimit < 1) {
+ throw new IllegalStateException("notification.sse.recovery-limit must be >= 1");
+ }
+ }🤖 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/starlight/application/notification/NotificationService.java`
around lines 35 - 36, The sseRecoveryLimit field in NotificationService can be
set to 0 or negative via config which will cause PageRequest.of(0, limit) to
throw; add a startup validation method (e.g., a `@PostConstruct`
validateSseRecoveryLimit()) in NotificationService that checks the injected
sseRecoveryLimit and if it's less than 1 either set it to 1 (clamp) or throw an
IllegalStateException with a clear message; also log a warning when clamping so
operators see the misconfiguration. Ensure the check references the
sseRecoveryLimit field and protects code paths that call
NotificationJpa/PageRequest.of(...).
2ghrms
approved these changes
May 27, 2026
Member
2ghrms
left a comment
2ghrms
left a comment
There was a problem hiding this comment.
고생하셨습니다!! 메시지에 페이로드를 줄이는 선택 너무 좋은 것 같습니다
| and notification.id > :notificationId | ||
| order by notification.id asc | ||
| """) | ||
| List<Notification> findAllByMemberIdAndIdGreaterThanOrderByIdAsc( |
Member
There was a problem hiding this comment.
지금 @query를 작성해서 JPQL을 사용하고 있는데, 이러면 메소드 명을 Spring Data JPA의 네이밍 규칙을 엄격하게 유지할 필요 없이 findAllAfterCursor와 같이 간결하게 변경하시는건 어떨까요?
아니면, 현재 로직이 그렇게 복잡한 로직이 아니니, @query를 제거하고, 정렬 조건(Sort)을 호출부의 Pageable 파라미터에 녹여서 아래와 같이 변경하는건 어떨까요?
List<Notification> findByMemberIdAndIdGreaterThan(
Long memberId,
Long id,
Pageable pageable
);
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.
🚀 Why - 해결하려는 문제가 무엇인가요?
lastEventId이후 알림을 제한 없이 조회할 수 있어, 장시간 미접속 후 재연결 시 과도한 복구 전송이 발생할 수 있었습니다.✅ What - 무엇이 변경됐나요?
NotificationPublishMessagepayload를memberId,notificationId,type으로 축소했습니다.NotificationRealtimeMessage를 추가해 Redis 메시지와 클라이언트 전송 메시지의 역할을 분리했습니다.notification.sse.recovery-limit설정을 추가하고 기본값을 100개로 제한했습니다.Last-Event-ID헤더 우선순위, payload 축소, 구독자 유무에 따른 DB 조회 여부를 검증하는 테스트를 추가했습니다.🛠️ How - 어떻게 해결했나요?
memberId,notificationId,type만 publish합니다.NotificationUseCase.sendRealtime(...)로 전달합니다.sendRealtime(...)에서는 먼저 현재 서버에 해당memberId의 SSE emitter가 있는지 확인하고, 없으면 DB 조회 없이 종료합니다.notificationId로 DB에서 알림을 조회한 뒤NotificationRealtimeMessage로 변환해 SSE로 전송합니다.memberId + lastEventId이후 알림을 id 오름차순으로 조회하되, 설정값 기준 최대 개수까지만 전송하도록 제한했습니다.Summary by CodeRabbit
릴리스 노트
새로운 기능
테스트