[JDBC 라이브러리 구현하기 - 4단계] 히이로(문제웅) 미션 제출합니다. #569
Conversation
| try { | ||
| Connection conn = DataSourceUtils.getConnection(dataSource); | ||
| try { | ||
| conn.setAutoCommit(false); | ||
| userService.changePassword(id, newPassword, createBy); | ||
| conn.commit(); | ||
| } catch (SQLException e) { | ||
| conn.rollback(); | ||
| throw e; | ||
| } | ||
| } catch (SQLException e) { | ||
| throw SQLExceptionTranslator.translate("", e); | ||
| } finally { | ||
| DataSourceUtils.releaseConnection(dataSource); | ||
| } |
There was a problem hiding this comment.
현재 트랜잭션이 적용된 메소드를 보면 다음과 같습니다.
try {
Connection conn = DataSourceUtils.getConnection(dataSource);
try {
conn.setAutoCommit(false);
// 서비스 로직
conn.commit();
} catch (SQLException e) {
conn.rollback();
throw e;
}
} catch (SQLException e) {
throw SQLExceptionTranslator.translate("", e);
} finally {
DataSourceUtils.releaseConnection(dataSource);
}여기서 보면 저희가 앞으로 트랜잭션을 적용하는 메소드가 생기게 되면 //서비스 로직 이외에는 중복되는 로직을 작성해야할 텐데요 이부분을 jdbcTemplate 리팩토링했던 것처럼 추출해서 관리하면 좋을 것 같습니다. 리팩토링 하실 때 여러 서비스가 생기는 상황도 고려해서 분리해보시면 좋을 것 같아요! 😊
There was a problem hiding this comment.
아코가 리뷰해주신 것처럼 해당 부분에도 템플릿 콜백 패턴을 적용해보려고 고민을 좀 해봤었어요. 그런데 JdbcTemplate 때와는 다르게 템플릿 메서드가 반환 타입이 있는 경우와 없는 경우로 나뉘어 질 것 같아서 현재 TxUserService 수준에서 굳이 필요한 패턴 적용일까에 대한 의문이 들었기에 적용하지 않았었는데요, 아코가 확장성을 생각해보라고 해주신 김에 적용해보았습니다! 확실히 서비스 메서드가 늘어나면 늘어날 수록 패턴을 적용한 구조가 트랜잭션 관련 코드 중복을 제거하기에는 용이할 것 같네요 😄
| @Override | ||
| public User findById(long id) { | ||
| return userService.findById(id); | ||
| } | ||
|
|
||
| @Override | ||
| public void insert(User user) { | ||
| userService.insert(user); | ||
| } |
There was a problem hiding this comment.
혹시 이 메소들에는 트랜잭션을 적용해주시지 않은 이유가 있으신가요? 🤔
There was a problem hiding this comment.
해당 부분은 미션 최소 요구사항인 changePassword 테스트 통과 기준이 아니었다 보니 누락한 것 같습니다 ㅎㅎ... 이번에 템플릿 콜백 패턴을 적용하면서 함께 적용될 수 있도록 리팩토링했어요!
| public <T> List<T> query(final String sql, final RowMapper<T> rowMapper, boolean closeResource, Object... args) { | ||
| PreparedStatementCallback<List<T>> callBack = pstmt -> { | ||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
| return extractResultSet(pstmt.executeQuery(), rowMapper); | ||
| }; | ||
| return executeQuery(callBack, sql, closeResource, args); |
There was a problem hiding this comment.
저번 3단계 마지막 리뷰이긴한데 따로 반영이나 다른 답변이 없으셔서 다시 한번 남깁니다!
현재에는 query의 Object가 필수로 들어가야 하는 상황인데
Select id, account, password from user도 고려해서 Object... ags에 @Nullable을 붙어주면 좋을 것 같아요!
There was a problem hiding this comment.
아코가 지난 단계 PR을 merge 해주시면서 추가로 달아주신 리뷰들을 제가 미처 보지 못한 것 같네요 😭
해당 부분은 사용자가 인자를 넘기지 않으면 Object[] 처럼 빈 배열로 들어가기 때문에 따로 생각해보지 못한 부분이었는데요, 빈 배열로써 사용되게 될텐데 여기에 @nullable을 선언해주는 것이 옳을까?라는 의문이 들더라구요.
그래서 좀 더 생각을 해봤는데 가변인자로 생성되는 배열 자체에 대한 값이 null일 수 있다는 의미가 아니라 배열에 담기는 값들 중에 null이 섞여있을 수 있다는 가능성을 시사하기 위해 선언되는 것이 아닐까 싶은 생각이 들었습니다. 실제로 JdbcTemplate 클래스에도 가변인자에 nullable 어노테이션이 선언되어 있기도 하구요. 저는 이러한 관점에서는 @nullable을 선언해주는 게 의미가 있을 수 있다고 생각이 드는데 아코 생각은 어떠신가요?
There was a problem hiding this comment.
아 일단 @nullable 선언은 반영했어요~!! 😄
There was a problem hiding this comment.
저도 히이로와 같은 생각입니다. 👍👍 (제 리뷰의 의도가 명확하지 않았던 것 같아요 ㅎㅎ) 저도 현재 라이브러리를 구현하는 만큼 사용자에게 명확한 가이드를 제공하는 것이 좋다고 생각을 했었어서 @Nullable을 붙이는 것에 대한 리뷰를 남겼었습니다.
| public int update(final String sql, boolean closeResources, Object... args) { | ||
| Connection conn = DataSourceUtils.getConnection(dataSource); | ||
| try (PreparedStatement pstmt = conn.prepareStatement(sql)) { | ||
| log.debug("query : {}", sql); | ||
|
|
||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
|
|
||
| return pstmt.executeUpdate(); | ||
| } catch (SQLException e) { | ||
| log.error(e.getMessage(), e); | ||
| throw SQLExceptionTranslator.translate(sql, e); | ||
| } | ||
| } | ||
|
|
||
| public int update(final Connection conn, final String sql, Object... args) { | ||
| try ( | ||
| PreparedStatement pstmt = conn.prepareStatement(sql) | ||
| ) { | ||
| log.debug("query : {}", sql); | ||
|
|
||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
|
|
||
| return pstmt.executeUpdate(); | ||
| } catch (SQLException e) { | ||
| log.error(e.getMessage(), e); | ||
| throw SQLExceptionTranslator.translate(sql, e); | ||
| } finally { | ||
| if (closeResources) { | ||
| DataSourceUtils.releaseConnection(dataSource); | ||
| } |
There was a problem hiding this comment.
이 부분도 저번 3단계 마지막 리뷰이긴한데 따로 반영이나 다른 답변이 없으셔서 다시 한번 남깁니다!
update 메소드도 현재 구현해 둔 executeQuery를 활용해 볼 수 있을 것 같습니다!
There was a problem hiding this comment.
요 부분은 이전 PR에서 아코가 일러주신대로 반영해봤습니다! 이래서 템플릿 - 콜백 패턴이 jdbcTemplate에서 적용되었구나 감을 잡을 수 있었던 것 같네요 ㅎㅎ 👏
| ) { | ||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
| private <T> T executeQuery(final PreparedStatementCallback<T> action, final String sql, boolean closeResources, Object... args) { |
There was a problem hiding this comment.
혹시 이번에 넣으신 closeResources는 transaction이 걸리지 않는 요청에 대해서 자원을 닫아주기 위해서 추가하신 것인가요?
궁금한 부분이 있는데 transaction이 걸리지 않고 jdbcTemplate으로만 요청이 걸리는 상황이 있을까요?
저는 데이터를 변경하는 것이 아닌 단건 데이터를 조회하는 것이라도 transaction의 격리수준을 보장받기 위해서 transaction을 적용할 것 같은데 히이로의 어떻게 생각하시나요?
There was a problem hiding this comment.
해당 부분은 connection을 얻어오는 파트에서 connection을 close하지 않아도 괜찮을까?라는 의문에서 구현하게 되었습니다. 트랜잭션을 여러 메서드에 걸쳐 수행하게 되면서 connection을 얻어오는 메서드에서 connection을 close하지 말아야 하는 상황이 발생했었는데요, 이 상황과는 별개로 resource를 얻은 파트에서 close의 책임을 아예 지지 않는 것이 옳은가에 대해서는 그렇다고 답변하기가 저는 개인적으로 힘들었습니다.
위와 같은 이유로 jdbcTemplate 메서드를 사용하는 사용자가 해당 메서드에서 획득하는 connection을 close할지 말지를 결정할 수 있도록 closeResources를 추가하게 되었습니다. 트랜잭션 유지로 인해 connection을 close 해야할 지, 말아야 할지 라이브러리 사용자로 하여금 한번 더 생각하게 만들 수 있도록 하는 역할도 같이 수행할 수 있을 것 같습니다. 실제로 구현된 jdbcTemplate에서는 connectionHolder를 통해 이를 관리할텐데 여기까지 비슷하게 구현해보기에는 시간이 좀 부족해서... 이런 방식으로 구현하게 되었습니다!
There was a problem hiding this comment.
저는 개인적으로는 closeRescoure 여부를 결정을 하게 하는 것은 사용자들이 고려해야하는 부분을 많이 제공하는 것 같습니다. 현재의 히이로의 코드에서 예시를 들어보겠습니다.
public User findById(final Long id) {
final var sql = "select id, account, password, email from users where id = ?";
return jdbcTemplate.queryForObject(sql, USER_ROW_MAPPER, false, id);
}현재 이와 같이 Rescourse close를 하지 않는 메소드가 있는데 사용자가 이를 직접 제어하려면 Rescourse를 close를 해주는 메소드로 구현이 되어야 한다는 것입니다.
public User findByIdNotResourceClose(final Long id) {
final var sql = "select id, account, password, email from users where id = ?";
return jdbcTemplate.queryForObject(sql, USER_ROW_MAPPER, false, id);
}
public User findByIdResourceClose(final Long id) {
final var sql = "select id, account, password, email from users where id = ?";
return jdbcTemplate.queryForObject(sql, USER_ROW_MAPPER, true, id);
}위와 같이 connection을 닫는 코드를 직접 제어를 해야하다보니 모든 메소드들에 대해서 2개씩 만들어 두고 이용해야한다는 단점이 있을 것 같습니다. 또한 이로 인해서 사용자의 실수를 자주 발생시키게 할 것 같습니다.
There was a problem hiding this comment.
지금 구현한 코드 상황에서는 제가 말씀드린 resource를 생성하는 부분에서 close를 명시하지 못하는 점과 아코가 말씀해주신 사용자가 고려해야할 부분이 많아진다는 점이 트레이드 오프가 될 수밖에 없는 것 같아요! 그래서 현재 스프링이 채택하고 있는 AOP를 활용한 트랜잭션 어노테이션이 등장하게 된게 아닐까 싶네요 ㅎㅎ 덕분에 깊게 생각해볼 수 있었어요 좋은 리뷰 감사합니다. 👍
| Connection conn = dataSource.getConnection(); | ||
| PreparedStatement pstmt = conn.prepareStatement(sql) | ||
| ) { | ||
| public int update(final String sql, boolean closeResources, Object... args) { |
There was a problem hiding this comment.
혹시 파라미터에 final을 붙이고 붙이지 않는 기준이 있으신가요? 어떤 파라미터는 final이 붙어있고 그렇지 않은 파라미터에 붙어있지 않아서 물어보았습니다.
There was a problem hiding this comment.
jdbcTemplate의 각 메서드 별로 콜백 익명함수를 정의하게 되면서 외부변수를 캡처해서 사용하게 되는 경우가 발생했습니다. 이 경우 외부 변수를 명시적인 final로 선언해주는 것이 좋기에 jdbcTemplate에 final 키워드를 적용하였습니다. closeResources 는 이후에 파라미터를 추가하게 되면서 제가 빠뜨렸네요 ㅎㅎ... 꼼꼼한 리뷰 감사합니다 아코!
| public <T> List<T> query(final String sql, final RowMapper<T> rowMapper, boolean closeResource, Object... args) { | ||
| PreparedStatementCallback<List<T>> callBack = pstmt -> { | ||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
| return extractResultSet(pstmt.executeQuery(), rowMapper); | ||
| }; | ||
| return executeQuery(callBack, sql, closeResource, args); |
There was a problem hiding this comment.
아코가 지난 단계 PR을 merge 해주시면서 추가로 달아주신 리뷰들을 제가 미처 보지 못한 것 같네요 😭
해당 부분은 사용자가 인자를 넘기지 않으면 Object[] 처럼 빈 배열로 들어가기 때문에 따로 생각해보지 못한 부분이었는데요, 빈 배열로써 사용되게 될텐데 여기에 @nullable을 선언해주는 것이 옳을까?라는 의문이 들더라구요.
그래서 좀 더 생각을 해봤는데 가변인자로 생성되는 배열 자체에 대한 값이 null일 수 있다는 의미가 아니라 배열에 담기는 값들 중에 null이 섞여있을 수 있다는 가능성을 시사하기 위해 선언되는 것이 아닐까 싶은 생각이 들었습니다. 실제로 JdbcTemplate 클래스에도 가변인자에 nullable 어노테이션이 선언되어 있기도 하구요. 저는 이러한 관점에서는 @nullable을 선언해주는 게 의미가 있을 수 있다고 생각이 드는데 아코 생각은 어떠신가요?
| public <T> List<T> query(final String sql, final RowMapper<T> rowMapper, boolean closeResource, Object... args) { | ||
| PreparedStatementCallback<List<T>> callBack = pstmt -> { | ||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
| return extractResultSet(pstmt.executeQuery(), rowMapper); | ||
| }; | ||
| return executeQuery(callBack, sql, closeResource, args); |
There was a problem hiding this comment.
아 일단 @nullable 선언은 반영했어요~!! 😄
| public int update(final String sql, boolean closeResources, Object... args) { | ||
| Connection conn = DataSourceUtils.getConnection(dataSource); | ||
| try (PreparedStatement pstmt = conn.prepareStatement(sql)) { | ||
| log.debug("query : {}", sql); | ||
|
|
||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
|
|
||
| return pstmt.executeUpdate(); | ||
| } catch (SQLException e) { | ||
| log.error(e.getMessage(), e); | ||
| throw SQLExceptionTranslator.translate(sql, e); | ||
| } | ||
| } | ||
|
|
||
| public int update(final Connection conn, final String sql, Object... args) { | ||
| try ( | ||
| PreparedStatement pstmt = conn.prepareStatement(sql) | ||
| ) { | ||
| log.debug("query : {}", sql); | ||
|
|
||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
|
|
||
| return pstmt.executeUpdate(); | ||
| } catch (SQLException e) { | ||
| log.error(e.getMessage(), e); | ||
| throw SQLExceptionTranslator.translate(sql, e); | ||
| } finally { | ||
| if (closeResources) { | ||
| DataSourceUtils.releaseConnection(dataSource); | ||
| } |
There was a problem hiding this comment.
요 부분은 이전 PR에서 아코가 일러주신대로 반영해봤습니다! 이래서 템플릿 - 콜백 패턴이 jdbcTemplate에서 적용되었구나 감을 잡을 수 있었던 것 같네요 ㅎㅎ 👏
| Connection conn = dataSource.getConnection(); | ||
| PreparedStatement pstmt = conn.prepareStatement(sql) | ||
| ) { | ||
| public int update(final String sql, boolean closeResources, Object... args) { |
There was a problem hiding this comment.
jdbcTemplate의 각 메서드 별로 콜백 익명함수를 정의하게 되면서 외부변수를 캡처해서 사용하게 되는 경우가 발생했습니다. 이 경우 외부 변수를 명시적인 final로 선언해주는 것이 좋기에 jdbcTemplate에 final 키워드를 적용하였습니다. closeResources 는 이후에 파라미터를 추가하게 되면서 제가 빠뜨렸네요 ㅎㅎ... 꼼꼼한 리뷰 감사합니다 아코!
| ) { | ||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
| private <T> T executeQuery(final PreparedStatementCallback<T> action, final String sql, boolean closeResources, Object... args) { |
There was a problem hiding this comment.
해당 부분은 connection을 얻어오는 파트에서 connection을 close하지 않아도 괜찮을까?라는 의문에서 구현하게 되었습니다. 트랜잭션을 여러 메서드에 걸쳐 수행하게 되면서 connection을 얻어오는 메서드에서 connection을 close하지 말아야 하는 상황이 발생했었는데요, 이 상황과는 별개로 resource를 얻은 파트에서 close의 책임을 아예 지지 않는 것이 옳은가에 대해서는 그렇다고 답변하기가 저는 개인적으로 힘들었습니다.
위와 같은 이유로 jdbcTemplate 메서드를 사용하는 사용자가 해당 메서드에서 획득하는 connection을 close할지 말지를 결정할 수 있도록 closeResources를 추가하게 되었습니다. 트랜잭션 유지로 인해 connection을 close 해야할 지, 말아야 할지 라이브러리 사용자로 하여금 한번 더 생각하게 만들 수 있도록 하는 역할도 같이 수행할 수 있을 것 같습니다. 실제로 구현된 jdbcTemplate에서는 connectionHolder를 통해 이를 관리할텐데 여기까지 비슷하게 구현해보기에는 시간이 좀 부족해서... 이런 방식으로 구현하게 되었습니다!
| try { | ||
| Connection conn = DataSourceUtils.getConnection(dataSource); | ||
| try { | ||
| conn.setAutoCommit(false); | ||
| userService.changePassword(id, newPassword, createBy); | ||
| conn.commit(); | ||
| } catch (SQLException e) { | ||
| conn.rollback(); | ||
| throw e; | ||
| } | ||
| } catch (SQLException e) { | ||
| throw SQLExceptionTranslator.translate("", e); | ||
| } finally { | ||
| DataSourceUtils.releaseConnection(dataSource); | ||
| } |
There was a problem hiding this comment.
아코가 리뷰해주신 것처럼 해당 부분에도 템플릿 콜백 패턴을 적용해보려고 고민을 좀 해봤었어요. 그런데 JdbcTemplate 때와는 다르게 템플릿 메서드가 반환 타입이 있는 경우와 없는 경우로 나뉘어 질 것 같아서 현재 TxUserService 수준에서 굳이 필요한 패턴 적용일까에 대한 의문이 들었기에 적용하지 않았었는데요, 아코가 확장성을 생각해보라고 해주신 김에 적용해보았습니다! 확실히 서비스 메서드가 늘어나면 늘어날 수록 패턴을 적용한 구조가 트랜잭션 관련 코드 중복을 제거하기에는 용이할 것 같네요 😄
| @Override | ||
| public User findById(long id) { | ||
| return userService.findById(id); | ||
| } | ||
|
|
||
| @Override | ||
| public void insert(User user) { | ||
| userService.insert(user); | ||
| } |
There was a problem hiding this comment.
해당 부분은 미션 최소 요구사항인 changePassword 테스트 통과 기준이 아니었다 보니 누락한 것 같습니다 ㅎㅎ... 이번에 템플릿 콜백 패턴을 적용하면서 함께 적용될 수 있도록 리팩토링했어요!
| PreparedStatementCallback<Integer> action = (pstmt) -> { | ||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
| return pstmt.executeUpdate(); | ||
| } catch (SQLException e) { | ||
| log.error(e.getMessage(), e); | ||
| throw SQLExceptionTranslator.translate(sql, e); | ||
| } finally { | ||
| if (closeResources) { | ||
| DataSourceUtils.releaseConnection(dataSource); | ||
| } | ||
| } | ||
| }; | ||
| return executeQuery(action, sql, closeResources, args); |
| ) { | ||
| ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args); | ||
| pss.setValues(pstmt); | ||
| private <T> T executeQuery(final PreparedStatementCallback<T> action, final String sql, boolean closeResources, Object... args) { |
There was a problem hiding this comment.
저는 개인적으로는 closeRescoure 여부를 결정을 하게 하는 것은 사용자들이 고려해야하는 부분을 많이 제공하는 것 같습니다. 현재의 히이로의 코드에서 예시를 들어보겠습니다.
public User findById(final Long id) {
final var sql = "select id, account, password, email from users where id = ?";
return jdbcTemplate.queryForObject(sql, USER_ROW_MAPPER, false, id);
}현재 이와 같이 Rescourse close를 하지 않는 메소드가 있는데 사용자가 이를 직접 제어하려면 Rescourse를 close를 해주는 메소드로 구현이 되어야 한다는 것입니다.
public User findByIdNotResourceClose(final Long id) {
final var sql = "select id, account, password, email from users where id = ?";
return jdbcTemplate.queryForObject(sql, USER_ROW_MAPPER, false, id);
}
public User findByIdResourceClose(final Long id) {
final var sql = "select id, account, password, email from users where id = ?";
return jdbcTemplate.queryForObject(sql, USER_ROW_MAPPER, true, id);
}위와 같이 connection을 닫는 코드를 직접 제어를 해야하다보니 모든 메소드들에 대해서 2개씩 만들어 두고 이용해야한다는 단점이 있을 것 같습니다. 또한 이로 인해서 사용자의 실수를 자주 발생시키게 할 것 같습니다.
| try { | ||
| try { | ||
| conn.setAutoCommit(false); | ||
| action.doInTransaction(); | ||
| conn.commit(); | ||
| } catch (SQLException e) { | ||
| conn.rollback(); | ||
| throw e; | ||
| } | ||
| } catch (SQLException e) { | ||
| throw SQLExceptionTranslator.translate("", e); | ||
| } finally { | ||
| DataSourceUtils.releaseConnection(dataSource); | ||
| } |
There was a problem hiding this comment.
이제 마지막 리뷰이네요!
이건 저도 받았던 리뷰인데 현재 SQLException만으로 rollback이 이루어지는 경우에 문제점은 어떤 것이 있을까요?
There was a problem hiding this comment.
runtime에서 발생하는 다른 예외들은 처리하지 못한다는게 단점이겠네요! 하지만 이번 미션에서 다룰 범위는 아니었다고 판단했기에 따로 더 생각해보지는 못했던 것 같아요! 마지막까지 생각할 부분 던져주셔서 감사합니다~ 🙇
안녕하세요 아코, 히이로입니다!
4단계에서 LMS에 나온 내용은 구현했으나 학습테스트에서 AOP 개념 까지 학습해서 Transactional 어노테이션을 구현해보고 싶었는데 시간 상 문제로 일단 제출하게 되었습니다. 오늘 안에 해보고 싶기는 한데... 한 번 노력해보겠습니다!
항상 정성스런 리뷰 감사합니다! 🙇