Fix/task schedule 분리로 인한 이벤트 처리 갱신#18
Hidden character warning
Conversation
There was a problem hiding this comment.
Pull request overview
POJO 도메인에서 Dirty Checking이 적용되지 않아 “다가오는 일정 알림(UpcomingScheduleNotification)”이 업데이트되지 않던 문제를, JPA bulk update 기반의 명시적 업데이트 경로로 전환해 해결하려는 PR입니다. 또한 Task/Schedule 분리 이후의 이벤트 문서를 보강합니다.
Changes:
UpcomingScheduleNotification업데이트를 엔티티 Dirty Checking 대신 JPQLUPDATE로 수행하도록 리포지토리 포트/어댑터 확장- 서비스 로직이 기존 도메인 객체 mutate 대신 리포지토리 update 메서드를 호출하도록 변경
- JPA/어댑터 단 테스트 추가 및 AsyncAPI 문서에 task 이벤트 채널/메시지 추가
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/me/pinitnotification/infrastructure/persistence/notification/UpcomingScheduleNotificationRepositoryAdapter.java | bulk update 호출 어댑터 메서드 추가(+ 로깅) |
| src/main/java/me/pinitnotification/infrastructure/persistence/notification/UpcomingScheduleNotificationJpaRepository.java | UUID PK로 정정 및 JPQL update 쿼리 메서드 추가 |
| src/main/java/me/pinitnotification/domain/notification/UpcomingScheduleNotificationRepository.java | 포트에 update 메서드 추가 |
| src/main/java/me/pinitnotification/application/notification/ScheduleNotificationService.java | 기존 POJO 업데이트 대신 리포지토리 update 호출로 전환 |
| src/test/java/me/pinitnotification/infrastructure/persistence/notification/UpcomingScheduleNotificationRepositoryAdapterTest.java | 어댑터 update 동작 테스트 추가 |
| src/test/java/me/pinitnotification/infrastructure/persistence/notification/UpcomingScheduleNotificationJpaRepositoryTest.java | JPQL update 쿼리 동작 테스트 추가 |
| src/test/java/me/pinitnotification/application/notification/ScheduleNotificationServiceTest.java | 서비스 업데이트 검증을 상태변경 assert → repository 호출 verify로 전환 |
| docs/task-async-api.yaml | Task 이벤트 채널/메시지 추가 및 일부 필드 format 변경 |
| public void updateScheduleStartTimeAndIdempotentKey(Long scheduleId, Long ownerId, String scheduleStartTime, String idempotentKey) { | ||
| int updatedRows = jpaRepository.updateScheduleStartTimeAndIdempotentKey(scheduleId, ownerId, scheduleStartTime, idempotentKey); | ||
| if (updatedRows == 0) { | ||
| log.debug("Skip updating notification. scheduleId={}, ownerId={} not found", scheduleId, ownerId); |
There was a problem hiding this comment.
(1) 문제점: update 쿼리가 0건 갱신되면(=대상이 없으면) debug 로그만 남기고 그대로 종료해서, 호출 측에서는 성공/실패를 구분할 수 없고 실제로는 갱신이 누락될 수 있습니다.
(2) 영향: 이벤트 처리 순서 뒤바뀜/동시 삭제 등으로 대상 레코드가 사라진 경우, 알림 정보가 최신 상태로 맞춰지지 않은 채 조용히 지나가 운영 중 추적이 어려워집니다.
(3) 수정 제안: updatedRows==0을 ‘정상 스킵’으로 처리할 게 아니라 (a) warn 로그로 격상하거나 (b) 예외를 던져 상위에서 재시도/보상 처리하게 하거나 (c) 필요 시 새로 save 하는 방식(업서트) 중 하나로 명확히 처리해 주세요.
| log.debug("Skip updating notification. scheduleId={}, ownerId={} not found", scheduleId, ownerId); | |
| log.warn("Skip updating notification because target not found. scheduleId={}, ownerId={}", scheduleId, ownerId); |
| existing -> notificationRepository.updateScheduleStartTimeAndIdempotentKey( | ||
| command.scheduleId(), | ||
| command.ownerId(), | ||
| resolveScheduleStartTime(command, existing), | ||
| command.idempotentKey() | ||
| ), |
There was a problem hiding this comment.
(1) 문제점: find 후 update를 수행하는 TOCTOU 흐름이라, find 시점엔 존재했어도 update 시점에 0건 갱신될 수 있는데(동시 삭제/정리 작업 등), 현재는 그 경우를 복구하지 않습니다.
(2) 영향: ‘업데이트 이벤트’를 처리했음에도 DB에 반영되지 않아 알림 발송/중복제거(idempotentKey) 로직이 틀어질 수 있습니다.
(3) 수정 제안: update 메서드가 갱신 건수를 반환하도록 포트/어댑터를 맞춘 뒤(updatedRows), 0이면 save로 전환하거나(업서트) 최소한 예외 처리로 실패를 명시해 주세요. 가능하면 find 없이 update→0이면 insert로 DB round-trip도 줄일 수 있습니다.
| existing -> notificationRepository.updateScheduleStartTimeAndIdempotentKey( | |
| command.scheduleId(), | |
| command.ownerId(), | |
| resolveScheduleStartTime(command, existing), | |
| command.idempotentKey() | |
| ), | |
| existing -> { | |
| int updatedRows = notificationRepository.updateScheduleStartTimeAndIdempotentKey( | |
| command.scheduleId(), | |
| command.ownerId(), | |
| resolveScheduleStartTime(command, existing), | |
| command.idempotentKey() | |
| ); | |
| if (updatedRows == 0) { | |
| // 동시 삭제 등으로 업데이트 대상이 사라진 경우, 신규로 생성하여 업서트 보장 | |
| notificationRepository.save( | |
| buildNotification( | |
| command.ownerId(), | |
| command.scheduleId(), | |
| command.idempotentKey(), | |
| toStringValue(command.newUpcomingTime()) | |
| ) | |
| ); | |
| } | |
| }, |
| newUpcomingTime: | ||
| type: string | ||
| format: date-time | ||
| format: designatedStartTime-time | ||
| occurredAt: | ||
| type: string | ||
| format: date-time | ||
| format: designatedStartTime-time | ||
| idempotentKey: |
There was a problem hiding this comment.
(1) 문제점: AsyncAPI 스키마의 string format을 표준 date-time에서 designatedStartTime-time로 변경했는데, 이 값은 OpenAPI/AsyncAPI tooling에서 일반적으로 인식되는 포맷이 아니고 문서 내에서도 별도 정의가 없습니다.
(2) 영향: 문서를 기반으로 한 검증/코드 생성/스키마 호환성 체크가 실패하거나, 소비자가 RFC3339(date-time)로 해석해야 하는 필드를 잘못 이해할 수 있습니다.
(3) 수정 제안: 실제 값이 RFC3339라면 format: date-time을 유지하고, 의미 차이는 description 또는 x-... 커스텀 필드로 보강해 주세요(커스텀 format을 쓰려면 문서 상단에 정의/설명을 추가).
변경된 점