채팅방 API 서버 - 엔티티 개선(1)

https://github.com/bellringstar/keepham/tree/master/BACKEND/src/main/java/com/ssafy/keepham/domain/chatroom

keepham/BACKEND/src/main/java/com/ssafy/keepham/domain/chatroom at master · bellringstar/keepham

keepham-refact/BACKEND/src/main/java/com/ssafy/keepham/domain/chatroom at main · bellringstar/keepham-refact (github.com)

keepham-refact/BACKEND/src/main/java/com/ssafy/keepham/domain/chatroom at main · bellringstar/keepham-refact

 

위에서 리팩토링 전/후의 전체 코드를 확인할 수 있습니다.

코드를 보면 아시겠지만 전체적으로 책임의 분리, 예외처리 일관성 부족, 동시성 이슈 등이 부족합니다.

ChatRoomEntity 개선 - 전

우선은 ChatRoomEntity부터 보겠습니다.

@Entity
@Table(name = "chat_room")
@Getter
@Setter
@ToString(callSuper = true)
@NoArgsConstructor
@AllArgsConstructor
@SuperBuilder
@Builder
public class ChatRoomEntity extends BaseEntity {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Long id;
    @Column(length = 255, nullable = false, unique = true)
    private String title;
    @Enumerated(value = EnumType.STRING)
    @Column(nullable = false)
    private ChatRoomStatus status;
    @Column(nullable = false)
    private Long storeId;
    @OneToOne
    @JoinColumn(name = "box_id")
    private Box box;
    private Integer extensionNumber;
    private int maxPeopleNumber;
    private String superUserId;
    @Column(columnDefinition = "boolean default 0", nullable = false)
    private boolean locked;
    @Column(length = 255, nullable = false)
    private String password;
    private LocalDateTime closedAt;
    private int step;
}

1. 우선 저 많은 롬복 어노테이션에 대해 하나하나 따져봅시다.
   1. @Entity JPA 엔티티임을 명시하는 데 필요한 어노테이션입니다.
      
      
   2. @Table 데이터베이스 테이블 이름을 지정하는 데 사용합니다. 없다면 chat_room_entity라는 이름으로 테이블이 생성됐을 것입니다. 오히려 클래스명을 Chatroom으로 변경하는 것이 옳다고 보입니다. 하지만 이는 팀 컨벤션에 따라 다를 수 있습니다.
      
      
   3. @Getter 모든 필드에 대한 getter 메서드를 생성해 줍니다. 개인적으로는 상태가 외부에 노출되기 때문에 반드시 필요한 필드에만 적용하는 게 맞다고 생각되지만 개발의 편의성을 위해 유지하는 것도 괜찮다고 봅니다.
      
      
   4. @Setter 제거 1순위입니다. 모든 필드에 대해 setter를 생성해 객체의 불변성을 해칩니다. 또한 이로 인해 사이드 이펙트가 발생할 수 있습니다. setter 보다는 보다 의미 있는 도메인 로직을 추가하는 것이 옳다고 생각됩니다.
      
      
   5. @ToString(callSuper=true) 해당 어노테이션은 ToString() 메서드를 자동으로 생성해 줍니다. callSuper 옵션을 통해 상위 클래스의 필드값 여기서는 created_at과 updated_at을 함께 출력하도록 설정했습니다. @ToString 어노테이션은 항상 순환 참조를 조심해야 합니다. 필요한 필드만 포함하도록 수정하는 것이 옳다고 생각됩니다.
      
      
   6. @NoArgsConstructor 매개변수가 없는 기본생성자를 생성해줍니다. JPA는 리플렉션을 사용해 엔티티 객체를 생성하기 때문에 필수적으로 필요하지만 과도한 노출을 제어하기 위해 접근 권한을 protected로 제한하는 것이 옳다고 생각됩니다.
      
      
   7. @AllArgsConstructor 부적절한 어노테이션입니다. 모든 필드를 매개변수로 가지는 생성자를 생성해 줍니다. 모든 필드를 매개변수로 받게 되면 객체 생성 시 오류 가능성을 높입니다. 따라서 제거하고 필요한 필드만 받는 생성자를 명시적으로 정의하는 게 옳다고 생각됩니다.
      
      
   8. @SuperBuilder, @Builder 부적절합니다. 모든 필드에 대해 빌더 메서드를 생성할 필요는 없습니다. 또한 @SuperBuilder를 쓸 이유조차 없습니다. 상위 클래스인 BaseEntity를 매개변수로 받으면 안 됩니다. 이 클래스의 필드들은 Auditing을 통해 자동으로 생성되고 있습니다. @Builder의 경우 @SuperBuilder와 역할이 중복됩니다.
2. 다음으로 필드들을 보겠습니다.
   1. storeId와 Box 둘 다 별개의 엔티티와 관계를 맺는 일종의 외래키 포지션의 필드입니다. 하지만 어떤 것은 Long 타입이고 어떤 것은 객체를 통해 관계를 맺고 있습니다.
      
      
   2. 용도가 불분명한 필드 extensionNumber, step 등의 필드가 어떤 역할을 하는 필드인지 불분명합니다. 보다 명확한 네이밍이 필요하다고 생각됩니다.

ChatRoomEntity 개선

@Entity
@Table(name = "chatroom")
@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@ToString(exclude = {"store", "box"})
public class ChatRoom extends BaseEntity {

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

    @Column(length = 255, nullable = false, unique = true)
    private String title;

    @Enumerated(value = EnumType.STRING)
    @Column(nullable = false)
    private ChatRoomStatus status;

    @Column(nullable = false)
    @OneToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "store_id")
    private Store store;

    @Column(nullable = false)
    @OneToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "box_id")
    private Box box;

    @Column(nullable = false)
    @OneToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "user_user_id")
    private User superUser;

    private int extensionCount;

    @Column(nullable = false)
    private int maxPeopleNumber;

    @Column(nullable = false)
    private boolean locked;

    @Column(length = 64, nullable = false)
    private String password;

    @Column(length = 16, nullable = false)
    private String salt;

    private LocalDateTime closedAt;

    @Enumerated(EnumType.STRING)
    @Column(nullable = false)
    private RoomPhase phase;

    @Builder
    private ChatRoom(String title, Store store, Box box, int maxPeopleNumber, User superUser, boolean locked, String password) {
        this.title = Objects.requireNonNull(title, "제목은 null일 수 없습니다.");
        this.store = Objects.requireNonNull(store, "store는 null일 수 없습니다.");
        this.box = Objects.requireNonNull(box, "Box는 null일 수 없습니다.");
        this.maxPeopleNumber = validateMaxPeopleNumber(maxPeopleNumber);
        this.superUser = Objects.requireNonNull(superUser, "Super user는 null일 수 없습니다.");
        this.locked = locked;
        this.password = locked ? Objects.requireNonNull(password, "비밀방에 password는 필수입니다.") : "";
        setPassword(password);
        this.status = ChatRoomStatus.OPEN;
        this.extensionCount = 0;
        this.phase = RoomPhase.CREATED;
    }

    public void setPassword(String password) {
        this.salt = generateSalt();
        this.password = hashPassword(password, this.salt);
    }

    public boolean checkPassword(String inputPassword) {
        String hashedInput = hashPassword(inputPassword, this.salt);
        return this.password.equals(hashedInput);
    }

    public void close() {
        if (this.status != ChatRoomStatus.OPEN) {
            throw new ApiException(ChatRoomError.BAD_REQUEST, "열려있는 채팅방만 닫을 수 있습니다.");
        }
        this.status = ChatRoomStatus.CLOSE;
        this.closedAt = LocalDateTime.now();
    }

    public void extendTime(long hours) {
        if (this.extensionCount >= 3) {
            throw new ApiException(ChatRoomError.BAD_REQUEST, "3회 이상 연장할 수 없습니다.");
        }
        this.closedAt = this.closedAt.plusHours(hours);
        this.extensionCount++;
    }

    public void changeSuperUser(User newSuperUser) {
        this.superUser = Objects.requireNonNull(newSuperUser, "Super user ID는 null일 수 없습니다.");
    }

    public void updatePhase(RoomPhase newPhase) {
        validatePhaseTransition(this.phase, newPhase);
        this.phase = newPhase;
    }

    private void validatePhaseTransition(RoomPhase currentPhase, RoomPhase newPhase) {
        if (newPhase.getStep() <= currentPhase.getStep()) {
            throw new ApiException(ChatRoomError.BAD_REQUEST, currentPhase + "에서 " + newPhase + "는 변경할 수 없습니다.");
        }
    }

    private int validateMaxPeopleNumber(int maxPeopleNumber) {
        if (maxPeopleNumber < 1 || maxPeopleNumber > 12) {
            throw new ApiException(ChatRoomError.BAD_REQUEST, "최대 인원은 1 ~ 12 입니다");
        }
        return maxPeopleNumber;
    }

    private String generateSalt() {
        SecureRandom random = new SecureRandom();
        byte[] salt = new byte[16];
        random.nextBytes(salt);
        return Base64.getEncoder().encodeToString(salt);
    }

    private String hashPassword(String password, String salt) {
        try {
            MessageDigest md = MessageDigest.getInstance("SHA-256");
            md.update(salt.getBytes());
            byte[] hashedPassword = md.digest(password.getBytes());
            return Base64.getEncoder().encodeToString(hashedPassword);
        } catch (NoSuchAlgorithmException e) {
            throw new RuntimeException("Failed to hash password", e);
        }
    }
}

위에서 언급됐던 문제들을 개선했습니다.

 

개선하며 고민한 점은 다음과 같습니다.

1. 도메인 로직 추가로 인한 동시성 이슈
   예를 들어 extendTime 메서드의 경우 확인하고 증가시키고 있기 때문에 경쟁 조건이 발생할 수 있습니다.
   처음에는 비즈니스 제약 상 superUser만 해당 요청들을 할 수 있기 때문에 동시성 이슈의 발생 가능성이 낮다고 생각했습니다.
   그렇지만 만약 superUser가 여러 탭을 열어서 동시에 요청을 하는 경우, 트랜잭션 간의 경합 등의 원인으로 이슈가 발생할 수 있다고 판단했습니다.
   구현이 간단하며 도메인 로직들의 충돌 발생 확률이 낮다고 판단했기에 낙관적 락을 선택했습니다.
   
   
2. superUserId 필드를 엔티티로 연관관계를 맺는게 맞는가
   기존의 String으로 사용자의 로그인ID를 가지고 있는 방식과 연관관계를 맺어 외래키로 userId를 가지고 있는 방식 사이에서의 고민이었습니다.
   로그인ID를 가지고 있는 경우에는 별도의 Join이 필요 없다는 장점이 있습니다. 또한 로그인 ID는 비즈니스 정책상 변경이 일어나지 않는 칼럼입니다.
   따라서 비정규화에서 오는 단점 또한 적다고 판단됩니다.
   하지만 만약 api에서 로그인 ID 이외의 사용자 정보가 함께 반환돼야 한다면 별도의 join이 필요하고 이는 pk가 아닌 별도의 로그인 ID 인덱스를 통해서 조인이 발생하게 됩니다.
   로그인 ID는 문자열 인덱스이기 때문에 pk에 비해 조금의 성능 저하가 있을 것으로 보입니다.
   User 엔티티를 @OneToOne으로 연관관계를 맺게 된다면 보다 손쉽게 User 정보에 접근할 수 있다는 장점이 있지만 항상 Join이 발생한다는 문제가 있습니다.
   
   최종 결정은 엔티티 연관 관계 방식을 선택했습니다.
   이유로는 로그인 ID라는 부분 때문입니다. 사용자의 로그인 ID를 외부로 노출시키고 싶지 않습니다. 따라서 주로 화면에는 닉네임으로 표시가 될 것입니다.
   그러면 어차피 로그인 ID를 가지고 있더라도 별도의 조인이 필요해집니다.
   그렇다면 닉네임을 가지고 있으면 되지 않느냐 생각하실 수 있습니다.
   하지만 닉네임은 변경이 가능한 칼럼입니다. 따라서 유저가 자신의 닉네임을 변경하게 되면 일관성을 위해 방장으로 있는 채팅방의 칼럼의 값도 변경해줘야 합니다.
   또한 사용자의 다른 정보가 필요할 수도 있는데 결국은 Join이 발생하는 건 똑같습니다.
   따라서 확장성을 위해 연관 관계 방식을 선택했습니다.
   
   
3.  채팅방 password의 암호화 정도
   암호화도 비용입니다. 채팅방의 password가 그 비용을 써야 할 만큼 중요한지에 대해 고민했습니다.
   채팅방 암호는 민감도가 상대적으로 낮습니다. 유출되면 새로 들어오는 해당 유저를 추방하거나 방을 새로 파면되니까요.
   또한 채팅방은 오래 유지되지 않기 때문에 db에서 과거의 password 혹은 현재 채팅방의 password 유출된다 하더라도 큰 문제가 없을 것으로 생각합니다.
   그렇다 하더라도 조금의 영향이 있을 수 있으니 최소한의 암호화를 적용하기로 결정했습니다. 암호화라기보다는 해싱이 정확하겠네요.
   해싱 방식으로는 SHA-256 해시 함수와 salt를 사용해 처리하였습니다. 완전한 암호화는 아니지만 원본을 그대로 저장하는 것보다는 낫다고 판단했습니다.