[FEAT] 선택한 경로로 출발하기 및 이동 중 관련 기능 구현 #58
Conversation
WalkthroughThis pull request updates the backend by expanding interceptor path patterns, modifying error code definitions, and introducing several new features. It adds REST controllers and services for managing plans that include creating, retrieving, and deleting operations. Domain models are updated by removing and adding fields, and new DTOs are introduced to better structure request and response data. Additionally, custom repository interfaces and exception classes are added to support new business logic, and modifications in the todo entity improve data consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant PC as PlanController
participant PS as PlanService
participant PR as Repository
participant DM as Domain Model
PC->>PS: savePlan(memberId, PlanSaveRequest)
PS->>PR: Validate member and persist plan data
PR-->>PS: Persisted plan and paths
PS->>DM: Create plan using factory methods (createPlan & createPath)
PS-->>PC: Return response (201 Created)
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
backend/src/main/java/com/dubu/backend/plan/infra/repository/CustomPathRepositoryImpl.java (1)
19-25: LGTM! Well-implemented QueryDSL query with fetch join.The implementation effectively uses QueryDSL to solve the N+1 problem by using fetch join.
Consider these improvements for handling large datasets:
- Add pagination support to prevent memory issues with large result sets
- Consider using
distinctto prevent cartesian product issues with the collection fetch join- For very large datasets, you might want to batch the fetch join to avoid the cartesian product problem:
public List<Path> findByPlanWithTodosOrderByPathOrder(Plan plan) { List<Path> paths = queryFactory .selectFrom(path) .where(path.plan.eq(plan)) .orderBy(path.pathOrder.asc()) .fetch(); return queryFactory .selectFrom(path) .leftJoin(path.todos, todo).fetchJoin() .where(path.in(paths)) .orderBy(path.pathOrder.asc()) .fetch(); }backend/src/main/java/com/dubu/backend/plan/application/RouteService.java (3)
35-35: Consider enhancing error handling for the API response.While the code changes look good, consider adding error handling for potential null values in the API response, especially when accessing nested objects like
apiPath.info().Add null checks or use Optional to handle potential null values:
private RouteSearchResponse convertApiPathToRoute(OdsayRouteApiResponse.Path apiPath, boolean isRecentlyUsed) { - OdsayRouteApiResponse.Info info = apiPath.info(); + OdsayRouteApiResponse.Info info = Optional.ofNullable(apiPath.info()) + .orElseThrow(() -> new IllegalArgumentException("Route info cannot be null")); int totalTime = info.totalTime(); int totalSectionTime = 0;Also applies to: 43-43, 46-46, 112-131
95-97: Replace magic numbers with constants for traffic types.Using magic numbers (1, 2, 3) for traffic types reduces code readability and maintainability. Consider creating an enum for traffic types.
Create a TrafficType enum:
public enum TrafficType { SUBWAY(1), BUS(2), WALK(3), UNKNOWN(-1); private final int code; TrafficType(int code) { this.code = code; } public static TrafficType fromCode(int code) { return Arrays.stream(values()) .filter(type -> type.code == code) .findFirst() .orElse(UNKNOWN); } }Then update the code:
- int tType = subPath.trafficType(); - String trafficType = switch (tType) { - case 1 -> "SUBWAY"; - case 2 -> "BUS"; - case 3 -> "WALK"; - default -> "UNKNOWN"; - }; + TrafficType trafficType = TrafficType.fromCode(subPath.trafficType()); + String trafficTypeStr = trafficType.name();Also applies to: 135-140
149-162: Simplify nested conditions using early returns.The nested conditions in
convertApiSubPathToPathDtocan be simplified for better readability.Consider refactoring to:
- if (subPath.lane() != null && !subPath.lane().isEmpty()) { - OdsayRouteApiResponse.Lane lane = subPath.lane().get(0); - if ("SUBWAY".equals(trafficType)) { - subwayName = lane.name(); - subwayCode = lane.subwayCode(); - startName = subPath.startName() + "역"; - endName = subPath.endName() + "역"; - } else if ("BUS".equals(trafficType)) { - busNumber = lane.busNo(); - busCode = lane.busID(); - startName = subPath.startName(); - endName = subPath.endName(); - } - } + if (subPath.lane() == null || subPath.lane().isEmpty()) { + return buildPath(trafficType, subPath, null, null, null, null, null, null); + } + + OdsayRouteApiResponse.Lane lane = subPath.lane().get(0); + if ("SUBWAY".equals(trafficType)) { + return buildPath( + trafficType, + subPath, + lane.name(), + lane.subwayCode(), + null, + null, + subPath.startName() + "역", + subPath.endName() + "역" + ); + } + + if ("BUS".equals(trafficType)) { + return buildPath( + trafficType, + subPath, + null, + null, + lane.busNo(), + lane.busID(), + subPath.startName(), + subPath.endName() + ); + }Add a helper method:
private RouteSearchResponse.Path buildPath( String trafficType, OdsayRouteApiResponse.SubPath subPath, String subwayName, Integer subwayCode, String busNumber, Integer busCode, String startName, String endName ) { return new RouteSearchResponse.Path( trafficType, subPath.sectionTime(), subwayName, subwayCode, busNumber, busCode, startName, endName ); }backend/src/main/java/com/dubu/backend/plan/dto/request/PlanSaveRequest.java (1)
5-16: Consider adding input validation for plan fields.
Though Java records are concise, you might want to ensure that values like 'totalSectionTime' or 'sectionTime' are non-null and non-negative. Bean Validation (e.g., @NotNull, @positive) can help gracefully handle invalid requests before they reach your service layer.backend/src/main/java/com/dubu/backend/plan/exception/PlanNotFoundException.java (1)
8-16: Include details about the missing plan for easier troubleshooting.
Currently, the exception only returns a generic message. Consider adding a constructor that accepts the missing plan’s ID for better clarity in logs and user feedback.Example:
public class PlanNotFoundException extends NotFoundException { - public PlanNotFoundException() { + public PlanNotFoundException(Long planId) { super(PLAN_NOT_FOUND.getMessage() + " Plan ID: " + planId); } ... }backend/src/main/java/com/dubu/backend/todo/entity/TodoType.java (1)
4-4: LGTM! Consider adding documentation for the new state.The addition of
IN_PROGRESSstate aligns well with the travel functionality requirements. Consider adding Javadoc comments to document the purpose and usage of each enum value, especially the newIN_PROGRESSstate.public enum TodoType { + /** Task scheduled for execution */ SCHEDULED, + /** Task saved for later */ SAVE, + /** Recommended task */ RECOMMEND, + /** Task currently in progress during travel */ IN_PROGRESS, + /** Completed task */ DONE;Also applies to: 11-11
backend/src/main/java/com/dubu/backend/plan/domain/enums/TrafficType.java (1)
10-15: Add null check to prevent NullPointerException.The implementation looks good, but consider adding a null check to prevent NullPointerException.
public static TrafficType from(String value) { + if (value == null) { + throw new InvalidTrafficTypeException("null"); + } return Arrays.stream(TrafficType.values()) .filter(type -> type.name().equalsIgnoreCase(value)) .findFirst() .orElseThrow(() -> new InvalidTrafficTypeException(value)); }backend/src/main/java/com/dubu/backend/plan/exception/InvalidMemberStatusException.java (1)
7-16: Add serialVersionUID for serializable exception.The exception implementation looks good. Consider adding serialVersionUID as it extends Exception (which is Serializable).
public class InvalidMemberStatusException extends BadRequestException { + private static final long serialVersionUID = 1L; + public InvalidMemberStatusException(String memberStatus) { super(INVALID_MEMBER_STATUS.getMessage().formatted(memberStatus)); }backend/src/main/java/com/dubu/backend/plan/api/PlanController.java (2)
17-24: Add request validation.Consider adding
@Validannotation to validate the request body and define validation constraints inPlanSaveRequest.
35-42: Use path variable for planId.Consider using
@PathVariableinstead of@RequestParamforplanIdas it represents a resource identifier:- @DeleteMapping - public void deletePlan( - @RequestAttribute("memberId") Long memberId, - @RequestParam("planId") Long planId - ){ + @DeleteMapping("/{planId}") + public void deletePlan( + @RequestAttribute("memberId") Long memberId, + @PathVariable Long planId + ){backend/src/main/java/com/dubu/backend/todo/entity/Todo.java (1)
41-43: Consider adding cascade type for Path relationship.The
@ManyToOnerelationship with Path should specify a cascade type to ensure proper relationship management.Apply this diff to add cascade type:
- @ManyToOne(fetch = FetchType.LAZY) + @ManyToOne(fetch = FetchType.LAZY, cascade = CascadeType.PERSIST)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/src/main/java/com/dubu/backend/global/config/WebConfig.java(1 hunks)backend/src/main/java/com/dubu/backend/global/exception/ErrorCode.java(2 hunks)backend/src/main/java/com/dubu/backend/global/interceptor/TokenInterceptor.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/api/PlanController.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/api/RouteController.java(2 hunks)backend/src/main/java/com/dubu/backend/plan/application/PlanService.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/application/RouteService.java(4 hunks)backend/src/main/java/com/dubu/backend/plan/domain/Path.java(3 hunks)backend/src/main/java/com/dubu/backend/plan/domain/Plan.java(2 hunks)backend/src/main/java/com/dubu/backend/plan/domain/enums/CongestionLevel.java(0 hunks)backend/src/main/java/com/dubu/backend/plan/domain/enums/TrafficType.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/dto/request/PlanSaveRequest.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/dto/response/OdsayRouteApiResponse.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/dto/response/PlanRecentResponse.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/dto/response/RouteSearchResponse.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/exception/InvalidMemberStatusException.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/exception/InvalidTrafficTypeException.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/exception/PlanNotFoundException.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/infra/client/OdsayApiClient.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/infra/repository/CustomPathRepository.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/infra/repository/CustomPathRepositoryImpl.java(1 hunks)backend/src/main/java/com/dubu/backend/plan/infra/repository/PathRepository.java(1 hunks)backend/src/main/java/com/dubu/backend/todo/entity/Todo.java(3 hunks)backend/src/main/java/com/dubu/backend/todo/entity/TodoType.java(1 hunks)
💤 Files with no reviewable changes (1)
- backend/src/main/java/com/dubu/backend/plan/domain/enums/CongestionLevel.java
✅ Files skipped from review due to trivial changes (4)
- backend/src/main/java/com/dubu/backend/global/interceptor/TokenInterceptor.java
- backend/src/main/java/com/dubu/backend/plan/dto/response/OdsayRouteApiResponse.java
- backend/src/main/java/com/dubu/backend/plan/dto/response/RouteSearchResponse.java
- backend/src/main/java/com/dubu/backend/plan/infra/client/OdsayApiClient.java
🔇 Additional comments (14)
backend/src/main/java/com/dubu/backend/plan/infra/repository/CustomPathRepository.java (1)
8-10: LGTM! Well-structured custom repository interface.The interface follows Spring Data JPA best practices with a clear, descriptive method name that indicates its functionality.
backend/src/main/java/com/dubu/backend/plan/infra/repository/PathRepository.java (1)
8-8: LGTM! Good use of repository composition.The interface properly extends both JpaRepository and CustomPathRepository, following Spring Data JPA best practices for combining standard and custom repository functionality.
backend/src/main/java/com/dubu/backend/plan/application/RouteService.java (1)
8-9: LGTM! Good use of read-only transactions.The
@Transactional(readOnly = true)annotation is appropriately used for this service class as it primarily contains search operations. This helps optimize performance by avoiding unnecessary transaction overhead.Also applies to: 15-15, 21-21
backend/src/main/java/com/dubu/backend/plan/application/PlanService.java (2)
43-45: Clarify the logic behind restricting plan creation to STATUS.STOP.
Currently, the check only permits plan creation if the member’s status is STOP. This might be inverted depending on your business requirements (e.g., you may want to prohibit plan creation if the member is STOP). Please verify that this condition accurately reflects the intended business rule.Would you like a verification script to review related code handling the STOP status and confirm consistency across the codebase?
62-69: Handle potential zero-length paths to avoid IndexOutOfBounds exceptions.
When assigning paths via round-robin, calling 'paths.get(i % paths.size())' will throw an exception if 'paths' is empty. Consider validating that 'paths' is not empty before iterating through 'existingTodos'.backend/src/main/java/com/dubu/backend/plan/exception/InvalidTrafficTypeException.java (1)
7-16: LGTM! Well-structured exception handling.The exception class is well-implemented with proper message formatting and error code handling.
backend/src/main/java/com/dubu/backend/plan/domain/Plan.java (2)
26-27: LGTM! Well-defined JPA relationship.The one-to-many relationship with
Pathentities is properly configured with appropriate cascade and orphan removal settings.
32-37: LGTM! Well-structured factory method.The static factory method follows best practices by:
- Using the builder pattern
- Encapsulating object creation logic
- Having clear parameter names
backend/src/main/java/com/dubu/backend/global/config/WebConfig.java (1)
41-41: LGTM! Proper security configuration.The path pattern for plans is correctly added to the token interceptor configuration.
backend/src/main/java/com/dubu/backend/plan/domain/Path.java (3)
31-32: LGTM! Well-defined JPA relationship.The one-to-many relationship with
Todoentities is properly configured with appropriate cascade and orphan removal settings.
50-59: LGTM! Well-structured factory method.The static factory method follows best practices by:
- Using the builder pattern
- Properly mapping request data to entity fields
- Having clear parameter names
1-60: Verify impact of removing congestionLevel field.The congestionLevel field has been removed. Please ensure that:
- All references to this field in the codebase are updated
- Any frontend code depending on this field is modified
- Database migration scripts are created
backend/src/main/java/com/dubu/backend/global/exception/ErrorCode.java (1)
34-36: LGTM! Error codes align with new features.The new error codes are well-organized and provide clear error messages for the journey-related features.
Also applies to: 44-49
backend/src/main/java/com/dubu/backend/plan/api/RouteController.java (1)
6-6: LGTM! Type updates are consistent across the controller.The changes consistently update the response type from
RouteSearchResponseDtotoRouteSearchResponseacross imports, method signature, and implementation.Since this changes the API response type, please ensure that:
- The API documentation (if any) has been updated
- The clients consuming this endpoint have been notified of the change
Also applies to: 19-19, 26-26
| @Transactional | ||
| public void removePlan(Long memberId, Long planId) { | ||
| memberRepository.findById(memberId) | ||
| .orElseThrow(() -> new MemberNotFoundException(memberId)); | ||
|
|
||
| planRepository.deleteById(planId); | ||
| } |
There was a problem hiding this comment.
Enforce plan ownership checks when deleting a plan.
The current logic only verifies that the member exists but not that the plan belongs to the member. This could allow a member to delete another user’s plan unintentionally.
Below is an example code diff to ensure the plan is bound to the correct member:
@Transactional
public void removePlan(Long memberId, Long planId) {
memberRepository.findById(memberId)
.orElseThrow(() -> new MemberNotFoundException(memberId));
+ Plan plan = planRepository.findById(planId)
+ .orElseThrow(() -> new PlanNotFoundException());
+ if (!plan.getMember().getId().equals(memberId)) {
+ throw new PlanNotFoundException(); // or any relevant unauthorized exception
+ }
- planRepository.deleteById(planId);
+ planRepository.delete(plan);
}📝 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.
| @Transactional | |
| public void removePlan(Long memberId, Long planId) { | |
| memberRepository.findById(memberId) | |
| .orElseThrow(() -> new MemberNotFoundException(memberId)); | |
| planRepository.deleteById(planId); | |
| } | |
| @Transactional | |
| public void removePlan(Long memberId, Long planId) { | |
| memberRepository.findById(memberId) | |
| .orElseThrow(() -> new MemberNotFoundException(memberId)); | |
| Plan plan = planRepository.findById(planId) | |
| .orElseThrow(() -> new PlanNotFoundException()); | |
| if (!plan.getMember().getId().equals(memberId)) { | |
| throw new PlanNotFoundException(); // or any relevant unauthorized exception | |
| } | |
| planRepository.delete(plan); | |
| } |
| ) { | ||
| public static PathTodoResponse from(Todo todo) { | ||
| return new PathTodoResponse( | ||
| todo.getType() == TodoType.DONE, |
There was a problem hiding this comment.
Fix incorrect todo status check.
The current implementation assumes TodoType.DONE is the only state to check for completion status. This might not handle all possible todo states correctly.
Apply this diff to fix the todo status check:
- todo.getType() == TodoType.DONE,
+ todo.getType() == TodoType.DONE || todo.getType() == TodoType.IN_PROGRESS,📝 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.
| todo.getType() == TodoType.DONE, | |
| todo.getType() == TodoType.DONE || todo.getType() == TodoType.IN_PROGRESS, |
| path.getTrafficType() == TrafficType.SUBWAY ? path.getPlan().getId().intValue() : null, | ||
| path.getTrafficType() == TrafficType.BUS ? path.getStartName() : null, |
There was a problem hiding this comment.
Fix incorrect traffic type handling logic.
The current implementation has logical issues in handling traffic types:
- Using
plan.getId()as subway code is incorrect - Using
startNameas bus number is incorrect
Apply this diff to fix the traffic type handling:
- path.getTrafficType() == TrafficType.SUBWAY ? path.getPlan().getId().intValue() : null,
- path.getTrafficType() == TrafficType.BUS ? path.getStartName() : null,
+ path.getTrafficType() == TrafficType.SUBWAY ? path.getSubwayCode() : null,
+ path.getTrafficType() == TrafficType.BUS ? path.getBusNumber() : null,📝 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.
| path.getTrafficType() == TrafficType.SUBWAY ? path.getPlan().getId().intValue() : null, | |
| path.getTrafficType() == TrafficType.BUS ? path.getStartName() : null, | |
| path.getTrafficType() == TrafficType.SUBWAY ? path.getSubwayCode() : null, | |
| path.getTrafficType() == TrafficType.BUS ? path.getBusNumber() : null, |
| public static Todo copyOf(Todo originalTodo, Path assignedPath) { | ||
| return Todo.builder() | ||
| .title(originalTodo.getTitle()) | ||
| .type(TodoType.IN_PROGRESS) | ||
| .difficulty(originalTodo.getDifficulty()) | ||
| .memo(originalTodo.getMemo()) | ||
| .path(assignedPath) | ||
| .category(originalTodo.getCategory()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
Add member field initialization in copyOf method.
The copyOf method doesn't initialize the member field, which could lead to data inconsistency.
Apply this diff to fix member initialization:
public static Todo copyOf(Todo originalTodo, Path assignedPath) {
return Todo.builder()
.title(originalTodo.getTitle())
.type(TodoType.IN_PROGRESS)
.difficulty(originalTodo.getDifficulty())
.memo(originalTodo.getMemo())
.path(assignedPath)
.category(originalTodo.getCategory())
+ .member(originalTodo.getMember())
.build();
}
|
|
||
| newTodos.add(clonedTodo); | ||
| } | ||
| todoRepository.saveAll(newTodos); |
There was a problem hiding this comment.
todo 를 하나씩 저장하는 것이 아닌 한 번에 저장하는 것이 좋았습니다!!
| memberRepository.findById(memberId) | ||
| .orElseThrow(() -> new MemberNotFoundException(memberId)); | ||
|
|
||
| planRepository.deleteById(planId); |
There was a problem hiding this comment.
deleteById 로 해당 plan 의 존재 여부를 확인하지 않고 바로 삭제하는 것이 좋네요!! 배워갑니다!!
Issue Number
#53
As-Is
출발하기 및 이동 중 관련 기능을 구현합니다.
To-Be
Check List
Test Screenshot
(Optional) Additional Description
Summary by CodeRabbit