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단계] 여우(조승현) 미션 제출합니다. #602

Merged
merged 7 commits into from
Oct 12, 2023

Conversation

BackFoxx
Copy link

안녕하세요 엔초! 벌써 마지막 단계네요 🥹
4단계에서 제공해준 DataSourceUtils 덕분에 서비스의 각 계층이 Connection을 계속 파라미터로 넘기는 고리를 끊고 훨씬 깔끔한 코드를 만들 수 있었어요.

하지만 서비스 계층에서 여전히 DataSource와 Connection을 사용해 트랜잭션을 뚱땅뚱땅 조작하는 코드가 섞여있어서, 정말로 집중해야 할 비즈니스 로직이 한눈에 들어오지 않는다는 문제가 여전히 남아있었습니다.

그래서 자바에서 제공해주는 jdk dynamic proxy를 사용해서, 트랜잭션 관련 처리는 프록시 레이어에서 수행하고 실제 서비스 객체는 비즈니스 로직에만 집중할 수 있도록 해내는 데에 성공했어요!
뭔가 뿌듯합니다.

2단계 때 엔초가 피드백 남겨주신
'jdbcTemplate에서 try-with resource 중복 코드를 제거하는 방법' 도 여러 시도를 해보면서 고민했는데 ,, 모르겠어요! 더 고민해봐야 할 것 같습니다.

마지막 리뷰 잘 부탁드려요!

Copy link

@kwonyj1022 kwonyj1022 left a comment

Choose a reason for hiding this comment

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

안녕하세요 여우~
간단한 궁금증이 있기도 하고 이번 미션 마지막 제출이라 혹시 코드를 개선하고 싶으신 부분이 있을까봐 request changes로 남겨두었습니다.
코드 변경 예정이 없다면 질문에 답변만 해주시고 다시 리뷰요청 해주세요!

Comment on lines 37 to 40
private RuntimeException handleTransactionFailure(final Connection connection, final Exception e) {
try {
connection.rollback();
return new RuntimeException(e);

Choose a reason for hiding this comment

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

저는 throw e 이렇게 다시 넘겨주었는데 여우는 예외를 RuntimeException으로 감싸서 던져주셨군요! RuntimeException으로 감싸서 넘겨주는 방식으로 구현하신 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

그냥 Exception을 던지면 메서드에 throws Exception! 이렇게 명시적으로 선언해주어야 한다는 게 불편하지 않을까라는 생각이었는데, 다시 보니 이미 메서드에서 throws Throwable 이렇게 명시가 되어있었네요! 🥹 엔초처럼 바로 throw e를 하도록 수정했습니다

Comment on lines +12 to +16
class DataSourceUtilsTest {

@Test
@DisplayName("DataSourceUtils를 이용해 커넥션을 가져올 수 있다.")
void getConnection() {

Choose a reason for hiding this comment

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

테스트도 작성해주셨군요!! 리스펙합니다 👍

Comment on lines +38 to 40
public static void remove() {
resources.remove();
}

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.

ThrealLocal을 비워주는 메서드인데, 제가 ThreadLocal을 처음 배울 때에는 요청을 처리하고 응답하기 전에 ThreadLocal을 꼭 비워주어야 한다고 배웠어요!
그래서 만들어 둔 메서드인데 이걸 어느 포인트에서 호출해야 할지 잘 모르겠네요 🥹

Comment on lines +14 to +17
public static final String ACCOUNT = "account";
public static final String PASSWORD = "password";
public static final String EMAIL = "email";
public static final String ID = "id";

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.

SonarLint가 이런 거 정말 잘 짚어주더라구요! 구구 짱 🙌🏻

Comment on lines -26 to +28
public int update(Connection connection, String sql, Object... parameters) {
public int update(String sql, Object... parameters) {
final Connection connection = DataSourceUtils.getConnection(dataSource);

Choose a reason for hiding this comment

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

JdbcTemplate 전체적인 사항인데 코멘트 라인 지정이 안돼서 여기에 남깁니다.

현재 트랜잭션을 사용하는 경우에만 DataSourceUtils.releaseConnection(connection, dataSource);를 해주고 있는 것 같아요. 그런데 만약 트랜잭션을 사용하지 않는 서비스에서 dao만 사용한다면(현재 여우 코드로는 UserServiceImpl을 동작시킨다면) 커넥션은 어떻게 될까요?

UserServiceTest의 testTransactionRollback() 테스트에서 사용되는 UserService를 UserServiceImpl로 바꿔보니 JdbcTemplate 실행 시 생성한 커넥션이 종료되지 않는 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

😮 좋은 피드백 감사해요 엔초!
connection의 autoCommit이 켜져있는지 꺼져있는지를 boolean으로 나타내는 getAutoCommit() 메서드를 활용해서,
jdbcTemplate의 작업이 끝날 때 Connection의 AutoCommit이 켜져있다면(=트랜잭션과 관련없는 단일 작업이라면) Connection 연결을 끊도록 변경했더니
엔초께서 말씀해주신 문제점이 해결되었어요 😊

Comment on lines +21 to +35
@Override
public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable {
final Connection connection = DataSourceUtils.getConnection(dataSource);
try {
connection.setAutoCommit(false);
final Object result = method.invoke(target, args);
connection.commit();

return result;
} catch (RuntimeException | SQLException e) {
throw handleTransactionFailure(connection, e);
} finally {
DataSourceUtils.releaseConnection(connection, dataSource);
}
}

Choose a reason for hiding this comment

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

오 프록시를 사용하셨군요! 덕분에 새로운 걸 배워갑니다. 🙇‍♀️

Copy link

@kwonyj1022 kwonyj1022 left a comment

Choose a reason for hiding this comment

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

안녕하세요 여우! 수정된 사항 모두 확인했습니다.
이번 미션도 수고하셨습니다!!!!

Comment on lines +109 to +117
private void releaseIfNotTransacting(final Connection connection) {
try {
if (connection.getAutoCommit()) {
DataSourceUtils.releaseConnection(connection, dataSource);
}
} catch (SQLException e) {
throw new SQLNonTransientConnectionException(e);
}
}

Choose a reason for hiding this comment

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

👍

@kwonyj1022 kwonyj1022 merged commit 614e6a7 into woowacourse:backfoxx 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