-
Notifications
You must be signed in to change notification settings - Fork 7
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
[14주차] 임현우 학습 PR 제출합니다. #2
base: main
Are you sure you want to change the base?
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.
전반적으로 너무 잘하셨습니다! 고생하셨습니다.
private Long member; | ||
private Long post; | ||
private String 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.
제약조건 걸어주기! 당장 member필드 post필드 content필드에 null이라 빈 값이 들어올 경우도 생각해주세요.
NotNull, NotEmpty, NotBlank의 차이점에 대해 공부해보면 좋습니다!
@OneToMany(mappedBy = "member", cascade = CascadeType.ALL, orphanRemoval = true) | ||
private List<Post> posts = new ArrayList<>(); | ||
|
||
@OneToMany(mappedBy = "member", cascade = CascadeType.ALL, orphanRemoval = true) | ||
private List<Comment> comments = new ArrayList<>(); | ||
|
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.
간단한 토이프로젝트라서 큰 상관은 없지만 양방향 매핑은 지양하는게 좋습니다!
왜 양방향 매핑을 지양해야 할까요? 특히 Jpa엔티티와 도메인이 같은 객체일 경우에는 일어날 수 있는 문제점이 정말 많습니다.
객체간 순환참조가 될 뿐만 아니라, 조금만 도메인이 많아져도 양방향 지옥에 빠져 어려움을 겪을 확률이 너무너무 큽니다... 관리도 안될 뿐더러 의도치 않는 쿼리가 나갈 수도 있고요.
정말 두 객체가 서로 알고 있어야하는지 고민해볼 필요가 있을 것 같아요.
@OneToMany(mappedBy = "member", cascade = CascadeType.ALL, orphanRemoval = true) | ||
private List<Post> posts = new ArrayList<>(); | ||
|
||
@OneToMany(mappedBy = "member", cascade = CascadeType.ALL, orphanRemoval = true) |
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.
CascadeType.DELETE와 orphanRemoval 차이에 대해서도 학습하시면 좋을 것 같아요!
List<String> comments = commentRepository.findByPostID(id) | ||
.stream() | ||
.map(Comment::getContent) | ||
.collect(Collectors.toList()); |
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.
메서드 분리하는게 좋아보여요!
private final PostRepository postRepository; | ||
private final MemberRepository memberRepository; | ||
private final CommentRepository commentRepository; |
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.
저는 개인적으로 PostRepository를 호출할 수 있는 것은 PostService라고 생각해요. 이 이야기는 뭐냐면 CommentRepository에 PostService가 직접 접근해 Comment를 가져와서 찾는 행위가 PostService에서 하는 것이 올바른 책임인가 생각이 듭니다.
CommentService에서 CommentRepository 사용하고 CommentService를 호출하는 것이 더 나아보입니다.
만약 다른 MemberService와 같이 많은 서비스에서 Comment가 필요하다면 그때마다 해당 도메인에 Service레이어가 CommentRepository를 의존해야하니까요.
물론 이런 경우에는 서비스 레이어간의 순환참조가 일어날 수도 있지 않느냐? 하는 의견이 있긴합니다만 저는 애당초 레이어간의 순환참조가 일어난다면 그건 애당초 잘못된 설계라고 생각합니다!
해당 내용에 대한 논의 글도 있으니 확인해보시면 좋을 것 같아요!
woowacourse/retrospective#15
@Query("select new com.example.jpa.comment.domain.Comment(c.member, c.content) " + | ||
"from Comment c " + | ||
"where c.post.id = :id") | ||
List<Comment> findByPostID(@Param("id") Long 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.
굳이 Query를 사용하신 이유가 있나요? 사용하지 않아도 될 것 같아요.
import java.util.List; | ||
|
||
@RestController | ||
@RequestMapping("api/posts") |
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.
이래도 작동은 하지만
"/api/posts" 이렇게 바꾸시는게 좋아요.
API: https://documenter.getpostman.com/view/30639641/2s9YXmYfpa