Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JDBC 라이브러리 구현하기 - 4단계] 디투(박정훈) 미션 제출합니다. #593

Merged
merged 6 commits into from
Oct 11, 2023

Conversation

shb03323
Copy link

@shb03323 shb03323 commented Oct 9, 2023

홍박사님 안녕하세요.

생각보다 오래 걸렸네요. 이전 코드에서 추가된 것 위주로 말씀드릴게요.

  • TransactionSynchronizedManager
    이 객체는 트랜잭션들을 바인딩, 언바인딩, 혹은 트랜잭션의 리소스를 들고오는 역할을 합니다. 원래 이렇게 3가지 기능만 하다가 트랜잭션 시작 유무를 확인하기 위해서 actualTransactionActive를 만들었습니다.

  • TransactionTemplate
    트랜잭션을 시작과 끝을 관리해주는 객체입니다.

  • TransactionCallback
    트랜잭션으로 어떤 작업을 하는지 함수형 인터페이스로 전달해줍니다.

@shb03323 shb03323 requested a review from hong-sile October 9, 2023 13:12
@shb03323 shb03323 self-assigned this Oct 9, 2023
Copy link
Member

@hong-sile hong-sile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 디투! 전반적으로 잘 구현해주셨군요.

레벨 4 미션이 크기도 했고, 제가 물어보고 싶은 부분도 있어서 한 번 RC 날립니다. 확인해주세요

} catch (SQLException e) {
System.out.println(e.getMessage());
rollback(connection);
throw new DataAccessException("transaction 설정에 오류가 발생했습니다.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQL Chekced Exception을 Unchecked로 바꿔서 Throw해주는 군요.
이럴 경우에, DataAccessException 에 메시지를 constant 하게 주는 것보다 SQlException e도 같이 넘겨보는건 어때요?

필수 반영사항은 아닙니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이야 놓쳤네요. 감사합니당. 프린트도 지우고 로그로 바꿨어요.

try (final PreparedStatement ps = createPreparedStatement(connection, sql, args)) {
return ps.executeUpdate();
} catch (SQLException e) {
log.error(e.getMessage());
throw new DataAccessException(e);
} finally {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

트랜잭션 활성화 여부에 따라서, connection을 close 해주는 군요 잘 짜주신 것 같아요,

final Connection connection = DataSourceUtils.getConnection(dataSource);
try {
connection.setAutoCommit(false);
TransactionSynchronizationManager.setActualTransactionActiveTrue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connection.setAutoCommit(false)랑 setActualTransactionActiveTrue안에 넣어보는건 어떨까요?
하나로 묶을 수 있는 작업인 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TransactionSynchronizationManager는 트랜잭션 스레드를 관리하는 객체라고 생각해요. autoCommit을 false로 바꾸는 건 TransactionTemplate의 execute 메소드가 하는 것이 더 어울린다고 생각해서요. connection 관련 작업들을 처리해주고 있어서 외부에서 처리하면 헷갈릴 것 같아요.
또, 정리해주어야하는 자원이기 때문에 최대한 한 파일에서 실행하는 것이 편할 것 같습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

납득되네요. TransactionSyncronizationManager가 jdbcTemplate 같은 곳에서도 쓰이긴 하니, setAutoCommit 같은 tx의 시작을 의미하는 메서드라인은 txTemlate에 있는게 낫겠네요

}

public void execute(final TransactionCallback transactionCallback) {
final Connection connection = DataSourceUtils.getConnection(dataSource);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 TransactionTemplate 안에서 TransactionTemplate이 사용되면 어떻게 될까요?

문제가 생길수도 있을 것 같아요. 한 번 확인해보시면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

감사합니다.


@BeforeEach
void setUp() {
dataSource = Mockito.mock(DataSource.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 요청 드린 테스트를 짜주셨군요.
아 DataSourceConfig가 없어서, Mockito.mock을 써주신거군요.

지금 TxSyncManager의 경우에는 단순하게 dataSource와 connection을 저장, 조회만 해서 mock으로만 해도 충분한 것 같아요.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

App에 있는 DataSourceConfig를 jdbc test 패키지에 넣어서 사용하는 방법도 있을 것 같고,
study 학습테스트에 있던 것 처럼 간단하게 datasource를 만들어 사용하는 방법도 있을 것 같네요.

private final UserService userService;
private final TransactionTemplate transactionTemplate;

public TxUserService(final UserService userService, final TransactionTemplate transactionTemplate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TxUserService에서 UserService 인터페이스로 받는 군요.

저는 실수 할 수도 있어서 AppUserService가 적당하다고 생각해요. UserService라는 인터페이스의 경우는 추가적인 다른 UserService가 생길일도 없을 것 같거든요.

디투의 생각은 어떤가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인정합니다.

JdbcTemplate jdbcTemplate;

@BeforeEach
void setUp() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jdbcTemplate의 경우에는 진짜 DataSource가 필요해요.

단순히 mock으로는 해결이 안 되죠.

이런 경우에 테스트만을 위한 클래스를 테스트 패키지에서 정의해서 사용하는 방식이 있을 것 같습니다.

(이제 테스트를 짜주실 필욘 없습니다. 짜기 위해 어떤 고민을 헀는가가 궁금했어요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의존성에 h2를 추가하는 것이 걸렸습니다. 모킹으로 생각해보았는데 DataSource와 Connection 메소드들을 다 만들어준다는 것이 리소스가 너무 많이 드는 작업이라고 생각했어요.

Copy link
Member

@hong-sile hong-sile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

디투 제가 요청한 것들도 다 적용해주셔서 좋았습니다.

재밌게 리뷰 진행했어요. 이만 Approve 하고 머지하겠습니다.

@hong-sile hong-sile merged commit 183b8a6 into woowacourse:shb03323 Oct 11, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants