Refactor/travel#224
Conversation
![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 refactors the travel itinerary management system into a UseCase-Command-Reader architecture, removes soft-delete logic, and implements Querydsl for repository queries. Feedback identifies missing validations for member existence and group membership, a date validation bug in entity setters, redundant database fetches, and inconsistent error handling.
| List<Long> memberIds = req.memberUuids().stream().map(userIdentityResolver::resolve).toList(); | ||
| List<User> members = userReader.findUsersInIds(memberIds).stream() | ||
| .filter(member -> !member.getId().equals(leader.getId())).toList(); |
There was a problem hiding this comment.
The refactored saveTravelItinerary logic has lost several important validations that were present in the original implementation:
- UUID Resolution: It does not check if
userIdentityResolver.resolve(uuid)returnsnull. This can lead tonullvalues being passed to the repository. - Member Existence: It no longer verifies that all requested members actually exist. The
UserReader.findUsersInIdsonly checks if the result is not empty, but doesn't ensure the count matches the input. - Group Membership: It no longer verifies that the invited members are actually joined in the group (
JoinStatus.JOINED).
These omissions could allow creating travel itineraries with non-existent users or users who are not part of the group.
| public void changeStartAt(final LocalDateTime startAt) { | ||
| if (startAt == null) return; | ||
| this.startAt = validateStartAt(startAt); | ||
| validateDateOrder(this.startAt, this.endAt); | ||
| } | ||
|
|
||
| private void updateEndAt(LocalDateTime endAt) { | ||
| public void changeEndAt(final LocalDateTime endAt) { | ||
| if (endAt == null) return; | ||
| this.endAt = validateEndAt(endAt); | ||
| validateDateOrder(this.startAt, this.endAt); | ||
| } |
There was a problem hiding this comment.
Moving validateDateOrder into the individual setters changeStartAt and changeEndAt introduces a bug when updating the travel period to a range that does not overlap with the current one. For example, if the current period is [Jan 1, Jan 5] and you try to update it to [Feb 1, Feb 5], calling changeStartAt(Feb 1) will throw an exception because it compares the new start date against the old end date (Jan 5).
In TravelCommand.updateTravel, changeStartAt is called before changeEndAt, making this failure inevitable for any forward shift in the schedule. It is recommended to remove the validation from the setters and either validate once after all changes are applied or provide a dedicated method to update the period (e.g., changePeriod(startAt, endAt)).
|
|
||
| public void updateTravel(final Long travelItineraryId, final String title, final LocalDateTime startAt, | ||
| final LocalDateTime endAt, final String description) { | ||
| TravelItinerary travel = travelReader.findTravel(travelItineraryId); |
There was a problem hiding this comment.
This method performs a redundant database fetch. The caller (TravelUseCase.updateTravel) has already performed authorization checks which likely involved fetching the same entity or its related mappings. Consider passing the TravelItinerary entity directly to this method to avoid unnecessary database round-trips.
| .orElseThrow(() -> new BusinessException(UserTravelItineraryErrorCode.USER_TRAVEL_ITINERARY_NOT_FOUND)); | ||
| } | ||
| return userTravelItineraryJpaRepository.findByUserIdAndTravelItineraryIdAndUserRole(userId, travelId, userRole) | ||
| .orElseThrow(() -> new BusinessException(TravelItineraryErrorCode.USER_TRAVEL_NOT_FOUND)); |
There was a problem hiding this comment.
Inconsistent error codes are used for the same logical failure (user not found in travel). Line 54 throws UserTravelItineraryErrorCode.USER_TRAVEL_ITINERARY_NOT_FOUND while line 57 throws TravelItineraryErrorCode.USER_TRAVEL_NOT_FOUND. It is recommended to use a single, consistent error code for these cases to improve maintainability.
| Long addUserId = resolveMemberUserIdOrThrow(req.userUuid()); | ||
| User addUser = userReader.findUserById(addUserId); | ||
|
|
||
| travelCommand.addUserTravelItinerary(addUser, travelItinerary, UserRole.MEMBER); |
There was a problem hiding this comment.
The addTravelUser method is missing a check to ensure that the user being added (addUser) is a member of the group associated with the travel itinerary. This could allow unauthorized users to be added to private group travels.
| Optional<TravelItinerary> findByIdAndGroupIdAndIsDeletedFalseForUpdate(Long travelItineraryId, Long groupId); | ||
|
|
||
| @Query(""" | ||
| SELECT t FROM TravelItinerary t | ||
| WHERE t.group.id = :groupId | ||
| AND t.isDeleted = false | ||
| ORDER BY t.id DESC | ||
| """) | ||
| List<TravelItinerary> findGroupTravelsFirstPage(@Param("groupId") Long groupId, Pageable pageable); | ||
|
|
||
| @Query(""" | ||
| SELECT t FROM TravelItinerary t | ||
| WHERE t.group.id = :groupId | ||
| AND t.isDeleted = false | ||
| AND t.id < :cursor | ||
| ORDER BY t.id DESC | ||
| """) | ||
| List<TravelItinerary> findGroupTravelsNextPage( | ||
| @Param("groupId") Long groupId, | ||
| @Param("cursor") Long cursor, | ||
| Pageable pageable | ||
| ); | ||
|
|
||
| @Lock(LockModeType.PESSIMISTIC_WRITE) | ||
| @Query("select t from TravelItinerary t where t.id = :travelId and t.isDeleted = false") | ||
| @Query("select t from TravelItinerary t where t.id = :travelId") | ||
| Optional<TravelItinerary> findByIdAndIsDeletedFalseForUpdate(Long travelId); |
There was a problem hiding this comment.
The method names findByIdAndGroupIdAndIsDeletedFalseForUpdate and findByIdAndIsDeletedFalseForUpdate are now misleading because the isDeleted field has been removed from the entity and the corresponding @Query no longer filters by it. These methods should be renamed to reflect the current implementation (e.g., findByIdAndGroupIdForUpdate).
| if(users.isEmpty()) { | ||
| throw new BusinessException(UserErrorCode.USER_NOT_FOUND); | ||
| } |
There was a problem hiding this comment.
Checking users.isEmpty() is insufficient when findAllById is used. If some IDs in the input list are valid and others are not, JPA will return only the valid ones, and this check will pass. To ensure all requested users were found, you should compare the size of the result list with the number of unique, non-null IDs provided in the input.
chungjeongsu
force-pushed
the
refactor/travel
branch
from
76529f1 to
0d9ca7c
Compare
2 participants
No description provided.