[JDBC 라이브러리 구현하기 - 4단계] 에버(손채영) 미션 제출합니다. #923

helenason · 2024-10-14T08:49:34Z

몰리 안녕하세요 ~~~~

벌써 마지막 미션의 마지막 단계네요!

요구사항은 모두 만족시켰는데, 제 방식이 옳은지 확신이 서지 않아요.

편하게 의견 남겨주세요!

감사합니다.

jminkkk

안녕하세요 에버!
이번 단계도 미션 잘 진행해주셨네요 😁

몇가지 코멘트 남겨보았으니 이번 단계도 파이팅입니다 🔥

jminkkk · 2024-10-15T02:29:08Z

jdbc/src/main/java/com/interface21/jdbc/core/ConnectionManager.java

-            throw new DataAccessException(e);
+        } finally {
+            DataSourceUtils.releaseConnection(conn, dataSource);
+            TransactionSynchronizationManager.unbindResource(dataSource);


DataSourceUtils.getConnection로 Connection을 가져올 때 내부에서는 TransactionSynchronizationManager.bindResource를 호출해주도록 하고 있는데요
unbindResource는 DataSourceUtils 내부에 위치시키지 않은 이유가 궁금해요!

사실 저도 그 부분을 고려해 DataSourceUtils 내부에 위치시킬까 하였는데요. 고민하다가 일단은 기존 작성되어있는 코드를 따르기로 했었습니다. 하지만 지금 코드를 다시 보니, 내부에 위치시키는 게 맞아보이네요!

jminkkk · 2024-10-15T02:34:48Z

jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java

    }

    public static void bindResource(DataSource key, Connection value) {
+        if (resources.get() == null) {
+            Map<DataSource, Connection> map = new HashMap<>();


ThreadLocal에는 withInitial 라는 유틸리티 메서드를 제공하고 있다는 사실...!

헉 너무 좋은데요? 👍

jminkkk · 2024-10-15T11:07:35Z

jdbc/src/test/java/com/interface21/jdbc/core/TransactionManagerTest.java

+    @DisplayName("실행을 실패할 경우 롤백한다.")
+    @Test
+    void should_rollbackLogic_when_manageFail() throws SQLException {
+        // given
+        when(dataSource.getConnection()).thenThrow(SQLException.class);
+
+        List<String> test = new ArrayList<>();
+
+        // when & then
+        assertThatThrownBy(() -> {
+            transactionManager.manage(conn -> {
+                test.add("new");
+            });
+        }).isInstanceOf(CannotGetJdbcConnectionException.class);
+        assertThat(test).hasSize(0);
+    }


꼼꼼한 테스트 좋네요 굿 👍👍

jminkkk · 2024-10-15T11:07:49Z

jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java

+        if (resources.get() == null) {
+            return null;
+        }
+        return resources.get().get(key);


resources.get()에 key가 존재하지 않을 경우에는 NPE가 터질 일은 없을까요...??

사용하는 쪽에서 null 검증을 해주고 있기 때문에 문제 없다고 생각하긴 했지만 안일했던 것 같네요. @Nullable 어노테이션으로 명시함으로써 NPE 발생 가능성을 줄여보았습니다. 혹시 더 좋은 방법이 있을까요오 🧐

앗 제가 잘못 리뷰드렸네요 ㅎㅎ...
map에 해당하는 key가 없는 경우 npe가 아니라 null을 반환하네요. 제가 착각했습니다...ㅎㅎ

@Nullable 최고에요 👍👍

jminkkk · 2024-10-15T11:17:37Z

app/src/test/java/com/techcourse/service/support/MockUserHistoryDao.java

@@ -1,10 +1,9 @@
-package com.techcourse.service;
+package com.techcourse.service.support;


support 패키지 안으로 이동하신 이유가 궁금해요!
main 패키지 하위에도 support가 있어서 자칫 main.~.support에 대한 테스트로 오해할 수도 있을 것 같은데 에버의 생각이 궁금합니다!

저의 원래 의도는 jdbc 모듈 테스트 패키지 내부의 support와 같은 패키지였습니다! 하지만 몰리의 의견대로 오해의 소지가 있겠네요! 테스트를 돕는 도구이니 helper 정도로 네이밍해볼까 합니다 ㅎㅎ

helenason · 2024-10-15T16:07:36Z

몰리~~~

리뷰 반영하고 학습테스트 추가해서 리뷰 재요청 드립니다!

이번 학습테스트는 구현에 집중했던 것 같아요.
그래서 혹시 학습테스트 진행하시면서 학습했던 키워드 있다면 던져주시면 감사하겠습니다!

이번 리뷰도 잘 부탁드려요 :)

jminkkk

에버 안녕하세요 ㅎㅎ
리뷰가 늦었네요 ㅠㅠ

이번이 마지막 요청일 것 같아요!
미션 코드에 대해서는 잘 반영해주셔서 학습 테스트 부분에 사소한 코멘트 남겼으니 확인해주세요!
마지막까지 파이팅입니다 🔥

jminkkk · 2024-10-17T10:40:28Z

study/src/test/java/aop/stage0/TransactionHandler.java

+        if (!target.getClass().getMethod(method.getName(), method.getParameterTypes())
+                .isAnnotationPresent(Transactional.class)) {


Suggested change

if (!target.getClass().getMethod(method.getName(), method.getParameterTypes())

.isAnnotationPresent(Transactional.class)) {

if (!method.isAnnotationPresent(Transactional.class)) {

Transactional 어노테이션이 없는 경우를 판단하는 로직 같아요!
Method 클래스의 isAnnotationPresent를 사용하면 가독성이 좋아질 것 같은데 어떤가요?ㅎㅎ

실제 구현체(target)가 가지고 있는 method에서 어노테이션 검증을 하는 방식이 아닌, 파라미터로 받은 method를 통해 검증하게 되면 interface의 메서드를 검증하게 되어 @Transactional이 없다는 결과가 출력돼요! 따라서 위와 같이 target 구현체의 method를 사용하게 되었습니다 :)

jminkkk · 2024-10-17T10:41:50Z

study/src/test/java/aop/stage1/TransactionAdvice.java

+            return ret;
+        } catch (Exception e) {
+            platformTransactionManager.rollback(transactionStatus);
+            log.info("transaction roll back: {}", invocation.getMethod().getName());


로그 굿 👍👍👍
사소하지만 invocation.getMethod().getName()가 여러번 사용되는 것 같아, 변수로 추출해도 좋을 것 같네요 ㅎㅎ

jminkkk · 2024-10-17T10:43:17Z

study/src/test/java/aop/stage2/AopConfig.java

+    @Bean
+    public DefaultAdvisorAutoProxyCreator defaultAdvisorAutoProxyCreator() {
+        DefaultAdvisorAutoProxyCreator proxyCreator = new DefaultAdvisorAutoProxyCreator();
+        proxyCreator.setProxyTargetClass(true);


setProxyTargetClass() 설정이 false, true 인 경우 각각 어떤 상황이 벌어지나요?

proxyTragetClass 설정이 true인 경우 CGLIB, false인 경우 JDK Proxy를 사용하는 것으로 알고 있습니다!

jminkkk · 2024-10-17T10:52:58Z

jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java

+        if (resources.get() == null) {
+            return null;
+        }
+        return resources.get().get(key);


앗 제가 잘못 리뷰드렸네요 ㅎㅎ...
map에 해당하는 key가 없는 경우 npe가 아니라 null을 반환하네요. 제가 착각했습니다...ㅎㅎ

@Nullable 최고에요 👍👍

helenason · 2024-10-17T17:41:10Z

몰리 마지막까지 감사했습니다!

남은 기간 파이팅이에요 👊

helenason added 5 commits October 14, 2024 17:13

refactor: 트랜잭션 동기화 적용

9684780

refactor: UserService 추상화

f806693

test: AppUserService

7acffa0

refactor: 사용 않는 코드 제거

9ff51b9

refactor: 패키지 이동

d07bce5

helenason self-assigned this Oct 14, 2024

jminkkk requested changes Oct 15, 2024

View reviewed changes

helenason added 8 commits October 15, 2024 20:30

test: 학습테스트 JDK Dynamic Proxy

7991eca

test: 학습테스트 ProxyFactoryBean

7c423ea

refactor: TransactionAdvice target 필드 제거

7325402

refactor: AOP 추가

5db8014

refactor: 트랜잭션 리소스 해제 로직 DataSourceUtils 내부로 이동

52a5b08

refactor: ThreadLocal 초기화 방식 변경

3e2d23c

refactor: 반환값 nullable 명시

b58cf3e

refactor: 테스트 돕는 패키지명 변경

cb7f8c0

jminkkk requested changes Oct 17, 2024

View reviewed changes

refactor: 메서드명 변수 추출

7655bb8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JDBC 라이브러리 구현하기 - 4단계] 에버(손채영) 미션 제출합니다. #923

[JDBC 라이브러리 구현하기 - 4단계] 에버(손채영) 미션 제출합니다. #923

helenason commented Oct 14, 2024

jminkkk left a comment

jminkkk Oct 15, 2024

helenason Oct 15, 2024

jminkkk Oct 15, 2024

helenason Oct 15, 2024

jminkkk Oct 15, 2024

jminkkk Oct 15, 2024

helenason Oct 15, 2024

jminkkk Oct 17, 2024

jminkkk Oct 15, 2024

helenason Oct 15, 2024

helenason commented Oct 15, 2024

jminkkk left a comment

jminkkk Oct 17, 2024

helenason Oct 17, 2024

jminkkk Oct 17, 2024

jminkkk Oct 17, 2024

helenason Oct 17, 2024

jminkkk Oct 17, 2024

helenason commented Oct 17, 2024

		@@ -1,10 +1,9 @@
		package com.techcourse.service;
		package com.techcourse.service.support;

		if (!target.getClass().getMethod(method.getName(), method.getParameterTypes())
		.isAnnotationPresent(Transactional.class)) {

	if (!target.getClass().getMethod(method.getName(), method.getParameterTypes())
	.isAnnotationPresent(Transactional.class)) {
	if (!method.isAnnotationPresent(Transactional.class)) {

[JDBC 라이브러리 구현하기 - 4단계] 에버(손채영) 미션 제출합니다. #923

Are you sure you want to change the base?

[JDBC 라이브러리 구현하기 - 4단계] 에버(손채영) 미션 제출합니다. #923

Conversation

helenason commented Oct 14, 2024

jminkkk 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

helenason commented Oct 15, 2024

jminkkk 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

helenason commented Oct 17, 2024