[3단계 - Transaction 적용하기] 시소(이갱민) 미션 제출합니다. #1181
Conversation
|
@coderabbitai review start |
|
@egaeng09, I'll start reviewing the changes in your PR for the Transaction implementation step. ✅ Actions performedReview triggered.
|
Walkthrough데이터베이스 접근 계층이 외부 JDBC Connection을 직접 받아 작업할 수 있도록 확장되었습니다. UserDao에 Connection 파라미터 오버로드(update(Connection, User), findById(Connection, Long))가 추가되고 로거 필드가 제거되었습니다. UserHistoryDao는 DataSource 기반에서 JdbcTemplate 기반으로 마이그레이션되며 생성자가 JdbcTemplate을 받도록 변경되고 log(Connection, UserHistory) 오버로드가 추가되었습니다. UserService는 DataSource 의존성을 도입하고 changePassword에 Connection 기반 트랜잭션 로직을 도입했습니다. MockUserHistoryDao에 Connection 오버로드가 추가되었고 테스트는 생성자 호출과 @disabled 제거로 업데이트되었습니다. JdbcTemplate에 Connection 기반의 update/query/queryForObject 메서드가 추가되었습니다. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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/MockUserHistoryDao.java (1)
9-23: 상속 기반 목(mock) 대신 포트/인터페이스 도입 고려생산 코드 DAO를 상속하는 방식은 내부 변경에 테스트가 쉽게 깨지는 결합도를 만듭니다. 인터페이스(포트)를 두고 테스트 더블을 구현하면 결합도를 낮추고 의도를 더 명확히 표현할 수 있어요. 만약 포트를 도입한다면 UserService는 포트에 의존하고, 실제/목 DAO는 그 포트를 구현하도록 분리해보면 어떨까요?
app/src/test/java/com/techcourse/service/UserServiceTest.java (2)
31-43: 성공 시 부수효과까지 검증해보면 어떨까요?비밀번호 변경 성공만 확인하고 있어요. 같은 트랜잭션에 포함된 user_history insert가 실제로 기록됐는지도 함께 검증하면 트랜잭션 경계가 완전하게 테스트됩니다. 어떤 지표(행 수, 최신 레코드의 created_by 등)를 확인할지 스스로 정해보세요.
45-60: 롤백 시 “아무것도 남기지 않음”까지 확인 필요예외로 인해 비밀번호 미변경은 확인했지만, user_history가 기록되지 않았다는 보증은 아직 없어요. 트랜잭션 원자성 관점에서 이력 테이블에도 변화가 없음을 함께 검증해보는 건 어떨까요?
또한 H2 메모리 DB 설정(DB_CLOSE_DELAY=-1)과 DatabasePopulatorUtils가 각 테스트 시작 시 ID 초기화/상태 격리를 확실히 보장하는지 점검해보셨나요?
app/src/main/java/com/techcourse/dao/UserDao.java (1)
10-62: DAO API 혼용과 SQL 중복 관리
- 내부(DataSource 사용) 메서드와 외부(Connection 주입) 메서드가 공존하면 호출측에서 혼용될 수 있어요. “트랜잭션 경계 안에서는 반드시 Connection 오버로드만 사용” 같은 팀 규칙/가이드를 명문화해보셨나요?
- 동일 SQL이 여러 메서드에 분산되어 있어 변경 비용이 커질 수 있습니다. SQL 상수화/전략화 등 중복을 줄이는 방향을 검토해보세요.
서비스 계층의 모든 트랜잭션 경로가 실제로 Connection 오버로드만 사용하도록 테스트로 보장되고 있나요?
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
103-117: ParameterMetaData 기반 파라미터 개수 검증 — 드라이버 의존성 주의일부 JDBC 드라이버는 ParameterMetaData를 부분 지원하거나 부정확한 값을 반환하기도 합니다. H2에서는 대체로 동작하지만, 다른 DB로 전환 시 실패 지점이 될 수 있어요. 실패 시점/메시지가 개발자 경험에 도움이 되는지도 함께 점검해보면 좋겠습니다. 필요한 경우 선택적 검증이나 테스트로 보강해보는 접근을 고려해보세요.
현재 사용하는 드라이버 조합(H2 포함)에서 이 검증이 항상 안정적으로 동작하는지 테스트 증거가 있나요?
app/src/main/java/com/techcourse/service/UserService.java (4)
23-27: 트랜잭션 책임의 위치 — Service에 DataSource를 둘 것인가?현재 Service가 커넥션/트랜잭션을 직접 관리합니다. 학습 단계에선 적절하지만, 확장 시 트랜잭션 템플릿(혹은 유즈케이스 수준의 경계)로 분리하면 테스트 용이성과 재사용성이 더 좋아집니다. “연결 관리”와 “비즈니스 규칙”을 의식적으로 분리하는 설계를 고려해보세요.
38-57: changePassword — 원자성 확보 OK, 설계 개선 포인트 몇 가지
- 메서드 길이가 다소 길고(규칙 6), 예외/자원 처리까지 함께 다룹니다. 트랜잭션 경계(시작/종료)와 실제 도메인 행위를 작은 메서드로 더 분해해보면 어떨까요?
- 원시값 포장(규칙 3): id, newPassword, createBy를 도메인 값 객체로 감싸면 의도를 더 분명히 표현할 수 있어요(형식/정책 검증도 같은 곳에 응집).
- 실패 시 예외 매핑: NoSuchElementException(도메인 상황)까지 DataAccessException(인프라)로 래핑되면 의미가 흐려집니다. 도메인 예외와 인프라 예외를 구분해 전파하는 방식을 고민해보세요.
이 메서드가 실패할 수 있는 경로(사용자 없음, DB 예외 등)를 모두 테스트로 커버하고 있나요?
59-63: private findById(Connection, …) — “점 하나”와 예외 의미
- 규칙 1/4 관점에서 메서드 체이닝을 줄이기 위해 중간 개념을 도입(예: 조회 결과를 나타내는 타입/전략)해보면 복잡도가 낮아집니다.
- 예외 타입은 도메인 의미가 드러나는 전용 예외로 바꿔보면 상위 계층에서의 처리 정책이 명확해집니다.
64-72: rollback/close 자원 처리 보완 여지현재도 안전하지만, “성공/실패 로그의 일관성”과 “후속 복구 실패 시 신속 가시성”을 위해 로그 수준/메시지 표준화를 고려해보세요. 또한 트랜잭션 템플릿을 도입하면 이 두 메서드는 공통 유틸로 이동시켜 재사용할 수 있습니다.
Also applies to: 74-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/com/techcourse/dao/UserDao.java(3 hunks)app/src/main/java/com/techcourse/dao/UserHistoryDao.java(1 hunks)app/src/main/java/com/techcourse/service/UserService.java(1 hunks)app/src/test/java/com/techcourse/service/MockUserHistoryDao.java(2 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(2 hunks)jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java(4 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.javaapp/src/test/java/com/techcourse/service/MockUserHistoryDao.javajdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.javaapp/src/main/java/com/techcourse/dao/UserHistoryDao.javaapp/src/main/java/com/techcourse/service/UserService.javaapp/src/main/java/com/techcourse/dao/UserDao.java
🧬 Code graph analysis (2)
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/UserService.java (3)
app/src/main/java/com/techcourse/dao/UserDao.java (1)
UserDao(10-62)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(7-41)app/src/main/java/com/techcourse/domain/UserHistory.java (1)
UserHistory(5-59)
🔇 Additional comments (5)
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (1)
20-23: 트랜잭션 경로용 오버로드 추가 적절변경된 UserService 흐름(연결 주입)에 맞춰 예외 유발 지점을 정확히 겨냥할 수 있게 되었어요. 테스트 목적에 부합합니다.
app/src/main/java/com/techcourse/dao/UserDao.java (1)
35-38: 트랜잭션 연동을 위한 오버로드 추가 적절서비스가 동일 커넥션을 공유해 원자성을 보장하기에 필요한 확장입니다. Optional로의 래핑도 일관적이에요.
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (2)
66-73: 외부 커넥션 자원 관리 경계 명확PreparedStatement만 try-with-resources로 닫고, Connection은 호출자가 소유하도록 남겨둔 선택이 트랜잭션 시나리오에 적합합니다. 호출측(UserService)와의 역할 분리가 명확해졌어요.
Also applies to: 84-90
40-46: JDK 호환성 검토 불필요 — 프로젝트가 이미 JDK 21로 설정됨이 리뷰 코멘트는 프로젝트의 실제 JDK 타깃 버전을 확인하지 않고 작성되었습니다.
jdbc/build.gradle에서 다음과 같이 명시적으로 설정되어 있습니다:sourceCompatibility = JavaVersion.VERSION_21 targetCompatibility = JavaVersion.VERSION_21프로젝트가 JDK 21을 타깃하므로,
List.getFirst()사용은 완벽히 호환되며 문제가 없습니다. 이 API는 Java 21 SequencedCollection에서 정식 제공되는 메서드입니다.결론: 현재 코드는 올바르며, 이 리뷰 코멘트는 무시해도 됩니다.
Likely an incorrect or invalid review comment.
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
9-13: JdbcTemplate 기반으로의 전환이 잘 되었습니다!DataSource에서 JdbcTemplate으로 전환하면서 의존성 주입을 적절히 활용하고 있고, 인스턴스 변수도 1개로 단순하게 유지되고 있습니다.
egaeng09
changed the title
There was a problem hiding this comment.
시소 try-with-resources 처리를 한 번 개선해보는 건 어때요?
이건 2중 try-catch 사용하면 로직이 간소화 되어 메서드 분리도 없어질 거 같아요!
public void changePassword(final long id, final String newPassword, final String createBy) {
try (Connection conn = dataSource.getConnection()) {
conn.setAutoCommit(false);
try {
final var user = findById(conn, id);
user.changePassword(newPassword);
userDao.update(conn, user);
userHistoryDao.log(conn, new UserHistory(user, createBy));
conn.commit();
} catch (Exception e) {
conn.rollback();
throw e;
}
} catch (DataAccessException | SQLException e) {
throw new DataAccessException("비밀번호 변경 실패", e);
}
}2 participants
안녕하세용 비타 💊
미션 진행이 더 지연되면 리뷰어 입장에서도 부담될 수 있을 것 같아서 최대한 빨리 미션 진행해보았어요!
이번 스텝도 잘 부탁드려요 🙇 🙇