Skip to content

[4단계 - Transaction synchronization 적용하기] 모코(송선권) 미션 제출합니다. #1173

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: songsunkook
Choose a base branch
from
+249 −165
Open

[4단계 - Transaction synchronization 적용하기] 모코(송선권) 미션 제출합니다. #1173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc174da
test: 요구사항 테스트 코드 반영
songsunkook Oct 19, 2025
c72259b
refactor: Service 프록시 적용
songsunkook Oct 19, 2025
0fb61dc
refactor: 트랜잭션 매니저 분리
songsunkook Oct 19, 2025
1249556
fix: 트랜잭션 정리 로직에서 메모리 누수 해결
songsunkook Oct 19, 2025
fce0b6c
fix: 중첩 트랜잭션의 롤백이 의도치 않게 동작하는 버그 수정
songsunkook Oct 19, 2025
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
3 changes: 2 additions & 1 deletion app/src/main/java/com/techcourse/config/TransactionManagerConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import javax.sql.DataSource;

import com.interface21.dao.DataAccessException;
import com.interface21.jdbc.core.GeneralTransactionManager;
import com.interface21.jdbc.core.TransactionManager;

public class TransactionManagerConfig {
Expand All @@ -15,7 +16,7 @@ public class TransactionManagerConfig {
public static TransactionManager getInstance() {
if (Objects.isNull(INSTANCE)) {
DataSource dataSource = DataSourceConfig.getInstance();
INSTANCE = new TransactionManager(dataSource);
INSTANCE = new GeneralTransactionManager(dataSource);
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

싱글턴 초기화 동시성 안전성 보강 필요

멀티스레드 환경에서 getInstance 동시 호출 시 중복 초기화 가능성이 있습니다. 정적 홀더/enum/volatile DCL 등 확실한 초기화 전략을 고려해보셨나요?

🤖 Prompt for AI Agents 
In app/src/main/java/com/techcourse/config/TransactionManagerConfig.java around
line 19, the singleton INSTANCE is assigned directly (INSTANCE = new
GeneralTransactionManager(dataSource)) allowing race conditions on concurrent
getInstance calls; change to a thread-safe initialization strategy such as: make
INSTANCE volatile and implement double-checked locking in getInstance, or use
the static holder idiom (private static class Holder { static final
GeneralTransactionManager INSTANCE = new GeneralTransactionManager(...); }) or
convert to an enum singleton; update getInstance accordingly to ensure only one
instance is created safely under concurrency.

}
return INSTANCE;
}
Expand Down
26 changes: 13 additions & 13 deletions app/src/main/java/com/techcourse/dao/UserDao.java
View file
Open in desktop
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 org.slf4j.Logger;
Expand All @@ -9,6 +8,7 @@
import com.interface21.jdbc.core.JdbcTemplate;
import com.interface21.jdbc.core.jpa.SimpleJpa;
import com.techcourse.config.JpaConfig;
import com.techcourse.config.TransactionManagerConfig;
import com.techcourse.domain.User;

public class UserDao {
Expand All @@ -23,35 +23,35 @@ public UserDao(final JdbcTemplate jdbcTemplate) {
this.simpleJpa = new SimpleJpa(jdbcTemplate, JpaConfig.BASE_PACKAGE);
}

public void insert(Connection connection, final User user) {
simpleJpa.insert(connection, user);
public void insert(final User user) {
simpleJpa.insert(TransactionManagerConfig.getCurrentConnection(), user);
Copy link

Choose a reason for hiding this comment

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

TransactionManagerConfig로 트랜잭션 경계를 설정하는 건 정말 좋은데요?
UserDao 내부에서 getCurrentConnection()을 호출해서 Connection을 직접 가져와 SimpleJpa에 넘겨주신 이유가 궁금해요!

DAO 레벨에서는 Connection을 전혀 모르게 해서 simplaJpa에서 TransactionManagerConfig.getCurrentConnection()을 호출하는 방법과 비교했을때, 이렇게 하신 이유는 뭔가요?

}

public void update(Connection connection, final User user) {
simpleJpa.update(connection, user);
public void update(final User user) {
simpleJpa.update(TransactionManagerConfig.getCurrentConnection(), user);
}

public List<User> findAll(Connection connection) {
return simpleJpa.selectAll(connection, User.class);
public List<User> findAll() {
return simpleJpa.selectAll(TransactionManagerConfig.getCurrentConnection(), User.class);
}

public User findById(Connection connection, final Long id) {
List<User> users = simpleJpa.selectById(connection, User.class, id);
public User findById(final Long id) {
List<User> users = simpleJpa.selectById(TransactionManagerConfig.getCurrentConnection(), User.class, id);
if (users.size() > 1) {
throw new IllegalStateException("Multiple users found with id: " + id);
}
return users.isEmpty() ? null : users.getFirst();
}
Comment on lines 43 to 44
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

3항 연산자 사용 금지 위반

두 곳의 반환부에서 삼항 연산자를 사용하고 있습니다. 코딩 가이드(추가 프로그래밍 요구사항) 위반이므로, 가드 클로즈나 명시적 분기로 풀어 가독성을 높여주세요. 왜 중요한가: 조건이 늘어날수록 가독성이 급격히 떨어집니다.

[As per coding guidelines]

Also applies to: 51-52

🤖 Prompt for AI Agents 
In app/src/main/java/com/techcourse/dao/UserDao.java around lines 43-44 (and
similarly at 51-52), replace the ternary return expressions with a guard-clause
or explicit branching: check if the users collection is empty and return null
early, otherwise return users.getFirst(); do the same for the other occurrence
so both return paths use clear if/else or early-return style instead of the ? :
operator to comply with the coding guideline.


public User findByAccount(Connection connection, final String account) {
List<User> users = simpleJpa.selectByColumn(connection, User.class, "account", account);
public User findByAccount(final String account) {
List<User> users = simpleJpa.selectByColumn(TransactionManagerConfig.getCurrentConnection(), User.class, "account", account);
if (users.size() > 1) {
throw new IllegalStateException("Multiple users found with account: " + account);
}
return users.isEmpty() ? null : users.get(0);
}

public void delete(Connection connection, User user) {
simpleJpa.deleteById(connection, user.getClass(), user.getId());
public void delete(User user) {
simpleJpa.deleteById(TransactionManagerConfig.getCurrentConnection(), user.getClass(), user.getId());
Copy link

Choose a reason for hiding this comment

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

지금 UserDao는 모든 메서드가 TransactionManagerConfig를 통해 얻은 Connection을 사용하는 것 같아요. 만약 트랜잭션이 전혀 필요 없는 단순 조회 (예: findAll) 같은 경우에도 항상 트랜잭션 Connection을 사용하는 것이 괜찮다고 생각하셨을까요?

}
}
7 changes: 3 additions & 4 deletions app/src/main/java/com/techcourse/dao/UserHistoryDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
package com.techcourse.dao;

import java.sql.Connection;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.interface21.jdbc.core.JdbcTemplate;
import com.interface21.jdbc.core.jpa.SimpleJpa;
import com.techcourse.config.JpaConfig;
import com.techcourse.config.TransactionManagerConfig;
import com.techcourse.domain.UserHistory;

public class UserHistoryDao {
Expand All @@ -22,7 +21,7 @@ public UserHistoryDao(final JdbcTemplate jdbcTemplate) {
this.simpleJpa = new SimpleJpa(jdbcTemplate, JpaConfig.BASE_PACKAGE);
}

public void log(Connection connection, final UserHistory userHistory) {
simpleJpa.insert(connection, userHistory);
public void log(final UserHistory userHistory) {
simpleJpa.insert(TransactionManagerConfig.getCurrentConnection(), userHistory);
}
Comment on lines +24 to 26
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

DAO가 정적 구성요소에 직접 의존 — 테스트 용이성/결합도 이슈

TransactionManagerConfig.getCurrentConnection()에 직접 의존하면 인프라 결합이 커집니다.

  • ConnectionProvider/TransactionManager 주입(DIP)이나 포트-어댑터 구조를 고려해보셨나요?
  • 트랜잭션 외부 호출 시 IllegalStateException이 발생하는데, 이 실패 모드를 명시적으로 관리할 방법(가드/검증)도 고민해보세요.
🤖 Prompt for AI Agents 
In app/src/main/java/com/techcourse/dao/UserHistoryDao.java around lines 24 to
26, the DAO directly calls TransactionManagerConfig.getCurrentConnection()
causing tight static coupling and testability issues; refactor to accept a
ConnectionProvider or TransactionManager (constructor- or setter-injected) and
use that injected dependency instead of the static call, and add a
guard/validation before using the connection that checks for presence (or throws
a clear IllegalStateException with a descriptive message) so callers and tests
can control/verify transaction context.

}
35 changes: 35 additions & 0 deletions app/src/main/java/com/techcourse/service/AppUserService.java
View file
Open in desktop
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 insert(final User user) {
userDao.insert(user);
}

@Override
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));
}
Comment on lines +28 to +34
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

사용자 미존재 시 NPE 위험

userDao.findById가 null을 반환할 수 있다면 user.changePassword에서 NPE가 납니다.

  • “찾을 수 없음”을 명시적 예외/Optional로 표현하고, 서비스에서 도메인 예외로 전환하는 방식을 고려해보셨나요?
🤖 Prompt for AI Agents 
In app/src/main/java/com/techcourse/service/AppUserService.java around lines
28-34, the code calls userDao.findById(...) then directly
user.changePassword(...), which risks an NPE if the DAO returns null; change the
flow to explicitly handle missing users: have the DAO return Optional<User> (or
check for null), and in the service translate absence into a domain/service
exception (e.g., throw new UserNotFoundException(id)) before any mutating calls;
only call changePassword, userDao.update, and userHistoryDao.log after the
presence check to guarantee no NPE and produce a clear error path for callers.

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

import com.techcourse.config.TransactionManagerConfig;
import com.techcourse.domain.User;

public class TxUserService implements UserService {

private final AppUserService appUserService;

public TxUserService(AppUserService appUserService) {
this.appUserService = appUserService;
}

public User findById(final long id) {
return TransactionManagerConfig.executeInTransaction(connection -> {
Copy link

Choose a reason for hiding this comment

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

이렇게 TxUserService를 통해 트랜잭션 관련 로직이 AppUserService와 분리되니 좋네요!

return appUserService.findById(id);
});
}

public void insert(final User user) {
TransactionManagerConfig.executeInTransaction(connection -> {
appUserService.insert(user);
});
}

public void changePassword(final long id, final String newPassword, final String createBy) {
TransactionManagerConfig.executeInTransaction(connection -> {
appUserService.changePassword(id, newPassword, createBy);
});
}
}
35 changes: 4 additions & 31 deletions app/src/main/java/com/techcourse/service/UserService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,12 @@
package com.techcourse.service;

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

public class UserService {
public interface UserService {

private final UserDao userDao;
private final UserHistoryDao userHistoryDao;
User findById(final long id);

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

public User findById(final long id) {
return TransactionManagerConfig.executeInTransaction(connection -> {
return userDao.findById(connection, id);
});
}

public void insert(final User user) {
TransactionManagerConfig.executeInTransaction(connection -> {
userDao.insert(connection, user);
});
}

public void changePassword(final long id, final String newPassword, final String createBy) {
TransactionManagerConfig.executeInTransaction(connection -> {
final var user = userDao.findById(connection, id);
user.changePassword(newPassword);
userDao.update(connection, user);
userHistoryDao.log(connection, new UserHistory(user, createBy));
});
}
void changePassword(final long id, final String newPassword, final String createBy);
}
62 changes: 38 additions & 24 deletions app/src/test/java/com/techcourse/dao/UserDaoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import com.interface21.jdbc.core.JdbcTemplate;
import com.techcourse.config.DataSourceConfig;
import com.techcourse.config.TransactionManagerConfig;
import com.techcourse.domain.User;
import com.techcourse.support.jdbc.init.DatabasePopulatorUtils;

Expand All @@ -28,62 +29,75 @@ void setup() throws SQLException {

userDao = new UserDao(new JdbcTemplate(datasource));
final var user = new User("gugu", "password", "hkkang@woowahan.com");
userDao.insert(connection, user);
TransactionManagerConfig.executeInTransaction(conn -> {
userDao.insert(user);
});
}

@Test
void findAll() {
final var users = userDao.findAll(connection);

assertThat(users).isNotEmpty();
TransactionManagerConfig.executeInTransaction(conn -> {
final var users = userDao.findAll();
assertThat(users).isNotEmpty();
});
}

@Test
void findById() {
final var user = userDao.findById(connection, 1L);
TransactionManagerConfig.executeInTransaction(conn -> {
final var user = userDao.findById(1L);

assertThat(user.getAccount()).isEqualTo("gugu");
assertThat(user.getAccount()).isEqualTo("gugu");
});
}

@Test
void findByAccount() {
final var account = "gugu";
final var user = userDao.findByAccount(connection, account);
TransactionManagerConfig.executeInTransaction(conn -> {
final var account = "gugu";
final var user = userDao.findByAccount(account);

assertThat(user.getAccount()).isEqualTo(account);
assertThat(user.getAccount()).isEqualTo(account);
});
}

@Test
void insert() {
final var account = "insert-gugu";
final var user = new User(account, "password", "hkkang@woowahan.com");
userDao.insert(connection, user);
TransactionManagerConfig.executeInTransaction(conn -> {
final var account = "insert-gugu";
final var user = new User(account, "password", "hkkang@woowahan.com");
userDao.insert(user);

final var actual = userDao.findById(connection, 2L);
final var actual = userDao.findById(2L);

assertThat(actual.getAccount()).isEqualTo(account);
assertThat(actual.getAccount()).isEqualTo(account);
});
}

@Test
void update() {
final var newPassword = "password99";
final var user = userDao.findById(connection, 1L);
user.changePassword(newPassword);
TransactionManagerConfig.executeInTransaction(conn -> {
final var newPassword = "password99";
final var user = userDao.findById(1L);
user.changePassword(newPassword);

userDao.update(connection, user);
userDao.update(user);

final var actual = userDao.findById(connection, 1L);
final var actual = userDao.findById(1L);

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

@Test
void delete() {
final var user = userDao.findById(connection, 1L);
userDao.delete(connection, user);
TransactionManagerConfig.executeInTransaction(conn -> {
final var user = userDao.findById(1L);
userDao.delete(user);

final var actual = userDao.findById(connection, 1L);
final var actual = userDao.findById(1L);

assertThat(actual).isNull();
assertThat(actual).isNull();
});
}
}
4 changes: 1 addition & 3 deletions app/src/test/java/com/techcourse/service/MockUserHistoryDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.techcourse.service;

import java.sql.Connection;

import com.interface21.dao.DataAccessException;
import com.interface21.jdbc.core.JdbcTemplate;
import com.techcourse.dao.UserHistoryDao;
Expand All @@ -14,7 +12,7 @@ public MockUserHistoryDao(final JdbcTemplate jdbcTemplate) {
}

@Override
public void log(Connection connection, final UserHistory userHistory) {
public void log(final UserHistory userHistory) {
throw new DataAccessException();
}
}
18 changes: 10 additions & 8 deletions app/src/test/java/com/techcourse/service/UserServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.sql.SQLException;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

Expand All @@ -22,19 +20,20 @@ class UserServiceTest {
private UserDao userDao;

@BeforeEach
void setUp() throws SQLException {
void setUp() {
this.jdbcTemplate = new JdbcTemplate(DataSourceConfig.getInstance());
this.userDao = new UserDao(jdbcTemplate);

DatabasePopulatorUtils.execute(DataSourceConfig.getInstance());
final var user = new User("gugu", "password", "hkkang@woowahan.com");
userDao.insert(DataSourceConfig.getInstance().getConnection(), user);
final var userService = new TxUserService(new AppUserService(userDao, new UserHistoryDao(jdbcTemplate)));
userService.insert(user);
}

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

final var newPassword = "qqqqq";
final var createBy = "gugu";
Expand All @@ -49,13 +48,16 @@ void testChangePassword() {
void testTransactionRollback() {
// 트랜잭션 롤백 테스트를 위해 mock으로 교체
final var userHistoryDao = new MockUserHistoryDao(jdbcTemplate);
final var userService = new UserService(userDao, userHistoryDao);
// 애플리케이션 서비스
final var appUserService = new AppUserService(userDao, userHistoryDao);
// 트랜잭션 서비스 추상화
final var userService = new TxUserService(appUserService);

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
Loading