refactor: 스타카토 양방향 연관관계 끊기 #547 #569

Ho-Tea · 2024-12-13T03:41:55Z

⭐️ Issue Number

refactor: 스타카토 양방향 연관관계 끊기 #547

🚩 Summary

기존에 Moment와 MomentImage의 양방향 연관관계를 끊음

Moment와 MomentImage의 양방향 연관관계를 끊음으로써 부모 엔티티에서 자식 엔티티의 생성을 관리했던 로직이 Service에 위치하게 되었습니다.
그로인해 MomentImageRepository에 쿼리 메서드와 BulkDelete(JPQL Query)가 추가되었습니다.
Moment 단위테스트를 실행할 때 MomentImage에 관한 설정사항이 제외되었습니다.

양방향 연관관계를 끊는것의 목적은 아래 2가지가 존재하였습니다.

Cascade 설정과 양방향 연관관계에서 발생하는 연관관계 주인 설정 문제, 엔티티 상태 동기화 문제(양쪽 필드 갱신), 직렬화 시 순환 참조 등의 다양한 이슈를 피하기 위해
단방향 관계나 최소한의 관계 매핑만 유지하게 되어, 도메인 모델의 단순화와 유지보수성 향상에 기여하기 위해

현재 PR에는 BulkInsert관련 커밋은 올라가있지 않은 상태입니다. 의견 나눈 후 적용할지 정하려고 합니다!

추가로 양방향 연관관계를 끊는 것에서 더 나아가

기존에는 영속 전이 상태를 통해 아래와 같이 새롭게 들어온 MomentImage에 대해 각 객체 별 Insert 쿼리가 하나씩 나가는 문제가 있었습니다.

이를 BulkInsert로 해결하고자 하였고,
저희는 현재 기본키 생성 전략중 IDENTITY 전략을 사용하고 있는 상태입니다.
영속성 컨텍스트 내부에 엔티티를 식별할때 엔티티 타입과 PK값으로 식별하지만, IDENTITY전략의 경우 DB에 Insert 한 후 PK 확인이 가능하기 때문에 Hibernate는 insert batching을 지원하지 않는 상태였습니다.

jdbc or Native SQL을 이용해야 하는 상황
- Native SQL은 모든 디비에 반영이 될 수 없기에 지양

참고자료

따라서 새롭게 JdbcTemplate을 통해 BatchUpdate하는 방향으로 구성해 보았습니다.

BulkInsert 로그

🛠️ Technical Concerns

BulkInsert가 정말 필요한가?

저희의 IDENTITY 전략에서는 jdbcTemplate을 통해 새롭게 BulkInsert하는 과정이 필요했습니다.
이를 위해서 새롭게 BulkInsert 전용 클래스가 필요해졌고, 그로 인해 아래와 같은 다이어그램 형식이 되었습니다.

저희는 아무리 많아도 한번에 최대 5개의 이미지만을 저장할 수 있기에, 하나의 요청당 최대(이미지 5개 저장)로 줄일 수 있는 쿼리는 4개(기존 5개 나가던 쿼리를 1개로)로 제한됩니다. -> 성능상 이점 미미
1차캐시를 이용하지 못하기에 아래와 같이 Entity의 id를 가지고 오고 싶다면 다시 DB를 조회해야 하는 문제가 있습니다 -> 번거롭고, 휴먼에러 가능성 높아짐

저장된 Entity의 id를 바로 조회하게 된다면 NullPointerException이 발생하게 된다.

id값을 직접 넣어준다면 테스트 통과

BulkInsert의 도입이 정말 필요할까 얘기 나눠보고 싶습니다!

🙂 To Reviewer

📋 To Do

BulkInsert를 도입할것인지 아닐지 결정하기!

github-actions · 2024-12-13T03:43:52Z

Test Results

25 files 25 suites 5s ⏱️
166 tests 166 ✅ 0 💤 0 ❌
173 runs 173 ✅ 0 💤 0 ❌

Results for commit fe68126.

github-actions · 2024-12-13T03:43:53Z

🌻Test Coverage Report

Overall Project	78.51%	🍏
Files changed	100%	🍏

File	Coverage
MomentService.java	100%	🍏
MomentImages.java	100%	🍏
MomentRequest.java	100%	🍏
MomentDetailResponse.java	100%	🍏
MemoryService.java	98.39%	🍏
Moment.java	66.92%	🍏
MomentImage.java	30.51%	🍏

linirini · 2024-12-13T07:44:02Z

호티가 말한대로, 스타카토 생성 시 이미지 업로드는 최대 5개만 가능하기 때문에 추가 쿼리로 인한 성능 저하 혹은 개선 효과는 미미하다고 생각합니다.
저는 그래서 Bulk Insert를 취하여 얻는 이점보다는 코드 복잡도로 인한 불편함이 클 것 같다고 생각해요:)

linirini

호티! 구현하시느라 너무 수고하셨습니다.
조심스럽지만, 스타카토 양방향 연관관계를 끊는 것이 더 좋은 선택이었을지에 대한 의문이 들어서 리뷰에 함께 언급해놓았습니다. 호티의 의견이 매우 궁금합니다 ㅎㅎ
수정 사항이 많았을 것 같은데, 정말 수고 많으셨습니다bb

linirini · 2024-12-13T07:49:49Z

backend/src/main/java/com/staccato/moment/domain/MomentImages.java

    }

-    private void validateNumberOfImages(List<String> addedImages) {
+    private <T> void validateNumberOfImages(List<T> addedImages) {


제네릭으로 한 이유가 있을까요?

List<String>, List<MomentImage> 형식 모두를 호환하고자 제네릭으로 표현했었는데, 사용되는 곳은 압축생성자 한 곳이네요! 필요없어요!

linirini · 2024-12-13T07:53:45Z

backend/src/main/java/com/staccato/moment/domain/MomentImages.java

-        originalImages.forEach(this.images::remove);
+    private boolean contains(MomentImage momentImage) {
+        return this.images.stream()
+                .anyMatch(image -> image.getImageUrl().equals(momentImage.getImageUrl()));


image에 같은지 물어보는 메서드를 분리하면 어떨까요?
코드 한 줄에 .이 너무 많은 것 같아요!

linirini · 2024-12-13T07:54:18Z

backend/src/main/java/com/staccato/moment/domain/MomentImages.java

-    protected void update(MomentImages momentImages, Moment moment) {
-        removeExistsImages(new ArrayList<>(images));
-        addAll(momentImages, moment);
+    public List<MomentImage> findValuesNotPresentIn(MomentImages targetMomentImages) {


findImages가 아닌 findValues로 네이밍한 이유가 있을까요?

특별한 이유가 없습니다!
findImages가 더 나은 네이밍 같네요!

linirini · 2024-12-13T08:02:52Z

backend/src/main/java/com/staccato/moment/service/MomentService.java

@@ -69,18 +74,43 @@ public void updateMomentById(
        Moment moment = getMomentById(momentId);
        validateMemoryOwner(moment.getMemory(), member);

-        Memory targetMemory = getMemoryById(momentRequest.memoryId());
-        validateMemoryOwner(targetMemory, member);
+        Memory memory = getMemoryById(momentRequest.memoryId());


메서드 순서 확인해주세요! (getMemoryById)

이 부분은 이해가 잘 가지않았습니다!
보충 설명 부탁드려용

linirini · 2024-12-13T08:06:21Z

backend/src/main/java/com/staccato/moment/service/MomentService.java


        momentRepository.save(moment);
+        momentImageRepository.saveAll(newMomentImages.images());


momentImages 양방향 관계를 끊어보니 어떤가요?
사실 저는 스타카토 관련 작업을 할 때 Image 없이 작업을 하는 일이 잘 없다보니 양방향이 필요한 부분이라고 생각했었는데요! 실제로 서비스 모든 로직에서 Images 관련 작업이 별개로 필요해지니, 양방향을 사용할 때보다 휴먼 에러가 나오기 쉽겠다는 생각이 들었어요!
호티는 그 2가지를 다 경험해보았으니 어땠는지 궁금합니다.
(추가로 궁금한 건 삭제가 잘 안되던 버그를 해결하기 위해 양방향 참조를 끊은걸까요? BatchInsert를 위해 양방향 참조를 끊은걸까요?)

삭제 관련된 전이를 끊으니(orphanRemoval) 부모엔티티가 자식 엔티티의 생성 생명주기만을 관리하게 되었고,
양방향 연관관계는 추후에 언제든 예기치 못한 예외상황을 만들 수 있다고 생각되었고,
영속성전이 속성으로 인해 Insert가 항상 생성되는 엔티티마다 나가는 문제도 있어서 양방향 연관관계를 끊는 방식으로 구성하게 되었습니다.

양방향 연관관계를 끊으면서 Insert가 생성되는 엔티티마다 나가는 것을 하나의 BulkInsert를 통해 성능적 이점을 도모하려고 시도하였습니다. 하지만 PR에 적어놓은 것 처럼 성능적 이점은 미미했고, 더 나아가 양방향 연관관계를 끊는 것으로 인해 수정된 곳이 많아졌고, 서비스 로직에 굳이 없어도 될 비즈니스 로직이
명시되게 되었습니다.

굳이 없어도 될 비즈니스 로직

// MomentService.java 의 updateMomentById 메서드 MomentImages momentImages = getMomentImagesBy(moment); MomentImages newMomentImages = momentRequest.toMomentImages(moment); removeExistImages(momentImages, newMomentImages); saveNewImages(momentImages, newMomentImages);

BulkInsert를 통해 얻는 성능적 이점이 미미

양방향 연관관계를 끊음으로 인해 Serivce에 굳이 없어도 될 비즈니스 로직이 존재

휴먼에러 가능성이 증가

이와 같은 이유로 양방향 연관관계로 다시 유지하는게 나을 것 같다는 생각이 들었습니다.
직접 양방향과 단방향 연관관계의 이점과 단점을 비교하여 설정한 이 과정 자체가 의미있었다 생각합니다 👍

linirini · 2024-12-13T08:11:04Z

backend/src/main/java/com/staccato/moment/service/MomentService.java

클래스에 메서드가 굉장히 많아졌네요!
책임 분리가 조금 필요해보이는데, 생각한 방향이 있을까요?
호티의 의견이 궁금합니다!

확실히 기존에 MomentImages 가 가지고 있던 비즈니스 로직이 밖으로 나오게 되어 책임이 모호해 졌습니다.

양방향 연관관계를 유지한다면 고려하지 않아도 될 것 같아요

양방향 연관관계를 유지하지 않는다면 퍼사드 패턴과 같이 새로운 계층 구조를 통해 해결할 수 있을 것 같아요

linirini · 2024-12-13T08:16:20Z

backend/src/test/java/com/staccato/moment/domain/MomentImagesTest.java

@@ -29,44 +25,32 @@ void addMomentImages(int size) {
        }

        // when & then
-        assertThatNoException().isThrownBy(() -> new MomentImages(images));
+        assertThatNoException().isThrownBy(() -> MomentImages.of(images, null));


참조 무결성이 지켜지지 않아도 괜찮을까요? 실제로 MomentImage에서는 moment 검증을 따로 하고 있진 않네요!
나아가, MomentImages는 도메인일까요? 서비스일까요? (저도 지금 작업하는 내용에서 비슷한 고민을 하고 있어서 여쭤봐요!)
moment 없이 MomentImage를 만들 수 있게 허용한다면 참조 무결성 위반 문제 여지가 있다고 생각해요! 만약, 이에 대한 검증을 추가한다면 지금 테스트는 MomentImages에 대한 테스트임에도 Moment를 생성하고, 연관관계를 매핑하는 추가 작업이 필수적으로 필요할 것 같아요. 저는 이 지점에서 이 클래스는 서비스 테스트처럼 작성하게 되었고, 그래서 서비스인지 도메인인지 고민했을 때 서비스라는 결론을 내렸었는데요.
호티의 의견이 궁금합니다.

MomentImages는 MomentImage List를 필드로 가지는 일급컬렉션으로 MomentImage에 설정되는 Moment가 null로 들어오는지에 대한 (참조 무결성 위반) 것은 MomentImages가 검증해야 하는 책임에서는 멀다고 생각했어요!

MomentImage가 책임져야할 부분

Moment를 설정하는 곳은 MomentImage내부에서 이루어지기 때문에 참조 무결성 위반이 이루어지지 않게끔 검증하는 부분 또한 MomentImage 내부라고 생각했어요!

MomentImages는 단순히 MomentImage List를 가지고 있고, 해당 MomentImage 에 대한 비즈니스로직 작업이 정상적으로 이루어지는지만 판단하면 된다고 생각했어요!

MomentImage가 정상적으로 생성이 되는 것 -> MomentImage의 책임

linirini · 2024-12-13T08:21:39Z

backend/src/test/java/com/staccato/moment/repository/MomentImageRepositoryTest.java

+                .containsExactlyInAnyOrder("url1", "url2", "url3");
+    }
+
+    @DisplayName("특정 스타카토의 이미지 중 가장 작은 ID 값을 가진 이미지를 조회한다.")


ID 값을 기준으로 생각하는 게 맞을까요?
썸네일 == 사진 순서 중 제일 첫번째 사진
이라면, 기존의 스타카토에 새로운 사진을 추가하고, 새로운 사진을 가장 첫 장으로 설정한다면 썸네일은 새로운 사진이 되어야 할 것 같아요.
지금의 로직에서 썸네일 설정이 잘 보장되고 있는지 추가적으로 서비스 테스트를 해봐야할 것 같습니다.

Ho-Tea added 4 commits December 12, 2024 22:30

refactor: MomentImage와의 양방향 연관관계 삭제

2974d8b

refactor: MomentImage 일급 컬렉션 비즈니스 로직 수정

23b25b3

refactor: MomentImageRepository 배치 삭제와 Moment 별 조회 로직 추가

252d68f

refactor: 스타카토 수정시 이미지 수동 삭제 로직 구현

fe68126

Ho-Tea added backend We are backend>< feat 기능 (새로운 기능) refactor 리팩토링 (변수 및 메서드 네이밍 변경) labels Dec 13, 2024

Ho-Tea added this to the sprint-7 milestone Dec 13, 2024

Ho-Tea requested review from BurningFalls and linirini December 13, 2024 03:41

Ho-Tea self-assigned this Dec 13, 2024

linirini requested changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: 스타카토 양방향 연관관계 끊기 #547 #569

refactor: 스타카토 양방향 연관관계 끊기 #547 #569

Ho-Tea commented Dec 13, 2024 •

edited by linirini

Loading

github-actions bot commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

linirini commented Dec 13, 2024

linirini left a comment

linirini Dec 13, 2024

Ho-Tea Dec 19, 2024

linirini Dec 13, 2024

linirini Dec 13, 2024

Ho-Tea Dec 21, 2024

linirini Dec 13, 2024

Ho-Tea Dec 21, 2024

linirini Dec 13, 2024

Ho-Tea Dec 21, 2024

linirini Dec 13, 2024

Ho-Tea Dec 21, 2024

linirini Dec 13, 2024

Ho-Tea Dec 21, 2024

linirini Dec 13, 2024


		momentRepository.save(moment);
		momentImageRepository.saveAll(newMomentImages.images());

refactor: 스타카토 양방향 연관관계 끊기 #547 #569

Are you sure you want to change the base?

refactor: 스타카토 양방향 연관관계 끊기 #547 #569

Conversation

Ho-Tea commented Dec 13, 2024 • edited by linirini Loading

⭐️ Issue Number

🚩 Summary

현재 PR에는 BulkInsert관련 커밋은 올라가있지 않은 상태입니다. 의견 나눈 후 적용할지 정하려고 합니다!

추가로 양방향 연관관계를 끊는 것에서 더 나아가

🛠️ Technical Concerns

BulkInsert가 정말 필요한가?

🙂 To Reviewer

📋 To Do

github-actions bot commented Dec 13, 2024

Test Results

github-actions bot commented Dec 13, 2024

🌻Test Coverage Report

linirini commented Dec 13, 2024

linirini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ho-Tea commented Dec 13, 2024 •

edited by linirini

Loading