[3단계 - Transaction 적용하기] 링크(손준형) 미션 제출합니다. #1158
Conversation
Walkthrough이 변경사항은 JDBC 트랜잭션 관리를 위한 인프라를 구축합니다. UserHistoryDao의 수동 JDBC 코드가 JdbcTemplate으로 대체되었고, UserService에 DataSource 의존성과 선택적 트랜잭션 처리가 추가되었습니다. JdbcTemplate의 연결 관리가 DataSourceUtils를 사용하도록 변경되었으며, DataSourceUtils는 바인딩된 연결을 구분하는 로직이 추가되었습니다. TransactionSynchronizationManager는 ThreadLocal 기반의 리소스 맵을 구현하여 스레드별 연결을 추적합니다. Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
34-35: 테스트에서 싱글턴 DataSource 사용으로 인한 상태 누수 가능성 점검 요청
DataSourceConfig.getInstance()는 메모리 H2를 VM 생애 동안 유지(DB_CLOSE_DELAY=-1)합니다. 병렬 실행/다른 테스트 클래스와의 상호작용 시 스키마/데이터가 엮일 수 있어요. 테스트 격리를 어떻게 보장하실 계획인가요? DatabasePopulatorUtils.execute가 매 테스트에 완전 초기화(drop+create+truncate)를 수행하는지 확인해보셨나요?
트랜잭션 롤백 테스트는 비밀번호만 검증합니다. user_history 레코드 미삽입까지 검증하면 의도(원자성)가 더 분명해집니다.
왜 중요한가: 테스트 격리는 신뢰도와 재현성을 좌우하며, 트랜잭션 테스트는 “모두 또는 전무”를 관찰가능하게 해야 합니다. (객체지향 생활 체조: 작은 단위, 단일 책임의 테스트 유지)
Also applies to: 49-50
app/src/main/java/com/techcourse/service/UserService.java (3)
41-67: changePassword의 들여쓰기(depth)와 책임 분리현재 try-catch 안에 또 다른 try-catch가 중첩되어 depth가 2단계입니다. 규칙 1(메서드 들여쓰기 1단계)을 위반할 소지가 있어 가독성과 테스트 용이성이 떨어집니다. 롤백 처리/커밋/리소스 정리를 별도 메서드(또는 템플릿)로 분리해 한 단계로 유지하는 방향을 고려해보셨나요? 또한 “트랜잭션 경계(시작/커밋/롤백/정리)”와 “도메인 작업(비번 변경/이력 기록)”의 책임을 명확히 나누면 응집도가 올라갑니다. 왜 중요한가: 복잡도 감소와 변경 용이성 향상.
48-66: 트랜잭션 경계와 인프라 결합도 축소 제안서비스가 DataSourceUtils와 TransactionSynchronizationManager에 직접 의존하며 경계를 제어합니다. 트랜잭션 템플릿(또는 전략)으로 경계를 감싸 “무엇을”과 “어떻게(인프라)”를 분리하면 테스트가 쉬워지고 재사용성이 높아집니다. “만약 다른 전파 규칙이나 중첩 트랜잭션이 생긴다면 변경은 어디에 집중되어야 할까요?”를 스스로 점검해보세요.
41-45: 이름 명확성: createBy → createdBy 검토도메인(UserHistory#getCreateBy)과 서비스 파라미터(createBy)의 철자가 일관되지 않습니다. 규칙 5(축약 금지, 의도 명확화)에 따라 “createdBy”로 통일하면 의도가 더 분명해집니다. SQL 컬럼(created_by)와도 대응이 쉬워집니다.
Also applies to: 69-74
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
42-46: 예외 원인(cause) 보존close 실패 시 CannotGetJdbcConnectionException을 던지지만 원인 SQLException을 함께 전달하지 않아 원인 분석이 어려워집니다. 메시지와 함께 cause를 연결해 추적 가능성을 높여보세요.
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (2)
10-18: JdbcTemplate 주입 방향성 일관화DataSource 기반 생성자와 JdbcTemplate 기반 생성자가 공존합니다. 생성자 오버로드 자체는 유연하지만, 한 코드베이스에서 “무엇을 주입할지”가 일관되지 않으면 조합이 늘어나 테스트/사용 패턴이 흔들릴 수 있어요. 팀 컨벤션을 정해 단일 형태로 수렴하는 것을 고민해보셨나요? (예: 항상 JdbcTemplate 주입)
23-31: 파라미터 의미 강화와 이름 일관성
- 6개의 원시값을 그대로 전달합니다. 규칙 3(원시값 포장)을 적용하면 UserHistory의 불변성/유효성 검증을 DAO 경계 안에서 더 단단히 유지할 수 있어요. “만약 컬럼이 늘거나 형식이 바뀐다면 어떤 객체 경계에서 변경을 흡수할 수 있을까요?”
- created_by와 메서드명 getCreateBy의 철자 불일치가 있습니다. 도메인과 SQL, 서비스 파라미터를 “createdBy”로 통일하는 방향을 검토해보세요.
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
49-51: 로그 레벨 검토SQL을 info 레벨로 출력하면 운영 로그가 과도해질 수 있습니다. 개발/테스트 환경은 debug, 운영은 마스킹/샘플링 등 정책을 분리하는 방식을 고려해보세요. 왜 중요한가: 관측성은 중요하지만 과도한 로그는 비용과 신뢰도를 해칩니다.
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
14-20: 메서드 로직은 명확하고 안전합니다.null 체크와 early return을 사용하여 깔끔하게 구현되었습니다.
다만, 매개변수 이름
key가 조금 추상적입니다. 객체지향 생활 체조 원칙 5번(축약 금지)의 연장선에서,dataSource와 같이 구체적인 이름을 사용하면 코드의 의도가 더 명확해질 수 있습니다. 이는 다른 메서드들의 매개변수에도 동일하게 적용됩니다.
key→dataSourcevalue→connection(bindResource에서)💡 힌트: 메서드 시그니처가 이미 타입 정보를 제공하지만, 매개변수 이름이 도메인 용어를 사용하면 가독성이 향상됩니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/com/techcourse/dao/UserHistoryDao.java(1 hunks)app/src/main/java/com/techcourse/service/UserService.java(2 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(2 hunks)jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java(2 hunks)jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java(1 hunks)jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 🎯 코드 품질 중심 리뷰 가이드라인이 리뷰는 코드 품질과 객체지향 원칙에 집중합니다.
미션 달성 여부가 아닌, 코드 설계와 품질 개선에 대한 피드백을 한글로 제공해주세요.📚 학습 원칙
- 직접 코드를 제공하지 마세요 (학습자가 명시적으로 요청하는 경우 제외)
- 문제 해결 과정을 안내하되, 정답을 바로 알려주지 마세요
- 작은 단계로 문제를 분해하여 접근하도록 도와주세요
💡 피드백 방법
- 유도 질문 활용: "만약 ~라면 어떻게 될까요?", "~를 고려해보셨나요?"
- 힌트 제공: 방향은 제시하되, 구체적인 구현은 학습자가 하도록
- 다양한 접근법 제시: 한 가지 해결책이 아닌 여러 가능성을 제안
- 왜?에 집중: 단순히 무엇이 잘못되었는지보다 왜 그런지 이해하도록
⚡ 객체지향 생활 체조 원칙 검토
다음은 객체지향 생활 체조(Object Calisthenics) 9가지 원칙입니다.
위반 시 학습 효과를 위해 반드시 피드백을 제공하되, 왜 이 원칙이 중요한지 설명해주세요:규칙 1: 한 메서드에 오직 한 단계의 들여쓰기만
- 들여쓰기 depth 최대 1 (중첩 제어구조 금지)
- 📖 이유: 메서드 복잡도 감소, 단일 책임 원칙 강화
- 💡 힌트: "이 부분을 별도 메서드로 추출하면 어떨까요?"
규칙 2: else 예약어 금지
- else, switch/case 사용 금지
- 📖 이유: 복잡한 분기 제거, 명확한 코드 흐름
- 💡 힌트: "early return이나 가드 클로즈 패턴을 고려해보세요"
규칙 3: 모든 원시값과 문자열 포장
- 원시 타입과 String을 객체로 포장
- 📖 이유: 도메인 개념 명확화, 비즈니스 로직 응집
- 💡 예시:
int port→Port port,String name→Name name규칙 4: 한 줄에 점 하나만 (디미터 법칙)
- 메서드 체이닝 제한
- 📖 이유: 결합도 감소, 캡슐화 향상
- 💡 나쁜 예:
request.getUri().getPath().substring()- 💡 좋은 예:
request.extractPath()규칙 5: 축약 금지
- 명확한 이름 사용 (축약어 금지)
- 📖 이유: 코드 가독성, 의도 명확화
- 💡 예시:
req→request,calcAmt→calculateAmount규칙 6: 모든 엔티티를 작게 유지
- 클래스 50줄, 메서드 10줄 이하
- 📖 이유: 단일 책임, 이해와 테스트 용이성
- 💡 힌트: "이 클래스가 너무 많은 일을 하고 있지 않나요?"
규칙 7: 인스턴스 변수 3개 이하
- 클래스당 최대 3개의 인스턴스 변수
- 📖 이유: 높은 응집도, 단일 책임 유지
- 💡 힌트: "관련 필드들을 별도 객체로 묶을 수 있을까요?"
규칙 8: 일급 컬렉션 사용
- 컬렉션을 감싸는 클래스 사용
- 📖 이유: 컬렉션 로직 캡슐화, 불변성 보장
- 💡 예시:
List<HttpHeader>→HttpHeaders클래스규칙 9: 게터/세터/프로퍼티 금지
- Tell, Don't Ask 원칙 준수
- 📖 이유: 진정한 캡슐화, 객체 자율성
- 💡 나쁜 예:
if (user.getPassword().equals(input))- 💡 좋은 예:
if (user.authenticate(input))🎯 추가 프로그래밍 요구사항
코드 스타일
- Java Style Guide 준수
- 3항 연산자 사용 금지
테스트 및 구조
- **모든 기능 T...
Files:
app/src/test/java/com/techcourse/service/UserServiceTest.javajdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.javaapp/src/main/java/com/techcourse/dao/UserHistoryDao.javajdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.javajdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.javaapp/src/main/java/com/techcourse/service/UserService.java
🧬 Code graph analysis (4)
app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
DataSourceUtils(11-48)
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(8-42)
app/src/main/java/com/techcourse/service/UserService.java (2)
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
DataSourceUtils(11-48)jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(8-42)
🔇 Additional comments (3)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
107-115: JDK 호환성 문제 없음 - Java 21 환경에서 List.getFirst() 사용 가능빌드 설정 검증 결과, 프로젝트의 모든 모듈(app, jdbc, mvc, study)이 Java 21로 설정되어 있습니다.
List.getFirst()는 Java 21에서 추가된 메서드이므로 현재 환경에서는 호환성 문제가 없습니다. 코드는 정상적으로 컴파일되고 실행됩니다.Likely an incorrect or invalid review comment.
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (2)
31-41: 훌륭한 메모리 관리입니다! 👍
unbindResource메서드가 ThreadLocal 메모리 누수를 방지하기 위해 올바르게 구현되었습니다:
- Line 38: map이 비어있을 때
resources.remove()를 호출하여 ThreadLocal을 정리합니다.- 이는 thread pool 환경에서 매우 중요한 패턴입니다.
📖 이유: ThreadLocal은 thread가 재사용될 때 이전 값이 남아있을 수 있어, 명시적으로 제거하지 않으면 메모리 누수와 예상치 못한 동작이 발생할 수 있습니다.
null 체크와 early return을 통한 방어적 프로그래밍도 잘 적용되었습니다.
22-29: 원래 우려사항 재검토: 중복 바인딩은 실제 문제가 아닙니다.코드를 분석한 결과,
DataSourceUtils.getConnection()의 캐시 체크 로직(16-17줄)이 중복 바인딩을 효과적으로 방지합니다:Connection connection = TransactionSynchronizationManager.getResource(dataSource); if (connection != null) { return connection; // 이미 바인딩된 연결은 캐시에서 반환 }같은
DataSource로getConnection()이 호출되면:
- 첫 번째 호출: 캐시가 비어있음 → 새 연결 획득 →
bindResource()호출(23줄)- 두 번째 호출: 캐시 확인 → 기존 연결 반환 →
bindResource()호출 안 함따라서 같은
DataSource에 대해bindResource()가 여러 번 호출될 수 없습니다.코드상 유일한 개선점:
Line 23의
map변수는 null 체크 후 재할당이 없으므로final로 선언할 수 있습니다 (다른 메서드와 일관성 유지):final Map<DataSource, Connection> map = resources.get();
| final Connection conn = getConnection(); | ||
| try (PreparedStatement pstmt = conn.prepareStatement(sql)) { | ||
| setPreparedStatementParameter(args, pstmt); | ||
| log.info("query = {}", sql); | ||
|
|
||
| return executor.execute(pstmt); | ||
| } catch (SQLException e) { | ||
| throw new DataAccessException("sql 실행 과정에서 문제가 발생하였습니다.", e); | ||
| } finally { | ||
| com.interface21.jdbc.datasource.DataSourceUtils.releaseConnection(conn, dataSource); | ||
| } |
There was a problem hiding this comment.
치명적: 비(非)트랜잭션 경로에서 커넥션 해제가 되지 않을 수 있음
execute()가 finally에서 releaseConnection을 호출하지만, DataSourceUtils가 “바인딩된 커넥션은 닫지 않음”으로 정의되어 있어 트랜잭션이 없는 호출에서도 커넥션이 닫히지 않을 수 있습니다. 현재 DataSourceUtils는 getConnection 시 무조건 바인딩하므로 둘의 결합으로 누수가 발생합니다.
해결 방향(설계 힌트):
- “트랜잭션 활성” 개념을 도입하여 JdbcTemplate의 일반 호출은 바인딩 없이 획득/즉시 해제되도록 하고, 서비스 경계에서만 바인딩을 사용,
- 또는 트랜잭션 템플릿을 통해 경계 내 execute 호출들이 바인딩된 커넥션을 재사용하도록 통제.
왜 중요한가: 커넥션 풀 고갈, 요청 지연/장애로 직결됩니다.
Also applies to: 143-145
🤖 Prompt for AI Agents
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java around lines
46-56 (also applies to 143-145): the current code always obtains a connection in
a way that DataSourceUtils treats as "bound", so finally-releaseConnection may
not close it on non-transactional paths, causing pool leaks. Fix by
distinguishing transactional vs non-transactional acquisition: when no active
transaction, obtain a non-bound connection (or mark it non-transactional) and
ensure it is closed in finally; when in a transaction, obtain/keep a bound
connection so it can be reused and released by transaction management.
Concretely, add a transactionActive check (or use
DataSourceUtils.isConnectionTransactional) before obtaining/deciding how to
release the connection, and ensure releaseConnection actually closes non-bound
connections (or call conn.close() for non-bound) while leaving bound connections
to transaction manager; alternatively move connection-binding logic into a
TransactionTemplate so plain execute() uses short-lived connections.
| if (connection == null) { | ||
| return; | ||
| } | ||
|
|
||
| // 트랜잭션 컨텍스트에서 관리되는 Connection인 경우 닫지 않음 | ||
| Connection boundConnection = TransactionSynchronizationManager.getResource(dataSource); | ||
| if (boundConnection == connection) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
치명적: 트랜잭션 외부에서 커넥션 누수 가능성
현재 동작 조합:
- getConnection: 바인딩이 없으면 새 커넥션을 획득 “하고” TransactionSynchronizationManager에 바인딩합니다.
- releaseConnection: 바인딩된 커넥션이면 닫지 않고 반환합니다.
트랜잭션이 없는 일반 JdbcTemplate 호출에서도 커넥션이 바인딩되고, JdbcTemplate의 finally에서 releaseConnection이 “바인딩됨”을 이유로 닫지 않아 스레드-로컬에 남을 수 있습니다. 서비스 계층이 unbind를 수행하지 않는 한 커넥션이 장시간 유지/고갈될 위험이 있습니다.
개선 방향(설계 힌트):
- “트랜잭션 활성” 신호가 있을 때만 바인딩하도록 분기(예: TSM에 active 플래그/스택 도입)하거나,
- 트랜잭션 경계에서만 명시적으로 bind/unbind하고, 일반 getConnection은 바인딩하지 않도록 역할을 분리,
- release 시 “트랜잭션 활성”을 고려하여 닫을지/유지할지를 결정.
왜 중요한가: 리소스 누수는 장애로 직결되며, 테스트 환경에서는 드러나지 않고 운영에서만 표면화됩니다.
1 participant
안녕하세요 칼리~~!
테코톡 때문에 바쁘실텐데 끝나고 천천히 리뷰해주세요~ 😀