fix: Route의 dateSchedule 필드를 설정하지 않음#313
Conversation
- Route 엔티티 생성 시 dateSchedule 업데이트 로직 추가 - 상위 엔티티(DateSchedule)를 먼저 생성하고, 하위 엔티티(Route)가 이를 참조하도록 엔티티 생성 순서 변경
Summary of ChangesHello @lovelyAlien, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 Route 엔티티와 DateSchedule 엔티티 간의 관계 설정 문제를 해결합니다. 기존에는 Route 생성 시 DateSchedule 필드가 제대로 연결되지 않아 데이터 불일치가 발생할 수 있었습니다. 이를 개선하기 위해 엔티티 생성 순서를 조정하고, Route 엔티티가 DateSchedule을 명시적으로 참조하도록 로직을 변경하여 데이터 무결성을 강화하고 코드의 구조를 개선했습니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Code Review
이 Pull Request는 Route 엔티티 생성 시 dateSchedule 필드가 설정되지 않던 버그를 수정합니다. 엔티티 생성 순서를 변경하여 DateSchedule을 먼저 생성하고 Route가 이를 참조하도록 한 점과, 장소 생성 로직을 별도 메서드로 분리하여 코드 가독성을 높인 점이 좋습니다. 또한, 이 과정에서 장소 이름이 잘못 설정되던 잠재적인 버그도 함께 수정되었습니다.
리뷰에서는 몇 가지 개선점을 제안했습니다. 첫째, 양방향 연관관계에서 객체 상태의 일관성을 유지하기 위한 방안을 DateSchedFacade와 Route 엔티티 양쪽에서 제안했습니다. 둘째, getOrCreatePlace 메서드에서 문자열을 숫자로 변환할 때 발생할 수 있는 NumberFormatException에 대한 방어적인 코딩을 제안하여 안정성을 높이도록 했습니다.
| DateSchedule dateSchedule = DateSchedule.builder() | ||
| .name(request.getName()) | ||
| .scheduleAt(LocalDate.parse(request.getScheduleAt())) | ||
| .user(user) | ||
| .build(); | ||
| dateSchedService.save(dateSchedule); | ||
|
|
||
| // 3. Route 생성 (DateSchedule 참조) | ||
| Route route = Route.builder() | ||
| .name(request.getName()) | ||
| .status(RouteStatus.WRITE) | ||
| .createdBy(user) | ||
| .routeType(request.getPlaces().size() == 1 ? RouteType.PLACE : RouteType.COURSE) | ||
| .routeType(places.size() == 1 ? RouteType.PLACE : RouteType.COURSE) | ||
| .dateSchedule(dateSchedule) | ||
| .build(); | ||
| routeService.saveRoute(route); |
There was a problem hiding this comment.
현재 로직은 데이터베이스 영속성 관점에서는 올바르게 동작하지만, 양방향 연관관계가 설정된 객체의 상태 일관성을 해칠 수 있는 잠재적인 위험이 있습니다.
dateSchedule과 route 객체를 각각 생성하고 저장한 후에, route 객체에는 dateSchedule을 설정했지만, dateSchedule 객체의 route 필드는 설정되지 않은 상태로 남아있게 됩니다. 이로 인해 해당 트랜잭션 내에서 dateSchedule.getRoute()를 호출하면 null이 반환되어 예상치 못한 동작을 유발할 수 있습니다.
양방향 연관관계에서는 연관관계의 양쪽 모두를 설정해주는 것이 좋습니다. DateSchedule 엔티티에 Route를 설정하는 편의 메서드(예: setRoute(Route route))를 추가하고, route 생성 후 이를 호출하여 객체 그래프의 일관성을 유지하는 것을 고려해보세요.
| .latitude(Double.valueOf(placeDto.getLatitude())) | ||
| .longitude(Double.valueOf(placeDto.getLongitude())) |
There was a problem hiding this comment.
위도와 경도 값을 String에서 Double로 변환할 때 Double.valueOf()를 직접 사용하면, 유효하지 않은 숫자 형식의 문자열이 들어올 경우 NumberFormatException이 발생하여 500 에러로 이어질 수 있습니다. validatePlace 메서드에서 이 값들의 형식을 검증하는 로직이 없다면, DTO 레벨에서 @Pattern과 같은 유효성 검사 어노테이션을 추가하거나, 이 메서드 내에서 try-catch 블록으로 예외를 처리하여 적절한 비즈니스 예외(예: 400 Bad Request)를 던지는 것을 고려해보세요.
| public void updateDateSchedule(DateSchedule dateSchedule) { | ||
| this.dateSchedule = dateSchedule; | ||
| } |
There was a problem hiding this comment.
양방향 연관관계를 설정할 때는 연관관계의 양쪽 모두를 업데이트하여 객체 상태의 일관성을 유지하는 것이 중요합니다. 현재 updateDateSchedule 메서드는 Route의 dateSchedule 필드만 설정하고, 반대쪽인 DateSchedule의 route 필드는 업데이트하지 않습니다.
일관성을 유지하기 위해 이 메서드 내에서 dateSchedule.setRoute(this)와 같이 반대쪽 연관관계도 함께 설정하는 로직을 추가하는 것을 권장합니다. 이를 위해서는 DateSchedule 엔티티에 route를 설정할 수 있는 setter나 편의 메서드가 필요할 수 있습니다.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #312 by fixing the Route entity's dateSchedule field initialization. The changes restructure entity creation order to create DateSchedule first, then Route, ensuring proper foreign key relationships. Additionally, a bug where Place names were incorrectly set has been fixed.
Changes:
- Added
updateDateSchedule()method to Route entity for managing the dateSchedule relationship - Refactored
createDateSchedule()method to create entities in the correct order: DateSchedule → Route → RoutePlace - Extracted place creation/retrieval logic into a helper method
getOrCreatePlace()to improve code organization
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/main/java/com/server/domain/route/entity/Route.java | Added updateDateSchedule() method to allow setting/updating the dateSchedule field after construction |
| src/main/java/com/server/domain/dateSchedule/service/DateSchedFacade.java | Restructured createDateSchedule() to create DateSchedule before Route, extracted getOrCreatePlace() helper method, and fixed Place.name to use placeDto.getName() instead of request.getName() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .routeType(places.size() == 1 ? RouteType.PLACE : RouteType.COURSE) | ||
| .dateSchedule(dateSchedule) | ||
| .build(); | ||
| routeService.saveRoute(route); |
There was a problem hiding this comment.
The bidirectional relationship between DateSchedule and Route is not being properly maintained. While Route.dateSchedule is set (line 75), the corresponding DateSchedule.route field is never set. This breaks the bidirectional OneToOne mapping.
Since DateSchedule has @OneToOne(mappedBy = "dateSchedule"), the Route entity owns the relationship. However, for proper bidirectional consistency, you should either:
- Call route.updateDateSchedule(dateSchedule) after saving the route, OR
- Add a method to DateSchedule to set the route field and call it after creating the route
Note that DateSchedule has factory methods like createWithNewRoute() that properly manage this bidirectional relationship.
| routeService.saveRoute(route); | |
| routeService.saveRoute(route); | |
| route.updateDateSchedule(dateSchedule); |
| public void createDateSchedule(User user, DateSchedCreateRequest request) { | ||
| // 1. Place 목록 준비 (조회 또는 생성) | ||
| List<Place> places = request.getPlaces().stream() | ||
| .map(placeDto -> getOrCreatePlace(user, placeDto)) | ||
| .toList(); | ||
|
|
||
| // 2. DateSchedule 생성 | ||
| DateSchedule dateSchedule = DateSchedule.builder() | ||
| .name(request.getName()) | ||
| .scheduleAt(LocalDate.parse(request.getScheduleAt())) | ||
| .user(user) | ||
| .build(); | ||
| dateSchedService.save(dateSchedule); | ||
|
|
||
| // 3. Route 생성 (DateSchedule 참조) | ||
| Route route = Route.builder() | ||
| .name(request.getName()) | ||
| .status(RouteStatus.WRITE) | ||
| .createdBy(user) | ||
| .routeType(request.getPlaces().size() == 1 ? RouteType.PLACE : RouteType.COURSE) | ||
| .routeType(places.size() == 1 ? RouteType.PLACE : RouteType.COURSE) | ||
| .dateSchedule(dateSchedule) | ||
| .build(); | ||
| routeService.saveRoute(route); | ||
|
|
||
| for (DateSchedPlaceDto placeDto : request.getPlaces()) { | ||
| Long placeId = placeDto.getPlaceId(); | ||
| Place place; | ||
| if (placeId != null) { | ||
| place = placeService.getPlaceById(placeId); | ||
| } else { | ||
| validatePlace(placeDto); | ||
|
|
||
| Place newPlace = Place.builder() | ||
| .name(request.getName()) | ||
| .region1depth(placeDto.getRegion1depth()) | ||
| .region2depth(placeDto.getRegion2depth()) | ||
| .region3depth(placeDto.getRegion3depth()) | ||
| .address(placeDto.getAddress()) | ||
| .latitude(Double.valueOf(placeDto.getLatitude())) | ||
| .longitude(Double.valueOf(placeDto.getLongitude())) | ||
| .score(0L) | ||
| .user(user) | ||
| .status(PlaceStatus.WRITE) | ||
| .build(); | ||
| place = placeService.save(newPlace); | ||
| } | ||
|
|
||
| // 4. RoutePlace 생성 | ||
| for (Place place : places) { | ||
| RoutePlace routePlace = new RoutePlace(route, place); | ||
| routeService.saveRoutePlace(routePlace); | ||
| } | ||
| } |
There was a problem hiding this comment.
The refactored createDateSchedule method lacks test coverage. While there is a commented-out test in DateScheduleControllerTest, the method's business logic - particularly the new entity creation order, bidirectional relationship handling, and the extracted getOrCreatePlace helper method - should be covered by unit or integration tests to prevent regressions.
| .region3depth(placeDto.getRegion3depth()) | ||
| .address(placeDto.getAddress()) | ||
| .latitude(Double.valueOf(placeDto.getLatitude())) | ||
| .longitude(Double.valueOf(placeDto.getLongitude())) |
There was a problem hiding this comment.
Potential uncaught 'java.lang.NumberFormatException'.
| .region2depth(placeDto.getRegion2depth()) | ||
| .region3depth(placeDto.getRegion3depth()) | ||
| .address(placeDto.getAddress()) | ||
| .latitude(Double.valueOf(placeDto.getLatitude())) |
There was a problem hiding this comment.
Potential uncaught 'java.lang.NumberFormatException'.
Features
Issue
resolves #312