Skip to content
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

Open
wants to merge 15 commits into
base: seungwon
Choose a base branch
from

Conversation

SW-H
Copy link

@SW-H SW-H commented Aug 4, 2023

📌 과제 설명

JPA로 게시판을 구현하고 rest-docs를 이용해 문서화했습니다.

👩‍💻 요구 사항과 구현 내용

  • SpringDataJPA 를 설정한다
    datasource : h2 or mysql

  • 엔티티를 구성한다

    • 회원(User)
      - id (PK) (auto increment)
      - name
      - age
      - hobby
      - created_at
      - created_by
    • 게시글(Post)
      - id (PK) (auto increment)
      - title
      - content
      - created_at
      - created_by
  • 회원과 게시글에 대한 연관관계를 설정한다.

    • 회원과 게시글은 1:N 관계이다.
  • 게시글 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필드를 모두 적어주는게 꽤 번거롭게 느껴지는데 감수하고 사용하는건가요..? 실제 많이 사용하나요..?

Copy link

@player-geun player-geun left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 승원님 👍

리뷰 남겨뒀으니 확인부탁드립니다 !!

Comment on lines 21 to 24

@LastModifiedDate
LocalDateTime updatedAt;
}
Copy link

Choose a reason for hiding this comment

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

요구사항은 @ CreatedBy 가 아닌가요??

Copy link
Author

Choose a reason for hiding this comment

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

createdBy도 추가했습니다!!

Comment on lines 67 to 72
private void validateHobby(String hobby) {
if (hobby.length() > 100 || hobby.isEmpty()) {
throw new InvalidDataException(
MessageFormat.format("입력된 취미={0}자. 취미는 최대 {1}자 입력 가능합니다.", hobby.length(), HOBBY_MAX_LENGTH));
}
}

Choose a reason for hiding this comment

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

isBlank 가 아닌 isEmpty 를 사용하지 않으신 이유가 있으실까요??

Copy link
Author

Choose a reason for hiding this comment

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

isEmpty와 isBlank의 차이는 공백 처리가 다른데 isBlank가 더 맞는것 같아 수정했습니다. 항상 isBlank로 했는데 왜 그랬는지 모르겠네요!

Comment on lines 12 to 13
@RestController
@RequestMapping("/members")

Choose a reason for hiding this comment

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

url 을 api/members 로 바꾸는 것은 어떨까요?

Comment on lines 30 to 63
@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);
}
Copy link

Choose a reason for hiding this comment

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

클래스 레벨에서 @ Transactional 을 걸지 않고 일일히 메서드 단위로 걸어주신 이유가 있으실까요??

Comment on lines +21 to +25
@PostMapping()
ResponseEntity<Long> create(@RequestBody MemberRequestDto memberRequestDto) {
Long id = memberService.create(memberRequestDto);
return ResponseEntity.ok(id);
}

Choose a reason for hiding this comment

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

저장이 성공하면, 어떤 리소스가 저장되었는지 리소스 자체를 리턴하는 것이 더 좋지 않을까요??

이렇게 되면 회원정보를 저장하고, 다시 조회하는 즉, 두번의 네트워크 트래픽이 발생해야 회원의 정보를 가져와 조금 비효율적일 것 같습니다 !

Copy link
Author

Choose a reason for hiding this comment

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

저는 글을 등록하면 전체 목록을 보여주는 상황을 가정했습니다. 그래서 어차피 전체조회가 필요하고 특정 글에 대한 상세보기가 필요하면 다시 조회해야 된다고 생각해서 이렇게 개발했습니다. 뭔가 이 부분은 요구사항이나 서비스에 따라 달라질 것 같습니다:)

Choose a reason for hiding this comment

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

두분 말씀 다 적절할듯합니다.
상황에 따라 응답에 포함되는게 효율적일 수도 있고, 반대로 불필요한 트래픽을 유발할 수도 있습니다.
바디를 주던 주지 않던 각각 장단점이 있다는 것만 이해하고 계시면 됩니다.

Comment on lines 19 to 27
public Long create(MemberRequestDto memberRequestDto) {
Member member = Member.builder()
.age(26)
.hobby("운동")
.name("한승원")
.build();
Member result = memberRepository.save(member);
return result.getId();
}

Choose a reason for hiding this comment

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

?? 요건 MemberRequestDto를 활용하도록 코드가 작성되어야 할 것 같은데, 테스트 할때 쓰던 코드가 그대로 들어간 것 같습니다.

Copy link
Author

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();
}

Comment on lines 58 to 64
Long updater = postRequestDto.writerId();
Long initialWriter = post.getWriter().getId();
if (updater != initialWriter) {
throw new InvalidRequestException(
MessageFormat.format("작성자는 변경 불가능합니다[기존 작성자 id={0}, 요청된 작성자 id={1}] ", initialWriter, updater)
);
}

Choose a reason for hiding this comment

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

이 체크 로직이 Post 안쪽으로 들어가면 어떨까요?!

Comment on lines 42 to 67
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);
}

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);
	}

이런식으로..?

Copy link
Author

Choose a reason for hiding this comment

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

이 부분 자꾸 놓치는데 전반적으로 수정했습니다!!

@lleellee0
Copy link

@SW-H

승원님 수고하셨습니다~

질문 주신 내용 답변드려볼게요.

id 값으로 DB에서 생성되는 값을 사용했는데 이 값은 외부에 노출되면 안될 것 같습니다.. 그런데 특정 게시글에 대한 조회를 할때 /posts/id URI로 요청을 보내고 response값에도 id값이 포함됩니다. 그럼 이 경우엔 db에서 자동으로 생성되는 id외에도 노출시킬 id를 별도로 추가하는 편이 좋을까요?

-> 이건 혹시 외부에 왜 노출되면 안되죵..? 글 번호 이야기하는거 아닌가요??

rest docs로 문서화를 하다보니 swagger의 경우처럼 서비스 코드에 문서화와 관련된 어노테이션 등이 섞이지 않는다는 장점은 있는 것 같습니다. 그런데 테스트 코드나 각 response/request필드를 모두 적어주는게 꽤 번거롭게 느껴지는데 감수하고 사용하는건가요..? 실제 많이 사용하나요..?

저는 보통 swagger를 사용하긴 하는데, rest docs도 꽤 많이 사용하는 것 같습니다.
rest docs의 장점은 문서화를 위해 컨트롤러에 대한 테스트 코드를 강제하고, 이 강제화된 테스트 코드에 의해 실제 코드와 완전히 일치하는 문서를 유지할 수 있다는데 장점이 있을 것 같습니다.
다만 저도 승원님이 이야기하신 포인트대로 꽤 번거롭다고 생각하는데요, 다른 팀들 사용하는거 보니까 DSL(https://www.jetbrains.com/ko-kr/mps/concepts/domain-specific-languages/) 같은거로 대체해서 쓰기도 하시더라구요. 아니면 chatgpt 통해서 테스트 코드 초안을 만드시는 것도 괜찮은 방법일 것 같습니다.

Comment on lines +40 to +50
@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);
}

Choose a reason for hiding this comment

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

정의하신 InvalidDataException 의도를 보면 METHOD_NOT_ALLOWED는 조금 맞지 않는것 같네요

Choose a reason for hiding this comment

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

에러에 대한 응답을 어떻게 구성하는지 여기서 결정되는걸로 보이는데요
지금보면 메시지(한글)을 그대로 던져주는 형태네요
현재 사용성이 확정되지 않은 지금, 클라이언트에서 서버에서 응답 받은 한글 에러를 그대로 활용해도 무방하다고 봅니다.

나중에 조금더 서비스가 고도화 된다면 클라이언트에서 에러에 따라 분기처리가 필요할 수 있기 때문에 영어 코드나 숫자 코드로 특정 예외상황을 인지시켜주면 좋을것 같네요(Http status 코드 말고 직접 정의한 다른 코드)

추가로 더 나가본다면 클라언트가 사용할 에러 메시지와 로깅용으로 사용할 메시지도 구분해볼 수 있을것 같습니다.

Comment on lines 26 to 28
@Id
@GeneratedValue(strategy = GenerationType.AUTO)
Long id;

Choose a reason for hiding this comment

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

GenerationType.AUTO 말고 명시적으로 id 생성 전략을 지정해주시는게 좋습니다.
상황에 따라 예측하지 못한 전략으로 매핑될 위험이 있을 수 있습니다.

Comment on lines +30 to +33
@AutoConfigureRestDocs
@AutoConfigureMockMvc
@SpringBootTest
class BoardApplicationTests {

Choose a reason for hiding this comment

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

현재 레스트 도큐먼트 구성 목적으로만 테스트 코드가 존재해서
@AutoConfigureRestDocs
@WebMvcTest(PostController.class)
위 코드만으로 될것 같아요(아마도?)

추가로, 실제 응답을 검증하는 테스트는 없는것 같은데요 이번에 한번 작성해보시면 좋을것 같네요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants