-
Notifications
You must be signed in to change notification settings - Fork 204
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
[4기 한승원] Spring Boot JPA로 게시판 구현 미션 제출합니다. #250
base: seungwon
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.
수고하셨습니다 승원님 👍
리뷰 남겨뒀으니 확인부탁드립니다 !!
|
||
@LastModifiedDate | ||
LocalDateTime updatedAt; | ||
} |
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.
요구사항은 @ CreatedBy 가 아닌가요??
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.
createdBy도 추가했습니다!!
private void validateHobby(String hobby) { | ||
if (hobby.length() > 100 || hobby.isEmpty()) { | ||
throw new InvalidDataException( | ||
MessageFormat.format("입력된 취미={0}자. 취미는 최대 {1}자 입력 가능합니다.", hobby.length(), HOBBY_MAX_LENGTH)); | ||
} | ||
} |
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.
isBlank 가 아닌 isEmpty 를 사용하지 않으신 이유가 있으실까요??
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.
isEmpty와 isBlank의 차이는 공백 처리가 다른데 isBlank가 더 맞는것 같아 수정했습니다. 항상 isBlank로 했는데 왜 그랬는지 모르겠네요!
@RestController | ||
@RequestMapping("/members") |
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.
url 을 api/members 로 바꾸는 것은 어떨까요?
@Transactional(readOnly = true) | ||
public Page<PostResponseDto> findAll(Pageable pageable) { | ||
return postRepository.findAll(pageable) | ||
.map(PostResponseDto::new); | ||
} | ||
|
||
@Transactional(readOnly = true) | ||
public PostResponseDto findBy(Long id){ | ||
Post post = postRepository.findById(id) | ||
.orElseThrow(() -> new NoSuchDataException(MessageFormat.format("해당 포스트가 존재하지 않습니다[id={0}] ", id))); | ||
return new PostResponseDto(post); | ||
} | ||
|
||
@Transactional | ||
public PostSaveRequestDto create(PostRequestDto postRequestDto) { | ||
Long userId = postRequestDto.writerId(); | ||
Member member = memberRepository.findById(userId).orElseThrow(); | ||
Post post = Post.builder() | ||
.writer(member) | ||
.title(postRequestDto.title()) | ||
.content(postRequestDto.content()) | ||
.build(); | ||
Post result = postRepository.save(post); | ||
return new PostSaveRequestDto(result); | ||
} | ||
|
||
@Transactional | ||
public PostSaveRequestDto update(Long id, PostRequestDto postRequestDto) { | ||
Post post = postRepository.findById(id) | ||
.orElseThrow(() -> new InvalidRequestException( | ||
MessageFormat.format("존재하지 않는 포스트에 대한 수정 요청입니다[id={0}] ", id))); | ||
post.modify(postRequestDto.title(), postRequestDto.content()); | ||
return new PostSaveRequestDto(post); | ||
} |
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.
클래스 레벨에서 @ Transactional 을 걸지 않고 일일히 메서드 단위로 걸어주신 이유가 있으실까요??
@PostMapping() | ||
ResponseEntity<Long> create(@RequestBody MemberRequestDto memberRequestDto) { | ||
Long id = memberService.create(memberRequestDto); | ||
return ResponseEntity.ok(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.
저장이 성공하면, 어떤 리소스가 저장되었는지 리소스 자체를 리턴하는 것이 더 좋지 않을까요??
이렇게 되면 회원정보를 저장하고, 다시 조회하는 즉, 두번의 네트워크 트래픽이 발생해야 회원의 정보를 가져와 조금 비효율적일 것 같습니다 !
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.
두분 말씀 다 적절할듯합니다.
상황에 따라 응답에 포함되는게 효율적일 수도 있고, 반대로 불필요한 트래픽을 유발할 수도 있습니다.
바디를 주던 주지 않던 각각 장단점이 있다는 것만 이해하고 계시면 됩니다.
public Long create(MemberRequestDto memberRequestDto) { | ||
Member member = Member.builder() | ||
.age(26) | ||
.hobby("운동") | ||
.name("한승원") | ||
.build(); | ||
Member result = memberRepository.save(member); | ||
return result.getId(); | ||
} |
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.
?? 요건 MemberRequestDto를 활용하도록 코드가 작성되어야 할 것 같은데, 테스트 할때 쓰던 코드가 그대로 들어간 것 같습니다.
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.
실수했던 것 같습니다! 아래처럼 수정했습니다!
public Long create(MemberRequestDto memberRequestDto) {
Member member = Member.builder()
.age(memberRequestDto.age())
.hobby(memberRequestDto.hobby())
.name(memberRequestDto.name())
.build();
Member result = memberRepository.save(member);
return result.getId();
}
Long updater = postRequestDto.writerId(); | ||
Long initialWriter = post.getWriter().getId(); | ||
if (updater != initialWriter) { | ||
throw new InvalidRequestException( | ||
MessageFormat.format("작성자는 변경 불가능합니다[기존 작성자 id={0}, 요청된 작성자 id={1}] ", initialWriter, updater) | ||
); | ||
} |
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.
이 체크 로직이 Post 안쪽으로 들어가면 어떨까요?!
public PostSaveRequestDto create(PostRequestDto postRequestDto) { | ||
Long userId = postRequestDto.writerId(); | ||
Member member = memberRepository.findById(userId).orElseThrow(); | ||
Post post = Post.builder() | ||
.writer(member) | ||
.title(postRequestDto.title()) | ||
.content(postRequestDto.content()) | ||
.build(); | ||
Post result = postRepository.save(post); | ||
return new PostSaveRequestDto(result); | ||
} | ||
|
||
public PostSaveRequestDto update(Long id, PostRequestDto postRequestDto) { | ||
Post post = postRepository.findById(id) | ||
.orElseThrow(() -> new InvalidRequestException( | ||
MessageFormat.format("존재하지 않는 포스트에 대한 수정 요청입니다[id={0}] ", id))); | ||
Long updater = postRequestDto.writerId(); | ||
Long initialWriter = post.getWriter().getId(); | ||
if (updater != initialWriter) { | ||
throw new InvalidRequestException( | ||
MessageFormat.format("작성자는 변경 불가능합니다[기존 작성자 id={0}, 요청된 작성자 id={1}] ", initialWriter, updater) | ||
); | ||
} | ||
post.modify(postRequestDto.title(), postRequestDto.content()); | ||
return new PostSaveRequestDto(post); | ||
} |
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.
전반적으로 코드 블록간에 개행이 추가되면 좋을 것 같습니다.
public PostSaveRequestDto create(PostRequestDto postRequestDto) {
Long userId = postRequestDto.writerId();
Member member = memberRepository.findById(userId).orElseThrow();
Post post = Post.builder()
.writer(member)
.title(postRequestDto.title())
.content(postRequestDto.content())
.build();
Post result = postRepository.save(post);
return new PostSaveRequestDto(result);
}
이런식으로..?
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.
이 부분 자꾸 놓치는데 전반적으로 수정했습니다!!
승원님 수고하셨습니다~ 질문 주신 내용 답변드려볼게요.
-> 이건 혹시 외부에 왜 노출되면 안되죵..? 글 번호 이야기하는거 아닌가요??
저는 보통 swagger를 사용하긴 하는데, rest docs도 꽤 많이 사용하는 것 같습니다. |
@ExceptionHandler(InvalidDataException.class) | ||
public ResponseEntity<Object> invalidDataException(InvalidDataException ex, | ||
WebRequest request) { | ||
|
||
ErrorResponse errorResponse = ErrorResponse.builder() | ||
.status(HttpStatus.METHOD_NOT_ALLOWED) | ||
.message(ex.getMessage()) | ||
.build(); | ||
|
||
return handleExceptionInternal(ex, errorResponse, null, HttpStatus.METHOD_NOT_ALLOWED, request); | ||
} |
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.
정의하신 InvalidDataException 의도를 보면 METHOD_NOT_ALLOWED는 조금 맞지 않는것 같네요
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.
에러에 대한 응답을 어떻게 구성하는지 여기서 결정되는걸로 보이는데요
지금보면 메시지(한글)을 그대로 던져주는 형태네요
현재 사용성이 확정되지 않은 지금, 클라이언트에서 서버에서 응답 받은 한글 에러를 그대로 활용해도 무방하다고 봅니다.
나중에 조금더 서비스가 고도화 된다면 클라이언트에서 에러에 따라 분기처리가 필요할 수 있기 때문에 영어 코드나 숫자 코드로 특정 예외상황을 인지시켜주면 좋을것 같네요(Http status 코드 말고 직접 정의한 다른 코드)
추가로 더 나가본다면 클라언트가 사용할 에러 메시지와 로깅용으로 사용할 메시지도 구분해볼 수 있을것 같습니다.
@Id | ||
@GeneratedValue(strategy = GenerationType.AUTO) | ||
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.
GenerationType.AUTO 말고 명시적으로 id 생성 전략을 지정해주시는게 좋습니다.
상황에 따라 예측하지 못한 전략으로 매핑될 위험이 있을 수 있습니다.
@AutoConfigureRestDocs | ||
@AutoConfigureMockMvc | ||
@SpringBootTest | ||
class BoardApplicationTests { |
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.
현재 레스트 도큐먼트 구성 목적으로만 테스트 코드가 존재해서
@AutoConfigureRestDocs
@WebMvcTest(PostController.class)
위 코드만으로 될것 같아요(아마도?)
추가로, 실제 응답을 검증하는 테스트는 없는것 같은데요 이번에 한번 작성해보시면 좋을것 같네요
📌 과제 설명
JPA로 게시판을 구현하고 rest-docs를 이용해 문서화했습니다.
👩💻 요구 사항과 구현 내용
SpringDataJPA 를 설정한다
datasource : h2 or mysql
엔티티를 구성한다
- id (PK) (auto increment)
- name
- age
- hobby
- created_at
- created_by
- id (PK) (auto increment)
- title
- content
- created_at
- created_by
회원과 게시글에 대한 연관관계를 설정한다.
게시글 Repository를 구현한다. (PostRepository)
API를 구현한다.
- 게시글 조회
- 페이징 조회 (GET "/posts")
- 단건 조회 (GET "/posts/{id}")
- 게시글 작성 (POST "/posts")
- 게시글 수정 (POST "/posts/{id}")
REST-DOCS를 이용해서 문서화한다
✅ PR 포인트 & 궁금한 점
id 값으로 DB에서 생성되는 값을 사용했는데 이 값은 외부에 노출되면 안될 것 같습니다.. 그런데 특정 게시글에 대한 조회를 할때
/posts/id
URI로 요청을 보내고 response값에도 id값이 포함됩니다. 그럼 이 경우엔 db에서 자동으로 생성되는 id외에도 노출시킬 id를 별도로 추가하는 편이 좋을까요?rest docs로 문서화를 하다보니 swagger의 경우처럼 서비스 코드에 문서화와 관련된 어노테이션 등이 섞이지 않는다는 장점은 있는 것 같습니다. 그런데 테스트 코드나 각 response/request필드를 모두 적어주는게 꽤 번거롭게 느껴지는데 감수하고 사용하는건가요..? 실제 많이 사용하나요..?