-
Notifications
You must be signed in to change notification settings - Fork 78
[Spring Data JPA] 홍석주 미션 제출합니다. #152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: somefood
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
석주님 미션을 수행하시느라 고생 많으셨습니다.
pr에 남기신 요청에 대해서 이야기를 해보고자 합니다.
제가 4, 5단계는 조회 시 JpaRepository를 활용해서 조회를 활용했는데,
6단계는 네이티브 쿼리가 필요할거 같아 JdbcTemplate을 활용했습니다.
jpa에서도 네이티브 쿼리를 이용할 수 있는 것 같은데 jdbcTemplate을 통해서 네이티브 쿼리를 이용하신 이유는 무엇인가요?
같이 얘기 나누고 싶은 사항으론 제가 Waiting Entity에 대해서는 다른 Entity와 달리 연관관계를 매핑시켜주기보단
Embedded, Embeddable를 활용한 VO 객체를 넣어보았습니다.
이유는 연관 탐색이 쉽다는 이유로 매핑을 무분별하게 사용하면 향후 더 사용이 어려워질거라고 봤기 때문입니다.
Waiting 엔티티만 해도 당장 3개의 엔티티와 연관이 되어있는데, lazy로 걸면 쿼리가 여러 차례 나갈 것이고, eager로 해두면 의도치 않은 결과가 나올 수도 있다고 합니다.
저도 이 부분은 동의합니다. 불필요한 직접 연관관계를 가지는 경우에는 객체 탐색에는 용이하지만 jpa에서는 추가적인 쿼리가 발생하기에 간접 연관관계을 맺는 것을 우선적으로 선택합니다. 하지만 항상 해당 정보를 대부분 같이 불러와야 하는 경우에는 직접 연관관계를 맺고 fetch join을 활용해서 쿼리가 한번만 나가도록 구성하는 편입니다.
CQRS 패턴 등을 활용해서 조회는 MyBatis나 JdbcTemplate을 활용하고, 저장 삭제는 JPA를 사용해보려고 해두었는데
제가 이해한 CQRS 패턴의 목적은 명령과 조회 과정을 분리해서 각각 확장할 수 있게 나아가는 개발론으로 알고 있는데 조회는 MyBatis나 JdbcTemplate을 이용하고, 저장 삭제는 JPA를 이용하시려고 의도하신 이유는 무엇인가요?
리뷰 천천히 확인하시고 반영해주세요!
| private String password; | ||
| private String role; | ||
|
|
||
| public Member() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 기본 생성자의 접근제어자는 public 대신에 protected로 변경해주시면 좋을 것 같습니다.
| .orElseThrow(() -> new IllegalArgumentException("Invalid theme ID")); | ||
|
|
||
| Reservation reservation = Reservation.of( | ||
| reservationRequest.getName(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 해당 사용자 정보를 LoginMember나 getMemberBy로 불러온 Member 객체에서 이용하면 굳이 reservationRequest에 withUserName이라는 메소드는 불필요 할 것 같습니다.
| @Embedded | ||
| @AttributeOverrides( | ||
| @AttributeOverride(name = "id", column = @Column(name = "member_id")) | ||
| ) | ||
| private MemberId memberId; | ||
|
|
||
| @Embedded | ||
| @AttributeOverrides( | ||
| @AttributeOverride(name = "id", column = @Column(name = "theme_id")) | ||
| ) | ||
| private ThemeId themeId; | ||
|
|
||
| @Embedded | ||
| @AttributeOverrides( | ||
| @AttributeOverride(name = "id", column = @Column(name = "time_id")) | ||
| ) | ||
| private TimeId timeId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 불필요하게 추가 쿼리가 발생하는 것은 좋은 방향이 아니라고 생각해서 항상 같이 조회가 되어야 하는 상황이 아니라면 주로 석주님처럼 간접 참조를 하는 방향으로 진행했을 것 같습니다.
다만 현재 상황에서는 지금 waiting의 경우는 mypage에서 테마정보와 시간정보를 함께 불러오는 상황이어서 직접 참조를 하더라도 문제가 없을 것 같기도 합니다.
저 같은 경우 직접 참조를 할 경우 조회 메소드를 두개 만들긴 합니다. 하나는 fetch join을 이용해서 연관된 객체를 불러오는 메소드, 그리고 기본 jpa에서 조회하는 메소드를 구현해서 연관관계 정보까지 함께 필요한 경우는 전자에 메소드를 이용합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public TimeService(TimeDao timeDao, ReservationDao reservationDao) { | ||
| this.timeDao = timeDao; | ||
| this.reservationDao = reservationDao; | ||
| public TimeService(TimeRepository timeRepository, ReservationRepository reservationRepository) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 서비스의 메소드 중 save와 deleteById에 @Transactional이 필요할 것 같습니다.
| this.reservationDao = reservationDao; | ||
| public ReservationService(ReservationRepository reservationRepository, WaitingRepository waitingRepository, MemberRepository memberRepository, ThemeRepository themeRepository, TimeRepository timeRepository) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 save와 deleteById에 @Transactional를 붙여주세요
| Long rank = waitingService.create(loginMember.getId(), waitingRequest); | ||
|
|
||
| return ResponseEntity | ||
| .created(URI.create("/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
location 헤더의 "/" 대신에 다른 url를 넣어야 할 것 같은데 어떻게 생각하시나요?
| } | ||
|
|
||
| @PostMapping("/waitings") | ||
| public ResponseEntity<WaitingResponse> waiting(LoginMember loginMember, @RequestBody WaitingRequest waitingRequest) throws MalformedURLException, URISyntaxException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 MalfromedURLException과 URISyntaxException을 throw를 하는 이유는 무엇인가요?
| public List<MyReservationResponse> findAllByMemberId(Long memberId) { | ||
| String sql = """ | ||
| SELECT | ||
| A.id, | ||
| B.name, | ||
| A.date, | ||
| C.time_value, | ||
| RANK() OVER (PARTITION BY A.theme_id, A.time_id, A.date ORDER BY A.id) AS rank | ||
| FROM WAITING A | ||
| JOIN THEME B ON A.theme_id = B.id | ||
| JOIN TIME C ON A.time_id = C.id | ||
| where A.member_id = ? | ||
| """; | ||
|
|
||
| return jdbcTemplate.query(sql, (rs, rowNum) -> new MyReservationResponse( | ||
| rs.getLong("id"), | ||
| rs.getString("name"), | ||
| rs.getString("date"), | ||
| rs.getString("time_value"), | ||
| rs.getString("rank") + "번째 예약대기" | ||
| ), memberId); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DB가 직접 responseDto를 반환하고 있는데 이런 경우에는 어떤 점이 문제가 될 수 있을 것 같나요?
| @Transactional | ||
| public Long create(Long memberId, WaitingRequest waitingRequest) { | ||
| Waiting waiting = new Waiting( | ||
| new MemberId(memberId), | ||
| new ThemeId(waitingRequest.getTheme()), | ||
| new TimeId(waitingRequest.getTime()), | ||
| waitingRequest.getDate() | ||
| ); | ||
|
|
||
| if (waitingRepository.existWaiting(new MemberId(memberId), new ThemeId(waitingRequest.getTheme()), new TimeId(waitingRequest.getTime()))) { | ||
| throw new WaitingAlreadyExistException(); | ||
| } | ||
|
|
||
| waitingRepository.save(waiting); | ||
|
|
||
| return waitingRepository.findMyRank(memberId, waitingRequest.getDate(), waitingRequest.getTheme(), waitingRequest.getTime()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 create 메소드에서는 waiting의 id가 아닌 rank 즉 몇번째 정보를 반환하시는 이유는 무엇인가요?
waiting을 생성하는 메소드와 rank를 찾는 메소드를 분리해서 관리하는 것은 어떤가요?
| @JsonCreator | ||
| public WaitingResponse(@JsonProperty("waitingNumber") Long waitingNumber) { | ||
| this.waitingNumber = waitingNumber; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 어노테이션이 없어도 올바르게 동장하는 것 같은데 위의 어노테이션을 이용하신 이유는 무엇인가요?

안녕하세요. 석환님,
4, 5, 6단계 미션 작성해서 제출드립니다~!
제가 4, 5단계는 조회 시 JpaRepository를 활용해서 조회를 활용했는데,
6단계는 네이티브 쿼리가 필요할거 같아 JdbCTemplate을 활용했습니다.
같이 얘기 나누고 싶은 사항으론 제가 Waiting Entity에 대해서는 다른 Entity와 달리 연관관계를 매핑시켜주기보단
Embedded, Embeddable를 활용한 VO 객체를 넣어보았습니다.
이유는 연관 탐색이 쉽다는 이유로 매핑을 무분별하게 사용하면 향후 더 사용이 어려워질거라고 봤기 때문입니다.
Waiting 엔티티만 해도 당장 3개의 엔티티와 연관이 되어있는데, lazy로 걸면 쿼리가 여러 차례 나갈 것이고, eager로 해두면 의도치 않은 결과가 나올 수도 있다고 합니다.
그래서 CQRS 패턴 등을 활용해서 조회는 MyBatis나 JdbcTemplate을 활용하고, 저장 삭제는 JPA를 사용해보려고 해두었는데, 석환님은 어떤 식으로 접근하실지 궁금해서 문의 남겨둡니다 ㅎㅎ