[4단계 - Transaction synchronization 적용하기] 머랭 미션 제출합니다. #1237
Conversation
개요이 변경사항은 데이터 접근 계층과 서비스 계층의 트랜잭션 관리 방식을 재구성합니다. UserService는 구체적인 클래스에서 인터페이스로 전환되었으며, ApplicationUserService와 TransactionalUserService라는 두 가지 구현체가 추가되었습니다. UserDao와 UserHistoryDao에서 트랜잭션 관련 메서드가 제거되고 PreparedStatementSpecification 빌더 패턴으로 통합되었습니다. JdbcTemplate의 트랜잭션 메서드들도 제거되었으며, DataSourceUtils를 통한 리소스 관리로 변경되었습니다. 또한 PreparedStatementSpecification은 레코드로 변환되고 예외 처리 방식이 SQLSyntaxErrorException에서 IllegalArgumentException으로 변경되었습니다. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
28-43: 심각: execute 메서드에서 Connection 자원 누수 및 ThreadLocal 오염현재 코드 분석 결과, 문제가 확인되었습니다.
DataSourceUtils.getConnection()은 새로운 Connection을 획득할 때마다TransactionSynchronizationManager.bindResource(dataSource, connection)로 ThreadLocal에 바인딩합니다. 그러나execute메서드는 try-with-resources로 PreparedStatement만 관리하고, Connection에 대해서는:
releaseConnection호출 없음TransactionSynchronizationManager.unbindResource처리 없음결과:
비트랜잭션 경로(TransactionalUserService 거치지 않는 호출)에서execute종료 후에도 Connection은 닫히지 않고, ThreadLocal에 바인딩된 상태가 유지됩니다. 스레드 풀 재사용 환경에서는 이전 요청의 커넥션이 여전히 바인딩되어 풀 고갈 및 데이터 오염으로 이어집니다.개선 방향:
execute메서드 내 finally 블록(또는 현재 try-with-resources 이후)에서DataSourceUtils.releaseConnection호출과 함께TransactionSynchronizationManager.unbindResource(dataSource)처리를 추가하세요. 다만 트랜잭션 컨텍스트 존재 여부를 판단하여, 트랜잭션이 활성화된 경우 해제를 스킵하는 로직도 함께 고려해야 합니다.
🧹 Nitpick comments (19)
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (1)
15-17: 테스트 더블 설계는 방향 좋습니다만, 실패 정보 표현을 더 분명히 해보면 어떨까요?
- 변경된 시그니처(log(UserHistory))와 일치해 깔끔합니다.
- 단, 예외를 비워 던지면(메시지/원인 없음) 실패 원인 추적이 어렵습니다. 의도된 실패라는 힌트를 메시지로 남기거나, 목킹 프레임워크로 행위 기반 스텁을 고려해보세요.
As per coding guidelines
app/src/test/java/com/techcourse/dao/UserDaoTest.java (1)
54-56: 테스트에서 콘솔 출력은 지양해 주세요.
- 테스트는 “검증”에 집중하고, 출력은 노이즈가 됩니다. 필요한 경우 로깅 캡처/어설션으로 의도를 표현하거나 불필요하면 제거를 권합니다.
As per coding guidelines
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (2)
25-37: 파라미터 인덱스의 수동 관리 대신 ‘순서 보장’ 추상화를 고려해보셨나요?
- 인덱스를 직접 숫자로 부여하면 컬럼 추가/순서 변경 시 유지보수 비용과 오류 위험이 큽니다.
- 빌더가 내부적으로 순차 인덱스를 관리하거나, 컬럼-값 매핑 객체를 도입해 “순서/일치”를 캡슐화하는 접근을 검토해보세요.
- 왜? placeholder 수만 맞고 인덱스가 어긋나면 런타임에 미묘한 데이터 오염이 생길 수 있습니다.
As per coding guidelines
23-40: DAO에서 값 ‘질문’ 대신 ‘요청(Tell, Don’t Ask)’으로 전환을 검토해보세요.
- 여러 getter로 상태를 꺼내는 대신, UserHistory가 “자신을 영속화에 적합한 형태로 변환”하도록 메시지를 제공하면 응집도가 높아집니다.
- 힌트: UserHistory가 “삽입 명세에 자신을 적용”하는 역할(예: 매퍼/변환 메서드)을 갖도록 하면 DAO는 더 단순한 조립자로 남습니다.
As per coding guidelines
jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementFactory.java (1)
27-35: 검증 로직을 ‘개수 비교’에서 한 단계 강화해보세요.
- 현재는 '?' 총합과 파라미터 “개수”만 비교합니다.
- 에지케이스(문자열 리터럴 내 '?', 주석 등)나 인덱스 중복/비연속(1,3,3…)은 잡지 못합니다.
- 질문: 인덱스의 유일·연속(1..N) 검증, 혹은 “순차 바인딩” 모델로의 이행을 고려해보셨나요?
As per coding guidelines
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (2)
34-37: 스레드 로컬 청소: 빈 맵이면 remove까지 해보는 건 어떨까요?
- map.remove(key)만으로는 빈 맵이 남아 스레드 재사용(풀 환경) 시 불필요한 객체가 유지될 수 있습니다.
- 리소스가 0개가 되면 ThreadLocal 자체를 비우는 전략을 고민해보세요.
As per coding guidelines
16-20: null 반환 대신 Optional을 고려할 여지가 있습니다.
- getResource가 null을 리턴하면 호출부에서 널 가드가 반복될 수 있습니다. Optional 반환으로 의도를 더 명확히 할 수 있을까요?
As per coding guidelines
app/src/main/java/com/techcourse/service/UserService.java (1)
5-11: 원시값 포장과 도메인 용어 명세를 고려해 인터페이스를 다듬어보면 좋겠습니다.
- id, newPassword, createdBy가 원시/String으로 노출됩니다. 값 객체(예: UserId, Password, CreatedBy)로 포장하면 유효성·의미가 응집됩니다.
- 메서드 이름 save의 범위가 생성/갱신 모두인지 모호합니다. 팀 용어집에 맞춰 의도가 드러나는 이름·분리를 고민해보실래요? (예: 생성 전용, 변경 전용, 혹은 저장 정책 명시)
As per coding guidelines
app/src/test/java/com/techcourse/service/UserServiceTest.java (2)
31-42: 비밀번호 변경 성공 시 ‘이력 기록’까지 검증해보면 어떨까요?
- 현재 테스트는 비밀번호 변경만 확인합니다. 변경 이력이 요구사항이라면 user_history에 기록되었는지도 함께 검증하면 계약이 더 명확해집니다.
- 힌트: DAO/레포지토리 수준에서 조회 API를 활용하거나, 테스트 전용 헬퍼로 카운트/최근 레코드를 확인하는 방법을 고려해보세요.
As per coding guidelines
45-64: 롤백 테스트는 “비밀번호 유지”뿐 아니라 “이력 미기록”도 확인해보세요.
- 현재는 비밀번호가 바뀌지 않았음을 검증합니다. 추가로 user_history에 레코드가 남지 않았음을 확인하면 트랜잭션 경계의 효과가 더 분명해집니다.
- 만약 확인 수단이 없다면, 테스트 가능하도록 조회 포트를 도입하는 것도 한 방법입니다.
As per coding guidelines
app/src/main/java/com/techcourse/dao/UserDao.java (4)
32-42: 파라미터 인덱스 수동 지정 반복 → 유지보수 비용/실수 위험 대두각 DAO에서 1..N 인덱스를 직접 나열하면 SQL 컬럼 순서 변경 시 다중 위치를 함께 수정해야 합니다. 인덱스를 목록 순서로 자동 매핑하거나(인덱스를 없애고 순서 기반 바인딩), 도메인 객체가 스스로 바인딩 값 집합을 제공하도록 캡슐화하면 결합도를 더 낮출 수 있습니다. 만약 인덱스가 하나라도 틀리면 지금 구조에서 어떤 테스트가 가장 먼저 실패할까요? 이를 통해 리스크를 드러내보면 좋겠습니다.
47-58: 업데이트 파라미터 구성 중복insert와 동일한 방식의 중복이 반복됩니다. 파라미터 빌더/팩토리로 공통화하거나 User가 “업데이트용 파라미터”를 제공하도록 위임하면(“Tell, Don’t Ask”) DAO의 책임이 줄어듭니다. 또한 “규칙 3: 원시값 포장” 관점에서 account/password/email을 값 객체로 포장하면(예: Account, Password, Email) 유효성 로직이 응집되고, 이 레이어의 실수 가능성이 더 낮아집니다. 왜 포장이 중요한지 작은 도메인 제약(길이, 포맷)을 떠올려 보시면 설득력이 생깁니다.
75-79: 예외 타입의 의도 표현존재하지 않는 유저는 “잘못된 인자”라기보다 “도메인 상태 부재”에 가깝습니다. IllegalArgumentException 대신 UserNotFound 같은 도메인 예외로 의도를 드러내면 호출부에서 분기(가드 클로즈)와 로깅 정책이 명확해집니다. “규칙 5: 축약 금지” 관점에서도 의미적 네이밍의 이점이 큽니다. 호출자 입장에서 이 예외를 어떻게 처리할지(재시도? 404 매핑?)까지 연쇄적으로 생각해보셨나요?
88-91: 일관된 접근 — findByAccount도 동일 개선 포인트 적용위와 같은 파라미터 구성/예외 타입 논의를 동일하게 적용하면 API 일관성이 좋아집니다. 또한 account를 값 객체로 포장하면(규칙 3) 조회 키의 유효성 보장이 DAO 바깥으로 누출되지 않습니다.
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
40-42: 예외 포장 정보 부족모든 예외를 RuntimeException으로 래핑하면 호출자는 원인/문맥을 파악하기 어렵습니다. SQL 식별자(문장 ID/카테고리) 등 최소한의 컨텍스트를 담은 도메인 수준 DataAccess 예외로 변환하는 방식을 고려해 보세요. 단, 파라미터의 민감정보는 메시지에 포함하지 않도록 주의가 필요합니다.
app/src/main/java/com/techcourse/service/TransactionalUserService.java (1)
15-17: DataSource 직접 획득(정적 접근) → 결합도 상승/테스트 용이성 저하서비스가 DataSourceConfig에 직접 의존하면 교체/테스트가 어려워집니다. 생성자 주입으로 외부에서 DataSource를 전달받도록 바꾸면(규칙 6/7 관점에서도 책임 분리) 트랜잭션 경계 관리가 명확해지고 테스트 더블 주입이 쉬워집니다. 만약 그렇게 바꾸면 어떤 테스트가 단순해질까요?
jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java (3)
19-34: 검증 로직 널 세이프티와 SQL 파싱 단순화의 한계
- parameters가 null이면 NPE가 발생합니다. 입력에 대한 널 방어가 필요합니다.
- 단순 '?' 카운팅은 리터럴 내 '?'를 구분하지 못합니다. 현재 사용 범위를 고려하면 충분할 수 있으나, 향후 확장(예: 복잡한 SQL/라이브러리 도입)을 염두에 두고 책임 분리를 고민해 보셨나요? 예를 들면, 파라미터 인덱스를 제거하고 “순서 기반”으로만 바인딩하면 이 검증 자체의 복잡도를 낮출 수 있습니다.
49-52: varargs 처리 주의null varargs 전달 시 스트림 생성에서 NPE 위험이 있습니다. 방어 코드를 두거나 varargs 대신 반복 호출을 유도하는 API로 단순화해도 좋습니다. 또한 인덱스를 외부에서 지정하게 하기보다 “추가 순서=인덱스” 규칙으로 단순화하면 실수 여지를 더 줄일 수 있습니다.
59-64: 빌더-레코드 간 컬렉션 소유권 명확화build 시 컬렉션의 소유권을 이전한다는 계약을 명확히 해 주세요(문서화/불변화). 그래야 이후 빌더 재사용/파라미터 추가 시 스펙 인스턴스가 변하지 않습니다. “한 줄에 점 하나(규칙 4)” 관점에서도 API가 더 단순해지고 사용자가 실수하기 어려워집니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/src/main/java/com/techcourse/dao/UserDao.java(2 hunks)app/src/main/java/com/techcourse/dao/UserHistoryDao.java(1 hunks)app/src/main/java/com/techcourse/service/ApplicationUserService.java(1 hunks)app/src/main/java/com/techcourse/service/TransactionalUserService.java(1 hunks)app/src/main/java/com/techcourse/service/UserService.java(1 hunks)app/src/test/java/com/techcourse/dao/UserDaoTest.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(2 hunks)jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementFactory.java(2 hunks)jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java(3 hunks)jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java(1 hunks)study/src/test/java/connectionpool/stage0/Stage0Test.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/MockUserHistoryDao.javaapp/src/main/java/com/techcourse/dao/UserHistoryDao.javaapp/src/test/java/com/techcourse/dao/UserDaoTest.javajdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementFactory.javajdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.javaapp/src/main/java/com/techcourse/service/TransactionalUserService.javastudy/src/test/java/connectionpool/stage0/Stage0Test.javaapp/src/main/java/com/techcourse/service/UserService.javaapp/src/main/java/com/techcourse/dao/UserDao.javajdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.javaapp/src/main/java/com/techcourse/service/ApplicationUserService.javajdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.javaapp/src/test/java/com/techcourse/service/UserServiceTest.java
🧬 Code graph analysis (5)
app/src/main/java/com/techcourse/service/TransactionalUserService.java (4)
jdbc/src/main/java/com/interface21/jdbc/CannotGetJdbcConnectionException.java (1)
CannotGetJdbcConnectionException(5-18)jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
DataSourceUtils(11-37)jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(9-38)app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
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/core/JdbcTemplate.java (1)
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
DataSourceUtils(11-37)
app/src/main/java/com/techcourse/service/ApplicationUserService.java (2)
app/src/main/java/com/techcourse/dao/UserDao.java (1)
UserDao(15-96)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(13-41)
app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
🔇 Additional comments (6)
study/src/test/java/connectionpool/stage0/Stage0Test.java (1)
44-44: 오타 수정 감사합니다!"DirverManager"를 "DriverManager"로 정확하게 수정하셨네요. 교육용 주석의 정확성이 향상되었습니다.
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
34-37: 원래 코멘트의 반환형 변경 우려는 실제 문제 없음; 대신 ThreadLocal 정리 메커니즘 검토 필요.검증 결과:
unbindResource호출처는 1곳뿐이며, 반환값을 사용하지 않음 (TransactionalUserService.java:46)- 반환형을
void로 변경해도 호출부에 영향 없음다만 더 중요한 잠재 문제가 있습니다:
- 현재 코드는
map.remove(key)실행 후, 맵이 비었을 때resources.set(null)미실행- 스레드풀 환경에서 스레드 재사용 시 빈 Map이 ThreadLocal에 계속 남아 메모리 누수 위험
unbindResource메서드에서 마지막 항목 제거 후 빈 맵 정리 로직 추가 여부 확인이 필요합니다.jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementFactory.java (1)
10-22: 체크 예외 격리 전략이 구현되어 있습니다.검증 결과,
PreparedStatementFactory.initialize()에서 발생하는SQLException은JdbcTemplate.execute()메서드의catch (Exception e)블록에서 포착되어RuntimeException으로 래핑됩니다. 따라서 서비스/도메인 계층에 체크 예외가 노출되지 않습니다.다만 한 가지 확인사항:
DataSourceUtils에서는CannotGetJdbcConnectionException(DataAccessException 계층)을 사용하는 반면,JdbcTemplate에서는RuntimeException을 사용합니다. 일관성 측면에서DataAccessException계층의 전용 예외 클래스를 고려해볼 수 있습니다.app/src/main/java/com/techcourse/dao/UserDao.java (1)
65-67: LGTM — 파라미터 없는 조회 스펙 단순 명료단순 조회에서 스펙 빌더 사용으로 일관성이 생겼습니다.
app/src/main/java/com/techcourse/service/ApplicationUserService.java (2)
18-21: LGTM — 조회 위임 단순 명확서비스가 DAO에 명확히 위임하고 있어 가독성이 좋습니다.
23-26: LGTM — 저장 위임 단순 명확불필요한 도메인 지식 유출 없이 위임만 수행합니다.
| new PreparedStatementParameter(1, userHistory.getUserId()), | ||
| new PreparedStatementParameter(2, userHistory.getAccount()), | ||
| new PreparedStatementParameter(3, userHistory.getPassword()), | ||
| new PreparedStatementParameter(4, userHistory.getEmail()), | ||
| new PreparedStatementParameter(5, userHistory.getCreatedAt()), | ||
| new PreparedStatementParameter(6, userHistory.getCreateBy()) | ||
| ) |
There was a problem hiding this comment.
명명 일관성: createdBy vs createBy
- 도메인/서비스 테스트에서는 createdBy를 사용하지만 여기서는 getCreateBy로 보입니다.
- 도메인 용어의 철자/시제를 통일하면 오해를 줄이고, 검색/리팩터링 시 안전합니다. 한번 전역적으로 용어 사전을 정리해보실래요?
As per coding guidelines
🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/dao/UserHistoryDao.java around lines 30 to
36, the code uses getCreateBy() while the domain/tests use createdBy; update
this to use getCreatedBy() (and the corresponding PreparedStatementParameter) to
keep naming consistent: change the method call here to
userHistory.getCreatedBy(), ensure the UserHistory class getter/field is renamed
from createBy to createdBy (and update its setter and any usages), and if
necessary reconcile the DB column/SQL parameter names to created_by to maintain
consistency across codebase and tests.
| @Override | ||
| public void changePassword(final long id, final String newPassword, final String createdBy) { | ||
| try (Connection connection = DataSourceUtils.getConnection(dataSource)) { | ||
| try { | ||
| connection.setAutoCommit(false); | ||
| userService.changePassword(id, newPassword, createdBy); | ||
| connection.commit(); | ||
| } catch (Exception e) { | ||
| connection.rollback(); | ||
| throw e; | ||
| } | ||
| } catch (CannotGetJdbcConnectionException | SQLException e) { | ||
| throw new RuntimeException(dataSource.toString() + "로부터 커넥션을 획득하는 데에 실패했습니다.", e); | ||
| } finally { | ||
| TransactionSynchronizationManager.unbindResource(dataSource); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
트랜잭션 경계 설정은 적절하나, 동일 DataSource 인스턴스 사용 보장과 예외 메시지 민감정보 노출 주의
- 동일 DataSource 인스턴스가 JdbcTemplate 내부에서도 사용되어야 동일 커넥션이 바인딩됩니다. 전 구간에서 DataSourceConfig.getInstance()가 일관되게 사용되는지 확인해 보셨나요? 만약 일부가 다른 인스턴스를 쓰면 트랜잭션이 분리됩니다.
- 예외 메시지에 dataSource.toString()을 포함하면 JDBC URL 등 민감 정보가 노출될 수 있습니다. 메시지는 일반화하고 상세는 cause로 위임하는 편이 안전합니다(컴플라이언스/보안 관점).
동일 DataSource 사용 여부를 점검해 보세요:
🏁 Script executed:
#!/bin/bash
# JdbcTemplate 생성부와 DataSourceConfig 사용 흔적 조회
rg -nP 'new\s+JdbcTemplate\s*\(' -C3
rg -nP 'DataSourceConfig\s*\.\s*getInstance\s*\(' -C2Length of output: 4580
예외 메시지에서 민감 정보 노출 제거
- 일반화된 에러 메시지를 사용자에게 반환하고 상세 정보는 별도로 로깅하는 것이 모범 사례입니다. 현재 코드에서
dataSource.toString()을 예외 메시지에 포함하면 JDBC URL 등 시스템 아키텍처 정보가 노출될 수 있습니다. - 예외 메시지는 "데이터베이스 커넥션을 획득할 수 없습니다"와 같이 일반화하고, 상세 정보(dataSource의 문자열 표현)는 로그에만 기록하세요.
DataSourceConfig.getInstance() 일관성은 codebase 전반에서 잘 유지되고 있으므로 트랜잭션 분리 위험은 낮습니다.
🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/service/TransactionalUserService.java around
lines 32 to 48, remove the sensitive dataSource.toString() from the
RuntimeException message and replace it with a generalized message like "데이터베이스
커넥션을 획득할 수 없습니다."; instead log the detailed dataSource.toString() and exception
stacktrace using the class logger before throwing the generic RuntimeException
so sensitive connection details (JDBC URL etc.) are only in logs and not exposed
in the exception message; keep the existing catch and finally behavior including
TransactionSynchronizationManager.unbindResource(dataSource).
| public record PreparedStatementSpecification(String sql, List<PreparedStatementParameter> parameters) { | ||
|
|
||
| final String sql; | ||
| final List<PreparedStatementParameter> parameters; | ||
|
|
||
| public PreparedStatementSpecification( | ||
| String sql, List<PreparedStatementParameter> parameters) throws SQLSyntaxErrorException { | ||
| public PreparedStatementSpecification { | ||
| validateSpecification(sql, parameters); |
There was a problem hiding this comment.
레코드 불변성 보장 취약 — 파라미터 리스트 방어적 복사 필요
현재 리스트 레퍼런스가 그대로 보관되므로, 빌더나 외부에서 리스트를 변경하면 이미 생성된 스펙이 변질될 수 있습니다(숨은 사이드이펙트/동시성 리스크). 생성자에서 불변 리스트로 방어적 복사를 보장해 주세요. “왜?” 값 객체(레코드)의 핵심은 관찰 가능한 상태가 생성 시점 이후 변하지 않는다는 계약이기 때문입니다.
🤖 Prompt for AI Agents
In
jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java
around lines 9 to 12, the record stores the incoming List reference directly
which allows external mutation and breaks record immutability; instead perform a
defensive copy into an unmodifiable/immutable list (e.g.,
List.copyOf(parameters) or Collections.unmodifiableList(new
ArrayList<>(parameters))) in the canonical constructor after validation, and
also validate that the parameters list and its elements are non-null before
copying so the record retains an immutable, safe snapshot of the parameters.
| throw new IllegalArgumentException( | ||
| new SQLSyntaxErrorException("PreparedStatementContext의 SQL과 파라미터의 개수가 일치하지 않습니다.") | ||
| ); |
There was a problem hiding this comment.
예외 메시지 네이밍 불일치
메시지에 “PreparedStatementContext”라고 표기되어 있으나 타입명은 PreparedStatementSpecification입니다. 용어를 일치시키면 디버깅/검색성이 좋아집니다(규칙 5).
🤖 Prompt for AI Agents
In
jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java
around lines 30 to 32, the thrown exception message refers to
"PreparedStatementContext" but the actual type is
PreparedStatementSpecification; update the exception message text to use
"PreparedStatementSpecification" (keeping the same exception types and
structure) so the wording matches the class name for consistent
debugging/searchability.
| userService.changePassword(id, newPassword, createdBy); | ||
| connection.commit(); | ||
| } catch (Exception e) { | ||
| connection.rollback(); |
There was a problem hiding this comment.
connection.rollback() 메서드 시그니처에 throws SQLException 가 정의 되어 있습니다.
userService.changePassword(...); 로직 중 예외가 발생하여 롤백을 해야하는 상황을 가정하겠습니다.
connection.rollback() 메서드 동작 중 예외가 발생하여 SQLException을 던지면 바깥에 있는 catch 블럭에서 예외가 발생합니다.
} catch (CannotGetJdbcConnectionException | SQLException e) {
throw new RuntimeException(dataSource.toString() + "로부터 커넥션을 획득하는 데에 실패했습니다.", e);
} 이런 경우 changePassword(...) 메서드를 호출한 객체 입장에서는
changePassword 관련 예외를 받지 못하고 RuntimeException(dataSource.toString() + "로부터 커넥션을 획득하는 데에 실패했습니다.", e) 예외를 받게됩니다.
이런 상황이 자주 있지 않겠지만 원본 예외가 손실될 수 있는 문제가 발생합니다.
제가 미션을 할 때 고민한 결과 자원 반납에서 발생하는 예외는 무시하는 방식으로 처리했습니다.
머랭이 생각하는 문제 해결 아이디어가 있으면 공유해주실 수 있나요?
There was a problem hiding this comment.
오 rollback()이 SQLException을 던지는군요!
롤백을 try-catch로 감싸서 해결해 볼 수 있을 것 같네요! 코드는 조금 더러워지겠지만요.
@Override
public void changePassword(final long id, final String newPassword, final String createdBy) {
try (Connection connection = DataSourceUtils.getConnection(dataSource)) {
try {
connection.setAutoCommit(false);
userService.changePassword(id, newPassword, createdBy);
connection.commit();
} catch (Exception e) {
try {
connection.rollback();
} catch (SQLException sqlException) {
throw new RuntimeException("트랜잭션 롤백에 실패했습니다.", sqlException);
}
}
} catch (CannotGetJdbcConnectionException | SQLException e) {
throw new RuntimeException(dataSource.toString() + "로부터 커넥션을 획득하는 데에 실패했습니다.", e);
} finally {
TransactionSynchronizationManager.unbindResource(dataSource);
}
}There was a problem hiding this comment.
예외 관리 좋네요 👍
자원 반납처럼 finally 구문에서 예외가 발생하면 catch문에서 던지는 예외가 덮어씌워지니 이 점 유의하면 좋을 것 같아요.
...rc/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java
Show resolved
Hide resolved
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java
Show resolved
Hide resolved
| private <T> T execute( | ||
| PreparedStatementSpecification preparedStatementSpecification, | ||
| PreparedStatementExecutor<T> preparedStatementExecutor | ||
| ) { | ||
| Connection connection = DataSourceUtils.getConnection(dataSource); | ||
| try ( | ||
| Connection connection = dataSource.getConnection(); | ||
| PreparedStatement preparedStatement = PreparedStatementFactory.initialize( | ||
| connection, | ||
| preparedStatementSpecification | ||
| ) | ||
| ) { | ||
| return preparedStatementExecutor.process(preparedStatement); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
TransactionalUserService를 사용해야 커넥션 획득과 반납의 과정이 일어납니다.
만약 트랜잭션 관련 서비스를 사용하지 않고 Dao가 직접 JdbcTemplate 메서드를 호출하면
JdbcTemplate 안에서 새로운 커넥션이 생성됩니다.
하지만 JdbcTemplate 안에서는 커넥션을 close 하지 않는 문제가 발생합니다.
지금은 커넥션 풀 방식을 사용하지 않아 계속해서 커넥션을 생성하여 커넥션 고갈 문제가 일어나지 않지만, 커넥션 풀 방식일 때 커넥션이 반납되지 않으면 커넥션 고갈 문제가 발생할 것 같습니다.
이 문제를 어떻게 해결해야 할까요?
There was a problem hiding this comment.
그 부분은 커넥션 풀을 구현하면서 고려해야 될 것 같습니다.
커넥션 풀의 세부 구현이 없는 상황에서는 해당 문제를 깔끔하게 해결할 수 있을 것 같지는 않아 보이네요!
There was a problem hiding this comment.
지금은 JdbcTemplate에서 생성된 커넥션이 반납[Connection.close()] 되지 않아 남긴 코멘트였어요.
저는 JdbcTemplate에서 트랜잭션 동작 중이지 않은 커넥션이면 JdbcTemplate에서 처음 시작된 커넥션이라 생각하여 DB 작업 후 커넥션 반납을 했습니다. 👍
위 방법 외에도 여러 구현 방법이 있을 것 같아요. spring은 이 문제를 어떻게 해결했는지 찾아보는 것도 좋을 것 같아요 😄
| @Override | ||
| public void changePassword(final long id, final String newPassword, final String createdBy) { | ||
| try (Connection connection = DataSourceUtils.getConnection(dataSource)) { | ||
| try { | ||
| connection.setAutoCommit(false); | ||
| userService.changePassword(id, newPassword, createdBy); | ||
| connection.commit(); | ||
| } catch (Exception e) { | ||
| connection.rollback(); | ||
| throw e; | ||
| } | ||
| } catch (CannotGetJdbcConnectionException | SQLException e) { | ||
| throw new RuntimeException(dataSource.toString() + "로부터 커넥션을 획득하는 데에 실패했습니다.", e); | ||
| } finally { | ||
| TransactionSynchronizationManager.unbindResource(dataSource); | ||
| } | ||
| } |
There was a problem hiding this comment.
커넥션 획득 + 트랜잭션 시작 + 커밋 또는 롤백 진행 + 커넥션 반납의 과정이
모든 Transactional***Service 형식의 메서드에 중복될 것 같습니다.
이 중복을 어떻게 해결하면 좋을까요?
There was a problem hiding this comment.
AOP를 사용하면 깔끔하게 해결되겠네요~!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java (2)
9-16: 레코드 불변성 보장 필요 - 파라미터 리스트 방어적 복사현재 레코드가 전달받은
parameters리스트의 참조를 그대로 저장하고 있습니다. 이는 외부에서 리스트를 변경할 경우 레코드의 상태가 변경될 수 있어 불변성이 깨질 수 있습니다.왜 이것이 중요할까요?
레코드는 불변 값 객체로 설계되었습니다. 생성 시점 이후 관찰 가능한 상태가 변하지 않아야 한다는 것이 레코드의 핵심 계약입니다.고민해볼 질문:
- 생성자에서 전달받은 리스트를 어떻게 불변 리스트로 변환할 수 있을까요?
- Java 표준 라이브러리에서 제공하는 불변 컬렉션 생성 방법은 무엇이 있을까요?
33-35: 예외 메시지의 클래스명 불일치Line 34의 예외 메시지에서 "PreparedStatementContext"라고 표기하고 있지만, 실제 클래스명은
PreparedStatementSpecification입니다.왜 일치시켜야 할까요?
- 예외 발생 시 개발자가 문제의 출처를 빠르게 파악할 수 있습니다
- 코드 검색 시 일관된 용어를 사용하면 관련 코드를 쉽게 찾을 수 있습니다
- 객체지향 생활 체조 규칙 5(축약 금지)와도 연결되는 명확한 네이밍의 중요성입니다
메시지를 실제 클래스명과 일치하도록 수정해보세요.
🧹 Nitpick comments (1)
jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java (1)
33-35: 예외 래핑 전략에 대한 질문현재
IllegalArgumentException의 생성자에SQLSyntaxErrorException을 직접 전달하여 cause로 설정하고 있습니다. 이 패턴은 문법적으로 유효하지만, 몇 가지 고민해볼 점이 있습니다:질문:
- 예외 메시지가 cause의 메시지만 표시되는데,
IllegalArgumentException자체의 메시지도 함께 제공하면 어떨까요?- 다음과 같은 패턴을 고려해보셨나요?
throw new IllegalArgumentException( "적절한 설명 메시지", new SQLSyntaxErrorException("...") );왜 이것이 중요할까요?
- 예외 스택을 보는 개발자가 어떤 validation이 실패했는지 더 명확하게 파악할 수 있습니다
- 외부 계층에서 예외를 catch할 때 메시지만으로도 문제를 이해할 수 있습니다
현재 패턴을 선택한 이유가 있다면 그것도 좋은 학습 포인트가 될 것 같습니다!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/techcourse/service/TransactionalUserService.java(1 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(2 hunks)jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/techcourse/service/TransactionalUserService.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.javajdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java
🧠 Learnings (1)
📚 Learning: 2025-10-21T09:53:41.119Z
Learnt from: egaeng09
Repo: woowacourse/java-jdbc PR: 1181
File: app/src/main/java/com/techcourse/dao/UserHistoryDao.java:15-26
Timestamp: 2025-10-21T09:53:41.119Z
Learning: egaeng09의 java-jdbc 프로젝트에서는 도메인 모델의 순수성을 우선시하며, 도메인 객체가 JDBC와 같은 기술적 구현체에 의존하지 않도록 합니다. DAO 계층에서 도메인 객체의 getter를 사용하는 것은 계층 분리를 유지하기 위해 허용됩니다.
Applied to files:
jdbc/src/main/java/com/interface21/jdbc/core/preparedstatement/PreparedStatementSpecification.java
🧬 Code graph analysis (1)
app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
🔇 Additional comments (1)
app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
44-64: 트랜잭션 롤백 테스트 구조가 잘 설계되었습니다서비스 계층을 명확히 분리하고(ApplicationUserService, TransactionalUserService), Mock을 활용해 트랜잭션 롤백 동작을 검증하는 구조가 좋습니다. 특히 주석을 통해 각 서비스 계층의 역할을 명시한 점이 테스트 의도를 명확하게 전달합니다.
| final var userService = new TransactionalUserService(new ApplicationUserService(userDao, userHistoryDao)); | ||
|
|
||
| final var newPassword = "qqqqq"; | ||
| final var createBy = "gugu"; |
There was a problem hiding this comment.
변수명 일관성 문제
Line 36의 createBy 변수명이 Line 54의 createdBy와 일치하지 않습니다. 같은 의미를 가진 변수라면 일관된 이름을 사용하는 것이 코드의 가독성을 높입니다.
어느 것이 더 명확한 이름일까요? createBy와 createdBy 중 도메인 의미를 더 잘 표현하는 것은 무엇인지 고민해보세요.
🤖 Prompt for AI Agents
In app/src/test/java/com/techcourse/service/UserServiceTest.java around line 36
(and usage at line 54), the variable name `createBy` is inconsistent with
`createdBy`; rename the variable to `createdBy` (or choose the consistent
domain-preferred name) and update all local usages in this test to match so the
same semantic identifier is used throughout.
3 participants
이제 마지막이네요 ㅎㅎ
이전 이야기 를 반영했습니다.
기존에는 PreparedStatementSpecification의 생성자 시그니처에 throws 가 있었기 때문에, 사용하는쪽은 반드시 예외 핸들링을 해야 했습니다.
저는 제가 설계한 이 구조가 문제라고 판단했습니다.
메서드 시그니처에 예외를 명시하는 이유는 무엇일까요?
저는 "핸들링하지 않으면 큰일나는" 예외이기 때문이라 생각했습니다.
하지만, PreparedStatementSpecification 에서 명시하고 있는 SQLSyntaxErrorException는 핸들링하지 않아도 별 문제 없다고 생각합니다.
개발자의 잘못이고, 테스트 코드를 작성할 것이기 때문입니다.
따라서, 저는 PreparedStatementSpecification이 SQLSyntaxErrorException를 throw한다는 것을 시그니처에서 숨겼습니다.
이에 따라 Dao 들의 중복 코드도 자연스럽게 제거되었습니다.