[REFACTOR] Repository 상속 객체 전환 #53

leegwichan · 2025-01-13T07:36:38Z

🚩 연관 이슈

closed #48

🗣️ 리뷰 요구사항 (선택)

Repositoy가 왜 이렇게 생겼는지 의문이 들텐데 코멘트 남겨 놓았으니 참고 부탁드려요~

github-actions · 2025-01-13T07:38:20Z

Test Results

68 tests 68 ✅ 7s ⏱️
34 suites 0 💤
34 files 0 ❌

Results for commit 0a79cdd.

♻️ This comment has been updated with latest results.

github-actions · 2025-01-13T07:38:22Z

📝 Test Coverage Report

Overall Project	84.74%	🍏
Files changed	100%	🍏

File	Coverage
MemberService.java	100%	🍏
ParliamentaryService.java	100%	🍏
ParliamentaryTableRepository.java	100%	🍏
ParliamentaryTimeBoxRepository.java	100%	🍏
ParliamentaryTimeBoxes.java	100%	🍏
MemberRepository.java	58.33%	🍏

leegwichan

Repository 코드가 이해 안될 것 같아 추가 코멘트 남깁니다~

leegwichan · 2025-01-13T07:37:30Z

src/main/java/com/debatetimer/domain/parliamentary/ParliamentaryTimeBoxes.java

-        timeBoxes.sort(TIME_BOX_COMPARATOR);
-        this.timeBoxes = timeBoxes;
+        this.timeBoxes = timeBoxes.stream()
+                .sorted(TIME_BOX_COMPARATOR)
+                .toList();


이거 가변 객체에서만 되는 거라 바꿨음

leegwichan · 2025-01-13T07:38:38Z

src/main/java/com/debatetimer/repository/parliamentary/ParliamentaryTimeBoxRepository.java

+    @Transactional
+    @Modifying
+    @Query("DELETE FROM ParliamentaryTimeBox ptb WHERE ptb IN :timeBoxes")
+    void deleteAll(List<ParliamentaryTimeBox> timeBoxes);


Repository에서는 deleteAllInBatch를 지원하지 않아서 QueryCreation 했습니다.
@Transactional은 떼도 될 것 같은데, 의견 부탁드립니다.

@Transactional은 떼도 될 것 같은데, 의견 부탁드립니다.

관련해서 오류나 문제가 발생하지 않는다면 상관없습니다

기존에 @transactional을 붙인 이유는 무엇인가요?

@Modifying의 경우 1차 캐시를 무시하고 바로 DB에 쿼리를 날리기 때문에 영속성 컨텍스트에는 아직 객체가 남아있게 됩니다. 이때 DB상에서 삭제된 객체를 조회해오거나 삭제된 객체를 참조하는 객체를 조회하게 되면 오류가 발생하는 이슈가 있습니다. 따라서 clearAutomatically =true 설정을 통해 해당 쿼리 이후 영속성 컨텍스트를 비워두어야 할 것 같아요.

ref)
https://wildeveloperetrain.tistory.com/142

@를 사용할 때는 멘션이 걸리지 않게 백틱으로 감싸주세요

[반영 완료]

기존에 @transactional을 붙인 이유는 무엇인가요?

GPT가 붙이라고 해서 혹시나 해서 붙였습니다. 쿼리 하나 날려서 문제 없을 것 같아서 지우겠습니다.

@Modifying의 경우 1차 캐시를 무시하고 바로 DB에 쿼리를 날리기 때문에 영속성 컨텍스트에는 아직 객체가 남아있게 됩니다. 이때 DB상에서 삭제된 객체를 조회해오거나 삭제된 객체를 참조하는 객체를 조회하게 되면 오류가 발생하는 이슈가 있습니다. 따라서 clearAutomatically =true 설정을 통해 해당 쿼리 이후 영속성 컨텍스트를 비워두어야 할 것 같아요.

오 좋네요! 그러면 서비스에 있는 entityManager.clear()까지 같이 지우도록 하겠습니다~

[사소한 질문]

entityManager를 비우는 것은 좋아요! 일단 서비스 코드 보시죠

@Transactional public void deleteTable(Long tableId, Member member) { ParliamentaryTable table = getOwnerTable(tableId, member.getId()); ParliamentaryTimeBoxes timeBoxes = timeBoxRepository.findTableTimeBoxes(table); timeBoxRepository.deleteAll(timeBoxes.getTimeBoxes()); // entityManager 비움 tableRepository.delete(table); // entityManager에 table 객체 없어서 다시 조회한 후에 지움 }

쓸모없는 조회가 1회 추가되는데, delete도 Query Creation해서 영속성 컨텍스트 참조하지 않게 처리할까요?

친절하게 주석으로 설명해주어서 감사합니다.

ORM을 사용한다면 최대한 제공한 인터페이스를 쓰는 편을 선호하고

1회 쿼리 조회가 큰 성능상 문제가 되지 않으리라 생각하여

그대로 두어도 괜찮다고 생각됩니다.

Repository에서는 deleteAllInBatch를 지원하지 않아서 QueryCreation 했습니다. @Transactional은 떼도 될 것 같은데, 의견 부탁드립니다.

이것 관련해서 더 찾아보았는데 @Transactional 붙이는 편이 더 좋을 것 같아요. 레포지토리 커스텀 update / delete 메서드의 경우 상위 객체에서 트랜잭션이 열리지 않으면 repository 호출임에도 트랜잭션이 열리지 않는다고 하네요. 기존에 정의된 메서드의 경우 simpleJpaRepository 상위에 @Transactional이 붙어 있어서 트랜잭션이 열리게 된다고 합니다.

상위 서비스의 트랜잭션 여부에 상관없이 독립적으로 날릴 수 있는 메서드여야 한다고 생각해서 @Transactional 붙여주면 좋을 것 같습니다.

ref)

https://yeonyeon.tistory.com/288

[반영 완료]

콜리 꼼꼼히 찾아봐줘서 고마워요~ 반영해서 @Transcational 추가했습니다~

leegwichan · 2025-01-13T07:39:47Z

src/main/java/com/debatetimer/repository/parliamentary/ParliamentaryTimeBoxRepository.java

+    @Transactional
+    default List<ParliamentaryTimeBox> saveAll(List<ParliamentaryTimeBox> timeBoxes) {
+        return timeBoxes.stream()
+                .map(this::save)
+                .toList();
+    }


마찬가지로 Repository에서는 saveAll()을 지원하지 않아요.
CrudRepositry 참고했는데, save를 for문으로 하고 있길래, stream으로 돌려서 default method로 구현했습니다.

아마 Identity 전략의 경우에는 키를 발급 받기 이전이기 때문에 쓰기지연이 안되어서 한번에 벌크 save 쿼리가 안되는 것으로 알고 있어요. 그래서 saveAll에서도 for문으로 돌리도록 짜놓은 것으로 알고 있습니다.
해당 구현도 괜찮은 것 같아요.

ref)
https://medium.com/@hee98.09.14/jpa-id%EC%A0%84%EB%9E%B5%EC%9D%B4-identity%EC%9D%B8-%EC%83%81%ED%83%9C%EC%97%90%EC%84%9C-bulk-insert-%EC%88%98%ED%96%89%ED%95%98%EA%B8%B0-8bf9c760bd82

unifolio0 · 2025-01-13T07:40:25Z

/noti

코멘트가 어디있나요?

unifolio0

/noti

건의사항 1개만 남겨서 바로 approve 합니다

unifolio0 · 2025-01-13T07:46:52Z

src/main/java/com/debatetimer/repository/parliamentary/ParliamentaryTableRepository.java

+public interface ParliamentaryTableRepository extends Repository<ParliamentaryTable, Long> {
+
+    Optional<ParliamentaryTable> findById(long id);


저희 컨벤션 중에 CRUD 순서에 맞추어 메서드를 정렬한다.가 있는데 적용하는 건 어떨까요?

[반영 완료]
Repository 모두 CRUD 순으로 변경했습니다

[질문]
default method도 같이 CRUD 순으로 정렬하는게 맞을까요?

저는 디폴트 메소드도 정렬 해야한다에 한표입니다.

저도 default method더라도 정렬하는게 좋은 것 같아요.

unifolio0 · 2025-01-13T07:47:29Z

src/main/java/com/debatetimer/repository/parliamentary/ParliamentaryTimeBoxRepository.java

+    @Transactional
+    @Modifying
+    @Query("DELETE FROM ParliamentaryTimeBox ptb WHERE ptb IN :timeBoxes")
+    void deleteAll(List<ParliamentaryTimeBox> timeBoxes);


@Transactional은 떼도 될 것 같은데, 의견 부탁드립니다.

관련해서 오류나 문제가 발생하지 않는다면 상관없습니다

coli-geonwoo

/noti

커찬! 빠르게 작업해주어 감사합니다
늦은 리뷰에 염치없지만 몇가지 질문 남겼어요. 답변만 확인하고 approve 하겠습니다.

coli-geonwoo · 2025-01-14T08:22:42Z

src/main/java/com/debatetimer/repository/parliamentary/ParliamentaryTimeBoxRepository.java

+    @Transactional
+    @Modifying
+    @Query("DELETE FROM ParliamentaryTimeBox ptb WHERE ptb IN :timeBoxes")
+    void deleteAll(List<ParliamentaryTimeBox> timeBoxes);


기존에 @transactional을 붙인 이유는 무엇인가요?

@Modifying의 경우 1차 캐시를 무시하고 바로 DB에 쿼리를 날리기 때문에 영속성 컨텍스트에는 아직 객체가 남아있게 됩니다. 이때 DB상에서 삭제된 객체를 조회해오거나 삭제된 객체를 참조하는 객체를 조회하게 되면 오류가 발생하는 이슈가 있습니다. 따라서 clearAutomatically =true 설정을 통해 해당 쿼리 이후 영속성 컨텍스트를 비워두어야 할 것 같아요.

ref)
https://wildeveloperetrain.tistory.com/142

coli-geonwoo · 2025-01-14T08:28:14Z

src/main/java/com/debatetimer/repository/parliamentary/ParliamentaryTimeBoxRepository.java

+    @Transactional
+    default List<ParliamentaryTimeBox> saveAll(List<ParliamentaryTimeBox> timeBoxes) {
+        return timeBoxes.stream()
+                .map(this::save)
+                .toList();
+    }


아마 Identity 전략의 경우에는 키를 발급 받기 이전이기 때문에 쓰기지연이 안되어서 한번에 벌크 save 쿼리가 안되는 것으로 알고 있어요. 그래서 saveAll에서도 for문으로 돌리도록 짜놓은 것으로 알고 있습니다.
해당 구현도 괜찮은 것 같아요.

ref)
https://medium.com/@hee98.09.14/jpa-id%EC%A0%84%EB%9E%B5%EC%9D%B4-identity%EC%9D%B8-%EC%83%81%ED%83%9C%EC%97%90%EC%84%9C-bulk-insert-%EC%88%98%ED%96%89%ED%95%98%EA%B8%B0-8bf9c760bd82

coli-geonwoo · 2025-01-14T08:29:46Z

src/test/java/com/debatetimer/service/member/MemberServiceTest.java

            assertAll(
                    () -> assertThat(actual.nickname()).isEqualTo(request.nickname()),
-                    () -> assertThat(memberCount).isEqualTo(1L)
+                    () -> assertThat(foundMember).isNotEmpty()


존재하는지를 검증하는 isPresent 검증부가 있었던 것 같은데 조금 더 의미가 명확해질 것 같아요.

Suggested change

() -> assertThat(foundMember).isNotEmpty()

() -> assertThat(foundMember).isPresent()

[반영 완료]

MemberServiceTest.CreateMember, ParliamentaryServiceTest.DeleteTable 모두 수정했습니다~

coli-geonwoo · 2025-01-14T08:38:09Z

/noti

@leegwichan
리뷰 완료했습니다! 한번씩 확인 부탁드릴게요!

leegwichan · 2025-01-15T08:26:14Z

/noti @debate-timer/backend
리뷰 반영 모두 완료했습니다~ 질문 2개 남겼으니 확인해주세요.

coli-geonwoo · 2025-01-16T12:30:04Z

/noti

커찬! 반영 여부 잘 확인했습니다! 코멘트 하나에 대해서만 의견 묻고 싶어서 확인한 이후 바로 approve 하겠습니다!

#53 (comment)

leegwichan · 2025-01-17T03:29:14Z

/noti @coli-geonwoo
해당 코멘트 반영 완료했습니다~ 확인 부탁드릴께요!

coli-geonwoo

/noti

확인해주어 감사합니다 커찬~ 확인해서 approve&merge 할게요!

leegwichan added 3 commits January 13, 2025 15:50

refactor: MemberRepository 가 Repository 를 상속하도록 변경

0cd7146

refactor: ParliamentaryTableRepository 가 Repository 를 상속하도록 변경

6c94899

refactor: ParliamentaryTimeBoxRepository 가 Repository 를 상속하도록 변경

f94ec8f

unifolio0 self-requested a review January 13, 2025 07:36

unifolio0 added the refactor 리팩토링 label Jan 13, 2025

unifolio0 requested a review from coli-geonwoo January 13, 2025 07:36

unifolio0 assigned leegwichan Jan 13, 2025

leegwichan commented Jan 13, 2025

View reviewed changes

unifolio0 approved these changes Jan 13, 2025

View reviewed changes

coli-geonwoo requested changes Jan 14, 2025

View reviewed changes

leegwichan and others added 4 commits January 15, 2025 17:16

refactor: deleteAll 메서드에서 영속성 컨텍스트를 비우도록 리팩토링 실시

8fe1114

test: actual 문에서 isNot~() 형식을 is~() 형식으로 수정

e8fa9f5

refactor: Repository 메서드를 CRUD 순으로 순서 변경

9fe87ab

Merge branch 'develop' into refactor/#48

52a6242

refactor: @Query를 이용한 메서드에서 트랜젝션을 사용하기 위해 @Transactional 추가

0a79cdd

coli-geonwoo approved these changes Jan 17, 2025

View reviewed changes

coli-geonwoo merged commit ebcd17c into develop Jan 17, 2025
5 checks passed

coli-geonwoo deleted the refactor/#48 branch January 17, 2025 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REFACTOR] Repository 상속 객체 전환 #53

[REFACTOR] Repository 상속 객체 전환 #53

leegwichan commented Jan 13, 2025

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

leegwichan left a comment

leegwichan Jan 13, 2025

leegwichan Jan 13, 2025

unifolio0 Jan 13, 2025

coli-geonwoo Jan 14, 2025

unifolio0 Jan 14, 2025

leegwichan Jan 15, 2025

coli-geonwoo Jan 16, 2025

coli-geonwoo Jan 16, 2025 •

edited

Loading

leegwichan Jan 17, 2025

leegwichan Jan 13, 2025

coli-geonwoo Jan 14, 2025

unifolio0 commented Jan 13, 2025

unifolio0 left a comment

unifolio0 Jan 13, 2025

leegwichan Jan 15, 2025

unifolio0 Jan 16, 2025

coli-geonwoo Jan 16, 2025

unifolio0 Jan 13, 2025

coli-geonwoo left a comment •

edited

Loading

coli-geonwoo Jan 14, 2025

coli-geonwoo Jan 14, 2025

coli-geonwoo Jan 14, 2025

leegwichan Jan 15, 2025

coli-geonwoo commented Jan 14, 2025

leegwichan commented Jan 15, 2025

coli-geonwoo commented Jan 16, 2025

leegwichan commented Jan 17, 2025

coli-geonwoo left a comment

		public interface ParliamentaryTableRepository extends Repository<ParliamentaryTable, Long> {

		Optional<ParliamentaryTable> findById(long id);

	() -> assertThat(foundMember).isNotEmpty()
	() -> assertThat(foundMember).isPresent()

[REFACTOR] Repository 상속 객체 전환 #53

[REFACTOR] Repository 상속 객체 전환 #53

Conversation

leegwichan commented Jan 13, 2025

🚩 연관 이슈

🗣️ 리뷰 요구사항 (선택)

github-actions bot commented Jan 13, 2025 • edited Loading

Test Results

github-actions bot commented Jan 13, 2025 • edited Loading

📝 Test Coverage Report

leegwichan 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

coli-geonwoo Jan 16, 2025 • edited Loading

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

unifolio0 commented Jan 13, 2025

unifolio0 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

coli-geonwoo left a comment • edited Loading

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

coli-geonwoo commented Jan 14, 2025

leegwichan commented Jan 15, 2025

coli-geonwoo commented Jan 16, 2025

leegwichan commented Jan 17, 2025

coli-geonwoo left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

coli-geonwoo Jan 16, 2025 •

edited

Loading

coli-geonwoo left a comment •

edited

Loading