[Team-12 BE] 1주차 첫 PR (2022/4/20)

[zbqmgldjfh]

안녕하세요 리뷰어님!

Team12의 BE조원 검봉, Shine 이라고 합니다.
저희 팀의 리뷰를 담당해주셔서 우선 감사의 말 전합니다!

질문

팀원과 Spring Data Jdbc를 사용하던 도중, OneToMany 관계를 이용할 때 Map을 통해 저장하도록 하였었습니다.
Category : item = 1 : N 의 관계로 말이죠!

하지만 해당 Map을 이용할 경우 어그리게이트 루트에 해당되는 Category는 auto_increment가 잘 적용되지만,
item은 auto_increment가 정상작동하지 않았습니다.

혹시 Map을 사용할때는 id값을 직접 지정해줘야 하나요?

@Test
public void itemSaveTest() {
    // given
    Category mainCategory = new Category(CategoryType.MAIN);
    Item newItem = new Item("고기", "맛있는 고기", BigDecimal.valueOf(10000),
            10.0, Badge.EVENT, "풍성한 고기 반찬", 10, BigDecimal.valueOf(100), "html");
    
    // when
    mainCategory.saveItem(newItem); // item의 id가 null
    Category savedCategory = categoryRepository.save(mainCategory); // category는 id생성, but item의 아이디는??
  
    // then
    Category findCategory = categoryRepository.findById(savedCategory.getId()).get();
    List<Item> findItems = findCategory.getItems();
    Item findItem = findItems.get(0);
    assertThat(findItem.getTitle()).isEqualTo("고기");
}

public class Category {

    @Id
    private Long id; // 1L
    private CategoryType type;

    public Category(CategoryType type) {
        this.type = type;
    }

    @MappedCollection(idColumn = "category_id", keyColumn = "id")
    private Map<String, Item> items = new ConcurrentHashMap<>();

    public Long getId() {
        return id;
    }

    public List<Item> getItems() {
        return new ArrayList<>(items.values());
    }

    public void saveItem(Item item) {
        items.put(item.getId(), item);
    }
}

잘 작동하지않아 우선 List로 변경하여 처리하였습니다.

[wooody92]

안녕하세요~ 🙃

질문 주신 부분에 대해 먼저 말씀드리면, 우선 일반적인 List 타입이 아닌 Map으로 설정하려고 했는지 궁금합니다. (단일 쓰레드로 동작하는 것 같은데 일반적인 HashMap이 아닌 ConcurrentHashMap를 설정한 이유도 궁금하구요.)

확고한 이유가 있다면 spring-data-jdbc에서 @MappedCollection 사용 시 어떻게 동작하는지 이해하시고 (또는 Map 사용시에는 어떻게 id auto_increment가 진행되는지) 구글링으로 답을 찾아가시는게 더 도움이 될 것 같네요.

추가로 ERD 작성했다면 PR에 링크를 걸어주시면 앞으로 코드를 파악하는데 더 도움이 될 것 같습니다. (ERD를 작성하지 않았다면 다음 PR부터는 꼭 작성해주세요.)

그럼 수고 많으셨고, 남은 이번 주도 화이팅입니다. 💪

[wooody92]

테스트 용 데이터 초기화를 서비스 코드에서 처리하는 방식은 실제 운영 관점에서 매우 위험해보입니다. 특별한 사유가 없다면 사용하지 않았으면 좋겠네요.

1. profile로 관리한다고하여도 실수 등으로 실제 운영 중인 DB를 초기화 할 수 있음
2. 운영중인 서비스 코드에 테스트 데이터가 들어가는 것은 좋지 않음

[zbqmgldjfh]

배포전 데이터를 삽입하는 용도로만 사용하고, build 전에 삭제하도록 하겠습니다 ㅎㅎ

[wooody92]

pageId는 어떤값인가요? categoryService.findItemsByPageId(pageId) 메서드에서는 categoryId로 사용되는 것으로 보이는데 이름이 달라서요.

[zbqmgldjfh]

원래 생각했던것은 pageId를 통해 페이징을 처리하려 했는데, 구현하고 보니 categoryId를 넘기고 있었습니다... ㅠ,ㅠ....
수정하여 구현은 변경하였습니다!

[wooody92]

이 부분이 좀 난해한데요.. 페이지네이션을 위한 부분일까요??

[zbqmgldjfh]

Spring HATEOAS를 사용하여 REST API의 상태전이를 적용시킨 부분입니다!!
RESTful한 API 작성을 위해 필수적인 부분입니다 ㅠ,ㅠ

[wooody92]

enum 사용 좋습니다~

[zbqmgldjfh]

감사합니다!!!

[wooody92]

schema 처음과 끝 f.k check 설정 변경 부분도 특별한 사유가 없다면 제거해주세요.

[zbqmgldjfh]

Category 에 의존하고 있던 Item을 먼저 drop 하게 순서를 변경하여, f,k check에서 에러가 발생하지 않도록 변경하였습니다!!

[wooody92]

REST Docs 적용 좋습니다. 👏

[zbqmgldjfh]

빌드 설정에서 꼬여 많은 문제를 겪고 있습니다 ㅠ,ㅠ........

[wooody92]

controller 테스트에서는 불필요해보입니다.

[zbqmgldjfh]

해당 테스트의 원래 목적은 데이터 베이스 연동까지 확인해보고 싶었기 때문에 통합 테스트로 이름을 변경해 주었습니다!
변경된 이름은 다음과 같습니다 : CategoryIntergrationTest

실제 데이터를 삽입해보고 싶었기 때문에 @transactional을 추가하였습니다!!

혼동되는 이름 죄송합니다 ㅠ,ㅠ

[wooody92]

이렇게하면 controller test에 사용하지않는 불필요한 빈까지 전부 불러옵니다. @WebMvcTest 에 대해 학습해보시길 추천드립니다.

[zbqmgldjfh]

Slice Test 에 해당되는 Controller 만의 테스트가 아닌 통합테스트를 해보기 위해 이름을 따로 변경해 주었습니다!
데이터까지 직접 들어가서 확인해 보고 싶었거든요 ㅠ,ㅠ
@WebMvcTest 또한 학습하였습니다 !! 감사합니다!

[wooody92]

이 설정들도 필수인가요?

[zbqmgldjfh]

@AutoConfigureMockMvc 는 통합테스트 에서 MockMvc 사용을 위해 추가하였으며,
@AutoConfigureRestDocs 는 필수는 아니지만, Rest doc 작성을 위한 많은 설정을 해주기 때문에 사용하였습니다.

[wooody92]

테스트여도 기본적인 접근제어자는 설정해주세요.

[zbqmgldjfh]

private 로 변경하였습니다!!!

[crispindeity]

안녕하세요 검봉입니다.
우선 바쁘신 와중에 리뷰를 남겨주셔서 감사드립니다.
남겨주신 리뷰 내용 반영하여 다음 PR전 까지 미션 진행하도록 하겠습니다.
다시한번 감사드립니다.