Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JDBC 라이브러리 구현하기 - 2, 3단계] 히이로(문제웅) 미션 제출합니다. #455

Merged
merged 10 commits into from
Oct 8, 2023

Conversation

MoonJeWoong
Copy link

@MoonJeWoong MoonJeWoong commented Oct 5, 2023

안녕하세요 아코! 히이로입니다.
명절에 재택까지 여러모로 다사다난한데 잘 지내고 계신가요? ㅎㅎ

이번 PR은 개인 일정에 치여서 좀 많이 늦었네요... 그래서 2단계에 3단계까지 해서 제출하게 되었습니다.
각 단계에서 초점을 맞춘 부분은 다음과 같아요.

2단계

  • JdbcTemplate을 보며 구현하다 보니 다른 부분들은 따로 더 리팩토링할 곳이 잘 보이지는 않더라구요...! 그래서 이번에는 SQLException을 DataAccessException으로 전환해주는 로직을 구현해보는데 집중해봤습니다.
  • 원래 제대로 구현했다면 각 DB 벤더사별로 반환하는 SQLException 에러코드 당 DAO Exception 클래스를 정의해줬어야 했을텐데 현재 미션 단위 프로젝트에서는 그 정도 수준의 구현은 불필요하겠다고 판단했습니다.
  • 그래서 현재 미션에서 사용하는 H2 DB에서 반환하는 SQLErrorCode들을 기반으로 SQLException들을 spring DAO Exception 혹은 Jdbc Exception 예외로 전환시켜주도록 구현했습니다. (사실 스플릿 PR을 보고 영감을 받은 부분이기도 해요 ㅎㅎ...)

3단계

  • UserService 테스트를 통과시키는데 집중했습니다. LMS에도 나와있지만 자연스럽게 Dao 내부로 캡슐화했던 DataSource와 Connection이 튀어나올 수 밖에 없겠더라구요 ㅎㅎ... 이 부분을 당연히 해결해봐야겠지만 해당 내용이 4단계에서 수행하게 되는 것 같아 일단 테스트만 통과 시키고 리뷰를 요청하게 되었습니다.

추가적으로 다른 크루들 리뷰를 보다보면 템플릿 패턴, 템플릿 콜백 패턴 등의 키워드로 얘기하는 게 종종 보였는데 시간이 없어서 저는 고려를 못해봤거든요... ㅎㅎ 아코는 혹시 생각을 좀 해보셨나요? 해보셨다면 해당 개념으로 어떤 부분을 또 리팩토링할 수 있을지 얘기해볼 수 있으면 좋을 것 같습니다 :)

제가 미처 생각하지 못했거나 누락된 부분이 있다면 리뷰 남겨주시거나 DM 주세요. 감사합니다! 🙇

@MoonJeWoong MoonJeWoong self-assigned this Oct 5, 2023
Copy link

@seokhwan-an seokhwan-an left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 히이로!

이번 단계로 깔끔하게 구현을 해주셔서 코드를 리뷰하는데 큰 어려움이 없었습니다.
또한 추가적으로 예외처리를 세분화해서 처리한 것에 배울점이 많았습니다!👍
히이로가 질문 주신 부분에 대한 답변과 히이로의 의견을 듣고 싶은 부분을 위주로 리뷰를 남겼습니다.
혹시 리뷰가 명확하게 이해가 되지 않으면 언제든지 DM 보내주세요!


public class UserService {

private final UserDao userDao;
private final UserHistoryDao userHistoryDao;
private final DataSource dataSource;
private final SQLExceptionTranslator sqlExceptionTranslator = new SQLExceptionTranslator();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번에 구현해 주신 SQLExceptionTranslator는 유틸 클래스가 아닌 객체로 만들어서 이용하시는 이유가 있으신가요?
저라면 객체를 생성하는 방식 보다는 유틸클래스로 이용하는 편이 더 좋은 방식인 것 같은데 히이로는 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 JdbcTemplate 코드에서는 SQLExceptionTranslator가 스프링 빈으로 등록이 되서 사용이 되어 static 클래스가 아니었기 때문에 이렇게 구현하고 나서 다시 생각해보진 못했네요...! 현 상황에서는 따로 빈으로 등록해서 사용하는 것도 아니기에 굳이 인스턴스화해서 사용할 필요는 없을 것 같아요 ㅎㅎ 유틸 클래스로 static 처리 했습니다!

Comment on lines 9 to 19
public DataAccessException translate(@Nullable String sql, SQLException e) {
Class<? extends DataAccessException> exceptionClazz =
H2SQLErrorCodeToDataAccessExceptionMapper.mapSQLErrorCode(e.getErrorCode());

try {
return exceptionClazz.getDeclaredConstructor(String.class, SQLException.class)
.newInstance(sql, e);
} catch (ReflectiveOperationException ex) {
return new DataAccessException("Some exception was thrown : " + e.getMessage());
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 translate 메소드를 보면 sql 필드는 @Nullable 을 이용해서 null이 들어올 수 있음을 암시해 두었는데 혹시 null이 들어올 수 있는 상황이 있는 것인가요?
밑에 execptionClazz의 경우 인스턴스를 생성하는 과정에서 sql이 꼭 필요한것 같아서요!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가적으로 리플랙션을 이용하는 과정에서 try-catch문을 이용해 주셨는데 ReflectiveOperationException이 발생하면 최종적으로 DataAccessException을 발생시키는 이유가 있을까요?
이 부분은 아무래도 translate 메소드가 DataAccessException을 반환하기 때문인 것 같은데요!
저는 이 부분이 분리가 되어서 다른 예외가 발생하는 해야하는 것 같은데 히이로는 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 스프링 라이브러리에 구현된 SQLErrorCodeSQLExceptionTranslator는 nullable로 받고 있어서 이유가 있겠거니 하고 따라했다가 잊어먹었네요 ㅎㅎ... 현재 미션 수준 코드에서는 굳이 필요없는 어노테이션이라는 생각에 동의합니다! 해당 부분은 수정했어요 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기에서 예외가 발생하는 경우는 기존 벤더사에 등록되어 있던 예외 코드와는 다른 예외가 발생했을 경우라고 생각했습니다. 그래서 이런 경우에도 라이브러리의 사용자는 발생한 SQLException의 예외 내용은 알 수 있어야 한다고 생각해서 이런 방향으로 구현해봤었습니다! 혹시 제가 생각하지 못한 부분이 더 있었다면 말씀해주세요~

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 그렇게 생각을 할 수 있을 것 같네요! 저는 리플랙션 부분이 실패해서 발생하는 예외이다 보니까 라이브러리 자체가 문제가 있는 것이라고 판단을 했어서 DataAccessException이 아닌 다른 커스텀 에러를 발생시켜야 한다고 생각을 했었습니다!

Comment on lines +14 to +22
private static final Map<Integer, Class<? extends DataAccessException>> mapper = new HashMap<>();

static {
setUpBadSqlGrammarCodes();
setUpDuplicationKeyException();
setUpDataIntegrityViolationException();
setUpDataAccessResourceFailureException();
setUpCannotAcquireLockException();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분 캐싱 둔 것 좋네요!👍👍
이 부분 엄청 고생하신 것 같습니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분 구현하실 때 자료 https://www.h2database.com/javadoc/org/h2/api/ErrorCode.html 이부분을 참고해서 하신 것인가요?
현재 자료를 보면 엄청 많은 예외처리 코드가 있는데 이 중에서 선별해서 넣으신 기준이 있으신가요? 이번에 추가하신 것에 보면 미션에서 발생할 수 있는 예외 외에도 부가적인 부분에 대한 예외 코드도 처리가 되어있어서 궁금했습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀해주신 javadoc 레퍼런스도 참고하긴 했었는데요, 정확하게 어떤 에러코드에 어떤 예외 클래스가 매핑되는지 나와있는 레퍼런스는 없더라구요 ㅎㅎ... 그래서 이것 저것 찾아보다가 spring-jdbc 라이브러리에 위치하는 sql-error-codes.xml 파일을 찾게 되었는데 각 DB 벤더사 별로 정의해 둔 에러코드들을 스프링 빈으로 관리할 수 있게끔 정리해놨더라구요! 그리고 여기에서 에러코드들을 분류한 기준대로 SQLErrorCodeSQLExceptionTranslator 클래스에서 매핑을 시켜주는 예외 클래스들이 있습니다. 이 예외 클래스들을 참고해서 구현했었어요! 참고해보시면 좋을 것 같습니다~!!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

라이브러리에 정리가 되어 있었군요! 좋은 정보 감사합니다!

Comment on lines 44 to 56
public int update(Connection conn, 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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

히이로가 말한 탬플릿 콜백을 이용한 리팩토링에 대해서 이야기를 해볼려고 합니다.
먼저 저도 이번에 탬플릿 콜백 패턴을 활용해서 리팩토링을 해보지 않았지만 이 부분을 적용해볼 수 있는 곳에 말하겠습니다.
탬플릿 콜백 패턴의 핵심은 콜백이라는 용어가 핵심이라고 생각합니다. 이는 인자를 받은 함수의 실행 시점을 제어할 수 있는 것으로 보면 좋을 것 같아요!
예를 들면 메소드 a가 메소드 b를 인자로 받고 실행시점을 제어할 수 있습니다.

public void a (메소드 b) {
    1
    // a 메소드 로직
    2
   // a 메소드 로직
    3
}

위와 같이 a 메소드가 있으면 메소드 b의 실행을 1,2,3 곳에서 실행시점을 제어가 가능합니다.
다시 돌아와서 미션의 상황을 살펴보겠습니다.
queryForObject, query, update를 구현하면서 느낀 점이 있었을 텐데요

try (Connection conn = dataSource.getConnection();
PreparedStatement pstmt = conn.prepareStatement(sql)) {

// 각 메소드에 따른 다른 동작

}} catch (SQLException e) {
    log.error(e.getMessage(), e);
    throw sqlExceptionTranslator.translate(sql, e);
}

// 각 메소드에 따른 다른 동작 부분만 다르고 이 부분이 중복이 되는 것을 확인할 수 있습니다.
중복된 코드를 분리한 메소드를 설정하고(현재에는 execute라고 하겠습니다.) 그 메소드에 저 동작 과정을 인자로 받게 처리하면 중복된 코드를 제거 할 수 있게 됩니다.

여기서 // 각 메소드에 따른 다른 동작 부분은 결국 prepareStatement를 실행시키고 결과를 반환을 하는 것이기에 이부분을 함수형 인터페이스로 선언을 하고 이를 execute에서 실행하는 방식으로 리팩토링을 할 수 있을 것 같습니다.

이렇게 함으로써 얻을 수 있는 장점은 jdbcTemplate에 새로운 메소드가 추가가 되더라도 핵심 동작만 고려해서 추가가 가능하다는 것입니다. (connection을 연결을 하고 실행시 예외를 발생하는 로직을 반복해서 작성할 필요가 없음)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❗️현재 query()메소드에는 수정 부분이 없어서 여기에 리뷰납깁니다.

이 부분은 저도 받았던 리뷰였는데 고려하면 좋을 것 같아서 남깁니다.
현재는 select id, account, password from user만 고려하고 있지만
select id, account, password from user where account = ?에서도 query를 이용할 수 있는 상황이 있을 것 같아요!
(예를 들면 account은 중복을 허용하는 경우)
이부분에 대해서도 동작할 수 있게 query에 파라미터를 받을 수 있게 수정하면 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

라이브러리는 범용성을 잘 제공해야 사용자가 편하게 사용할 수 있겠죠! 좋은 리뷰 감사해요 아코~ 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

템플릿 콜백 패턴은 한 번 적용해봤어요! 아코가 일러주신대로 try-catch 패턴 코드의 중복을 줄이고 PreparedStatement 객체로 무엇을 하고 싶은지에 대해서만 집중할 수 있도록 개선해볼 수 있었던 것 같습니다. 😃

update 메서드 쪽도 함께 진행해보고 싶었는데 일단 트랜잭션 관련해서 추가된 connection을 인자로 받는 update 메서드는 나중에 제거될 예정이다보니, update 메서드는 중복되는 로직이 따지고 보면 없는지라 굳이 지금 콜백 패턴을 적용해야 하나라는 생각이 들더라구요.. ㅎㅎㅎ

혹시 이상한 부분이 있다면 말씀해주세요~!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 Connection이 있는 update는 콜백 패턴을 적용할 필요 없다고 생각합니다!
히이로가 콜백 패턴을 적용한 것을 활용하면 update 메소드 중에 connection을 받지 않는 것은 콜백 패턴을 다음과 적용해볼 수 있을 것 같아요!

public int update(final String sql, Object... args) {
    return executeQuery(PreparedStatement::executeUpdate, sql, args);
}


public class UserService {

private final UserDao userDao;
private final UserHistoryDao userHistoryDao;
private final DataSource dataSource;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 userSerive가 DataSource를 의존하는 것이 아쉽지만 이부분은 4단계에서 해결이 될 것 같네요!

import java.sql.SQLException;
import org.springframework.dao.DataAccessException;

public class BadSqlGrammarException extends DataAccessException {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 다른 예외들은 dao 패키지에 있는데 혹시 이 Sql문법 오류만 따로 관리하는 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

원래 스프링에는 spring-jdbc 모듈과 spring-tx 모듈이 따로 존재하고 있습니다. 이름에서도 볼 수 있듯이 jdbc 관련 코드들과 트랜잭션 관련 코드들이 서로 다른 모듈로서 분리되어 있는 것인데요, 실제 SQLExceptionTranslator는 이 두 모듈에 걸쳐 위치하는 예외 클래스들을 매핑시켜주고 있습니다. BadSqlGrammarException은 spring-jdbc 모듈에, 나머지 exception들은 spring-tx에 위치하고 있어요.

제 생각에는 BadSqlGrammarException 같은 경우 트랜잭션과 직접적으로 관련된 예외 상황이 아니기 때문에 이렇게 실제로도 예외 클래스들을 서로 다른 모듈에서 관리하고 있는 것 같습니다. 이런 부분을 생각하고 따로 관리하는 방향으로 구현했었어요!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 그 부분까지는 고려하지 못했고 BadSqlGrammarException 역시 큰 범위에서는 persistence 영역의 에러라고 생각을 했었어서 같이 관리하는 것이 더 응집성이 좋다고 생각을 했었는데 스프링에서는 분리해서 관리를 하는 군요! 좋은 정보 감사합니다! 👍👍

Copy link
Author

@MoonJeWoong MoonJeWoong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 아코! 바쁘신 와중에도 이렇게 정성스런 리뷰를 받으니 감동을 받지 않을 수가 없네요 ㅎㅎ 정말 감사합니다.

말씀해주신 사항들을 반영하거나 코멘트를 달아봤는데요 확인해주시고 혹시 더 추가로 얘기해볼만한 사항들이 있다면 DM 주세요~


public class UserService {

private final UserDao userDao;
private final UserHistoryDao userHistoryDao;
private final DataSource dataSource;
private final SQLExceptionTranslator sqlExceptionTranslator = new SQLExceptionTranslator();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 JdbcTemplate 코드에서는 SQLExceptionTranslator가 스프링 빈으로 등록이 되서 사용이 되어 static 클래스가 아니었기 때문에 이렇게 구현하고 나서 다시 생각해보진 못했네요...! 현 상황에서는 따로 빈으로 등록해서 사용하는 것도 아니기에 굳이 인스턴스화해서 사용할 필요는 없을 것 같아요 ㅎㅎ 유틸 클래스로 static 처리 했습니다!

Comment on lines 44 to 56
public int update(Connection conn, 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);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

라이브러리는 범용성을 잘 제공해야 사용자가 편하게 사용할 수 있겠죠! 좋은 리뷰 감사해요 아코~ 😄

import java.sql.SQLException;
import org.springframework.dao.DataAccessException;

public class BadSqlGrammarException extends DataAccessException {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

원래 스프링에는 spring-jdbc 모듈과 spring-tx 모듈이 따로 존재하고 있습니다. 이름에서도 볼 수 있듯이 jdbc 관련 코드들과 트랜잭션 관련 코드들이 서로 다른 모듈로서 분리되어 있는 것인데요, 실제 SQLExceptionTranslator는 이 두 모듈에 걸쳐 위치하는 예외 클래스들을 매핑시켜주고 있습니다. BadSqlGrammarException은 spring-jdbc 모듈에, 나머지 exception들은 spring-tx에 위치하고 있어요.

제 생각에는 BadSqlGrammarException 같은 경우 트랜잭션과 직접적으로 관련된 예외 상황이 아니기 때문에 이렇게 실제로도 예외 클래스들을 서로 다른 모듈에서 관리하고 있는 것 같습니다. 이런 부분을 생각하고 따로 관리하는 방향으로 구현했었어요!

Comment on lines +14 to +22
private static final Map<Integer, Class<? extends DataAccessException>> mapper = new HashMap<>();

static {
setUpBadSqlGrammarCodes();
setUpDuplicationKeyException();
setUpDataIntegrityViolationException();
setUpDataAccessResourceFailureException();
setUpCannotAcquireLockException();
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀해주신 javadoc 레퍼런스도 참고하긴 했었는데요, 정확하게 어떤 에러코드에 어떤 예외 클래스가 매핑되는지 나와있는 레퍼런스는 없더라구요 ㅎㅎ... 그래서 이것 저것 찾아보다가 spring-jdbc 라이브러리에 위치하는 sql-error-codes.xml 파일을 찾게 되었는데 각 DB 벤더사 별로 정의해 둔 에러코드들을 스프링 빈으로 관리할 수 있게끔 정리해놨더라구요! 그리고 여기에서 에러코드들을 분류한 기준대로 SQLErrorCodeSQLExceptionTranslator 클래스에서 매핑을 시켜주는 예외 클래스들이 있습니다. 이 예외 클래스들을 참고해서 구현했었어요! 참고해보시면 좋을 것 같습니다~!!

Comment on lines 44 to 56
public int update(Connection conn, 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);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

템플릿 콜백 패턴은 한 번 적용해봤어요! 아코가 일러주신대로 try-catch 패턴 코드의 중복을 줄이고 PreparedStatement 객체로 무엇을 하고 싶은지에 대해서만 집중할 수 있도록 개선해볼 수 있었던 것 같습니다. 😃

update 메서드 쪽도 함께 진행해보고 싶었는데 일단 트랜잭션 관련해서 추가된 connection을 인자로 받는 update 메서드는 나중에 제거될 예정이다보니, update 메서드는 중복되는 로직이 따지고 보면 없는지라 굳이 지금 콜백 패턴을 적용해야 하나라는 생각이 들더라구요.. ㅎㅎㅎ

혹시 이상한 부분이 있다면 말씀해주세요~!

Comment on lines 9 to 19
public DataAccessException translate(@Nullable String sql, SQLException e) {
Class<? extends DataAccessException> exceptionClazz =
H2SQLErrorCodeToDataAccessExceptionMapper.mapSQLErrorCode(e.getErrorCode());

try {
return exceptionClazz.getDeclaredConstructor(String.class, SQLException.class)
.newInstance(sql, e);
} catch (ReflectiveOperationException ex) {
return new DataAccessException("Some exception was thrown : " + e.getMessage());
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 스프링 라이브러리에 구현된 SQLErrorCodeSQLExceptionTranslator는 nullable로 받고 있어서 이유가 있겠거니 하고 따라했다가 잊어먹었네요 ㅎㅎ... 현재 미션 수준 코드에서는 굳이 필요없는 어노테이션이라는 생각에 동의합니다! 해당 부분은 수정했어요 :)

Comment on lines 9 to 19
public DataAccessException translate(@Nullable String sql, SQLException e) {
Class<? extends DataAccessException> exceptionClazz =
H2SQLErrorCodeToDataAccessExceptionMapper.mapSQLErrorCode(e.getErrorCode());

try {
return exceptionClazz.getDeclaredConstructor(String.class, SQLException.class)
.newInstance(sql, e);
} catch (ReflectiveOperationException ex) {
return new DataAccessException("Some exception was thrown : " + e.getMessage());
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기에서 예외가 발생하는 경우는 기존 벤더사에 등록되어 있던 예외 코드와는 다른 예외가 발생했을 경우라고 생각했습니다. 그래서 이런 경우에도 라이브러리의 사용자는 발생한 SQLException의 예외 내용은 알 수 있어야 한다고 생각해서 이런 방향으로 구현해봤었습니다! 혹시 제가 생각하지 못한 부분이 더 있었다면 말씀해주세요~

Copy link

@seokhwan-an seokhwan-an left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 히이로!

리뷰 드린 것을 빠르게 적용해주셨군요👍👍

히이로가 예외처리를 구현한 것을 리뷰하면서 저도 많이 배울 수 있었습니다!

탬플릿 콜백 패턴도 깔끔하게 구현이 된 것 같아요!

이번 단계에서는 크게 이야기를 나눌 부분이 더이상 없다고 생각해서 approve 하겠습니다!

4단계도 화이팅입니다 💪💪

@@ -76,11 +76,14 @@ public <T> T queryForObject(String sql, RowMapper<T> rowMapper, Object... args)
}
}

public <T> List<T> query(String sql, RowMapper<T> rowMapper) {
public <T> List<T> query(String sql, RowMapper<T> rowMapper, Object... args) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Select id, account, password from user도 고려해서 Object... ags@Nullable을 붙어주면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge 하면서 달아주신 리뷰들은 확인을 미처 못했었네요... 😭 요 부분은 다음 스텝 리뷰에 제 의견 달아두었습니다..!

Comment on lines +14 to +22
private static final Map<Integer, Class<? extends DataAccessException>> mapper = new HashMap<>();

static {
setUpBadSqlGrammarCodes();
setUpDuplicationKeyException();
setUpDataIntegrityViolationException();
setUpDataAccessResourceFailureException();
setUpCannotAcquireLockException();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

라이브러리에 정리가 되어 있었군요! 좋은 정보 감사합니다!

import java.sql.SQLException;
import org.springframework.dao.DataAccessException;

public class BadSqlGrammarException extends DataAccessException {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 그 부분까지는 고려하지 못했고 BadSqlGrammarException 역시 큰 범위에서는 persistence 영역의 에러라고 생각을 했었어서 같이 관리하는 것이 더 응집성이 좋다고 생각을 했었는데 스프링에서는 분리해서 관리를 하는 군요! 좋은 정보 감사합니다! 👍👍

Comment on lines 86 to 98
private <T> T executeQuery(final PreparedStatementCallback<T> action, final String sql, Object... args) {
try (
Connection conn = dataSource.getConnection();
PreparedStatement pstmt = conn.prepareStatement(sql)
) {
ArgumentPreparedStatementSetter pss = new ArgumentPreparedStatementSetter(args);
pss.setValues(pstmt);

return extractResultSet(pstmt.executeQuery(), rowMapper);
return action.doInPreparedStatement(pstmt);
} catch (SQLException e) {
log.error(e.getMessage(), e);
throw SQLExceptionTranslator.translate(sql, e);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깔끔하게 분리가 된거 같아요!👍👍

@FunctionalInterface
public interface PreparedStatementCallback<T> {

T doInPreparedStatement(PreparedStatement ps) throws SQLException, DataAccessException;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 여기서 DataAccessException을 throws를 해주시는 이유가 있으신가요?
상위에서 이를 이용하는 execute 메소드는 현재 SQLException을 catch해서 DataAccessException으로 변경하는 역할을 합니다. 즉, 현재 doInPreparedStatement는 SQLException만 발생해서 저는 DataAccessException을 throws를 할 필요가 없다고 생각하는데 히이로는 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 제가 확실히 생각하지 못한 부분이었네요! 현재는 아코가 말씀해주신 것처럼 SQLException만 외부로 throw되도록 하는 것이 더 올바른 방향인 것 같습니다. 반영했습니다~!

Comment on lines 44 to 56
public int update(Connection conn, 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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 Connection이 있는 update는 콜백 패턴을 적용할 필요 없다고 생각합니다!
히이로가 콜백 패턴을 적용한 것을 활용하면 update 메소드 중에 connection을 받지 않는 것은 콜백 패턴을 다음과 적용해볼 수 있을 것 같아요!

public int update(final String sql, Object... args) {
    return executeQuery(PreparedStatement::executeUpdate, sql, args);
}

Comment on lines 9 to 19
public DataAccessException translate(@Nullable String sql, SQLException e) {
Class<? extends DataAccessException> exceptionClazz =
H2SQLErrorCodeToDataAccessExceptionMapper.mapSQLErrorCode(e.getErrorCode());

try {
return exceptionClazz.getDeclaredConstructor(String.class, SQLException.class)
.newInstance(sql, e);
} catch (ReflectiveOperationException ex) {
return new DataAccessException("Some exception was thrown : " + e.getMessage());
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 그렇게 생각을 할 수 있을 것 같네요! 저는 리플랙션 부분이 실패해서 발생하는 예외이다 보니까 라이브러리 자체가 문제가 있는 것이라고 판단을 했어서 DataAccessException이 아닌 다른 커스텀 에러를 발생시켜야 한다고 생각을 했었습니다!

@seokhwan-an seokhwan-an merged commit 400df75 into woowacourse:moonjewoong Oct 8, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants