[JDBC 라이브러리 구현하기 - 3,4단계] 에코(조은찬) 미션 제출합니다. #548

echo724 · 2023-10-09T01:27:44Z

안녕하세요 연어!
주말에 조금 바빠서 3,4단계 동시에 진행했습니다~~~ ㅜㅜ
이번 단계에서는 1. Spring Docs에 나온 JDBCTemplate API 적극 활용(Callback) 2. 트랜젝션 미션적용 이었던 것 같습니다.
트랜잭션 부분도 API docs를 보고 추상화를 해보려고 했지만 학습할 내용이 많아 미션 구현 위주로 하였습니다.

아래는 커멘트에 대한 제 생각입니다!

update를 제외한 모든 메서드명이 query인데 사용자는 시그니처를 보고 용도를 파악할 수 있을 것 같네요. 하지만 메서드명도 좀 더 구체적이면 더 파악하기 쉽지 않을까요??
→ Spring Docs를 참고하여 메서드를 만들어서 그런 것 같습니다! docs에서도 수많은 query, update, execute 메서드들과 몇몇 추가 메서드를 제공하더라구요!
PreparedStatementSetter를 사용하는 버전과 가변인자를 사용하는 버전이 둘 다 존재합니다. 레거시 호환성을 위해서 남겨두신 걸까요?? 만약 그렇다면 남겨두는 것은 좋아요! 하지만 현재의 UserDao 로직은 가변 인자를 사용하도록 리팩토링해봐도 좋을 것 같다고 생각합니다.
→ 말씀하신대로 레거시를 최대한 유지해보자는 생각으로 리펙토링을 해서 그렇습니다! 특히나 앱이 아닌 라이브러리를 만들고 있는 상황이기 때문에 public 메서드의 시그니쳐 변경은 큰 비용을 가져온다고 생각해요. 그래서 가변인자도 받는 메서드들을 오버로딩 해주었습니다!

마지막 단계 리뷰 잘 부탁 드립니다. 감사합니다~~~!

nuyh99

에코 반가워요!
3, 4단계 통째로 구현하느라 힘드셨을 것 같아요ㅠㅠ
저도 요즘 너무 바쁜 것 같네요...

Spring Data JDBC를 열심히 공부하신 것 같은 느낌을 받았습니다.
미션 요구 사항을 잘 지키신 것 같아요!!

제 미션할 때의 기억 중 이 부분은 추상화하면 좋겠다라고 생각한 것들을 함수형 인터페이스로 잘 만들어주셨더라구요!!
덕분에 저도 관련 문서들 찾아보면서 공부할 수 있었습니다.

이번에도 마찬가지로 궁금한 점들을 코멘트로 남겨봤습니다!!
머지할까 고민했는데 저도 궁금증을 해소하고 싶기도 하고 얼마 안 남은 우테코 미션이라서 마지막까지 최대한 같이 성장하고 싶어요.
감사합니다!!

nuyh99 · 2023-10-09T14:10:09Z

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

+        return update(sql, pstmt -> {
+            for (int i = 0; i < args.length; i++) {
+                pstmt.setObject(i + 1, args[i]);
            }
-        } catch (final SQLException e) {
-            throw new DataAccessException(e);
-        }
+        });


가변인자로부터 PreparedStatementSetter를 만드는 로직이 공통적으로 보이는데 메서드화할 수 있을 것 같아요!

옷 좋습니다!

nuyh99 · 2023-10-09T14:35:42Z

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

-    public <T> List<T> query(final String sql, final RowMapper<T> rowMapper) throws DataAccessException {
-        try (final Connection connection = dataSource.getConnection();
-             final PreparedStatement pstmt = connection.prepareStatement(sql)) {
+    public <T> List<T> queryForList(final String sql, final RowMapper<T> rowMapper, Object... args) throws DataAccessException {


queryForList의 시그니처를 봤을 때 SQL에 필요한 파라미터들을 가변인자로 받겠구나 하고 생각했어요.
하지만 실제 쓰임은 PreparedStatementSetter를 받아서 args라는 변수명으로 쓰이고 있네요!
최종적으로는 execute(sql, preparedStatementCallBack) -> execute(connectionCallBack)을 호출하면서 메서드의 호출이 종료됩니다.

query(String, RowMapper, Object...)를 호출하면 query, queryForList, query, execute, execute 의 순서로 5번의 내부호출이 되는 것 같네요...??!(클론해보지 않아서 확실하지는 않습니다)

지금 봤을 때는

호출되는 과정에서 계속해서 익명 객체가 생성됩니다

로직의 흐름을 따라가려니 클래스의 아래까지 갔다가 다시 위의 메서드를 보는 등 한 눈에 보이지는 않습니다

등의 단점이 있다고 생각해요. 하나씩 오버로딩 버전을 넣는 과정에서 불필요한 체이닝이 발생하고 중복 코드가 발생했을 수 있을 것 같네요.
현재와 같이 구현한 특별한 이유가 있을 지 궁금합니다.

개인적으로 에코라면 충분히 기존 메서드의 호환성을 유지하면서도 체이닝의 이점을 누리는 식으로 리팩토링할 수 있을 것 같은데 어떤가요??

연어 얘기해주신대로, 기존에 메서드들이 오버로딩이 많이 되어있어서 저도 헷갈렸더라구요 그래서 기존 로직에서 중복되는 코드는 최대한 줄였고, 외부에서 Object나 List를 가져오는 경우 queryForList, queryForObject로 직접 명시하였습니다. 그래서 현재 구조는

update -> execute

queryForObject -> query -> execute

queryForList -> query -> execute

이런식으로 호출 순서를 줄였습니다. 다만 가변인자의 경우에는 여전히 호환성을 유지하기 위해 PreparedStatementSetter를 인자라로 받는 동일한 queryForObject, queryForList를 사용하도록 했습니다.

nuyh99 · 2023-10-09T14:37:11Z

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

+
+@FunctionalInterface
+public interface ConnectionCallBack<T> {
+    T doInConnection(Connection con) throws SQLException;


커넥션을 맺고 정리하는 과정을 템플릿화하기 위해서 중간의 로직을 추상화했군요.
굿굿!!

nuyh99 · 2023-10-09T14:42:45Z

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

+
+@FunctionalInterface
+public interface PreparedStatementCallback<T> {
+    T doInPreparedStatement(PreparedStatement ps) throws SQLException;


이 인터페이스는 하나의 PreparedStatement에서 여러 executeUpdate 등을 실행시키기 위한 추상화네요.(https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/jdbc/core/PreparedStatementCallback.html)
다만 현재의 쓰임에서는 필요한가?라는 개인적인 의문이 들기도 합니다...!

어떤 근거로 무슨 목적을 위해 사용하게 되셨는지 알려주세요!

말씀하신대로 executeUpdate나 executeQuery를 상위 메서드에서 호출하도록 할 수 있게 해줍니다. 또한 파라미터 설정과, ResultSet 꺼내오는 작업 등 PreparedStatement 사용을 query와 update 메서드에서 다르게 설정할 수 있도록 해주죠. 다만 오히려 ConnectionCallback이 하는 역할이 별로 없어서 삭제해주었습니다.

nuyh99 · 2023-10-09T14:46:28Z

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

 import java.util.Map;

 public abstract class TransactionSynchronizationManager {

    private static final ThreadLocal<Map<DataSource, Connection>> resources = new ThreadLocal<>();

-    private TransactionSynchronizationManager() {}
+    static {
+        resources.set(new LinkedHashMap<>());


HashMap이 아닌 LinkedHashMap을 사용하신 이유가 궁금합니다!

앗 따로 없습니다! 기본 해시맵을 깜빡했네요!

nuyh99 · 2023-10-09T14:51:24Z

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

+        userHistoryDao.log(new UserHistory(user, createBy));
+    }
+
+    public User findById(final long id) {


@Override가 있으면 더 좋을 것 같아요!

nuyh99 · 2023-10-09T14:54:32Z

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

+    }
+
+    @Override
+    public void insert(final User user) {


AppUserService인데 이 메서드는 여전히 인프라 코드를 가지고 있네요!

잉 마지막 커밋이 안올라간거 같네요! 수정해두겠습니다;;

nuyh99 · 2023-10-09T14:57:10Z

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

+            throw new DataAccessException(e);
+        } finally {
+            try {
+                connection.setAutoCommit(true);


재사용할 게 아니라 어차피 release할 자원이라서요.
커넥션을 다시 Auto Commit으로 변경해줄 필요가 있을 지 궁금해요!

그럴꺼 같네요! 말씀하신대로 바로 release를 해주기 때문에 필요 없을 것 같습니다!

nuyh99 · 2023-10-09T14:57:47Z

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

+        try {
+            return userService.findById(id);
+        } catch (final Exception e) {
+            throw new RuntimeException(e);


RuntimeException보다는 좀 더 인프라 관련 예외이면 좋을 것 같습니다!

앗 그렇군요. 다만 이 메서드의 경우 따로 트랜젝션을 걸어줄 필요가 없다고 생각되어 트랜젝션 경계 부분은 제거했습니다!

nuyh99 · 2023-10-09T15:02:04Z

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

+        });
+    }
+
+    public Connection getConnection() throws DataAccessException {


저는 getConnection이 DataSourceUtils의 책임이라고 생각하는데 필요한 메서드인지 궁금합니다!

앗 옛날에 사용한 메서드인데 테스트에서 사용하느라 계속 있었던 것 같네요 필요 없을 것 같습니다~

echo724 · 2023-10-10T15:49:02Z

안녕하세요 연어! 상세한 리뷰 감사드려요..!!! 놓친 부분도 많고 개선할 부분도 많았는데 잘 짚어주셔서 손볼 수 있었던 것 같네요! 커멘트로 답변 달아두었습니다! 리뷰 잘 부탁드려요~~!

nuyh99

에코 JDBC 미션 고생 많으셨습니다!!
바쁘신 와중 반영 엄청 잘해주셨네요...ㄷㄷ

좀 더 신경써서 리뷰 남기고 싶었는데 저도 현생이 바빠서 제대로 못 해드린 것 같기도 해요...도움이 많이 못 되어 아쉽습니다ㅠ

지금 당장 눈에 보이는 것들은 전반적으로 최고입니다!!
간단한 코멘트를 남기기는 하지만 반영보다는 그냥 확인만 해주시면 좋을 것 같아요!
어푸루브 날리겠습니다.

앞으로도 화이팅!!!!

nuyh99 · 2023-10-11T09:46:32Z

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

+        });
+    }
+
+    private <T> T execute(final String sql, final PreparedStatementCallback<T> preparedStatementCallback) throws DataAccessException {


이 메서드는 위에서 두 번째 update 메서드 아래에 위치하면 좋을 것 같아요!

nuyh99 · 2023-10-11T09:47:13Z

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

        }
    }

-    private ResultSet executeQuery(final Object[] args, final PreparedStatement pstmt) throws SQLException {
+    private void setPreparedStatementWithArgs(final Object[] args, final PreparedStatement pstmt) throws SQLException {


얘는 첫 번째 queryForObject 아래에 위치하면...! 굿굿

nuyh99 · 2023-10-11T09:50:48Z

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


    public static Connection getResource(DataSource key) {
-        return null;
+        return resources.get().get(key);


resources.get()

모든 메서드에서 위의 로직으로 Map 인스턴스를 참조하고 있는데 메서드로 분리하면 좋을 것 같아요!

nuyh99 · 2023-10-11T09:51:52Z

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

    }

    public static void bindResource(DataSource key, Connection value) {
+        resources.get().put(key, value);


조금만 더 가독성을 챙기자면...!
String.format()으로 에러 메세지를 한 눈에 읽히도록 할 수 있을 것 같아요!!

nuyh99 · 2023-10-11T09:53:20Z

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

@@ -29,6 +30,10 @@ public static Connection getConnection(DataSource dataSource) throws CannotGetJd

    public static void releaseConnection(Connection connection, DataSource dataSource) {
        try {
+            final Connection resource = TransactionSynchronizationManager.getResource(dataSource);
+            if (resource.equals(connection)) {


조건이 꼼꼼하네요...! 굿굿

nuyh99 · 2023-10-11T09:58:22Z

jdbc/src/test/java/nextstep/jdbc/JdbcTemplateTest.java

@@ -23,8 +23,8 @@ class JdbcTemplateTest {

    @BeforeEach
    public void setUp() {
-        final DataSource instance = DataSourceConfig.getInstance();
-        try (final Connection connection = instance.getConnection()) {
+        Connection connection = DataSourceUtils.getConnection(DataSourceConfig.getInstance());


이 커넥션은 DDL을 실행하기 위한 커넥션이군요!
지금은 DataSourceUtils.getConnection으로 서비스 레이어에서 커넥션을 얻어서 쓰기 때문에 재사용이 가능한 것 같아요!

하지만 전체 테스트를 돌릴 때에는 마지막으로 releaseConnection이 호출되지 않으면 계속 커넥션이 살아있을 수도 있을 것 같네요!

@AfterAll 등으로 정리해주는 것도 좋다고 생각됩니다.

echo724 added 8 commits October 9, 2023 00:23

refactor: update에 가변인자를 받는 메서드 추가 및 try-with-resources 사용

6621ac8

refactor: Callback과 ResultSetExtractor을 이용해 try-with-resources 구문 분리

2445a64

feat: Transaction 경계 구현

0df3c32

refactor: 서비스에서 트랜젝션 구현을 위해 Connection 파라미터 추가

a4e763d

feat: TransactionSynchronizationManager 및 DataSourceUtils 사용

986128c

refactor: DataSourceUtils 사용으로 불필요한 Connection parameter 제거

58dd6a6

refactor: 트랜젝션 서비스 추상화

77e4929

refactor: 리뷰 반영

7531c53

nuyh99 self-requested a review October 9, 2023 14:58

nuyh99 requested changes Oct 9, 2023

View reviewed changes

echo724 added 4 commits October 10, 2023 23:04

refactor: 불필요한 getConnection 메서드 제거

07573b8

refactor: JdbcTemplate 메서드 정리

e23caa6

refactor: 인프라 코드 제거

5300ee4

fix: UserService 수정

48437c8

echo724 requested a review from nuyh99 October 10, 2023 15:47

nuyh99 approved these changes Oct 11, 2023

View reviewed changes

nuyh99 merged commit 20ffa66 into woowacourse:echo724 Oct 11, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JDBC 라이브러리 구현하기 - 3,4단계] 에코(조은찬) 미션 제출합니다. #548

[JDBC 라이브러리 구현하기 - 3,4단계] 에코(조은찬) 미션 제출합니다. #548

echo724 commented Oct 9, 2023

nuyh99 left a comment

nuyh99 Oct 9, 2023

echo724 Oct 10, 2023

nuyh99 Oct 9, 2023

echo724 Oct 10, 2023

nuyh99 Oct 9, 2023

nuyh99 Oct 9, 2023

echo724 Oct 10, 2023

nuyh99 Oct 9, 2023

echo724 Oct 10, 2023

nuyh99 Oct 9, 2023

nuyh99 Oct 9, 2023

echo724 Oct 10, 2023

nuyh99 Oct 9, 2023

echo724 Oct 10, 2023

nuyh99 Oct 9, 2023

echo724 Oct 10, 2023

nuyh99 Oct 9, 2023

echo724 Oct 10, 2023

echo724 commented Oct 10, 2023

nuyh99 left a comment

nuyh99 Oct 11, 2023

nuyh99 Oct 11, 2023

nuyh99 Oct 11, 2023

nuyh99 Oct 11, 2023

nuyh99 Oct 11, 2023

nuyh99 Oct 11, 2023

[JDBC 라이브러리 구현하기 - 3,4단계] 에코(조은찬) 미션 제출합니다. #548

[JDBC 라이브러리 구현하기 - 3,4단계] 에코(조은찬) 미션 제출합니다. #548

Conversation

echo724 commented Oct 9, 2023

nuyh99 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

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

echo724 commented Oct 10, 2023

nuyh99 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