Conversation
- ConnectionManager 인터페이스 정의 (향후 커넥션 풀 확장 가능성 고려) - H2 db에 대한 DriverManager를 통해 Connection을 생성하는 H2DbManager 클래스 개발 - Database 관련 설정을 관장하는 DatabaseConfig 클래스 개발
- DatabaseConfig에 DAO 클래스를 등록하는 기능 추가 - 등록된 DAO 클래스들에 대한 DDL을 자동생성하는 DdlGenerator 클래스 개발
- DAO에 대한 CRU 쿼리를 생성해서 Connection에 보내는 CrudRepository 개발 - Delete의 경우 향후 추가 예정
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
종합 검토
리뷰 포인트인 DDL 자동 생성과 CRU 쿼리 자동 생성 기능 중심으로 검토했습니다.
주요 이슈
1. SQL 인젝션 및 보안
findByColumn()에서 컬럼명이 검증 없이 쿼리에 포함되어 조작될 위험이 있습니다.- DDL 생성 시에는 고정된 엔티티 클래스만 사용하므로 현재는 안전하지만, 향후 동적 기능 추가 시 주의가 필요합니다.
2. 구성 및 리소스 관리
DatabaseConfig의 DB URL, 사용자명이 하드코딩되어 있어 환경별 관리가 불가능합니다. 환경 변수/설정 파일 도입이 필수입니다.DatabaseConfig에서 새AppConfig인스턴스를 생성하면 싱글톤이 깨질 수 있습니다.- 매 쿼리마다 새로운 Connection을 생성하면서 리소스 누적 위험이 있습니다(현재 try-with-resources로 사용되므로 괜찮으나 향후 개선 필요).
3. 예외 처리 및 타입 안전성
SQLException발생 시 원본 정보를 버려서 디버깅이 어려워집니다.mapRow()에서 매개변수 없는 생성자를 요구하므로, 이를 만족하지 않는 엔티티는 런타임 에러가 발생합니다. 문서화 또는 더 유연한 구현이 필요합니다.
정상 작동하는 부분
- User 클래스를 User.id 기반으로 변경하고 관련 코드를 일관성 있게 수정했습니다.
- CRU 쿼리 생성 로직은 전반적으로 정확하고, id 제외 및 자동 증가 처리도 올바릅니다.
- 리플렉션 활용으로 제네릭하게 여러 엔티티를 지원하는 구조는 잘 설계되었습니다.
| String tableName = toTableName(entityClass); | ||
| String ddl = buildDdlForEntity(entityClass, tableName); | ||
| log.info("DDL for {}:\n{}", tableName, ddl); | ||
| stmt.execute(ddl); | ||
| } | ||
|
|
There was a problem hiding this comment.
SQL 인젝션 취약점: tableName이 사용자 입력에서 영향을 받을 수 있는 경우 위험합니다. 현재는 고정된 ENTITY_CLASSES에서만 가져오므로 괜찮지만, 향후 동적 테이블명 지원 시 주의가 필요합니다. 또한 stmt.execute(ddl)은 여러 SQL문을 한 번에 실행할 수 있어 DDL이 예상과 다르게 파싱될 수 있습니다.
| } catch (SQLException e) { | ||
| throw new RuntimeException("DDL 생성/실행 중 오류", e); | ||
| } |
There was a problem hiding this comment.
예외 처리 미흡: SQLException 발생 시 원본 예외 정보(원인, SQL 상태 코드 등)를 버리고 일반적인 RuntimeException으로 변환합니다. 디버깅을 어렵게 만들므로 throw new RuntimeException("DDL 생성/실행 중 오류", e);를 유지하되, 더 자세한 context 정보(현재 테이블명, 시도한 DDL 문)를 포함하면 좋습니다.
| } | ||
|
|
||
| public List<T> findAll() { | ||
| String sql = "SELECT * FROM " + tableName; | ||
|
|
||
| try (Connection conn = connectionManager.getConnection(); | ||
| PreparedStatement pstmt = conn.prepareStatement(sql); | ||
| ResultSet resultSet = pstmt.executeQuery()) { | ||
|
|
||
| List<T> result = new ArrayList<>(); | ||
| while (resultSet.next()) { | ||
| result.add(mapRow(resultSet)); | ||
| } | ||
| log.info("{} queried({} results): {}", tableName, result.size(), sql); | ||
| return result; | ||
|
|
||
| } catch (SQLException e) { | ||
| throw new ErrorException("엔티티 전체 조회 중 오류", e); | ||
| } | ||
| } | ||
|
|
||
| public List<T> findByColumn(String columnName, Object value) { | ||
| String sql = "SELECT * FROM " + tableName + " WHERE " + columnName + " = ?"; | ||
|
|
||
| try (Connection conn = connectionManager.getConnection(); |
There was a problem hiding this comment.
SQL 인젝션 위험: findByColumn() 메서드에서 columnName 파라미터가 검증 없이 SQL에 직접 포함됩니다. 사용자가 통제하는 컬럼명이라면 쿼리가 왜곡될 수 있습니다. 화이트리스트 검증이나 애노테이션을 통한 컬럼명 사전 등록을 고려하세요.
| } else { | ||
| pstmt.setObject(index, value); | ||
| } | ||
| } | ||
|
|
||
| private void setId(T entity, Long id) { | ||
| try { | ||
| Field idField = type.getDeclaredField("id"); | ||
| idField.setAccessible(true); | ||
| idField.set(entity, id); | ||
| } catch (NoSuchFieldException | IllegalAccessException e) { | ||
| throw new ErrorException("id 필드 설정 중 오류", e); | ||
| } | ||
| } | ||
|
|
||
| private Long getId(T entity) { | ||
| try { |
There was a problem hiding this comment.
타입 안전성 문제: mapRow()에서 선언자가 없는 생성자를 요구합니다. 엔티티 클래스에 선언자가 없으면 런타임 에러가 발생합니다. 이 제약을 문서화하거나, 리플렉션으로 가장 적절한 생성자를 선택하는 로직을 추가하세요.
| try { | ||
| return DriverManager.getConnection(DB_URL, DB_USER, DB_PASSWORD); | ||
| } catch (SQLException e){ | ||
| log.info(e.fillInStackTrace().toString()); | ||
| log.info(e.getSQLState()); | ||
| throw new ErrorException("DB error"); | ||
| } | ||
| } |
There was a problem hiding this comment.
커넥션 누수 위험: 매번 getConnection()이 호출될 때마다 새로운 Connection 객체가 생성되고, 호출자가 close()를 명시적으로 호출하지 않으면 리소스가 누적됩니다. try-with-resources로 사용하는 현재 코드베이스에선 괜찮지만, 향후 커넥션 풀 도입 시 이 점을 고려하세요. 또한 에러 로깅에서 log.info() 대신 log.error() 사용을 권장합니다.
| public static final List<String> RESOLVED_WORD = List.of( | ||
| "user" | ||
| ); | ||
|
|
There was a problem hiding this comment.
하드코딩된 환경 설정: 데이터베이스 URL, 사용자명이 소스 코드에 하드코딩되어 있습니다. 운영 환경마다 다른 DB 주소를 사용해야 하므로 환경 변수나 설정 파일에서 읽도록 변경하세요. 특히 파일 시스템 절대 경로(/Users/apple/h2/testdb)는 개발 환경에만 동작합니다.
| import java.util.List; | ||
|
|
||
| public class DatabaseConfig { | ||
| private final AppConfig appConfig = new AppConfig(); |
There was a problem hiding this comment.
AppConfig 불필요한 인스턴스화: DatabaseConfig에서 new AppConfig()를 생성하면 AppConfig의 싱글톤 인스턴스들이 다시 만들어질 가능성이 있습니다. WebServer에서 이미 만든 AppConfig 인스턴스를 주입받도록 리팩토링하세요.
…62 (see PR #63) develop <- feat/database/repo template#62
1 participant
💻 작업 내용
✨ 리뷰 포인트
DDL 자동 생성과 CRU 쿼리 자동 생성 기능에 문제가 없는지 체크해주세요.
📝 메모
Crud 레포지토리에서 Delete 기능은 아직 미구현입니다.
DAO는 DatabaseConfig 클래스에서 등록 가능하고, Long 타입의
id라는 이름을 가진 컬럼이 자동으로 pk가 됩니다.id값은 무조건적으로 auto increase 방식으로 생성되므로 Insert시 제외하고 주입합니다.
🎯 관련 이슈
closed #62