-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: 상품 상세 조회 api 응답 필드 수정 #92
The head ref may contain hidden characters: "87-refactor-\uC0C1\uD488-\uC0C1\uC138-\uC870\uD68C-api-\uC751\uB2F5-\uD544\uB4DC-\uC218\uC815"
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정하시느라 고생하셨습니다!
기획 변경사항 대응이 빠르시군요.
도메인 관련하여 가볍게 코멘트 남겨두었습니다~!
@Embedded | ||
val title: ProductDescriptionTitle, | ||
|
||
@Embedded | ||
val content: ProductDescriptionContent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
값객체로 분리한 이유가 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title, Content 각각 서로 다른 정책에 영향을 받을 것이라 예상했습니다.
가령, Title과 Content 는 글자수 제한이 다를 것으로 예상됩니다. 아직 이러한 정책이 구체적으로 나오지 않아 코드로 구현하지는 않았습니다만, 정책의 내용과 검증 로직이 각 값 객체 내에 존재하는 것이 바람직하다고 판단했습니다. 따라서 값 객체로 분리해두었습니다.
EXAMPLE("상품 예시 이미지"), | ||
DESCRIPTION("상품 상세 정보 이미지"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EXAMPLE
이 상품 캐러셀 이미지에 들어가는 이미지들 같은데 SAMPLE
이런건 어떨까요..?
EXAMPLE
이 예시
가 떠올라서 좀 더 혼동스러운 느낌입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SAMPLE 좋네요 네이밍 감사합니다!
import jakarta.persistence.Id | ||
|
||
@Entity | ||
class ProductDescription( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변경사항이 많아서 분리한걸까요?
product와 table도 분리하신 이유를 더 듣고싶습니다..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순히 변경사항이 많아서 분리한 것은 아닙니다! 분리한 이유는 다음과 같습니다.
Product 는 상품 이름, 가격 등을 포함하는 핵심 도메인입니다. 반면 ProductDescription은 상품에 대한 추가적인 설명입니다. ProductDescription이 Product 에서 분리되었을 때 상품의 핵심 정보와 부수 정보가 확실히 나뉜다고 생각하여 두 테이블을 분리하면 좋겠다고 판단했습니다.
또한 기획자님 말에 따르면 ProductDescription 은 Product 마다 존재할 수도, 존재하지 않을 수도 있습니다. 관련 와이어프레임 등이 아직은 없어 펫프렌즈 어플을 참고 링크로 남겨드립니다.
상품 설명이 존재하지 않는 Product 의 경우 Product Table 내에서 해당 컬럼을 비워두기 보다는, ProductDescription Table 에서 해당 productId에 대한 데이터가 존재하지 않는 것이 데이터를 저장하고 다루는데 있어 더 효율적이라고 느꼈습니다.
한편, 이번 PR에서 ProductDescription의 형식이 Title, Content 로 나뉘면서 기존에 개발되었던 내용과 달라졌는데요. 이러한 변화에 Product 도메인 객체가 영향을 받지 않았으면 하는 마음에 Table을 분리했습니다.
정리하자면, 객체 관심사의 분리, DB 테이블 관리, 의존성 등의 측면에서 table 을 분리하는 것이 더 낫다는 판단을 내렸습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다시 테스트를 해보니, ProductDescription 이 존재하지 않는 경우 상품 상세 보기 API 가 작동하지 않네요! 상품 상세 보기 쿼리에서 Product와 ProductDescripton을 InnerJoin 했기 때문입니다. LeftJoin 으로 수정하고, 데이터가 존재하지 않는 경우 빈 문자열로 반환하도록 수정했습니다.
2f1be1a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
친절한 설명에 버그해결까지... 감사합니다.
의도에 대해 완전 이해했습니다!
현재 Product를 보면 Info, Category, Option, Description 의 부수적인 정보들이 존재하는데요.
저는 그래도 Product라는 Aggregate을 정의한다면, Product
가 루트 도메인이라고 생각해요!
그래서 Category처럼 Description이나 Info 같은 1:1 관계로 설명 가능한 애그리거트들을 Product가 관리하게 하는건 어떨까요?
하나의 컬럼이긴 하지만 앞서 말씀하신 null 필드가 존재할 수 있지만, Description이 nullable 하다는 정보도 Product에서 정의 될 수 있고, DB에서 Description
테이블에 동일한 product_id가 추가적으로 쌓일 수도 있어서요! (물론 방어로직과 unique 조건을 걸면 되긴 합니다!)
또, 봉달에 상품을 추가할 때 해당 description 도 보여주어야 한다고 하면 Cart도메인이 Product에서 해결하지 못하고, ProductDescription 까지 알아야 하는 상황이 생기기도 하고요!
어차피 필요한 데이터들을 모두 join 하는건 마찬가지지만 주최가 Product가 되도록 구성하는건 어떨까 싶습니다..!
Info랑 Option들이 있었는데... 이제와서 의견 제시하는건 죄송합니다,...!🥹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 정성 답변 감사드립니다
1:1 관계로 설명 가능한 애그리거트들을 Product가 관리하게 하는건 어떨까요?
이 부분이 살짝 이해가 가지 않아서요! Product가 해당 엔티티들의 id 를 갖는다는 말씀이실까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다..!
@Entity
class Product(
@Id @GeneratedValue(strategy = GenerationType.IDENTITY)
val id: Long = 0L,
@Column(nullable = false)
val name: String,
@Column(nullable = false)
val categoryId: Long = 0,
val descriptionId: Long = 0,
@Column(nullable = false)
val infoId: Long = 0,
요렇게요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 좋습니다. 그런데 해당 부분 홍고 의견도 듣고 반영 시작할게요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Product가 description등 다른 정보들에 대한 진입점이 되는 거 좋네요! 찬성합니다!
title = "물생활 핵 인싸어, 레드 브론즈 구피", | ||
content = "레드 턱시도라고도 불리며 지느러미가 아름다운 구피입니다" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이미지도 추가하셨던데..
디테일한 테스트케이스입니다..!👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Product에 필드가 굉장히 많아졌네요...! 고생이 많으십니다...
새로 추가된 필드도 너무 깔끔하고 테스트도 깔끔해서 리뷰할 게 없네요...ㅎㅎ
수정사항 전부 반영된 다음에 한 번 더 오겠습니닷...
547e040
to
e2b0c00
Compare
@Column(nullable = false) | ||
val productOptionId: Long, | ||
|
||
val productDescriptionId: Long?, | ||
|
||
@Column(nullable = false) | ||
val productInfoId: Long, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그러고보니 category랑 store의 id는 0L로 초기화가 되어있는데, option이랑 info는 초기화값이 없는 이유가 있나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다 없는 게 맞을 것 같습니다! 수정할게요
bac00f6
to
de638e0
Compare
📌 관련 이슈
📁 작업 설명
펫프렌즈 예시 화면과 같이 Description 은 제목과 내용으로 나뉜다고 합니다(피그마에 반영되어 있지는 않음). 이를 반영하기 위해 Content, Title을 만들었습니다. 그리고 Product 객체가 이러한 변경의 영향을 받지 않도록 Description Entity를 따로 분리했습니다.