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단계] 가비 미션 제출합니다. #523

Merged
merged 19 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
0b720af
refactor: 메서드명 수정
iamjooon2 Oct 7, 2023
e21617d
fix: DataSourceUtil과 TransactionSynchronizationManager를 이용하여 Connecti…
iamjooon2 Oct 8, 2023
1ae2ea5
refactor: findById의 반환 타입에서 Optional 제거
iamjooon2 Oct 8, 2023
b05df24
feat: DataSourceUtils 내 TransactionSynchronizationManger 자원 해제 추가
iamjooon2 Oct 8, 2023
8637110
feat: TransactionManager 구현
iamjooon2 Oct 8, 2023
9c51e1d
feat: UserService 추상화
iamjooon2 Oct 8, 2023
b072844
feat: AppUserService, TxUserService 구현
iamjooon2 Oct 8, 2023
87a8363
test: testTransactionRollback() 메서드를 LMS에 있는 내용으로 변경
iamjooon2 Oct 8, 2023
f47ee27
fix : jdbctemplate에서 connection 해제하는 부분 제거
iamjooon2 Oct 8, 2023
06d7f7c
refactor: Connection 선언부를 try-catch 밖으로 분리
iamjooon2 Oct 8, 2023
c89797e
refactor: TranactionManager의 메서드 반환값을 void로 변경
iamjooon2 Oct 8, 2023
faa6f7d
chore: final 키워드 통일
iamjooon2 Oct 8, 2023
26a140a
fix: TransactionSynchronizationManager.unbindResource(dataSource) 중복 …
iamjooon2 Oct 9, 2023
42d84c0
fix: 불필요한 static 제거
iamjooon2 Oct 11, 2023
f0061ad
feat: DatabasePopulatorUtils도 DataSoureUtils를 이용해 connection을 가지고 오도록 변경
iamjooon2 Oct 11, 2023
a281e05
refactor: service에서 TransactionManager를 주입받도록 변경
iamjooon2 Oct 11, 2023
7ad67af
refactor: 템플릿 콜백 패턴 적용
iamjooon2 Oct 11, 2023
0077696
refactor: 함수형 인터페이스 실행 메서드 분리
iamjooon2 Oct 11, 2023
245d83e
refactor: DatasourcePopulatorUtils connection 해제 추가
iamjooon2 Oct 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions app/src/main/java/com/techcourse/dao/UserDao.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.techcourse.dao;

import java.sql.Connection;
import java.util.List;
import java.util.Optional;

Expand Down Expand Up @@ -42,14 +41,6 @@ public void update(final User user) {
jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail(), user.getId());
}

public void update(final Connection connection, final User user) {
final String sql = "update users set account = ?, password = ? , email = ? where id = ?";

log.debug("query : {}", sql);

jdbcTemplate.update(connection, sql, user.getAccount(), user.getPassword(), user.getEmail(), user.getId());
}

public Optional<User> findById(final Long id) {
final String sql = "select id, account, password, email from users where id = ?";

Expand Down
17 changes: 0 additions & 17 deletions app/src/main/java/com/techcourse/dao/UserHistoryDao.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.techcourse.dao;

import java.sql.Connection;

import com.techcourse.domain.UserHistory;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -32,19 +30,4 @@ public void log(final UserHistory userHistory) {
log.debug("query : {}", sql);
}

public void log(final Connection connection, final UserHistory userHistory) {
final var sql = "insert into user_history (user_id, account, password, email, created_at, created_by) values (?, ?, ?, ?, ?, ?)";

jdbcTemplate.update(connection, sql,
userHistory.getUserId(),
userHistory.getAccount(),
userHistory.getPassword(),
userHistory.getEmail(),
userHistory.getCreatedAt(),
userHistory.getCreateBy()
);

log.debug("query : {}", sql);
}

}
36 changes: 36 additions & 0 deletions app/src/main/java/com/techcourse/service/AppUserService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package com.techcourse.service;

import com.techcourse.dao.UserDao;
import com.techcourse.dao.UserHistoryDao;
import com.techcourse.domain.User;
import com.techcourse.domain.UserHistory;

public class AppUserService implements UserService {

private final UserDao userDao;
private final UserHistoryDao userHistoryDao;

public AppUserService(final UserDao userDao, final UserHistoryDao userHistoryDao) {
this.userDao = userDao;
this.userHistoryDao = userHistoryDao;
}

@Override
public User findById(final long id) {
return userDao.findById(id).orElseThrow();
}

@Override
public void insert(final User user) {
userDao.insert(user);
}

@Override
public void changePassword(final long id, final String newPassword, final String createBy) {
final User user = findById(id);
user.changePassword(newPassword);
userDao.update(user);
userHistoryDao.log(new UserHistory(user, createBy));
}

}
32 changes: 32 additions & 0 deletions app/src/main/java/com/techcourse/service/TxUserService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package com.techcourse.service;

import com.techcourse.config.DataSourceConfig;
import com.techcourse.domain.User;
import org.springframework.transaction.TransactionManager;

public class TxUserService implements UserService {

private final UserService userService;
private final TransactionManager transactionManager;

public TxUserService(final UserService userService) {
this.userService = userService;
this.transactionManager = new TransactionManager(DataSourceConfig.getInstance());
Copy link

Choose a reason for hiding this comment

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

Service에서 DataSource를 주입해주신 이유가 있으신가요?? 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

TransactionManager를 주입받도록 변경했습니다

}

@Override
public User findById(final long id) {
return userService.findById(id);
}
Copy link

Choose a reason for hiding this comment

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

여기는 트랜잭션 처리를 안해주신 이유가 있나요? read-only여서인가요?

Copy link
Author

Choose a reason for hiding this comment

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

넵 단순 조회이기도 하고 반환값이 있는 함수형 인터페이스 새로 만들어줘야해서였는데
생각해보니 자바 기본 함수형 인터페이스가 있었네요🥲


@Override
public void insert(final User user) {
transactionManager.run(() -> userService.insert(user));
}

@Override
public void changePassword(final long id, final String newPassword, final String createBy) {
transactionManager.run(() -> userService.changePassword(id, newPassword, createBy));
}

}
55 changes: 4 additions & 51 deletions app/src/main/java/com/techcourse/service/UserService.java
Original file line number Diff line number Diff line change
@@ -1,60 +1,13 @@
package com.techcourse.service;

import java.sql.Connection;
import java.sql.SQLException;
import java.util.Optional;

import com.techcourse.config.DataSourceConfig;
import com.techcourse.dao.UserDao;
import com.techcourse.dao.UserHistoryDao;
import com.techcourse.domain.User;
import com.techcourse.domain.UserHistory;
import org.springframework.dao.DataAccessException;

public class UserService {

private final UserDao userDao;
private final UserHistoryDao userHistoryDao;

public UserService(final UserDao userDao, final UserHistoryDao userHistoryDao) {
this.userDao = userDao;
this.userHistoryDao = userHistoryDao;
}

public Optional<User> findById(final long id) {
return userDao.findById(id);
}

public void insert(final User user) {
userDao.insert(user);
}
public interface UserService {

public void changePassword(final long id, final String newPassword, final String createBy) {
Connection connection = null;
try {
connection = DataSourceConfig.getInstance().getConnection();
connection.setAutoCommit(false);
User findById(final long id);

final User user = findById(id).orElseThrow();
user.changePassword(newPassword);
userDao.update(connection, user);
userHistoryDao.log(connection, new UserHistory(user, createBy));
void insert(final User user);

connection.commit();
} catch (final SQLException e) {
try {
connection.rollback();
} catch (final SQLException rollbackException) {
throw new DataAccessException(rollbackException);
}
throw new DataAccessException(e);
} finally {
try {
connection.close();
} catch (final SQLException e) {
throw new DataAccessException(e);
}
}
}
void changePassword(final long id, final String newPassword, final String createBy);

}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public MockUserHistoryDao(final JdbcTemplate jdbcTemplate) {
}

@Override
public void log(final Connection connection, final UserHistory userHistory) {
public void log(final UserHistory userHistory) {
throw new DataAccessException();
}

Expand Down
20 changes: 12 additions & 8 deletions app/src/test/java/com/techcourse/service/UserServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,31 +31,35 @@ void setUp() {
@Test
Copy link

Choose a reason for hiding this comment

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

여기 setUp()에서 사용되는 DatabasePopulatorUtils의 connection은 DataSourceUtils로 connection을 가져오지 않으신 이유가 있나요? DatabasePopulatorUtils가 실행하는 쿼리에는 트랜잭션이 적용되지 않아도 될까요?

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

Choose a reason for hiding this comment

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

ㅎㅎㅎ... 진짜 궁금해서 여쭤본거였는데.... ☺

Copy link
Author

@iamjooon2 iamjooon2 Oct 11, 2023

Choose a reason for hiding this comment

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

저는 setUp() 메서드에 DatabasePopulatorUtils라는 다른 종류의 클래스를 사용하는걸 미처 보지 못했어요
그리고 자연히 테스트에서도 동일하게 트랜잭션을 적용해야한다고 생각합니다.. 적용하지 않을 이유는 없다고 생각해요

놓친 것이 있다면 알려주심 감사하겠습니다..

void testChangePassword() {
final UserHistoryDao userHistoryDao = new UserHistoryDao(jdbcTemplate);
final UserService userService = new UserService(userDao, userHistoryDao);
final UserService userService = new AppUserService(userDao, userHistoryDao);

final String newPassword = "qqqqq";
final String createBy = "gugu";
userService.changePassword(1L, newPassword, createBy);

final User actual = userService.findById(1L).orElseThrow();
final User actual = userService.findById(1L);

assertThat(actual.getPassword()).isEqualTo(newPassword);
}

@Test
void testTransactionRollback() {
// 트랜잭션 롤백 테스트를 위해 mock으로 교체
final UserHistoryDao userHistoryDao = new MockUserHistoryDao(jdbcTemplate);
final UserService userService = new UserService(userDao, userHistoryDao);
final var userHistoryDao = new MockUserHistoryDao(jdbcTemplate);
// 애플리케이션 서비스
final UserService appUserService = new AppUserService(userDao, userHistoryDao);
// 트랜잭션 서비스 추상화
final UserService txUserService = new TxUserService(appUserService);

final String newPassword = "newPassword";
final String createBy = "gugu";
final var newPassword = "newPassword";
final var createBy = "gugu";
// 트랜잭션이 정상 동작하는지 확인하기 위해 의도적으로 MockUserHistoryDao에서 예외를 발생시킨다.
assertThrows(DataAccessException.class,
() -> userService.changePassword(1L, newPassword, createBy));
() -> txUserService.changePassword(1L, newPassword, createBy));

final User actual = userService.findById(1L).orElseThrow();
final var actual = txUserService.findById(1L);

assertThat(actual.getPassword()).isNotEqualTo(newPassword);
}

}
26 changes: 3 additions & 23 deletions jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.dao.DataAccessException;
import org.springframework.jdbc.datasource.DataSourceUtils;

public class JdbcTemplate {

Expand All @@ -23,26 +24,6 @@ public JdbcTemplate(final DataSource dataSource) {
this.dataSource = dataSource;
}

public void update(final Connection connection, final String sql, final Object... elements) {
execute(connection, sql, PreparedStatement::executeUpdate, elements);
}

private <T> T execute(
final Connection connection,
final String sql,
final PreparedStatementExecutor<T> executor,
final Object... elements
) {
try (final PreparedStatement preparedStatement = connection.prepareStatement(sql)) {
setElements(elements, preparedStatement);

return executor.action(preparedStatement);
} catch (final SQLException e) {
log.error(e.getMessage(), e);
throw new DataAccessException(e);
}
}

public void update(final String sql, final Object... elements) {
execute(sql, PreparedStatement::executeUpdate, elements);
}
Expand All @@ -52,9 +33,8 @@ private <T> T execute(
final PreparedStatementExecutor<T> executor,
final Object... elements
) {
try (final Connection connection = dataSource.getConnection();
final PreparedStatement preparedStatement = connection.prepareStatement(sql)
) {
final Connection connection = DataSourceUtils.getConnection(dataSource);
try (final PreparedStatement preparedStatement = connection.prepareStatement(sql)) {
setElements(elements, preparedStatement);

return executor.action(preparedStatement);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public static Connection getConnection(DataSource dataSource) throws CannotGetJd

public static void releaseConnection(Connection connection, DataSource dataSource) {
try {
TransactionSynchronizationManager.unbindResource(dataSource);
connection.close();
} catch (SQLException ex) {
throw new CannotGetJdbcConnectionException("Failed to close JDBC Connection");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package org.springframework.transaction;

import java.sql.Connection;
import java.sql.SQLException;

import javax.sql.DataSource;
import org.springframework.dao.DataAccessException;
import org.springframework.jdbc.datasource.DataSourceUtils;
import org.springframework.transaction.support.TransactionExecutor;

public class TransactionManager {

private final DataSource dataSource;

public TransactionManager(final DataSource dataSource) {
this.dataSource = dataSource;
}

public void run(final TransactionExecutor transactionExecutor) {
final Connection connection = DataSourceUtils.getConnection(dataSource);
try {
connection.setAutoCommit(false);

transactionExecutor.action();

connection.commit();
} catch (final SQLException e) {
rollBackConnection(connection);
throw new DataAccessException(e);
} finally {
DataSourceUtils.releaseConnection(connection, dataSource);
}
}
Copy link

Choose a reason for hiding this comment

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

여기도 템플릿 콜백패턴을 적용하면 로직이 더 깔끔해질 것 같습니다! 이미 알고 계실 것 같지만 리뷰가 넘 심심한 것 같아 쥐어짜내봅니다 😄 (반영은 안 해주셔도 됩니다!!!)

Copy link

Choose a reason for hiding this comment

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

지금은 TransactionManager가 하나밖에 없지만 Spring을 보면 HibernateTransactionManager같이 TransactionManager가 여러 개 있더군요! 추상화를 위해 rollBack을 메서드로 분리하신 것처럼 commit도 메서드로 분리하셔도 좋을 것 같습니다. 이 부분도 콜백패턴을 적용하면 자연스럽게 반영되겠군여. 쥐어짜낸 리뷰222 (반영안해주셔도 됩니다222)

Copy link
Author

Choose a reason for hiding this comment

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

소중한 리뷰 감사합니다 😊
첫 리뷰 반영하면서 템플릿 콜백으로 변경했어요...!
connection 부분은 메서드만 분리했습니다..!


private static void rollBackConnection(final Connection connection) {
try {
connection.rollback();
} catch (final SQLException e) {
throw new DataAccessException(e);
}
}
Copy link

Choose a reason for hiding this comment

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

덕분에 try depth가 줄어들었군요👍


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package org.springframework.transaction.support;

@FunctionalInterface
public interface TransactionExecutor {

void action();

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,30 @@

import javax.sql.DataSource;
import java.sql.Connection;
import java.util.HashMap;
import java.util.Map;

public abstract class TransactionSynchronizationManager {

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

private TransactionSynchronizationManager() {}
private TransactionSynchronizationManager() {
}

public static Connection getResource(DataSource key) {
return null;
return getResources().get(key);
}

public static void bindResource(DataSource key, Connection value) {
getResources().put(key, value);
}

public static void unbindResource(DataSource key) {
getResources().remove(key);
}
Comment on lines +23 to 25
Copy link

Choose a reason for hiding this comment

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

저도 가비랑 동일하게 코드를 짰었는데 소나가 "ThreadLocal" variables should be cleaned up when no longer used라고 경고를 띄워주더라고요!! Thread Pool을 사용할 경우, 사용한 ThreadLocal을 remove해주지 않으면 예상치못하게 동작할 수도 있다고 해요. (참고한 글)
물론 지금은 Thread Pool을 사용하지 않아 remove를 해주지 않아도 될 것 같지만, ThreadLocal을 사용할 땐 의식적으로 값이 비었을 경우 remove해줘도 괜찮을 것 같다... 는 생각이 들었지만 제 뇌피셜입니다


public static Connection unbindResource(DataSource key) {
return null;
private static Map<DataSource, Connection> getResources() {
return resources.get();
}
Comment on lines +27 to 29
Copy link

Choose a reason for hiding this comment

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

resources.get()의 반복 사용이 없어졌네요 👍👍


}
2 changes: 1 addition & 1 deletion jdbc/src/test/java/nextstep/jdbc/JdbcTemplateTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class queryForObject는 {


@Test
void 결과가_존재하지_않으면_Optional을_반환한다() throws Exception {
void 결과가_존재하지_않으면_Optional_empty를_반환한다() throws Exception {
// given
when(resultSet.next()).thenReturn(false);
final String sql = "select * from users where id = ?";
Expand Down