feat/#71: 학생 알림 조회 기능 구현 #72
Conversation
jjaeroong
had a problem deploying
to
production
GitHub Actions
Failure
Walkthrough알림 시스템의 새로운 도메인을 추가합니다. Notification 엔티티, 저장소, 서비스, 컨트롤러, DTOs, 예외 처리, 유틸리티 클래스가 포함됩니다. CouncilPostPushListener가 업데이트되어 알림을 저장하고, UserRepository에 사용자 조회 메서드가 추가됩니다. Changes
Sequence DiagramsequenceDiagram
participant Event as CouncilPostCreatedEvent
participant Listener as CouncilPostPushListener
participant Service as NotificationService
participant UserRepo as UserRepository
participant Mapper as NotificationMapper
participant NotifRepo as NotificationRepository
Event->>Listener: handleCouncilPostCreatedEvent()
Listener->>Service: savePostCreatedNotification(event, title, body)
Service->>Service: findUsersByTopic(topic)
Service->>UserRepo: 사용자 조회 by scope<br/>(major/college/school)
UserRepo-->>Service: List<User>
loop 각 사용자별
Service->>Mapper: createNotification(user, type, ...)
Mapper-->>Service: Notification 객체
end
Service->>NotifRepo: saveAll(notifications)
NotifRepo-->>Service: 저장 완료
Listener->>Listener: Firebase 메시지 발송
sequenceDiagram
participant Client as 클라이언트
participant Controller as NotificationController
participant Service as NotificationService
participant NotifRepo as NotificationRepository
participant Mapper as NotificationMapper
participant TimeFormatter as TimeFormatter
Client->>Controller: GET /notifications?limit=20&cursorCreatedAt=...&cursorId=...
Controller->>Service: getNotificationsByCursor(userId, cursor...)
alt 첫 페이지
Service->>NotifRepo: findByUserOrderByCreatedAtDescIdDesc()
else 다음 페이지
Service->>NotifRepo: findNextByCursor(user, cursorCreatedAt, cursorId)
end
NotifRepo-->>Service: List<Notification>
loop 각 알림
Service->>Mapper: toResponse(notification)
Mapper->>TimeFormatter: formatRelativeTime(createdAt)
TimeFormatter-->>Mapper: "N분 전" (상대시간)
Mapper-->>Service: NotificationResponse
end
Service->>Service: NextCursor 생성, hasNext 계산
Service-->>Controller: CursorResponse<NotificationResponse>
Controller-->>Client: CommonResponse with data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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: 6
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/campus/campus/domain/councilpost/application/CouncilPostPushListener.java (1)
41-52: 예외 발생 시 에러 처리 및 로깅 메커니즘 추가 필요
saveCouncilPostCreated와sendToTopic호출에 대한 에러 처리가 없어, 예외 발생 시 로그가 기록되지 않고 실패 여부를 추적할 수 없습니다. 특히@Async메서드에서는 예외가 호출자에게 전파되지 않으므로 문제 진단이 어렵습니다.다음 중 하나를 고려해 주세요:
- try-catch로 각 작업을 감싸고 실패 시 에러 로깅 또는 메트릭 기록
- 또는
saveCouncilPostCreated실패 시에도 푸시가 발송되어야 한다면 두 작업 순서를 분리하고 독립적으로 처리
🤖 Fix all issues with AI agents
In
`@src/main/java/com/campus/campus/domain/council/domain/entity/CouncilType.java`:
- Around line 60-63: The compilation fails because CouncilType is missing the
abstract method declarations that the enum constants override; add abstract
method signatures for scopeId(...) and scope(...) to the CouncilType class
(alongside the existing public abstract boolean hasAccess(User user,
StudentCouncil writer) and public abstract String topic(StudentCouncil writer))
using the exact method names and parameter/return types that the enum constant
overrides use so the `@Override` annotations compile.
In
`@src/main/java/com/campus/campus/domain/notification/application/dto/NotificationListResponse.java`:
- Around line 5-9: Remove the unused DTO class NotificationListResponse: delete
the record NotificationListResponse declaration and any imports or references to
it; ensure NotificationController.getNotifications() and
NotificationService.getNotificationsByCursor() continue using
CursorResponse<NotificationResponse> and run a build to fix any leftover imports
or compile errors after deletion.
In
`@src/main/java/com/campus/campus/domain/notification/domain/repository/NotificationRepository.java`:
- Around line 14-35: The Notification queries in NotificationRepository (methods
findNextByCursor and findByUserOrderByCreatedAtDescIdDesc) rely on a composite
index for efficient cursor-based pagination but the Notification entity lacks
one; add a composite index on (user_id, created_at DESC, id DESC) either by
annotating the Notification entity with a `@Table`(indexes = `@Index`(...)) that
names the index (e.g., idx_notification_user_created_id) and lists columnList =
"user_id, created_at DESC, id DESC", or add a DB migration that creates the
index (CREATE INDEX idx_notification_user_created_id ON notification(user_id,
created_at DESC, id DESC)); ensure column names match the entity mappings.
In `@src/main/java/com/campus/campus/domain/notification/util/TimeFormatter.java`:
- Around line 16-22: The code in TimeFormatter (where Duration duration =
Duration.between(dateTime, now); long seconds = duration.getSeconds();) doesn't
handle future dateTime values and seconds can be negative; update the method to
detect negative duration (seconds < 0) and handle it consistently (e.g., clamp
seconds to 0 or immediately return a suitable string like "방금 전" or "곧") before
the existing if/else branches, using the same variables dateTime, duration, and
seconds so downstream logic stays correct.
- Around line 40-48: The current logic in TimeFormatter (around the weeks/months
calculation) can produce "0개월 전" for days==28 or 29; update the logic in the
method that computes weeks/months so that when weeks >=4 but days <30 you return
"4주 전" (or otherwise ensure months is at least 1) instead of computing months =
0; locate the block using the variables weeks and months and change the
condition to handle days in [28,29] explicitly (or set months = Math.max(1,
days/30)) and return the corrected string accordingly.
🧹 Nitpick comments (7)
src/main/java/com/campus/campus/domain/notification/presentation/NotificationResponseCode.java (1)
14-19: 메시지 형식 일관성 검토마지막 응답 코드
NOTIFICATION_UNREAD_EXISTS_SUCCESS의 메시지만 마침표(.)로 끝나고 있습니다. 일관성을 위해 다른 메시지들과 형식을 맞추는 것이 좋습니다.♻️ 제안된 수정
- NOTIFICATION_UNREAD_EXISTS_SUCCESS(200, HttpStatus.OK, "미확인 알림 존재 여부 조회에 성공했습니다."); + NOTIFICATION_UNREAD_EXISTS_SUCCESS(200, HttpStatus.OK, "미확인 알림 존재 여부 조회 성공");src/main/java/com/campus/campus/domain/notification/application/dto/CursorResponse.java (1)
5-9: 커서 페이지네이션 응답 통합 고려
CursorPageReviewResponse가review도메인에 이미 존재합니다(nextCursorCreatedAt,nextCursorId,hasNext필드 사용). 현재 구현은NextCursor객체를 사용하여 더 깔끔하지만, 프로젝트 전반에 걸쳐 두 가지 커서 페이지네이션 패턴이 존재하게 됩니다.향후 공통 패키지(
global.common.dto등)에서 제네릭 커서 응답을 통합하는 것을 고려해 보세요.src/main/java/com/campus/campus/domain/notification/util/TimeFormatter.java (1)
11-17: 테스트 용이성을 위한 Clock 주입 고려
LocalDateTime.now()직접 호출은 단위 테스트 시 시간 제어가 어렵습니다.java.time.Clock을 주입받으면 테스트에서 고정된 시간을 사용할 수 있습니다.♻️ 리팩토링 제안
+import java.time.Clock; import java.time.Duration; import java.time.LocalDateTime; `@Component` public class TimeFormatter { + private final Clock clock; + + public TimeFormatter() { + this.clock = Clock.systemDefaultZone(); + } + + public TimeFormatter(Clock clock) { + this.clock = clock; + } + public String formatRelativeTime(LocalDateTime dateTime) { if (dateTime == null) { return null; } - LocalDateTime now = LocalDateTime.now(); + LocalDateTime now = LocalDateTime.now(clock);src/main/java/com/campus/campus/domain/notification/domain/entity/Notification.java (2)
23-26: @SuperBuilder vs@Builder불일치
BaseEntity가@SuperBuilder를 사용하는데Notification은@Builder를 사용하고 있습니다. 이 경우 부모 클래스의 필드(createdAt,updatedAt)가 빌더에 포함되지 않을 수 있습니다. 현재 코드에서는createdAt이@CreatedDate로 자동 설정되므로 큰 문제는 없지만, 일관성을 위해@SuperBuilder사용을 권장합니다.♻️ 수정 제안
`@Entity` `@Getter` `@NoArgsConstructor`(access = AccessLevel.PROTECTED) +@SuperBuilder public class Notification extends BaseEntity { // ... - `@Builder` - public Notification(User user, NotificationType type, String title, - String body, Long referenceId) { - this.user = user; - this.type = type; - this.title = title; - this.body = body; - this.referenceId = referenceId; - }Also applies to: 55-63
65-68: markAsRead() 동시 호출 시 멱등성 확인현재 구현은 이미 읽은 알림에 대해 다시 호출해도 문제없이 동작합니다. 다만, 멱등성을 명시적으로 보장하려면
isRead상태 확인을 추가하는 것도 고려해 볼 수 있습니다.♻️ 선택적 개선
public void markAsRead() { + if (this.isRead) { + return; + } this.isRead = true; this.readAt = LocalDateTime.now(); }src/main/java/com/campus/campus/domain/notification/application/mapper/NotificationMapper.java (1)
19-29: toResponse()에서 null 입력 처리 고려
notification파라미터가 null인 경우 NPE가 발생합니다. 호출자가 null을 전달하지 않도록 보장된다면 괜찮지만, 방어적으로 처리하거나@NonNull어노테이션을 추가하는 것도 좋습니다.src/main/java/com/campus/campus/domain/notification/application/service/NotificationService.java (1)
38-69:@Transactional(readOnly = true)어노테이션 추가를 권장합니다.읽기 전용 메서드에
@Transactional(readOnly = true)를 추가하면 다음과 같은 이점이 있습니다:
- JPA 더티 체킹 비활성화로 성능 향상
- 읽기 전용 레플리카로 라우팅 가능 (데이터베이스 구성에 따라)
- 다른 메서드들(
hasUnread)과의 일관성 유지♻️ 권장 수정 사항
+ `@Transactional`(readOnly = true) public CursorResponse<NotificationResponse> getNotificationsByCursor( Long userId, LocalDateTime cursorCreatedAt, Long cursorId, int limit ) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/main/java/com/campus/campus/domain/council/domain/entity/CouncilType.javasrc/main/java/com/campus/campus/domain/councilpost/application/CouncilPostPushListener.javasrc/main/java/com/campus/campus/domain/notification/application/dto/CursorResponse.javasrc/main/java/com/campus/campus/domain/notification/application/dto/NextCursor.javasrc/main/java/com/campus/campus/domain/notification/application/dto/NotificationListResponse.javasrc/main/java/com/campus/campus/domain/notification/application/dto/NotificationResponse.javasrc/main/java/com/campus/campus/domain/notification/application/exception/ErrorCode.javasrc/main/java/com/campus/campus/domain/notification/application/exception/NotificationAccessDeniedException.javasrc/main/java/com/campus/campus/domain/notification/application/exception/NotificationNotFoundException.javasrc/main/java/com/campus/campus/domain/notification/application/mapper/NotificationMapper.javasrc/main/java/com/campus/campus/domain/notification/application/service/NotificationService.javasrc/main/java/com/campus/campus/domain/notification/domain/entity/Notification.javasrc/main/java/com/campus/campus/domain/notification/domain/entity/NotificationType.javasrc/main/java/com/campus/campus/domain/notification/domain/repository/NotificationRepository.javasrc/main/java/com/campus/campus/domain/notification/presentation/NotificationController.javasrc/main/java/com/campus/campus/domain/notification/presentation/NotificationResponseCode.javasrc/main/java/com/campus/campus/domain/notification/util/TimeFormatter.javasrc/main/java/com/campus/campus/domain/user/domain/repository/UserRepository.java
🧰 Additional context used
🧬 Code graph analysis (11)
src/main/java/com/campus/campus/domain/notification/application/dto/NextCursor.java (3)
src/main/java/com/campus/campus/domain/review/application/dto/response/CursorPageReviewResponse.java (1)
Getter(8-16)src/main/java/com/campus/campus/global/entity/BaseEntity.java (1)
Getter(16-29)src/main/java/com/campus/campus/domain/review/application/mapper/ReviewMapper.java (1)
toCursorReviewResponse(80-88)
src/main/java/com/campus/campus/domain/notification/application/exception/NotificationAccessDeniedException.java (1)
src/main/java/com/campus/campus/domain/councilpost/application/exception/PostAccessDeniedException.java (1)
PostAccessDeniedException(5-9)
src/main/java/com/campus/campus/domain/notification/application/mapper/NotificationMapper.java (1)
src/main/java/com/campus/campus/domain/notification/util/TimeFormatter.java (1)
Component(8-53)
src/main/java/com/campus/campus/domain/notification/application/dto/NotificationResponse.java (3)
src/main/java/com/campus/campus/domain/user/application/dto/response/UserInfoResponse.java (1)
UserInfoResponse(5-21)src/main/java/com/campus/campus/domain/place/application/dto/response/RecommendPartnershipPlaceResponse.java (1)
RecommendPartnershipPlaceResponse(5-24)src/main/java/com/campus/campus/domain/school/application/dto/response/CollegeFindResponse.java (1)
CollegeFindResponse(5-12)
src/main/java/com/campus/campus/domain/user/domain/repository/UserRepository.java (2)
src/main/java/com/campus/campus/domain/user/domain/entity/User.java (2)
Entity(26-93)updateProfile(72-76)src/main/java/com/campus/campus/domain/school/domain/entity/Major.java (1)
Entity(18-40)
src/main/java/com/campus/campus/domain/council/domain/entity/CouncilType.java (3)
src/main/java/com/campus/campus/domain/council/application/service/CouncilLoginService.java (2)
validateCouncilScope(201-247)CouncilScope(261-266)src/main/java/com/campus/campus/domain/council/domain/entity/StudentCouncil.java (1)
Entity(28-103)src/main/java/com/campus/campus/domain/council/application/exception/InvalidCouncilScopeException.java (1)
InvalidCouncilScopeException(6-8)
src/main/java/com/campus/campus/domain/notification/application/dto/CursorResponse.java (2)
src/main/java/com/campus/campus/domain/review/application/dto/response/CursorPageReviewResponse.java (1)
Getter(8-16)src/main/java/com/campus/campus/domain/review/application/mapper/ReviewMapper.java (2)
toCursorReviewResponse(80-88)toEmptyCursorReviewResponse(38-45)
src/main/java/com/campus/campus/domain/notification/domain/entity/Notification.java (2)
src/main/java/com/campus/campus/domain/user/domain/entity/User.java (1)
Entity(26-93)src/main/java/com/campus/campus/global/entity/BaseEntity.java (1)
Getter(16-29)
src/main/java/com/campus/campus/domain/notification/util/TimeFormatter.java (2)
src/main/java/com/campus/campus/domain/notification/application/mapper/NotificationMapper.java (1)
Component(13-41)src/main/java/com/campus/campus/domain/councilpost/application/dto/response/NormalizedDateTime.java (1)
NormalizedDateTime(5-9)
src/main/java/com/campus/campus/domain/notification/presentation/NotificationController.java (2)
src/main/java/com/campus/campus/domain/councilpost/application/CouncilPostPushListener.java (1)
Slf4j(18-61)src/main/java/com/campus/campus/domain/notification/application/service/NotificationService.java (1)
Slf4j(29-122)
src/main/java/com/campus/campus/domain/notification/application/service/NotificationService.java (5)
src/main/java/com/campus/campus/domain/notification/application/exception/NotificationAccessDeniedException.java (1)
NotificationAccessDeniedException(5-9)src/main/java/com/campus/campus/domain/notification/application/exception/NotificationNotFoundException.java (1)
NotificationNotFoundException(5-9)src/main/java/com/campus/campus/domain/user/application/exception/UserNotFoundException.java (1)
UserNotFoundException(5-9)src/main/java/com/campus/campus/domain/councilpost/application/CouncilPostPushListener.java (1)
Slf4j(18-61)src/main/java/com/campus/campus/domain/notification/presentation/NotificationController.java (1)
Slf4j(24-79)
🪛 GitHub Actions: campus CI pipeline
src/main/java/com/campus/campus/domain/council/domain/entity/CouncilType.java
[error] 1-1: Gradle build failed due to compilation errors in CouncilType.java: multiple methods annotated with @Override do not override/implement any method from the supertype.
🔇 Additional comments (12)
src/main/java/com/campus/campus/domain/notification/application/dto/NextCursor.java (1)
5-8: LGTM!커서 기반 페이지네이션을 위한 레코드가 깔끔하게 구현되어 있습니다.
LocalDateTime과Long타입을 사용하여 타입 안전성을 확보한 점이 좋습니다.src/main/java/com/campus/campus/domain/notification/application/exception/ErrorCode.java (1)
12-20: LGTM!에러 코드 enum이 적절하게 구현되어 있습니다. HTTP 상태 코드 매핑이 각 에러 유형에 맞게 올바르게 설정되어 있습니다.
src/main/java/com/campus/campus/domain/notification/application/exception/NotificationAccessDeniedException.java (1)
5-9: LGTM!기존
PostAccessDeniedException패턴과 일관되게 구현되어 있습니다.src/main/java/com/campus/campus/domain/notification/application/exception/NotificationNotFoundException.java (1)
1-9: LGTM!예외 클래스가
ApplicationException을 올바르게 확장하고ErrorCode.NOTIFICATION_NOT_FOUND를 사용하여 일관된 예외 처리 패턴을 따르고 있습니다.src/main/java/com/campus/campus/domain/user/domain/repository/UserRepository.java (1)
46-50: LGTM!Spring Data JPA 프로퍼티 탐색 문법을 올바르게 사용하고 있으며, soft-delete 패턴(
DeletedAtIsNull)을 일관되게 적용했습니다.User엔티티의Major,College,School관계와 각 엔티티의 ID 필드명이 메서드 시그니처와 일치합니다.src/main/java/com/campus/campus/domain/notification/domain/entity/NotificationType.java (1)
1-6: LGTM!알림 유형을 명확하게 정의한 enum입니다.
COUNCIL_POST_CREATED와SYSTEM_NOTICE두 가지 타입으로 현재 요구사항을 충족합니다.src/main/java/com/campus/campus/domain/notification/application/dto/NotificationResponse.java (1)
7-29: LGTM!DTO 구조가 깔끔하고 프로젝트의 다른 Response 클래스들(UserInfoResponse, CollegeFindResponse 등)과 일관된 패턴을 따르고 있습니다. Swagger 문서화도 적절합니다.
src/main/java/com/campus/campus/domain/notification/domain/entity/Notification.java (1)
28-53: 엔티티 구조 적절함JPA 매핑과 필드 구성이 잘 되어 있습니다.
user의 LAZY 로딩,type의 STRING 매핑,referenceId를 통한 유연한 참조 설계가 좋습니다.src/main/java/com/campus/campus/domain/notification/application/mapper/NotificationMapper.java (1)
31-40: LGTM!
createNotification팩토리 메서드가 빌더 패턴을 적절히 활용하고 있습니다. Notification 엔티티 생성 로직이 한 곳에서 관리되어 유지보수성이 좋습니다.src/main/java/com/campus/campus/domain/notification/presentation/NotificationController.java (1)
24-79: LGTM! 컨트롤러가 깔끔하게 구현되었습니다.
- 엔드포인트가 RESTful 규칙을 잘 따르고 있습니다
@DateTimeFormat어노테이션으로 ISO 형식의 날짜/시간 파싱이 올바르게 처리됩니다- Swagger 문서화가 적절하게 되어 있습니다
- 서비스 계층에 비즈니스 로직이 위임되어 있어 책임 분리가 잘 되어 있습니다
src/main/java/com/campus/campus/domain/notification/application/service/NotificationService.java (2)
71-84: LGTM! 알림 소유권 검증이 올바르게 구현되었습니다.
markAsRead메서드에서 사용자와 알림 존재 여부를 확인한 후, 소유권 검증을 통해 다른 사용자의 알림에 접근하지 못하도록 보호하고 있습니다.
86-102: LGTM!REQUIRES_NEW전파 속성이 적절하게 사용되었습니다.
CouncilPostPushListener에서@TransactionalEventListener(phase = AFTER_COMMIT)으로 호출되므로, 기존 트랜잭션이 이미 커밋된 상태입니다.REQUIRES_NEW를 사용하여 새로운 트랜잭션에서 알림을 저장하는 것이 올바른 접근입니다.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
src/main/java/com/campus/campus/domain/council/domain/entity/CouncilType.java
Show resolved
Hide resolved
...ain/java/com/campus/campus/domain/notification/application/dto/NotificationListResponse.java
Outdated
Show resolved
Hide resolved
| private List<User> findUsersByTopic(String topic) { | ||
|
|
||
| String[] parts = topic.split("_"); | ||
| String scope = parts[0]; | ||
| Long scopeId = Long.valueOf(parts[1]); | ||
|
|
||
| return switch (scope) { | ||
| case "major" -> userRepository.findAllByMajor_MajorIdAndDeletedAtIsNull(scopeId); | ||
| case "college" -> userRepository.findAllByCollege_CollegeIdAndDeletedAtIsNull(scopeId); | ||
| case "school" -> userRepository.findAllBySchool_SchoolIdAndDeletedAtIsNull(scopeId); | ||
| default -> List.of(); | ||
| }; | ||
| } |
There was a problem hiding this comment.
토픽 파싱 시 예외 처리가 필요합니다.
topic.split("_") 결과가 예상과 다를 경우(예: "invalid", "major" 등) ArrayIndexOutOfBoundsException 또는 NumberFormatException이 발생할 수 있습니다. 이 메서드는 @Async 이벤트 리스너에서 호출되므로, 예외 발생 시 알림 저장이 실패하고 로깅 없이 문제를 추적하기 어려워집니다.
🔒️ 방어적 코드 추가 권장
private List<User> findUsersByTopic(String topic) {
+ if (topic == null || !topic.contains("_")) {
+ log.warn("Invalid topic format: {}", topic);
+ return List.of();
+ }
String[] parts = topic.split("_");
+ if (parts.length < 2) {
+ log.warn("Invalid topic format: {}", topic);
+ return List.of();
+ }
+
String scope = parts[0];
- Long scopeId = Long.valueOf(parts[1]);
+ Long scopeId;
+ try {
+ scopeId = Long.valueOf(parts[1]);
+ } catch (NumberFormatException e) {
+ log.warn("Invalid scopeId in topic: {}", topic);
+ return List.of();
+ }
return switch (scope) {
case "major" -> userRepository.findAllByMajor_MajorIdAndDeletedAtIsNull(scopeId);
case "college" -> userRepository.findAllByCollege_CollegeIdAndDeletedAtIsNull(scopeId);
case "school" -> userRepository.findAllBySchool_SchoolIdAndDeletedAtIsNull(scopeId);
default -> List.of();
};
}📝 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.
| private List<User> findUsersByTopic(String topic) { | |
| String[] parts = topic.split("_"); | |
| String scope = parts[0]; | |
| Long scopeId = Long.valueOf(parts[1]); | |
| return switch (scope) { | |
| case "major" -> userRepository.findAllByMajor_MajorIdAndDeletedAtIsNull(scopeId); | |
| case "college" -> userRepository.findAllByCollege_CollegeIdAndDeletedAtIsNull(scopeId); | |
| case "school" -> userRepository.findAllBySchool_SchoolIdAndDeletedAtIsNull(scopeId); | |
| default -> List.of(); | |
| }; | |
| } | |
| private List<User> findUsersByTopic(String topic) { | |
| if (topic == null || !topic.contains("_")) { | |
| log.warn("Invalid topic format: {}", topic); | |
| return List.of(); | |
| } | |
| String[] parts = topic.split("_"); | |
| if (parts.length < 2) { | |
| log.warn("Invalid topic format: {}", topic); | |
| return List.of(); | |
| } | |
| String scope = parts[0]; | |
| Long scopeId; | |
| try { | |
| scopeId = Long.valueOf(parts[1]); | |
| } catch (NumberFormatException e) { | |
| log.warn("Invalid scopeId in topic: {}", topic); | |
| return List.of(); | |
| } | |
| return switch (scope) { | |
| case "major" -> userRepository.findAllByMajor_MajorIdAndDeletedAtIsNull(scopeId); | |
| case "college" -> userRepository.findAllByCollege_CollegeIdAndDeletedAtIsNull(scopeId); | |
| case "school" -> userRepository.findAllBySchool_SchoolIdAndDeletedAtIsNull(scopeId); | |
| default -> List.of(); | |
| }; | |
| } |
| public interface NotificationRepository extends JpaRepository<Notification, Long> { | ||
|
|
||
| List<Notification> findByUserOrderByCreatedAtDescIdDesc(User user, Pageable pageable); | ||
|
|
||
| @Query(""" | ||
| select n from Notification n | ||
| where n.user = :user | ||
| and ( | ||
| n.createdAt < :cursorCreatedAt | ||
| or (n.createdAt = :cursorCreatedAt and n.id < :cursorId) | ||
| ) | ||
| order by n.createdAt desc, n.id desc | ||
| """) | ||
| List<Notification> findNextByCursor( | ||
| @Param("user") User user, | ||
| @Param("cursorCreatedAt") LocalDateTime cursorCreatedAt, | ||
| @Param("cursorId") Long cursorId, | ||
| Pageable pageable | ||
| ); | ||
|
|
||
| boolean existsByUser_IdAndIsReadFalse(Long userId); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e java "Notification.java" -type fRepository: our-campUS/our-campUS-BE
Length of output: 238
🏁 Script executed:
# Search for Notification entity class definition
rg "class Notification" -t java -B 5 -A 50Repository: our-campUS/our-campUS-BE
Length of output: 25698
🏁 Script executed:
# Check for index definitions on Notification entity
rg "@Index|@Table" -t java -B 2 -A 2 | grep -A 10 -B 10 "Notification"Repository: our-campUS/our-campUS-BE
Length of output: 50
🏁 Script executed:
# Verify the repository file exists and check current state
cat -n src/main/java/com/campus/campus/domain/notification/domain/repository/NotificationRepository.javaRepository: our-campUS/our-campUS-BE
Length of output: 1440
🏁 Script executed:
rg "class BaseEntity" -B 3 -A 20 --max-count=1Repository: our-campUS/our-campUS-BE
Length of output: 1127
🏁 Script executed:
# Search for index definitions in migration files or entity annotations
fd -name "*.sql" -o -name "*.yml" -o -name "*.yaml" | xargs rg -l "notification" -i 2>/dev/null | head -20Repository: our-campUS/our-campUS-BE
Length of output: 1909
🏁 Script executed:
# Check for `@Table` or `@Index` annotations on Notification entity
cat -n src/main/java/com/campus/campus/domain/notification/domain/entity/Notification.java | head -30Repository: our-campUS/our-campUS-BE
Length of output: 1156
🏁 Script executed:
find . -path "*/db/migration*" -type f | head -20Repository: our-campUS/our-campUS-BE
Length of output: 50
🏁 Script executed:
# Search for index definitions on notification table
rg "create.*index.*notification|notification.*index" -iRepository: our-campUS/our-campUS-BE
Length of output: 50
🏁 Script executed:
# Check for Flyway migrations directory
fd -type d "migration" "db" "flyway"Repository: our-campUS/our-campUS-BE
Length of output: 238
복합 인덱스 정의가 필요합니다: (user_id, created_at, id)
JPQL 쿼리와 메서드 이름은 커서 기반 페이지네이션을 올바르게 구현하고 있습니다. 다만 Notification 엔티티에 성능 최적화를 위한 복합 인덱스가 정의되어 있지 않습니다. @Index 애노테이션 또는 데이터베이스 마이그레이션 파일에서 다음 인덱스를 명시적으로 정의하세요:
CREATE INDEX idx_notification_user_created_id
ON notification(user_id, created_at DESC, id DESC);또는 엔티티에 추가:
`@Table`(indexes = `@Index`(name = "idx_notification_user_created_id",
columnList = "user_id, created_at DESC, id DESC"))🤖 Prompt for AI Agents
In
`@src/main/java/com/campus/campus/domain/notification/domain/repository/NotificationRepository.java`
around lines 14 - 35, The Notification queries in NotificationRepository
(methods findNextByCursor and findByUserOrderByCreatedAtDescIdDesc) rely on a
composite index for efficient cursor-based pagination but the Notification
entity lacks one; add a composite index on (user_id, created_at DESC, id DESC)
either by annotating the Notification entity with a `@Table`(indexes =
`@Index`(...)) that names the index (e.g., idx_notification_user_created_id) and
lists columnList = "user_id, created_at DESC, id DESC", or add a DB migration
that creates the index (CREATE INDEX idx_notification_user_created_id ON
notification(user_id, created_at DESC, id DESC)); ensure column names match the
entity mappings.
There was a problem hiding this comment.
@jjaeroong 인덱스를 정의하는 것도 좋은 방법이지만(인덱스를 정의하면 알림 조회 성능이 좋아질 것으로 예상) 개인적인 생각으로 추후 사용자와 알림이 많아진다면 인덱스 정의로 인해 알림을 전송하는데 조금 더 오랜 시간이 걸릴 수도 있어 해당 부분은 아직 적용하기에는 이를 것으로 생각이 들긴합니다.
개인적인 생각이지만 재영님도 한번 확인 정도만 해주세요!!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
동의합니다! 전송 쪽이 더 중요하다고 생각해요
| LocalDateTime now = LocalDateTime.now(); | ||
| Duration duration = Duration.between(dateTime, now); | ||
|
|
||
| long seconds = duration.getSeconds(); | ||
|
|
||
| if (seconds < 60) { | ||
| return "방금 전"; |
There was a problem hiding this comment.
미래 시간 입력 시 음수 처리 누락
dateTime이 현재 시간보다 미래인 경우 duration.getSeconds()가 음수를 반환하여 예상치 못한 결과가 발생할 수 있습니다.
🛡️ 방어 코드 추가 제안
LocalDateTime now = LocalDateTime.now();
Duration duration = Duration.between(dateTime, now);
long seconds = duration.getSeconds();
+
+ if (seconds < 0) {
+ return "방금 전";
+ }
if (seconds < 60) {
return "방금 전";
}📝 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.
| LocalDateTime now = LocalDateTime.now(); | |
| Duration duration = Duration.between(dateTime, now); | |
| long seconds = duration.getSeconds(); | |
| if (seconds < 60) { | |
| return "방금 전"; | |
| LocalDateTime now = LocalDateTime.now(); | |
| Duration duration = Duration.between(dateTime, now); | |
| long seconds = duration.getSeconds(); | |
| if (seconds < 0) { | |
| return "방금 전"; | |
| } | |
| if (seconds < 60) { | |
| return "방금 전"; |
🤖 Prompt for AI Agents
In `@src/main/java/com/campus/campus/domain/notification/util/TimeFormatter.java`
around lines 16 - 22, The code in TimeFormatter (where Duration duration =
Duration.between(dateTime, now); long seconds = duration.getSeconds();) doesn't
handle future dateTime values and seconds can be negative; update the method to
detect negative duration (seconds < 0) and handle it consistently (e.g., clamp
seconds to 0 or immediately return a suitable string like "방금 전" or "곧") before
the existing if/else branches, using the same variables dateTime, duration, and
seconds so downstream logic stays correct.
| long weeks = days / 7; | ||
| if (weeks < 4) { | ||
| return weeks + "주 전"; | ||
| } | ||
|
|
||
| long months = days / 30; | ||
| if (months < 12) { | ||
| return months + "개월 전"; | ||
| } |
There was a problem hiding this comment.
에지 케이스 버그: "0개월 전" 출력 가능
weeks < 4 조건이 실패하면 (즉, days >= 28일 때), days / 30 계산에서 days가 28 또는 29인 경우 months = 0이 됩니다. 이 경우 "0개월 전"이라는 잘못된 문자열이 반환됩니다.
🐛 수정 제안
long weeks = days / 7;
- if (weeks < 4) {
+ if (weeks < 5) {
return weeks + "주 전";
}
long months = days / 30;
- if (months < 12) {
+ if (months > 0 && months < 12) {
return months + "개월 전";
}
+
+ if (months == 0) {
+ return weeks + "주 전";
+ }또는 더 간단하게:
long weeks = days / 7;
- if (weeks < 4) {
+ if (days < 30) {
return weeks + "주 전";
}🤖 Prompt for AI Agents
In `@src/main/java/com/campus/campus/domain/notification/util/TimeFormatter.java`
around lines 40 - 48, The current logic in TimeFormatter (around the
weeks/months calculation) can produce "0개월 전" for days==28 or 29; update the
logic in the method that computes weeks/months so that when weeks >=4 but days
<30 you return "4주 전" (or otherwise ensure months is at least 1) instead of
computing months = 0; locate the block using the variables weeks and months and
change the condition to handle days in [28,29] explicitly (or set months =
Math.max(1, days/30)) and return the corrected string accordingly.
There was a problem hiding this comment.
수고하셨습니다!!! 걱정하셨던 부분과는 다르게 상당히 잘 구현되었다고 개인적으로 생각합니다!! 특히 읽음 처리 기능을 구현하기 위해 노력하신 부분이 보여 좋다고 생각했습니다.
물론 현재 서비스가 실제 사용되고 사용자가 정말 많아지며 총학생회에서 공지 혹은 게시글을 올려 알림이 전송된다면 수많은 사람들의 User 엔티티가 메모리에 로딩되어 DB가 버티지 못할 가능성이 커 개선의 필요성이 있지만 현재로서는 해당 구현 방식도 충분히 좋다고 생각합니다!!
특히 알림 저장은 별도 트랜잭션으로 처리하신 것은 만일 알림 전송이 실패했더라도 알림 내용은 남길 수 있기에 좋은 방식이라 생각했습니다.
간단한 부분들에 대한 리뷰를 남겼으니 한번 확인만 부탁드리고 마지막 행 개행 처리가 이루어지지 않은 부분들이 있던데 개행처리 추가로 부탁드려요!!
...ain/java/com/campus/campus/domain/notification/application/dto/NotificationListResponse.java
Outdated
Show resolved
Hide resolved
| boolean isRead, | ||
|
|
||
| @Schema(description = "생성 시각 (상대 시간)", example = "5분 전") | ||
| String createdAt |
There was a problem hiding this comment.
BaseEntity에는 createdAt이 포함되어 있어 이름이 겹치기에 해당 이름을 createTimeBeforeNow 이런식으로 바꾸는 것이 좋아보여요!!
| } | ||
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public void saveCouncilPostCreated(CouncilPostCreatedEvent event, String title, String body) { |
There was a problem hiding this comment.
사소하지만 메서드명을 savePostCreatedNotification으로 하면 조금 더 직관적으로 보일 것 같아요!!
| public interface NotificationRepository extends JpaRepository<Notification, Long> { | ||
|
|
||
| List<Notification> findByUserOrderByCreatedAtDescIdDesc(User user, Pageable pageable); | ||
|
|
||
| @Query(""" | ||
| select n from Notification n | ||
| where n.user = :user | ||
| and ( | ||
| n.createdAt < :cursorCreatedAt | ||
| or (n.createdAt = :cursorCreatedAt and n.id < :cursorId) | ||
| ) | ||
| order by n.createdAt desc, n.id desc | ||
| """) | ||
| List<Notification> findNextByCursor( | ||
| @Param("user") User user, | ||
| @Param("cursorCreatedAt") LocalDateTime cursorCreatedAt, | ||
| @Param("cursorId") Long cursorId, | ||
| Pageable pageable | ||
| ); | ||
|
|
||
| boolean existsByUser_IdAndIsReadFalse(Long userId); | ||
| } |
There was a problem hiding this comment.
@jjaeroong 인덱스를 정의하는 것도 좋은 방법이지만(인덱스를 정의하면 알림 조회 성능이 좋아질 것으로 예상) 개인적인 생각으로 추후 사용자와 알림이 많아진다면 인덱스 정의로 인해 알림을 전송하는데 조금 더 오랜 시간이 걸릴 수도 있어 해당 부분은 아직 적용하기에는 이를 것으로 생각이 들긴합니다.
개인적인 생각이지만 재영님도 한번 확인 정도만 해주세요!!
| Long cursorId, | ||
| int limit | ||
| ) { | ||
| User user = userRepository.findById(userId) |
There was a problem hiding this comment.
findById()보다는 삭제되지 않은 유저를 가져오는 findByIdAndDeleteAtIsNull이 조금 더 좋아보여요!!
밑에 다른 메서드들도 마찬가지입니다!!
| Pageable pageable | ||
| ); | ||
|
|
||
| boolean existsByUser_IdAndIsReadFalse(Long userId); |
There was a problem hiding this comment.
아마 IsRead를 JPA에서 인식하지 못할 수도 있을 것 같은데 제대로 동작하는지 확인한번만 부탁드려요!!
컬럼명이 앞에 is가 붙으면 JPARepository에서 잘 인식하지 못해서 인텔리제이에서 경고문을 보내는 경우가 있더라구요!!
There was a problem hiding this comment.
이거 안그래도 어제 확인해봤는데 되더라고요!!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/com/campus/campus/domain/councilnotice/application/service/StudentCouncilNoticeService.java`:
- Around line 94-98: The PageRequest uses a non-existent property
"createTimeBeforeNow" causing PropertyReferenceException; update the Sort in the
Pageable creation (the PageRequest.of call that builds pageable) to use the
actual timestamp field "createdAt" (i.e., replace Sort.by(...,
"createTimeBeforeNow") with Sort.by(..., "createdAt") so sorting uses the
BaseEntity's createdAt field).
♻️ Duplicate comments (1)
src/main/java/com/campus/campus/domain/notification/application/service/NotificationService.java (1)
109-121: 토픽 파싱 시 예외 처리가 필요합니다.이전 리뷰에서 언급된 대로,
topic.split("_")결과가 예상과 다를 경우ArrayIndexOutOfBoundsException또는NumberFormatException이 발생할 수 있습니다.@Async이벤트 리스너에서 호출되므로 예외 발생 시 알림 저장이 실패하고 문제 추적이 어려워집니다.
🧹 Nitpick comments (2)
src/main/java/com/campus/campus/domain/notification/application/service/NotificationService.java (2)
38-69: 읽기 전용 트랜잭션 어노테이션 추가 권장이 메서드는 데이터 조회만 수행하므로
@Transactional(readOnly = true)를 추가하는 것이 좋습니다. 이를 통해 Hibernate의 dirty checking을 비활성화하고, 복제 DB 환경에서 읽기 전용 복제본으로 라우팅되며, JPA 영속성 컨텍스트의 스냅샷 저장을 생략하여 성능을 개선할 수 있습니다.♻️ 제안 수정
+ `@Transactional`(readOnly = true) public CursorResponse<NotificationResponse> getNotificationsByCursor( Long userId, LocalDateTime cursorCreatedAt, Long cursorId, int limit ) {
86-102: 대량 사용자 알림 생성 시 성능 고려
school범위 토픽의 경우 대상 사용자 수가 많을 수 있습니다. 현재 구현은 모든 대상 사용자에 대해 메모리에Notification객체를 생성한 후saveAll로 일괄 저장합니다. 사용자 수가 수천 명 이상일 경우 메모리 사용량과 DB 부하가 증가할 수 있으므로, 향후 배치 처리나 청크 단위 저장을 고려해 보세요.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/campus/campus/domain/councilnotice/application/service/StudentCouncilNoticeService.javasrc/main/java/com/campus/campus/domain/councilpost/application/CouncilPostPushListener.javasrc/main/java/com/campus/campus/domain/notification/application/service/NotificationService.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/campus/campus/domain/notification/application/service/NotificationService.java (3)
src/main/java/com/campus/campus/domain/notification/application/exception/NotificationAccessDeniedException.java (1)
NotificationAccessDeniedException(5-9)src/main/java/com/campus/campus/domain/notification/application/exception/NotificationNotFoundException.java (1)
NotificationNotFoundException(5-9)src/main/java/com/campus/campus/domain/user/application/exception/UserNotFoundException.java (1)
UserNotFoundException(5-9)
src/main/java/com/campus/campus/domain/councilpost/application/CouncilPostPushListener.java (1)
src/main/java/com/campus/campus/domain/councilpost/application/dto/request/CouncilPostCreatedEvent.java (1)
CouncilPostCreatedEvent(5-10)
🔇 Additional comments (4)
src/main/java/com/campus/campus/domain/notification/application/service/NotificationService.java (2)
71-84: LGTM!소유권 검증 로직이 적절하게 구현되어 있습니다. 사용자 검증 시
findByIdAndDeletedAtIsNull을 사용하여 삭제된 사용자를 제외하고 있으며, 알림 소유권 확인 후 읽음 처리를 수행합니다.
104-107: LGTM!효율적인 존재 여부 확인 쿼리를 사용하고 있으며,
@Transactional(readOnly = true)가 적절하게 적용되어 있습니다.src/main/java/com/campus/campus/domain/councilpost/application/CouncilPostPushListener.java (2)
41-42: 알림 저장 실패 시 푸시 발송 동작 확인 필요
savePostCreatedNotification호출이sendToTopic전에 위치하여, 알림 저장 중 예외 발생 시 푸시 알림도 발송되지 않습니다. 이것이 의도된 동작인지 확인이 필요합니다.반대로,
NotificationService.findUsersByTopic에서 유효하지 않은 토픽으로 인해 빈 리스트가 반환되면 DB 알림은 저장되지 않지만 푸시 알림은 정상 발송됩니다. 두 채널 간 일관성을 유지하려면findUsersByTopic의 예외 처리 개선과 함께 에러 핸들링 전략을 검토해 주세요.
29-29: LGTM!
NotificationService의존성 주입이 적절하게 추가되었습니다.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
🔀 변경 내용
✅ 작업 항목
📸 스크린샷 (선택)
📎 참고 이슈
관련 이슈 번호 #71
Summary by CodeRabbit
릴리스 노트
✏️ Tip: You can customize this high-level summary in your review settings.