[JDBC 라이브러리 구현하기 - 1단계] 도치(김동흠) 미션 제출합니다. #324

hum02 · 2023-09-29T09:18:31Z

안녕하세요 리오~ 잘 부탁드립니다

Jaeyoung22

안녕하세요 도치! 연휴 잘 보내고 계신가요?!
코드 잘 읽었습니다!
도치의 생각이 궁금한 점이 있어서 몇 가지 커멘트 남겨봤어요.
확인해주시고 답변 주시면 감사하겠습니다!

Jaeyoung22 · 2023-09-29T10:06:00Z

app/src/main/java/com/techcourse/service/UserService.java

@@ -16,7 +16,8 @@ public UserService(final UserDao userDao, final UserHistoryDao userHistoryDao) {
    }

    public User findById(final long id) {
-        return userDao.findById(id);
+        return userDao.findById(id)
+                .orElseThrow(() -> new IllegalArgumentException("해당 아이디의 회원이 존재하지 않습니다."));


Jaeyoung22 · 2023-09-29T10:13:49Z

app/src/main/java/com/techcourse/dao/UserDao.java

+        final String sql = "select id, account, password, email from users";
+
+        return jdbcTemplate.query(sql, rs -> new User(
+                rs.getLong(1),


쿼리의 파라미터 순서대로 매핑을 해주셨군요!
이러면 User의 생성자 파라미터 순서가 달라지거나 쿼리가 달라지면 이 부분도 수정이 되어야할 것 같아요!
더 좋은 방법은 없을까요?

cloumnLabel을 통해 변수명으로 매핑하는 게 더 안정적일 것 같아요!

Jaeyoung22 · 2023-09-29T10:22:19Z

app/src/main/java/com/techcourse/dao/UserDao.java

    }

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


저는 코드의 통일성을 조금 중요시하는데요,
sql문이 어떤 건 타입추론을 하게 되어 있고 어떤 건 타입을 명시해주셔서 조금 읽는데 불편함이 있네요..! 도치는 어떻게 생각하실까요?

기존 코드와 섞여서 통일성이 부족했네요.. 수정하겠습니다

Jaeyoung22 · 2023-09-29T10:23:45Z

jdbc/src/main/java/org/springframework/jdbc/core/RowMapper.java

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


추상 메서드가 한 개이므로 @FunctionalInterface를 붙여주는 건 어떨까요?

Jaeyoung22 · 2023-09-29T10:25:12Z

app/src/main/java/com/techcourse/dao/UserDao.java

+    public Optional<User> findByAccount(final String account) {
+        final String sql = "select id, account, password, email from users where users.account = ?";
+
+        return jdbcTemplate.queryForObject(sql, rs -> new User(


RowMapper가 중복되고 있네요!
위에 언급했던 문제가 생긴다면 해당 메서드들을 전부 수정해야할 것 같아요
한 곳에서 관리해보는 건 어떨까요?

Jaeyoung22 · 2023-09-29T10:36:51Z

jdbc/src/main/java/org/springframework/jdbc/core/StatementCreator.java

+        }
+    }
+
+    private static void setPreparedStatement(PreparedStatement pstmt, Object... arguments) throws SQLException {


여기서 SQLException을 던질 수 밖에 없군요!

넵 모두 SQLException이니 다양한 상황에 대해 어떻게 구분하지? 하는 고민이 있었네요
jdbcTemplate에서는 SQLExceptionTranslator라는 걸 implements해 따로 처리를 하고 있더라고요. 저는 이렇게까지는 못할 것 같지만 간략하게 몇 가지 케이스를 추가해 cutom예외로 처리하려 합니다!

Jaeyoung22 · 2023-09-29T10:37:47Z

jdbc/src/main/java/org/springframework/jdbc/core/StatementCreator.java

+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+
+public class StatementCreator {


Static 메서드들만 존재하는 클래스라면 private 생성자를 추가해주는 건 어떨까요?

Jaeyoung22 · 2023-09-29T10:45:12Z

jdbc/src/main/java/org/springframework/jdbc/core/StatementCreator.java

+            setPreparedStatement(preparedStatement, arguments);
+            return preparedStatement;
+        } catch (SQLException e) {
+            throw new IllegalStateException("creating statement - cannot connect database", e);


여기서도 의도적으로 SQLException에 대해 cannot connect database라고 명시해주셨군요..! 그러면 여기서는 왜 CannotGetJdbcConnectionException이 아닌 IllegalStateException을 반환하셨나요?!

CannotGetJdbcConnectionException은 jdbcTemplate단에서 throw하고 그 아래에서는 다양한 상황에 대해 다양한 에러를 발생시키고자 했는데 메서드에서 SqlException으로 통합해 throw하고 있는 걸 놓쳤습니다.. 감사합니다

굳이 illegalState예외를 발생시켰던 이유는 CannotGetJdbcConnectionException에IllegalStateException을 인자로 받는 생성자가 이미 있길래 이를 활용하고자 했습니다.

Jaeyoung22 · 2023-09-29T10:47:04Z

jdbc/src/main/java/org/springframework/jdbc/core/StatementCreator.java

+    private static void setPreparedStatement(PreparedStatement pstmt, Object... arguments) throws SQLException {
+        for (int argumentIdx = 1; argumentIdx < arguments.length + 1; argumentIdx++) {
+            Object argument = arguments[argumentIdx - 1];
+            if (argument instanceof String) {


indent가 2가 됐네요..! 리팩터링 해보는 건 어떨까요?

Jaeyoung22 · 2023-09-29T10:52:12Z

jdbc/src/main/java/org/springframework/jdbc/core/StatementCreator.java

+        for (int argumentIdx = 1; argumentIdx < arguments.length + 1; argumentIdx++) {
+            Object argument = arguments[argumentIdx - 1];
+            if (argument instanceof String) {
+                argument = ((String) argument).replaceAll("[^a-zA-Z0-9- ]", "");


이러한 방식으로 SQL Injection에 대한 방어를 할 수 있겠네요! 좋습니다!

하지만 정규식을 사용할 때의 단점은 뭐가 있을까요?
그리고 argument로 [email protected]과 [email protected]이 온다면 어떻게 될까요?

꼼꼼하시군요! 그런 예시는 생각하지 못했네요
userdao테스트에도 email까지는 검사하지 않아 지나쳐버렸네요. 꼼꼼한 테스트가 중요하다 느낍니다.

정규식을 사용하면 압축적 표현이 가능하지만 사실 읽기가 쉽지는 않고, 한 글자만으로 표현이 상이하게 바뀌기도 해서 인지하지 못하고 넘어가기도 하는 단점이 있다고 느껴요! 다만 지금의 상황에서는 길어질 코드를 축약할 수 있다는 장점이 있어 사용했습니다.

hum02 · 2023-09-30T08:53:50Z

안녕하세요 리오! 리뷰 잘해주셔서 sqlException에 대해서 좀 더 공부해볼 수 있게 된 것 같아요
Exception translator를 간략하게나마 구현해서 SQLException이 가지는 SQLState를 검사해 좀 더 구체적인 상황에 대해 다른 예외들을 던지도록 수정해보았어요.

jdbcTemplate테스트는 완벽하진 않지만 제 궁금증을 해소할 정도만 작성해보았어요. db에 따라 실제로 sql표준대로 SQLState가 세팅될 지 궁금해서 mock은 하지 않았습니다. 대신 의존성이 추가되었어요...

리오도 명절 잘 보내세요~~

Jaeyoung22

안녕하세요 도치! 리뷰 반영 잘 해주셨네요! 고생하셨습니다~~
구현 잘 해주셔서 merge하겠습니다!
다만 몇 가지 커멘트 남겼는데, 해당 부분에 대한 답변과 반영은 2단계에서 해주시면 좋을 것 같아요!
2단계에서 봬요!

Jaeyoung22 · 2023-10-01T00:55:54Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

+        }
+    }
+
+    private class SqlExceptionTranslator {


Jaeyoung22 · 2023-10-01T00:57:13Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

+        private JdbcException translateException(SQLException e) {
+            String sqlState = e.getSQLState();
+            String classCode = sqlState.substring(0, 2);
+            if (DATA_ACCESS_RESOURCE_FAILURE_CODES.contains(classCode)) {


사소하지만.. 저한테는 classCode.contains(DATA_ACCESS_RESOURCE_FAILURE_CODES) 같은 순서로 쓰는 게 더 잘 이해되는 것 같아서 커멘트 남겨봅니다!

이 부분 해보려했는데 charSequence로 바꾸기 위한 조작이 더 생길 것 같아서 현재대로 유지하겠습니다!

Jaeyoung22 · 2023-10-01T00:59:46Z

jdbc/src/main/java/org/springframework/jdbc/core/StatementCreator.java

+    private static Object filterArgument(Object argument) {
+        Object filtered = argument;
+        if (argument instanceof String) {
+            filtered = ((String) argument).replaceAll("[^a-zA-Z0-9-@.]", EMPTY_STRING);


' 을 제외한 다른 특수문자들은 허용되는 것이 좋을 것 같기도 한데..
이 부분은 조금 더 고민을 해볼까요?
SQL Injection을 막으면서 정책에 유연하게 대처할 수 있는 다른 방법도 있을 것 같아요!

특수문자들을 좀 더 추가해봤어요!
그런데 드는 생각이 jdbc는 들어온 sql을 잘 실행하도록 하는 게 목적이지 sql injection을 막는 건 jdbc의 책임이 아닌 듯하네요..
사실 이건 sql을 실행하는 것보다 더 앞단에서 구현해야할 책임이 맞는 것 같아요!
jdbc는 받은 sql이 어디서 왔는지 알 수도 없고 알 필요도 없어보이네요. sql문법의 검사는 db단에서 하니 해줄필요가 없구요

Jaeyoung22 · 2023-10-01T01:03:21Z

jdbc/src/test/java/nextstep/jdbc/JdbcTemplateTest.java

+        jdbcTemplate.update("delete from users;");
+    }
+
+    @Test


다른 메서드에 대해서도 테스트를 작성해보는 건 어떨까요?!

kang-hyungu and others added 3 commits September 26, 2023 13:27

패키지 위치 변경 및 코드 정리

06379e5

feat:jdbcTemplate으로 userDao 구현

ccd9c82

refactor:예외처리 구체화

c062a39

hum02 changed the title ~~JDBC 라이브러리 구현하기 - 1단계] 도치(김동흠) 미션 제출합니다.~~ [JDBC 라이브러리 구현하기 - 1단계] 도치(김동흠) 미션 제출합니다. Sep 29, 2023

Jaeyoung22 self-requested a review September 29, 2023 10:03

Jaeyoung22 requested changes Sep 29, 2023

View reviewed changes

hum02 added 4 commits September 30, 2023 14:54

refactor(UserDao):rowmapper와 타입 수정해 가독성 개선

87c2e97

fix:유저의 이메일 정보 일부기호를 sql문 생성시 누락시키는 오류 수정

697727b

refactor:sql문의 인자 필터링 메서드 분리

5235221

feat:SqlException의 예외를 구체화한 jdbcException으로 변환하는 기능 구현

b269c25

refactor:jdbcExcpetion 계층화

2d5f8e5

Jaeyoung22 approved these changes Oct 1, 2023

View reviewed changes

Jaeyoung22 merged commit 0e42841 into woowacourse:hum02 Oct 1, 2023
1 check failed

hum02 mentioned this pull request Oct 5, 2023

[JDBC 라이브러리 구현하기 - 2단계] 도치(김동흠) 미션 제출합니다. #417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JDBC 라이브러리 구현하기 - 1단계] 도치(김동흠) 미션 제출합니다. #324

[JDBC 라이브러리 구현하기 - 1단계] 도치(김동흠) 미션 제출합니다. #324

hum02 commented Sep 29, 2023

Jaeyoung22 left a comment

Jaeyoung22 Sep 29, 2023

Jaeyoung22 Sep 29, 2023

hum02 Sep 30, 2023

Jaeyoung22 Sep 29, 2023

hum02 Sep 30, 2023

Jaeyoung22 Sep 29, 2023

Jaeyoung22 Sep 29, 2023

Jaeyoung22 Sep 29, 2023

hum02 Sep 30, 2023 •

edited

Loading

Jaeyoung22 Sep 29, 2023

Jaeyoung22 Sep 29, 2023

hum02 Sep 30, 2023

Jaeyoung22 Sep 29, 2023

Jaeyoung22 Sep 29, 2023

hum02 Sep 30, 2023

hum02 commented Sep 30, 2023 •

edited

Loading

Jaeyoung22 left a comment

Jaeyoung22 Oct 1, 2023

Jaeyoung22 Oct 1, 2023

hum02 Oct 4, 2023

Jaeyoung22 Oct 1, 2023

hum02 Oct 4, 2023 •

edited

Loading

Jaeyoung22 Oct 1, 2023


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

[JDBC 라이브러리 구현하기 - 1단계] 도치(김동흠) 미션 제출합니다. #324

[JDBC 라이브러리 구현하기 - 1단계] 도치(김동흠) 미션 제출합니다. #324

Conversation

hum02 commented Sep 29, 2023

Jaeyoung22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hum02 Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hum02 commented Sep 30, 2023 • edited Loading

Jaeyoung22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hum02 Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hum02 Sep 30, 2023 •

edited

Loading

hum02 commented Sep 30, 2023 •

edited

Loading

hum02 Oct 4, 2023 •

edited

Loading