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

User bounded context에서 JPA 사용하도록 수정 #348

Merged
merged 3 commits into from
Jul 21, 2024

Conversation

Zeniuus
Copy link
Contributor

@Zeniuus Zeniuus commented Jul 14, 2024

Checklist

  • 충분한 양의 자동화 테스트를 작성했는가?
    • 계단정복지도 서비스는 사이드 프로젝트로 진행되는 만큼 충분한 QA 없이 배포되는 경우가 많습니다. 따라서 자동화 테스트를 꼼꼼하게 작성하는 것이 서비스 품질을 유지하는 데 매우 중요합니다.

Proposed Changes

  • sqldelight를 JPA로 전환하기 위한 첫 작업입니다. 한 번에 적용하기에는 작업 기간이 길어져서 컨플릭이 심할 것 같아서, 점진적으로 적용하려고 합니다.
  • 점진적인 전환을 할 때 주의해야 할 것은 한 트랜잭션 안에서 sqldelight와 jpa를 동시에 사용하는 곳인데, 아래와 같이 해결합니다.
    • 트랜잭션 관리 - 어차피 sqldelight의 주 목적은 ORM이 아니라 sql을 type-safe하게 쓰는 것이므로, 그냥 Spring JPA에서 열어주는 트랜잭션 안에서 sql을 날려도 무방합니다. 한편 Spring에서 JPA를 사용할 경우, 트랜잭션 관리와 EntityManager의 라이프사이클 관리가 JpaTransactionManager로 인해 일원화되기 때문에 sqldelight가 열어주는 트랜잭션을 쓰기 어렵습니다. 따라서 TransactionManager는 JPA의 것을 사용하도록 수정하고, sqldelight 쪽에서는 트랜잭션 관리와 관련된 기능을 완전히 제거합니다.
    • 엔티티 연관관계 - JPA는 ORM이므로, 연관관계에 따른 eager/lazy loading 등을 자동으로 관리합니다. 따라서 연관관계에 있는 엔티티 중 일부는 jpa를 사용하고, 다른 일부는 sqldelight를 사용하면 문제가 발생할 수 있습니다. 이를 방지하기 위해 최대한 bounded context 단위로 JPA 변환을 진행합니다.
  • 마이그레이션 과정에서, 기존에 ,,를 delimiter로 사용하던 리스트 컬럼을 json으로 바꾸도록 수정합니다.

논의해야 할 것

  • 현재 application 레이어에 repository 인터페이스를 두고 infra에 repository 구현체를 두는데, JPA로 넘어간 이후에도 이런 식으로 레이어링을 유지할지, 아니면 application 레이어에서 바로 jpa repository를 선언해서 쓸지 선택이 필요합니다. 현재 PR에서는 기존 구현을 존중하여 전자 방식으로 구현했습니다.

이 작업의 범위가 아닌 것

  • sqldelight의 sqm 파일은 전체 엔티티를 JPA로 넘기기 전까지는 그대로 두려고 합니다. 이후에 flyway나 liquibase 등으로 마이그레이션 진행하면 될 것 같습니다. 용성님이 liquibase로 넘어가고 싶다고 하신 게 기억이 나네요 ㅋㅋ

@Zeniuus Zeniuus self-assigned this Jul 14, 2024
@Zeniuus Zeniuus requested a review from a team as a code owner July 14, 2024 07:08
Comment on lines +35 to +42
override fun equals(other: Any?): Boolean {
if (this === other) return true
if (javaClass != other?.javaClass) return false

other as User

return id == other.id
}
Copy link
Member

Choose a reason for hiding this comment

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

data class 가 아니고 class 면 이거 override 해줄 필요가 없지 않나요?

Copy link
Contributor

Choose a reason for hiding this comment

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

Class의 동등성 비교가 기본으로 메모리상 주소 비교일 것 같은 느낌...?

Copy link
Member

@jyoo0515 jyoo0515 Jul 14, 2024

Choose a reason for hiding this comment

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

In Kotlin, the equals() function is inherited by all classes from the Any class. By default, the equals() function implements referential equality. However, classes in Kotlin can override the equals() function to provide a custom equality logic and, in this way, implement structural equality.

넹 그렇네요. 그럼 spring data jpa 쓰면서 other 가 Hibernate$Proxy 인 케이스도 고려해서 작성해줘야 하지 않을까요? User$Proxy 면 as User 로 다운 캐스팅하다 에러 날 것 같기도 해서 as? 로 하는건 어떤가요? proxy 면 애초에 javaClass 가 다른가? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

회사에서 이렇게 쓰고 있는데 별 문제가 없었어서... bytecode로 instrument 됐을 때 클래스가 어떻게 될지는 모르겟는데 일단 잘 동작하는 것 같긴 합니다 ㅋㅋ

@jyoo0515
Copy link
Member

현재 application 레이어에 repository 인터페이스를 두고 infra에 repository 구현체를 두는데, JPA로 넘어간 이후에도 이런 식으로 레이어링을 유지할지, 아니면 application 레이어에서 바로 jpa repository를 선언해서 쓸지 선택이 필요합니다.

DB 에 접근하는 것도 out port 로 보면 현재와 같은 레이어링이 맞는 것 같아 보이기도 하는데 JPA 는 애초에 repository 가 interface 여서 더 어색한 것 같기도 하네요.

어차피 spring jpa 플러그인을 모든 subproject 에 적용하기도 했고 application 에서도 tx 관리 + entity 관리를 한번에 하는게 좋아 보여서 jpa repository 를 그냥 application 에 넣는 것에 한표 던집니다. 보일러플레이트 코드도 좀 줄 것 같고요.

@Zeniuus
Copy link
Contributor Author

Zeniuus commented Jul 21, 2024

어차피 spring jpa 플러그인을 모든 subproject 에 적용하기도 했고 application 에서도 tx 관리 + entity 관리를 한번에 하는게 좋아 보여서 jpa repository 를 그냥 application 에 넣는 것에 한표 던집니다. 보일러플레이트 코드도 좀 줄 것 같고요.

저도 동의가 돼서, JPA로 전환할 때 이렇게 바꿔볼게용

@Zeniuus Zeniuus force-pushed the migrate-to-jpa/user-bounded-context branch from 8c1b119 to 45123a0 Compare July 21, 2024 07:49
@Zeniuus Zeniuus merged commit ac23698 into main Jul 21, 2024
1 check passed
@Zeniuus Zeniuus deleted the migrate-to-jpa/user-bounded-context branch July 21, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants