-
Notifications
You must be signed in to change notification settings - Fork 1
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
[HW/#7] 3차 세미나 실습과제 #8
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 static <T> SuccessStatusResponse of(SuccessMessage successMessage, T responseDto) { | ||
return new SuccessStatusResponse(successMessage.getStatus(), successMessage.getMessage(), responseDto); | ||
} |
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.
SuccessStatusResponse처럼 반환타입에도 제네릭을 붙여주세요.
현재 코드에서 는 responseDto에만 적용되어 responseDto에만 타입 안전성이 보장됩니다.
반환타입에도 붙이면 가독성, 안전성을 향상시킬 수 있습니다!
|
||
@PostMapping("post/{blogId}") | ||
public ResponseEntity<SuccessStatusResponse> publishPost( | ||
@RequestHeader(name = "memberId") Long memberId, | ||
@PathVariable(name = "blogId") Long blogId, | ||
@Valid @RequestBody PostCreateRequest postCreateRequest | ||
) { |
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.
rest api에서는 복수형이 선호된다고 알고 있습니다.
그리고 endpoint를 /blogs/{blogId}/posts 로 하는 것은 어떨까요?
조금 더 rest에 부합한다고 생각합니다. 블로그중 blogId에 해당하는 blog에 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.
오 endpoint에 대한 고민이 많았는데 조언해주신대로 바꿔보겠습니다 감사합니다!
return ResponseEntity.status(HttpStatus.CREATED).header( | ||
"Location", | ||
postService.create(memberId, blogId, postCreateRequest)) | ||
.body(SuccessStatusResponse.of(SuccessMessage.POST_CREATE_SUCCESS)); |
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.
.created()를 사용해도 좋을 것 같습니다
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.
.created() 등 관련된 메소드에 대해서 잘 몰랐는데 이 기회에 더 공부해보겠습니다 👍 👍
if (!requestMemberId.equals(findMemberId)) { | ||
throw new CustomValidateException(ErrorMessage.BLOG_UNAUTHORIZED); | ||
} |
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.
BLOG_UNAUTHORIZED가 404를 갖고 있는데 이 경우에는 권한이 없다는 403이 더 적합하지 않을까요?
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.
동의합니당 404를 쓰기에는 적절하지 않은 것 같아요!
|
||
@GetMapping("post/{blogId}") |
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.
마찬가지로 /blogs/{blogId}/posts가 더 적합해 보입니다.
|
||
public PostListFindDto findAllPost(Long memberId, Long blogId){ | ||
|
||
Blog findBlog = blogService.validateOwner(blogId, memberId); |
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.
조회는 모든 사람들이 볼 수 있도록 해주는 것이 좋을 것 같은데 validateOwner를 추가하신 이유가 궁금합니다!
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.
오... 요구사항 로직이 머릿속에서 꼬여서 이렇게 해버린 것 같습니다..ㅎㅎ 바꿔보도록하겠습니다!
|
||
} | ||
|
||
@GetMapping("post/{blogId}/{postId}") |
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.
여기는 /posts/{postId} 또는 /blogs/{blogId}/posts/{postId} 둘중에 하나가 rest에 더 부합하는 것 같습니다
|
||
public PostFindDto findPost(Long memberId, Long blogId, Long postId){ | ||
|
||
blogService.validateOwner(blogId, memberId); |
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.
서진님 고생하셨습니당~! 갓재연님이 좋은 리뷰 많이 달아주셔서 구경 겸... 조금씩 보이는거 리뷰 달아봤어요
전반적으로 코드가 깔끔해서 알아보기 편하고 좋았습니다 ㅎㅁㅎ
if (!requestMemberId.equals(findMemberId)) { | ||
throw new CustomValidateException(ErrorMessage.BLOG_UNAUTHORIZED); | ||
} |
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.
동의합니당 404를 쓰기에는 적절하지 않은 것 같아요!
import jakarta.validation.constraints.NotBlank; | ||
|
||
public record PostCreateRequest( | ||
@NotBlank(message = "글의 제목은 필수입니다.") |
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.
과제하느라 고생했어요!
이미 좋은 리뷰가 많이 달려있어서 구경했네요! 코멘트 확인하고 반영하면 좋을 것 같아요!
|
||
MEMBER_NOT_FOUND_BY_ID_EXCEPTION(HttpStatus.NOT_FOUND.value(), "ID에 해당하는 사용자가 존재하지 않습니다."), | ||
BLOG_NOT_FOUND(HttpStatus.NOT_FOUND.value(), "ID에 해당하는 블로그가 없습니다."), | ||
BLOG_UNAUTHORIZED(HttpStatus.NOT_FOUND.value(), "해당 블로그의 소유자가 아닙니다."), |
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.
Status가 Unauthorized 가 아니네요! 한 번 확인해보시는 걸 추천드립니다!
|
||
|
||
|
||
|
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.
요 네줄은 사용되지 않으니 클린코드를 위해 지우는 방향이 어떨까요?
3차 세미나 pr
명세서
API 명세서
공통부분
SuccessStatusResponse
<응답 예시>
CustomValidateException
블로그 소유주 검증
BlogService
CustomValidateException
발생postId로 조회하는 메서드 추가
PostService
Post 작성 API
dto
controller
/api/v1/post/{blogId}
로 하였습니다.service
validateOwner
를 이용하여 블로그 소유주를 검증합니다.Post 개별조회 API
dto
controller
/api/v1/post/{blogId}/{postId}
로 하였습니다.SuccessMessage.POST_FIND_SUCCESS
) 와PostFindDto
를 리턴합니다.service
validateOwner
를 이용하여 블로그 소유주를 검증합니다.Post 전체조회 API
dto
controller
/api/v1/post/{blogId}
로 하였습니다.SuccessMessage.POST_FIND_SUCCESS
) 와PostListFindDto
를 리턴합니다.service
validateOwner
를 이용하여 블로그 소유주를 검증합니다.고민
API URL
/api/v1/post/{blogId}/{postId}
로 했는데 적절한지 의문입니다../api/v1/blog/posts
로 설정해야해도 괜찮을까요..?blog 소유주 검증 위치
BlogService
에 만들었는데, 이게 적절한지 의문입니다.validateOwner
인데 return 타입은 Blog라 좋은 코드인지 잘 모르겠습니다.(뭔가 Boolean이 더 적절해보여서..)PostService
에선 검증없이 리턴값을 바로 받아서 사용하는데, 해당 메소드 안에서CustomValidateException
예외처리를 해주니 그냥 사용해도 괜찮을까요..? 뭔가 정말 예기치 않은 오류가 터질까 불안해서..