[FIX] 지도 API 수정 #55
[FIX] 지도 API 수정 #55
Conversation
Test Results1 tests 1 ✅ 0s ⏱️ Results for commit 6ac6ae0. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
저번 PR에서 놓친 부분 같은 @Trasactional이 빠진 메서드가 몇개 보이는데, 의견이 궁금해요(질문)
There was a problem hiding this comment.
지도 화면의 경우 검색 기록의 저장이 필요한 findMenuByMenuIdOnMap 메소드 외에 메소드에서는 모두 조회만을 필요로 하여 @transactional을 사용하지 않았었는데, 이를 @transactional(readOnly = tue)를 사용하는 것이 맞는 방법인 것 같습니다!
There was a problem hiding this comment.
기존에는 제가 Transactional에 대한 기본 지식이 부족하여 위 내용에 대해 이것저것 찾아보게 되었는데, @transactional(readOnly = true)을 사용함에 따라 발생하는 트레이드오프가 있는 것 같아요!
장점
- 성능 최적화
- 데이터 일관성
- 가독성 향상
단점
- 추가적인 쿼리 발생 가능
- 스냅샷 유지, 커넥션 등의 오버헤드
위와 같은 사항들로 인해 어노테이션을 사용하는 것이 맞는 지에 대한 고민이 생겼는데, Lazy-loading이 필요한 부분에 한해서만 어노테이션을 사용하는 것도 괜찮겠다는 생각이 들었습니다.(참조 : https://hungseong.tistory.com/74)
There was a problem hiding this comment.
위 내용에 대해서는 리팩토링 과정에서 수정하는 것이 나을 것 같아 우선 기존 코드 그대로 유지하려고 합니당
There was a problem hiding this comment.
일단 트랜잭션이 사용되지 않은 로직에 대해서는 크게 문제 없는 것으로 확인됩니다!
관련해서 스프링 트랜잭션보다 DB단에서 이야기를 해보면 트랜잭션에서는 단일 조회에서는 트랜잭션을 사용하지 않아도 크게 문제되지 않을 수도 있어요.
그런데 여러 테이블 조회에서 문제가 발생할 수 있는 것으로 알고 있어요(팬텀 읽기)
관련해서 찾아보셔도 좋을듯!! 👍
| @@ -63,29 +67,33 @@ public List<MenuOnMapDto> findMenusOnMap(Long userId) { | |||
| .collect(Collectors.toList()); | |||
| } | |||
|
|
|||
| /** | |||
| * | |||
| public List<MapSearchHistoryDto> findSearchHistoryOnMap(Long userId) { | ||
| User user = userRepository.findById(userId) | ||
| .orElseThrow(NotFoundUserException::new); | ||
|
|
||
| Pageable pageable = PageRequest.of(0, 10); |
There was a problem hiding this comment.
페이징 설정 값을 주입 받는 것 보다 내부에서 정의하는게 더 좋을까요? (질문)
There was a problem hiding this comment.
제가 이해한 바로는 PageRequest.of(0 ,10)으로 정의해 놓은 방식에 대해 질문해 주신 것 같은데, @PageDefault을 사용하여 디폴트 값을 설정하고, 입력도 받을 수 있게 설정해도 될 것 같습니다. 구현한 시점에서의 생각은 검색 기록은 10개를 제공하는 것으로 고정되어 있어 위와 같이 구현하였습니다 :)
| User user = userRepository.findById(userId) | ||
| .orElseThrow(NotFoundUserException::new); | ||
|
|
||
| Menu menu = menuRepository.findByIdAndUserId(menuId, userId) | ||
| .orElseThrow(NotFoundMenuException::new); |
There was a problem hiding this comment.
비슷한 코드가 반복되고 있어요.
지난 회의를 떠올려보면, 서비스 간 순환 참조 문제를 방지하고, 복잡한 의존성 관리 대신에 모두 Repository으로 가리키고 사용하신거 같은데, 맞나요?
There was a problem hiding this comment.
제 의견을 덧붙이면, 추후에 리펙토링 하면 정말 좋은 부분이라고 생각해요. 저도 비슷한 문제를 구현하면서 만났는데 개선 방법을 생각해봐여(지금 생각으로 서비스 레이어를 분리하는 방식)
There was a problem hiding this comment.
비슷한 코드가 반복되고 있어요. 지난 회의를 떠올려보면, 서비스 간 순환 참조 문제를 방지하고, 복잡한 의존성 관리 대신에 모두 Repository으로 가리키고 사용하신거 같은데, 맞나요?
기존의 의도는 말씀해주신 바와 같은데, 해당 메소드 뿐만 아니라 위와 같은 코드가 정말 빈번하게 반복되어 작성하면서도 모호했던 부분이었던 것 같습니다. 이후에 전체적으로 리팩토링하면 좋을 것 같아요 👍
✏️ 작업 개요
⛳ 작업 분류
🔨 작업 상세 내용
💡 생각해볼 문제