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 라이브러리 구현하기 - 1단계] 쥬니(전정준) 미션 제출합니다. #271

Merged
merged 6 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 48 additions & 97 deletions app/src/main/java/com/techcourse/dao/UserDao.java
Original file line number Diff line number Diff line change
@@ -1,121 +1,72 @@
package com.techcourse.dao;

import com.techcourse.domain.User;
import org.springframework.jdbc.core.JdbcTemplate;
import java.util.List;
import javax.sql.DataSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.sql.DataSource;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.List;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.RowMapper;

public class UserDao {

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 =
Comment on lines 13 to +14

Choose a reason for hiding this comment

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

Logger와 RowMapper를 둘다 static으로 두셨는데요.
각각 static 변수로 두신 이유가 있으실까요?

Copy link
Author

Choose a reason for hiding this comment

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

저는 static 키워드를 사용할 때, 두 가지정도만 고민해보고 합당하면 사용하는 편인데요.

  1. 동시성 이슈가 발생하는가 ?
  2. 상태를 가지고 있는가 ?

위 두 가지 조건을 고려했을 때, 문제 되는 부분이 없는 것 같다고 판단했어요.
현재 코드에서 static 키워드를 사용하지 않을 이유는 없다는 생각도 들었구요 ㅎㅎ (같은 말인가..?)

Choose a reason for hiding this comment

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

그렇네요.
클래스 변수로 사용해도 딱히 상태가 변화하지 않으니까 굳이 인스턴스 생성할 때마다 생성하지 않아도 돼서 좋은 것 같아요!!

rs -> new User(
rs.getLong("id"),
rs.getString("account"),
rs.getString("password"),
rs.getString("email")
);
Comment on lines +14 to +20

Choose a reason for hiding this comment

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

2번 질문에 대한 제 생각을 먼저 말씀드리겠습니다. (어느 코드에 달지 모르겠어서 여기 달게용❤️)
일단 저는 현재 상황에서 RowMapper사용은 불가피하다고 생각하고 있습니다.
그렇게 생각한 이유는 다음과 같습니다.

  1. JdbcTemplate은 쿼리의 결과를 모른다.
  2. JdbcTemplate은 엔티티의 생성자를 모른다.

위 이유들로 현실적으로 리플렉션을 사용해도 구현은 쉽지 않을 것 같습니다.

그리고 저는 엔티티의 생성을 어디서 하는지는 JdbcTemplate을 역할을 어디까지 볼 것인가의 차이인 것 같습니다.
저는 JdbcTemplate이 Repository 레이어을 감싸고 있는 외부 라이브러리라고 생각합니다.

현재는 엔티티의 생성을 RowMapper에게 시키기고 그를 Dao에서 JdbcTemplate쪽으로 넘겨주게 됩니다.
만약 JdbcTmeplate에게 RowMapper<T> 대신에 Class<T>로 넘겨줘서 그 인스턴스를 생성하려면,
1, 2번에 의해서 쿼리문과 추가적인 엔티티의 생성자의 제약조건이 많이 생길 것 같습니다.
제가 알기론 JPA도 리플렉션을 통해서 기본생성자와 필드의 값을 set해주면서 엔티티 객체를 완성시키는 것으로 알고 있는데요.
그렇다면 외부 라이브러리의 영향이 가장 안쪽에 있어야할 도메인 레이어의 영향이 침투된 것 같습니다.
이것이 JPA에서도 꽤 큰 트레이드 오프라고 생각합니다.

그래서 현재로서는 쥬니처럼 RowMapper를 통해서 객체 생성을 쿼리문의 어떤 결과의 값으로 구성할지 DAO단에서 정의하는 것이 저는 괜찮은 그림이라고 생각합니다!

Choose a reason for hiding this comment

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

그리고 추가적으로 쥬니가 말한 Class type을 매개변수로 넘겨주는 JdbcTemplate 기능들을 찾아보았는데요.
image
다 getSingleColumnRowMapper라는 내부 메서드를 통해서 각 로우를 해당 타입으로 매핑합니다.
이름에서도 볼 수 있듯이, 해당 기능들은

select count(*) from user;

위와 같이 각 로우의 결과가 단일로 나올 때 사용하기 위함입니다.
따라서 User 같은 객체를 매핑하는 용도라기 보다는 Integer.class, String.class같은 단일 컬럼을 매핑하는데 사용되고 있는것으로 보여집니다🥺

Copy link
Author

Choose a reason for hiding this comment

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

와 정성 미쳤다 !!!!!!!!!! 고마워요 루카 😘

클래스 타입을 받는 것에 대한 오해가 있었군요 ㅎㅎㅎㅎㅎ 이정도 정성이면 리뷰 늦은거 용서해드립니다 ^^

중요한 부분은 아니지만, 답변 중에 궁금한 점이 있어서요 ~!

제가 알기론 JPA도 리플렉션을 통해서 기본생성자와 필드의 값을 set해주면서 엔티티 객체를 완성시키는 것으로 알고 있는데요.
그렇다면 외부 라이브러리의 영향이 가장 안쪽에 있어야할 도메인 레이어의 영향이 침투된 것 같습니다.
이것이 JPA에서도 꽤 큰 트레이드 오프라고 생각합니다.

JPA는 라이브러리인가요 ? 프레임워크인가요 ?
저는 ORM 프레임워크로 알고 있는데, 라이브러리와 프레임워크의 차이는 프로그램의 흐름 제어(생명 주기 등등)에 있다고 생각해요.
JPA가 프레임워크라면, 도메인 레이어에 영향을 주는 것이 문제가 될까요 ?

Choose a reason for hiding this comment

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

쥬니 맞는말씀이세요!!
JPA는 프레임워크라서 흐름을 제어하는 것이고 그러기 위해서 사용하는 것 같습니다!
그것 자체가 어떤 문제!라기 보다는 저희는 JPA를 사용하기 위해서 Domain에 @Entity어노테이션을 붙인다던가 기본생성자를 필수로 둔다던가 하는 JPA의 사용법을 도메인에 적용시키고 JPA의 예외 상황같은 것들이 도메인에게 영향을 끼칠 수 있는 상황이 가장 안쪽에 순수하게 있어야될 도메인에게 외부 프레임워크가 영향을 끼친 트레이드 오프가 있다는 말씀을 드리고 싶었습니다!!

그래서 만약 유저가 RowMapper같이 생성을 설정하지 않고
클래스타입만 가지고 리플렉션을 사용해서 객체를 JdbcTemplate이 알아서 생성하려면,
도메인이 JdbcTemplate이 이미 정해놓은 설정에 의해서 특정 생성자를 꼭 넣어야된다던지 하는 영향이 끼칠 것 같아서 드린말씀이였습니다!!


private final JdbcTemplate jdbcTemplate;

public UserDao(DataSource dataSource) {
this.jdbcTemplate = new JdbcTemplate(dataSource);
}

public UserDao(final JdbcTemplate jdbcTemplate) {
this.dataSource = null;
public UserDao(JdbcTemplate jdbcTemplate) {
this.jdbcTemplate = jdbcTemplate;
}

public void insert(final User user) {
final var sql = "insert into users (account, password, email) values (?, ?, ?)";

Connection conn = null;
PreparedStatement pstmt = null;
try {
conn = dataSource.getConnection();
pstmt = conn.prepareStatement(sql);

log.debug("query : {}", sql);

pstmt.setString(1, user.getAccount());
pstmt.setString(2, user.getPassword());
pstmt.setString(3, user.getEmail());
pstmt.executeUpdate();
} catch (SQLException e) {
log.error(e.getMessage(), e);
throw new RuntimeException(e);
} finally {
try {
if (pstmt != null) {
pstmt.close();
}
} catch (SQLException ignored) {}

try {
if (conn != null) {
conn.close();
}
} catch (SQLException ignored) {}
}
public void insert(User user) {
String sql = "insert into users (account, password, email) values (?, ?, ?)";

log.debug("query : {}", sql);

jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail());
}

public void update(final User user) {
// todo
public void update(User user) {
String sql = "UPDATE users SET account = ?, password = ?, email = ? WHERE id = ?";

log.debug("query : {}", sql);

jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail(), user.getId());
}

public List<User> findAll() {
// todo
return null;
String sql = "SELECT * from users";

log.debug("query : {}", sql);

return jdbcTemplate.query(sql, rowMapper);
}

public User findById(final Long id) {
final var sql = "select id, account, password, email from users where id = ?";

Connection conn = null;
PreparedStatement pstmt = null;
ResultSet rs = null;
try {
conn = dataSource.getConnection();
pstmt = conn.prepareStatement(sql);
pstmt.setLong(1, id);
rs = pstmt.executeQuery();

log.debug("query : {}", sql);

if (rs.next()) {
return new User(
rs.getLong(1),
rs.getString(2),
rs.getString(3),
rs.getString(4));
}
return null;
} catch (SQLException e) {
log.error(e.getMessage(), e);
throw new RuntimeException(e);
} finally {
try {
if (rs != null) {
rs.close();
}
} catch (SQLException ignored) {}

try {
if (pstmt != null) {
pstmt.close();
}
} catch (SQLException ignored) {}

try {
if (conn != null) {
conn.close();
}
} catch (SQLException ignored) {}
}
public User findById(Long id) {
String sql = "select * from users where id = ?";

log.debug("query : {}", sql);

return jdbcTemplate.queryForObject(sql, rowMapper, id);
}

public User findByAccount(final String account) {
// todo
return null;
public User findByAccount(String account) {
String sql = "select * from users where account = ?";

log.debug("query : {}", sql);

return jdbcTemplate.queryForObject(sql, rowMapper, account);
}

}
32 changes: 19 additions & 13 deletions app/src/test/java/com/techcourse/dao/UserDaoTest.java
Original file line number Diff line number Diff line change
@@ -1,68 +1,74 @@
package com.techcourse.dao;

import static org.assertj.core.api.Assertions.assertThat;

import com.techcourse.config.DataSourceConfig;
import com.techcourse.domain.User;
import com.techcourse.support.jdbc.init.DatabasePopulatorUtils;
import java.util.List;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;
import org.springframework.jdbc.core.JdbcTemplate;

class UserDaoTest {

private final JdbcTemplate jdbcTemplate = new JdbcTemplate(DataSourceConfig.getInstance());
private UserDao userDao;

@BeforeEach
void setup() {
DatabasePopulatorUtils.execute(DataSourceConfig.getInstance());

jdbcTemplate.update("TRUNCATE TABLE users RESTART IDENTITY");

Choose a reason for hiding this comment

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

👍

userDao = new UserDao(DataSourceConfig.getInstance());
final var user = new User("gugu", "password", "[email protected]");

User user = new User("gugu", "password", "[email protected]");
userDao.insert(user);
}

@Test
void findAll() {
final var users = userDao.findAll();
List<User> users = userDao.findAll();

assertThat(users).isNotEmpty();
}

@Test
void findById() {
final var user = userDao.findById(1L);
userDao.insert(new User("gugu", "password", "[email protected]"));
User user = userDao.findById(1L);

assertThat(user.getAccount()).isEqualTo("gugu");
}

@Test
void findByAccount() {
final var account = "gugu";
final var user = userDao.findByAccount(account);
String account = "gugu";
User user = userDao.findByAccount(account);

assertThat(user.getAccount()).isEqualTo(account);
}

@Test
void insert() {
final var account = "insert-gugu";
final var user = new User(account, "password", "[email protected]");
String account = "insert-gugu";
User user = new User(account, "password", "[email protected]");
userDao.insert(user);

final var actual = userDao.findById(2L);
User actual = userDao.findById(2L);

assertThat(actual.getAccount()).isEqualTo(account);
}

@Test
void update() {
final var newPassword = "password99";
final var user = userDao.findById(1L);
String newPassword = "password99";
User user = userDao.findById(1L);
user.changePassword(newPassword);

userDao.update(user);

final var actual = userDao.findById(1L);
User actual = userDao.findById(1L);

assertThat(actual.getPassword()).isEqualTo(newPassword);
}
Expand Down
82 changes: 79 additions & 3 deletions jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java
Original file line number Diff line number Diff line change
@@ -1,17 +1,93 @@
package org.springframework.jdbc.core;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;
import java.util.NoSuchElementException;
import javax.sql.DataSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.sql.DataSource;

public class JdbcTemplate {

private static final Logger log = LoggerFactory.getLogger(JdbcTemplate.class);

private final DataSource dataSource;

public JdbcTemplate(final DataSource dataSource) {
public JdbcTemplate(DataSource dataSource) {
this.dataSource = dataSource;
}

public void update(String sql, Object... args) {

Choose a reason for hiding this comment

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

파라미터에 final이 붙는 경우도 있고 붙지 않는 경우도 있는데, 혹시 기준이 있으실까용?

Copy link
Author

Choose a reason for hiding this comment

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

실수인데... 아마 기존 코드가 붙어 있었어서...

파라미터에 final, var 키워드 사용하는 것은 개인적으로 좋아하지 않습니다 ! (귀찮거덩요)

제거하지 못한 final, var 키워드는 제거하여 반영하였습니다 !

try (Connection conn = dataSource.getConnection();
PreparedStatement pstmt = conn.prepareStatement(sql)) {

initializePstmtArgs(pstmt, args);

pstmt.executeUpdate();
} catch (SQLException e) {
log.error(e.getMessage(), e);
throw new RuntimeException(e);
}
}

private void initializePstmtArgs(PreparedStatement pstmt, Object... args) throws SQLException {
for (int i = 0; i < args.length; i++) {
pstmt.setObject(i + 1, args[i]);
}
}
Comment on lines +37 to +41

Choose a reason for hiding this comment

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

PreparedSatement의 변수 설정하는 중복로직을 줄이기 위해서 선언해주셨네요.👍

근데 이 클래스 내에서 이 중복말고도, try-resource-catch 하는 과정들도 많이 중복되는 것으로 보여지는데요. 혹시 이 중복도 제거할 수 있는지 고려해보섰나요?
사실 제가 고민 중인 내용이긴 한데, 저도 아직 완벽히 확신이 없어서 이 부분은 2단계 때 고려해보고 더 명확하게 전달드리겠습니당❤️

Copy link
Author

Choose a reason for hiding this comment

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

미션 당시에는 크게 신경쓰지 않았던 것 같아요. (2단계가 리팩토링이길래 ㅎㅎ)

함수형 인터페이스를 어떻게 잘 사용해보면.. 해결할 수 있지 않을까 ? 싶긴 한데, 직접 해봐야 알 것 같네요 !

먼저 2단계를 진행하시다가, 좋은 방법을 발견하시면 공유해주세요 ~!

Choose a reason for hiding this comment

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

저도 시도하다가 실패해가지고 조금 더 고민해봐야될 것 같아요!! ㅋㅋ
2단계에서 같이 고민해보아용~


public <T> List<T> query(String sql, RowMapper<T> rowMapper) {
try (Connection conn = dataSource.getConnection();
PreparedStatement pstmt = conn.prepareStatement(sql);
ResultSet rs = pstmt.executeQuery()) {

List<T> results = new ArrayList<>();

while (rs.next()) {
T result = rowMapper.mapRow(rs);

results.add(result);
}

return results;
} catch (SQLException e) {
log.error(e.getMessage(), e);
throw new RuntimeException(e);
}
}

public <T> T queryForObject(String sql, RowMapper<T> rowMapper, Object... args) {
try (Connection conn = dataSource.getConnection();
PreparedStatement pstmt = getQueryPstmtForObject(sql, conn, args);
ResultSet rs = pstmt.executeQuery()) {

if (rs.next()) {
T result = rowMapper.mapRow(rs);

if (rs.next()) {
throw new IllegalArgumentException("Incorrect Result Size ! Result must be one");
}

return result;
}

throw new NoSuchElementException("Incorrect Result Size ! Result is null");
Copy link

@dooboocookie dooboocookie Sep 27, 2023

Choose a reason for hiding this comment

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

(위 예외 리뷰와 이어지는 얘기입니다)
결과가 조회되지 않을 때,
null을 반환하거나 Optional.empty()를 반환하셨을 수도 있으셨을 것 같은데요.
예외를 발생시킨 이유가 있으실까요?

Copy link
Author

Choose a reason for hiding this comment

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

메서드 명이 'queryForObject` 잖아요 ?
For Object라는 말에서, 해당 메서드는 쿼리의 결과 값으로, 어떠한 단일 객체를 반환하다는 느낌이 강하게 드는 것 같아요.

null은 객체로 보기 힘들 것 같고, Optional은 객체로 볼 수는 있지만, 실제(?) 객체가 있는지 없는지 불분명하다는 의미가 있잖아요 ?
그래서, 명확한 객체를 반환할 수 있을 때에만 정상 동작하는 것이 자연스럽다고 생각했어요.

Choose a reason for hiding this comment

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

좋네요!!
라이브러리에서 무결하게 1개 아니면 반환 안해줘! 이런 기준도 좋은 것 같아요.
저는 사용하는 사람 입장에서 있으면 쓰고 없으면 알아서 핸들링하도록 하는 것이 조금 더 좋을 것 같아서 Optional로 반환하긴 했습니다.
하지만 쥬니가 말씀하신대로 명확한 기준이 있는 것이 더 철저한 라이브러리 인것 같습니다

} catch (SQLException e) {
log.error(e.getMessage(), e);
throw new RuntimeException(e);
}
}

private PreparedStatement getQueryPstmtForObject(String sql, Connection conn, Object... args) throws SQLException {
PreparedStatement pstmt = conn.prepareStatement(sql);

initializePstmtArgs(pstmt, args);

return pstmt;
}

Choose a reason for hiding this comment

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

불필요한 개행으로 보여집니다!!

Copy link
Author

Choose a reason for hiding this comment

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

아니이 팀 컨벤션이라고여 !!!!

Choose a reason for hiding this comment

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

🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️
괜찮을 지도 팀의 컨벤션 다시 한번 응원합니다

}
11 changes: 11 additions & 0 deletions jdbc/src/main/java/org/springframework/jdbc/core/RowMapper.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package org.springframework.jdbc.core;

import java.sql.ResultSet;
import java.sql.SQLException;

@FunctionalInterface
public interface RowMapper<T> {

T mapRow(ResultSet resultSet) throws SQLException;

Choose a reason for hiding this comment

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

(이 뒤로는 아마 다 컨벤션 관련 리뷰일 것 같습니다)

불필요한 개행은 지워보는 것이 어떨까용?

Copy link
Author

Choose a reason for hiding this comment

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

지금 괜찮을지도 백엔드 팀원들 무시하는 건가요 ?
저희는 중괄호를 클래스 범위에서 사용할 때, 앞 뒤로 한 칸씩 개행을 사용하기로 했어요. ^^

11번 라인에서 중괄호가 닫히기 때문에, 10번 라인에 개행이 있는거죠 !
(사실 아직도 왜 이렇게 하는지 이해 못함.. 팀원들이 하자니까 하는중..)

Choose a reason for hiding this comment

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

🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️
괜찮을 지도 팀의 컨벤션이였군요!
응원합니다.

}
Loading