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

Step3 PR 입니다 #227

Open
wants to merge 2 commits into
base: misudev
Choose a base branch
from
Open
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
47 changes: 47 additions & 0 deletions src/main/java/persistence/sql/common/Person.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package persistence.sql.common;

import jakarta.persistence.*;

@Table(name = "users")
@Entity
public class Person {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

@Column(name = "nick_name")
private String name;

@Column(name = "old")
private Integer age;

@Column(nullable = false)
private String email;

@Transient
private Integer index;

public Person() {

}

public Person(Long id) {
this.id = id;
}

public Person(Long id, String name, Integer age, String email, Integer index) {
this.id = id;
this.name = name;
this.age = age;
this.email = email;
this.index = index;
}

public Person(String name, Integer age, String email, Integer index) {
this.name = name;
this.age = age;
this.email = email;
this.index = index;
}

}
11 changes: 6 additions & 5 deletions src/main/java/persistence/sql/ddl/DDLHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

import jakarta.persistence.Entity;
import jakarta.persistence.Table;
import persistence.sql.ddl.model.Column;
import persistence.sql.model.ColumnFactory;

import java.util.Arrays;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;


Expand All @@ -14,9 +15,9 @@ public String getCreateQuery(Class<?> clazz) {
var tableName = getTableName(clazz);

String columnInfo = Arrays.stream(clazz.getDeclaredFields())
.map(Column::from)
.filter(Objects::nonNull)
.map(Column::getDDLColumnQuery)
.map(ColumnFactory::createColumn)
.filter(Optional::isPresent)
.map(column -> column.get().getDDLColumnQuery())
Comment on lines +18 to +20
Copy link
Member

Choose a reason for hiding this comment

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

entity에서 Column을 추출해서 List로 들고있어도 되지 않을까요? Columns로 일급컬랙션을 만든다면 이런 로직들이 전부 이관되고, 이렇게 List<Column>을 사용하는 다양한 클래스들에서 사용될 수 있을 것 같아요.

.collect(Collectors.joining(", "));

return "create table " + tableName + "(" + columnInfo + ");";
Expand Down
23 changes: 0 additions & 23 deletions src/main/java/persistence/sql/ddl/Person.java

This file was deleted.

132 changes: 132 additions & 0 deletions src/main/java/persistence/sql/dml/DMLHelper.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
package persistence.sql.dml;

import jakarta.persistence.Column;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.Id;
import jakarta.persistence.Table;
import persistence.sql.model.ColumnFactory;

import java.lang.annotation.Annotation;
import java.lang.reflect.Field;
import java.util.Arrays;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;


public class DMLHelper {
private static final String INSERT_QUERY_FORMAT = "insert into %s (%s) values (%s);";
private static final String FIND_ALL_QUERY_FORMAT = "select * from %s;";
private static final String FIND_QUERY_FORMAT = "select * from %s %s;";
private static final String DELETE_QUERY_FORMAT = "delete from %s %s;";

public String getInsertQuery(Class<?> clazz, Object object) {
return String.format(INSERT_QUERY_FORMAT, getTableName(clazz), columnsClause(clazz), valueClause(object));
}

public String getFindAllQuery(Class<?> clazz) {
return String.format(FIND_ALL_QUERY_FORMAT, getTableName(clazz));
}

public String getFindByIdQuery(Class<?> clazz, Object id) {
try {
var idField = Arrays.stream(clazz.getDeclaredFields())
.filter(field -> {
var column = ColumnFactory.createColumn(field);
return column.isPresent() && column.get().isPK();
})
.findFirst()
.orElseThrow(() -> new IllegalArgumentException("기본키가 존재하지 않습니다."));

var object = clazz.getDeclaredConstructor().newInstance();
idField.setAccessible(true);
idField.set(object, id);
Comment on lines +41 to +43
Copy link
Member

Choose a reason for hiding this comment

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

Object로 넘기게 된다면 사용하는 메소드에서 타입과 관련된 정보를 알 수 없어요. 사용하는 측에서 무엇을 사용해야하는지 명확하게 하기 위해서 하나의 클래스를 만들어보는 건 어떨까요?

이미 잘 만들어주신 Column을 사용한다면 Map<Column, Object>를 가지고 있는 일급컬랙션하나를 만들 수 있을 것 같아요. 그럼 whereClause쪽 코드들의 복잡도가 내려갈 것 같은데 어떻게 생각하세요?


return String.format(FIND_QUERY_FORMAT, getTableName(clazz), whereClause(object));
} catch (Exception e) {
throw new IllegalArgumentException("생성 불가 한 class 입니다.");
}
}

public String getDeleteQuery(Class<?> clazz, Object object) {
return String.format(
DELETE_QUERY_FORMAT,
getTableName(clazz),
whereClause(object)
);
}

private String whereClause(Object object) {
var clazz = object.getClass();

return "where " + Arrays.stream(clazz.getDeclaredFields())
.map(field -> {
var columnValue = getColumnValue(field, object);
var column = ColumnFactory.createColumn(field);
if (columnValue.isEmpty() || column.isEmpty()) return null;
return String.format("%s = %s", column.get().getName(), columnValue.get());
}
)
.filter(Objects::nonNull)
.collect(Collectors.joining(" and "));
}

private String getTableName(Class<?> clazz) {
var annotations = clazz.getAnnotations();
return Arrays.stream(annotations)
.filter(annotation -> annotation.annotationType().equals(Table.class))
.map(table -> ((Table) table).name())
.findFirst()
.orElse(clazz.getSimpleName());
}
Comment on lines +74 to +81
Copy link
Member

Choose a reason for hiding this comment

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

Column과 마찬가지로 Class<?>를 받아 테이블 이름을 가지는 TableName 클래스가 있으면 이 책임이 거기로 이관 될 것 같네요.

public class TableName {
    private final String value;

    public TableName(Class<?> clazz) {
    // 테이블 어노테이션이 있으면 그 값을 사용하고 아니면 클래스 명 사용
}


private String columnsClause(Class<?> clazz) {
var fields = clazz.getDeclaredFields();
return Arrays.stream(fields)
.filter(this::isInsertQueryTarget)
.map(ColumnFactory::createColumn)
.filter(Optional::isPresent)
.map(column -> column.get().getName())
.collect(Collectors.joining(", "));
}

private String valueClause(Object object) {
var clazz = object.getClass();
return Arrays.stream(clazz.getDeclaredFields())
.filter(this::isInsertQueryTarget)
.map(field -> getColumnValue(field, object))
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.joining(", "));
}
private boolean isInsertQueryTarget(Field field) {
var isId = false;
var isGeneratedValue = false;
var isColumn = false;

for (Annotation annotation : field.getAnnotations()) {
if (annotation instanceof Column) {
isColumn = true;
} else if (annotation instanceof Id) {
isId = true;
} else if (annotation instanceof GeneratedValue) {
isGeneratedValue = true;
}
}

return isColumn || (isId && !isGeneratedValue);
}
Comment on lines +93 to +118
Copy link
Member

Choose a reason for hiding this comment

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

이런 로직들도 모두 Column, Columns를 활용한다면 충분히 옮겨질 수 있는 로직으로 보여요. 이미 만들어진 로직과 겹치는 부분들도 보이구요.

DMLHelper는 dml 쿼리를 만드는 역할을 가지고 있고, Class<?>를 받아 쿼리를 생성하긴 하지만 리플랙션에 관련된 책임들은 모두 model패키지에 잘 만들어주신 도메인 객체들에게 넘겨주시면 좋을 것 같아요.


private Optional<String> getColumnValue(Field field, Object object) {
try {
field.setAccessible(true);
var column = ColumnFactory.createColumn(field);
if (column.isPresent()) {
return Optional.ofNullable(column.get().getQueryValue(field.get(object)));
}
return Optional.empty();
} catch (IllegalAccessException e) {
throw new IllegalStateException("접근할 수 없는 필드입니다.");
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

(반영하지 않으셔도 됩니다.) 지금도 충분히 잘 구현해주셔서 변경하지 않으셔도 되는데 제가 의도했던 바만 코멘트 추가로 남기겠습니다.

PkColumn와 SimpleColumn을 생각해보았을때 PkColumn은 SimpleColumn에서 추가로 id와 관련된 기능이 추가된 클래스에요. 그래서 PkColumn이 SimpleColumn을 들고 있고 그 둘을 Column이라는 인터페이스로 묶는건 어떨까하여 드린 피드백이었습니다. 구현의 차이가 있을 것 같은데 추상화를 통해서 공통된 부분을 묶는다는 컨셉은 동일해요 😄

public interface Column
public class PkColumn implements Column {
    private final String strategy;
    private final SimpleColumn;
    // ...
public class SimpleColumn implements Column {
    private final String name;
    private final Type type;
    // ...

Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package persistence.sql.ddl.model;

import jakarta.persistence.Id;
package persistence.sql.model;

import java.lang.reflect.Field;
import java.util.Arrays;
import java.util.Objects;
import java.util.Optional;

public class Column {
private String name;
Expand All @@ -22,30 +22,16 @@ public Condition getCondition() {
return condition;
}

public Column(String name, Type type, Condition condition) {
Column(String name, Type type, Condition condition) {
this.name = name;
this.type = type;
this.condition = condition;
}

public static Column from(Field field) {
var isPKColumn = Arrays.stream(field.getAnnotations())
.anyMatch(annotation -> annotation.annotationType().equals(Id.class));

if (isPKColumn) return PKColumn.from(field);
var columnAnnotation = Arrays.stream(field.getAnnotations())
.filter(annotation -> annotation.annotationType().equals(jakarta.persistence.Column.class))
.map(annotation -> (jakarta.persistence.Column) annotation)
.findFirst();

if (columnAnnotation.isEmpty()) return null;
var column = columnAnnotation.get();

var name = column.name().isBlank() ? field.getName() : column.name();
var type = Type.from(field.getType());
var condition = Condition.from(column);

return new Column(name, type, condition);
Column(String name, Type type) {
this.name = name;
this.type = type;
this.condition = Condition.DEFAULT_CONDITION;
}

public String getDDLColumnQuery() {
Expand All @@ -57,4 +43,23 @@ public String getDDLColumnQuery() {
if (condition.isUnique()) sb.append(" ").append("UNIQUE");
return sb.toString();
}

public String getQueryValue(Object value) {
if (Objects.isNull(value)) return null;
Copy link
Member

Choose a reason for hiding this comment

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

QueryValue에 사용하는 정보를 반환하기 때문에 null을 반환하기 보다는 "null"로 String값을 반환하는게 좋을 것 같아요. null을 출력하는 것은 동일하지만 null 자체를 반환하는 것과 출력에 필요한 "null"을 반환하는건 기능상의 차이가 있습니다. null을 반환하여 외부에서 이를 컨트롤하도록 기대하는 것은 프로그램의 장애를 발생시키는 요인이 될 수 있어요.

if (type == Type.VARCHAR) {
return String.format("'%s'", value);
}
return String.valueOf(value);
}

public boolean isPK() {
return false;
}

protected static Optional<jakarta.persistence.Column> getColumnAnnotation(Field field) {
return Arrays.stream(field.getAnnotations())
.filter(annotation -> annotation.annotationType().equals(jakarta.persistence.Column.class))
.map(annotation -> (jakarta.persistence.Column) annotation)
.findFirst();
}
}
23 changes: 23 additions & 0 deletions src/main/java/persistence/sql/model/ColumnFactory.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package persistence.sql.model;

import jakarta.persistence.Id;
import jakarta.persistence.Transient;

import java.lang.reflect.Field;
import java.util.Arrays;
import java.util.Optional;

public class ColumnFactory {
public static Optional<Column> createColumn(Field field) {
boolean isPKColumn = false;
var isTransient = false;
for(var annotation : field.getAnnotations()) {
if (annotation.annotationType().equals(Id.class)) isPKColumn = true;
if (annotation.annotationType().equals(Transient.class)) isTransient = true;
}

if (isTransient) return Optional.empty();
if (isPKColumn) return Optional.of(PKColumn.from(field));
return Optional.of(SimpleColumn.from(field));
}
Comment on lines +11 to +22
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static Optional<Column> createColumn(Field field) {
boolean isPKColumn = false;
var isTransient = false;
for(var annotation : field.getAnnotations()) {
if (annotation.annotationType().equals(Id.class)) isPKColumn = true;
if (annotation.annotationType().equals(Transient.class)) isTransient = true;
}
if (isTransient) return Optional.empty();
if (isPKColumn) return Optional.of(PKColumn.from(field));
return Optional.of(SimpleColumn.from(field));
}
public static Optional<Column> createColumn(Field field) {
if (field.isAnnotationPresent(Transient.class)) {
return Optional.empty();
}
if (field.isAnnotationPresent(Id.class)) {
return Optional.of(PKColumn.from(field));
}
return Optional.of(SimpleColumn.from(field));
}

isAnnotationPresent()를 활용하면 더 직관적으로 표현될 수 있을 것 같아요.

(반영하지 않으셔도 됩니다.) 이와는 별개로 Factory가 과연 필요할지는 고민이 필요할 것 같아요. Column에서 정적 팩토리 메소드로 들고있어도 되지 않을까요? Factory는 복잡한 로직에 대해서 생성에 대한 책임을 분산시킬수 있도록 도움을 주기도 하지만 자칫 생성에 대한 책임이 분산되어 클래스가 늘어나 클래스 관리가 더 힘들어질 수 있어요.

}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package persistence.sql.ddl.model;
package persistence.sql.model;

public class Condition {
public static final Condition DEFAULT_CONDITION = new Condition(false, true);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
package persistence.sql.ddl.model;
package persistence.sql.model;

import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;

import java.lang.reflect.Field;
import java.util.Arrays;
import java.util.Objects;

public class PKColumn extends Column {
private String strategy;
Expand All @@ -16,10 +15,7 @@ public PKColumn(String name, Type type, Condition condition, String strategy) {
}

public static PKColumn from(Field field) {
var column = Arrays.stream(field.getAnnotations())
.filter(annotation -> annotation.annotationType().equals(jakarta.persistence.Column.class))
.map(annotation -> (jakarta.persistence.Column) annotation)
.findFirst();
var column = getColumnAnnotation(field);

var name = column.filter(value -> !value.name().isBlank())
.map(jakarta.persistence.Column::name)
Expand Down Expand Up @@ -56,4 +52,9 @@ public String getDDLColumnQuery() {
sb.append(" PRIMARY KEY");
return sb.toString();
}

@Override
public boolean isPK() {
return true;
}
}
28 changes: 28 additions & 0 deletions src/main/java/persistence/sql/model/SimpleColumn.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package persistence.sql.model;

import java.lang.reflect.Field;
import java.util.Arrays;

public class SimpleColumn extends Column {
public SimpleColumn(String name, Type type, Condition condition) {
super(name, type, condition);
}

public SimpleColumn(String name, Type type) {
super(name, type);
}

public static SimpleColumn from(Field field) {
var columnAnnotation = getColumnAnnotation(field);
if (columnAnnotation.isEmpty()) {
return new SimpleColumn(field.getName(), Type.from(field.getType()));
}

var column = columnAnnotation.get();
var name = column.name().isBlank() ? field.getName() : column.name();
Copy link
Member

Choose a reason for hiding this comment

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

(반영하지 않으셔도 됩니다.) 삼항연산자보다는 if else를 직접 표현하는 것이 가독성이 더 올라갑니다. 이정도의 기능이라면 parseColumnName()이라는 메소드로 분리해보는건 어떨까요?

private static String parseColumnName(Field field, jakarta.persistence.Column column) {
    if (column.name().isBlank()) {
        return field.getName();
    }
    return column.name();
}

var type = Type.from(field.getType());
var condition = Condition.from(column);

return new SimpleColumn(name, type, condition);
}
}
Loading