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 라이브러리 구현하기 - 4단계] 하마드(이건회) 미션 제출합니다. #538

Merged
merged 6 commits into from
Oct 12, 2023

Conversation

rawfishthelgh
Copy link

팍스 마지막 PR이네요...
코딱지 코드를 좀 개선시켜 봤습니다.
주요 변경사항은 다음과 같아요.

  1. 이전에는 jdbcTemplate에서 checked 예외를 런타임으로 변환 처리했는데요. 지금은 jdbcTemplate에서는 그냥 밖으로 던지고 dao 단에서 런타임 변환을 시켰습니다. 이유는 jdbcTemplate은 라이브러리이고 dao는 직접 개발자가 짜는 부분이다 보니, jdbcTemplate에서는 예외만 발생시키고 이걸 어떻게 처리할지는 dao에서 개발자가 결정하는게 옳다고 판단했어요
  2. 트랜잭션 동기화를 위해 뼈대 코드인 TransactionSynchronizationManager를 사용했습니다. dao에서 커넥션이나 DataSource를 넘겨받지 않아도 jdbcTemplate에서 같은 데이터소스를 넘겨받기만 하면 동일 커넥션을 조회할 수 있더군요!
  3. TransactionSynchronizationManager에서 getResource(DataSource key)를 호출할 때 자꾸 NPE가 나서 찾아보니, resources.get()을 호출할 때 스레드 로컬 안의 맵이 초기화되지 않아 에러가 나더군요...그래서 ThreadLocal.withInitial(HashMap::new)로 HashMap을 초기화시켜 넣었는데 어떤가요??
  4. 서비스 계층 추상화를 시켰는데 상당히 맘에 듭니다

이번에도 좋은 리뷰 부탁합니다ㅎㅎ

Copy link

@BackFoxx BackFoxx left a comment

Choose a reason for hiding this comment

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

하마드 하이하이~!
이전보다 훨씬 깔끔하고 단정한 게 마치 하마드같네요.
재밌는 과제 하나 내드렸으니 이것만 수행하시면 이번 미션 마무리할 수 있습니다~!

try {
jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail(), user.getId());
} catch (SQLException e) {
throw new DataAccessException(e) {

Choose a reason for hiding this comment

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

실제 JdbcTemplate에서 SQLException을 잡은 후 자기네 DataAccessException으로 바꾸어 던지는 이유가 무엇일까요?

마드짱의 insert sql에 아무 글자나 넣어서 오타를 낸 후 실행해보면 JdbcSQLSyntaxErrorException이 나오는데,
image
예외의 패키지를 보면 org.h2.jdbc로 h2 라이브러리에서 제공하는 예외임을 알 수가 있죠.

이건 우리가 h2를 사용하기 때문에 h2 예외가 발생한 것이고, mysql을 쓰면 mysql 예외가, mariaDB를 쓰면 mariaDB 예외가 발생하겠죠.

지금의 UserDao는 catch문에서 SQLException 전체를 잡고 있는데.

    public void update(final User user) {
        String sql = "UPDATE users SET account = ?, password = ?, email = ? WHERE id = ?";
        try {
            jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail(), user.getId());
        } catch (SQLException e) {
            throw new DataAccessException(e) {
            };
        }
    }

만약 SyntaxError 하나만 잡아서 특별한 처리를 하는 상황이 왔다고 가정해 보아요.

    public void update(final User user) {
        String sql = "UPDATE users SET account = ?, password = ?, email = ? WHERE id = ?";
        try {
            jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail(), user.getId());
        } catch (JdbcSQLSyntaxErrorException e) {
            throw new DataAccessException(e) {
            };
        }
    }

h2를 쓰고 있는 지금은 syntax에러가 발생했을 때 catch문에서 딱 잡아주지만
만약 데이터베이스를 MySQL로 바꾸었다고 하면? 안 잡히겠죠! (두둥)
왜냐면 위 catch문에서 잡는 SyntaxError는 h2 예외이기 때문에
mysql에서 나온 SyntaxError는 당연히 안 잡겠죠?

이를 방지하려면 SQLException을 잡는 모든 try-catch문을 찾아가 h2 예외를 잡는 대신 mysql 예외를 잡도록 바꿔야겠네요.

public void update(final User user) {
        String sql = "UPDATE users SET account = ?, password = ?, email = ? WHERE id = ?";
        try {
            jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail(), user.getId());
        } catch (org.mysql.jdbc.JdbcSQLSyntaxErrorException e) {
            throw new DataAccessException(e) {
            };
        }
    }

그런데 운영 환경에서는 mysql을 쓰지만 테스트 환경에서는 h2를 쓰겠다! 라고 하면
또다시 모든 try-catch문을 찾아가 h2 예외를 잡는 catch문을 밑에 더 붙여줘야 할 거에요.

public void update(final User user) {
        String sql = "UPDATE users SET account = ?, password = ?, email = ? WHERE id = ?";
        try {
            jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail(), user.getId());
        } catch (org.mysql.jdbc.JdbcSQLSyntaxErrorException e) {
            throw new DataAccessException(e) {
            };
        } catch (org.h2.jdbc.JdbcSQLSyntaxErrorException e) {
            throw new DataAccessException(e) {
            };
        }
    }

'jdbcTemplate에서는 예외만 발생시키고 이걸 어떻게 처리할지는 dao에서 개발자가 결정하는게 옳다' 라고 하마드는 말씀해 주셨지만,
현재 사용중인 DBMS와 미래에 사용하게 될 DBMS의 종류까지 신경써가며 코드를 작성해야 하는 것이
과연 개발자에게 건강한 결정권을 주었다고 볼 수 있는지 모르겠어요.
하마드의 생각이 궁금하군요!

Copy link
Author

Choose a reason for hiding this comment

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

오 여우의 상세한 리뷰를 보고 나니 제가 착각한 부분이 있었네요...!
JdbcTemplate이 SQL Exception을 던진다고 착각했는데, Checked 예외를 DataAccessException으로 바꿔 던지는 것을 간과하고 있었군요.
수정하도록 할게요 좋은 리뷰 고마워요ㅎㅎ

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

public static void releaseConnection(Connection connection, DataSource dataSource) {
try {
connection.setAutoCommit(true);

Choose a reason for hiding this comment

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

어차피 닫을 거라면 AutoCommit을 켜는 게 어떤 의미가 있을까요? 🤨🤨

Copy link
Author

@rawfishthelgh rawfishthelgh Oct 11, 2023

Choose a reason for hiding this comment

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

앗 실수네요ㅋㅋㅋㅋ
코드를 짤 때는 아무런 생각 없이 커넥션이 해제되면 풀에 돌아갈 테니까 오토커밋 끄고 반납해야지~라고 생각했는데,
커넥션 자체를 닫아버리면 의미가 없군요! 수정하겠습니다

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

Choose a reason for hiding this comment

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

releaseConnection() 메서드 안에 unbindResource() 작업을 포함해도 되지 않을까라는 생각이 들어요!

Copy link
Author

Choose a reason for hiding this comment

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

좋은 생각입니다 👍

}
}

public void changePassword(final long id, final String newPassword, final String createBy) {

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.

순수 비즈니스 로직만 가진 서비스 클래스 (AppUserService
)와, 트랜잭션 처리 관심사를 담당하면서 위 AppUserService를 인스턴스로 받아 비즈니스 로직을 호출하는 클래스로 구분해서 둘의 관심사를 끊어내기 위함이었습니다.(요구사항을 지키기 위해서요)

그런데 테스트 코드에서

AppUserService appUserService = new AppUserService(userDao, userHistoryDao);
final var userService = new TxUserService(appUserService,dataSource);

다음과 같이 직접 구현체를 주입시켜야 하는 어색함이 있네요...

Choose a reason for hiding this comment

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

오홍 이해했습니다!
테스트 코드에서는 테스트하려는 구현체를 직접 넣는 경우가 흔하기 때문에 저는 어색하다고 느껴지지 않았어요 😊

import javax.sql.DataSource;
import java.sql.Connection;
import java.sql.SQLException;
public interface UserService {

Choose a reason for hiding this comment

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

좋은 과제가 하나 떠올랐어요 하마드!
마침 UserService를 인터페이스로 만드셨으니,
JDK Dynamic Proxy라는 키워드로 학습하셔서
서비스 코드와 트랜잭션 처리 코드를 완벽하게 분리해보세요.

    public void changePassword(final long id, final String newPassword, final String createBy) {
        final var user = userDao.findById(id);
        user.changePassword(newPassword);
        userDao.update(user);
        userHistoryDao.log(new UserHistory(user, createBy));
    } <- 서비스 코드는 비즈니스 로직만 있게, 근데도 사용할 때는 트랜잭션 적용까지 되어 있게 해보기

만약 성공하시면 진짜 기분 끝내줄 거에요!

Copy link
Author

Choose a reason for hiding this comment

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

오오 알지 못하고 있던 키워드였는데, JDK Dynamic Proxy를 활용하면 트랜잭션 처리 구현체를 따로 만들고, 트랜잭션 구현체에서 호출하는 메소드마다 트랜잭션 처리를 일일이 적용할 필요가 없겠네요! 부끄럽지만 여우가 제출한 코드를 레퍼런스 삼아서 저도 적용해 봤어요^^

궁금한 점이 하나 있는데, 이전 방식은
서비스 인터페이스(UserService) 생성
-> 비즈니스 로직만 가진 서비스 구현체(AppUserService) 생성
-> 비즈니스 구현체(AppUserService)를 주입받는 트랜잭션 처리용 서비스 구현체(TxUserService) 생성
하는 방식으로 트랜잭션 처리 관심사와 비즈니스 관심사를 분리했는데,

제가 다이나믹 프록시를 적용하면서 느낀 점은 이전 트랜잭션 처리 구현체를 만든 것과 다르게,
“메소드를 일일이 @OverRide 할 필요가 없다” 라는 것이었는데요.

그렇다면 결국 관심사 분리는 이전 방식으로도 이루어질 수 있지만, 단순히 코드 경량화가 이루어진다는 것이 추가된 장점일까요? 다른 장점이 있는지 잘 와닿지가 않아 질문해요.

Choose a reason for hiding this comment

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

사실 하마드의 코드도 관심사의 분리가 잘 된 코드에요!
다만 TxUserService은 Override한 메서드 각각에 트랜잭션 처리 코드가 중복으로 구현된 반면
프록시를 사용한 코드는 invoke() 메서드만 Override하면 일괄적으로 적용이 된다는 점에서,
'같은 관심사를 가진 코드를 한 곳에 모아서 수정과 확장이 편리하게 한다'는 관심사 분리의 장점을 극대화할 수 있다고 할 수 있어요.
그걸 쫌 간략하게 얘기하면 하마드의 말처럼 코드 경량화라고 표현할 수도 있겠네요. 어차피 클린코드라는 것도 결국 코에 걸면 코걸이고 귀에 걸면 귀걸이고 .. 😁

Copy link

@BackFoxx BackFoxx left a comment

Choose a reason for hiding this comment

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

수고했어요 하마드!! 즐거웠습니다

@BackFoxx BackFoxx merged commit fafd56e into woowacourse:rawfishthelgh Oct 12, 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