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

Fixture 관련 쿼리를 테스트 메서드 실행 전 호출할 수 있도록 로직 변경 #748

Closed
wants to merge 1 commit into from

Conversation

apptie
Copy link
Collaborator

@apptie apptie commented Nov 13, 2023

📄 작업 내용 요약

Fixture에 em.flush() 호출 추가

🙋🏻 리뷰 시 주의 깊게 확인해야 하는 코드

이슈에 관련된 내용을 첨부했습니다
예시로 BidServiceFixture만 수정했습니다
BidService로 한 이유는 외래 키 데드락 찾느라고 계속 얘만 돌리고 있기 때문입니다

📎 Issue 번호

@apptie apptie added backend 백엔드와 관련된 이슈나 PR에 사용 test 테스트 추가 시 suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 labels Nov 13, 2023
@apptie apptie added this to the 레벨 5 milestone Nov 13, 2023
@apptie apptie self-assigned this Nov 13, 2023
Copy link
Collaborator

@kwonyj1022 kwonyj1022 left a comment

Choose a reason for hiding this comment

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

질문

지토가 이슈에 적어주신 통합 테스트를 하기 위해 별도의 Fixture 클래스에서 필요한 데이터를 insert하는데, 코드로 하다보니 데이터를 저장하는 과정에서 @Transactional이 필요한 문제였습니다. 에 대한 이유가 궁금합니다.

이전에도 말씀드린 적이 있지만, 저희가 저번에 시간을 들여 repository의 jpa 의존성을 제거하고 도메인 레이어에 repository 인터페이스를 만들어서 분리하는 작업을 했었습니다.
그럼에도 불구하고 서비스 레이어에서는 jpa가 제공하는 변경감지를 사용하고 있기 때문에 이를 개선해야한다고 생각합니다.
마찬가지로 테스트코드의 경우에도 Jpa를 사용하지만 JpaRepository에 대한 테스트가 아니라면 최대한 그 의존을 줄여야한다고 생각하기 때문에 픽스처 생성 시에도 변경감지를 사용하지 않아야 한다고 생각합니다. (지금은 변경감지를 사용하기 때문에 지금 상태에서 @Transactional만 뺀다면 테스트가 실패할 것 같네요) 아무튼 그래서 값을 변경한 엔티티의 경우 일일이 save()를 다시 해주는 것이 맞다고 생각하고, em.flush() 도 사용하지 않는 것이 좋다고 생각합니다.

또, 테스트에서 @Transactional을 사용하지 않으면 픽스처에서 save() 호출 시마다 flush 및 commit이 일어날 것이기 때문에 굳이 em.flush()를 안 해도 되지 않을까요?

@apptie
Copy link
Collaborator Author

apptie commented Nov 17, 2023

통합 테스트를 하기 위해 별도의 Fixture 클래스에서 필요한 데이터를 insert하는데, 코드로 하다보니 데이터를 저장하는 과정에서 @Transactional이 필요한 문제였습니다.
-> 이 파트는 cascade로 인한 문제입니다
예를 들어 AuctionImage를 영속화하는 레포지토리는 존재하지 않습니다
이건 어쩔 수 없이 직접 sql을 날리거나 AuctionImage에 대한 라이프 사이클 관리를 담당하고 있는 Auction을 통해서 해야 합니다
여기서 전자의 경우 이미 코드상으로 구현된 Fixture를 다시 sql 기반으로 변경해야하므로 비용이 너무 비싸서 좋지 않다고 생각했고
Auction의 cascade를 통해 AuctionImage를 넣어주기 위해서는(정확히는 JPA가 동작하기 위해서는) @Transactional이 필수라서 그렇습니다

그럼에도 불구하고 서비스 레이어에서는 jpa가 제공하는 변경감지를 사용하고 있기 때문에 이를 개선해야한다고 생각합니다.
-> 요 부분은 이전에 제가 도메인 엔티티와 영속성 객체를 분리하는건 어케 생갹하느냐고 질문드린 부분이라고 생각되는데 그 때 기억으로는 아마도 반대 의견이 많아서 흐지부지된걸로 기억하고 있습니다
제 개인적인 의견은 엔초가 말씀해주신 부분에 동의하기는 합니다

그리고 이 부분이 조금 애매한게 도메인 패키지에 JpaRepository를 위치시켰으니 이거는 컬렉션에서 도메인 엔티티를 꺼내는 용도로 사용되는 의미라고 볼 수도 있다고 생각합니다
List와 같은 컬렉션의 요소를 꺼내 값을 변경한다면 다시 변경된 객체를 List에 넣어줄 필요가 없는 것처럼, JPA의 변경감지는 객체 지향적인 코드를 위한 목적도 포함하고 있다고 생각합니다
예를 들어 도메인 패키지에 위치하고 있는 Repository의 구현체를 순수 자바 구조로 변경하면서 save() 시 Map 같은 곳에다가 저장한다면 이 또한 지금과 동일하게 Map에서 요소를 꺼내고 변경한 뒤 Map에 또 put()을 해줄 필요 없는 그런 느낌입니다
이거는 JPA의 변경 감지에 의존한다고 보기도 좀 애매하고...하지만 @Transactional이 없으면 예외가 터지니 JPA에 의존한다기보다는 영속성 계층(Transaction)에 의존한다고 볼 수 있을 것 같고...그렇다기에는 하나의 비즈니스 로직이니 트랜잭션으로 관리해줘야 하는거 아닌가 싶기도 하고 어떤 관점에서 보느냐에 따라서 좀 갈릴 것 같습니다

마찬가지로 테스트코드의 경우에도 Jpa를 사용하지만 JpaRepository에 대한 테스트가 아니라면 최대한 그 의존을 줄여야한다고 생각하기 때문에 픽스처 생성 시에도 변경감지를 사용하지 않아야 한다고 생각합니다.
-> 변경 감지도 사용하고 있지만 @Transactional을 사용하는 가장 큰 이유는 cascade 때문이므로 해당 PR과는 별개의 문제라고 생갹합니다

테스트에서 @Transactional을 사용하지 않으면 픽스처에서 save() 호출 시마다 flush 및 commit이 일어날 것이기 때문에 굳이 em.flush()를 안 해도 되지 않을까요?
-> 이 부분은 cascade만 제외한다면 동의합니다
cascade까지 저장하기 위해서는 @Transactional이 필요해서 좀 애매합니다
변경 감지를 모두 찾아서 save()로 변경하는 것보다는 em.flush()를 날리는게 비용이 적은 이유도 있기는 하지만요...
또한 모든 Fixture에서 적용되야 하므로 논의가 필요할 것 같네요
테스트 코드에서의 @Transactional에 대한 토비님의 질답과 같이 테스트에서 @Transactional을 쓰냐 마냐는 좀...의견이 갈리는 주제라고 생각이 됩니다


그래서 아무튼 결론은 지금 cascade를 사용하기 때문에 save()를 호출해준다고 해도 불가능(TransactionRequiredException 예외 발생)하다고 볼 수 있고 이걸 어케 풀어나가느냐는 논의가 필요할 것 같네요
지금은 em.flush() 단순 호출이 가장 간단한 것 같기는 합니다

@kwonyj1022
Copy link
Collaborator

영속성 계층에 의존한다고 볼 수도 있을 것 같다는 것에 대해서 그러면 만약에 아주 만약에 저희가 Jpa를 사용하지 않게 된다면 casecade나 변경 감지를 사용하는 서비스와 테스트 코드를 모두 변경해야 하는 건가요? 이런 상황을 대비할 필요는 없는지 궁금합니다. 뭔가 저번에 인프라 리팩토링 하면서 Impl을 붙이네 마네 이런 얘기하면서 Jpa 사용 안 하는 경우에 대해 이야기했던 것 같아서 질문드려요

도메인 패키지에 JpaRepository를 위치시켰으니

이 부분에 대해서는 저희 JpaRepository는 infrastructure/persistence 패키지에 있지 않나요? 그냥 repository를 말씀하시는 거죠..?

@apptie
Copy link
Collaborator Author

apptie commented Nov 17, 2023

저희가 Jpa를 사용하지 않게 된다면 cascade나 변경 감지를 사용하는 서비스와 테스트 코드를 모두 변경해야 하는 건가요?
-> 둘 다 어떤 기술을 사용하느냐에 따라서 달라질 것 같기는 한데 그래도 최소한의 변경 사항은 존재하지 않을까 싶습니다
예를들어서 Spring Data JPA에서 Spring Data Jdbc로 변경하면은 저 jdbc도 cascade 비슷한 걸 지원해주지만 아예 같은건 아니라서 변경 사항이 있지? 않을까? 싶네요
그래도 cascade는 도메인 엔티티까지만 영향을 끼치지 않을까 싶고 변경 감지는 모르겠네요 아는게 없어서..
그냥 JdbcTemplate이나 mybatis를 쓴다면은 변경 감지도 없고 cascade도 없으니 무조건 바꿔야 할 것 같고 그렇습니다

요런 부분은 인프라 리펙토링으로 인해서 JpaReposiory와 같은 구체적인 부분에 대한 의존은 약해졌지만 ORM을 사용한다는 가정 하에 비즈니스 로직을 작성했으니 어쩔 수 없다 싶기는 합니다
그래도 하이버네이트에서 이클립스 뭐시기로 ORM 구현체를 바꾼다면 그에는 영향을 받지 않으니 의존성이 약해졌다는 목표는 달성한 것 같고 그렇습니다
도메인 엔티티랑 영속성 객체랑 분리하면서 해소할 수 있을 것 같기는 한데 지금 상태에서는 어쩔 수 없다고 받아들어야 할 것 같습니다

이 부분에 대해서는 저희 JpaRepository는 infrastructure/persistence 패키지에 있지 않나요? 그냥 repository를 말씀하시는 거죠..?
-> 넵 맞습니다 잘못 말했네요
JpaRepository X Repository O입니다

@apptie apptie closed this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend 백엔드와 관련된 이슈나 PR에 사용 suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 test 테스트 추가 시
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants