[3단계 - Transaction 적용하기] 레오(김준영) 미션 제출합니다. #1182
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
워크스루이 PR은 JDBC Connection을 명시적으로 사용하는 트랜잭션 관리를 지원하도록 DAO 레이어와 UserService를 리팩토링합니다. UserDao와 UserHistoryDao에 Connection 매개변수를 받는 오버로드된 메서드가 추가되어 UserService가 changePassword 작업의 트랜잭션을 관리할 수 있게 합니다. JdbcTemplate 전반에서 DataAccessApiException을 제거하고 DataAccessException으로 예외 처리를 통합합니다. UserServiceTest의 @disabled 주석이 제거되어 이전에 건너뛴 테스트를 실행하도록 활성화합니다. 이러한 변경사항은 단일 트랜잭션 컨텍스트 내에서 여러 데이터베이스 작업을 조율하기 위한 기반을 구축합니다. 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 (8)
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (1)
21-24: Mock이 항상 예외를 던지는 설계 의도 확인 필요
- 이 Mock이 “실패 시나리오 전용”인지요? 성공 경로도 검증하려면 동작을 구성(플래그/주입) 가능하게 하는 편이 테스트 가독성과 재사용성에 유리합니다.
- 예외에 메시지/원인을 포함하면 실패 원인 추적이 쉬워집니다.
- 명명도 Mock보다는 Stub/Fake/FailingDouble 등 의도를 드러내면 더 명확합니다.
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (2)
40-52: 중복 SQL 제거로 응집도 향상두 오버로드가 동일한 SQL과 파라미터 구성을 중복합니다. 공통 준비/바인딩 로직을 추출하면 변경 비용과 실수를 줄일 수 있어요.
- 왜: DRY 위배 → 유지보수성 저하.
- 힌트: 공통 메서드/상수로 SQL과 매핑을 한곳에서 관리.
3-4: 이름 충돌로 인한 가독성 저하 가능성클래스 필드 Logger 이름이 log, 메서드 이름도 log입니다. 독자 입장에서 혼동을 유발할 수 있어요. Logger를 더 명시적인 이름으로 바꾸는 것을 고려해보세요.
Also applies to: 40-52
app/src/main/java/com/techcourse/dao/UserDao.java (1)
50-64: 중복 로직 제거 및 예외 타입 일관화 검토
- 두 update 오버로드가 거의 동일합니다. 공통화로 변경 누락 리스크를 줄여보세요.
- 서비스/템플릿은 DataAccessException을 사용 중인데, 여기서는 RuntimeException을 던집니다. 예외 계층을 통일하면 호출 측 처리와 로깅이 단순해집니다.
- 비밀번호가 DB에 저장될 때 해싱이 보장되는지 확인 부탁드립니다. 도메인/DAO 어느 계층에서 책임질지 결정해 응집도를 높여보세요.
app/src/main/java/com/techcourse/service/UserService.java (3)
17-25: DataSource를 직접 획득하는 강한 결합 — DI로 테스트성과 확장성 향상
- 서비스가 DataSourceConfig 싱글턴에 의존해 숨은 의존성이 생깁니다. 생성자/팩토리 주입으로 결합을 낮추면 테스트 더블 주입과 환경 전환이 쉬워집니다.
36-46: 트랜잭션 경계 안/밖 I/O 혼재 여부 점검
- findById는 별도 커넥션으로 동작하고, update/log는 동일 커넥션을 사용합니다. 일관된 스냅샷이 필요한 경우 조회도 동일 커넥션(또는 적절한 격리 수준)에서 처리할지 고민해보셨나요?
36-61: 메서드 중첩(depth)과 길이 — 규칙 1, 6 관점에서 단순화
- 중첩 try-catch 및 다단 분기 구조로 depth>1, 10라인 초과입니다.
- 힌트: 트랜잭션 템플릿(고차함수/콜백) 추출, rollback/close 유틸 분리로 메서드 본문을 “행위” 중심으로 평탄화해보세요.
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
72-75: Connection 보유권과 사용 계약을 명시하면 오용을 줄일 수 있어요
- 호출자가 커넥션 수명과 트랜잭션 경계를 책임지고, 템플릿은 Statement만 관리한다는 계약을 Javadoc/문서로 드러내면 실수(autocommit 설정 변경, close 시점 혼동)를 줄일 수 있습니다.
- 로그 메시지 표현(“query” vs “update”)도 통일하면 운영 관찰성이 좋아집니다.
Also applies to: 80-94
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/com/techcourse/dao/UserDao.java(2 hunks)app/src/main/java/com/techcourse/dao/UserHistoryDao.java(2 hunks)app/src/main/java/com/techcourse/service/UserService.java(2 hunks)app/src/test/java/com/techcourse/service/MockUserHistoryDao.java(2 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(0 hunks)jdbc/src/main/java/com/interface21/jdbc/core/DataAccessApiException.java(0 hunks)jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java(4 hunks)
💤 Files with no reviewable changes (2)
- jdbc/src/main/java/com/interface21/jdbc/core/DataAccessApiException.java
- app/src/test/java/com/techcourse/service/UserServiceTest.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/main/java/com/techcourse/dao/UserHistoryDao.javaapp/src/test/java/com/techcourse/service/MockUserHistoryDao.javaapp/src/main/java/com/techcourse/dao/UserDao.javaapp/src/main/java/com/techcourse/service/UserService.javajdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java
🧬 Code graph analysis (2)
app/src/main/java/com/techcourse/service/UserService.java (4)
app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)app/src/main/java/com/techcourse/dao/UserDao.java (1)
UserDao(12-80)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(13-53)app/src/main/java/com/techcourse/domain/UserHistory.java (1)
UserHistory(5-59)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
jdbc/src/main/java/com/interface21/dao/DataAccessException.java (1)
DataAccessException(3-26)
🔇 Additional comments (2)
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (1)
3-4: 테스트 설정 확장은 타당합니다.Connection 기반 시그니처 추가로 트랜잭션 경로 테스트가 가능해졌습니다. 목적에 부합해 보여요.
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
45-49: 예외 타입 일원화 좋습니다DataAccessException으로 통일되어 상위 계층의 예외 처리 일관성이 개선됩니다.
Also applies to: 68-68, 103-103
Comment on lines
+40
to
+52
| public void log(final Connection connection, final UserHistory userHistory) { | ||
| final var sql = "insert into user_history (user_id, account, password, email, created_at, created_by) values (?, ?, ?, ?, ?, ?)"; | ||
| jdbcTemplate.update( | ||
| connection, | ||
| sql, | ||
| userHistory.getUserId(), | ||
| userHistory.getAccount(), | ||
| userHistory.getPassword(), | ||
| userHistory.getEmail(), | ||
| userHistory.getCreatedAt(), | ||
| userHistory.getCreateBy() | ||
| ); | ||
| } |
There was a problem hiding this comment.
비밀번호를 이력 테이블에 그대로 저장 — 심각한 보안/컴플라이언스 이슈
- user_history에 password 컬럼까지 함께 적재되고 있어요. 평문/가역형태로의 저장은 규정 위반 및 사고 리스크가 큽니다.
- “왜 중요한가”: 유출 시 영구 피해, 규제 위반(GDPR/개인정보보호), 감사 시 지적 포인트.
- “검토 포인트”: 정말 필요한가? 불가피하다면 비가역 해시(솔트 포함) 또는 아예 저장 제거/마스킹. 운영/감사 관점에서 대체 식별자(예: user_id)만 남기는 방향을 고려해보세요.
🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/dao/UserHistoryDao.java around lines 40 to
52, the code inserts the user's plaintext password into the user_history table;
remove this to avoid storing reversible passwords. Fix by removing the password
column and its parameter from the SQL and jdbcTemplate.update call (i.e., no
password value inserted), or if audit requires retaining a verifiable value
store a securely hashed value instead: compute a salted, one-way hash (e.g.,
bcrypt/Argon2 via a PasswordEncoder) and insert that into a dedicated
password_hash column (update schema accordingly) while ensuring parameter order
matches the SQL. Ensure tests and any callers are updated for the changed
parameter list and that no plaintext password is logged or persisted anywhere.
Comment on lines
+36
to
+61
| Connection connection = null; | ||
| try { | ||
| connection = dataSource.getConnection(); | ||
| connection.setAutoCommit(false); | ||
|
|
||
| final var user = findById(id); | ||
| user.changePassword(newPassword); | ||
| userDao.update(connection, user); | ||
| userHistoryDao.log(connection, new UserHistory(user, createBy)); | ||
| connection.commit(); | ||
| } catch (Exception e) { | ||
| try { | ||
| connection.rollback(); | ||
| } catch (SQLException sqlException) { | ||
| throw new DataAccessException(sqlException); | ||
| } | ||
| throw new DataAccessException(e); | ||
| } finally { | ||
| if (connection != null) { | ||
| try { | ||
| connection.close(); | ||
| } catch (SQLException e) { | ||
| throw new DataAccessException(e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
rollback 시 NPE 가능성과 예외 마스킹 위험 — 트랜잭션 안정성 보완 필요
- 실패가 getConnection 이전에 발생하면 connection이 null일 수 있습니다. 이 상태에서 rollback을 호출하면 NPE가 발생하고 SQLException만 처리하는 현재 분기에서는 포착되지 않습니다.
- finally에서 close 실패 시 새 예외가 던져져 기존 본질적 예외를 가릴 수 있습니다(마스킹).
- 개선 힌트:
- rollback 호출 전 null 여부를 가드하세요.
- 자원 정리는 실패를 로깅하고 본래 예외를 우선 보존하는 쪽이 운영/디버깅에 유리합니다.
🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/service/UserService.java around lines 36 to
61, guard against calling rollback on a null connection and avoid masking the
original exception when close fails: check if connection != null before calling
connection.rollback(); when rollback or close throws, do not replace the
original exception — either log the secondary SQLException and, if you have a
caught primary exception, add the SQLException as suppressed to the primary (or
log it) before rethrowing the original DataAccessException; similarly, in
finally, on connection.close() failures log the error and add it as suppressed
to the previously caught exception instead of throwing a new exception so the
original root cause is preserved.
| throw new DataAccessException(sqlException); | ||
| } | ||
| throw new DataAccessException(e); | ||
| } finally { |
There was a problem hiding this comment.
try-with-resources를 사용해보는 것이 어떨까요?
close() 호출을 누락하는 실수를 방지할 수 있고, 예외가 발생해도 커넥션이 자동으로 정리되기 때문에 안전하기 때문이에요 !
Comment on lines
+47
to
+52
| try { | ||
| connection.rollback(); | ||
| } catch (SQLException sqlException) { | ||
| throw new DataAccessException(sqlException); | ||
| } | ||
| throw new DataAccessException(e); |
There was a problem hiding this comment.
롤백에서 예외가 발생하는 경우, 롤백 예외(sqlException)가 아닌 원인 예외(e)를 감싸서 던지는 건 어떨까요?
롤백 실패는 2차 실패여서 본질 원인을 가릴 수 있다고 생각해요 !
} catch (Exception e) {
try {
connection.rollback();
} catch (SQLException rollbackEx) {
log.error("ROLLBACK 실패", rollbackEx);
}
throw new DataAccessApiException(e);
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요 밍트, 리뷰 잘부탁드려용.
여러 작업에 동일한 Connection을 주입하는 방식으로 트랜잭션을 구현했습니다.
관심사 분리가 되지 않아 유지 보수성이 떨어지는 문제가 보이지만,
이는 4단계 요구사항인 것 같아 4단계에서 리팩터링해 볼 계획입니다!