Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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
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
Expand Up @@ -3,7 +3,6 @@
import com.interface21.jdbc.core.JdbcTemplate;
import com.interface21.jdbc.core.RowMapper;
import com.techcourse.domain.User;
import java.sql.Connection;
import java.util.List;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -35,14 +34,6 @@ public void update(final User user) {
jdbcTemplate.update(sql, user.getPassword(), user.getEmail(), user.getId());
}

/*
Connection 공유 버전 overloading
*/
public void update(final Connection connection, final User user) {
final var sql = "update users set password = ?, email = ? where id = ?";
jdbcTemplate.update(connection, sql, user.getPassword(), user.getEmail(), user.getId());
}

public List<User> findAll() {
final var sql = "select id, account, password, email from users";
return jdbcTemplate.query(sql, USER_ROW_MAPPER);
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
Expand Up @@ -2,7 +2,6 @@

import com.interface21.jdbc.core.JdbcTemplate;
import com.techcourse.domain.UserHistory;
import java.sql.Connection;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -27,20 +26,4 @@ public void log(final UserHistory userHistory) {
userHistory.getCreateBy()
);
}

/*
Connection 공유 버전 overloading
*/
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()
);
}
}
35 changes: 35 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,35 @@
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 save(final User user) {
userDao.insert(user);
}

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

import com.interface21.dao.DataAccessException;
import com.interface21.jdbc.datasource.DataSourceUtils;
import com.techcourse.domain.User;
import java.sql.Connection;
import java.sql.SQLException;
import java.util.function.Consumer;
import javax.sql.DataSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class TxUserService implements UserService {

private static final Logger log = LoggerFactory.getLogger(TxUserService.class);

private final UserService userService;
private final DataSource dataSource;

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

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

@Override
public void save(User user) {
userService.save(user);
}

@Override
public void changePassword(final long id, final String newPassword, final String createdBy){
try {
doInTransaction(connection -> {
userService.changePassword(id, newPassword, createdBy);
});
} catch (SQLException e) {
throw new DataAccessException(e);
}
}
Comment on lines +37 to +45
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

🧩 Analysis chain

예외 전파/전환 정책 불일치 — 현재 SQLException 전환 경로가 사실상 작동하지 않음

doInTransaction 내부에서 모든 예외를 잡아 RuntimeException으로 재던지므로, changePassword의 catch(SQLException)은 도달하지 않습니다. 예외 전환 정책을 한 곳으로 모아 일관되게 유지하세요. 선택지는 대략 두 가지입니다:

  • 트랜잭션 경계(doInTransaction)에서 표준 런타임 예외(예: DataAccessException 파생)로 전환해 상위는 그대로 전파받게 하거나,
  • doInTransaction이 체크 예외를 전달하고, 퍼블릭 서비스 계층에서만 DataAccessException으로 일괄 전환.

현재 구조에 맞는 하나의 정책을 정하고 전역으로 일치시켜보세요. As per coding guidelines.

Also applies to: 46-58


예외 처리 정책 일관성을 검증하기 위해 저장소를 탐색하겠습니다.


---

🏁 Script executed:

```shell
#!/bin/bash
# Find TxUserService and related transaction handler files
fd -e java -E target -E build | head -20

Length of output: 1262


🏁 Script executed:

#!/bin/bash
# Look at the TxUserService file structure
find . -name "TxUserService.java" -type f 2>/dev/null

Length of output: 114


🏁 Script executed:

#!/bin/bash
# Search for doInTransaction method implementation
rg -n "doInTransaction" --type java -A 10 -B 2

Length of output: 2147


🏁 Script executed:

#!/bin/bash
# Get the complete TxUserService file to understand full exception handling pattern
rg -n "class TxUserService" --type java -A 50 2>/dev/null | head -80

Length of output: 7096



---

🏁 Script executed:

```shell
#!/bin/bash
# Get full TxUserService.java file
cat ./app/src/main/java/com/techcourse/service/TxUserService.java

Length of output: 2415


🏁 Script executed:

#!/bin/bash
# Search for handleTransactionException method
rg -n "handleTransactionException" --type java -A 5 -B 2

Length of output: 1377


🏁 Script executed:

#!/bin/bash
# Search for closeConnection to understand complete flow
rg -n "closeConnection" --type java -A 5 -B 2

Length of output: 1444


예외 전파 정책 불일치 확인됨 — changePassword의 SQLException 캐치 블록은 도달 불가능

현재 코드의 문제점:

  1. handleTransactionException (60줄)에서 모든 예외를 RuntimeException으로 변환하여 던짐

    • throw e instanceof RuntimeException ? (RuntimeException) e : new RuntimeException(e);
  2. doInTransaction은 "throws SQLException" 선언하지만 실제로는 RuntimeException만 발생

    • 선언과 구현의 불일치
  3. changePassword의 catch(SQLException e) 블록 (41줄)은 실행되지 않음

    • RuntimeException은 SQLException이 아니므로 캐치되지 않음

선택할 수 있는 일관된 정책:

  • 옵션 1: 트랜잭션 경계(doInTransaction)에서만 예외 전환 → doInTransaction이 throws SQLException 선언 제거하고 RuntimeException만 전파하도록 수정
  • 옵션 2: 퍼블릭 메서드 계층에서만 예외 전환 → changePassword와 모든 공개 메서드에서 일관되게 처리

하나의 정책을 선택하여 doInTransaction, handleTransactionException, changePassword 간 예외 처리 흐름을 일관되게 재설계하세요.

🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/service/TxUserService.java around lines 36
to 44, the current catch(SQLException) in changePassword is unreachable because
doInTransaction/handleTransactionException convert exceptions to
RuntimeException; choose the consistent policy of propagating RuntimeException
only: remove the "throws SQLException" from doInTransaction signature and ensure
handleTransactionException continues converting to RuntimeException, then update
changePassword to stop catching SQLException and instead catch RuntimeException
(or let it propagate) and wrap it in DataAccessException so public API surfaces
DataAccessException consistently.


private void doInTransaction(Consumer<Connection> consumer) throws SQLException {
Connection connection = DataSourceUtils.getConnection(dataSource);
connection.setAutoCommit(false);
try {
consumer.accept(connection);
connection.commit();
} catch (Exception e) {
log.error("트랜잭션 수행 중 예외 발생");
handleTransactionException(e, connection);
} finally {
closeConnection(connection);
}
}

private void handleTransactionException(Exception e, Connection connection) {
if(connection != null) {
try { // 커넥션이 존재할 경우 롤백 시도
connection.rollback();
log.info("Rollback 완료");
} catch (SQLException ex) { // 롤백 도중 예외 발생
log.error("Rollback 실패", ex);
}
}
throw e instanceof RuntimeException ? (RuntimeException) e : new RuntimeException(e);
}

private void closeConnection(Connection connection) {
if(connection != null){
try {
connection.close();
DataSourceUtils.releaseConnection(connection, dataSource);
} catch (SQLException ignored) {}
}
}
}
77 changes: 4 additions & 73 deletions app/src/main/java/com/techcourse/service/UserService.java
Original file line number Diff line number Diff line change
@@ -1,79 +1,10 @@
package com.techcourse.service;

import com.techcourse.dao.UserDao;
import com.techcourse.dao.UserHistoryDao;
import com.techcourse.domain.User;
import com.techcourse.domain.UserHistory;
import java.sql.Connection;
import java.sql.SQLException;
import java.util.function.Consumer;
import javax.sql.DataSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class UserService {
public interface UserService {

private static final Logger log = LoggerFactory.getLogger(UserService.class);

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

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

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) {
doInTransaction(connection -> {
final var user = findById(id);
user.changePassword(newPassword);
userDao.update(connection, user);
userHistoryDao.log(connection, new UserHistory(user, createBy));
});
}

private void doInTransaction(Consumer<Connection> consumer){
Connection connection = null;
try {
connection = dataSource.getConnection();
connection.setAutoCommit(false);
consumer.accept(connection);
connection.commit();
} catch (Exception e) {
log.error("비밀번호 변경 중 예외 발생");
handleTransactionException(e, connection);
} finally {
closeConnection(connection);
}
}

private void handleTransactionException(Exception e, Connection connection) {
if(connection != null) {
try { // 커넥션이 존재할 경우 롤백 시도
connection.rollback();
log.info("Rollback 완료");
} catch (SQLException ex) { // 롤백 도중 예외 발생
log.error("Rollback 실패", ex);
}
}
throw e instanceof RuntimeException ? (RuntimeException) e : new RuntimeException(e);
}

private void closeConnection(Connection connection) {
if(connection != null){ // 커넥션이 존재할 경우 close
try {
connection.close();
} catch (SQLException ignored) {}
}
}
User findById(final long id);
void save(final User user);
void changePassword(final long id, final String newPassword, final String createdBy);
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import com.interface21.jdbc.core.JdbcTemplate;
import com.techcourse.dao.UserHistoryDao;
import com.techcourse.domain.UserHistory;
import java.sql.Connection;

public class MockUserHistoryDao extends UserHistoryDao {

Expand All @@ -13,7 +12,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();
}
}
24 changes: 6 additions & 18 deletions app/src/test/java/com/techcourse/service/UserServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import com.interface21.jdbc.core.JdbcTemplate;
import com.techcourse.config.DataSourceConfig;
import com.techcourse.dao.UserDao;
import com.techcourse.dao.UserHistoryDao;
import com.techcourse.domain.User;
import com.techcourse.support.jdbc.init.DatabasePopulatorUtils;
import javax.sql.DataSource;
Expand All @@ -31,31 +30,20 @@ void setUp() {
userDao.insert(user);
}

@Test
void testChangePassword() {
final var userHistoryDao = new UserHistoryDao(jdbcTemplate);
final var userService = new UserService(dataSource, userDao, userHistoryDao);

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

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

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

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

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

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

Expand Down
24 changes: 5 additions & 19 deletions jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.interface21.dao.DataAccessException;
import com.interface21.jdbc.IncorrectResultSizeException;
import com.interface21.jdbc.datasource.DataSourceUtils;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
Expand All @@ -23,22 +24,8 @@ public JdbcTemplate(final DataSource dataSource) {
}

public void update(String sql, Object... params){
try (Connection conn = dataSource.getConnection();
PreparedStatement pstmt = conn.prepareStatement(sql)){
log.debug("query : {}", sql);
setParameters(pstmt, params);
pstmt.executeUpdate();
} catch (SQLException e) {
log.error(e.getMessage(), e);
throw new DataAccessException(e.getMessage(), e);
}
}

/*
Connection 공유 버전 오버로딩
*/
public void update(Connection connection, String sql, Object... params){
try (PreparedStatement pstmt = connection.prepareStatement(sql)){
Connection conn = DataSourceUtils.getConnection(dataSource);
Copy link
Member

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.

그렇네요!!
짱구 말대로 finally에 DataSourceUtils.releaseConnection()을 추가해주는게 좋을 것 같아서 추가했습니다!
좋은 피드백 감사합니다!!

Copy link
Member

Choose a reason for hiding this comment

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

좋습니다 👍

try (PreparedStatement pstmt = conn.prepareStatement(sql)){
log.debug("query : {}", sql);
setParameters(pstmt, params);
pstmt.executeUpdate();
Expand All @@ -49,9 +36,8 @@ public void update(Connection connection, String sql, Object... params){
}

public <T> List<T> query(String sql, RowMapper<T> rowMapper, Object... params){
try (Connection conn = dataSource.getConnection();
PreparedStatement pstmt = conn.prepareStatement(sql)){

Connection conn = DataSourceUtils.getConnection(dataSource);
try (PreparedStatement pstmt = conn.prepareStatement(sql)){
log.debug("query : {}", sql);
setParameters(pstmt, params);
return executionResult(rowMapper, pstmt);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

import com.interface21.jdbc.CannotGetJdbcConnectionException;
import com.interface21.transaction.support.TransactionSynchronizationManager;

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

// 4단계 미션에서 사용할 것
public abstract class DataSourceUtils {
Expand All @@ -30,6 +29,7 @@ public static Connection getConnection(DataSource dataSource) throws CannotGetJd
public static void releaseConnection(Connection connection, DataSource dataSource) {
try {
connection.close();
TransactionSynchronizationManager.unbindResource(dataSource);
Copy link
Member

Choose a reason for hiding this comment

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

connection.close() 도중에 예외가 발생하면 커넥션을 unbind할 수 없을 것 같습니다!
TransactionSynchronizationManager.unbindResource(dataSource)가 먼저 실행되게 하는게 좋을 것 같은데 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

놓친 부분 꼼꼼하게 피드백 해주셔서 감사해요!!

다시 고민해보니 트랜잭션에 대해 unbind 하는 역할은 (TransactionSynchronizationManager.unbindResource(dataSource)) TxUserService에 위임하는 것이 적절할 것 같아서 이동했습니다!

fix: 트랜잭션 존재 여부 판단을 DataSourceUtils에서 하도록 위임

Copy link
Member

Choose a reason for hiding this comment

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

확인했습니다 👍

다만, 트랜잭션 외부에서 실행되는 쿼리는 TransactionSynchronizationManager.unbindResource()가 되지 않고 있어요. 따라서 DataSourceUtils.releaseConnection()에서 커넥션이 닫히지 않는 것 같습니다.
한 번 확인해보셔도 좋을 것 같아요!

} catch (SQLException ex) {
throw new CannotGetJdbcConnectionException("Failed to close JDBC Connection");
}
Expand Down
Loading