[Review] 2022/06/23 마지막 PR (Issue Tracker)

이번 리뷰는 Brain 께서 해주셨다! 리뷰해주셔서 감사합니다 !!

https://github.com/codesquad-members-2022/issue-tracker/pull/200

[Team-26][BE : Shine] Issue-Tracker 2주차 2회차 PR by zbqmgldjfh · Pull Request #200 · codesquad-members-2022/issue-tracke

https://github.com/codesquad-members-2022/issue-tracker/pull/231

[Team-26][BE : Shine] Issue-Tracker 3주차 1회차 PR by zbqmgldjfh · Pull Request #231 · codesquad-members-2022/issue-tracke

2주차 2번째 PR에서 수정한 부분이 적어 한번에 정리해본다!

 

1. 질문

1-1) Service 안에서 다른 Service 의존하기

Brain 께서는 "가급적 안하는게 좋습니다. 정말 필요한지 설계를 다시 검토해 보시는 것이 좋기는 합니다." 라고 답변해 주셨다.

하지만 항상 코딩은 정답이 없는 영역이기에....

 

백기선님은 다음과 같이 답변하셨다.

https://www.inflearn.com/questions/21703

Service와 Repository의 관계에 대한 질문입니다! - 인프런 | 질문 & 답변

 

내가 내린 결론은 Service에서 순환 관계만 잘 끊어내면 사용해도 무관할 것 같다? 라고 생각하게 되었다.

 

1-2) Controller에서의 커맨드, 쿼리 메서드 분리

우선 Issue 컨트롤러는 다음과 같습니다.

CQRS를 생각했기 때문에 이를 분리하려 노력하였다.

 

커맨드성 메서드들은 CUD 시에 사용하면 반환값이 void 이고, 쿼리성 메서드들은 R시에 사용하며 반환값이 DTO 입니다.

이렇게 분리하여 컨트롤러에서 구현하고 있는데, 혹시 커맨드 메서드들도 정상적으로 수행 됬음을 상태코드가 아닌, 뭔가를 반환해줘야 할까요?

 

답변 : 클라이언트 개발자와 잘 합의하셔서 개발하시면 됩니다. 어떻게 해도 무방합니다.

 

1-3) 동적쿼리 질문 (지금 생각하면 약간 바보같은 질문?)

동적 쿼리 작성시 질문
우선 동적 쿼리를 이용하여 조건에 따른 검색을 진행하는 repository 코드는 다음과 같습니다.

@Override
public Page<Issue> searchIssueByCondition(User loginUser, SearchConditionRequest condition, String query, Pageable pageable) {
    List<Issue> issues = queryFactory.selectFrom(issue)
            .join(issue.author, user).fetchJoin()
            .join(issue.milestone, milestone).fetchJoin()
            .where(
                    queryIn(condition.getQuery()),
                    isOpen(condition.getOpen()),
                    isClose(condition.getClose()),
                    isOwner(condition.getOwner(), loginUser),
                    assigneeEquals(condition.getAssignee()), // 문제의 지점 (1:N)
                    commenterEquals(condition.getComment()) // 문제의 지점 (1:N)
            )
            .offset(pageable.getOffset())
            .limit(pageable.getPageSize())
            .fetch();

    return null;
}

 

1) assignee나 comment 같은 경우 1:N 관계라 fetchJoin을 하기에는 데이터 중복 현상이 발생할 것 이고...

2) 우선 그냥 1:N부분은 proxy로 두고 조회 한 후, BatchSize로 가져오고, 어플리케이션 level에서 필터링을 한번 더 해줘야 하니... 동적 쿼리의 의미가 없어지는 것 같아서요...

3) 그렇다고 다 fetchJoin하고 distinct 키워드를 추가하기에는 다 메모리로 퍼올리는 부담이 있어서..
이런 경우에는 어떻게 해결해야 할까요??

 

우선 돌이켜 생각해보니.... where절에 필터링 하는거랑.... 데이터를 가져오는 fetchJoin이랑 연관지어 잘못생각하고 있었던것이 잘못 이였다...

 

이 질문은 Brain 과 Dion에게 각각 물어봤으며 두분의 답변은 다음과 같았다.

우선 Brain은 

라고 말씀해 주셨다.

우선 DB에서 꼭 최적화를 끝내서 쿼리를 날리기 보다는, 쿼리를 날린후 어플리케이션 수준에서 필터링을 해도 된다는 의미인것 같다.

 

Dion은 다음과 같이 말씀해 주셨다.

 Dion의 답변에 내가 잘못 생각하고 있던 포인트를 잡았다....

아니 생각해보니까 fetchJoin은 데이터를 N+1상황에서 가져올때의 문제를 해결하기 위함인데...

이걸 where절까지 끌고 들어와서 filter부분에서 생각을 잘못하고 있었다.

 

그냥 where절에서는 딱 조건! 예를 들어 user.email.eq(userEmail)과 같이 필터링 조건만 주면된다!!

fetchJoin의 문제는 그 다음 결과를 가져올때의 문제인 것 이다!

 

너무 처음부터 최적화를 생각하려 한것같기도...

 

2. 리뷰 정리

2-1) 너무 과도한 검증

혹여 테스트를 작성할때라도 음수를 전달할까봐 작성하긴 하였습니다만.... 사실상... DB에 저장할때는 예외가 발생하지 않을것 같습니다.

 

2-2) Static Method의 필요성

지금 돌이켜 보니 그렇다. 이걸 i.getter로 하나하나 전달하고 있었다니....

그냥 Dto.of(i) 이런식으로 전달하고, IssueResponse도 IssueResponse.of(i)로 끝냈으면 됬을 것 같다! 수정해야겠다!

 

2-3) 변경감지 될것인가?

엇? findIssue는 1차 케쉬에 있을거고, 수정을 가하면 flush 시점에 변경감지로 자동으로 되지 않나요?

라고 처음에 생각했다. 하지만 Brian께서 그점을 모르실리는 전무하니.... 다시한번 나의 코드를 살펴보게 되었다.

 

우선 editAssignees()의 코드는 다음과 같다.

public void editAssignees(List<User> assigneeList) {
    this.issueAssignees.forEach(IssueAssignee::clearUser);
    this.issueAssignees.clear(); // 우선 초기화 후 선택된 유저만 새롭게 할당
    addAssignees(assigneeList);
}

우선 IssueAssginee::clearUser()로 IssueAssginee -> User의 연관관계를 끊어낸다.

이후 Issue 자체의 IssueAssginee 자체에서도 List를 비워버린다.

 

이후 List자체를 새롭게 할당하게 된다.

 

여기까지가 나의 생각이였고, 후반부에서는 시간이 부족하여 다른 API 처럼 test를 작성하지 못하고 넘어가 버렸다.

 

리뷰를 받은 후 다음과 같이 테스트 코드를 작성하게 되었다.

@Test
public void issue_assignee_test() {

    // given
    User user1 = new User("shine1", "shine@naver.com1", "avatarurl");
    User user2 = new User("shine2", "shine@naver.com2", "avatarurl");
    User user3 = new User("shine3", "shine@naver.com3", "avatarurl");
    userRepository.save(user1);
    userRepository.save(user2);
    userRepository.save(user3);

    // 이슈 생성후 저장
    Issue issue = new Issue("test", true, user1);
    issue.addAssignees(List.of(user1, user2, user3));
    Issue savedIssue = issueRepository.save(issue);

    // 영속성 컨텍스트 초기화
    em.flush();
    em.clear();

    // when
    Issue findIssue = issueRepository.findById(savedIssue.getId()).get();
    issueService.editAssignees(findIssue.getId(), new AssigneesEditRequest(List.of(user2.getId())));

    // then
    assertThat(findIssue.getIssueAssignees().size()).isEqualTo(1);
    assertThat(findIssue.getIssueAssignees().get(0).getUser()).isEqualTo(user2);
}

테스트 코드의 결과 정상작동 하였다?

 

find 로 찾아온 Issue가 영속성 컨텍스트 안에 있고, 따라서 변경감지되어 수정된 것 같다!

 

1-3) List와 Set의 비교

정말 좋은 리뷰인것 같다.

지금 우리의 시나리오에서는, 하나의 Issue에 user가 할당되게 되는데 이때 동일한 User가 2번 할당될일은 없다.

이런 경우 List보다는 Set이 더 적절한 자료구조의 선택인것 같다.

 

JPA Context에서 HashSet은 중복을 허용하지 않으므로 add() 메소드로 객체를 추가할 때 마다 equals() 메소드로 같은 객체가 있는지 비교한다.

같은 객체가 없으면 객체를 추가하고 true를 반환하고, 같은 객체가 이미 있어서 추가에 실패하면 false를 반환한다. 참고로 HashSet은 해시 알고리즘을 사용하므로 hashcode()도 함께 사용해서 비교한다.

Set은 엔티티를 추가할 때 중복된 엔티티가 있는지 비교해야 한다. 따라서 엔티티를 추가할 때 지연 로딩된 컬렉션을 초기화한다.