[4단계 - Transaction synchronization 적용하기] 드라고(이용화) 미션 제출합니다. #1161
Conversation
WalkthroughUserService가 클래스에서 인터페이스로 변경되었고, 도메인 로직은 AppUserService가 구현하며 TxUserService가 TransactionTemplate으로 트랜잭션 경계를 적용하는 래퍼로 추가되었습니다. 기존 jdbc.core의 TransactionSynchronizationManager 클래스는 삭제되고, com.interface21.transaction.support 패키지의 TransactionSynchronizationManager가 ThreadLocal 기반 Map<DataSource, Connection> 리소스 관리를 도입했습니다. JdbcTemplate, TransactionTemplate, DataSourceUtils는 새로운 리소스 관리 방식에 맞춰 커넥션 획득·해제 로직을 수정했으며 테스트 코드는 해당 변경에 따라 업데이트되었습니다. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/test/java/com/techcourse/dao/UserDaoTest.java (1)
21-34: ThreadLocal 리소스와 Connection 정리 누락으로 인한 리소스 누수 문제Line 26의
bindResource()에 대응하는unbindResource()메서드가 존재하는데,@AfterEach clean()메서드에서 이를 호출하지 않고 있습니다. 또한bindResource()는 자원을 현재 스레드에 바인드하며, 이는 ThreadLocal을 통해 저장되므로 명시적으로 해제하지 않으면 테스트 완료 후에도 메모리에 남아있게 됩니다.필요한 수정 사항:
- Line 26에서 얻은
Connection객체를 닫아야 합니다@AfterEach clean()메서드의 마지막에TransactionSynchronizationManager.unbindResource(dataSource)를 호출하여 ThreadLocal에서 리소스를 제거해야 합니다이 두 가지 정리 작업이 없으면:
- 연결 풀이 고갈될 수 있음
- ThreadLocal에 메모리 누수 발생 가능
- 테스트 스위트 실행 시 후속 테스트에 영향을 미칠 수 있음
🧹 Nitpick comments (15)
app/src/main/java/com/techcourse/service/AppUserService.java (2)
30-35: 비즈니스 예외와 감사 로깅 경계 명확화 필요
- id에 해당하는 사용자가 없을 때 처리(도메인 예외, Optional 등)를 어떻게 하실지 결정하셨나요? NPE/DAO 예외 전파보다 의도를 드러내는 예외가 가독성에 좋습니다.
- 비밀번호 변경 규칙(복잡도, 재사용 금지 등)을 어디서 보증할지 고민해보세요. User 스스로 검증하거나 값 객체로 제한하면 응집도가 올라갑니다.
- UserHistory 기록은 트랜잭션 경계(TxUserService) 또는 도메인 이벤트로 위임하면 서비스가 “무엇을”만 표현하고 “언제/어떻게”는 분리됩니다.
20-21: 원시값 포장으로 의도와 불변식 드러내기 (규칙 3)long id, String newPassword, String createdBy를 값 객체(UserId, Password, CreatedBy)로 포장하면 유효성 보증, 오사용 방지, 테스트 용이성이 개선됩니다. 적용 범위를 어디까지 가져갈지(서비스 계층 vs 도메인) 고민해보세요.
Also applies to: 30-31
jdbc/src/main/java/com/interface21/jdbc/core/TransactionTemplate.java (1)
49-51: 자원 해제 책임 중복과 실패 처리 정책 정렬
- releaseConnection(언바인드) 후 직접 close까지 수행합니다. 향후 DataSourceUtils가 close까지 수행하도록 바뀌면 이중 해제가 될 수 있어 책임 경계를 하나로 통일하는 편이 안전합니다.
- close 실패를 로그만 남기고 삼키면 누수 원인 추적이 어려울 수 있습니다. 최소한 모니터링 신호(메트릭/경고) 추가나 상위로 신호를 전달할 기준을 정해두세요.
Also applies to: 52-59
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (2)
14-21: 내부 상태 노출 최소화getResourceMap이 내부 가변 Map을 그대로 노출합니다. 외부에서 직접 조작될 여지를 줄이기 위해 공개 API는 get/bind/unbind로만 한정하고 Map은 숨기는 방향을 고려해 보세요(캡슐화, 불변식 보호).
31-37: 방어적 사용성 강화다른 스레드에서 unbind하거나, 바인딩이 없는 상태에서 unbind가 호출될 때의 처리를 정의해 두면(명시적 예외/무시/로그) 디버깅이 쉬워집니다. 또한 bind/unbind 짝이 맞는지 진단용 훅(디버그 로그/검증)도 도움이 됩니다.
app/src/test/java/com/techcourse/service/UserServiceTest.java (3)
39-41: 서비스 조합 구조 👍 (애플리케이션 서비스 + 트랜잭션 데코레이터)관심사 분리가 명확해졌습니다. 읽기 전용 질의에 대한 트랜잭션 정책(읽기 전용/격리수준)도 추가로 검토해 보세요.
Also applies to: 56-59
31-31: 디미터 법칙/점 하나 규칙 고려 (규칙 4, 5)한 줄에서 dataSource.getConnection()과 bindResource를 동시에 호출하면 의도가 흐려질 수 있습니다. 의미 있는 이름으로 중간 결과를 드러내면 테스트 가독성이 좋아집니다.
61-65: createdBy 네이밍 일관성 확인도메인(UserHistoryDao)이 getCreateBy()를 사용 중입니다(스니펫 참조). 테스트/서비스는 createdBy를 사용합니다. 도메인/DAO/테스트 전 구간에서 네이밍을 통일해 혼동을 줄이는 편이 좋습니다.
app/src/main/java/com/techcourse/service/UserService.java (3)
7-11: 원시값/문자열 포장 고려 (규칙 3).id, newPassword, createdBy가 원시/문자열입니다. 도메인 제약(길이, 포맷, 감사 주체 등)을 객체로 담으면 유효성·의미가 응집됩니다. 예: 식별자, 비밀번호 정책, 감사 정보 객체. "왜 포장해야 할까?"를 관점으로, 검증 위치와 예외 메시지 품질을 떠올려 보시면 방향이 보입니다.
9-9: save의 반환값 설계 재검토.호출자는 저장 결과(생성된 식별자, 버전, 전체 엔티티 등)가 필요할 수 있습니다. 현재 void면 호출자가 후속 조회를 해야 합니다. 저장 후 어떤 정보를 외부에 약속해야 할지, “명령-질의 분리” 관점에서 고민해보셨나요?
11-11: changePassword 파라미터 의미 모델링.newPassword, createdBy는 규칙 9(Tell, Don’t Ask)·감사 모델링과도 연결됩니다. 비밀번호 규칙과 감사 주체를 값 객체/컨텍스트로 캡슐화하면 서비스 시그니처가 더 의도를 드러낼 수 있습니다. “서비스가 무엇을 조율하고, 검증은 어디에 둘 것인가?”를 자문해 보세요. 또한 createdBy로의 리네임이 전체 호출부에 반영되었는지 확인 부탁드립니다.
app/src/main/java/com/techcourse/service/TxUserService.java (4)
14-18: 구성요소 주입으로 결합도 낮추기.DataSourceConfig를 직접 호출하면 환경/테스트 대체가 어려워집니다. TransactionTemplate 또는 DataSource를 생성자 주입으로 전환해 DIP/테스트 용이성을 확보하는 건 어떨까요? “이 클래스를 단위 테스트할 때 무엇을 모킹/스텁할지”를 상상해 보세요.
12-12: 제네릭 raw type 사용으로 인한 타입 안전성 저하.TransactionTemplate가 제네릭인데 raw type을 쓰면서 캐스팅이 발생합니다. 컴파일 타임 보장을 잃고, 오용 시 런타임 오류 위험이 있습니다. 메서드별로 적절한 타입을 부여하거나(혹은 템플릿의 제네릭을 메서드 수준으로 재설계) 제네릭 일관성을 확보하는 방식을 고려해보셨나요?
Also applies to: 21-23, 26-31, 34-39
21-23: 조회에 대한 트랜잭션 경계 재고.findById까지 트랜잭션으로 감싸면 커넥션 점유/락 전략에 영향이 있습니다. 일관성 보장이 필요한 시나리오인지, 아니면 읽기 전용/비트랜잭션으로도 충분한지 요구사항을 다시 점검해보실까요? 읽기 전용 플래그나 분리(읽기=비트랜잭션, 쓰기=트랜잭션)도 선택지입니다.
9-9: 축약 네이밍 지양 (규칙 5).“TxUserService”의 “Tx”는 축약입니다. 의도를 더 명확히 드러내는 이름(예: Transactional... 형태)을 고민해보세요. 팀 내 명명 규칙과 일관성도 함께 확인하면 좋습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
study/test.mv.dbis excluded by!**/*.db
📒 Files selected for processing (10)
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/dao/UserDaoTest.java(2 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(3 hunks)jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java(2 hunks)jdbc/src/main/java/com/interface21/jdbc/core/TransactionSynchronizationManager.java(0 hunks)jdbc/src/main/java/com/interface21/jdbc/core/TransactionTemplate.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(2 hunks)
💤 Files with no reviewable changes (1)
- jdbc/src/main/java/com/interface21/jdbc/core/TransactionSynchronizationManager.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:
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.javajdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.javaapp/src/main/java/com/techcourse/service/AppUserService.javajdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.javaapp/src/main/java/com/techcourse/service/TxUserService.javajdbc/src/main/java/com/interface21/jdbc/core/TransactionTemplate.javaapp/src/test/java/com/techcourse/service/UserServiceTest.javaapp/src/test/java/com/techcourse/dao/UserDaoTest.javaapp/src/main/java/com/techcourse/service/UserService.java
🧬 Code graph analysis (8)
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(8-38)
app/src/main/java/com/techcourse/service/AppUserService.java (1)
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(8-31)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(8-38)
app/src/main/java/com/techcourse/service/TxUserService.java (2)
jdbc/src/main/java/com/interface21/jdbc/core/TransactionTemplate.java (1)
TransactionTemplate(11-61)app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
jdbc/src/main/java/com/interface21/jdbc/core/TransactionTemplate.java (1)
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
DataSourceUtils(11-33)
app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(8-38)
app/src/test/java/com/techcourse/dao/UserDaoTest.java (1)
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (1)
TransactionSynchronizationManager(8-38)
app/src/main/java/com/techcourse/service/UserService.java (1)
study/src/test/java/aop/stage1/UserService.java (1)
UserService(9-36)
🔇 Additional comments (3)
app/src/test/java/com/techcourse/dao/UserDaoTest.java (1)
6-6: 새로운 패키지 구조에 맞게 잘 업데이트하셨습니다.
TransactionSynchronizationManager가interface21.transaction.support패키지로 이동한 것에 맞춰 import를 올바르게 수정하셨네요.jdbc/src/main/java/com/interface21/jdbc/core/TransactionTemplate.java (1)
23-23: 커넥션 획득 책임 이관 좋습니다DataSourceUtils로 일원화한 선택이 타 모듈과의 일관성을 높입니다. 중첩 트랜잭션(이미 바인딩된 커넥션 재사용) 시语도 기대대로 동작하는지 한 번 더 점검해 주세요.
app/src/main/java/com/techcourse/service/UserService.java (1)
5-11: 인터페이스로의 분리는 좋습니다.구현 세부를 분리해 결합도를 낮추고 테스트 용이성이 좋아졌습니다.
|
|
||
| DatabasePopulatorUtils.execute(dataSource); | ||
| TransactionSynchronizationManager.setConnection(dataSource.getConnection()); | ||
| TransactionSynchronizationManager.bindResource(dataSource, dataSource.getConnection()); |
There was a problem hiding this comment.
🧩 Analysis chain
테스트 자원 정리 필수
setUp에서 커넥션을 바인딩하지만 해제/종료(@AfterEach)가 보이지 않습니다. 테스트 간 누수를 막기 위해 언바인드와 커넥션 종료를 보장하는 정리 절차를 추가해 주세요. 트랜잭션 템플릿이 닫았다 하더라도 테스트가 직접 연 커넥션의 lifecycle을 명확히 하는 편이 안전합니다.
---
🏁 Script executed:
```shell
#!/bin/bash
# 테스트 파일 내용 조회
cat "app/src/test/java/com/techcourse/service/UserServiceTest.java"
Length of output: 2562
🏁 Script executed:
#!/bin/bash
# @AfterEach 메서드 검색
rg -n "@AfterEach|@After|tearDown" "app/src/test/java/com/techcourse/service/UserServiceTest.java" -A 10Length of output: 0
🏁 Script executed:
#!/bin/bash
# bindResource와 unbind 관련 호출 검색
rg -n "bindResource|unbindResource|close|finally" "app/src/test/java/com/techcourse/service/UserServiceTest.java"Length of output: 98
---
🌐 Web query:
```web
TransactionSynchronizationManager bindResource connection cleanup teardown
💡 Result:
Short summary — how bindResource cleanup/teardown works in Spring's TransactionSynchronizationManager:
-
Transaction managers (e.g. AbstractPlatformTransactionManager implementations) call initSynchronization() at transaction start and clearSynchronization() at the end; resource binding is intended to be done by those managers (not application code). getResource/getResource check thread-bound resources. [1]
-
bindResource(key,value) ties one resource per key to the current thread and throws IllegalStateException if a resource is already bound. Corresponding unbindResource(key) removes and returns the bound value (throws if none), while unbindResourceIfPossible(key) returns null if none. Use unbindResourceIfPossible when cleanup must be no-op-safe. [1]
-
TransactionSynchronization implementations (e.g. ResourceHolderSynchronization) are registered with registerSynchronization(...) and are used to suspend/resume and to unbind/cleanup resources at the appropriate lifecycle points (suspend(), beforeCompletion/afterCompletion). ResourceHolderSynchronization will unbind resources from TransactionSynchronizationManager when suspending/clearing. [2]
-
Concrete managers release physical resources during teardown. For example JpaTransactionManager unbinds the EntityManager/Connection holders and explicitly releases the JDBC connection via the JpaDialect during transaction cleanup to avoid leaks. Always prefer the transaction manager’s cleanup rather than manual thread-local handling. [3]
If you want code snippets or exact places to call unbindResourceIfPossible vs unbindResource in your code, tell me which transaction manager or resource type you’re using and I’ll show the recommended pattern with lines referenced.
Sources:
[1] Spring TransactionSynchronizationManager Javadoc.
[2] ResourceHolderSynchronization Javadoc.
[3] JpaTransactionManager — connection/holder unbind & release example.
@AfterEach에 리소스 정리 메서드 추가 필수
setUp에서 dataSource.getConnection()으로 열린 커넥션과 bindResource()로 바인딩된 리소스가 테스트 후 정리되지 않고 있습니다. 각 테스트 실행 후 @AfterEach 메서드를 추가하여 다음 작업을 수행해야 합니다:
TransactionSynchronizationManager.unbindResourceIfPossible(dataSource)로 바인딩된 리소스 제거- 열려 있는 커넥션을 명시적으로
close()
이를 통해 테스트 간 리소스 누수를 방지할 수 있습니다.
🤖 Prompt for AI Agents
In app/src/test/java/com/techcourse/service/UserServiceTest.java around line 31,
the test setup binds a Connection via
TransactionSynchronizationManager.bindResource(dataSource,
dataSource.getConnection()) but never cleans it up; add an @AfterEach method
that (1) calls
TransactionSynchronizationManager.unbindResourceIfPossible(dataSource) to remove
the bound resource and (2) closes the Connection opened in setUp (store that
Connection in a test-instance field when calling dataSource.getConnection() so
you can call connection.close() in @AfterEach), ensuring both unbind and
explicit close to prevent resource leaks between tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
74-74: 트랜잭션 외부에서 NPE 발생 가능성 — 안전한 커넥션 획득 방식으로 변경 필요
TransactionSynchronizationManager.getResource(dataSource)는 바인딩된 리소스가 없을 때 null을 반환합니다. 다음 줄(75번)에서conn.prepareStatement(sql)을 호출하면 NPE가 발생하게 됩니다.현재는 모든 테스트에서
setUp()에bindResource()를 미리 호출해두어 문제가 가려져 있습니다. 하지만 JdbcTemplate은 트랜잭션 여부와 무관하게 독립적으로 동작할 수 있어야 합니다.개선 방향:
DataSourceUtils.getConnection(dataSource)를 사용하면 바인딩된 커넥션이 있으면 반환하고, 없으면 새로 생성하므로 null 위험이 없습니다. 이미 TransactionTemplate에서도 같은 방식을 사용하고 있어요.트랜잭션 없이 JdbcTemplate을 직접 사용하는 경우를 고려해보셨나요?
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.java (1)
30-32: releaseConnection이 실제 커넥션을 닫지 않음 — 리소스 누수 위험현재
releaseConnection메서드는 ThreadLocal에서 바인딩만 해제하고 실제 커넥션을 닫지 않습니다. 메서드 시그니처에서Connection파라미터를 받고 있지만 전혀 사용하지 않고 있어요.TransactionTemplate이 이 메서드 호출 후 별도로
connection.close()를 수행하고 있지만(TransactionTemplate 54-55줄), 다른 곳에서 이 메서드를 직접 호출하면 커넥션이 닫히지 않아 누수가 발생합니다.생각해볼 점:
- 메서드 이름이 "release"인데 실제로는 "unbind"만 수행하고 있습니다. 이게 의도한 설계인가요?
- 만약 unbind만 하는 것이 의도라면, 왜 Connection 파라미터를 받고 있을까요?
- 호출하는 쪽에서 항상 별도로 close를 보장할 수 있을까요?
리소스 정리 책임을 어느 계층에서 가져갈지 명확히 하고, 메서드 이름과 동작이 일치하도록 개선이 필요합니다.
app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
31-31: 테스트 리소스 정리 누락 — @AfterEach 메서드 추가 필수
setUp()에서bindResource(dataSource, dataSource.getConnection())로 커넥션을 열고 바인딩했지만, 테스트 종료 후 이를 정리하는@AfterEach메서드가 없습니다.필요한 작업:
- 26번 줄에서 열린 커넥션을 인스턴스 변수에 저장
@AfterEach메서드 추가TransactionSynchronizationManager.unbindResource(dataSource)호출- 저장된 커넥션을
close()테스트 간 격리를 위해 각 테스트가 사용한 리소스를 완전히 정리하는 습관을 들이는 것이 중요합니다.
🧹 Nitpick comments (5)
jdbc/src/main/java/com/interface21/jdbc/core/TransactionTemplate.java (1)
59-59: 불필요한 공백 라인 제거59번 줄의 빈 줄이 불필요합니다. 메서드 종료 직전의 공백은 코드 가독성을 해치지 않도록 제거하는 것이 좋습니다.
app/src/main/java/com/techcourse/service/UserService.java (1)
11-11: changePassword 메서드의 파라미터 개수 고려현재
changePassword는 3개의 파라미터(id, newPassword, createdBy)를 받고 있습니다. 객체지향 생활 체조 원칙에서는 메서드 파라미터를 최소화할 것을 권장합니다.생각해볼 점:
만약 비밀번호 변경 요청 정보가 더 늘어난다면? 예를 들어 변경 사유, 이전 비밀번호 확인 등이 추가된다면?이런 경우
PasswordChangeRequest같은 객체로 파라미터들을 묶는 것을 고려해볼 수 있습니다. 다만 현재 수준에서는 무리한 추상화일 수 있으니, 요구사항이 늘어날 때 리팩터링하는 것도 좋은 전략입니다.app/src/main/java/com/techcourse/service/TxUserService.java (3)
14-18: 의존성 주입 방식을 고려해보세요현재
DataSource를DataSourceConfig.getInstance()로 직접 가져오고 있는데, 이 방식에는 몇 가지 고려할 점이 있습니다:
- 테스트 용이성: 만약 이 클래스를 단위 테스트할 때 다른
DataSource를 사용하고 싶다면 어떻게 해야 할까요?- 결합도: 현재
TxUserService는DataSourceConfig에 직접 의존하고 있습니다. 이것이 클래스의 재사용성과 유연성에 어떤 영향을 미칠까요?- 의존성 주입 원칙:
UserService는 생성자로 주입받는데,DataSource는 왜 다르게 처리했을까요?💡 힌트: 생성자 파라미터로 필요한 의존성을 모두 받는 방식을 고려해보세요. 이렇게 하면 테스트 시 원하는 구현체를 주입할 수 있고, 클래스 간 결합도도 낮출 수 있습니다.
</invoke_end>
20-23: 타입 캐스팅이 필요한 이유를 확인해보세요
transactionTemplate.execute()의 반환값을(User)로 캐스팅하고 있습니다.
- 람다 표현식에서 이미
User타입을 반환하는데, 왜 캐스팅이 필요할까요?TransactionTemplate의 제네릭 타입 설계를 확인해보셨나요?💡 이런 명시적 캐스팅은 타입 안정성을 떨어뜨리고 런타임 에러의 가능성을 높입니다.
TransactionTemplate의 설계를 개선하면 이 캐스팅을 제거할 수 있을 것 같습니다.</invoke_end>
25-31: void 메서드에서 null을 반환하는 패턴에 대해 생각해보세요
save와changePassword메서드에서 트랜잭션 내부 로직 실행 후null을 반환하고 있습니다.
- 이 패턴이 반복되는 이유가 무엇일까요?
TransactionTemplate이 void 반환 타입을 지원하지 않는 것 같은데, 이것이 좋은 API 설계일까요?💡 힌트:
- 반환값이 있는 경우와 없는 경우를 구분하는 별도의 메서드를 제공하는 방법
- Consumer 같은 함수형 인터페이스를 활용하는 방법
을 고려해보면 코드의 의도가 더 명확해질 것 같습니다.
</invoke_end>
Also applies to: 33-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
study/test.mv.dbis excluded by!**/*.db
📒 Files selected for processing (10)
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/dao/UserDaoTest.java(2 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(3 hunks)jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java(2 hunks)jdbc/src/main/java/com/interface21/jdbc/core/TransactionSynchronizationManager.java(0 hunks)jdbc/src/main/java/com/interface21/jdbc/core/TransactionTemplate.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(2 hunks)
💤 Files with no reviewable changes (1)
- jdbc/src/main/java/com/interface21/jdbc/core/TransactionSynchronizationManager.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:
jdbc/src/main/java/com/interface21/jdbc/datasource/DataSourceUtils.javajdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.javaapp/src/main/java/com/techcourse/service/AppUserService.javaapp/src/test/java/com/techcourse/service/UserServiceTest.javajdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.javaapp/src/test/java/com/techcourse/dao/UserDaoTest.javajdbc/src/main/java/com/interface21/jdbc/core/TransactionTemplate.javaapp/src/main/java/com/techcourse/service/TxUserService.javaapp/src/main/java/com/techcourse/service/UserService.java
🔇 Additional comments (10)
jdbc/src/main/java/com/interface21/jdbc/core/TransactionTemplate.java (1)
49-59: closeConnection 메서드가 releaseConnection과 close를 모두 수행 — 중복 책임현재
closeConnection메서드는:
DataSourceUtils.releaseConnection()호출 (50줄)- 별도로
connection.close()호출 (54-55줄)이는 DataSourceUtils의
releaseConnection이 실제로 커넥션을 닫지 않기 때문에 필요한 workaround입니다.생각해볼 점:
- DataSourceUtils가 완전한 정리를 수행한다면, 이 메서드는 단순히
releaseConnection만 호출하면 되지 않을까요?- 현재 구조에서는 "release"와 "close"의 책임이 명확하게 분리되지 않아 혼란스러울 수 있습니다.
DataSourceUtils.releaseConnection의 책임이 명확해지면 이 부분도 자연스럽게 개선될 것 같습니다.
jdbc/src/main/java/com/interface21/transaction/support/TransactionSynchronizationManager.java (2)
14-21: LGTM! Map 기반 리소스 관리 구조Lazy initialization을 통해 필요할 때만 HashMap을 생성하는 구조가 좋습니다. 여러 DataSource를 지원할 수 있게 되었네요.
단일 DataSource에서 Map으로 전환한 이유가 무엇인지 궁금합니다. 미션 요구사항이었나요, 아니면 확장성을 고려한 설계 개선인가요?
31-37: ThreadLocal 메모리 누수 방지 로직 — 훌륭합니다!Map이 비었을 때 ThreadLocal을 제거하는 로직(33-35줄)이 인상적입니다. 이 부분이 없으면 장기 실행 애플리케이션에서 메모리 누수가 발생할 수 있는데, 이를 잘 방어하셨네요.
이런 세심한 리소스 관리가 프로덕션 환경에서 중요한 이유를 이해하고 계신 것 같습니다!
app/src/test/java/com/techcourse/service/UserServiceTest.java (2)
39-40: Decorator 패턴을 활용한 트랜잭션 분리 — 잘 설계하셨습니다!
AppUserService에서 비즈니스 로직을,TxUserService에서 트랜잭션 관리를 담당하도록 관심사를 명확히 분리했네요. 이는 단일 책임 원칙(SRP)을 잘 따른 설계입니다.이렇게 분리하면 어떤 장점이 있을까요?
- 비즈니스 로직을 트랜잭션 없이 테스트 가능
- 트랜잭션 전략 변경 시 비즈니스 로직 영향 없음
- 코드의 의도가 명확해짐
실무에서도 자주 사용되는 패턴입니다. 이번 미션을 통해 관심사 분리의 가치를 체감하셨나요?
55-58: 의도를 설명하는 주석 — 좋은 습관입니다!코드 구조를 설명하는 주석이 테스트 가독성을 높여주네요. 특히 "애플리케이션 서비스"와 "트랜잭션 서비스 추상화"를 명시적으로 구분한 점이 인상적입니다.
다만, 주석이 필요 없을 만큼 코드가 자명하다면 더 좋겠죠? 예를 들어 변수명만으로도 역할이 명확하다면 주석 없이도 이해할 수 있을 것 같습니다.
app/src/main/java/com/techcourse/service/UserService.java (1)
5-12: 인터페이스 추출로 추상화 계층 추가 — 좋은 설계 방향입니다!구체 클래스를 인터페이스로 전환하여 구현체(AppUserService, TxUserService)를 숨기고 추상화에 의존하도록 만든 점이 좋습니다. 이는 의존성 역전 원칙(DIP)을 따른 설계입니다.
질문:
이렇게 인터페이스를 도입하면서 어떤 이점을 느끼셨나요? 테스트 작성이나 코드 확장 측면에서 차이가 있었나요?app/src/main/java/com/techcourse/service/AppUserService.java (3)
8-16: LGTM! 깔끔한 의존성 주입 구조두 개의 DAO를 final 필드로 받아 불변성을 보장하고 있습니다. 인스턴스 변수가 2개로 객체지향 생활 체조의 "인스턴스 변수 3개 이하" 원칙도 잘 지키고 있네요.
20-27: 조회와 저장 로직 — 심플하고 명확합니다
findById와save메서드가 각각 하나의 책임만 가지고 있어 이해하기 쉽습니다.작은 질문:
25-27줄의save메서드가 내부적으로userDao.insert()를 호출하는데, 메서드 이름이 다릅니다. 이는 의도적인 선택인가요?
- 도메인 용어로는 "save"가 더 적절하지만
- DAO 계층에서는 "insert/update"로 구분
이런 용어 선택의 이유를 팀원들과 공유해두면 좋을 것 같습니다.
30-35: 비즈니스 로직의 흐름이 명확합니다비밀번호 변경의 흐름이 순차적으로 잘 표현되어 있습니다:
- 사용자 조회
- 비밀번호 변경
- 변경사항 저장
- 이력 기록
각 단계가 한 줄로 표현되어 가독성이 좋네요. 메서드의 들여쓰기도 1단계로 객체지향 생활 체조 원칙을 잘 따르고 있습니다.
질문:
만약 3단계(userDao.update)가 실패하면 4단계(이력 기록)는 어떻게 될까요? 트랜잭션이 이를 어떻게 보호하는지 생각해보셨나요?app/src/main/java/com/techcourse/service/TxUserService.java (1)
9-40: Decorator 패턴을 잘 적용하셨습니다! 👍트랜잭션 처리 책임을 별도의 클래스로 분리하신 점이 훌륭합니다:
- ✅
AppUserService는 비즈니스 로직에만 집중- ✅
TxUserService는 트랜잭션 경계 관리에만 집중- ✅ 단일 책임 원칙(SRP)을 잘 따르고 있습니다
이런 구조는 나중에 트랜잭션 전략을 변경하거나, 로깅/모니터링 같은 횡단 관심사를 추가할 때도 유리합니다.
🎓 학습 포인트: 이처럼 핵심 기능(Core)과 부가 기능(Cross-cutting concerns)을 분리하는 것이 AOP(Aspect-Oriented Programming)의 기본 아이디어입니다. Spring의
@Transactional도 내부적으로 비슷한 프록시 패턴을 사용합니다.</invoke_end>
| TransactionSynchronizationManager.bindResource(dataSource, dataSource.getConnection()); | ||
| userDao = new UserDao(jdbcTemplate); | ||
| } |
There was a problem hiding this comment.
테스트 리소스 정리 누락 — @AfterEach에서 커넥션 해제 필요
setUp()에서 dataSource.getConnection()으로 커넥션을 열고 bindResource()로 바인딩했지만, 테스트 종료 후 이를 정리하는 코드가 없습니다.
현재 @AfterEach clean() 메서드는 데이터베이스 데이터만 정리하고 있어요(30-34줄). 이로 인해 테스트 간 리소스 누수가 발생할 수 있습니다.
개선 방향:
- 26번 줄에서 열린 커넥션을 인스턴스 필드에 저장
clean()메서드에서unbindResource()호출- 저장해둔 커넥션을 명시적으로
close()
테스트가 독립적으로 실행되려면 각 테스트가 사용한 리소스를 완전히 정리해야 합니다. 이 부분을 어떻게 개선할 수 있을지 고민해보세요.
🤖 Prompt for AI Agents
In app/src/test/java/com/techcourse/dao/UserDaoTest.java around lines 26-28, the
Connection opened with dataSource.getConnection() and bound via
TransactionSynchronizationManager.bindResource(...) in setUp() is not released;
modify setUp() to save the returned Connection to an instance field, and in the
@AfterEach clean() method call
TransactionSynchronizationManager.unbindResource(dataSource) and then close the
stored Connection (with null check and swallowing or logging any SQLException)
to ensure the bound resource is unbound and the connection is closed after each
test.
1 participant
하루 안녕하세요!
마지막 미션도 리뷰 잘 부탁드립니다!!
1. Transaction Synchronization 패턴 적용
TransactionSynchronizationManager를 transaction.support 패키지로 이동2. Connection 관리 개선
JdbcTemplate:TransactionSynchronizationManager를 통한 connection 조회TransactionTemplate:DataSourceUtils를 활용한 connection 생명주기 관리DataSourceUtils: resource binding/unbinding 로직 통합