[4기 - 김민희, 박세영] JPA 미션 1, 2, 3 PR 제출합니다. #321

onetuks · 2023-08-01T13:41:10Z

springboot-jpa

📌 과제 설명

페어프로그래밍

👩‍💻 요구 사항과 구현 내용

미션1 : 단일 엔티티(Customer)를 이용한 CRUD를 구현
미션2 : 영속성 컨텍스트 생명주기 실습
미션3 : 4-2. 연관관계매핑(order, order_item, item의 연관관계 매핑 실습)

✅ 피드백 반영사항

빌더 패턴 빠진 속성 방어
- Assert.notNull() 로 방어
nullable 생성자 파라미터
- 컬럼에 nullable = false
테이블 이름 컨벤션
- 모두 복수형
영속성 전이
- 영속, 삭제 시 부모-관계 설정한 Order, OrderItem에만 의도한 동작 수행된다고 판단
- Cascade=All + 고아객체 고려 X

✅ PR 포인트 & 궁금한 점

devYSK · 2023-08-04T01:07:21Z

customer.mermaid

@@ -0,0 +1,15 @@
+classDiagram


devYSK · 2023-08-04T01:08:03Z

src/main/java/com/blackdog/springbootjpa/domain/customer/dto/CustomerCreateRequest.java

+        int age,
+
+        @NotBlank
+        @Email


해당 이메일 validation과 우리 요구사항에서의 validation이 다를 수 있습니다.

패턴은 강의에서 사용해봤기 때문에 이메일 validation을 써보고 싶었습니다.
메시지를 지정해주는 방식으로 사용을 명확히 했습니다.

validation message가 없네융

devYSK · 2023-08-04T01:09:15Z

src/main/java/com/blackdog/springbootjpa/domain/customer/dto/CustomerUpdateRequest.java

+        @Size(max = 3)
+        int age,


@min 으로 제한을 걸어도 좋을거 같은데요?

name : !@#~~!@#박세영!@#!@#
age : -999살

이름에는 정규식,
나이에는 @ Min, @ Max 로 걸어두었습니다.

@devYSK min 유저한테 태그걸었심ㅋㅋ

@WooSungHwan
ㅋㅋ ㅋㅋㅋㅋ ㅠㅠ

devYSK · 2023-08-04T01:09:27Z

src/main/java/com/blackdog/springbootjpa/domain/customer/dto/CustomerUpdateRequest.java

+
+public record CustomerUpdateRequest(
+        @NotBlank
+        @Size(max = 50)


메시지도 같이 넘겨주는것은 어떨까요?

수정했습니다.

devYSK · 2023-08-04T01:10:04Z

src/main/java/com/blackdog/springbootjpa/domain/customer/model/Customer.java

+
+    private void validateCustomer(Name name, Age age, Email email) {
+        Assert.notNull(name, "name이 존재하지 않습니다.");
+        Assert.notNull(age, "age가 존재하지 않습니다.");
+        Assert.notNull(email, "email이 존재하지 않습니다.");
+    }
+
+    public void changeCustomer(CustomerDto customerDto) {
+        changeName(customerDto.name());
+        changeAge(customerDto.age());
+        changeEmail(customerDto.email());
+    }
+
+    private void changeName(Name name) {
+        this.name = name;
+    }
+
+    private void changeAge(Age age) {
+        this.age = age;
+    }
+
+    private void changeEmail(Email email) {
+        this.email = email;
+    }
+
+}


private 중간에 public 메소드가 있어 눈에 잘 들어오지 않는다는 단점이 있어요.

규칙을 세우면 어떨까요?

devYSK · 2023-08-04T01:36:37Z

src/test/java/com/blackdog/springbootjpa/domain/item/repository/ItemRepositoryTest.java

+
+@ActiveProfiles("test")
+@DataJpaTest
+@TestPropertySource(locations = "classpath:application-test.yaml")
+@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)


이 어노테이션들. 하나로 묶어서 custom 어노테이션을 사용해도 좋겠네요

devYSK · 2023-08-04T01:36:58Z

src/test/java/com/blackdog/springbootjpa/domain/item/repository/ItemRepositoryTest.java

+@TestPropertySource(locations = "classpath:application-test.yaml")
+@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
+class ItemRepositoryTest {
+


여긴 왜 given-when-then 패턴이 안보일까요!
다른 테스트코드와 통일성을 맞춰주세요

devYSK · 2023-08-04T01:37:23Z

src/test/java/com/blackdog/springbootjpa/domain/item/repository/ItemRepositoryTest.java

+
+    private static final Item item = Item.builder()
+            .price(new Price(1000))
+            .nation(new OriginNation("KR"))
+            .build();
+
+    private static final Item newItem = Item.builder()
+            .price(new Price(2000))
+            .nation(new OriginNation("JP"))
+            .build();


외부에 Test Fixture 클래스를 만들어서 제공해줘도 좋을것같습니다

devYSK · 2023-08-04T01:37:54Z

src/test/java/com/blackdog/springbootjpa/domain/item/repository/ItemRepositoryTest.java

+
+        Item result = repository.findById(savedItem.getId()).get();
+
+        assertThat(result.getNation()).isEqualTo(item.getNation());


레포지토리 레벨에선 id가 존재하는지 여부도 중요할것같네요
identity 전략이니까요

devYSK · 2023-08-04T01:38:25Z

src/test/resources/application-test.yaml

+        show_sql: true
+        format_sql: true
+        dialect: org.hibernate.dialect.MySQLDialect
+    open-in-view: false


open-in-view false가 무엇을 의미할까요?

OSIV전략은 API 응답이 끝날 때까지 영속성 컨텍스트와 데이터베이스 커넥션을 유지하게 해줍니다!
영속성 컨텍스트의 범위가 뷰 단까지 이어져 지연로딩 등의 기능을 이용할 수 있지만, 현재 프로젝트에서는 Controller에 DTO를 넘겨주고 있기에 false로 설정 후 사용하고 있습니다!

@KimMinheee

잘 아시는것 같습니다! 그러면 false로 의도한 이유가 더 궁금해 지네요.
true, false일때는 각각 어떤 trade-off가 있으며 어떤때에 어떻게 사용하는것이 좋을까요?

OSIV를 true로 켜놓는 경우, controller단에서 지연로딩을 사용할 수 있습니다.
다만, OSIV를 true로 켜놓는 경우 Controller 내에서 service로직을 수행할 때 영속성 컨텍스트 내에 있는 값을 변경한 후 @ Transactional이 걸린 로직을 바로 실행하는 경우 변경감지가 동작하여 값이 변경되는 것을 주의해야하며, DB 커넥션을 반환할 때 까지 물고있기때문에 동시 요청이 많은 실시간 트래픽이 중요한 시스템인 경우 장애가 발생할 수 있습니다!

OSIV를 false로 설정하는 경우에는 Controller(뷰 단)에서 지연로딩을 사용할 수 없습니다.
다만, DB 커넥션을 반환하는 시점이 트랜잭션이 끝나는 시점과 동일하여 위에서 언급한 OSIV가 true일 때 발생하는 문제점을 해결할 수 있습니다.

OSIV를 사용하기에 적합한 프로젝트는 서버 사이드 렌더링으로 구성되는 view단을 개발해야하는 경우라고 생각합니다.
현재 프로젝트는 API 기반의 프로젝트이고 Service에서 Controller로 dto를 넘겨주고 있기 때문에 OSIV를 false로 설정하여 개발했습니다!

WooSungHwan

과제하시느라 수고 많으셨습니다 ㅎㅎ VO로 따로 빼서 embeddable을 고려하였다면 반드시 강한 비즈니스적 이유가 있어야하고 그 이유가 VO 안에 고스란히 들어있어야 할겁니다. 그 부분은 꼭 기억하셨으면 좋겠습니다. 단순히 랩핑한다고하고 이유를 들고 끝날 케이스가 아닙니다. 이 부분만 좀 짚고 넘어가시면 나중에 vo로 뺄지 심플하고 컴팩트하게 갈지 결정하기 쉬울 것 같습니다.

수고 많으셨습니다.

WooSungHwan · 2023-08-10T13:30:46Z

src/main/java/com/blackdog/springbootjpa/domain/customer/dto/CustomerCreateRequest.java

+        int age,
+
+        @NotBlank
+        @Email


validation message가 없네융

WooSungHwan · 2023-08-10T13:31:15Z

src/main/java/com/blackdog/springbootjpa/domain/customer/dto/CustomerUpdateRequest.java

+        @Size(max = 3)
+        int age,


@devYSK min 유저한테 태그걸었심ㅋㅋ

WooSungHwan · 2023-08-10T13:31:44Z

src/main/java/com/blackdog/springbootjpa/domain/customer/model/Customer.java

+        Assert.notNull(name, "name이 존재하지 않습니다.");
+        Assert.notNull(age, "age가 존재하지 않습니다.");
+        Assert.notNull(email, "email이 존재하지 않습니다.");


좋네여 ㅎㅎ

WooSungHwan · 2023-08-10T13:32:29Z

src/main/java/com/blackdog/springbootjpa/domain/customer/model/Customer.java

+    private void changeName(Name name) {
+        this.name = name;
+    }
+
+    private void changeAge(Age age) {
+        this.age = age;
+    }
+
+    private void changeEmail(Email email) {
+        this.email = email;
+    }


여기서도 Assert.nonNull 필요하지 않을까요?

WooSungHwan · 2023-08-10T13:37:16Z

src/test/java/com/blackdog/springbootjpa/CustomerServiceTest.java

+        //then
+        Customer customer = customerRepository.findById(response.id()).get();
+        assertThat(response.id()).isEqualTo(customer.getId());
+    }


@devYSK ㅋㅋㅋ 어디서 많이 들은 얘긴데 ㅋㅋㅋ 좋네요

WooSungHwan · 2023-08-10T13:41:46Z

src/test/java/com/blackdog/springbootjpa/CustomerServiceTest.java

+        assertThat(updatedCustomer.getName().getNameValue()).isEqualTo(request.name());
+        assertThat(updatedCustomer.getAge().getAgeValue()).isEqualTo(request.age());
+        assertThat(updatedCustomer.getEmail().getEmailAddress()).isEqualTo(request.email());
+    }


Name, Age, Email 얘네들이 Entity 클래스 외부에서 사용 되어져야할 껀덕지가 있는가? 에 대한 부분도!
엔티티 내부 편의성을 위해 고려된게 아니라면 사실 분리도 그저 분리했다고밖에 이야기 할 수 밖에 없는 이유가 되겠죠. 대표적으로 예를 들자면 age 같은 녀석을 좀더 고립시키고 강한 비즈니스 요구사항이 필요로 해서 뺐다. 그럼 그 강한 요구사항이 저 Age 안에 들어있어야 해요. 근데 지금은 그런게 전혀 보이지 않아요... 그저 raw 데이터를 wrapping 한것 뿐이에요. 그렇게 했다고 객체지향의 근거로 작동할 순 없으니 뭔가 내가 개발하는게 삐끗한게 보였다면 그 원인부터 제거해야하는게 맞을 수도 있어보여요.

onetuks added 7 commits July 31, 2023 01:17

chore: 초기세팅

57b86da

feat: mission#1 단일 엔티티를 이용한 CRUD를 구현

c01b231

feat: mission#2 customer 엔티티를 이용하여 생명주기 실습

3d59d30

feat: mission#3 연관관계 매핑

6298889

refactor: 피드백 반영

a327774

docs: PR 문서 정리

c2afb47

Merge branch 'mission_3' into main

e090c4f

onetuks added Team-Blackdog 🔥 매운맛 labels Aug 1, 2023

onetuks requested review from WooSungHwan and devYSK August 1, 2023 13:41

onetuks assigned onetuks and KimMinheee Aug 1, 2023

devYSK requested changes Aug 4, 2023

View reviewed changes

KimMinheee requested review from byeolhaha and young970 and removed request for byeolhaha and young970 August 8, 2023 07:24

WooSungHwan approved these changes Aug 10, 2023

View reviewed changes

onetuks added 2 commits August 18, 2023 14:47

refactor: 피드백 반영

d0a34d2

docs: 피드백 반영 문서 작업

7cf44c7

onetuks requested a review from devYSK August 18, 2023 06:33

refactor: 피드백 반영

461fcb2

onetuks requested review from devYSK and removed request for devYSK August 24, 2023 03:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4기 - 김민희, 박세영] JPA 미션 1, 2, 3 PR 제출합니다. #321

[4기 - 김민희, 박세영] JPA 미션 1, 2, 3 PR 제출합니다. #321

onetuks commented Aug 1, 2023 •

edited by byeolhaha

Loading

devYSK Aug 4, 2023

devYSK Aug 4, 2023

onetuks Aug 10, 2023

WooSungHwan Aug 10, 2023

devYSK Aug 4, 2023

onetuks Aug 10, 2023

WooSungHwan Aug 10, 2023

devYSK Aug 18, 2023

devYSK Aug 4, 2023

onetuks Aug 10, 2023

devYSK Aug 4, 2023

devYSK Aug 4, 2023

devYSK Aug 4, 2023

devYSK Aug 4, 2023

devYSK Aug 4, 2023

devYSK Aug 4, 2023

KimMinheee Aug 17, 2023

devYSK Aug 18, 2023

KimMinheee Aug 24, 2023

WooSungHwan left a comment

WooSungHwan Aug 10, 2023

WooSungHwan Aug 10, 2023

WooSungHwan Aug 10, 2023

WooSungHwan Aug 10, 2023

WooSungHwan Aug 10, 2023

WooSungHwan Aug 10, 2023


		Item result = repository.findById(savedItem.getId()).get();

		assertThat(result.getNation()).isEqualTo(item.getNation());

[4기 - 김민희, 박세영] JPA 미션 1, 2, 3 PR 제출합니다. #321

Are you sure you want to change the base?

[4기 - 김민희, 박세영] JPA 미션 1, 2, 3 PR 제출합니다. #321

Conversation

onetuks commented Aug 1, 2023 • edited by byeolhaha Loading

springboot-jpa

📌 과제 설명

👩‍💻 요구 사항과 구현 내용

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WooSungHwan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onetuks commented Aug 1, 2023 •

edited by byeolhaha

Loading