[1단계 - 엔티티 매핑] 디투 미션 제출합니다 #61

shb03323 · 2023-07-07T01:04:42Z

안녕하세요 솔라
바톤 (디투, 주디, 헤나, 에단) 팀 엔티티 맵핑 진행했습니다.
잘부탁드립니다.

solarsalt

1단계 PR에서 2단계 요구사항인 연관관계 매핑까지 한번에 진행하셨네요😅

일단은 1단계 필수 요구사항인 @DataJpaTest를 사용하여 학습 테스트를 해 본다.를 놓치신 것 같아요!
이부분은 수정이 필요합니다.
나머지 코멘트는 학습하실때 도움이 될 만한 고민들입니다. 참고해주시고, 작업자의 생각을 남겨주세요.
1단계 PR에서 2단계 요구사항을 한번에 진행하셔서, 2단계 미션 PR에서는 어떤 작업을 하실 예정이실지 모르겠습니다;;;
- 만약 1,2단계 모두 PR요청-리뷰 단계를 수행하실 거라면,
  이번 1단계 PR에서 2단계 내용을 제외한 작업물로만 PR리뷰를 받으시고, 2단계 작업을 하셔야 할 것 같습니다.

cc @woowahan-pjs 제이슨 다른팀과 일관되게 단계별로 PR이 되도록,코드 수정해야할지 의견 부탁드려용~

solarsalt · 2023-07-09T06:19:55Z

src/main/java/qna/config/JpaConfig.java

+import org.springframework.context.annotation.Configuration;
+import org.springframework.data.jpa.repository.config.EnableJpaAuditing;
+
+@Configuration(proxyBeanMethods = false)


proxyBeanMethods = false 설정을 추가하신 이유가 궁금해요~

저도 궁금해요!

CGLIB의 기능을 추가적으로 이용할 것이 없습니다.

굳이 proxy로 감싸지 않아서 성능상의 이점을 챙길 수 있습니다.

스프링 공식 문서는 수동 컨피규레이션에 proxyBeanMethods = false를 주로 사용하고 있습니다.

solarsalt · 2023-07-09T09:00:00Z

src/main/resources/application.yml

+spring:
+  datasource:
+    url: jdbc:h2:tcp://localhost/~/ethan;MODE=MySql
+    driver-class-name: org.h2.Driver
+    username: sa
+    password:
+
+  jpa:
+    properties:
+      hibernate:
+        format_sql: true
+        dialect: org.hibernate.dialect.MySQL57Dialect
+    show-sql: true
+    hibernate:
+      ddl-auto: create


yml에 있는 정보가 properties 파일에도 그대로 있는데, 2개의 파일에 중복된 정보를 허용한 이유가 궁금해요~

실수로 지우지 못했습니다. 지우겠습니다!

solarsalt · 2023-07-09T09:18:15Z

src/main/java/qna/domain/Question.java

+    @OneToOne(mappedBy = "question")
+    private Answer answer;


질문 1개에는 답변이 1개만 존재하는 가정(1대1연관관계)을 하시게 된 이유가 궁금합니다.

JPA 연관관계를 고민하기 이전에 질문-응답 서비스가 운영 될 flow를 상상해보고
가능한 경우의 수(ex. 질문은 있는데, 답변이 아직 없을수있는가? 질문이 있으면 답변이 여러개일수있는가?
질문이 삭제됐는데 답변만 조회가 가능한가? 등)
를 하나하나 생각해보시면 연관관계 설정에도 도움이 됩니다 :)
특히, 이런 경우의 수는 동료와 함께 생각하면 놓치는 케이스가 적어집니다.

create table question ( id bigint not null auto_increment, contents longtext, created_at datetime(6) not null, deleted bit not null, title varchar(100) not null, updated_at datetime(6), writer_id bigint, primary key (id) ) engine=InnoDB create table answer ( id bigint not null auto_increment, contents longtext, created_at datetime(6) not null, deleted bit not null, question_id bigint, updated_at datetime(6), writer_id bigint, primary key (id) ) engine=InnoDB

저희는 의도하지 않았지만 2단계까지 진행을 했습니다. 2단계 문서를 보지 않고, 저희 마음대로 1대1 관계로 세팅했습니다.

조금 더 꼼꼼하게 보겠습니다.

자율적인 부분이라고 생각했습니다.

solarsalt · 2023-07-09T09:20:20Z

src/main/java/qna/domain/Answer.java

        this.contents = contents;
    }

    public boolean isOwner(User writer) {
-        return this.writerId.equals(writer.getId());
+        return this.writer.equals(writer);


User객체의 equals 를 활용하셨군요!
writer가 갖고있는 어떤 필드를 기준으로 동일한지 판단하는지 궁금합니다~

꼼꼼하지 못했습니다. id 필드를 기준으로 equals & hashcode 를 넣었습니다.

solarsalt · 2023-07-09T09:24:44Z

src/main/java/qna/domain/DateHistory.java

+
+@MappedSuperclass
+@EntityListeners(AuditingEntityListener.class)
+public abstract class DateHistory {


이름의 의도가 DeleteHistory와 네이밍 스타일을 맞추신걸까요?

History는 보통 저장하려는 내용이 이력, log 성격의 데이터일때 postfix로 고려되는 단어이긴하죠.
그런 의미에서는 생성일시와,수정일시만 갖고있는 객체이다 보니
그자체가 이력은 아니라서 History라고 해야할지 조금 고민이 되네요~
조금 더 적합한 이름을 한번 고민해보는 시도도 좋을 것 같아요!

BaseEntity 라는 이름으로 수정했습니다.

solarsalt · 2023-07-09T09:26:27Z

src/main/java/qna/domain/DeleteHistory.java

    private ContentType contentType;
    private Long contentId;
-    private Long deletedById;
+    @ManyToOne
+    @JoinColumn(name = "deleted_by_id")


name을 지정하지 않으면, 코드는 어떻게 동작하게될까요?

현재 코드에서 외래키 이름은 무엇인가요? 요구사항에서 외래키이름을 확인해볼까요?
**외래키를 'deleted_by_id' 컬럼으로 지정하신것은 확인했습니다. 외래키 이름은 별도의 내용입니다.

alter table delete_history add constraint fk_delete_history_to_user foreign key (deleted_by_id) references user (id)

name을 지정하지 않으면, 코드는 어떻게 동작하게될까요?

deleted_by_id로 그대로 나옵니다.

현재 코드에서 외래키 이름은 무엇인가요? 요구사항에서 외래키이름을 확인해볼까요?

외래키 이름을 외래키 제약조건의 이름으로 여쭈어보신걸까요?

@shb03323 네 맞아요 외래키가 fk_delete_history_to_user 이 이름으로 생성되고 있는지 확인해볼까요?

따로 지정하지 않으면 foreign key가 임의의 값으로 지정됩니다.

solarsalt · 2023-07-09T09:34:28Z

src/main/java/qna/domain/DeleteHistory.java

@@ -25,12 +47,12 @@ public boolean equals(Object o) {
        return Objects.equals(id, that.id) &&
                contentType == that.contentType &&
                Objects.equals(contentId, that.contentId) &&
-                Objects.equals(deletedById, that.deletedById);
+                Objects.equals(deletedBy, that.deletedBy);


이전 코드에서는 user의 id만 비교했는데요,
변경 후 코드에서는 user의 id뿐만 아니라 다른 것도 비교하게 되나요?

의도하신 변화인지 궁금했습니다~

이것 또한 id로 비교하도록 수정했습니다.

solarsalt · 2023-07-09T09:39:24Z

src/test/java/qna/domain/AnswerTest.java

+    @DisplayName("생성한다.")
+    @Nested
+    class Create {


@DataJpaTest를 활용해서, 생성한 객체들이 의도한 대로 저장/조회되는지 함께 확인해보는 건 어떨까요?

시행 하겠습니다!

solarsalt · 2023-07-09T09:40:18Z

src/test/java/qna/domain/UserTest.java

+    @DisplayName("업데이트 테스트")
+    @Nested


@DataJpaTest를 활용해 업데이트 기능이 DB를 포함해 잘 동작하는지 확인하는 것은 어떨까요?

시행 하겠습니다!

solarsalt · 2023-07-09T09:43:50Z

src/test/java/qna/domain/UserTest.java

+            User ethan = new User("ethan", "1234", "김석호", "test@test.com");
+            User d2 = new User("D2", "1234", "박정훈", "test1@test.com");
+
+            // when
+            ethan.update(ethan, d2);


실제 이름을 활용하는 방법도 있지만, 기대값과 기존값을 쉽게 확인하기 위해
expected, actual 같은 변수명을 활용할 수도 있습니다.

수정하겠습니다!

solarsalt · 2023-07-10T07:40:10Z

@shb03323
안녕하세요 바톤팀~
1,2단계 요구사항을 한번에 PR요청 주셔서, 제가 2단계내용까지 코멘트를 이미 한 상태입니다.

아래 3가지관련 코멘트는 필수로 확인해주셔야 하고, 나머지는 코멘트는 학습하시는데 참고해주세요~

1단계 요구사항인 @DataJpaTest를 활용한 테스트 포함하기
OneToOne 연관관계로 결정한 이유를 소개해 주세요.
delete_history 테이블의 외래키 이름을 확인부탁드려요.

shb03323 · 2023-07-11T23:19:41Z

안녕하세요 솔라 바톤팀입니다.
페어 프로그래밍을 하면서 요구 사항에 대해 궁금한 점이 있습니다.

create table question
(
    id         bigint generated by default as identity,
    contents   clob,
    created_at timestamp    not null,
    deleted    boolean      not null,
    title      varchar(100) not null,
    updated_at timestamp,
    writer_id  bigint,
    primary key (id)
)

1단계 요구 사항의 question 테이블인데요. 왜 fk에 null이 허용되게 하셨는지 궁금합니다. 이 테이블 뿐만 아니라 모든 테이블 다요!
혹시 테스트에서 해당 도메인만 테스트할 수 있도록 하신 것을 의도하신 걸까요?
만약 그렇다면, 테스트를 할 때만 null이 들어가도록 허용하는 것은 테스트를 위해 프로덕션 코드를 불안정하게 만든 것 같다는 생각이 들어서요.

저희가 이런 고민을 한 이유는, 뼈대 코드에서 Question에서는 writer를 넣을 때 null을 허용했지만, Answer에서는 writer가 null인 경우 예외를 발생하도록 했기 때문입니다.

solarsalt · 2023-07-14T07:07:06Z

안녕하세요 솔라 바톤팀입니다. 페어 프로그래밍을 하면서 요구 사항에 대해 궁금한 점이 있습니다.
create table question
(
    id         bigint generated by default as identity,
    contents   clob,
    created_at timestamp    not null,
    deleted    boolean      not null,
    title      varchar(100) not null,
    updated_at timestamp,
    writer_id  bigint,
    primary key (id)
)
1단계 요구 사항의 question 테이블인데요. 왜 fk에 null이 허용되게 하셨는지 궁금합니다. 이 테이블 뿐만 아니라 모든 테이블 다요! 혹시 테스트에서 해당 도메인만 테스트할 수 있도록 하신 것을 의도하신 걸까요? 만약 그렇다면, 테스트를 할 때만 null이 들어가도록 허용하는 것은 테스트를 위해 프로덕션 코드를 불안정하게 만든 것 같다는 생각이 들어서요.

저희가 이런 고민을 한 이유는, 뼈대 코드에서 Question에서는 writer를 넣을 때 null을 허용했지만, Answer에서는 writer가 null인 경우 예외를 발생하도록 했기 때문입니다.

1단계에서는 not null 제약조건이 없는 것이 의도된 상태가 맞습니다. 3단계에서 리팩토링 미션을 하시면서 이와 관련한 문제를 만나게 될 예정이니, 지금은 1단계 요구사항에만 집중해주셔도 됩니다~!

solarsalt · 2023-07-14T07:10:25Z

@shb03323
1,2단계의 필수 요구사항을 모두 완료하고, 아래 사항도 완료하셨을때 다시 한번 리뷰요청 부탁드려요!

1단계 요구사항인 @DataJpaTest를 활용한 테스트 포함하기
OneToOne 연관관계로 결정한 이유를 소개해 주세요.
delete_history 테이블의 외래키 이름을 확인부탁드려요.

shb03323 · 2023-07-17T23:22:49Z

안녕하세요 솔라 말씀해주신 부분 수정했습니다!

1단계 요구사항인 @DataJpaTest를 활용한 테스트 포함하기

기존에 RepositoryTestConfig 안에 @DataJpaTest 어노테이션을 설정하여 모든 Repository test를 진행했습니다.

OneToOne 연관관계로 결정한 이유를 소개해 주세요.

게시글의 경우 여러개의 댓글이 달릴 수 있지만, qna인 경우에는 하나의 질문에 하나의 답변만 남길 수 있다고 생각했습니다.

delete_history 테이블의 외래키 이름을 확인부탁드려요.

수정했습니다!

solarsalt

@DataJpaTest를 활용한 테스트 포함하기
기존에 RepositoryTestConfig 안에 @DataJpaTest 어노테이션을 설정하여 모든 Repository test를 진행했습니다.

RepositoryTestConfig 관련한 코멘트를 여러개의 테스트 클래스에 남겼습니다. 아무래도 2단계에서 상속, 불필요한 디펜던시가 있는 경우에 대한 고민을 하고 가시면 더 성장하실 수 있을 것 같아서 request change 요청을 드렸습니다 😎
이부분에 대한 충분한 고민을 해보시고, 팀의 의사결정대로 코드변경이 있으면 다시 리뷰 요청 부탁드릴게요
리뷰요청시 저를 멘션 @solarsalt 해주시면 더 좋습니다!

OneToOne 연관관계로 결정한 이유를 소개해 주세요.
게시글의 경우 여러개의 댓글이 달릴 수 있지만, qna인 경우에는 하나의 질문에 하나의 답변만 남길 수 있다고 생각했습니다.

이유 설명해주셔서 감사해요😊~ 팀에서 생각하신 가정도 충분히 가능할것 같습니다!
저는 stackoverflow같은 서비스를 상상해 n개의 답변이 자연스럽다고 생각했습니다.
+학습측면에서는 question 1: answer N 개를 가정해보시는 것이 좀더 복잡한 케이스를 고려한 학습 기회가 될 수 있을 것 같으니 다음 단계를 구현하실 때 참고해주세요. 물론 지금처럼 1:1을 유지하셔도 됩니다.

delete_history 테이블의 외래키 이름을 확인부탁드려요.

잘 적용해주신 것 확인 했습니다 💯 점점 JPA 기능에 익숙해지시고 있네요!

solarsalt · 2023-07-18T07:44:13Z

src/test/java/qna/repository/AnswerRepositoryTest.java

+        User user = userRepositoryFixture.save("hyena", "1234", "헤나", "example@example.com");
+        Question question = questionRepositoryFixture.save("질문이 뭐에요?", "저도 모르겠어요.");


User user = userRepositoryFixture.save("hyena", "1234", "헤나", "example@example.com"); Question question = questionRepositoryFixture.save("질문이 뭐에요?", "저도 모르겠어요.");

테스트 클래스 내에 중복된 코드를 줄이면 더 가독성이 좋은 코드가 될 것 같아요~

테스트하려는 대상은 answer인데, user와 question 을 의존하고 있습니다.
answer테스트를 위해 연관된 객체를 매번 의존하지 않을 수 있는 방법이 있을까요?

solarsalt · 2023-07-18T07:54:50Z

src/test/java/qna/repository/QuestionRepositoryTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class QuestionRepositoryTest extends RepositoryTestConfig {


RepositoryTestconfig를 상속받아서QuestionRepositoryFixture를 갖고있는 상태인데, 테스트 대상은 questionRepository 으로 새롭게 설정하신 이유가 궁금합니다.

추측컨데 @DataJpaTest 어노테이션은 적용하고 싶은데, QuestionRepositoryTest 클래스에서는
QuestionRepository 외에 다른 Repository는 불필요한 상황인 것으로 보여요~
사용하지 않는 의존성을 포함하는 클래스는 어떤 문제가 있을까요? 별 문제가 없다고 판단하셨다면, 이유도 궁금합니다.

solarsalt · 2023-07-18T07:56:30Z

src/test/java/qna/repository/UserRepositoryTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class UserRepositoryTest extends RepositoryTestConfig {


해당 클래스도 RepositoryTestConfig클래스로 많은 의존성을 갖고있지만,
테스트 대상코드에서는 하나도 활용되지 않고 있네요~

활용하는 코드가 없다면, 상속을 제거하거나

RepositoryTestConfig클래스를 좀 더 날씬하게 변경하거나

하는 선택이 필요할 것 같습니다.

solarsalt · 2023-07-18T07:57:58Z

src/test/java/qna/repository/config/RepositoryTestConfig.java

+    protected UserRepositoryFixture userRepositoryFixture;
+    protected QuestionRepositoryFixture questionRepositoryFixture;
+    protected AnswerRepositoryFixture answerRepositoryFixture;
+    protected DeleteHistoryRepositoryFixture deleteHistoryRepositoryFixture;


4개 repo를 모두 의존하는 test class가 있는지 궁금합니다.
없다면, 이 클래스의 역할을 객체지향관점에서 다시 고민해보시면 좋을 것 같습니다.

solarsalt · 2023-07-18T07:59:28Z

src/main/java/qna/domain/Answer.java

+    @JoinColumn(name = "writer_id", foreignKey = @ForeignKey(name = "fk_answer_writer"))
    private User writer;
    @ManyToOne
-    @JoinColumn(name = "question_id")
+    @JoinColumn(name = "question_id", foreignKey = @ForeignKey(name = "fk_answer_to_question"))


외래키 이름을 잘 적용해주셨네요 👍

shb03323 added 8 commits July 7, 2023 08:23

feat: properties 설정

4cf81f7

feat: jpa config

75a7bad

feat: createdAt, updatedAt 객체 생성

83ae245

feat: 테이블 엔티티 생성

b120040

refactor: 레포지토리 패키지 분리

dfdcc30

refactor: 설정파일을 yml로 변경

22ae0f7

test: User update 메서드 테스트 코드 작성

becfbb7

test: 질문과 답변에 대한 테스트 코드 작성

e97691e

solarsalt self-requested a review July 7, 2023 13:12

solarsalt requested changes Jul 9, 2023

View reviewed changes

shb03323 added 8 commits July 11, 2023 09:31

refactor: 생성일자, 수정일자 entity 명 변경

6b9960a

refactor: equals and hashcode 재정의

a5a37c3

refactor: 연관관계 매핑 수정

94b5d1d

refactor: application.properties 삭제

e63ad95

test: 사용자 id 조회 테스트 작성

e7b59be

test: 삭제되지 않은 질문 조회 테스트 작성

c2322de

test: 삭제되지 않은 질문중에서 식별자로 조회 테스트 작성

857df74

chore: 테스트 설정 파일에 sql 로깅 추가

6aa5168

shb03323 added 10 commits July 12, 2023 08:21

refactor: question 엔티티의 작성자를 변경하지 못하도록 설정

ea2793e

refactor: 엔티티에 필요없는 세터 제거

112e461

test: repository fixture 파일 생성

7f7ca56

test: 삭제되지 않은 답변 중에서 질문을 식별자로 조회하는 테스트 작성

510c055

test: 질문 식별자로 삭제되지 않은 답변 조회 테스트 작성

a6f69f4

refactor: repository 테스트 패키지 구조 변경

672844c

feat: Question equals & hashcode 재정의

3ff066e

test: Question 테스트 변수명 수정

6a829f3

test: Answer 엔티티 테스트 사용하지 않는 변수 제거

fa203a4

test: 답변의 식별자로 답변을 검색할 때, 삭제된 경우 검색 안되는 테스트 작성

099dda3

shb03323 added 3 commits July 13, 2023 08:42

test: 레포지터리 Fixture 작성

b5af277

test: Answer 레포지터리 테스트 Fixture 적용

19a9bea

refactor: 레포지토리 test 패키지 구조 변경

0b0de7d

shb03323 added 2 commits July 18, 2023 08:14

refactor: entity 외래키 제약 조건 이름 설정

9b8e2af

test: 모든 data jpa test를 RepositoryTestConfig 상속하도록 함

32ca40f

solarsalt requested changes Jul 18, 2023

View reviewed changes

		User user = userRepositoryFixture.save("hyena", "1234", "헤나", "example@example.com");
		Question question = questionRepositoryFixture.save("질문이 뭐에요?", "저도 모르겠어요.");


		import static org.assertj.core.api.Assertions.assertThat;

		class QuestionRepositoryTest extends RepositoryTestConfig {


		import static org.assertj.core.api.Assertions.assertThat;

		class UserRepositoryTest extends RepositoryTestConfig {

[1단계 - 엔티티 매핑] 디투 미션 제출합니다 #61

Are you sure you want to change the base?

[1단계 - 엔티티 매핑] 디투 미션 제출합니다 #61

Conversation

shb03323 commented Jul 7, 2023

solarsalt 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

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

solarsalt Jul 9, 2023 • edited Loading

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

solarsalt commented Jul 10, 2023

shb03323 commented Jul 11, 2023 • edited Loading

solarsalt commented Jul 14, 2023

solarsalt commented Jul 14, 2023

shb03323 commented Jul 17, 2023

solarsalt 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

solarsalt Jul 9, 2023 •

edited

Loading

shb03323 commented Jul 11, 2023 •

edited

Loading