Skip to content
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

QueryDsl 필터링 검색 로직 리팩토링 #180

Merged
merged 18 commits into from
Dec 27, 2023
Merged

QueryDsl 필터링 검색 로직 리팩토링 #180

merged 18 commits into from
Dec 27, 2023

Conversation

weonest
Copy link
Contributor

@weonest weonest commented Dec 11, 2023

✅ PR 체크리스트

  • 테스트
  • 문서화 작업

💡 어떤 작업을 하셨나요?

Issue Number : close #179

작업 내용

  • QueryDsl 필터링 검색 로직을 수정하였습니다.
  • SteadyStack 양방향 연관관계를 단방향으로 수정하였습니다. (생명 주기가 다르기 때문)

📝리뷰어에게

  • QueryDsl : JPAJPQL 을 만들어주지만, from 절에서의 서브 쿼리를 지원하지 않습니다. QueryDsl : SQL 을 이용하면 from절에서도 서브 쿼리를 이용할 수 있게 변경할 수 있다고 하여 직접 적용해보았습니다.

하지만, 몇몇 단점들이 존재했는데, 첫 번째로는 기존의 QClass를 온전하게 사용할 수 없다는 점입니다. QClass를 별도로 생성해야 한다는데 이는 너무 비효율적이고 번거로운 작업이라 추천하지 않는다고 합니다. 따라서 Expressions 를 통해 직접 어떤 클래스의 어떤 필드인지 직접 지정해주는 방식을 사용하면 별도의 QClass 생성 없이도 이용이 가능했습니다. 하지만 기존의 QClass를 재활용 하지 못하고 매번 이렇게 Expressions를 통해 대상 필드를 지정해줘야 한다는 것이 꽤나 불편하고 가독성이 떨어지는 것 같습니다.

또한, Querydsl : SQL을 사용하게됨으로써 기존의 JPAQueryFactory 가 아닌 JPQSQLQuery 를 통해 코드를 생성해야 하는데, 해당 클래스는 이전에 사용한 쿼리를 초기화 시켜주지 않기 때문에 다음 쿼리를 작성하기 위해선 직접 객체를 초기화 시켜줘야 했습니다.

이러한 점들로 미루어 보았을 때 프로젝트 내에서 지속적으로 QueryDsl : SQL을 사용할 수 있을지 의문입니다. 리뷰어분들의 생각은 어떠신가요?

@weonest weonest added the 🧚 Refactor 리팩토링 label Dec 11, 2023
@weonest weonest self-assigned this Dec 11, 2023
Copy link

github-actions bot commented Dec 11, 2023

Test Results

 25 files   25 suites   6s ⏱️
124 tests 124 ✅ 0 💤 0 ❌
125 runs  125 ✅ 0 💤 0 ❌

Results for commit 5b56705.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@K-jun98 K-jun98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다.

Comment on lines 89 to 95
@OneToMany(mappedBy = "steady", cascade = CascadeType.ALL, orphanRemoval = true)
private List<SteadyStack> steadyStacks = new ArrayList<>();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

steadyStacks 양방향 관계를 제거한 이유가 궁금합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

팀내에서 양방향 관계를 설정하는 기준을 생명주기로 결정하였었는데, steadyStacks 의 생명주기가 steady와 다르다는 것을 파악하여 단방향 관계로 변경하였습니다!

Copy link
Contributor Author

@weonest weonest Dec 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

단방향 연관관계로 변경했을 시에 다음과 같은 문제가 발생하였습니다.

  1. 쿼리 발생 개수 증가
    지연초기화가 불가능하기 때문에 steadyStacks를 따로 조회해서 응답 객체에 넣어주어야 하는데, 이렇게 되면 default_batch_fetch_size를 적용하더라도 따로 직접 조회하는 쿼리에 대해서는 적용되지 않기 때문에 쿼리 발생 개수가 기존보다 증가하게 됩니다.

  2. Projections 을 통한 해결이 불가능하다
    위 문제를 Projections을 통해서 Querydsl 을 통한 조회단계에서 필요한 모든 steadyStacks를 가져오려고 하였습니다. 그런데, 일대다 관계의 경우 부모 데이터가 중복으로 발생하기 때문에 Qeurydsl의 transform()groupBy() 등을 사용하여 해결해야 하는데, transform() 의 경우 필요한 데이터를 select 절에 포함시켜 조회하기 때문에 중복된 데이터를 걸러내기 위해 사용한 distinct()가 적용되지 않습니다.

  3. Querydsl-sql 의 문제점
    쿼리 최적화를 위해 서브쿼리를 사용하기 위해서 Querydsl을 JPQL이 아닌 NativeSQL로 변경시켜주는 Querydsl-sql 을 사용하였었는데, 해당 의존성을 사용하면 transform()이 정상적으로 동작하지 않는 것을 확인했습니다. 비슷한 상황을 겪으셨던 영경님의 경우, Querydsl 내부의 Hibernate5TemplateHibernateHandler 동작 방식으로 인해 발생하는 오류였고 이를 JPQLTemplates.DEFAULT 를 사용하게 함으로써 해결을 하셨었는데, 저의 경우에는 이러한 방식으로도 해결이 불가능하였습니다.

따라서 이러한 문제점들로 인해 다시 양방향 관계로 변경하는 것이 옳다고 생각하여 변경하게 되었습니다.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오.. 롤백됐군요 좋습니다 ㅎㅎ

Comment on lines 31 to 33
List<SteadyStackResponse> stacks = steady.getSteadyStacks().stream()
.map(SteadyStackResponse::from)
.toList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

삭제된 이유 궁금합니다.

Copy link
Contributor Author

@weonest weonest Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 프로젝트 초반에 프론트엔드의 요구사항에 검색 결과에 기술 스택 정보를 보여달라는 요구가 있었는데, 추후 해당 기능이 사용 되지 않아 변경하였었습니다.

  2. 하지만, 다시 넣어달라는 요구 사항이 발생하였고, 기존의 쿼리 최적화 리팩토링과 이번 리팩토링 과정에서의 쿼리 변경 및 연관관계 변경이 발생하였고, 이에 따라 필터링 검색 기능에서 Steady 엔티리를 반환할 필요가 없어지게 되었습니다. 따라서, Projections을 이용하여 해당 정보를 넘겨줄 수 있도록 변경하였습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 사항에 대한 변경점 위 이슈에 추가하였습니다!

Comment on lines 242 to 255
var stack = createStack();
ReflectionTestUtils.setField(stack, "id", 1L);
var steadyPosition = createSteadyPosition(steady, position);
var steadyStack = createSteadyStack(steady, stack);
var response = SteadyDetailResponse.of(steady,
List.of(steadyPosition),
List.of(steadyStack),
true,
1L,
false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

픽스처를 사용하는게 어떤가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이후 이슈인 인스턴시오를 활용한 테스트 코드 리팩토링 부분에서 해결하였습니다!

Comment on lines 240 to 242
private SteadyStack createSteadyStack(List<Long> stacks, Steady steady, int index) {
Stack stack = stackRepository.getById(stacks.get(index));
return new SteadyStack(stack, steady);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stacks와 index를 모두 넘기는 거라면 stack을 뽑아서 넘겨줄 수 있을거같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5b56705
steadyStack은 양방향 관계로 변경되어 삭제되어 해당 로직은 삭제되었으나, steadyPosition 생성 방식이 동일했기에 수정하였습니다.

Copy link
Contributor

@na-yk na-yk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QueryDsl : JPAfrom 절에서의 서브 쿼리를 지원하지 않는다는 사실을 처음 알게 됐습니다! 고민을 많이 하신 것 같아요 고생하셨습니다!

@@ -10,7 +10,7 @@
import java.util.Arrays;
import java.util.List;

public record SearchConditionDto(
public record FilterConditionDto(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
DTO 이름이 더 명확하게 수정된 것 같습니다!

@@ -122,7 +122,7 @@ void createSteadyTest() {
assertAll(
() -> assertThat(steady.getId()).isEqualTo(steadyId),
() -> assertThat(participants).hasSameSizeAs(steady.getParticipants().getAllParticipants()),
() -> assertThat(steadyStacks).hasSameSizeAs(steady.getSteadyStacks()),
() -> assertThat(steadyStacks).hasSameSizeAs(steadyRequest.stacks()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -164,7 +164,7 @@ void getSteadiesSearchTest() {
List<SteadySearchResponse> content = response.content();
assertAll(
() -> assertThat(content).hasSameSizeAs(steadies),
() -> assertThat(content.get(0).createdAt()).isAfter(content.get(1).createdAt())
() -> assertThat(content.get(0).createdAt()).isBefore(content.get(1).createdAt())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 이렇게 변경된 이유를 잘 이해하지 못했는데 설명 부탁 드려도 될까요?

Copy link
Contributor Author

@weonest weonest Dec 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Querydsl-sql 사용으로 인해 테스트 케이스에서 order by 쿼리를 통한 정렬이 제대로 이루어지지 않는 문제를 발견하고 일시적으로 위와 같이 변경하였습니다.

문제 상황은 다음과 같습니다.

  • 테스트 환경에서 DB에 저장되는 데이터 개수가 6개 이하order by가 동작하지 않는다.
    최초에는 내장 H2 DB를 사용하기 때문에 발생하는 문제라고 생각하여 로컬 MySQL로 변경하였는데, 그래도 문제가 해결되지 않았습니다.

실제 DB에서는 어떻게 동작하는지 확인하기 위해 쿼리를 직접 날려보니 실제 DB에서도 제대로 동작하지 않는 것을 확인하였고, 데이터의 개수가 7개 이상일 때만 정상적으로 동작하는 것을 확인하였습니다.

SELECT DISTINCT s.*
FROM   steadies s
       JOIN (SELECT DISTINCT s.id,
                             s.bio
             FROM   steadies s
                    LEFT JOIN steady_likes sl
                           ON s.id = sl.steady_id
             ORDER  BY s.bio DESC
             LIMIT  0, 10) AS s2
         ON s.id = s2.id
       JOIN steady_stacks ss
         ON s.id = ss.steady_id
       JOIN steady_positions sp
         ON s.id = sp.steady_id;

테스트 당시 사용한 쿼리는 위와 같으며 데이터가 각각 데이터 개수에 따른 실행계획을 확인했을 때 모두 같은 실행계획을 사용하는 것을 확인했습니다. 따라서 정확한 원인은 모르겠으나 제가 추측하는 바로는 데이터의 개수가 6개 이하일 경우 Mysql 옵티마이저의 동작이 변경되는 것 같은데 무엇이 어떻게 변경되는지 까지는 파악하지 못하였습니다.

order by를 서브 쿼리가 아닌 메인 쿼리에 추가하면 데이터 개수와 상관없이 정상적인 동작을 하게 되는데 이 경우에는 Using Filesort 전략을 사용하게 되고 (from 절의 서브쿼리로 생긴 임시 테이블에는 인덱스가 없기 때문에 메인 쿼리의 order by가 인덱스로 처리되지 못하기 때문이라고 생각합니다) 이는 데이터가 많아질 경우 성능 저하를 유발시킬 수 있습니다.

  • Querydsl-sql 사용에 대한 회의감
    이번에 from절에서 서브쿼리를 사용하기 위해 Querydsl-sql을 사용하면서 꼭 이 라이브러리를 사용해서 구현해야만 할까? 라는 생각을 하게 되었습니다. 기존의 QClass를 제대로 활용하지 못하기 때문에 쿼리에 사용되는 객체들의 Path를 일일이 작성해주어야 한다는 불현함부터 시작해서, JPASQLQuery 수동 초기화, 사용할 수 없는 transfrom() 등 여러 문제점들이 보이기 시작하였고, 결국에는 기존에 사용하던 Querydsl-jpa 로 다시 돌아가기로 결정하였습니다.

추가적으로 기존의 서브쿼리 방식의 최적화에는 한계가 있다고 판단하여 offset 기반의 페이징 방식을 포기하고, no-offset 기반의 커서 페이징 방식을 도입하기로 결정하였습니다.

@weonest weonest merged commit 05fcfd9 into dev Dec 27, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Steady] QueryDsl 로직 리팩토링
3 participants