-
Notifications
You must be signed in to change notification settings - Fork 203
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
[5기 엄예림, 정의진] Spring Boot JPA 게시판 구현 미션 제출합니다. #265
base: YU
Are you sure you want to change the base?
Conversation
errors.properties 추가
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.
오 요 api 문서 형식은 강의에서 보았던 방식은 아닌 것 같은데 어떤 것을 사용하신 건가요?
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 호출을 파일로 관리하는 점이 참 좋네요 👍
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.
@Sehee-Lee-01 문서 형식을 따로 참고한 것은 아니고, 저희가 임의로 만들었습니다!
아직도 postman 쓰세요? Intellij http를 통해 api를 테스트해보자! 이 링크에 Intellij http로 api 문서화 하는 방법이 꽤 잘 설명 되어 있어서 참고해도 좋을 것 같아요!
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.
이 html 파일 저희가 실습할 땐 build 디렉토리에 들어가던데ㅠㅠ 현재 경로(src/docs/asciidoc)로 들어가도록 어떻게 설정하셨나요? 👍
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.
사실 저희는 build 폴더에 있는 http 파일을 복사해서 넣어 놨습니다!😅
} | ||
|
||
@GetMapping | ||
public ResponseEntity<ListResponse> getPosts(Pageable pageable) { |
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.
ListResponse로 정보를 잘 감싸주었군요! 👍덕분에 코드도 깔끔해보이네용!
private final PostService postService; | ||
|
||
@PostMapping | ||
public ResponseEntity<PostResponse> createPost(@Valid @RequestBody CreatePostRequest 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.
DTO(PostResponse) 네이밍이 직관적이라서 좋은 것 같습니다!
private String createdBy; | ||
|
||
@Column(name = "created_at", columnDefinition = "TIMESTAMP", updatable = false) | ||
@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'") |
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.
format까지 설정하신 부분이 너무 좋은 것 같습니다! 하나 배워갑니당!! 👍
NotNull={0}은(는) 필수 입력 항목입니다. | ||
NotBlank={0}은(는) 필수 입력 항목이며 공백을 제외한 문자를 하나 이상 포함해야 합니다. | ||
Range={0}의 범위는 {2}에서 {1}사이여야 합니다. | ||
Length={0}의 길이는 {2}에서 {1}사이여야 합니다. |
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.
저 { }
안에는 BindingResult Error와 관련된 argument가 들어가는 것일까요?
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 ObjectMapper objectMapper; | ||
|
||
@MockBean | ||
private PostService postService; |
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.
여기 프로젝트에서는 각 레이어별 유효성 검사를 하기 때문에 컨트롤러만 유효성 검사가 잘 되는지 mocking 이용해서 테스트 하셨군요! 프로젝트 특성에 맞게, 필요한 부분에만 테스트를 작성한 것 같아서 좋은 것 같습니다! 👍
@DisplayName("제목이 1 ~ 30자 범위를 초과해 게시글 생성에 실패한다.") | ||
@ParameterizedTest(name = "{index}. {0} 제목의 글자 수 범위를 초과한다.") | ||
@EmptySource | ||
@MethodSource("provideLongTitle") |
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.
메소드 설정해서 소스를 만들 수 있군요! 하나 배워갑니다용👍
.andExpect(status().isCreated()) | ||
.andDo(document("post-create", | ||
preprocessRequest(prettyPrint()), | ||
preprocessResponse(prettyPrint()), |
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.
👍prettyPrint() 배워갑니당!
|
||
protected abstract Object initController(); | ||
|
||
} |
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.
코드 리뷰를 하면서 많이 배운 것 같아요!
저의 개인적인 의견이 다소 많아서 반영을 위해 무리하시기 보단 이렇게도 생각할 수 있겠군..
처럼 가볍게 생각해주시면 좋을 것 같아요!
그리고 저도 모르는게 정말 많아서 반박 및 질문 부탁드립니다! 🤗
container_name: board | ||
environment: | ||
MYSQL_DATABASE: board_system | ||
MYSQL_ROOT_HOST: '%' | ||
MYSQL_ROOT_PASSWORD: 1234 | ||
TZ: 'Asia/Seoul' | ||
ports: | ||
- "23306:3306" |
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.
docker compose 파일로 db 관리에 편의성을 가져가는 부분이 좋네요 👍
- 보통 docker compose는 서비스에서 다수의 컨테이너를 사용하고 관리할 때 사용하다 보니 단일 컨테이너를 사용하는 지금은 dockerFile을 고려하는 것도 좋을 것 같아요
- 만약 docker compose에서 test에서 사용하는 DB를 관리한다면 지금처럼 compose 파일에서 관리하는 것도 좋을 것 같네요
- 컨테이너 이름이나 서버 port, db에 대한 설정 값은 변경이 잦은 경우가 많은 것 같아서 직접적으로 버전 관리가 되도록 하는 것 보다는 runtime에 사용자가 입력하도록 하는 걸 고려하면 좋을 것 같아요
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 호출을 파일로 관리하는 점이 참 좋네요 👍
==== HTTP request | ||
|
||
include::{snippets}/post-update/http-request.adoc[] | ||
include::{snippets}/post-update/path-parameters.adoc[] |
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.
path 파라미터도 스니펫으로 존재하는 줄 몰랐네요!
배워 갑니다~
} | ||
|
||
@PatchMapping("/{id}") | ||
public ResponseEntity<PostResponse> updatePost(@PathVariable Long id, @Valid @RequestBody UpdatePostRequest 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.
이 라인은 120자를 넘어가는 것 같네요
아래와 같이 수정하면 좋을 것 같아요
public ResponseEntity<PostResponse> updatePost(@PathVariable Long id,
@Valid @RequestBody UpdatePostRequest request) {
@PatchMapping("/{id}") | ||
public ResponseEntity<PostResponse> updatePost(@PathVariable Long id, @Valid @RequestBody UpdatePostRequest 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.
title이나 content가 모두 바뀌는 것이 아니어서 해당 메서드를 PATCH로 하신 것 같아요
제 생각으로는 front-end가 게시글의 title과 content를 모두 알고 있어야 요청을 보낼 수 있고 현재 애플리케이션이 게시글이 존재하지 않을 때와 존재할 때에 대한 요청 시 포스트 제목과 내용이 각각 멱등성이 보장되지 않기 때문에 PUT 메서드를 사용하시면 좋을 것 같아요!
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.
작성자나 생성시간 등이 변경 되지 않기 때문에 PATCH로 결정을 했었습니다 그리고 사용자가 수정할 수 있는 것은 제목과 내용 뿐이라고 생각을 했었는데요!
그런데 혹시 _"현재 애플리케이션이 게시글이 존재하지 않을 때와 존재할 때에 대한 요청 시 포스트 제목과 내용이 각각 이 보장되지 않기 때문에"_라는 말씀이 어떤 의미인지 알 수 있을까요?
저희도 front-end가 게시글의 title과 content를 모두 알고 있어야 요청을 보낼 수 있고, 만약 제목만 변경하고 싶더라도 { "title": "수정된 title", "content": "기존 content"} 이런식으로 요청을 보낸다고 생각을 했었습니다! 이 경우에는 title과 content를 기존값에 관계 없이 항상 요청된 상태로 설정하기 때문에, 멱등성이 지켜지지 않을까요..? 혹시 제가 이해한 바가 맞는지 궁금합니다..!
그런데 생각해보니 { "title": "수정된 title" } 이런식으로 요청이 오는 경우가 아예 없다고 보장할 수 없으니 null 처리에 대한 방어를 해둬야 겠다는 생각이 드네요..! PATCH를 유지하되 null 처리를 해주는 방법은 혹시 어떨까요?
|
||
@DisplayName("수정된 제목이 1 ~ 30자 범위를 초과로 게시글 수정에 실패한다.") | ||
@ParameterizedTest | ||
@EmptySource |
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.
@NullAndEmptySource
를 활용하는 것도 좋을 것 같아요!
|
||
@RestControllerAdvice | ||
@RequiredArgsConstructor | ||
public class GlobalExceptionHandler { |
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.
커버 되지 않는 부분의 테스트를 추가하면 더 좋을 것 같아요 💯
responseFields( | ||
fieldWithPath("id").type(NUMBER).description("Post Id"), | ||
fieldWithPath("title").type(STRING).description("Title"), | ||
fieldWithPath("content").type(STRING).description("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.
파라미터 커버리지가 넓어지면 해당 테스트가 통과되지 않을 것 같아서 nullable
한 필드에 대해선 optional을 붙여주는게 좋을 것 같아요!
fieldWithPath("content").type(STRING).description("Content").optional(),
parameterWithName("page").description("페이지"), | ||
parameterWithName("size").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.
api 문서의 파라미터와 필드 설명 중 이 부분만 한글이라 통일하면 좋을 것 같아요!
assertThatThrownBy(() -> postService.createPost(request)) | ||
.isInstanceOf(ConstraintViolationException.class); |
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 post = postRepository.findById(id) | ||
.orElseThrow(() -> new EntityNotFoundException("존재하지 않는 게시글입니다.")); | ||
|
||
post.editPostTitleContent(request.title(), request.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.
요기 update할 때 기존에 content에 데이터가 존재하는 경우에 {"title": "title"} 이런 식으로 받으면 content도 null로 수정되는 거 아닌가요??
import org.prgms.springbootboardjpayu.dto.response.UserResponse; | ||
|
||
public final class UserConverter { | ||
|
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.
어떤 클래스에는 상위에 개행이 있고 어떤 클래스에는 개행이 없는데 이런 스타일 맞춰줄 수 있는 formatter가 있는지 찾아보면 좋을 것 같습니다!
|
||
@Column(name = "age") | ||
@Range(min = 0, max = 200) | ||
private Integer age; |
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.
primitive 타입을 사용해서 null이 들어가지 않게 해도 괜찮을 것 같습니다.
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.
게시판에서 age가 필수값이 아니라고 생각해서 데이터베이스의 default 값인 nullable=true로 설정했습니다!
for (String code : codes) { | ||
try { | ||
return messageSource.getMessage(code, error.getArguments(), Locale.KOREA); | ||
} catch (NoSuchMessageException ignored) { |
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.
아니면 메서드를 분리해서 NoSuchMessageException이 발생했을 때 ""나 Optional.empty 반환해서 filter하고 findFirst로 찾아서 반환하고 orElse로 못찾은 경우 default message 반환하게 구성해도 좋을 것 같습니다.
class PostTest { | ||
@DisplayName("게시글을 생성할 수 있다.") | ||
@Test | ||
void createPost() { |
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.
validation 관련 예외 테스트도 짜보면 좋을 것 같습니다!
// then | ||
assertThat(savedPost.id()).isNotNull(); | ||
assertThat(savedPost.createdAt()).isNotNull(); | ||
assertThat(savedPost).extracting("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.
개인적으로 property를 스트링으로 써서 테스트를 할 때 PostResponse 내부 필드가 바뀌었을 때 ide에 에러로 잡히지 않을 꺼 같아서 잡힐 수 있게 따로 검증하는 방법도 생각해보면 좋을 것 같습니다.
|
||
} | ||
|
||
private static List<String> provideLongName() { |
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.
👍 UserServiceTest에도 duplicate function이 존재하는 것 같아서 이런 부분을 테스트 내부 유틸로 빼서 다른 함수들에서 같이 사용하면 더 좋을 것 같습니다.
CreatePostRequest request = new CreatePostRequest("제목", "내용", 1L); | ||
|
||
// when then | ||
mockMvc.perform( |
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.
MockMvc를 이용하여 컨트롤러 단에서의 모킹 테스트가 RestDocs에서 수행한 테스트와 다른점이 있나요?? 서블릿을 띄우는 E2E 테스트가 더 의미있을 것 같습니다. TestRestTemplate 같은 것을 찾아보면 좋을 것 같아요!
📌 과제 설명
Sprint Data JPA를 사용한 간단한 게시판
👩💻 요구 사항과 구현 내용
SpringDataJPA 를 설정한다.
엔티티를 구성한다
회원(User)
게시글(Post)
회원과 게시글에 대한 연관관계를 설정한다.
게시글 Repository를 구현한다. (PostRepository)
API를 구현한다.
회원 생성
게시글 작성 (POST "/posts")
게시글 조회
게시글 수정 (POST "/posts/{id}")
REST-DOCS를 이용해서 문서화한다.
✅ 실행 방법
docker 폴더로 이동해 아래 명령어를 입력하면 MySQL을 실행시킬 수 있습니다.
✅ PR 포인트 & 궁금한 점