Fix/사용하기 힘든 api 보완#25
Hidden character warning
Conversation
There was a problem hiding this comment.
Pull request overview
이 PR은 API 사용성을 개선하기 위해 V0 엔드포인트를 Deprecated 처리하고, V1 API에 의존성 정보와 작업 정보를 추가하여 응답을 보강합니다. 또한 의존성 서비스의 사이클 검증 방식을 개선했습니다.
Changes:
- V0 엔드포인트(Statistics, Member) Deprecated 처리 및 V1 버전 생성
- 작업 응답에 선행/후행 작업 ID 목록(
previousTaskIds,nextTaskIds) 추가 - 일정 응답에 연결된 작업 정보(
taskId,taskCompleted) 추가 DependencyService.checkCycle()메서드를assertNoCycle()로 변경하여 사이클 발견 시 예외 발생
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| TaskControllerV1.java | DependencyService 주입 및 작업 조회 시 의존성 정보 포함 |
| TaskResponse.java | 선행/후행 작업 ID 필드 추가 |
| ScheduleControllerV1.java | 주간 일정 조회 엔드포인트 추가 및 작업 정보 포함 |
| ScheduleSimpleResponse.java | 연결된 작업 ID 및 완료 여부 필드 추가 |
| StatisticsControllerV1.java | V1 통계 컨트롤러 신규 생성 |
| StatisticsControllerV0.java | Deprecated 마크 추가 |
| MemberControllerV1.java | V1 회원 컨트롤러 신규 생성 |
| MemberControllerV0.java | Deprecated 마크 추가 |
| TaskService.java | 커서 기반 페이지네이션에 의존성 정보 추가 |
| TaskAdjustmentService.java | 작업 생성/수정 시 사이클 검증 메서드명 변경 및 로직 순서 조정 |
| TaskDependencyAdjustCommand.java | null/0L을 selfId로 해결하는 로직 추가 |
| ScheduleAdjustmentService.java | 사이클 검증 메서드명 변경 |
| DependencyService.java | checkCycle → assertNoCycle 변경 및 의존성 조회 메서드 추가 |
| DependencyServiceTest.java | 예외 기반 검증으로 테스트 변경 |
| TaskAdjustmentServiceTest.java | 사이클 검증 메서드명 및 순서 변경 반영 |
| ScheduleAdjustmentServiceTest.java | 사이클 검증 메서드명 변경 반영 |
| TaskControllerV1Test.java | DependencyService 모킹 추가 |
| ScheduleControllerV1Test.java | TaskService 모킹 및 생성자 파라미터 추가 |
| README.md | V1 API 설명 업데이트 |
| Map<Long, Task> taskMap = taskService.findTasksByIds(memberId, schedules.stream() | ||
| .map(Schedule::getTaskId) | ||
| .filter(id -> id != null) | ||
| .toList()) | ||
| .stream() | ||
| .collect(Collectors.toMap(Task::getId, Function.identity())); |
There was a problem hiding this comment.
문제점: getSchedules와 getWeeklySchedules 메서드에서 Task 정보를 조회하는 로직이 완전히 동일하게 중복되어 있습니다 (라인 65-70, 82-87).
영향: 코드 중복으로 인해 유지보수성이 저하되며, 향후 로직 변경 시 두 곳을 모두 수정해야 합니다.
수정 제안: Task 정보를 조회하는 로직을 별도의 private 메서드로 추출하여 재사용하세요. 예를 들어, buildTaskMap(List<Schedule> schedules) 같은 메서드를 만들어 중복을 제거할 수 있습니다.
| return taskService.getTasks(memberId, pageable, readyOnly) | ||
| .map(TaskResponse::from); | ||
| Page<Task> tasks = taskService.getTasks(memberId, pageable, readyOnly); | ||
| var dependencyMap = dependencyService.getDependencyInfoForTasks(memberId, tasks.getContent().stream().map(Task::getId).toList()); |
There was a problem hiding this comment.
문제점: N+1 쿼리 문제가 발생할 수 있습니다. getDependencyInfoForTasks 메서드는 모든 의존성을 한 번에 조회하지만, 각 작업마다 map.get(task.getId())를 호출하여 매핑하고 있습니다. 또한 dependencyRepository.findAllByOwnerId(memberId)가 모든 Task의 의존성 정보 조회를 위해 매번 호출됩니다.
영향: 작업이 많을 경우 성능 저하가 발생할 수 있으며, 동일한 데이터를 반복적으로 조회하게 됩니다.
수정 제안: getDependencyInfoForTasks 메서드가 이미 모든 의존성을 한 번에 가져오므로 문제가 없지만, 메서드 호출 전에 빈 리스트를 확인하여 불필요한 데이터베이스 쿼리를 방지하세요. 예를 들어, tasks.getContent().isEmpty()를 체크하여 빈 경우 빈 맵을 반환하도록 최적화할 수 있습니다.
| var dependencyMap = dependencyService.getDependencyInfoForTasks(memberId, tasks.getContent().stream().map(Task::getId).toList()); | |
| if (tasks.isEmpty()) { | |
| // 작업이 없으면 불필요한 의존성 조회를 수행하지 않는다. | |
| return tasks.map(task -> TaskResponse.from(task, null)); | |
| } | |
| var dependencyMap = dependencyService.getDependencyInfoForTasks( | |
| memberId, | |
| tasks.getContent().stream().map(Task::getId).toList() | |
| ); |
| Task saved = taskService.createTask(command.buildTask()); | ||
| List<Dependency> addedDependencies = command.getAddDependencies(saved.getId()); | ||
| dependencyService.assertNoCycle(memberId, List.of(), addedDependencies); | ||
| dependencyService.saveAll(addedDependencies); | ||
| return saved; |
There was a problem hiding this comment.
문제점: 작업 생성 후에 사이클 검증을 수행하고 있습니다 (라인 21-23). 사이클이 발견되면 assertNoCycle에서 예외가 발생하지만, 이미 작업은 데이터베이스에 저장된 상태입니다.
영향: 트랜잭션이 롤백되므로 데이터 일관성 문제는 없지만, 불필요한 데이터베이스 작업이 발생하고 성능이 저하될 수 있습니다. 또한 ID 생성 전략에 따라 시퀀스 값이 소모될 수 있습니다.
수정 제안: 가능하다면 사이클 검증을 작업 생성 전에 수행하는 것이 좋습니다. 만약 selfId가 필요하다면, 의존성에서 null/0L을 사용하는 대신 사이클 검증 로직에서 임시 ID를 사용하여 검증하는 방법을 고려하세요.
| return new ScheduleSimpleResponse( | ||
| schedule.getId(), | ||
| schedule.getOwnerId(), | ||
| schedule.getTaskId(), | ||
| task == null ? null : task.isCompleted(), |
There was a problem hiding this comment.
문제점: taskCompleted 필드가 taskId가 null이 아니지만 task 객체가 null인 경우를 처리하지 못합니다. 라인 47에서 task == null ? null : task.isCompleted()로 처리하고 있지만, schedule.getTaskId()가 존재하는데 task가 null인 경우 불일치가 발생할 수 있습니다.
영향: API 응답에서 taskId는 있지만 taskCompleted가 null인 예상치 못한 상태가 발생할 수 있어, 클라이언트에서 혼란을 야기할 수 있습니다.
수정 제안: schedule.getTaskId()가 null이 아닌 경우 task 객체도 반드시 존재해야 한다는 것을 보장하거나, 그렇지 않은 경우 명확한 에러 처리를 추가하세요.
| return new ScheduleSimpleResponse( | |
| schedule.getId(), | |
| schedule.getOwnerId(), | |
| schedule.getTaskId(), | |
| task == null ? null : task.isCompleted(), | |
| if (schedule.getTaskId() != null && task == null) { | |
| throw new IllegalArgumentException( | |
| "Task must not be null when schedule has a non-null taskId. scheduleId=" + schedule.getId() | |
| ); | |
| } | |
| Boolean taskCompleted = (task == null) ? null : task.isCompleted(); | |
| return new ScheduleSimpleResponse( | |
| schedule.getId(), | |
| schedule.getOwnerId(), | |
| schedule.getTaskId(), | |
| taskCompleted, |
| public Map<Long, TaskDependencyInfo> getDependencyInfoForTasks(Long memberId, List<Long> taskIds) { | ||
| List<Dependency> allByOwnerId = dependencyRepository.findAllByOwnerId(memberId); | ||
| Graph graph = Graph.of(allByOwnerId); | ||
| return taskIds.stream() | ||
| .collect(Collectors.toMap( | ||
| id -> id, | ||
| id -> new TaskDependencyInfo( | ||
| graph.getPreviousTaskIds(id), | ||
| graph.getNextTaskIds(id) | ||
| ) | ||
| )); | ||
| } |
There was a problem hiding this comment.
문제점: getDependencyInfoForTasks 메서드는 빈 taskIds 리스트를 받아도 여전히 데이터베이스에서 모든 의존성을 조회합니다 (라인 88).
영향: 조회할 작업이 없을 때도 불필요한 데이터베이스 쿼리가 발생하여 성능이 저하됩니다.
수정 제안: 메서드 시작 부분에 빈 리스트 체크를 추가하여 조기 반환하세요. 예: if (taskIds.isEmpty()) return Collections.emptyMap();
| return from(task, null); | ||
| } | ||
|
|
||
| public static TaskResponse from(Task task, me.gg.pinit.pinittask.application.dependency.service.DependencyService.TaskDependencyInfo dependencyInfo) { |
There was a problem hiding this comment.
문제점: 라인 44의 메서드 시그니처에서 매개변수 타입이 완전한 패키지 경로 (FQCN)로 작성되어 있습니다: me.gg.pinit.pinittask.application.dependency.service.DependencyService.TaskDependencyInfo
영향: 코드 가독성이 떨어지며, 메서드 시그니처가 불필요하게 길어집니다.
수정 제안: 파일 상단에 import me.gg.pinit.pinittask.application.dependency.service.DependencyService.TaskDependencyInfo; 를 추가하고, 메서드 시그니처에서는 짧은 이름 TaskDependencyInfo만 사용하세요.
| if (rawId == 0L) { | ||
| return selfId; | ||
| } |
There was a problem hiding this comment.
문제점: 라인 79-86에서 ID 해결 로직이 null과 0L을 모두 selfId로 변환하고 있습니다. 이는 의도는 이해되지만, null과 0L이라는 두 가지 특별한 값을 사용하는 것이 혼란을 야기할 수 있습니다.
영향: API 사용자가 null과 0L 중 어느 것을 사용해야 하는지 명확하지 않아 일관성 없는 사용이 발생할 수 있습니다.
수정 제안: 하나의 특별한 값만 사용하도록 통일하거나, 문서화를 통해 두 값의 사용 시나리오를 명확히 구분하세요. 일반적으로 null만 사용하는 것이 더 명확합니다.
| if (rawId == 0L) { | |
| return selfId; | |
| } |
|
|
||
| * **V0**: 기존 호환용. 일정 생성/수정 시 Task를 함께 생성하거나 수정하며, 요청/응답에 작업 속성(마감일, 중요도/난이도[Fibonacci: 1,2,3,5,8,13,21])이 포함됩니다. | ||
| * **V1**: 분리 권장. 일정 단독 CRUD/상태 전환만 다루며 Task 정보를 노출하지 않습니다. | ||
| * **V1**: 분리 권장. 일정 단독 CRUD/상태 전환만 다루며 Task 정보를 노출하지 않습니다. 추가로 다음 엔드포인트를 제공합니다. |
There was a problem hiding this comment.
문제점: README 라인 78에서 "일정 단독 CRUD/상태 전환만 다루며 Task 정보를 노출하지 않습니다"라고 설명하고 있지만, 실제로는 이번 PR에서 V1 일정 응답에 Task 정보(taskId, taskCompleted)를 추가했습니다.
영향: 문서와 실제 구현 간의 불일치로 인해 API 사용자가 혼란을 겪을 수 있습니다.
수정 제안: V1의 설명을 수정하여 "일정 응답에 연결된 작업의 기본 정보(ID, 완료 여부)를 포함합니다" 등으로 실제 동작을 반영하세요.
| * **V1**: 분리 권장. 일정 단독 CRUD/상태 전환만 다루며 Task 정보를 노출하지 않습니다. 추가로 다음 엔드포인트를 제공합니다. | |
| * **V1**: 분리 권장. 일정 단독 CRUD/상태 전환을 중심으로 하되, 일정 응답에 연결된 작업의 기본 정보(ID: `taskId`, 완료 여부: `taskCompleted`)를 포함합니다. 추가로 다음 엔드포인트를 제공합니다. |
| package me.gg.pinit.pinittask.interfaces.web; | ||
|
|
||
| import io.swagger.v3.oas.annotations.Operation; | ||
| import io.swagger.v3.oas.annotations.Parameter; | ||
| import io.swagger.v3.oas.annotations.media.Content; | ||
| import io.swagger.v3.oas.annotations.media.Schema; | ||
| import io.swagger.v3.oas.annotations.responses.ApiResponse; | ||
| import io.swagger.v3.oas.annotations.responses.ApiResponses; | ||
| import io.swagger.v3.oas.annotations.tags.Tag; | ||
| import lombok.RequiredArgsConstructor; | ||
| import me.gg.pinit.pinittask.application.member.service.MemberService; | ||
| import me.gg.pinit.pinittask.interfaces.utils.MemberId; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
| import org.springframework.web.bind.annotation.RestController; | ||
|
|
||
| @RestController | ||
| @RequestMapping("/v1/members") | ||
| @Tag(name = "MemberV1", description = "회원 관련 정보 API (v1)") | ||
| @ApiResponses({ | ||
| @ApiResponse(responseCode = "400", description = "잘못된 요청", content = @Content(schema = @Schema(implementation = String.class))), | ||
| @ApiResponse(responseCode = "404", description = "대상을 찾을 수 없습니다.", content = @Content(schema = @Schema(implementation = String.class))), | ||
| @ApiResponse(responseCode = "500", description = "서버 내부 오류", content = @Content(schema = @Schema(implementation = String.class))) | ||
| }) | ||
| @RequiredArgsConstructor | ||
| public class MemberControllerV1 { | ||
|
|
||
| private final MemberService memberService; | ||
|
|
||
| @GetMapping("/now") | ||
| @Operation(summary = "현재 진행 중인 일정 ID 조회", description = "사용자의 현재 진행 중인 일정 ID를 조회합니다.") | ||
| public ResponseEntity<Long> getNowInProgressScheduleId(@Parameter(hidden = true) @MemberId Long memberId) { | ||
| Long scheduleId = memberService.getNowInProgressScheduleId(memberId); | ||
| return ResponseEntity.ok(scheduleId); | ||
| } | ||
|
|
||
| @GetMapping("/zone-offset") | ||
| @Operation(summary = "사용자 시간대 조회", description = "사용자의 시간대를 조회합니다.") | ||
| @ApiResponses({ | ||
| @ApiResponse(responseCode = "200", description = "사용자 시간대 조회 성공", content = @Content(schema = @Schema(implementation = String.class, example = "+09:00"))), | ||
| }) | ||
| public ResponseEntity<String> getMemberZoneOffset(@Parameter(hidden = true) @MemberId Long memberId) { | ||
| String zoneOffset = memberService.findZoneOffsetOfMember(memberId).toString(); | ||
| return ResponseEntity.ok(zoneOffset); | ||
| } | ||
| } |
There was a problem hiding this comment.
문제점: 새로 추가된 MemberControllerV1 클래스에 대한 테스트 커버리지가 없습니다. 다른 V1 컨트롤러들은 모두 단위 테스트 및 통합 테스트가 존재합니다.
영향: 회원 관련 API의 동작을 검증하지 못하여, 향후 리팩토링이나 변경 시 회귀 버그가 발생할 위험이 있습니다.
수정 제안: MemberControllerV1Test 및 MemberControllerV1IntegrationTest를 추가하여 회원 관련 엔드포인트의 동작을 검증하세요.
| @GetMapping("/week") | ||
| @Operation(summary = "주간 일정 조회 (작업 없이)", description = "주어진 날짜가 포함된 주간의 일정을 조회합니다.") | ||
| public List<ScheduleSimpleResponse> getWeeklySchedules(@Parameter(hidden = true) @MemberId Long memberId, | ||
| @RequestParam @DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME) LocalDateTime time, | ||
| @RequestParam ZoneId zoneId) { | ||
| List<Schedule> schedules = scheduleService.getScheduleListForWeek(memberId, dateTimeUtils.toZonedDateTime(time, zoneId)); | ||
| Map<Long, Task> taskMap = taskService.findTasksByIds(memberId, schedules.stream() | ||
| .map(Schedule::getTaskId) | ||
| .filter(id -> id != null) | ||
| .toList()) | ||
| .stream() | ||
| .collect(Collectors.toMap(Task::getId, Function.identity())); | ||
| return schedules.stream() | ||
| .map(schedule -> ScheduleSimpleResponse.from(schedule, taskMap.get(schedule.getTaskId()))) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
문제점: ScheduleControllerV1에 새로 추가된 getWeeklySchedules 엔드포인트에 대한 테스트가 없습니다. getSchedules 엔드포인트는 테스트되고 있지만 (라인 85-97), 주간 일정 조회는 테스트되지 않았습니다.
영향: 주간 일정 조회 기능의 동작을 검증하지 못하여, 향후 변경 시 회귀 버그가 발생할 위험이 있습니다.
수정 제안: getWeeklySchedules_returnsWeeklyScheduleList 같은 테스트 메서드를 추가하여 주간 일정 조회 기능을 검증하세요.
변경된 점