[레거시 코드 리팩터링 - 3단계] 오찌(오지훈) 미션 제출합니다. #363
Conversation
| public AlreadyGroupedException() { | ||
| super("이미 단체 지정한 테이블을 단체 지정할 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
CustomException 을 만들고 메시지까지 만들어주셨네요..!!
여기서 궁금한 점은 각각의 도메인과 관련된 예외라는 생각이 들어서 도메인들과 같은 패키지에 분리를 해놓으면 현재보다 예외들의 파악이 쉬울 것 같다는 생각이 들었어요!
예를 들어, OrderLineItemNotExistsException, CompletedOrderCannotChangeException 들이 Order 와 함께 묶여있으면 지금보다 Order 와 관련해서 이러한 예외들이 발생할 수 있구나를 파악하기 쉬울 것 같아요!
오찌의 생각은 어떠신가요??
There was a problem hiding this comment.
사실 이 부분은 고민을 많이 했는데요, 도메인에서 던지지 않고 서비스 레이어에서 던지는 예외들도 분명히 있고, 서로 다른 도메인 컨텍스트 간에 호출해야 하는 예외들도 있기 때문에 좀 더 유연하게 호출하기 위해서 아예 패키지를 분리했습니다.
| @@ -11,6 +11,7 @@ | |||
| import org.springframework.transaction.annotation.Transactional; | |||
|
|
|||
| @Service | |||
| @Transactional(readOnly = true) | |||
There was a problem hiding this comment.
@Transactional(readOnly=true) 를 클래스 레벨에 선언해주셨네요!!
이전과는 어떤 차이가 있을까요??
There was a problem hiding this comment.
@Transactional이 없는 조회 메서드에서는 OSIV가 꺼져 있으면 지연 로딩을 사용할 수 없는 문제가 있더라고요. 물론 저는 @BatchSize를 사용하기도 하고, OSIV도 기본 설정대로 켜져 있기도 하지만 혹시 모를 버그를 대비해 선언해주었습니다.
There was a problem hiding this comment.
OSIV 가 꺼져있으면 SimpleJpaRepository에 걸린 @Transactional 에서만 트랜잭션이 걸려서 조회가 끝나고 나서는 지연로딩이 안되는군요!!
| CREATE TABLE order_status_record | ||
| ( | ||
| order_id BIGINT(20) NOT NULL, | ||
| order_table_id BIGINT(20) NOT NULL, | ||
| order_status VARCHAR(255) NOT NULL, | ||
| PRIMARY KEY (order_id) | ||
| ); |
There was a problem hiding this comment.
주문의 상태를 저장하기 위한 새로운 테이블이 추가되었네요!
실제 Order의 status와 불일치하게 되면 어떻게 되나요???
Order 에도 order의 status 가 있는데 외래키로 걸어주지 않은 이유도 궁금합니다!
There was a problem hiding this comment.
실제 Order의 status와 불일치하게되면 버그가 나게 되겠네요 :) 이 부분은 배치 스케줄 처리든 다른 방식으로 정합성을 맞춰주는게 좋다고 생각합니다! Order의 status와 외래키를 걸지 않은 것은,
- 외래키 자체가 성능이 좋지 못함
- OrderStatusRecord가 Order를 알 필요가 없음
의 이유가 있을 것 같아요!
| @RestControllerAdvice | ||
| public class GlobalControllerAdvice { |
| import kitchenpos.domain.table.OrderTableRepository; | ||
| import kitchenpos.domain.table.TableGroup; |
There was a problem hiding this comment.
도메인 연관관계 포함해서
Menu -> Product
Order -> Menu
Order -> Table
으로 잘 끊어내주셨네요!!
| @Target(ElementType.TYPE) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @DataJpaTest | ||
| @AutoConfigureTestDatabase(replace = Replace.NONE) |
There was a problem hiding this comment.
replace=NONE 으로 설정해주신 이유가 궁금합니다! 지금은 별도의 데이터 소스를 지정해주지 않아서 여전히 Any 와 같이 동작할 것 같아서요!! (제가 잘 몰라서 여쭤봅니다...😅)
There was a problem hiding this comment.
Any를 하게 되면 매번 새 데이터베이스를 생성하기 때문에 어차피 테스트 격리가 잘 되고 있는 입장에서 낭비라고 생각했습니다 :)
| @Test | ||
| void 주문에_주문_항목이_존재하지_않으면_400() throws Exception { |
There was a problem hiding this comment.
400 응답에 대한 Controller 테스트를 Mocking 을 통해서 작성해주셨네요!!
바쁘신 와중에도 꼼꼼하게..👍👍
| List<Menu> menus = findMenus(orderCreateRequest.getOrderLineItems()); | ||
| OrderTable orderTable = orderTableRepository.findById(orderCreateRequest.getOrderTableId()) | ||
| .orElseThrow(OrderTableNotExistsException::new); | ||
| List<OrderLineItem> orderLineItems = orderLineItemMapper | ||
| .toOrderLineItems(orderCreateRequest.getOrderLineItems(), menus); | ||
| Order order = orderRepository.save( | ||
| orderMapper.toOrder(orderCreateRequest, orderLineItems, orderTable.isEmpty())); | ||
| orderStatusRecordRepository.save(new OrderStatusRecord(order.getId(), orderTable, order.getOrderStatus())); | ||
| return orderDtoMapper.toOrderResponse(order); |
There was a problem hiding this comment.
단방향으로 잘 끊어내주셨는데, 2단계를 진행하며 간단해졌던 Service 가 굉장히 무거워진 느낌이 들어요..ㅠ
이전에는 "검증, 생성, 저장, 반환" 이 한 눈에 들어온 반면 지금은 어떤 일을 하는지 파악하기 힘들어진 것 같아요..
어떻게 개선해볼 수 있을까요..ㅠㅠ
There was a problem hiding this comment.
굉장히 깔끔해졌네요!👍👍
다만, createAndSaveOrder 네이밍에서 두 가지 일을 한다는게 느껴져서 추후에 4단계 진행하신다면 조금 더 개선해볼 수는 있겟네요!!
| public Order(final Long orderTableId, final List<OrderLineItem> orderLineItems, final boolean orderTableEmpty) { | ||
| this(null, orderTableId, OrderStatus.COOKING, LocalDateTime.now(), orderLineItems, orderTableEmpty); | ||
| } | ||
|
|
There was a problem hiding this comment.
Order와 OrderStatusRecord 양쪽에서 사용하기 위함입니다! 우발적 중복이 아니냐고 볼 수도 있는데, 저는 이 부분들이 같은 상태라고 보기 때문에 의도적인 중복이라고 판단하고 common에 두어서 재활용할 수 있도록 했습니다 :)
| @ManyToOne(fetch = FetchType.LAZY, optional = false) | ||
| @JoinColumn(name = "order_table_id", nullable = false) | ||
| private OrderTable orderTable; |
There was a problem hiding this comment.
하나의 패키지(table) 안에 객체 사이에서도 최대한 연관관계를 제거해 객체간의 양방향 연관관계를 제거할 수 있으면 좋을 것 같아요!!
OrderstatusRecore 가 orderTable 을 알지 않아도 될 것 같습니다!
그렇다면 OrderTable 에서 일대달로 가져 update 쿼리가 나가는 사이드 이펙트가 있는데요! 어떻게 해결할 수 있을까요??
각각의 엔티티도 별도의 영속성이 관리되는 것이 아니라 하나의 영속성으로 관리되도록 값 컬렉션을 활용해보면 어떨까요?? (주문 id 와 orderStatus 를 값으로 가지는)
There was a problem hiding this comment.
값 타입 컬렉션은 결정적인 문제가 있는데요, 식별자가 없다 보니 값을 수정할 때 데이터베이스에 모든 값을 전부 delete하고 다시 수정된 값을 전부 insert한다는 문제가 있습니다. 때문에 값 타입 컬렉션을 엔티티로 대체하는 것이 권장되기도 합니다 :)
객체지향적인 관점에서 봐서는 일대다 단방향 매핑을 하는 것이 맞지만, JPA를 많이 쓰던 입장에서 일대다 단방향은 선뜻 손이 나가지 않더라고요... 어차피 같은 컨텍스트 내에 있으니 부득이하게 양방향 매핑을 하더라도 큰 문제는 없을거라 생각했습니다!
There was a problem hiding this comment.
맞습니다!!
저희 팀프로젝트에서도 해당 문제를 겪었었는데요, 해결 방법으로는 @ElementCollection 에서 다쪽에 @Column(nullable = false) 를 주고 컬렉션을 Set 을 사용해서 유니크하면서 null이 포함되지 않음을 명시해주면 모든 값을 전부 delete 하고 다시 수정된 값을 전부 insert한다 를 해결할 수 있었습니다!!
저는 개인적으로는 JPA 매핑을 위한 불필요한 양방향 관계보다는 객체지향적으로 설계하는것이 유지보수에도 좋다고 생각해서 해당 방법으로 프로젝트를 진행하고 있지만 오찌가 말씀해주신 것처럼 확실히 불편한 부분(?) 혹은 한계는 있는 것 같아요ㅠ.ㅠ
안녕하세요 디우! 원래 3, 4단계를 같이 해보려고 했는데, 4단계는 고민할 부분이 조금 남기도 했고, 멀티 모듈로 분리하게 되면 필연적으로 file changes가 많아지기 때문에 3단계만 하고 제출해보겠습니다 :)
전체적으로 의존성은 다음과 같습니다.
레이어 별
presentation(controller) -> application(service) -> domain(repository)
컨텍스트 별
order(Order, OrderLineItem) -> menu(MenuGroup, Menu, MenuProduct) -> product(Product)
order -> table(TableGroup, OrderTable)
해결하기 힘들었던 문제는 order 컨텍스트와 table 컨텍스트가 양방향 의존하고 있다는 문제였는데요,
table -> order 쪽 검증 로직을 이벤트 발행을 통해 의존성을 강제로 끊어냈습니다.OrderTable 하위에 Order의 정보를 담는 엔티티 OrderStatusRecord를 만들고, Order 생성 시점에 OrderStatusRecord를 만들어주고, OrderTable이나 TableGroup을 변경할 때 검증 로직을 OrderStatusRecord를 이용하도록 하여 끊어냈습니다. order -> table 쪽이 아니라 table -> order 쪽을 끊어낸 것은, table 컨텍스트는 생성 시점에 order의 존재를 알지 못하지만, order 컨텍스트는 생성 시점에 table 컨텍스트의 존재를 알아야 하기 때문입니다.우선은 이렇게 풀어내봤는데, 좋은 방법일지는 조금 더 고민해보도록 하겠습니다 :) 현재 상황에서는 생각나는 가장 좋은 방법이어서 이렇게 구현해보았습니다!
남은 과제로는, dto의 의존성이 양방향이라는 문제입니다. 현재 dto가 서비스 단까지 내려오고 있는데, requestDTO의 경우 생성 책임이 presentation 레이어에 있기 때문에, presentation과 application 레이어가 엄밀히 말해서는 의존성 사이클이 돌고 있다는 문제가 있습니다. 다만 이 부분을 굳이 해결해야 할 지에 대한 고민을 좀 더 해보고, 분리할 필요성이 있다면 presentation 레이어와 application 레이어에서 사용하는 dto를 분리해 볼 생각입니다.
이번 단계에서도 좋은 리뷰 부탁드려요!