[Fix] 알림 비활성화/활성화 API 변경 #376
Conversation
Walkthrough알람 비활성화 API가 단일 AlarmType에서 여러 AlarmType의 배치 처리로 변경되었습니다. 요청이 리스트로 바뀌고, 서비스는 기존 비활성화 항목을 일괄 조회하여 생성/삭제 대상을 집합으로 계산한 뒤 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as Controller
participant Service as AlarmService
participant Repo as UserDisabledAlarmRepository
participant DB as Database
Client->>Controller: PATCH /alarms/disable (alarmTypes: [A,B,C])
Controller->>Service: disableAlarm(user, [A,B,C])
Service->>Repo: findByUserAndAlarmTypeIn(user, [A,B,C])
Repo->>DB: SELECT existing disabled WHERE user & alarmType IN (...)
DB-->>Repo: [existing records]
Repo-->>Service: [existing disabled alarms]
Note over Service: 계산 — toCreate = requested - existing\ntoDelete = existing ∩ requested(remove flags)
Service->>Repo: saveAll(toCreate)
Repo->>DB: INSERT ... (batch)
DB-->>Repo: insert results
Repo-->>Service: created records
Service->>Repo: deleteAll(toDelete)
Repo->>DB: DELETE ... (batch)
DB-->>Repo: delete results
Repo-->>Service: deleted records
Service-->>Controller: PatchDisableAlarmResponse(userId, [DisableAlarmDto...])
Controller-->>Client: 200 OK { data: { userId, alarms: [...] } }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🧹 Nitpick comments (3)
src/main/java/ku_rum/backend/domain/alarm/application/AlarmService.java (2)
159-165: 루프 내 스트림 검색은 O(n*m) 복잡도를 유발합니다.매 반복마다
existingDisabledAlarms스트림을 순회하여 일치하는 항목을 찾고 있습니다.existingAlarmTypes를 Set으로 만들었듯이,existingDisabledAlarms도 Map으로 미리 변환하면 O(n) 복잡도로 개선할 수 있습니다.🔎 개선 제안
Set<AlarmType> existingAlarmTypes = existingDisabledAlarms.stream() .map(UserDisabledAlarm::getAlarmType) .collect(Collectors.toSet()); + Map<AlarmType, UserDisabledAlarm> existingAlarmMap = existingDisabledAlarms.stream() + .collect(Collectors.toMap(UserDisabledAlarm::getAlarmType, alarm -> alarm)); List<UserDisabledAlarm> toCreate = new ArrayList<>(); List<UserDisabledAlarm> toDelete = new ArrayList<>(); for (AlarmType alarmType : alarmTypes) { if (existingAlarmTypes.contains(alarmType)) { - existingDisabledAlarms.stream() - .filter(alarm -> alarm.getAlarmType().equals(alarmType)) - .findFirst() - .ifPresent(toDelete::add); + toDelete.add(existingAlarmMap.get(alarmType)); continue; }
147-149:alarmTypes가 null 또는 빈 리스트인 경우에 대한 검증이 필요합니다.
request.alarmTypes()가 null이거나 빈 리스트일 경우, 현재 코드는 정상적으로 동작하지만 불필요한 DB 조회가 발생합니다. 요청 유효성 검증을 추가하는 것이 좋습니다.🔎 개선 제안
public PatchDisableAlarmResponse disableAlarm(CustomUserDetails userDetails, PatchDisableAlarmRequest request) { User user = userService.getUser(); List<AlarmType> alarmTypes = request.alarmTypes(); + if (alarmTypes == null || alarmTypes.isEmpty()) { + return new PatchDisableAlarmResponse(user.getId(), List.of()); + } List<UserDisabledAlarm> existingDisabledAlarms = userDisabledAlarmRepository.findByUserAndAlarmTypeIn(user, alarmTypes);src/main/java/ku_rum/backend/domain/alarm/dto/request/PatchDisableAlarmRequest.java (1)
6-7: 요청 DTO에 유효성 검증 어노테이션 추가를 권장합니다.
alarmTypes가 null이거나 빈 리스트인 경우를 방지하기 위해 Jakarta Validation 어노테이션을 추가하는 것이 좋습니다.🔎 개선 제안
package ku_rum.backend.domain.alarm.dto.request; import java.util.List; +import jakarta.validation.constraints.NotEmpty; import ku_rum.backend.domain.alarm.domain.AlarmType; -public record PatchDisableAlarmRequest(List<AlarmType> alarmTypes) { +public record PatchDisableAlarmRequest(@NotEmpty List<AlarmType> alarmTypes) { }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/ku_rum/backend/domain/alarm/application/AlarmService.javasrc/main/java/ku_rum/backend/domain/alarm/domain/repository/UserDisabledAlarmRepository.javasrc/main/java/ku_rum/backend/domain/alarm/dto/request/PatchDisableAlarmRequest.javasrc/main/java/ku_rum/backend/domain/alarm/dto/response/DisableAlarmDto.javasrc/main/java/ku_rum/backend/domain/alarm/dto/response/PatchDisableAlarmResponse.javasrc/test/java/ku_rum/backend/domain/alarm/presentation/AlarmControllerTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
src/main/java/ku_rum/backend/domain/alarm/domain/repository/UserDisabledAlarmRepository.java (1)
16-17: LGTM!Spring Data JPA의 네이밍 컨벤션을 올바르게 사용하여 IN 절 쿼리를 생성합니다.
src/main/java/ku_rum/backend/domain/alarm/dto/response/DisableAlarmDto.java (1)
6-9: LGTM!새로운 DTO가 깔끔하게 구현되었습니다. 팩토리 메서드가 엔티티에서 필요한 데이터를 올바르게 추출합니다.
src/test/java/ku_rum/backend/domain/alarm/presentation/AlarmControllerTest.java (1)
205-227: LGTM!테스트가 배열 기반 API 구조를 올바르게 반영하고 있으며, REST Docs 필드 문서화도 적절하게 업데이트되었습니다.
| Long userId = null; | ||
| if (!toCreate.isEmpty()) { | ||
| userId = toCreate.get(0).getUser().getId(); | ||
| } else if (!toDelete.isEmpty()) { | ||
| userId = toDelete.get(0).getUser().getId(); | ||
| } |
There was a problem hiding this comment.
toCreate와 toDelete 모두 빈 리스트인 경우 userId가 null이 됩니다.
빈 리스트로 요청이 오거나 요청된 모든 알림 타입이 이미 현재 상태와 동일한 경우, userId가 null로 반환될 수 있습니다. 서비스 레이어에서 사용자 정보를 직접 전달하는 것을 고려해 주세요.
🔎 대안 제안
팩토리 메서드 시그니처를 변경하여 userId를 직접 받도록 수정:
- public static PatchDisableAlarmResponse of(List<UserDisabledAlarm> toCreate, List<UserDisabledAlarm> toDelete) {
- Long userId = null;
- if (!toCreate.isEmpty()) {
- userId = toCreate.get(0).getUser().getId();
- } else if (!toDelete.isEmpty()) {
- userId = toDelete.get(0).getUser().getId();
- }
+ public static PatchDisableAlarmResponse of(Long userId, List<UserDisabledAlarm> toCreate, List<UserDisabledAlarm> toDelete) {그리고 AlarmService에서 호출 시:
- return PatchDisableAlarmResponse.of(toCreate, toDelete);
+ return PatchDisableAlarmResponse.of(user.getId(), toCreate, toDelete);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/ku_rum/backend/domain/alarm/dto/response/PatchDisableAlarmResponse.java
around lines 9-14, the current logic can leave userId null when both toCreate
and toDelete are empty; change the factory/constructor to accept a non-null Long
userId (remove nullable deduction from lists), update all callers (AlarmService)
to pass the current user's id explicitly, and validate/assert userId is present
(or throw IllegalArgumentException) so the response never contains a null
userId.
src/main/java/ku_rum/backend/domain/alarm/dto/response/PatchDisableAlarmResponse.java
Show resolved
Hide resolved
Test Results 38 files 38 suites 11s ⏱️ Results for commit c859abd. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/ku_rum/backend/domain/alarm/application/AlarmService.java (1)
159-172: 토글 로직은 정확하지만, 성능 최적화가 가능합니다.각
alarmType에 대해existingDisabledAlarms를 반복적으로 필터링하는 것은 O(n×m) 복잡도를 가집니다.🔎 성능 개선을 위한 제안
먼저 Map을 생성하여 O(1) 조회가 가능하도록 개선할 수 있습니다:
Set<AlarmType> existingAlarmTypes = existingDisabledAlarms.stream() .map(UserDisabledAlarm::getAlarmType) .collect(Collectors.toSet()); + Map<AlarmType, UserDisabledAlarm> existingAlarmsMap = existingDisabledAlarms.stream() + .collect(Collectors.toMap(UserDisabledAlarm::getAlarmType, alarm -> alarm)); List<UserDisabledAlarm> toCreate = new ArrayList<>(); List<UserDisabledAlarm> toDelete = new ArrayList<>(); for (AlarmType alarmType : alarmTypes) { if (existingAlarmTypes.contains(alarmType)) { - existingDisabledAlarms.stream() - .filter(alarm -> alarm.getAlarmType().equals(alarmType)) - .findFirst() - .ifPresent(toDelete::add); + toDelete.add(existingAlarmsMap.get(alarmType)); continue; }src/main/java/ku_rum/backend/domain/alarm/dto/response/PatchDisableAlarmResponse.java (1)
19-23: 리스트 결합 방식을 Stream.concat으로 개선할 수 있습니다.현재 방식도 정확하지만, 더 함수형 스타일로 개선할 수 있습니다.
🔎 함수형 스타일 리팩토링 제안
- List<DisableAlarmDto> allAlarms = new ArrayList<>(); - allAlarms.addAll(createdAlarms); - allAlarms.addAll(deletedAlarms); + List<DisableAlarmDto> allAlarms = Stream.concat( + createdAlarms.stream(), + deletedAlarms.stream() + ).toList();이 경우
java.util.stream.Streamimport도 추가해야 합니다.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/ku_rum/backend/domain/alarm/application/AlarmService.javasrc/main/java/ku_rum/backend/domain/alarm/dto/response/PatchDisableAlarmResponse.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
src/main/java/ku_rum/backend/domain/alarm/application/AlarmService.java (3)
4-4: 배치 처리를 위한 import 추가가 적절합니다.
ArrayList,Set,Collectorsimport가 배치 연산 구현에 올바르게 사용되고 있습니다.Also applies to: 9-10
174-182: 조건부 배치 연산과 응답 생성이 올바릅니다.빈 리스트에 대한 불필요한 DB 호출을 방지하는 조건 검사가 적절하고, Line 181에서
user.getId()를 명시적으로 전달하여 이전 리뷰에서 지적된 userId null 이슈를 해결했습니다.
147-158: 배치 처리 구조가 올바르게 구현되었습니다.여러 AlarmType을 한 번에 처리하기 위한 구조가 적절하며,
UserDisabledAlarmRepository.findByUserAndAlarmTypeIn()메서드가 List 파라미터로 IN 절을 올바르게 처리합니다. 기존 비활성화된 알림을 조회하고 Set으로 변환하여 효율적인 조회가 가능합니다.src/main/java/ku_rum/backend/domain/alarm/dto/response/PatchDisableAlarmResponse.java (2)
7-9: 이전 리뷰의 userId null 이슈가 해결되었습니다.팩토리 메서드 시그니처가
userId를 파라미터로 직접 받도록 변경되어,toCreate와toDelete가 모두 비어있을 때 userId가 null이 되는 문제가 해결되었습니다.
11-17: isDisabled 플래그 로직이 올바르게 수정되었습니다.이전 리뷰에서 지적된 isDisabled 플래그 반전 이슈가 해결되었습니다:
toCreate(새로 비활성화) →isDisabled=true✓toDelete(재활성화) →isDisabled=false✓
#️⃣ 연관된 이슈
closes #375
📝작업 내용
작업 상세 내용
💬리뷰 요구사항
Summary by CodeRabbit
릴리스 노트
새 기능
테스트
✏️ Tip: You can customize this high-level summary in your review settings.