Skip to content

Conversation

@gongsua
Copy link
Collaborator

@gongsua gongsua commented Nov 10, 2025

📌 Related Issue

🚀 Description

  1. 로그인한 사용자는 게시물을 업로드할 수 있습니다.
  2. 비회원이라도 게시물 목록은 조회가능합니다.
  3. 게시물 클릭 시 게시물 상세조회가 가능합니다.

프론트 git :
https://github.com/f-lab-edu/buying-front

📸 Screenshot

  1. 게시글 목록
image
  1. 게시글 상세 내용
image
  1. 게시글 등록 페이지
image

📢 Notes

  • 감사합니다!

@gongsua gongsua requested a review from f-lab-ted November 10, 2025 15:59
@gongsua gongsua self-assigned this Nov 10, 2025
@gongsua gongsua linked an issue Nov 10, 2025 that may be closed by this pull request
Copy link

@f-lab-ted f-lab-ted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전반적으로 Post - PostDetail 구성이 업계에서 가지는 일반적인 관점과 조금 미묘하게 다른 것 같습니다.

제가 추측하기로는 Post 엔티티를 목록 조회용으로 사용하고, 상세 내용을 PostDetail로 보여주려는게 아닐까? 싶긴한데, 만약 맞다면 이건 전형적으로 데이터와 서비스 모델을 프레젠테이션(UI) 구성 맞춘 방식이라고 할 수 있을 것 같습니다.
보통 서비스 모델 -> 데이터 모델 -> 그리고 이에 맞춰서 프레젠테이션 모델을 설계하는데요, 현재 구현에서는 그 반대가 되어있는 것 같아요. 프레젠테이션 모델은 언제든지 변경될 수 있기 때문에, 서비스/데이터 모델을 먼저 설계 하고 조금 불편해 보이더라도 프레젠테이션 모델을 끼워 맞추는 식으로 설계하는게 맞습니다.

그리고, 아래 코멘트에 남긴 것처럼 Post - PostEntity 간 관계가 데이터 모델에서와 서비스 로직에서 불일치 하는 부분이 있는데 이 부분도 정리가 필요할 것 같습니다.

코드를 수정하기에 앞서, 다음 작업을 먼저 수행해주시면 좋을 것 같습니다.

  1. 서비스/데이터 레이어에서 각 엔티티의 설계 목적 그리고 관계 정리
  2. 엔티티와 대응되는 DTO 모델의 설계 이유 정리 (필드 구성이 왜 이렇게 되는지)

그리고 나서 코드를 어떻게 개선할지 혹은 생각하신대로 끌고 나갈지 판단해볼 수 있을 것 같습니다.

제가 남긴 리뷰 중에서 제가 잘 못 이해한 부분이 있다고 생각되시면, 코멘트 남겨주세요!


@Getter
@RequiredArgsConstructor
public enum PostErrorCode {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 에러 코드 체계화 한 부분 좋습니다!


@Getter
@RequiredArgsConstructor
public enum ResponseCodePostAndMessage {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGEST] 요청의 성공과 관련해서는 message를 전달할 꼭 필요가 있을지 고민해보면 좋을 것 같습니다.
요청 처리 실패 시에는 여러 실패 가능한 케이스 중 실제로 실패한 사유(메세지)를 돌려주는게 완성도 높은 API 구현을 위해 필요하지만,
처리 성공은 성공이라는 한 가지 케이스이므로 그냥 요청에 대한 응답 데이터를 돌려주는게 일반적입니다.
부가적인 메세지 전송은 클라이언트 - 서버 간 네트워크 전송량을 불필요하게 높이는 것이 아닐까 하는 생각이 드네요!

Long id,
String nickname
) {
public static MemberCardDto from(Member member) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변환을 위한 정적 메소드 활용, 좋습니다 👍

String content,
Integer price,
Integer quantity,
List<String> images

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] 이미지 등록(업로드)에 대한 부분은 찾아 볼 수가 없는데, 어떤식으로 구현할 계획이실까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이미지는 Poat서비스에 savePostImagesIfExists로 구현했습니다
첫번째 장은 썸네일용으로 저장하고 테이블에는 이미지 순서번호를 부여해 저장합니다.

다만 Text 로 변경하였는데도, 이미지 url이 길어져, s3에 저장하고 s3주소를 받아오도록 리팩터링해야합니다..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gongsua
이미지 URL 보다는, S3 Key 값을 저장하도록 해보세요.
그리고, 실제 URL을 전달하는 것이 아니라, Presigned URL을 제공하도록 해보세요.
Presigned URL 개념이 익숙하지 않다면, 어떤 개념이고 왜 필요한지 조사해서 정리해보세요~!
Presigned URL을 사용하기 위해서 S3 Key 값을 DB에 저장해야하는 이유도 함께 정리해보면 좋을 것 같습니다.

String thumbnailUrl = extractThumbnail(dto);
Post post = Post.create(member, dto.title(), dto.price(), thumbnailUrl);
postRepository.save(post);
savePostDetailIfExists(dto, post);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[REQUIRED]
Post : PostDetail 엔티티간 관계는 1:N인데, PostCreateRequestDto 모델과 서비스 로직은 이와 일치하지 않는 것 같아요.
이는 코드의 일관성을 떨어뜨려서 유지보수에 영향을 줄 수 있으므로 DTO 모델과 서비스 로직도 엔티티 모델과 일치시키는 것이 좋을 것 같습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostDetail 테이블을 삭제하고 Post 로 합쳤습니다..!


public PostDetailResponseDto getPostDetail(Long postId) {
Post post = postRepository.findById(postId)
.orElseThrow(() -> new IllegalStateException(PostErrorCode.POST_NOT_FOUND.getMessage()));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[REQUIRED]
IllegalStateException 은 Java 표준 예외이므로 Service 레이어에서 사용하기엔 적절하지 않은 것 같아요.
만약 서비스 동작 중에 IllegalStateException 이 발생했다는 기록을 확인하게 되면, 이 문제가 postRepository.findById(postId) 에서 발생한 것인지 postDetailRepository.findByPostId(postId) 에서 발생한 것인지 직관적으로 파악하기가 어렵습니다!
저수준 레벨의 코드를 작성하거나 공용 라이브러리 코드를 작성할 때는 java의 표준 예외를 활용하는 것이 best practice일 수 있는데, service 레이어에서는 상황별로 정의한 custom exception을 발행하여 문제 상황이 좀 더 명시적으로 드러나도록 해주세요!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고, 더불어서 Member 도메인에서는 BusinessException을 common 서브 패키지에 선언해두고 예외 상황에 BusinessException을 던지고 있는데, 이 부분도 도에인 서브 패키지(member, post) 별로 하위에 exception 서브 패키지를 생성하고 그 하위에 BusinessException을 상속받는 커스텀 예외(ex. PostNotFoundException)을 선언하여 service 레이어에서 활용할 수 있도록 해주세요.
또 한가지, 도메인 서브 패키지 별로 global exception handler를 도입해서 도메인 별로 발생시키는 커스텀 예외를 꼼꼼히 처리할 수 있도록 해주세요!

Copy link
Collaborator Author

@gongsua gongsua Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@f-lab-ted
PostNotFoundException 이런식으로 만들라는 뜻이 혹시

public class PostNotFoundException extends BusinessException {
    public PostNotFoundException() {
        super(PostErrorCode.POST_NOT_FOUND);
    }
}

이런식으로 상황에 맞는 예외처리 클래스를 하나하나 생성하라는 의미가 맞으신가요?

6e96029

현재는 이렇게 수정해보았습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gongsua
네 이런식으로 BusinessException을 상속 받으면서 예외 처리 클래스를 케이스별로 별도로 정의하란 말이었구요,
여기서 더 나아가서, post 도메인 서브 패키지 하위에 exception 혹은 error 서브 패키지를 두고, 이 하위에다가 post 도메인 관련 예외들을 정의해주면 좋습니다. 그리고 마찬가지로 이 서브 패키지 하위에 post 관련 예외들을 핸들링하는 global exception handler를 선언해주고요.
이런식으로 각 도메인 서브 패키지마다 하위에 관련 예외와 예외에 대한 핸들러들을 모아서 구현해주면 좋습니다.
이렇게 구현하면 어떤 장점이 있을까요?


// 상세 내용 저장
private void savePostDetailIfExists(PostCreateRequestDto dto, Post post) {
if (dto.content() != null && !dto.content().isBlank()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[REQUIRED]
구상한 로직 상, post detail이 존재하지 않는 post 엔티티가 있을 수 있나요?
현재 로직대로라면, dto.content()가 비어있는 경우 post detail 엔티티 자체가 생성되지 않고, 따라서 post detail이 없는 post 엔티티가 발생할 수 있습니다.
의도한 바가 맞는지 꼭 점검해보세요!

private String content;

@Column(nullable = false)
private Integer quantity;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[REQUIRED]
여기서 int라는 primitive 타입이 아니라 Integer Wrapper 타입을 사용한 이유가 있을까요?
아래의 정적 팩토리 메소드 create(...)에서도 마찬가지고, wrapper 타입을 사용할 경우 객체로 다루는 것이기 때문에 NPE 발생 가능성이 높아집니다.
primitive 타입을 사용하지 않아야할 이유가 있는지 다시 고민해보고 알려주세요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

60888ae
수정하였습니다!

"/v3/api-docs/**",
"/favicon.ico"
).permitAll()
.requestMatchers(HttpMethod.GET, "/posts/lists", "/posts/{id}").permitAll()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NOTE]
JwtTokenFilter의 path 필터링 로직과 중복인 것 같은데, Auth와 관련해서는 앞서서 슬랙 통해서 제안 드렸던 리팩토링 이후에 다시 확인해보는 것이 좋을 것 같습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 pr로 로직 분리하여 요청드렸습니다.!

@gongsua
Copy link
Collaborator Author

gongsua commented Nov 12, 2025

@f-lab-ted

말씀주신대로 UI에서 가져오는 것 기준으로 엔티티 설계가 진행된 것같습니다.
아래와 같이 변경하겠습니다.

  1. PostDetail 제거, Post에 content / quantity 필드 통합
  2. PostImage 유지

DTO 모델 설계 이유를 정리하면

  1. UI의 요청과 목적에 맞춘 필드를 추출하기 위해서
  2. 핵심데이터들을 보관하기 위해서 라고 생각합니다.

따라서 위와 같이 수정하겠습니다.

@gongsua gongsua merged commit 84fadec into main Nov 16, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feat] 상품 작성 기능 구현

3 participants