Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 0 additions & 18 deletions app/src/main/java/com/techcourse/dao/UserDao.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import com.interface21.jdbc.core.RowMapper;

import javax.sql.DataSource;
import java.sql.Connection;
import java.util.List;
import java.util.NoSuchElementException;

Expand Down Expand Up @@ -47,16 +46,6 @@ public void update(final User user) {
});
}

public void update(final Connection connection, final User user) {
final var sql = "update users set account = ?, password = ?, email = ? where id = ?";
jdbcTemplate.update(connection, sql, pstmt -> {
pstmt.setString(1, user.getAccount());
pstmt.setString(2, user.getPassword());
pstmt.setString(3, user.getEmail());
pstmt.setLong(4, user.getId());
});
}

public List<User> findAll() {
final var sql = "select id, account, password, email from users";
return jdbcTemplate.queryForObjects(sql, USER_ROW_MAPPER);
Expand All @@ -69,13 +58,6 @@ public User findById(final Long id) {
.orElseThrow(() -> new NoSuchElementException("User not found with id: " + id));
}

public User findById(final Connection connection, final Long id) {
final var sql = "select id, account, password, email from users where id = ?";
return jdbcTemplate.queryForObject(connection, sql, USER_ROW_MAPPER,
pstmt -> pstmt.setLong(1, id))
.orElseThrow(() -> new NoSuchElementException("User not found with id: " + id));
}

public User findByAccount(final String account) {
final var sql = "select id, account, password, email from users where account = ?";
return jdbcTemplate.queryForObject(sql, USER_ROW_MAPPER,
Expand Down
13 changes: 0 additions & 13 deletions app/src/main/java/com/techcourse/dao/UserHistoryDao.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import com.interface21.jdbc.core.JdbcTemplate;

import javax.sql.DataSource;
import java.sql.Connection;

public class UserHistoryDao {

Expand All @@ -29,16 +28,4 @@ public void log(final UserHistory userHistory) {
pstmt.setString(6, userHistory.getCreateBy());
});
}

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, pstmt -> {
pstmt.setLong(1, userHistory.getUserId());
pstmt.setString(2, userHistory.getAccount());
pstmt.setString(3, userHistory.getPassword());
pstmt.setString(4, userHistory.getEmail());
pstmt.setObject(5, userHistory.getCreatedAt());
pstmt.setString(6, userHistory.getCreateBy());
});
}
}
30 changes: 30 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,30 @@
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);
}

@Override
public void changePassword(final long id, final String newPassword, final String createdBy) {
final var user = userDao.findById(id);
user.changePassword(newPassword);
userDao.update(user);
userHistoryDao.log(new UserHistory(user, createdBy));
}
}
51 changes: 51 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,51 @@
package com.techcourse.service;

import com.interface21.dao.DataAccessException;
import com.interface21.jdbc.datasource.DataSourceUtils;
import com.interface21.transaction.support.TransactionSynchronizationManager;
import com.techcourse.domain.User;

import javax.sql.DataSource;
import java.sql.Connection;
import java.sql.SQLException;

public class TxUserService implements UserService {

Choose a reason for hiding this comment

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

필드로 UserService 들고있는 이상 impl 할 필요없을 것 같은데, 어떻게 생각하시나요?


private final UserService userService;
private final DataSource dataSource;

public TxUserService(final UserService userService, final DataSource dataSource) {
this.userService = userService;
this.dataSource = dataSource;
}

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

@Override
public void changePassword(final long id, final String newPassword, final String createdBy) {
Connection connection = DataSourceUtils.getConnection(dataSource);
try {

Choose a reason for hiding this comment

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

Connection 을 필드로 가지고 있는 객체가 나오면 좋을 것 같은데, 어떻게 생각하시나요?
이름은 Transaction 이 될 것 같아요.
해당 로직이 조금 추상화 된다면 좋을 것 같아서요. 폰병장님의 생각이 궁금하네요.

connection.setAutoCommit(false);

try {
userService.changePassword(id, newPassword, createdBy);
connection.commit();
} catch (Exception e) {
try {
connection.rollback();
} catch (SQLException rollbackEx) {
e.addSuppressed(rollbackEx);
}
throw new DataAccessException("Failed to change password for user id: " + id, e);
}
} catch (SQLException e) {
throw new DataAccessException("Failed to manage transaction", e);
} finally {
TransactionSynchronizationManager.unbindResource(dataSource);
DataSourceUtils.releaseConnection(connection, dataSource);

Choose a reason for hiding this comment

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

unbindResource 메서드 releaseConnection 에서 호출해도 되지 않나요?
connection, dataSource 처리 지점이 두 곳이라 추적이 어려울 것 같아요.

}
Comment on lines +28 to +49
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

트랜잭션 소유권(ownership)과 중첩 트랜잭션 참여(propagation) 미정의

현재 구현은 다음 두 가지 위험이 있습니다.

  • 이미 바인딩된 커넥션이 있는 경우에도 setAutoCommit(false)·commit/rollback·unbind를 무조건 수행합니다. 외부(상위) 트랜잭션이 존재하면 소유하지 않은 리소스를 해제하여 흐름을 깨뜨릴 수 있어요.
  • finally에서 항상 unbindResource 호출 → 자신이 시작하지 않은 바인딩까지 해제할 수 있습니다.

질문/힌트

  • “내가 트랜잭션을 시작했는가?”를 표시하는 플래그(참여 여부)로 commit/rollback 및 unbind 실행 조건을 분기하면 어떨까요?
  • 전파 속성(필요 시 REQUIRED, REQUIRES_NEW 등)과 ‘참여/비참여’ 개념을 최소한의 형태로라도 명시해볼 수 있을까요?
  • 커넥션이 이미 바인딩되어 있다면 setAutoCommit 상태 변경을 피해야 하지 않을까요?
    코딩 가이드라인 기준
🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/service/TxUserService.java around lines 28
to 49, the method unconditionally takes ownership of the connection and
transaction lifecycle even when a connection is already bound by an external
transaction; change it to detect whether a connection/transaction is already
bound and only manage autoCommit/commit/rollback/unbind when this method
actually started the transaction. Concretely: check
TransactionSynchronizationManager.hasResource(dataSource) (or equivalent) before
calling setAutoCommit(false) and mark a boolean "startedTx"; perform
commit/rollback and TransactionSynchronizationManager.unbindResource(dataSource)
only if startedTx is true (and avoid changing autoCommit for bound connections);
always release the connection via DataSourceUtils.releaseConnection in finally.
This implements minimal propagation semantics (participate if bound, start new
otherwise) and prevents unbinding resources you don't own.

}
}
53 changes: 3 additions & 50 deletions app/src/main/java/com/techcourse/service/UserService.java
Original file line number Diff line number Diff line change
@@ -1,56 +1,9 @@
package com.techcourse.service;

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

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

public class UserService {

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

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

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

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

public void changePassword(final long id, final String newPassword, final String createBy) {
try (Connection connection = dataSource.getConnection()) {
connection.setAutoCommit(false);

try {
final var user = userDao.findById(connection, id);
user.changePassword(newPassword);
userDao.update(connection, user);
userHistoryDao.log(connection, new UserHistory(user, createBy));

connection.commit();
} catch (Exception e) {
try {
connection.rollback();
} catch (SQLException rollbackEx) {
e.addSuppressed(rollbackEx);
}
throw new DataAccessException("Failed to change password for user id: " + id, e);
}
} catch (SQLException e) {
throw new DataAccessException("Failed to get connection", e);
}
}
User findById(final long id);
void changePassword(final long id, final String newPassword, final String createdBy);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,12 @@
import com.interface21.dao.DataAccessException;
import com.interface21.jdbc.core.JdbcTemplate;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;

class UserServiceTest {
class AppUserServiceTest {

private JdbcTemplate jdbcTemplate;
private UserDao userDao;
Expand All @@ -32,7 +31,8 @@ void setUp() {
@Test
void testChangePassword() {
final var userHistoryDao = new UserHistoryDao(DataSourceConfig.getInstance());
final var userService = new UserService(userDao, userHistoryDao, DataSourceConfig.getInstance());
final var appUserService = new AppUserService(userDao, userHistoryDao);
final var userService = new TxUserService(appUserService, DataSourceConfig.getInstance());

final var newPassword = "qqqqq";
final var createBy = "gugu";
Expand All @@ -47,7 +47,8 @@ void testChangePassword() {
void testTransactionRollback() {
// 트랜잭션 롤백 테스트를 위해 mock으로 교체
final var userHistoryDao = new MockUserHistoryDao(DataSourceConfig.getInstance());
final var userService = new UserService(userDao, userHistoryDao, DataSourceConfig.getInstance());
final var appUserService = new AppUserService(userDao, userHistoryDao);
final var userService = new TxUserService(appUserService, DataSourceConfig.getInstance());

final var newPassword = "newPassword";
final var createBy = "gugu";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import com.interface21.jdbc.core.JdbcTemplate;

import javax.sql.DataSource;
import java.sql.Connection;

public class MockUserHistoryDao extends UserHistoryDao {

Expand All @@ -22,9 +21,4 @@ public MockUserHistoryDao(final DataSource dataSource) {
public void log(final UserHistory userHistory) {
throw new DataAccessException();
}

@Override
public void log(final Connection connection, final UserHistory userHistory) {
throw new DataAccessException();
}
}
19 changes: 15 additions & 4 deletions jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.interface21.jdbc.core;

import com.interface21.dao.DataAccessException;
import com.interface21.jdbc.datasource.DataSourceUtils;
import java.util.Optional;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -52,11 +53,16 @@ public <T> List<T> queryForObjects(final String sql, final RowMapper<T> rowMappe
}

public <T> Optional<T> queryForObject(final String sql, final RowMapper<T> rowMapper, final PreparedStatementSetter setter) {
try (Connection conn = dataSource.getConnection()) {
Connection conn = DataSourceUtils.getConnection(dataSource);
try {
return queryForObject(conn, sql, rowMapper, setter);
} catch (SQLException e) {
} catch (DataAccessException e) {
throw e;
} catch (Exception e) {
log.error("SQL execution failed: {}", sql, e);
throw new DataAccessException(e);
} finally {
DataSourceUtils.releaseConnection(conn, dataSource);
}
}

Expand All @@ -79,11 +85,16 @@ public <T> Optional<T> queryForObject(final Connection connection, final String
}

public <T> T execute(final String sql, final PreparedStatementCallback<T> callback, final PreparedStatementSetter setter) {
try (Connection conn = dataSource.getConnection()) {
Connection conn = DataSourceUtils.getConnection(dataSource);
try {
return execute(conn, sql, callback, setter);
} catch (SQLException e) {
} catch (DataAccessException e) {
throw e;
} catch (Exception e) {
log.error("SQL execution failed: {}", sql, e);
throw new DataAccessException(e);
} finally {
DataSourceUtils.releaseConnection(conn, dataSource);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,18 @@ public static Connection getConnection(DataSource dataSource) throws CannotGetJd
}

public static void releaseConnection(Connection connection, DataSource dataSource) {
if (connection == null) {
return;
}

// 트랜잭션 관리 중인 Connection인지 확인
Connection boundConnection = TransactionSynchronizationManager.getResource(dataSource);
if (connection == boundConnection) {
// 트랜잭션 관리 중인 Connection은 닫지 않음
return;
}

// 트랜잭션 관리되지 않는 Connection만 닫기
try {
connection.close();
} catch (SQLException ex) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

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

public abstract class TransactionSynchronizationManager {
Expand All @@ -11,13 +12,31 @@ public abstract class TransactionSynchronizationManager {
private TransactionSynchronizationManager() {}

public static Connection getResource(DataSource key) {
return null;
Map<DataSource, Connection> map = resources.get();
if (map == null) {
return null;
}
return map.get(key);
}

public static void bindResource(DataSource key, Connection value) {
Map<DataSource, Connection> map = resources.get();
if (map == null) {
map = new HashMap<>();
resources.set(map);
}
map.put(key, value);
}
Comment on lines 22 to 29
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

중복 바인딩에 대한 처리를 고려해보셨나요?

현재 bindResource 메서드는 이미 바인딩된 DataSource에 대해 새로운 Connection을 덮어쓸 수 있습니다. 만약 동일한 DataSource에 대해 중복으로 bindResource가 호출된다면 어떤 일이 발생할까요?

고려할 점:

  • 이전에 바인딩된 Connection이 제대로 정리되지 않은 채 새로운 Connection으로 교체되면 어떤 문제가 발생할 수 있을까요?
  • 중복 바인딩을 감지하고 예외를 던지거나, 로깅을 추가하는 것이 더 안전하지 않을까요?

힌트:

if (map.containsKey(key)) {
    // 이미 바인딩된 경우 처리
}
🤖 Prompt for AI Agents
In
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java
around lines 22 to 29, bindResource currently overwrites an existing Connection
for the same DataSource without handling the previous binding; update the method
to detect duplicate binds (e.g. map.containsKey(key)) and handle them by either
throwing an IllegalStateException with a clear message or logging a warning
before replacing, and if replacing ensure the previous Connection is cleaned
up/closed or explicitly left for caller responsibility; implement the chosen
behavior consistently and document it in the method's Javadoc.


public static Connection unbindResource(DataSource key) {
return null;
Map<DataSource, Connection> map = resources.get();

Choose a reason for hiding this comment

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

void 여도 될 것 같아요

if (map == null) {
return null;
}
Connection connection = map.remove(key);
if (map.isEmpty()) {
resources.remove();
}
return connection;
}
}