[JDBC 라이브러리 구현 3,4단계] 리오(오영택) 미션 제출합니다. #574

Jaeyoung22 · 2023-10-09T07:25:16Z

안녕하세요 바론!
연휴 마지막 잘 보내고 계신가요?!

3단계도 하긴 했는데 결국 4단계를 하니 전부 엎어지네요..

Test 관련해서 바론 의견대로 queryForObject에서 두개 이상의 결과가 있으면 예외를 반환하게 했더니 터지는 테스트가 있더라구요
그래서 deleteAll()을 구현했습니다!

참고해서 리뷰부탁드려요! 매번 좋은 리뷰 감사합니다!

somsom13

안녕하세요 리오! 바론입니다 🙇‍♀️ 🙇‍♀️

미션 구현하신거 잘 봤습니다!! 이번에도 역시 빠르게 구현해주셨더라구요 👍 👍
트랜잭션 관련해서 수정해야 할 것 같은 부분이 조금 있어서 코멘트 남겨두었습니다 ㅎㅎ

고생하셨어요 리오~!~!!

app/src/main/java/com/techcourse/dao/UserDao.java

somsom13 · 2023-10-09T10:54:53Z

app/src/main/java/com/techcourse/service/TxUserService.java

+    }
+
+    @Override
+    public User findById(long id) {


findById는 TransactionTemplate 으로 실행시키지 않으신 이유가 궁금해요~!!

AppUserService의 changePassword 내에서 findById를 호출하는 걸 보면, findById도 트랜잭션 내에서 실행시켜줘야 하지 않을까 싶은데 리오는 어떻게 생각하시나요?

그렇네요 제가 readonly=False의 느낌으로 트랜잭션을 설정했었는데, 바론 말대로 한 트랜잭션 내에서 진행이 되어야 겠군요... 감사합니다!

somsom13 · 2023-10-09T11:11:25Z

...src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java

+        if (dataSourceAndConnection == null) {
+            throw new CannotGetJdbcConnectionException("DataSource에 binding된 Connection이 없습니다.");
+        }


현재 스레드에 설정된 값이 없으면 예외를 발생시키게 하셨군용!

현재 스레드에 설정된 값이 아예 없을 때는 CannotGetJdbcConnectionException 을 발생시키지만, .get(dataSource) 의 결과가 없을 때는 그대로 null을 반환하도록 하신 이유가 궁금해요!

오 꼼꼼13님 감사합니다! 반영할게요!

...src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java

somsom13 · 2023-10-09T11:13:04Z

...src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java

+        if (resources.get() == null || !resources.get().containsKey(dataSource)) {
+            throw new IllegalArgumentException("등록된 DataSource가 없습니다.");


궁금해서 남기는 질문입니다!! 여기도 dataSource에 따른 Connection이 없을 때 예외를 발생시키신 이유가 궁금해요~! 저는 null을 그냥 아무 행동도 하지 않고 넘어가도록 null을 반환하는 방식으로 구현했었거든요 ㅎㅎ

해당 dataSource에 대해서 connection을 unbinding하라는 메서드가, connection이 binding되지 않은 datasource에 대해서 실행되도록 코드가 작성되어있다는 뜻이니까, 해당 부분의 로직을 수정하는 편이 더 안전하고 유지보수에도 편리할 것 같아요!
그래서 디버깅과 로깅의 목적으로도 필요할 것 같아서 구현했습니다!
바론의 의견은 어떠실까요?

somsom13 · 2023-10-09T11:14:00Z

jdbc/src/main/java/org/springframework/transaction/support/TransactionTemplate.java

+        }
+    }
+
+


개행이 한 줄 더 추가된 것 같아요~~

kkomkkom13 👍

somsom13 · 2023-10-09T11:21:24Z

jdbc/src/main/java/org/springframework/transaction/support/TransactionTemplate.java

+        this.dataSource = dataSource;
+    }
+
+    public void execute(Runnable runnable) {


리오 코드를 받아서 확인해보았는데, JdbcTemplate 에서는 아예 새로운 커넥션을 생성해서 사용하고 있는 것 같아요 ㅠㅠ

같은 트랜잭션 내에서 실행되기 위해서는 같은 커넥션을 공유해야 하는데, 콘솔에 찍어본 결과 서로 다른 Connection을 사용하고 있는 것 같더라구요!

JdbcTemplate 에서도 DataSourceUtils.getConnection() 을 사용하도록 변경하면 같은 스레드의 같은 트랜잭션에서는 동일한 Connection을 사용하도록 수정할 수 있을 것 같은데 어떻게 생각하시나용?

허허 이번 미션을 너무 쉽게 생각했군요....
커넥션 때문에 하루종일 머리 좀 썼습니다...
바론 덕분에 더 깊게 생각할 수 있었네요...! 고맙습니다!

멋지게 수정하셨군요 👍 👍 최곱니다~~~~~

중요하지는 않은 내용이지만, release를 finally 블럭에서 처리하도록 합칠 수 있지 않을까 싶기도 하네용!

리오 :
여기 왜 댓글이 안달릴까요...?
저도 이 부분 고민했었는데, 뭔가 트랜잭션 관련한 메서드에서 release를 명시하고 싶지 않아서 각 메서드에 따로따로 넣어주는 방법으로 진행했습니다! 바론의 의견도 좋은 방법이라고 생각해요!

Jaeyoung22 · 2023-10-10T08:15:08Z

제가 미션을 너무 쉽게 생각했군요...!

여기저기 의존성이 분리가 제대로 안 되어 있어서(예외 포함) 깔끔하게 정리했습니다..!
의존성을 모듈별로 분리하고 DataSourceUtils만 Transaction 모듈을 바라보게 했습니다!
해당 부분 참고해서 다시 리뷰 부탁드릴게요! 감사합니다!

somsom13

안녕하세요 리오! 바론입니다~~~~~

리뷰 멋지게 반영해주셨군요 👍 👍 제가 구현했던 것 보다 더 책임 분리가 잘 되게 구현하신 것 같아서 많이 배워갑니다 🙇

요구 사항은 모두 만족하신 것 같아서 approve, merge 합니다~!! 추가적인 코멘트 조금 남겼는데, 이 부분은 나중에 확인해주시면 좋을 것 같아요 😄

JDBC 미션 고생 많으셨습니다~!!! 💯 💯

somsom13 · 2023-10-10T08:17:33Z

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

@@ -13,6 +13,7 @@ public MockUserHistoryDao(final JdbcTemplate jdbcTemplate) {

    @Override
    public void log(final UserHistory userHistory) {
+        System.out.println("asdf");


앗 ㅎㅎ 🙈

앗! 이대로 머지가 되다니... 슬프군요... 죄송합니다....

앗.. 이럴 수가.. DM으로 의견 여쭤보고 머지할걸 그랬네용.. 저도 죄송합니다 🥲

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

somsom13 · 2023-10-10T08:37:33Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java


+        try {
+            PreparedStatement pstmt = connection.prepareStatement(sql);


PreparedStatement는 Connection에 의해 자동으로 닫히는 부분이 아닌 것으로 알고 있어요!

디버깅을 찍어보았을 때,

try-with-resources에 connection.prepareStatement(sql) 을 넣었을 경우: JdbcPreparedStatement의 close() 호출

일반 try-catch에 connection.prepareStatement를 넣었을 경우: JdbcPreparedStatement의 close() 호출되지 않음

인 것 같아요!! PreparedStatement는 이전 코드처럼 닫아주면 어떨까요?.?

앗 그렇군요...
https://www.phind.com/search?cache=h8csh4x43gnfdyz497l20af4
이것만 보고 PreparedStatement도 알아서 닫히겠거니 했는데 진짜 하나하나 확인해야겠네요...
감사합니다!

헉 어쩌면 제가 디버깅을 잘못한 걸 수도...! 요건 저도 더 확인해볼게요!!
좋은 참고자료 감사합니다 🙇‍♀️

jdbc/src/main/java/org/springframework/jdbc/datasource/DataSourceUtils.java

...src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java

somsom13 · 2023-10-10T09:16:15Z

jdbc/src/main/java/org/springframework/transaction/support/TransactionTemplate.java

+        this.dataSource = dataSource;
+    }
+
+    public void execute(Runnable runnable) {


멋지게 수정하셨군요 👍 👍 최곱니다~~~~~

somsom13 · 2023-10-10T09:21:45Z

jdbc/src/main/java/org/springframework/transaction/support/TransactionTemplate.java

+        this.dataSource = dataSource;
+    }
+
+    public void execute(Runnable runnable) {


중요하지는 않은 내용이지만, release를 finally 블럭에서 처리하도록 합칠 수 있지 않을까 싶기도 하네용!

리오 :
여기 왜 댓글이 안달릴까요...?
저도 이 부분 고민했었는데, 뭔가 트랜잭션 관련한 메서드에서 release를 명시하고 싶지 않아서 각 메서드에 따로따로 넣어주는 방법으로 진행했습니다! 바론의 의견도 좋은 방법이라고 생각해요!

Jaeyoung22 added 6 commits October 6, 2023 19:05

test: 학습테스트1 작성

2785499

test: 학습테스트2 작성

275bd89

feat: 트랜잭션 구현

a61dae9

feat: 서비스 분리

c13f3d6

feat: 서비스 분리

eac3a30

feat: deleteAll 구현

40a783e

somsom13 requested changes Oct 9, 2023

View reviewed changes

Jaeyoung22 added 6 commits October 10, 2023 11:07

refactor: 조회시에도 트랜잭션 설정

7ef6aed

fix: 예외사항 추가

f0104fc

chore: 개행 삭제

692ec95

fix: 트랜잭션 안에서 같은 커넥션을 사용하도록 수정

d44941f

refactor: 커넥션을 닫는 로직 변경

72752e2

test: 테스트 변경

f5ba5dd

somsom13 approved these changes Oct 10, 2023

View reviewed changes

somsom13 merged commit e2e7412 into woowacourse:jaeyoung22 Oct 10, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JDBC 라이브러리 구현 3,4단계] 리오(오영택) 미션 제출합니다. #574

[JDBC 라이브러리 구현 3,4단계] 리오(오영택) 미션 제출합니다. #574

Jaeyoung22 commented Oct 9, 2023

somsom13 left a comment

somsom13 Oct 9, 2023 •

edited

Loading

Jaeyoung22 Oct 10, 2023

somsom13 Oct 9, 2023 •

edited

Loading

Jaeyoung22 Oct 10, 2023

somsom13 Oct 9, 2023

Jaeyoung22 Oct 10, 2023

somsom13 Oct 9, 2023

Jaeyoung22 Oct 10, 2023 •

edited

Loading

somsom13 Oct 9, 2023

Jaeyoung22 Oct 10, 2023

somsom13 Oct 10, 2023

somsom13 Oct 10, 2023 •

edited by Jaeyoung22

Loading

Jaeyoung22 commented Oct 10, 2023

somsom13 left a comment

somsom13 Oct 10, 2023

Jaeyoung22 Oct 10, 2023

somsom13 Oct 10, 2023

somsom13 Oct 10, 2023 •

edited

Loading

Jaeyoung22 Oct 10, 2023

somsom13 Oct 10, 2023

somsom13 Oct 10, 2023

somsom13 Oct 10, 2023 •

edited by Jaeyoung22

Loading

		if (resources.get() == null \|\| !resources.get().containsKey(dataSource)) {
		throw new IllegalArgumentException("등록된 DataSource가 없습니다.");


		try {
		PreparedStatement pstmt = connection.prepareStatement(sql);

[JDBC 라이브러리 구현 3,4단계] 리오(오영택) 미션 제출합니다. #574

[JDBC 라이브러리 구현 3,4단계] 리오(오영택) 미션 제출합니다. #574

Conversation

Jaeyoung22 commented Oct 9, 2023

somsom13 left a comment

Choose a reason for hiding this comment

somsom13 Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somsom13 Oct 9, 2023 • 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

Jaeyoung22 Oct 10, 2023 • 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

somsom13 Oct 10, 2023 • edited by Jaeyoung22 Loading

Choose a reason for hiding this comment

Jaeyoung22 commented Oct 10, 2023

somsom13 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

somsom13 Oct 10, 2023 • 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

somsom13 Oct 10, 2023 • edited by Jaeyoung22 Loading

Choose a reason for hiding this comment

somsom13 Oct 9, 2023 •

edited

Loading

somsom13 Oct 9, 2023 •

edited

Loading

Jaeyoung22 Oct 10, 2023 •

edited

Loading

somsom13 Oct 10, 2023 •

edited by Jaeyoung22

Loading

somsom13 Oct 10, 2023 •

edited

Loading

somsom13 Oct 10, 2023 •

edited by Jaeyoung22

Loading