-
Notifications
You must be signed in to change notification settings - Fork 1
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
Api: ✏️ 월별 지출내역 조회시 발생하는 N+1 문제 개선 #110
Conversation
일단 리뷰 전에... 여기선 커스텀 카테고리 N+1 테스트를 하고 싶은 거라서 랜덤 값을 삽입하는 것보다는 그냥 3~4개 정도의 커스텀 카테고리로 등록된 지출 내역 데이터를 등록하시는 게 더 나았을 거예요. |
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.
기존 테스트를 재활용하신 거니 수정하실 필요는 없지만, N+1 테스트를 위해 통합 테스트를 사용하는 건 비효율적입니다.
DAO 계층 테스트 케이스를 작성해서, 응답값을 given 절로 처리하면 DB 연결조차 생략 가능해요.
...external-api/src/main/java/kr/co/pennyway/api/apis/ledger/service/SpendingSearchService.java
Outdated
Show resolved
Hide resolved
...ernal-api/src/test/java/kr/co/pennyway/api/config/fixture/SpendingCustomCategoryFixture.java
Outdated
Show resolved
Hide resolved
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.
웬만하면 그냥 승인하려고 했는데, 정렬 부분 하나 확인 부탁드려요~
작업 이유
Spending
객체와 연관관계를 맺고있는SpendingCustomCategory
객체가 존재.QueryDSL
확장 레포지토리를 통한 월별 지출내역 조회시, N+1 문제 발생SpendingSearchService
를 제거하고, QueryDsl 확장성을 제공하는 부분을 domain 모듈의SearchService
로 이전작업 사항
테스트코드를 작성하면서
SpendingCustomCategoryFixture
가 필요해지고, 커스텀 카테고리를 가지는 지출내역의 Insert가 필요해지면서 변경사항이 길어졌습니다. 박살난 가독성을 위해 최대한 요약해 설명해보겠습니다.이때 Hibernate가 발생시키는 쿼리는 2회 이며, 이를 만족하지 않을시 테스트는 실패합니다.
Spending
과SpendingCustomCategory
객체들의 user가 될User
객체를 create할때 1회readSpendings
시 1회리뷰어가 중점적으로 확인해야 하는 부분
SpendingCustomRepositoryImpl
의findByYearAndMonth
의 QueryDsl 사용방법이 적절한지?발견한 이슈
X