[JDBC 라이브러리 구현하기 - 1단계] 쥬니(전정준) 미션 제출합니다. #271
Conversation
| // TODO: 2023/09/26 이 부분.. 질문 드리도록 하겠습니다. | ||
| // if (rs.next()) { | ||
| // throw new IllegalArgumentException("Incorrect Result Size ! Result must be one"); | ||
| // } |
There was a problem hiding this comment.
음 이부분에 대해서 여러가지 생각이 드는데요.
일단 메서드 사용에 대한 논리적인 문제점이 있는 것 같아요.
- USERS 테이블의 account는 유니크할 것이라는 기대로 테스트에서 queryForObject를 호출한다.
- gugu 유저는 매 테스트 추가된다.
그래서 이 라이브러리 레벨이나 DAO 레벨에서 어떤 트랜잭션 처리를 해서 해결할 수 있는 문제같지는 않아 보입니다!
- 테스트에서 gugu를 @beforeeach 말고 @BeforeAll을 사용해서 전체 테스트에서 한번만 insert한다.
- 매 테스트마다 테이블을 초기화하는 sql을 실행시킨다.
- 추가한 컬럼을 delete한다 (DAO에 메서드 추가)
이런 방식으로 테스트들을 격리 시키는 작업이 필요한 것 같습니다!!
There was a problem hiding this comment.
(위 질문과 이어지는 얘기는 아닙니다.)
혹시 2개 이상의 결과일 때 예외를 반환하신 이유가 있을까요?
더 먼저 있는 결과를 반환하는 것도 방법일 것 같은데, 예외를 발생시킨 이유가 있으실까용?
There was a problem hiding this comment.
/... 생략 .../
이런 방식으로 테스트들을 격리 시키는 작업이 필요한 것 같습니다!!
제가 생각을 조금 잘못 했던 것 같요 ㅎㅎ 접근 방식을 테스트 격리 쪽으로 갔어야 겠네요 !
jdbcTemplate.update("TRUNCATE TABLE users RESTART IDENTITY");해당 구문을 @BeforeEach를 사용하는 메서드에 포함하여 데이터 및 PK 값을 초기화 하여 테스트 격리를 수행하였습니다.
혹시 2개 이상의 결과일 때 예외를 반환하신 이유가 있을까요?
더 먼저 있는 결과를 반환하는 것도 방법일 것 같은데, 예외를 발생시킨 이유가 있으실까용?
실제 스프링 프레임워크의 JdbcTemplate을 사용할 때 값이 2개 이상일 때 예외가 발생한 경우가 있었던 것 같아요.
그래서, 예외가 발생하도록 구현했던 것 같아요. 개인적으로, 예외가 발생하는 것이 더 자연스럽고 프로그래머를 위한 일이라고 생각해요 !
루카가 말씀해주신 것 처럼, 더 먼저 조회되는 결과를 반환하게 된다면 무슨 일이 벌어질 수 있을까요 ?
항상, 조회되는 결과의 순서가 동일할까요 ? 데이터를 조회하기 위해 사용하는 파라미터는 그대로인데, 데이터베이스의 상태(레코드 락 등)에 따라 다른 값을 반환한다면 예상치 못한 동작을 할 수 있지 않을까요 ?
There was a problem hiding this comment.
안녕하세요 우주 최강 쥬니
약속한 기한 못지켜서 정말 미안해용🙏🙏🙏🙏
질문 주신 두 내용에 대해서는 코드에 리뷰 남겨놓았습니다.
1번 문제에 대해서는 코드를 주석처리를 해놓으신 것 같아서 주석을 해제해주셔야할 것 같아서 Request Change 남기겠습니다.
2번 문제는 제가 최대한 고민해보고 저만의 답변을 드려봤으니 더 이야기 나눠봐용.
사실 이대로도 충분히 완벽하지만, 조금 더 이야기 나눠보고 주석 같은 내용 지우는 과정이 필요할 것 같아서 Request Change로 남기겠습니다.
추석동안 PR 주시면 최대한 빨리 리뷰 할테니 많은 이야기 나눠보아용❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️
| private static final RowMapper<User> rowMapper = | ||
| rs -> new User( | ||
| rs.getLong("id"), | ||
| rs.getString("account"), | ||
| rs.getString("password"), | ||
| rs.getString("email") | ||
| ); |
There was a problem hiding this comment.
2번 질문에 대한 제 생각을 먼저 말씀드리겠습니다. (어느 코드에 달지 모르겠어서 여기 달게용❤️)
일단 저는 현재 상황에서 RowMapper사용은 불가피하다고 생각하고 있습니다.
그렇게 생각한 이유는 다음과 같습니다.
- JdbcTemplate은
쿼리의 결과를 모른다. - JdbcTemplate은
엔티티의 생성자를 모른다.
위 이유들로 현실적으로 리플렉션을 사용해도 구현은 쉽지 않을 것 같습니다.
그리고 저는 엔티티의 생성을 어디서 하는지는 JdbcTemplate을 역할을 어디까지 볼 것인가의 차이인 것 같습니다.
저는 JdbcTemplate이 Repository 레이어을 감싸고 있는 외부 라이브러리라고 생각합니다.
현재는 엔티티의 생성을 RowMapper에게 시키기고 그를 Dao에서 JdbcTemplate쪽으로 넘겨주게 됩니다.
만약 JdbcTmeplate에게 RowMapper<T> 대신에 Class<T>로 넘겨줘서 그 인스턴스를 생성하려면,
1, 2번에 의해서 쿼리문과 추가적인 엔티티의 생성자의 제약조건이 많이 생길 것 같습니다.
제가 알기론 JPA도 리플렉션을 통해서 기본생성자와 필드의 값을 set해주면서 엔티티 객체를 완성시키는 것으로 알고 있는데요.
그렇다면 외부 라이브러리의 영향이 가장 안쪽에 있어야할 도메인 레이어의 영향이 침투된 것 같습니다.
이것이 JPA에서도 꽤 큰 트레이드 오프라고 생각합니다.
그래서 현재로서는 쥬니처럼 RowMapper를 통해서 객체 생성을 쿼리문의 어떤 결과의 값으로 구성할지 DAO단에서 정의하는 것이 저는 괜찮은 그림이라고 생각합니다!
There was a problem hiding this comment.
그리고 추가적으로 쥬니가 말한 Class type을 매개변수로 넘겨주는 JdbcTemplate 기능들을 찾아보았는데요.
다 getSingleColumnRowMapper라는 내부 메서드를 통해서 각 로우를 해당 타입으로 매핑합니다.
이름에서도 볼 수 있듯이, 해당 기능들은
select count(*) from user;위와 같이 각 로우의 결과가 단일로 나올 때 사용하기 위함입니다.
따라서 User 같은 객체를 매핑하는 용도라기 보다는 Integer.class, String.class같은 단일 컬럼을 매핑하는데 사용되고 있는것으로 보여집니다🥺
There was a problem hiding this comment.
와 정성 미쳤다 !!!!!!!!!! 고마워요 루카 😘
클래스 타입을 받는 것에 대한 오해가 있었군요 ㅎㅎㅎㅎㅎ 이정도 정성이면 리뷰 늦은거 용서해드립니다 ^^
중요한 부분은 아니지만, 답변 중에 궁금한 점이 있어서요 ~!
제가 알기론 JPA도 리플렉션을 통해서 기본생성자와 필드의 값을 set해주면서 엔티티 객체를 완성시키는 것으로 알고 있는데요.
그렇다면 외부 라이브러리의 영향이 가장 안쪽에 있어야할 도메인 레이어의 영향이 침투된 것 같습니다.
이것이 JPA에서도 꽤 큰 트레이드 오프라고 생각합니다.
JPA는 라이브러리인가요 ? 프레임워크인가요 ?
저는 ORM 프레임워크로 알고 있는데, 라이브러리와 프레임워크의 차이는 프로그램의 흐름 제어(생명 주기 등등)에 있다고 생각해요.
JPA가 프레임워크라면, 도메인 레이어에 영향을 주는 것이 문제가 될까요 ?
There was a problem hiding this comment.
쥬니 맞는말씀이세요!!
JPA는 프레임워크라서 흐름을 제어하는 것이고 그러기 위해서 사용하는 것 같습니다!
그것 자체가 어떤 문제!라기 보다는 저희는 JPA를 사용하기 위해서 Domain에 @Entity어노테이션을 붙인다던가 기본생성자를 필수로 둔다던가 하는 JPA의 사용법을 도메인에 적용시키고 JPA의 예외 상황같은 것들이 도메인에게 영향을 끼칠 수 있는 상황이 가장 안쪽에 순수하게 있어야될 도메인에게 외부 프레임워크가 영향을 끼친 트레이드 오프가 있다는 말씀을 드리고 싶었습니다!!
그래서 만약 유저가 RowMapper같이 생성을 설정하지 않고
클래스타입만 가지고 리플렉션을 사용해서 객체를 JdbcTemplate이 알아서 생성하려면,
도메인이 JdbcTemplate이 이미 정해놓은 설정에 의해서 특정 생성자를 꼭 넣어야된다던지 하는 영향이 끼칠 것 같아서 드린말씀이였습니다!!
| private void initializePstmtArgs(PreparedStatement pstmt, Object... args) throws SQLException { | ||
| for (int i = 0; i < args.length; i++) { | ||
| pstmt.setObject(i + 1, args[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
PreparedSatement의 변수 설정하는 중복로직을 줄이기 위해서 선언해주셨네요.👍
근데 이 클래스 내에서 이 중복말고도, try-resource-catch 하는 과정들도 많이 중복되는 것으로 보여지는데요. 혹시 이 중복도 제거할 수 있는지 고려해보섰나요?
사실 제가 고민 중인 내용이긴 한데, 저도 아직 완벽히 확신이 없어서 이 부분은 2단계 때 고려해보고 더 명확하게 전달드리겠습니당❤️
There was a problem hiding this comment.
미션 당시에는 크게 신경쓰지 않았던 것 같아요. (2단계가 리팩토링이길래 ㅎㅎ)
함수형 인터페이스를 어떻게 잘 사용해보면.. 해결할 수 있지 않을까 ? 싶긴 한데, 직접 해봐야 알 것 같네요 !
먼저 2단계를 진행하시다가, 좋은 방법을 발견하시면 공유해주세요 ~!
There was a problem hiding this comment.
저도 시도하다가 실패해가지고 조금 더 고민해봐야될 것 같아요!! ㅋㅋ
2단계에서 같이 고민해보아용~
| // TODO: 2023/09/26 이 부분.. 질문 드리도록 하겠습니다. | ||
| // if (rs.next()) { | ||
| // throw new IllegalArgumentException("Incorrect Result Size ! Result must be one"); | ||
| // } |
There was a problem hiding this comment.
음 이부분에 대해서 여러가지 생각이 드는데요.
일단 메서드 사용에 대한 논리적인 문제점이 있는 것 같아요.
- USERS 테이블의 account는 유니크할 것이라는 기대로 테스트에서 queryForObject를 호출한다.
- gugu 유저는 매 테스트 추가된다.
그래서 이 라이브러리 레벨이나 DAO 레벨에서 어떤 트랜잭션 처리를 해서 해결할 수 있는 문제같지는 않아 보입니다!
- 테스트에서 gugu를 @beforeeach 말고 @BeforeAll을 사용해서 전체 테스트에서 한번만 insert한다.
- 매 테스트마다 테이블을 초기화하는 sql을 실행시킨다.
- 추가한 컬럼을 delete한다 (DAO에 메서드 추가)
이런 방식으로 테스트들을 격리 시키는 작업이 필요한 것 같습니다!!
| // TODO: 2023/09/26 이 부분.. 질문 드리도록 하겠습니다. | ||
| // if (rs.next()) { | ||
| // throw new IllegalArgumentException("Incorrect Result Size ! Result must be one"); | ||
| // } |
There was a problem hiding this comment.
(위 질문과 이어지는 얘기는 아닙니다.)
혹시 2개 이상의 결과일 때 예외를 반환하신 이유가 있을까요?
더 먼저 있는 결과를 반환하는 것도 방법일 것 같은데, 예외를 발생시킨 이유가 있으실까용?
| return result; | ||
| } | ||
|
|
||
| throw new NoSuchElementException("Incorrect Result Size ! Result is null"); |
There was a problem hiding this comment.
(위 예외 리뷰와 이어지는 얘기입니다)
결과가 조회되지 않을 때,
null을 반환하거나 Optional.empty()를 반환하셨을 수도 있으셨을 것 같은데요.
예외를 발생시킨 이유가 있으실까요?
There was a problem hiding this comment.
메서드 명이 'queryForObject` 잖아요 ?
For Object라는 말에서, 해당 메서드는 쿼리의 결과 값으로, 어떠한 단일 객체를 반환하다는 느낌이 강하게 드는 것 같아요.
null은 객체로 보기 힘들 것 같고, Optional은 객체로 볼 수는 있지만, 실제(?) 객체가 있는지 없는지 불분명하다는 의미가 있잖아요 ?
그래서, 명확한 객체를 반환할 수 있을 때에만 정상 동작하는 것이 자연스럽다고 생각했어요.
There was a problem hiding this comment.
좋네요!!
라이브러리에서 무결하게 1개 아니면 반환 안해줘! 이런 기준도 좋은 것 같아요.
저는 사용하는 사람 입장에서 있으면 쓰고 없으면 알아서 핸들링하도록 하는 것이 조금 더 좋을 것 같아서 Optional로 반환하긴 했습니다.
하지만 쥬니가 말씀하신대로 명확한 기준이 있는 것이 더 철저한 라이브러리 인것 같습니다
| private static final Logger log = LoggerFactory.getLogger(UserDao.class); | ||
|
|
||
| private final DataSource dataSource; | ||
|
|
||
| public UserDao(final DataSource dataSource) { | ||
| this.dataSource = dataSource; | ||
| private static final RowMapper<User> rowMapper = |
There was a problem hiding this comment.
Logger와 RowMapper를 둘다 static으로 두셨는데요.
각각 static 변수로 두신 이유가 있으실까요?
There was a problem hiding this comment.
저는 static 키워드를 사용할 때, 두 가지정도만 고민해보고 합당하면 사용하는 편인데요.
- 동시성 이슈가 발생하는가 ?
- 상태를 가지고 있는가 ?
위 두 가지 조건을 고려했을 때, 문제 되는 부분이 없는 것 같다고 판단했어요.
현재 코드에서 static 키워드를 사용하지 않을 이유는 없다는 생각도 들었구요 ㅎㅎ (같은 말인가..?)
There was a problem hiding this comment.
그렇네요.
클래스 변수로 사용해도 딱히 상태가 변화하지 않으니까 굳이 인스턴스 생성할 때마다 생성하지 않아도 돼서 좋은 것 같아요!!
| public interface RowMapper<T> { | ||
|
|
||
| T mapRow(ResultSet resultSet) throws SQLException; | ||
|
|
There was a problem hiding this comment.
(이 뒤로는 아마 다 컨벤션 관련 리뷰일 것 같습니다)
불필요한 개행은 지워보는 것이 어떨까용?
There was a problem hiding this comment.
지금 괜찮을지도 백엔드 팀원들 무시하는 건가요 ?
저희는 중괄호를 클래스 범위에서 사용할 때, 앞 뒤로 한 칸씩 개행을 사용하기로 했어요. ^^
11번 라인에서 중괄호가 닫히기 때문에, 10번 라인에 개행이 있는거죠 !
(사실 아직도 왜 이렇게 하는지 이해 못함.. 팀원들이 하자니까 하는중..)
There was a problem hiding this comment.
🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️
괜찮을 지도 팀의 컨벤션이였군요!
응원합니다.
| @@ -14,4 +21,76 @@ public class JdbcTemplate { | |||
| public JdbcTemplate(final DataSource dataSource) { | |||
| this.dataSource = dataSource; | |||
| } | |||
|
|
|||
| public void update(String sql, Object... args) { | |||
There was a problem hiding this comment.
파라미터에 final이 붙는 경우도 있고 붙지 않는 경우도 있는데, 혹시 기준이 있으실까용?
There was a problem hiding this comment.
실수인데... 아마 기존 코드가 붙어 있었어서...
파라미터에 final, var 키워드 사용하는 것은 개인적으로 좋아하지 않습니다 ! (귀찮거덩요)
제거하지 못한 final, var 키워드는 제거하여 반영하였습니다 !
|
|
||
| List<T> results = new ArrayList<>(); | ||
|
|
||
| while (rs.next()) { | ||
|
|
||
| T result = rowMapper.mapRow(rs); | ||
|
|
||
| results.add(result); | ||
| } | ||
|
|
There was a problem hiding this comment.
혹시 쥬니만의 메서드 내에서 변수의 fianl 여부와 개행의 기준이 있다면 공유해주실 수 있으실까요?
There was a problem hiding this comment.
메서드 내에서의 변수에는 final 절대 안씁니다.. (귀찮아서 ㅎㅎ)
개행의 기준은.. 딱히 정해진 것은 없는데 ! 개행 해보고, 안 해보고, 뭐가 더 예쁜지를 판단하는 것 같아요 ㅋㅋㅋㅋㅋㅋㅋㅋㅋ
그래도 나름의 기준을 큰 틀에서 보면, 변수 선언부 / 로직 수행 / 반환를 개행으로 나누는 타입이에요 !
근데, 위 코드에서는 while문을 시작하는 부분(52행)에 불필요한 개행이 들어가있네요..
이 부분은 제거했습니다 !
There was a problem hiding this comment.
변수 선언부 / 로직 수행 / 반환
이렇게 논리적인 단위로 개행 하는 방법 너무 좋은 것 같습니다!!
이 부분 앞으로 리뷰에 참고하겠습니다!!
|
|
||
| return pstmt; | ||
| } | ||
|
|
There was a problem hiding this comment.
🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️🙇♂️
괜찮을 지도 팀의 컨벤션 다시 한번 응원합니다
|
안녕하세요 루카 !! 리뷰는 늦었지만, 늦은 만큼 정성을 담으셨네요 ㅎㅎ 코멘트 주신 부분에 의견 및 반영 사항 남겨놓았습니다. 확인 부탁드려요 !! |
| private UserDao userDao; | ||
|
|
||
| @BeforeEach | ||
| void setup() { | ||
| DatabasePopulatorUtils.execute(DataSourceConfig.getInstance()); | ||
|
|
||
| jdbcTemplate.update("TRUNCATE TABLE users RESTART IDENTITY"); |
안녕하세요 루카 ~! 이렇게 만나뵙게 되서 영광입니다 히힛 😉
1단계를 우선적으로 구현해보았어요.
미션을 진행하면서, 궁금한 점이 두 가지정도 있었는데요..
두 가지 모두 JdbcTemplate의 queryForObject 메서드 관련입니다 !
1. 예외처리
스프링 부트에서 해당 메서드를 사용할 때, 결과가 없거나 2개 이상일 경우에 오류가 발생하는데요 !
현재는 데이터가 없는 경우에만 예외를 던지고 있는데, 2개 이상인 경우에는 주석처리를 해두었어요.
그 이유는 테스트 코드에서 롤백이 되지 않기 때문에.. 테스트가 통과가 안 되더라구요 ㅠㅠ
이 부분을 어떤 방식으로 해결할 수 있을까요..
2. 파라미터
제가 알기론, queryForObject 메서드의 파라미터 중에 Class 타입이 들어가는 것으로 알고 있는데요.
현재는.. rowMapper를 통해 구현해놓은 상태입니다.
Class 타입을 받아서.. 어떻게 구현할 수 있을지 고민입니다 ㅠㅠ
리플렉션을 사용해서, 필드를 초기화해주는 방법밖에 없을까요 ?