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

Refactor: #4의 DataEntity 구현을 수정합니다 #6

Merged
merged 12 commits into from
May 8, 2024
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
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package com.walking.api.web.repository.member;
package com.walking.api.repository.member;

import com.walking.data.entity.member.MemberEntity;
import org.springframework.data.jpa.repository.JpaRepository;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package com.walking.api.web.repository.member;
package com.walking.api.repository.member;

import com.walking.data.entity.member.PathFavoritesEntity;
import org.springframework.data.jpa.repository.JpaRepository;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package com.walking.api.web.repository.member;
package com.walking.api.repository.member;

import com.walking.data.entity.member.TrafficFavoritesEntity;
import org.springframework.data.jpa.repository.JpaRepository;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package com.walking.api.web.repository.traffic;
package com.walking.api.repository.traffic;

import com.walking.data.entity.traffic.TrafficEntity;
import org.springframework.data.jpa.repository.JpaRepository;
Expand Down
142 changes: 0 additions & 142 deletions api/src/test/java/com/walking/api/RepoTest.java

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package com.walking.api;
package com.walking.api.repository;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.walking.api.config.ApiDataSourceConfig;
Expand Down Expand Up @@ -38,4 +38,4 @@
ObjectMapper.class,
})
@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
class RepositoryTest {}
abstract class RepositoryTest {}
2 changes: 1 addition & 1 deletion data/src/main/java/com/walking/data/entity/BaseEntity.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
public abstract class BaseEntity {

@Id
@GeneratedValue(strategy = GenerationType.SEQUENCE)
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

@Column(nullable = false, updatable = false)
Copy link

Choose a reason for hiding this comment

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

코드 리뷰:

  1. 변경된 부분에서 ID 생성 전략이 GenerationType.SEQUENCE에서 GenerationType.IDENTITY로 변경되었습니다. 데이터베이스에 따라 IDENTITY 전략은 메모리 부족 문제를 유발할 수 있으므로 주의해야 합니다.
  2. BaseEntity 클래스에 다른 필드나 메서드가 있는지 확인하세요. 그것이 명시되어 있지 않습니다.
  3. JavaDoc이나 주석을 추가하여 클래스 및 속성에 대한 설명을 더 자세히 달아보세요.
  4. 테스트 코드를 작성하여 새 전략이 예상대로 작동하는지 확인하세요.
  5. 타입 및 어노테이션 사용에 대한 커뮤니티의 권장 사항을 따르는 것이 좋습니다.

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package com.walking.data.entity.member;

public enum CertificationSubject {
KAKAO,
}
Copy link

Choose a reason for hiding this comment

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

이 코드 패치는 간단한 enum을 선언하는 것으로 보입니다. 여러 가지 사항에 대해 고려할 수 있습니다:

  1. 다른 인증 주체나 유형의 경우를 나중에 추가할 계획이 있는지 확인해야 합니다. 그럴 경우, KAKAO 외에도 값을 추가해야 합니다.
  2. CertificationSubject enum이 현재 한 값만 가지고 있어 확장 가능성을 고려하셔서 미래에 다른 값이 필요할 수 있음을 염두에 두세요.
  3. Java naming conventions을 준수하고 있는지 확인하시기 바랍니다.

이외에는 이 코드 스니펫이 단순하고 문제가 없어 보입니다.

Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@
import com.walking.data.entity.BaseEntity;
import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.EnumType;
import javax.persistence.Enumerated;
import javax.persistence.Table;
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.experimental.SuperBuilder;
Expand All @@ -20,9 +23,32 @@
@SQLDelete(sql = "UPDATE member SET deleted=true where id=?")
public class MemberEntity extends BaseEntity {

/* 소셜 로그인을 통해 가입한 회원의 닉네임 */
@Column(nullable = false, unique = true, length = 50)
private String nickName;

/* 소셜 로그인을 통해 가입한 회원의 프로필 이미지 URL */
@Column(nullable = false)
private String memberId;
private String profile;

/* 소셜 로그인을 통해 가입한 회원의 식별자 */
@Column(nullable = false, unique = true)
private String certificationId;

/* 소셜 로그인을 통해 가입한 회원의 인증 주체 */
@SuppressWarnings("FieldMayBeFinal")
@Enumerated(EnumType.STRING)
@Builder.Default
@Column(nullable = false)
private String password;
private CertificationSubject certificationSubject = CertificationSubject.KAKAO;

@SuppressWarnings("FieldMayBeFinal")
@Enumerated(EnumType.STRING)
@Builder.Default
@Column(nullable = false)
private MemberStatus status = MemberStatus.REGULAR;

@Builder.Default
@Column(nullable = false, columnDefinition = "json")
private String resource = "{}";
}
Copy link

Choose a reason for hiding this comment

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

이 코드 패치의 코드 리뷰:

  1. @Enumerated를 사용할 때, EnumType.STRING을 명시하는 것은 좋은 접근 방식입니다. 그러나 certificationSubjectstatus 필드에 있는 @Builder.Default는 필요 없어 보입니다.
  2. 코드 상에서 나타나는 주석(Comment)은 코드 읽기를 돕지만, 오탈자가 있습니다. 수동 검토 및 수정이 필요합니다.
  3. profileresource 필드의 컬럼 타입이 json으로 지정되어 있으므로 ORM 프레임워크와 DB 버전 확인이 필요합니다.
  4. 문자열 길이를 더 엄격하게 체크하거나 validation을 추가하는 것이 유용할 수 있습니다.

개선 제안:

  1. 모든 필드 옵션들을 일관된 스타일로 정렬하여 가독성을 향상시킵니다.
  2. 필요한 경우, Validation annotation을 추가하여 데이터 유효성을 강화합니다.
  3. 그 외 개인적인 코딩 스타일에 맞춰 코드 스타일 가이드를 엄격히 지키면서 작업합니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.walking.data.entity.member;

import lombok.ToString;

@ToString
public enum MemberStatus {
REGULAR("정회원"),
ASSOCIATE("준회원"),
SEPARATE("장기미이용 회원"),
WITHDRAWN("탈퇴회원"),
;

private final String description;

MemberStatus(String description) {
this.description = description;
}
}
Copy link

Choose a reason for hiding this comment

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

이 코드는 깔끔하고 간단한 열거형(MemberStatus)을 정의하고 있습니다. 개선할 점으로는 다음과 같은 사항을 고려할 수 있습니다:

  1. 주석: 코드에 주석을 추가하여 각 회원 상태가 무엇을 나타내는지 더 명확히 설명할 수 있습니다.
  2. 검증 및 예외 처리: 입력 값에 대한 유효성 검사나 처리를 추가해야 할 수 있습니다.
  3. toString() 메서드 오버라이드: 추가로 필요한 경우, toString() 메서드를 오버라이딩하여 좀 더 유용한 정보를 출력할 수 있습니다.

위 내용은 코드 리뷰에서 고려될 수 있는 세 가지 포인트입니다.

Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,21 @@
public class PathFavoritesEntity extends BaseEntity {

@ManyToOne(fetch = FetchType.LAZY)
private MemberEntity memberEntity;
@JoinColumn(foreignKey = @ForeignKey(value = ConstraintMode.NO_CONSTRAINT))
private MemberEntity memberFk;

@Column(columnDefinition = "POINT")
@Column(nullable = false, columnDefinition = "POINT SRID 4326")
private Point startPoint;

@Column(columnDefinition = "POINT")
@Column(nullable = false, columnDefinition = "POINT SRID 4326")
private Point endPoint;

@Column(columnDefinition = "LINESTRING")
@Column(nullable = false, columnDefinition = "LINESTRING SRID 4326")
private LineString path;

@Column(nullable = false)
@Column(nullable = false, length = 50)
private String startAlias;

@Column(nullable = false)
@Column(nullable = false, length = 50)
private String endAlias;
}
Copy link

Choose a reason for hiding this comment

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

이 코드는 좋아보입니다. 몇 가지 제안사항은 다음과 같습니다:

  1. startPoint, endPoint, 및 pathnullable 속성을 false로 설정하여 필수 필드임을 명시적으로 표현합니다.
  2. 외래 키 (memberFk)에 ConstraintMode.NO_CONSTRAINT를 사용하는 것은 데이터베이스 무결성을 일부 부여해 줍니다.
  3. 이 코드에서 길이 제약이 있는 문자열 필드 (50자)를 정의했는데, 데이터의 적절한 길이를 파악 후 조정이 필요할 수 있습니다.

위의 제안을 고려하여 코드가 확장 가능하고 유지 보수가 쉬워질 수 있도록 하는 것이 좋습니다.

Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@
public class TrafficFavoritesEntity extends BaseEntity {

@ManyToOne(fetch = FetchType.LAZY)
private MemberEntity memberEntity;
@JoinColumn(foreignKey = @ForeignKey(value = ConstraintMode.NO_CONSTRAINT))
private MemberEntity memberFk;

@OneToOne(fetch = FetchType.LAZY)
private TrafficEntity trafficEntity;
@JoinColumn(foreignKey = @ForeignKey(value = ConstraintMode.NO_CONSTRAINT))
private TrafficEntity trafficFk;

@Column(nullable = false)
@Column(nullable = false, length = 50)
private String alias;
}
Copy link

Choose a reason for hiding this comment

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

이 코드 패치는 다음과 같은 변경을 포함합니다:

  1. memberEntitymemberFk로 변경하고, foreign key constraint 없이 설정되도록 합니다.
  2. trafficEntitytrafficFk로 변경하고, foreign key constraint 없이 설정되도록 합니다.
  3. alias 필드에 길이 제한인 length = 50을 추가했습니다.

개선 제안:

  1. memberEntity, trafficEntity가 이름에서 FK(Foreign Key)를 의미하는 Fk로 변경되었는지 주석으로 설명하는 것이 도움이 됩니다.
  2. @Column 어노테이션의 속성에 대해 주석으로 설명을 추가하는 것이 좋습니다. (예: nullablelength)

버그 리스크:

  1. 외래 키 제약 조건을 강제로 없앤다는 점에서 데이터 무결성 문제 발생 가능성이 있을 수 있습니다. 수정될 수 있습니다.

이 외에도 코드 구조 및 목적에 따라 다른 개선 사항이 있을 수 있습니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.walking.data.entity.support.listener;

import com.walking.data.entity.traffic.TrafficEntity;
import javax.persistence.PreRemove;

public class TrafficEntitySoftDeleteListener {

@PreRemove
private void preRemove(TrafficEntity entity) {
entity.delete();
}
}
Copy link

Choose a reason for hiding this comment

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

이 코드는 JPA Entity의 Soft Delete를 지원하기 위한 Listener로 보입니다. 해당 리스너는 TrafficEntity가 삭제되기 전에 delete() 메서드를 호출하여 소프트 삭제를 트리거합니다.

개선 제안:

  1. delete() 메서드에 대한 구현이 이 코드 조각에서만 사용되는 것으로 보이므로 해당 메서드가 어떻게 정의되었는지 확인이 필요합니다.
  2. 코드가 완전하지 않으면 일관성을 유지하기 위해 주석을 추가하는 것이 좋습니다.
  3. 리스너 메서드가 private으로 선언되어 있으므로 JPA가 올바르게 작동하려면 public으로 변경해야 합니다.

버그 리스크:

  1. @PreRemove 애노테이션은 파라미터 없는 메소드에 적용되어야 하며, 여기에서는 TrafficEntity를 파라미터로 갖고 있는 메소드로 사용했습니다. JPA 스펙에 따라 이렇게 하면 예상대로 동작하지 않을 수 있습니다.

코드 리뷰 결과, 리스너 메서드 시그니처를 수정하고 접근 제어자를 public으로 변경하면 이 코드 조각은 더욱 안정적으로 작동할 것으로 예상됩니다.

Original file line number Diff line number Diff line change
@@ -1,32 +1,61 @@
package com.walking.data.entity.traffic;

import com.walking.data.entity.BaseEntity;
import com.walking.data.entity.support.listener.TrafficEntitySoftDeleteListener;
import java.time.LocalDateTime;
import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.EntityListeners;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.Table;
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.experimental.SuperBuilder;
import org.hibernate.annotations.SQLDelete;
import org.locationtech.jts.geom.Point;
import org.springframework.data.annotation.CreatedDate;
import org.springframework.data.annotation.LastModifiedDate;
import org.springframework.data.jpa.domain.support.AuditingEntityListener;

@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@AllArgsConstructor(access = AccessLevel.PRIVATE)
@Entity
@SuperBuilder(toBuilder = true)
@EntityListeners({AuditingEntityListener.class, TrafficEntitySoftDeleteListener.class})
@Builder(toBuilder = true)
@Table(name = "traffic")
@SQLDelete(sql = "UPDATE traffic SET deleted=true where id=?")
public class TrafficEntity extends BaseEntity {
public class TrafficEntity {

@Column(nullable = false, updatable = false)
private String detail;

@Column(nullable = false)
private String name;

@Column(columnDefinition = "POINT SRID 4326", nullable = false)
@Column(nullable = false, columnDefinition = "POINT SRID 4326")
private Point point;

@Id
@GeneratedValue(strategy = GenerationType.SEQUENCE)
private Long id;

@Column(nullable = false, updatable = false)
@CreatedDate
private LocalDateTime createdAt;

@Column(nullable = false)
@LastModifiedDate
private LocalDateTime updatedAt;

@Builder.Default
@Column(nullable = false)
private Boolean deleted = false;

public void delete() {
this.deleted = true;
}
}
Copy link

Choose a reason for hiding this comment

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

이 코드는 대체로 잘 구성되어 있지만 몇 가지 개선 사항이 있습니다:

  1. 버그 및 위험 상황:

    • TrafficEntity 클래스에 삭제 관련 로직이 있으나, 이를 저장하는 부분은 누락되었습니다. 해당 엔티티가 실제로 삭제되지 않고 데이터베이스에서 보존되는 문제가 있을 수 있습니다.
    • id 필드는 @GeneratedValue(strategy = GenerationType.SEQUENCE) 옵션으로 설정되어 있지만 SEQUENCE가 정의되지 않았습니다. 이는 ID 생성 방법에 대한 부적절한 설정입니다.
  2. 개선 제안:

    • delete() 메서드가 있지만 이 메서드를 어떻게 사용할 지 명확하지 않습니다. 해당 메서드와 함께 사용해야 하는 다른 메서드나 프로세스가 필요합니다.
    • createdAtupdatedAt 필드를 LocalDateTime으로 유지하는 것 대신 Instant를 사용하는 것이 시간 관련 데이터를 다룰 때 더 안전하고 권장됩니다.
    • deleted 필드는 Boolean 값으로 갱신되며 ObjectMapper를 통해 수정되어 이슈를 초래할 수 있습니다. 따라서 delete() 메서드를 통해 논리적인 삭제를 처리하는 방법이 더 좋을 수 있습니다.
  3. 기타 고려 사항:

    • @Builder 어노테이션을 사용하여 생성자 자동화를 사용하는 경우, 필수 입력 매개변수와 선택적 매개변수를 구분하는 것이 중요합니다.

좀 더 명확하고 안정적인 코드를 작성하기 위해 위의 제안을 고려해보세요.

7 changes: 6 additions & 1 deletion data/src/main/resources/application-data-dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,9 @@ spring:
ddl-auto: validate
properties:
hibernate:
format_sql: true
format_sql: true
dialect: org.hibernate.spatial.dialect.mysql.MySQL8SpatialDialect
order_inserts: true
order_updates: true
jdbc:
Copy link

Choose a reason for hiding this comment

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

주요 변경 사항은 Hibernate dialect가 MySQL8SpatialDialect로 설정되었으며 insert 및 update 문장이 순서화되었습니다. jdbc의 batch_size도 설정되었습니다.

개선 제안:

  1. 적절한 권한이 없는 상황에서 자바를 실행하는 것을 방지하기 위해 보안 강화가 필요할 수 있습니다.
  2. 설정 값들이나 버전 관리 시스템과 같은 곳에 연결된 값들을 하드코딩하는 대신, 외부 속성 파일을 사용하여 값을 설정하는 것이 좋습니다.
  3. 주석을 추가하여 각각의 설정이 어떤 역할을 하는지 설명하는 것이 도움이 될 수 있습니다.
  4. 예기치 않은 입력값 오류를 방지하기 위해 입력 유효성 검사를 추가하는 것이 바람직합니다.

이 코드는 전반적으로 안정적으로 보입니다.

batch_size: ${JDBC_BATCH_SIZE:50}
Loading
Loading