[4단계 - Transaction synchronization 적용하기] 벨로(방예혁) 미션 제출합니다. #1212
Conversation
WalkthroughUserService가 구체 클래스에서 인터페이스로 변경되고, 비즈니스 로직을 수행하는 AppUserService와 트랜잭션 관리를 담당하는 TxUserService가 추가되었습니다. UserDao, UserHistoryDao, MockUserHistoryDao에서 Connection을 인자로 받는 오버로드 메서드가 제거되어 내부 JdbcTemplate/DataSourceUtils로 연결을 관리하도록 변경되었습니다. JdbcTemplate과 DataSourceUtils는 커넥션 획득·해제 및 예외 처리 로직을 개선했고, TransactionSynchronizationManager는 제네릭 리소스 맵 API를 도입했습니다. 관련 테스트들이 새로운 구조와 트랜잭션 흐름에 맞게 수정 및 확장되었습니다. 개요변경 사항은 서비스 계층을 인터페이스 기반으로 재구성하고 책임을 분리합니다: AppUserService가 도메인 로직을 담당하고 TxUserService가 트랜잭션 경계 관리를 담당합니다. DAO층에서는 외부 Connection 파라미터를 제거하여 JdbcTemplate/DataSourceUtils를 통한 중앙 집중식 연결 관리를 적용했습니다. JdbcTemplate은 쿼리·업데이트 수행 시 DataSourceUtils를 사용하도록 리팩터링되고 오류 메시지 및 리소스 처리가 개선되었으며, TransactionSynchronizationManager는 리소스 바인딩 API를 일반화했습니다. 테스트는 이 구조에 맞춰 수정되었습니다. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (15)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (2)
9-20: Object 기반 키/값 사용으로 타입 안정성 저하 및 키 충돌 가능성Map<Object, Object>로 일반화되며 컴파일 단계의 보호가 사라집니다. 동일 equals/hashCode를 갖는 키 충돌 가능성, 값 캐스팅 실수 위험이 커집니다. 전용 키 타입(예: DataSourceKey) 또는 ResourceHolder(예: ConnectionHolder)로 캡슐화하는 방향을 고려해보실까요? 이렇게 하면 응집도·가독성·검증 용이성이 개선됩니다.
Also applies to: 22-45
14-20: null 반환 정책 재검토getResource/unbindResource가 null을 반환하면 호출측 실수를 늦게 드러내거나 NPE를 유발할 수 있습니다. Optional 반환, 명시적 예외, 혹은 존재 여부를 먼저 질의하는 API로 의도를 드러내는 방식을 검토해보세요.
Also applies to: 35-45
jdbc/src/test/java/com/interface21/jdbc/core/JdbcTemplateTest.java (1)
210-214: SQL 포함 메시지 검증 적절hasMessageContaining으로 SQL 포함 여부를 점검하는 접근이 합리적입니다. 필요하다면 시작·핵심 구문만 체크해 메시지 변경에 덜 민감하게 유지하는 것도 고려해보세요.
app/src/main/java/com/techcourse/service/AppUserService.java (1)
32-36: 원시값 포장 규칙 검토id, newPassword, createdBy는 도메인 의미가 강합니다. UserId, Password, CreatedBy 같은 값 객체로 포장하면 유효성·불변성·의도를 강화할 수 있습니다. 작은 단계로, 우선 Password만 포장해보는 접근을 추천드립니다. 왜냐하면 비밀번호 규칙의 응집도가 가장 높기 때문입니다.
app/src/test/java/com/techcourse/service/UserServiceTest.java (2)
56-56: 네이밍 일관성createdBy로의 정정은 명확합니다. 첫 번째 테스트의 지역 변수(createBy)도 동일 컨벤션으로 맞추면 가독성이 좋아집니다.
34-36: DataSource 주입 방식에 대한 테스트 안정성DataSourceConfig.getInstance() 싱글턴을 직접 주입하면 테스트 간 상태 공유 가능성이 있습니다. 테스트 격리를 위해 DataSource를 팩토리/빌더로 생성해 주입하거나, DB 초기화 책임을 명확히 분리하는 방법을 고려해보셨나요?
Also applies to: 51-54
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (1)
14-17: 롤백 유도용 목 객체 설계가 명확합니다Connection 인자를 제거한 시그니처와 테스트 의도가 일치합니다. 다만, 프로덕션 DAO를 상속하는 대신 인터페이스/테스트 더블을 사용하는 방식도 결합도를 낮출 수 있습니다.
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
41-42: Throwable 포착 범위 축소 고려자원 해제에서 Throwable 전체를 포착하면 OOME 등 복구 불가 오류도 삼켜집니다. 필요 최소 범위(Exception 등)로 좁히고, 상위로 전파가 필요한 치명 오류는 그대로 두는 방식을 검토해보세요.
app/src/main/java/com/techcourse/service/UserService.java (1)
11-15: 원시값 포장·Tell, Don’t Ask 적용 여지id, newPassword, createdBy를 값 객체로 포장하면 검증과 불변성을 계약에 녹일 수 있습니다. 또한 changePassword가 “행위” 중심의 이름/모델이므로, 호출자가 데이터를 꺼내 확인하기보다 객체에 일을 시키는 흐름을 테스트에서도 유지하는 방안을 고민해보세요.
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (3)
91-99: 중첩 try로 인한 들여쓰기 depth 개선 여지객체지향 생활 체조 규칙 1(들여쓰기 한 단계)을 고려하면, 내부 자원 처리(PS/RS) 블록을 별도 메서드로 추출하거나 템플릿 메서드로 단순화해 depth 1을 유지하는 구조를 검토해보세요. 유지보수성과 테스트 용이성이 좋아집니다.
Also applies to: 114-117
145-147: 한 줄에 점 하나(디미터 법칙) 위반 가능sql.chars().filter().count()는 체이닝이 이어집니다. 플레이스홀더 카운트 계산을 전용 유틸/값 객체로 위임하거나, 표현을 단순화해 규칙 준수를 시도해보세요.
50-52: 예외 메시지 정책 정합성조회/수정 실패 시 메시지의 SQL 포함 여부가 경로마다 다릅니다(로그에는 포함, 예외 메시지는 일부 경로만 포함). 운영 관점에서 “로그만으로 충분한가, 예외 메시지에도 최소 컨텍스트를 담을 것인가” 정책을 정해 일관화해보세요.
Also applies to: 101-103, 119-121
app/src/main/java/com/techcourse/service/TxUserService.java (3)
37-69: 메서드 길이/들여쓰기 규칙 위반 — 작은 단계로 쪼개기
- changePassword가 10줄을 넘어가고(약 30줄) try/catch 안에 중첩 분기가 포함되어 “들여쓰기 depth=1” 원칙을 위반합니다.
- 읽기/변경/복구/정리의 서로 다른 책임이 한 메서드에 섞여 있습니다.
개선 힌트:
- “시작(begin) / 비즈니스 수행 / 커밋 / 롤백 / 정리(cleanup)”를 각기 독립된 작은 메서드로 추출해 depth를 1단계로 유지.
- 템플릿/전략 패턴(예: TransactionTemplate 스타일)을 도입해 트랜잭션 경계 보일러플레이트를 제거하고 비즈니스 콜백만 전달.
왜 중요한가:
- 단일 책임과 가독성 향상, 테스트 용이성(개별 단계 단위 테스트)이 커집니다. 코딩 가이드라인에 따라.
38-42: 원시값 포장(Primitive Obsession) 완화
- id(long), newPassword(String), createdBy(String)가 도메인 의미를 담는 값 객체로 포장되지 않았습니다.
힌트:- UserId, Password, Operator(또는 CreatedBy) 등 값 객체로 책임을 옮기면 유효성(빈 문자열/형식/길이/비밀번호 정책)과 불변성 보장이 쉬워집니다.
- 서비스 인터페이스 수준에서 타입으로 의도를 드러내면 오용을 줄이고 테스트 격리도 좋아집니다. 코딩 가이드라인에 따라.
49-56: 관측 가능성(Observability) 보강 제안
- 경계 로그가 롤백 실패만 다룹니다. 커밋/롤백 시점, 트랜잭션 식별(스레드/트레이스 아이디), 소요 시간 등을 디버그 레벨로 남기면 장애 분석과 회귀 조사에 유용합니다.
힌트:- 시작/커밋/롤백/정리 구간에 경량 로그 포인트를 배치하거나, 템플릿 적용 시 공통 로그를 중앙집중화하세요. 코딩 가이드라인에 따라.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/src/main/java/com/techcourse/dao/UserDao.java(0 hunks)app/src/main/java/com/techcourse/dao/UserHistoryDao.java(0 hunks)app/src/main/java/com/techcourse/service/AppUserService.java(1 hunks)app/src/main/java/com/techcourse/service/TxUserService.java(1 hunks)app/src/main/java/com/techcourse/service/UserService.java(1 hunks)app/src/test/java/com/techcourse/service/MockUserHistoryDao.java(1 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(2 hunks)jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java(4 hunks)jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java(1 hunks)jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java(1 hunks)jdbc/src/test/java/com/interface21/jdbc/core/JdbcTemplateTest.java(2 hunks)
💤 Files with no reviewable changes (2)
- app/src/main/java/com/techcourse/dao/UserDao.java
- app/src/main/java/com/techcourse/dao/UserHistoryDao.java
🧰 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/MockUserHistoryDao.javaapp/src/main/java/com/techcourse/service/TxUserService.javajdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.javaapp/src/test/java/com/techcourse/service/UserServiceTest.javaapp/src/main/java/com/techcourse/service/AppUserService.javaapp/src/main/java/com/techcourse/service/UserService.javajdbc/src/test/java/com/interface21/jdbc/core/JdbcTemplateTest.javajdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.javajdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java
🧬 Code graph analysis (6)
app/src/main/java/com/techcourse/service/TxUserService.java (1)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(7-46)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
DataSourceUtils(11-46)
app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
app/src/main/java/com/techcourse/service/AppUserService.java (2)
app/src/main/java/com/techcourse/dao/UserDao.java (1)
UserDao(9-82)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(8-33)
app/src/main/java/com/techcourse/service/UserService.java (1)
study/src/test/java/aop/stage1/UserService.java (1)
UserService(9-36)
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(7-46)
🔇 Additional comments (13)
jdbc/src/test/java/com/interface21/jdbc/core/JdbcTemplateTest.java (1)
189-193: 변경된 예외 메시지 반영 확인 완료“조회 결과가 존재하지 않습니다.”로의 변경 반영이 테스트에 잘 매핑되었습니다.
app/src/main/java/com/techcourse/service/AppUserService.java (2)
13-19: 애플리케이션 서비스와 트랜잭션 서비스의 책임 분리가 명확합니다도메인 변경(비밀번호)과 이력 로깅을 한 곳에서 조정하면서, 트랜잭션 경계는 별도 TxUserService로 위임한 구조가 좋습니다.
Also applies to: 31-41
37-41: 트랜잭션 일관성 전제 확인findById → update → history.log가 하나의 트랜잭션/커넥션에서 실행되어야 합니다. TxUserService가 시작 시 커넥션을 바인딩하고, 메서드 체인 내내 동일 커넥션이 사용되는지(조회가 트랜잭션 외부 커넥션으로 빠지지 않는지) 점검해보셨나요? 실패 시 반드시 finally에서 롤백·unbind가 호출되는지도 함께 확인해주세요.
app/src/test/java/com/techcourse/service/UserServiceTest.java (2)
34-36: 테스트에서 서비스 계층 합성이 명료합니다AppUserService를 TxUserService로 감싸 실제 운용 형태를 재현한 점이 좋습니다. 트랜잭션 경계 검증에 효과적입니다.
51-54: 트랜잭션 래퍼 구성 방식 적절롤백 시나리오에서도 동일한 합성 패턴을 재사용해 일관성이 좋습니다.
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (2)
18-27: 트랜잭션 바운드 커넥션 재사용 처리 적절이미 바인딩된 커넥션을 우선 반환하는 로직이 JdbcTemplate와의 연동에 잘 부합합니다. 에러 메시지도 맥락이 분명합니다.
30-44: 호출 규약 강화: DataSourceUtils.releaseConnection()에서 연결 객체 동일성 검증 필요Spring의 releaseConnection 구현은 connectionEquals(conHolder, con)로 바운드된 리소스와 인자로 받은 연결이 동일한지 검증하지만, 현재 코드는 단순히 바운드 리소스의 존재 여부만 확인합니다. dataSource.getConnection()은 항상 새로운 연결을 반환하지만 DataSourceUtils.getConnection()은 활성 트랜잭션이 있으면 그 트랜잭션의 연결을 반환합니다.
현재 로직이라면, 같은 스레드에서
DataSourceUtils.getConnection()으로 바인딩된 연결이 존재할 때, 우회하여dataSource.getConnection()으로 획득한 다른 연결의releaseConnection()호출 시 바인딩 리소스 존재 여부만으로 닫기를 건너뜁니다. 이로 인해 별도 연결이 누수될 가능성이 있습니다.인자로 받은
con이 실제로 바운드된 리소스와 동일한 객체인지 비교하는 방식 도입을 검토하세요.app/src/main/java/com/techcourse/service/UserService.java (1)
5-16: 인터페이스 추출로 계약이 명확해졌습니다구현 교체·데코레이터 적용(TxUserService)에 유리한 설계입니다.
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (2)
91-106: 커넥션 획득/해제 일관화가 잘 적용되었습니다DataSourceUtils 연동과 try-with-resources 적용으로 리소스 관리가 명확해졌습니다. 예외 전환도 일관적입니다.
Also applies to: 112-124
145-151: 파라미터 카운트 검증의 엣지 케이스 확인문자열 리터럴 내 '?'(예: "WHERE note = '?'")도 현재 로직에 포함됩니다. 실제 입력 SQL에 이런 케이스가 있을 수 있는지, 있다면 예외 케이스 테스트를 추가하고 전략(리터럴 무시 등)을 정교화하는 것을 권장합니다.
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
41-43: 스레드 로컬 정리 및 트랜잭션 경계 검증 완료 - 문제 없음검증 결과, 현재 구현은 스레드 로컬 리소스 정리와 트랜잭션 경계 처리가 올바르게 되어 있습니다:
- unbindResource 호출이 finally 블록에 위치: 모든 경로(정상/예외)에서 항상 실행됨 (TxUserService 67번 라인)
- map.isEmpty() 후 resources.remove(): ThreadLocal 메모리 누수 방지를 위한 올바른 패턴 (TransactionSynchronizationManager 41-43줄)
- 트랜잭션 경계 보호: commit/rollback이 try-catch로 처리되고, connection.close()도 finally에서 실행됨
간헐적 테스트 실패나 메모리 누수 우려는 해결되었습니다.
app/src/main/java/com/techcourse/service/TxUserService.java (2)
12-25: 레이어드 설계 선택은 적절합니다.트랜잭션 경계를 TxUserService로 올리고 AppUserService는 비즈니스 로직에 집중하도록 분리한 방향이 명확합니다. 유지보수성과 테스트 용이성이 좋아집니다. 다만, 이 래퍼가 “트랜잭션 전파 규칙”을 어디까지 책임질지(예: 이미 시작된 트랜잭션에 참여 vs 항상 신규 시작)를 팀 차원에서 합의하면 이후 확장이 수월해집니다. 코딩 가이드라인에 따라.
현재 코드베이스에서 TxUserService.changePassword가 다른 트랜잭션 경계 내부에서 호출될 가능성이 있는지 확인 부탁드립니다.
27-35: 읽기/쓰기 경계의 일관성 — findById/insert의 트랜잭션 처리 수준 확인
- 현재 findById/insert는 단순 위임으로 비트랜잭션입니다. 의도적으로 “쓰기(changePassword)만 트랜잭션”인지, 혹은 읽기도 “읽기 전용 트랜잭션”으로 묶을지 정책 결정을 명확히 해두면 좋습니다.
유도 질문:
- 도메인 규칙상 비밀번호 변경 직전에 수행되는 조회가 “동일 스냅샷 보장”을 요구하나요?
- 읽기 전용 트랜잭션(오토커밋 ON vs OFF, 격리수준)으로 일관성을 강화할 필요가 있나요? 코딩 가이드라인에 따라.
- 기존 트랜잭션이 있는 경우 참여하고, 없을 때만 새로 시작하도록 구현합니다.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/src/main/java/com/techcourse/service/TxUserService.java (1)
43-46: 중첩 트랜잭션 참여 가드가 문제를 잘 해소합니다이미 바인딩된 경우 즉시 위임·복귀하는 전략으로 외부 트랜잭션을 덮어쓰는 위험을 제거했습니다. 이전 코멘트의 핵심 우려가 해소되었습니다. 실패 경로(내부에서 예외 발생 시 외부가 롤백)까지 검증 테스트를 하나 추가해 전파 규칙을 명문화해보세요.
🧹 Nitpick comments (6)
app/src/test/java/com/techcourse/service/UserServiceTest.java (2)
41-41: 이름 일관성 유지변수명 createBy ↔ createdBy가 혼재합니다. 규칙 5(축약 금지) 관점에서 도메인 용어(createdBy)로 통일하면 의도가 선명해집니다. 코딩 가이드라인 기준.
70-114: 중첩 트랜잭션 테스트 강화와 구조 단순화 제안
- 테스트 의도는 잘 드러납니다. 다만 중첩 시 “참여(외부가 커밋/롤백/종료 담당)” 규칙을 더 확실히 검증해보면 어떨까요?
- finally 이후에 TransactionSynchronizationManager.hasResource(dataSource)가 false임을 확인해 “누수 없음”을 단언.
- 내부 로직에서 예외가 나면 외부가 롤백을 수행하는 시나리오(실패 경로)도 한 케이스 추가.
- 규칙 1(들여쓰기 depth 1) 관점에서 try/catch 안의 중첩 if들을 헬퍼 추출로 평탄화하면 가독성이 좋아집니다. 테스트도 생활 체조 원칙을 적용하면 의도가 더 분리됩니다. 코딩 가이드라인 기준.
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (3)
9-20: 타입 안전성과 계약 명시 부족Map<Object,Object>로 바뀌면서 호출 측 캐스팅 실수 시 런타임 오류 위험이 큽니다. 또한 key에 대한 null 검증이 없습니다. 다음을 고려해보세요:
- getResource에 타입 파라미터(Class)를 받는 오버로드를 제공해 캐스팅 안정성 확보.
- bind/get/unbind 모두 key의 null 금지 계약을 명시하고 검증.
왜 중요한가: 트랜잭션 리소스는 인프라의 근간이라 “빨리 실패하고, 명확히 실패”해야 디버깅 비용이 낮습니다.
27-38: 중복 바인딩 정책 부재이미 동일 key가 바인딩된 상태에서의 동작이 “덮어쓰기”로 묵시 처리됩니다. 전파 규칙을 보장하려면 정책을 명확히 해야 합니다.
- 허용하지 않음: 이미 존재하면 예외.
- 참여: 이미 존재하면 set/bind 생략(참여)하고 수명 관리(커밋/종료)는 외부에 위임.
정책을 정하고 Javadoc/테스트로 고정하면, 서비스/DAO가 일관되게 동작합니다.
40-50: unbind 계약과 누수 방지 검증키 미존재 시 null 반환은 호출 측 실수를 숨길 수 있습니다. 선택지:
- 관대한 해제(idempotent no-op)를 유지하되, 디버그 로그로 진단성 확보.
- 또는 계약 위반으로 간주하고 예외.
아울러 ThreadLocal 누수는 스레드 풀에서 치명적입니다. 모든 진입점에서 “finally에서 반드시 unbind”가 지켜지는지 테스트 한 케이스(성공/실패 경로)를 추가해 주세요.app/src/main/java/com/techcourse/service/TxUserService.java (1)
47-74: 경계 분리·일관성·가독성 개선 제안
- 일관성: DAO/JdbcTemplate가 사용하는 커넥션 획득/반납 방식과 이곳의 방식이 같아야 합니다. 헬퍼(DataSourceUtils 계열)가 있다면 동일 경로로 통일해 “누가 만들었는지에 따라 누가 닫는지”를 명확히 해보세요.
- 가독성/생활 체조: changePassword가 예외 처리·바인딩·커밋/롤백·정리까지 모두 담당해 길고 들여쓰기가 깊습니다. “참여 여부 판단 → 경계 시작 → 작업 실행 → 경계 종료”를 작은 메서드로 나누면 규칙 1, 6을 만족하고 테스트도 수월해집니다.
- 도메인 원시값 포장: newPassword, createdBy는 의미 있는 값 객체(Password, CreatedBy)로 포장 시 유효성·의도 표현이 좋아집니다(규칙 3).
- 진단성: 예외 메시지에 id 등 컨텍스트를 포함하면 운영 추적이 쉬워집니다.
코딩 가이드라인 기준.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/techcourse/service/TxUserService.java(1 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(3 hunks)jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java(1 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/transaction/support/TransactionSynchronizationManager.javaapp/src/main/java/com/techcourse/service/TxUserService.java
🧬 Code graph analysis (2)
app/src/test/java/com/techcourse/service/UserServiceTest.java (3)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(7-51)app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(8-33)
app/src/main/java/com/techcourse/service/TxUserService.java (1)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(7-51)
🔇 Additional comments (1)
app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
37-39: 계층 분리 적용이 명확합니다AppUserService와 TxUserService 조합으로 트랜잭션 경계와 비즈니스 로직을 분리한 점 좋아요. DAO가 동일 커넥션을 재사용하는지(트랜잭션 참여)까지 테스트로 간접 검증된 점도 의미 있습니다. 추가로 “서비스 계층이 외부에서 이미 바인딩된 리소스를 덮어쓰지 않는다”는 전제를 테스트 설명에 명시해두면 의도가 더 분명해집니다.
There was a problem hiding this comment.
벨로, 1~4단계까지 수고 많으셨습니다.
서비스 인터페이스 추상화는 정말 좋을 것 같아요 👍
피드백 반영하신 부분과 PR 잘 확인했습니다.
코드에도 문제없고, 테스트 코드도 잘 동작하는 것 같아요.
또한 요구사항도 잘 지켜주신 것 같습니다.
이번에는 프로젝트 등 여러 가지 일로 인해 미션에 많이 집중하지 못한 것 같아 죄송스럽습니다.
벨로의 코드를 보면서 예상한 부분에 딱 멋진 코드들이 있어서 재밌게 봤던 것 같아요.
조금 더 앞선 미션에서 만났다면, 더 재밌게 서로 리뷰를 주고받을 수 있었을 텐데 하는 아쉬움이 남습니다.
남은 5레벨도 화이팅하세요!!
응원합니다 💪
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/test/java/com/techcourse/service/UserServiceTest.java (2)
36-38: 서비스 생성 로직의 중복을 개선해볼 수 있을까요?세 개의 테스트 메서드에서 동일한 패턴으로
UserHistoryDao,AppUserService,TxUserService를 생성하고 있습니다. 이런 중복은 유지보수성을 떨어뜨립니다.몇 가지 질문을 드려볼게요:
- 만약 서비스 생성 방식이 변경된다면, 몇 군데를 수정해야 할까요?
- 테스트마다 다른 점은 무엇인가요? (예:
MockUserHistoryDaovs 실제UserHistoryDao)- 공통 부분과 차이점을 어떻게 분리할 수 있을까요?
💡 힌트:
@BeforeEach나 팩토리 메서드 패턴을 고려해보세요. 다만 각 테스트에서 필요한 차이점(Mock vs Real)은 어떻게 처리할지 고민이 필요합니다.Also applies to: 52-56, 71-74
41-42: 변수명 일관성을 확인해주세요59번 라인에서는
createdBy로 수정되었는데, 41번 라인에서는 여전히createBy를 사용하고 있습니다.두 변수가 같은 의미를 가지고 있다면, 일관된 이름을 사용하는 것이 코드 가독성에 도움이 됩니다. 특히 **규칙 5 (축약 금지)**를 고려하면,
createdBy가 "누구에 의해 생성되었는지"를 더 명확하게 표현합니다.💡 모든 테스트에서 동일한 변수명을 사용하도록 통일해보세요.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/test/java/com/techcourse/service/UserServiceTest.java(3 hunks)jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java
🧰 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.java
🧬 Code graph analysis (1)
app/src/test/java/com/techcourse/service/UserServiceTest.java (4)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(7-51)app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)app/src/main/java/com/techcourse/dao/UserDao.java (1)
UserDao(9-82)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(8-33)
| @Test | ||
| void testNestedTransaction() throws SQLException { | ||
| final var userHistoryDao = new UserHistoryDao(jdbcTemplate); | ||
| final var appUserService = new AppUserService(userDao, userHistoryDao); | ||
| final var dataSource = DataSourceConfig.getInstance(); | ||
| final var userService = new TxUserService(appUserService, dataSource); | ||
|
|
||
| Connection connection = null; | ||
| try { | ||
| connection = dataSource.getConnection(); | ||
| connection.setAutoCommit(false); | ||
| TransactionSynchronizationManager.bindResource(dataSource, connection); | ||
|
|
||
| final var newPassword = "nestedPassword"; | ||
| final var createdBy = "gugu"; | ||
| userService.changePassword(1L, newPassword, createdBy); | ||
|
|
||
| assertThat(connection.isClosed()).isFalse(); | ||
| assertThat(TransactionSynchronizationManager.hasResource(dataSource)).isTrue(); | ||
|
|
||
| connection.commit(); | ||
| } catch (final Exception e) { | ||
| if (connection != null) { | ||
| try { | ||
| connection.rollback(); | ||
| } catch (final SQLException ex) { | ||
| // ignore | ||
| } | ||
| } | ||
| throw new RuntimeException(e); | ||
| } finally { | ||
| if (TransactionSynchronizationManager.hasResource(dataSource)) { | ||
| TransactionSynchronizationManager.unbindResource(dataSource); | ||
| } | ||
| if (connection != null) { | ||
| try { | ||
| connection.close(); | ||
| } catch (final SQLException e) { | ||
| // ignore | ||
| } | ||
| } | ||
| } | ||
|
|
||
| final var actual = userService.findById(1L); | ||
| assertThat(actual.getPassword()).isEqualTo("nestedPassword"); | ||
| } |
There was a problem hiding this comment.
이 테스트가 검증하려는 핵심 동작이 무엇인지 명확하지 않습니다
testNestedTransaction 메서드를 보면서 몇 가지 고민해볼 점들이 있습니다:
🎯 테스트의 목적에 대해:
-
이 테스트는 무엇을 검증하려는 건가요?
- "중첩 트랜잭션"이라는 이름이 붙었는데, 실제로 중첩된 트랜잭션이 있나요?
- 외부에서 수동으로 트랜잭션을 시작하고, 내부에서
TxUserService가 또 트랜잭션을 시작하는 상황을 테스트하는 건가요?
-
만약 그렇다면, 예상 동작은 무엇인가요?
- 내부 트랜잭션이 외부 트랜잭션에 참여해야 하나요?
- 아니면 별도의 트랜잭션으로 동작해야 하나요?
📏 객체지향 생활 체조 원칙 관점에서:
- 규칙 1 위반: 메서드에 여러 단계의 들여쓰기가 있습니다 (try-catch-finally 중첩)
- 규칙 6 위반: 메서드가 45줄이 넘어 10줄 제한을 크게 초과합니다
45줄이 넘는 메서드는 너무 많은 일을 하고 있다는 신호입니다. 이 메서드를 더 작은 단위로 나눌 수 있을까요?
🔧 설계 관점에서:
3. 테스트가 구현 세부사항에 너무 의존적입니다:
connection.isClosed(),hasResource()같은 인프라 수준의 검증이 필요한가요?TxUserService를 사용하는 클라이언트 코드가 직접Connection을 관리하고TransactionSynchronizationManager를 다뤄야 하나요?
- 만약 외부 트랜잭션 내에서
TxUserService를 호출하는 상황을 테스트하려는 거라면:- 이런 시나리오가 실제 프로덕션 코드에서 발생하나요?
- 있다면, 어떤 레이어에서 이런 일이 일어나나요?
💡 힌트: 테스트는 "어떻게(How)"가 아닌 "무엇을(What)" 검증해야 합니다. 현재 테스트는 트랜잭션 인프라가 어떻게 동작하는지를 검증하고 있는데, 정작 서비스의 비즈니스 동작은 마지막 두 줄(112-113)에서만 확인하고 있네요.
이 테스트를 통해 검증하고 싶은 실제 비즈니스 시나리오를 먼저 명확히 하면, 더 간단하고 의미 있는 테스트를 작성할 수 있을 것 같습니다.
4 participants
안녕하세요 새로이~
벌써 마지막 단계네요.
이번에도 잘 부탁드려요! 😄
변경 커밋
변경 사항
비즈니스 로직과 트랜잭션 처리 로직을 분리했습니다.
TransactionSynchronizationManager를 구현하여 트랜잭션 동기화 메커니즘을 적용하고,서비스 계층을 인터페이스 기반으로 추상화해 관심사를 분리했습니다.