-
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
[4기 이홍섭] Spring Boot JPA로 게시판 구현 미션 제출합니다. #214
base: hongxeob
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.
홍섭님~ 리뷰 몇 가지 남겼습니다! 전체적으로 코드를 깔끔하게 짜신 것 같아서 보기 편했고, 이번에도 몇 가지 배워갑니다 👍 미션 진행하시느라 고생 많으셨습니다~!
public void updateUser(User author) { | ||
this.user = author; | ||
} |
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.
파라미터에 author가 갑자기 나왔는데 혹시 수정이 다 안 된 것 일까요~?
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.
해당 게시물의 유저가 작성자 라는것을 명시적으로 알려주기 위해 author로 이름을 변경해서 인자로 넣어줬습니다!
return PostResponse.create(savedPost); | ||
} | ||
|
||
@Transactional(readOnly = 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.
조회 작업에 @transactional(readOnly = true) 를 붙이면 성능이 향상될 수 있겠네요! 👍
import lombok.Getter; | ||
|
||
@Getter | ||
public class BusinessException extends RuntimeException { |
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 static final int MINIMUM_AGE = 1; | ||
|
||
@Id | ||
@GeneratedValue(strategy = GenerationType.AUTO) |
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.
@GeneratedValue(strategy = GenerationType.AUTO) | |
@GeneratedValue(strategy = GenerationType.SEQUENCE) |
h2를 사용하고 있으니 GenerationType.SEQUENCE로 명확하게 설정해보시는 건 어떨까요~?
|
||
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
@Getter | ||
public final class UserRequest { |
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.
final을 붙이신 이유가 있을까요~? 빼셔도 문제 없을 것 같습니다!
NOT_FOUND_POST(HttpStatus.NOT_FOUND, "존재하지 않는 게시물입니다."), | ||
; |
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.
NOT_FOUND_POST(HttpStatus.NOT_FOUND, "존재하지 않는 게시물입니다."), | |
; | |
NOT_FOUND_POST(HttpStatus.NOT_FOUND, "존재하지 않는 게시물입니다."); |
콤마 제거가 안 된 것 같습니다!
@Transactional | ||
public PostResponse update(Long postId, PostUpdateRequest updateRequest) { | ||
Post post = postRepository.findById(postId) | ||
.orElseThrow(() -> new BusinessServiceException(ErrorCode.NOT_FOUND_POST)); | ||
|
||
User user = userRepository.findById(post.getUser().getId()) | ||
.orElseThrow(() -> new BusinessServiceException(ErrorCode.NOT_FOUND_USER)); | ||
|
||
post.update(updateRequest.title(), updateRequest.content()); | ||
post.updateUser(user); |
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.
게시글을 업데이트 해줄 때 User까지 업데이트 해줘야할까요..?
일반적으로 게시판에서 작성자는 수정되지 않을텐데 이 부분이 의도된건지 궁금합니다.
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 HttpStatus httpStatus; | ||
private final String message; | ||
private final Map<String, String> valid; |
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.
field validation 내용을 담는 역할을 하는 친구인것 같은데 응답에 내려주는건가요?
지금 코드만 보면 응답에 활용되고 있지는 않아보이네요
네이밍도 조금 명확하게 수정되면 좋을것 같습니다
public static ResponseEntity<ExceptionResponse> toResponseEntity(ErrorCode errorCode) { | ||
return ResponseEntity | ||
.status(errorCode.getStatus()) | ||
.body(ExceptionResponse.builder() | ||
.httpStatus(errorCode.getStatus()) | ||
.message(errorCode.getMessage()) | ||
.build()); | ||
} |
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.
에러에 대한 응답을 어떻게 구성하는지 여기서 결정되는걸로 보이는데요
지금보면 errorCode 메시지(한글)을 그대로 던져주는 형태네요
현재 사용성이 확정되지 않은 지금, 클라이언트에서 서버에서 응답 받은 한글 에러를 그대로 활용해도 무방하다고 봅니다.
나중에 조금더 서비스가 고도화 된다면 클라이언트에서 에러에 따라 분기처리가 필요할 수 있기 때문에 영어 코드나 숫자 코드로 특정 예외상황을 인지시켜주면 좋을것 같네요
추가로 더 나가본다면 클라언트가 사용할 에러 메시지와 로깅용으로 사용할 메시지도 구분해볼 수 있을것 같습니다.
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.
감사합니다! 서비스 고도화시 말씀주신 영어,숫자 코드로 특정 예외상황을 팀내에서 정해서 인지시켜주는 것도 좋은 것 같습니다 ㅎㅎ!
@SpringBootTest | ||
@Transactional | ||
class PostServiceTest { |
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.
슬라이스 테스트로 한번 바꿔보세요
Auditing 때문에 조금 시행착오는 있을거에요 ㅋㅋ
import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; | ||
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; | ||
|
||
@SpringBootTest |
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(UserApiController.class)
이렇게만 붙여도 되실꺼에요
@Test | ||
@DisplayName("게시물 등록 성공") | ||
void createPostSuccessTest() throws Exception { | ||
|
||
//given | ||
PostCreateRequest postCreateRequest = PostCreateRequest.builder() | ||
.userId(user.getId()) | ||
.title("제목") | ||
.content("내용") | ||
.build(); | ||
|
||
//when -> //then | ||
mockMvc.perform(post("/api/v1/posts") | ||
.contentType(APPLICATION_JSON) | ||
.content(objectMapper.writeValueAsString(postCreateRequest))) | ||
.andExpect(status().isOk()) | ||
.andDo(print()) | ||
.andDo(document("post-create", | ||
Preprocessors.preprocessRequest(prettyPrint()), | ||
preprocessResponse(prettyPrint()), | ||
requestFields( | ||
fieldWithPath("userId").type(NUMBER).description("User Id"), | ||
fieldWithPath("title").type(STRING).description("Title"), | ||
fieldWithPath("content").type(STRING).description("Content")), | ||
responseFields( | ||
fieldWithPath("id").type(NUMBER).description("게시판ID"), | ||
fieldWithPath("title").type(STRING).description("제목"), | ||
fieldWithPath("content").type(STRING).description("내용"), | ||
fieldWithPath("authorId").type(NUMBER).description("작성자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.
전체적으로 document를 만들기위한 테스트는 있지만
실제 응답을 검증하는 테스트는 없는것 같은데요 이번에 한번 작성해보시면 좋을것 같습니다
@NotBlank(message = "유저 이름을 입력해주세요.") | ||
@Pattern(regexp = "^[a-zA-Z0-9]+$", message = "이름은 영어만 입력가능합니다.") | ||
private String name; |
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.
이게 @notblank가 null은 안잡아서 null로 들어오는 경우 validation이 통과되었던것 같은데.. regex 때문에 걸릴 수 있을것 같기도하고.. 한번 체크해보시면 좋을것 같습니다
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.
혹시 몰라 한 번 체크해보니 null도 validation이 정상적으로 잘 잡아주는 것을 확인했습니다 ㅎㅎ
또한 구글링을 통해 NotNull,NotEmpty,NotBlank의 차이점을 한 번더 숙지했습니다!
import org.springframework.web.bind.annotation.RestController; | ||
|
||
@RestController | ||
@RequestMapping("/api/v1/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.
리소스 경로 설정이 아주 깔끔쓰하군요 👍
@Transactional | ||
public PostResponse update(Long postId, PostUpdateRequest updateRequest) { | ||
Post post = postRepository.findById(postId) | ||
.orElseThrow(() -> new BusinessServiceException(ErrorCode.NOT_FOUND_POST)); | ||
|
||
User user = userRepository.findById(post.getUser().getId()) | ||
.orElseThrow(() -> new BusinessServiceException(ErrorCode.NOT_FOUND_USER)); | ||
|
||
post.update(updateRequest.title(), updateRequest.content()); | ||
post.updateUser(user); | ||
|
||
return PostResponse.create(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.
아무리 JPA에 대한 미션이지만, Post의 주인과 수정 요청을 보낸 사람이 동일한 사람인지 검증하는 로직은 필요 없을까요?!
📌 과제 설명
SpringBoot + JPA를 사용하여 게시판을 구현한다.
👩💻 요구 사항과 구현 내용
요구 사항
SpringDataJPA 를 설정한다.
엔티티를 구성한다
API를 구현한다.
REST-DOCS를 이용해서 문서화한다.
User
Post