Conversation
- 예약 PENDING 상태 제거 (즉시 확정형으로 변경) - 불필요 외래키 제거 (restaurantId는 slot에 포함되어 있으므로)
- 매장별 시간대 슬롯 중복 생성 방지를 위해 (restaurant_id, time) 유니크 제약 추가
- 신규 생성(save) 시 연관관계 매핑을 위해 실제 엔티티의 참조를 가져와서 적용
- 중복 시간대 예약 검즘 - 수량 검증 및 수용 인원 만큼만 차감 - 슬록 미오픈/수용 인원 부족 예외 처리 - 예약 저장 시 DB 유니크 제약 위반을 중복 예약 예외로 변환
- visitAt 기준으로 예약 시점 표현 통합
- 데이터 초기화를 위한 deleteAll() 추가
| User user = userRepository.findByEmail(email) | ||
| .orElseThrow(() -> new UserException(ErrorCode.RESOURCE_NOT_FOUND, "User")); | ||
|
|
||
| RestaurantSlot slot = restaurantSlotRepository.findById(requestDto.getSlotId()) | ||
| .orElseThrow(() -> new ReservationException(ErrorCode.RESOURCE_NOT_FOUND, "RestaurantSlot")); |
There was a problem hiding this comment.
요런 findXXX()성은 항상 Optional을 반환하기 때문에 T로 타입을 변환하는 과정에서 매번 orElseThrow() 코드가 반복됩니다.
보통 find할 때 T를 반환하는 메서드를 실무에서 fetch 라는 표현을 쓰는 편입니다.
fetchByEmail() 같은 메서드를 인터페이스에 정의하고, 하위 구현체에서 없으면 예외를 던지도록 구현해보는 건 어떨까요?
There was a problem hiding this comment.
그동안 예외 처리는 서비스 로직의 책임이라고 생각해 왔는데, 말씀해주신 것처럼 조회 실패 (orElseThrow) 처리는 전제조건 검증에 더 가까운 성격이라는 점에서 repository 계층으로 모으는 방법도 좋은 것 같습니다.
이에 따라 서비스 로직의 가독성과 예외 정책의 일관성을 함께 가져갈 수 있을 것 같습니다.
해당 방향처럼 repository 구현체에서 예외를 던지는 방식으로 수정해보겠습니다.
감사합니다!
| private void validatePartySize(int partySize, RestaurantSlot slot) { | ||
| if (partySize <= 0 || partySize > slot.getMaxCapacity()) { | ||
| throw new ReservationException(ErrorCode.INVALID_PARTY_SIZE); | ||
| } | ||
| } | ||
|
|
||
| private void validateEnoughCapacity(DailySlotCapacity capacity, int partySize) { | ||
| if (!capacity.hasEnough(partySize)) { | ||
| throw new ReservationException(ErrorCode.RESERVATION_CAPACITY_NOT_ENOUGH); | ||
| } | ||
| } |
There was a problem hiding this comment.
규칙을 가진 주체가 도메인에서 판단하고, 결과를 boolean으로 받아 서비스에서 예외 처리를 하는 방식으로 변경해보겠습니다.
| @Override | ||
| public void update(DailySlotCapacity dailySlotCapacity) { |
There was a problem hiding this comment.
JPA는 명시적으로 update 메서드를 호출할 수 없고 save() 메서드 통해 상황에 따라 insert 하거나 변경 감지로 인한 update를 합니다.
따라서 find하고 entity update 하는 로직은 서비스/도메인 레이어에 두는건 어떨까요?
There was a problem hiding this comment.
JPA 변경 감지를 활용하려면 영속 엔티티를 다루는 구조가 필요하다고 생각했지만, 이를 위해 Repository에서 Entity를 반환하는 것이 계층 책임을 흐릴 수 있다는 고민이 있었습니다. 그래서 도메인을 반환한 뒤 다시 엔티티를 조회해 변경하는 흐름이 어색해서 save()와 update() 메서드를 분리했습니다.
이 경우 Respository에서 Entity를 직접 반환하고 서비스에서 변경 감지를 유도하는 방식으로 수정하는 방향도 괜찮은지 궁금합니다!
There was a problem hiding this comment.
이거 제가 잘못 안내드렸네요 ㅠㅠ
보통 멘티 분들이 해당 방식을 어려워 하셔서 일반적인 JPA 방식을 많이 쓰다보니 착오가 있었습니다.
우선 update는 보통 특정 필드만 수정하는 patch 혹은 Input 그대로 수정하는 put으로 구분됩니다.
가령 updateEmail(String email)을 한다면 유저의 이메일만 patch하는 연산이고, update(User user)라면 Input user 그대로 기존 user와 비교하여 달라진 필드를 전부 수정하는 put 연산이 됩니다.
따라서 다음과 같이 정의해보면 좋겠습니다.
Domain Repository Interface는 save(T t), updateXXX(String, int, ...)를 정의합니다.
Infra Repository 구현체 입장에서 전자는 create or put (이를 DB에서는 upsert 라고도 함.)를 구현하기 위해 그저 jpaRepo.save(t)를 호출하면 되고, 후자는 내부에서 엔티티를 setter 등으로 수정하고 jpaRepo.save(entity)를 수행하면 patch가 됩니다. (물론 서비스 레이어에 트랜잭션 활성화되어있다면 인프라 레이어에서 save() 하지 않아도 변경 감지가 되지만, 실수 할 수 있으니 안전하게 넣어주면 좋습니다.)
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "capacity_slot_id", nullable = false) | ||
| private RestaurantSlotEntity slot; |
There was a problem hiding this comment.
capacity_slot_id 필드로 정의하지 않고 연관관계를 사용하신 이유가 있을까요?
There was a problem hiding this comment.
조회 편의성을 위해 연관관계를 사용했습니다.
일자별 수용량(DailySlotCapacity)이 특정 슬롯(RestaurantSlot)에 종속된 개념이고, 조회 시에도 슬롯 기준으로 접근하는 경우가 많을 것이라고 판단했습니다.
다만 ID 참조만 필요한 경우에는 연관관계가 오히려 과할 수 있고, 불필요한 조인이나 프록시 관리 비용이 발생하는 점을 인지하고 있습니다. MVP 이후 사용 패턴이 명확해지면 단순 FK 컬럼 방식으로 전환하는 것도 고려할 수 있을 것 같습니다!
There was a problem hiding this comment.
이번에는 엔티티 간 탐색보다 ID 기반 조합이 더 많을 것으로 예상되어서, 연관관계로 인한 복잡도를 피하고 FK 기반으로 단순한 엔티티 모델을 두도록 수정해보겠습니다.
There was a problem hiding this comment.
엔티티 간 탐색은 오히려 도메인 레이어 쪽에서 활용해보시면 좋겠습니다!
사실 연관관계라는 것이 도메인에서는 다른 도메인을 연관하지만, DB 세계는 연관이 없으니 객체지향의 성질을 위해 도입한 개념입니다. 따라서 도메인/엔티티 구분된 상황에서는 도메인이 다른 도메인을 연관하는 객체 그래프를 구현하는 것이 바람직합니다. (이거도 항상은 아니고 상황에 따라)
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "reservation_slot_id", nullable = false) | ||
| private RestaurantSlotEntity slot; |
|
|
||
| public interface DailySlotCapacityEntityRepository extends JpaRepository<DailySlotCapacityEntity, Long> { | ||
|
|
||
| Optional<DailySlotCapacityEntity> findBySlot_SlotIdAndDate(Long slotId, LocalDate date); |
There was a problem hiding this comment.
현재 연관관계 필드는 slot이고, 그 안에 PK가 slotId여서 JPA 메서드 네이밍 규칙 때문에 이렇게 했습니다.
FK 컬럼으로 두어서 findBySlotIdAndDate만으로도 동작하도록 변경하겠습니다.
| @Override | ||
| public DailySlotCapacity save(DailySlotCapacity dailySlotCapacity) { | ||
| DailySlotCapacityEntity entity = ReservationMapper.INSTANCE.toEntity(dailySlotCapacity); | ||
|
|
||
| entity.assignSlot(em.getReference(RestaurantSlotEntity.class, dailySlotCapacity.getSlotId())); | ||
|
|
There was a problem hiding this comment.
infrastructure 레벨에서 비즈니스의 용어인 "assign" 행위를 담지 않고, 서비스 레벨에서 담당할 수는 없을까요?
There was a problem hiding this comment.
assign 처럼 비즈니스 의미가 담긴 표현이 인프라 레이어에 있는 것이 어색할 수 있어서, 연관관계 세팅은 서비스에서 조립하고 repository에서는 save만 수행하도록 수정해보겠습니다!
| @Override | ||
| public Reservation save(Reservation reservation) { | ||
| ReservationEntity entity = ReservationMapper.INSTANCE.toEntity(reservation); | ||
|
|
||
| entity.assignSlot(em.getReference(RestaurantSlotEntity.class, reservation.getSlotId())); | ||
|
|
||
| ReservationEntity saved = reservationEntityRepository.save(entity); | ||
| return ReservationMapper.INSTANCE.toDomain(saved); | ||
| } |
| @Column(name = "restaurant_name", nullable = false, length = 100) | ||
| private String name; | ||
|
|
||
| @Column(name = "restaurant_address", nullable = false) | ||
| private String address; | ||
|
|
||
| @Column(name = "restaurant_description", columnDefinition = "TEXT") | ||
| private String description; | ||
|
|
||
| @Column(name = "restaurant_main_menu_name", length = 100) | ||
| private String mainMenuName; | ||
|
|
||
| @Column(name = "restaurant_main_menu_price") |
There was a problem hiding this comment.
테이블명이 restaurant인데 매번 필드명마다 restaurant를 꼭 붙여야 할까요?
There was a problem hiding this comment.
말씀처럼 불필요한 중복을 제거하는게 좋을 것 같습니다.
- DailySlotCapacityEntity maxCount 컬럼 제거 - 잔여 좌석 차감 로직 도메인으로 분리
|
1차 리뷰 이후 연관관계 설계에 대해 다시 고민해보았고, 현재 단계에서는 연관관계를 제거하고 FK(id) 기반으로 모델을 단순화하는 방향을 선택했습니다.
|
- 엔티티 변경 감지 기반 업데이트 구조로 수정
|
|
||
| Optional<DailySlotCapacity> findBySlotIdAndDate(Long restaurantSlotId, LocalDate date); | ||
|
|
||
| DailySlotCapacity save(DailySlotCapacity dailySlotCapacity); |
There was a problem hiding this comment.
만약 도메인을 받아서 도메인 값을 그대로 엔티티화하여 update로 쓰고 싶으시다면, create(domain), update(domain)을 나누는 것도 좋다고 봅니다.
이후 비슷한 케이스가 생기면 고려해봐주셔도 됩니다! (지금 꼭 하시라는 건 아닙니다.)
| verifyNoInteractions(dailySlotCapacityRepository, reservationRepository); | ||
| } | ||
|
|
||
| private User createTestUser(Long userId, String email) { |
There was a problem hiding this comment.
https://toss.tech/article/how-to-manage-test-dependency-in-gradle
java test fixture 적용하기 라는 주제로 이슈 하나 생성해 주시고, 나중에 반영 부탁드릴게요! 지금 적용하기에는 PR 성격과는 맞지 않다고 생각했습니다.
개요
메인 리뷰어 지정
리뷰 시 참고 사항
CONFIRMED상태로 확정되도록 정책을 변경했습니다.PENDING)가 길어질 경우, 사용자 입장에서는 예약 성공 여부를 즉시 알기 어렵고 UX가 저하된다고 판단했습니다.DailySlotCapacity에 version 기반 낙관적 락을 적용했습니다. (재시도는 우선 제외했습니다.)TODO
References
체크리스트
관련 이슈