✨ Feat: 알림 및 푸시 토큰 기능 구현 - #176
Conversation
- 관리자 전용 로그인 API 추가 - 관리자 알림 스케줄 등록 API 추가 - @scheduled 기반 예약 알림 실행 로직 구현 - 예약 알림 DB 저장 및 FCM 발송 처리 추가 - 일부 API 성공 응답을 200과 message 응답으로 정리
# Conflicts: # src/main/java/com/dodo/backend/admin/controller/AdminController.java # src/main/java/com/dodo/backend/admin/dto/response/AdminResponse.java # src/main/java/com/dodo/backend/admin/service/AdminService.java # src/main/java/com/dodo/backend/admin/service/AdminServiceImpl.java # src/test/java/com/dodo/backend/admin/controller/AdminControllerTest.java # src/test/java/com/dodo/backend/admin/service/AdminServiceTest.java
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive notification and FCM push token management features, including token registration, notification retrieval, and scheduled notification execution. It also adds an admin-only login endpoint with rate limiting and updates report endpoint status codes. Key feedback highlights critical architectural concerns: a transaction boundary issue in NotificationScheduleServiceImpl where external FCM API calls inside a database transaction risk connection pool exhaustion and duplicate notifications, and the lack of duplicate execution prevention in clustered environments. Additionally, the reviewer suggests using Querydsl or Specification for dynamic notification filtering, handling case-insensitivity when parsing notification type enums, and resolving page indexing inconsistencies (0-based vs 1-based) across controllers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @Transactional | ||
| public void executeDueSchedules() { |
There was a problem hiding this comment.
🚨 스케줄러 실행 시 트랜잭션 경계 및 외부 API 호출(FCM) 관련 심각한 개선 필요 사항
현재 executeDueSchedules 메서드는 @Transactional 어노테이션이 적용되어 있으며, 내부에서 루프를 돌며 각 스케줄을 처리하고 외부 FCM 서버로 동기 네트워크 요청을 보내고 있습니다. 이 구조는 운영 환경에서 다음과 같은 심각한 장애를 유발할 수 있습니다.
-
외부 API 호출(FCM)로 인한 커넥션 풀 고사 (Connection Pool Exhaustion)
- 데이터베이스 트랜잭션이 열려 있는 상태에서 외부 네트워크 I/O(
fcmNotificationSender.sendToUsers)를 수행하고 있습니다. - 만약 Firebase 서버의 응답이 지연되거나 타임아웃이 발생하면, 해당 시간 동안 데이터베이스 커넥션이 반환되지 않고 계속 점유됩니다. 이는 순식간에 커넥션 풀 고사 및 전체 시스템 장애로 이어질 수 있습니다.
- 데이터베이스 트랜잭션이 열려 있는 상태에서 외부 네트워크 I/O(
-
롤백 불가능한 부수 효과(Side Effect)와 중복 발송 위험
- FCM 푸시 발송은 외부 시스템의 동작이므로 데이터베이스 트랜잭션이 롤백되어도 취소되지 않습니다.
- 만약 50개의 스케줄 중 49번째 스케줄 처리 중 예외가 발생하여 트랜잭션이 롤백되면, 이미 푸시가 발송된 앞선 48개의 알림 데이터는 DB에 저장되지 않고 여전히
PENDING상태로 남게 됩니다. - 이로 인해 다음 스케줄러 실행 시 동일한 알림이 중복 발송되는 치명적인 버그가 발생합니다.
-
단일 트랜잭션 묶음으로 인한 전체 실패 (All-or-Nothing)
- 하나의 스케줄 실행 실패(예: 특정 사용자 데이터 오류 등)가 전체 50개 스케줄의 성공적인 처리를 가로막고 함께 롤백시킵니다.
💡 권장하는 개선 방향
각 스케줄의 실행 단위를 독립적인 트랜잭션으로 분리하고, 외부 API 호출은 데이터베이스 트랜잭션 외부에서 수행하도록 구조를 변경해야 합니다. executeDueSchedules에서 @Transactional을 제거하고, 개별 스케줄을 처리하는 별도의 컴포넌트나 메서드에 Propagation.REQUIRES_NEW 트랜잭션을 적용하는 방식을 고려해 주세요.
| return NotificationScheduleCreateResponse.toDto(savedSchedule); | ||
| } | ||
|
|
||
| @Scheduled(fixedDelayString = "${notification.scheduler.fixed-delay:60000}") |
There was a problem hiding this comment.
🌐 다중 서버(Clustered) 환경에서의 스케줄러 중복 실행 방지 대책 필요
현재 @Scheduled 설정은 다중 WAS 인스턴스(예: Kubernetes 환경에서 여러 개의 Pod가 뜨는 경우)가 기동될 때, 모든 서버에서 동시에 스케줄러가 실행되는 문제를 가지고 있습니다.
이 경우 동일한 시간에 여러 서버가 동일한 PENDING 상태의 스케줄을 조회하게 되어, 동일한 알림이 사용자들에게 중복해서 발송되는 장애가 발생할 수 있습니다.
💡 권장하는 해결 방안
다중 서버 환경에서도 스케줄러가 단 한 번만 실행되도록 보장하기 위해 다음 중 하나의 방법을 도입하는 것을 강력히 권장합니다.
- ShedLock 라이브러리 도입 (가장 간편하고 권장됨)
- Redis나 Database를 이용해 분산 락을 획득한 서버만 스케줄러를 실행할 수 있도록 제어합니다.
- 데이터베이스 비관적 락(Pessimistic Lock) 또는 상태 업데이트 선점
- 데이터를 조회할 때
SELECT ... FOR UPDATE를 사용하거나,PENDING상태의 레코드를 먼저PROCESSING상태로UPDATE쿼리를 날려 성공한 개수만큼만 가져와 처리하는 방식을 사용할 수 있습니다.
- 데이터를 조회할 때
| private Page<Notification> findNotifications(UUID userId, Boolean isRead, List<NotificationType> types, Pageable pageable) { | ||
| if (isRead != null && !types.isEmpty()) { | ||
| return notificationRepository.findByUserUsersIdAndIsReadAndNotificationTypeInOrderByNotificationCreatedAtDescNotificationIdDesc( | ||
| userId, | ||
| isRead, | ||
| types, | ||
| pageable | ||
| ); | ||
| } | ||
| if (isRead != null) { | ||
| return notificationRepository.findByUserUsersIdAndIsReadOrderByNotificationCreatedAtDescNotificationIdDesc(userId, isRead, pageable); | ||
| } | ||
| if (!types.isEmpty()) { | ||
| return notificationRepository.findByUserUsersIdAndNotificationTypeInOrderByNotificationCreatedAtDescNotificationIdDesc(userId, types, pageable); | ||
| } | ||
| return notificationRepository.findByUserUsersIdOrderByNotificationCreatedAtDescNotificationIdDesc(userId, pageable); | ||
| } |
There was a problem hiding this comment.
🛠️ 동적 쿼리 처리를 위한 Querydsl 또는 Specification 도입 고려
현재 findNotifications 메서드는 isRead와 types 필터의 유무에 따라 4가지 경우의 수로 나누어 각각 다른 Spring Data JPA 메서드를 호출하고 있습니다.
이 방식은 필터 조건이 늘어날수록 메서드 개수가 기하급수적으로 증가하며, Repository 인터페이스의 메서드명이 극도로 길어져 가독성과 유지보수성이 떨어집니다.
💡 개선 방안
Spring Data JPA의 Specification이나 Querydsl을 도입하면 단 하나의 메서드로 동적 쿼리를 깔끔하게 처리할 수 있습니다. 이를 통해 Repository에 불필요하게 긴 메서드들을 선언하지 않고도 동적 필터링을 유연하게 확장할 수 있습니다.
| List<NotificationType> types = Arrays.stream(type.split(",")) | ||
| .map(String::trim) | ||
| .filter(value -> !value.isBlank()) | ||
| .map(NotificationType::valueOf) | ||
| .distinct() | ||
| .toList(); |
There was a problem hiding this comment.
🛡️ Enum 변환 시 대소문자 구분 완화 (Defensive Programming)
현재 parseTypes 메서드에서는 클라이언트가 전달한 문자열을 NotificationType.valueOf(value)를 통해 Enum으로 변환하고 있습니다.
Java Enum의 valueOf 메서드는 대소문자를 엄격하게 구분하기 때문에, 클라이언트가 실수로 소문자(comment)나 혼용 표기(Comment)로 요청을 보낼 경우 IllegalArgumentException이 발생하여 요청이 실패하게 됩니다.
💡 개선 제안
클라이언트 요청의 유연성을 높이고 불필요한 에러를 방지하기 위해, Enum 변환 전에 toUpperCase()를 적용하는 것을 권장합니다.
| List<NotificationType> types = Arrays.stream(type.split(",")) | |
| .map(String::trim) | |
| .filter(value -> !value.isBlank()) | |
| .map(NotificationType::valueOf) | |
| .distinct() | |
| .toList(); | |
| List<NotificationType> types = Arrays.stream(type.split(",")) | |
| .map(String::trim) | |
| .filter(value -> !value.isBlank()) | |
| .map(value -> NotificationType.valueOf(value.toUpperCase())) | |
| .distinct() | |
| .toList(); |
| public ResponseEntity<AnnouncementListResponse> getAnnouncementList( | ||
| @ParameterObject @PageableDefault(size = 10) Pageable pageable | ||
| @RequestParam(defaultValue = "0") int page, | ||
| @RequestParam(defaultValue = "10") int size, | ||
| @RequestParam(defaultValue = "registrationUpdatedAt,desc") String sort | ||
| ) { |
There was a problem hiding this comment.
⚠️ API 간 페이지 번호(Page) 인덱스 일관성 결여 (0-based vs 1-based)
현재 AdminController의 공지 목록 조회 API(getAnnouncementList)는 페이지 번호의 기본값으로 0을 사용하며 0-based index로 동작합니다.
반면, NotificationController의 알림 목록 조회 API(getNotifications)는 기본값으로 1을 사용하며 내부적으로 page - 1 처리를 통해 1-based index로 동작하고 있습니다.
💡 개선 제안
동일한 프로젝트 내에서 API마다 페이지 번호 기준이 다르면 프론트엔드 개발 시 혼선이 발생하고 버그의 원인이 되기 쉽습니다. 프로젝트 전체의 페이지 번호 정책을 하나로 통일(예: 모두 1-based 또는 모두 0-based)하는 것을 강력히 권장합니다.
- 예약 알림 처리 트랜잭션을 스케줄 단위로 분리 - PROCESSING 선점 토큰으로 다중 서버 중복 실행 방지 - FCM 발송을 DB 트랜잭션 외부로 이동 - 알림 목록 조회를 Specification 기반 동적 쿼리로 정리 - 알림 유형 필터 대소문자 완화 및 페이지 기준 0-based 통일
2 participants
📄 작업 내용 (Description)
200 + message형태로 통일🔗 관련 이슈 (Related Issues)
✅ 체크리스트 (Checklist)
Style)Test)📸 스크린샷 (Screenshots)
💬 기타 사항 (Etc)