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

[BE] Repository 추상화 계층을 변경한다 #420

Merged
merged 4 commits into from
Sep 14, 2022

Conversation

kth990303
Copy link
Collaborator

@kth990303 kth990303 commented Sep 8, 2022

close #414
close #36

구현사항

  1. Repository 추상화 계층 제거 -> Spring Data JPA 인터페이스 상속 이용
  • update 쿼리는 repository가 아닌 service에서 엔티티 변경 감지를 이용하도록
    • repository에서 @Query를 이용하는 방법은 update쿼리만 날리고 select 쿼리를 날리지 않기 때문에 persistent context에 남아있고 트랜잭션이 끝나지 않아 캐시에 남아있음. 따라서 Find쿼리를 쓰면 변경 전 데이터를 가져와서 테스트 차질이 생김. 참고
  1. repository Test에서 @SpringBootTest 제거 후 @DataJpaTest 사용
  • 테스트 성능 개선
  1. repository에 있던 예외처리를 service로 이동

확인요망

  1. repository에서 서비스로 예외처리를 변경 -> 예외처리를 놓쳤거나 테스트 커버리지가 부족하지 않은지 확인 부탁드립니다.
  2. 테스트 속도가 개선이 됐는지 확인해보기

추상화 사용하지 않는 방향으로 변경
`@SpringBootTest` 대신 `@DataJpaTest` 사용
@naepyeon-sonarqube-1

This comment has been minimized.

Copy link
Collaborator

@asebn1 asebn1 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 +16 to +20
@Query("select m.id "
+ "from Member m "
+ "where m.platform = :platform and m.platformId = :platformId")
Optional<Long> findMemberIdByPlatformAndPlatformId(@Param("platform") final Platform platform,
@Param("platformId") final String platformId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

CustomRepo를 만들지에 대해 논의 하는 건 어떤가요?
지금도 나쁘진 않습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

반환 타입을 우리가 원하는대로 커스터마이징할 수 있다는 장점이 있겠네요.
수요일에 한 번 얘기해보죠!

Comment on lines +19 to +34
@Query(value = "select distinct new com.woowacourse.naepyeon.service.dto.WrittenMessageResponseDto"
+ "(m.id, r.id, r.title, t.id, t.name, m.content, m.color, "
+ "case when r.recipient = com.woowacourse.naepyeon.domain.rollingpaper.Recipient.MEMBER then p.nickname "
+ "when r.recipient = com.woowacourse.naepyeon.domain.rollingpaper.Recipient.TEAM then t.name "
+ "else '' end) "
+ "from Message m"
+ ", Rollingpaper r"
+ ", Team t"
+ ", TeamParticipation p "
+ "where m.rollingpaper.id = r.id "
+ "and r.team.id = t.id "
+ "and p.team.id = t.id "
+ "and m.author.id = :authorId "
+ "and (p.member.id = r.member.id or r.member.id is null)")
Page<WrittenMessageResponseDto> findAllByAuthorId(@Param("authorId") final Long authorId,
final Pageable pageRequest);
Copy link
Collaborator

Choose a reason for hiding this comment

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

쿼리 dsl을 활용해봐요!
그리고 세타조인이 아닌 left join과 batch size를 설정하여 Pagination을 처리할 수 있지 않을까요?

Copy link
Collaborator Author

@kth990303 kth990303 Sep 13, 2022

Choose a reason for hiding this comment

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

queryDSL은 #421 에서 해결할 예정입니다.

추가적으로 궁금한 점이 있습니다.
해당 메서드를 테스트하는 MessageRepositoryTest의 findAllByMemberIdAndPageRequest코드에서 아래와 같은 쿼리문이 총 4번 나갑니다.

select
        distinct message0_.message_id as col_0_0_,
        rollingpap1_.rollingpaper_id as col_1_0_,
        rollingpap1_.title as col_2_0_,
        team2_.team_id as col_3_0_,
        team2_.team_name as col_4_0_,
        message0_.content as col_5_0_,
        message0_.color as col_6_0_,
        case 
            when rollingpap1_.classification='MEMBER' then teampartic3_.nickname 
            when rollingpap1_.classification='TEAM' then team2_.team_name 
            else '' 
        end as col_7_0_ 
    from
        message message0_ cross 
    join
        rollingpaper rollingpap1_ cross 
    join
        team team2_ cross 
    join
        team_participation teampartic3_ 
    where
        message0_.rollingpaper_id=rollingpap1_.rollingpaper_id 
        and rollingpap1_.team_id=team2_.team_id 
        and teampartic3_.team_id=team2_.team_id 
        and message0_.member_id=? 
        and (
            teampartic3_.member_id=rollingpap1_.member_id 
            or rollingpap1_.member_id is null
        ) limit ? offset ?

1번 나가는 게 맞네요. 4개 중 2개는 다른 쿼리였고, 나머지 1개는 아래 옵션때문에 동일 쿼리임에도 Hibernate와 실제쿼리용 둘 다 띄워주는 것이었네요

logging.level:
  org.hibernate.SQL: debug

또 궁금한 점.
세타조인을 사용하고 있다고 생각했는데 from절 아래에 cross라고 적혀있어서 theta join과 cross join이 같은지 궁금해서 찾아봤지만, 마땅한 답을 찾지 못했습니다. 혹시 theta join과 cross join의 차이점이 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그리고 네오한테 피드백을 받았는데 아래 쿼리처럼 null을 포함하는 쿼리는 인덱싱을 적용할 수 없다고 해서 추가적으로 회의해봐야될 것 같아요!

and (
            teampartic3_.member_id=rollingpap1_.member_id 
            or rollingpap1_.member_id is null
        ) limit ? offset ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 부분은 양방향 연관을 맺고 나면 batch_size옵션으로 쿼리를 많이 단순화할 수 있을 것 같은데... TeamParticipation에 대한 양방향 매핑을 추가해서 처리하는 방법은 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저도 양방향도 괜찮은 것 같아요. 한번 논의해보는 게 좋겠네요

Comment on lines -16 to 19
@SpringBootTest
@Transactional
@DataJpaTest
@Import(JpaAuditingConfig.class)
class MemberRepositoryTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -70,7 +75,8 @@ void createMemberWhen() {
@DisplayName("회원 정보를 수정할 때 수정일자가 올바르게 나온다.")
void updateMemberWhen() throws InterruptedException {
final Member member = new Member("alex", "alex@naepyeon.com", Platform.KAKAO, "1");
final Long memberId = memberRepository.save(member);
final Long memberId = memberRepository.save(member)
.getId();

sleep(1);
Copy link
Collaborator

@asebn1 asebn1 Sep 12, 2022

Choose a reason for hiding this comment

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

sleep(1);이지금도 필요한가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

수정했습니다.
근데 sleep(1)은 원래도 필요 없었어야 맞는 것 같은데 이상하네요.
image
변경하기 전 develop 브랜치에서 sleep(1)을 빼고 테스트해봤는데 문제 없이 돌아가는 화면입니다.

3번 돌려봤는데 3번 다 문제 없었어요.

generationType = IDENTITY이므로 쓰기 지연이 일어나지 않고 바로 쿼리가 나가기 때문에 update가 save보다 무조건 이후에 되는게 확실할 것 같아 sleep(1)이 원래도 필요 없었을 것 같은데 당시에 왜 넣었는지 지금은 잘 모르겠네요..

Copy link
Collaborator

@Seungpang Seungpang left a comment

Choose a reason for hiding this comment

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

케이 수정하시느라 고생하셨습니다!
테스트 속도가 점점 개선되는게 좋네요! 👍


@SpringBootTest
@Transactional
@DataJpaTest
Copy link
Collaborator

Choose a reason for hiding this comment

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

DataJpaTest로 변경했으면
EntitiyManager대신 테스트용으로 만들어진 TestEntityManager를 사용해보는건 어떤가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

처음 알았네요.
테스트하는 데엔 불편함이 없겠군요. 성능상 이점이 있는진 잘 모르겠지만 DataJpaTest에서 테스트하기 용이하도록 제공하는 entitymanager 중 하나인 것 같아 도입해보도록 하겠습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TestEntityManager를 사용하면 의존성을 줄인다는 장점도 있는 것 같네요
image

@@ -70,7 +75,8 @@ void createMemberWhen() {
@DisplayName("회원 정보를 수정할 때 수정일자가 올바르게 나온다.")
void updateMemberWhen() throws InterruptedException {
final Member member = new Member("alex", "alex@naepyeon.com", Platform.KAKAO, "1");
final Long memberId = memberRepository.save(member);
final Long memberId = memberRepository.save(member)
.getId();

sleep(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

이제 더이상 sleep은 필요 없을거같네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

수정했습니다.
근데 sleep(1)은 원래도 필요 없었어야 맞는 것 같은데 이상하네요.
image
변경하기 전 develop 브랜치에서 sleep(1)을 빼고 테스트해봤는데 문제 없이 돌아가는 화면입니다.

3번 돌려봤는데 3번 다 문제 없었어요.

generationType = IDENTITY이므로 쓰기 지연이 일어나지 않고 바로 쿼리가 나가기 때문에 update가 save보다 무조건 이후에 되는게 확실할 것 같아 sleep(1)이 원래도 필요 없었을 것 같은데 당시에 왜 넣었는지 지금은 잘 모르겠네요..

@naepyeon-sonarqube-1
Copy link

Passed

Analysis Details

2 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 2 Code Smells

Coverage and Duplications

  • Coverage 97.56% Coverage (87.00% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.00% Estimated after merge)

Project ID: woowacourse-teams_2022-nae-pyeon_AYKuzmWfelLz0D2BhgWj

View in SonarQube

Copy link
Collaborator

@yxxnghwan yxxnghwan left a comment

Choose a reason for hiding this comment

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

변경할 부분이 많았을 텐데 수고하셨습니다!👍

@asebn1 asebn1 merged commit 74d2db5 into develop Sep 14, 2022
@kth990303 kth990303 deleted the feature/414-repository-change branch September 14, 2022 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BE] Repository 계층 추상화를 제거한다. [BE] Repository 계층 설계를 확정한다.
4 participants