[4주차] 임성현 /[feat] 추가 API 구현#158
Hidden character warning
Conversation
theSnackOverflow
left a comment
theSnackOverflow
left a comment
There was a problem hiding this comment.
동시성 제어(@Version), 중복 신고 다층 방어, 도메인 메서드 활용 등이 인상적으로 구현되어 있어요! 특히 Report에서 DB 유니크 제약과 서비스 레이어 체크를 이중으로 둔 점, Post.hide·Post.isOwner·Comment.pin/unpin처럼 도메인 행위를 엔티티 메서드로 녹여낸 설계가 읽기 좋았습니다. 몇 가지 확인해보시면 좋을 것 같은 부분 남겨봅니다!
추가로 가벼운 사항들도 정리했으니 참고해주세요:
- 다수 파일에 파일 끝 개행이 누락되어 있어요 (
Comment.java,Post.java,PostService.java외 다수). IDE의 "Ensure line feed at end of file" 옵션으로 일괄 적용하면 편리합니다. CommentController에 남아 있는📍디버깅 주석과Comment.java의 빈//주석은 정리해주시면 좋겠어요.CommentRepository.findAllByPostId와CommentPinRequestDTO는 현재 사용처가 보이지 않아 미사용 코드로 남아 있습니다.PostMapper.toHideResponse와PostHideResponse.from이 동일한 역할을 하면서 메시지 문구만 다르게 중복 존재합니다. 한 쪽으로 통일해보세요!
| // 게시물 숨기기 | ||
| @Transactional | ||
| public PostHideResponse hidePost(Long postId, Long userId) { | ||
| Post post = postRepository.findById(postId) |
There was a problem hiding this comment.
hidePost(Long postId, Long userId, PostHideRequest request) 오버로드가 함께 정의되어 있는데요, Controller에서는 hidePost(postId, userId) 시그니처만 호출하고 있어 request를 받는 첫 번째 메서드가 실제 요청 흐름에서 실행되지 않는 구조가 되어 있습니다. PR 설명에 비밀번호 인증이 언급되었지만 실제로는 적용되지 않는 상태예요. 두 메서드 중 의도에 맞는 하나로 통일해주시면 좋을 것 같아요!
| // 댓글 고정 | ||
| @Transactional | ||
| public CommentPinResponse pinComment(Long postId, Long commentId, Long userId) { | ||
| //게시글 존재 여부 확인 |
There was a problem hiding this comment.
pinComment 메서드가 PostService 안에 위치하고 있는데요, 댓글의 고정 여부를 변경하는 행위는 Comment 도메인의 책임에 가깝습니다. CommentService가 이미 존재하니 거기로 이동시키면 각 서비스의 역할 경계가 명확해질 것 같아요. post.isOwner(userId) 메서드도 그대로 재활용할 수 있으니 참고해보세요!
| public ResponseEntity<CommentResponse> createComment( | ||
| @PathVariable Long postId, | ||
| @RequestParam Long userId, | ||
| @RequestBody CommentRequest request) { // 📍 ) 를 넣어서 파라미터를 닫아줘야 합니다. |
There was a problem hiding this comment.
userId를 @RequestParam으로 받고 있는데요, 클라이언트가 임의 값을 보내도 서버에서 검증할 방법이 없어 인증을 우회할 수 있는 구조입니다. ReportController는 @AuthenticationPrincipal User user를 통해 인증 정보를 가져오고 있으니, 동일한 방식으로 통일해주시면 보안과 일관성 모두 챙길 수 있을 것 같아요!
|
|
||
| // 무엇을 신고하는지 구분 | ||
| private String type; | ||
|
|
There was a problem hiding this comment.
type 필드가 String으로 선언되어 있는데요, "POST".equalsIgnoreCase(...) 분기로 처리하다 보니 잘못된 값이 들어와도 마지막 throw IllegalArgumentException에서야 발견됩니다. ReportType 같은 enum으로 정의하면 역직렬화 시점에 잘못된 값을 즉시 거를 수 있고, 서비스 레이어의 분기도 좀 더 typesafe하게 정리할 수 있어요!
| @Service | ||
| @RequiredArgsConstructor | ||
| @Transactional | ||
| public class ReportService { |
There was a problem hiding this comment.
클래스 레벨에 @Transactional이 붙어 있어 모든 메서드가 쓰기 트랜잭션으로 동작하게 됩니다. 조회만 하는 메서드까지 불필요한 flush 비용이 생길 수 있어요. @Transactional(readOnly = true)를 클래스 레벨에, 실제로 변경이 일어나는 메서드에만 @Transactional을 별도로 두는 패턴을 참고해보세요!
| } | ||
|
|
||
| public void unpinAllComments() { | ||
| if (this.comments == null) return; |
There was a problem hiding this comment.
this.comments를 전체 순회해 핀된 댓글을 unpin()하는 구조인데요, post.comments가 lazy 로딩 컬렉션이라 댓글이 많을수록 불필요하게 전체를 로딩하게 됩니다. commentRepository.findByPostAndIsPinnedTrue(post)로 핀된 댓글만 조회하거나, @Modifying @Query로 업데이트 쿼리를 직접 날리는 방법이 좀 더 효율적일 것 같아요!
| // | ||
| @Column(name = "is_pinned", nullable = false) | ||
| private Boolean isPinned = false; // 📍 boolean -> Boolean으로 변경 | ||
|
|
There was a problem hiding this comment.
@Column(nullable = false)로 선언된 컬럼에 박싱 타입 Boolean이 사용되고 있는데요, DB에서 null이 들어올 수 없다면 primitive boolean이 더 자연스럽고 isPinned != null && 분기도 사라집니다. 또한 @Getter가 isPinned 기반 getter를 자동으로 생성하기 때문에 직접 정의한 public boolean isPinned() 메서드와 이름 충돌이 발생할 수 있어요. primitive로 바꾸고 직접 정의한 메서드를 제거하는 방향을 고려해보세요!
| ACTIVE, | ||
| HIDDEN, | ||
| DELETED //임시 저장 | ||
| } |
There was a problem hiding this comment.
DELETED 값 옆에 //임시 저장 주석이 달려 있는데요, enum 이름은 "삭제됨"을 의미하지만 주석은 "임시 저장(DRAFT)"을 가리키고 있어 의미가 엇갈립니다. 혹시 의도하신 건가요? 의도에 맞게 enum 이름을 DRAFT로 바꾸거나 주석을 "삭제됨"으로 수정해주시면 좋을 것 같아요!
|
POST/GET/PATCH 다양한 HTTP 메서드를 적절하게 사용하고, 200/201/400/409/500 등 상황에 맞는 HTTP 상태코드를 정확하게 반환하고 있는것 같습니다. 요청과 응답의 구조도 명확하게 정의되어 있고, 에러 메시지도 사용자가 이해하기 쉽게 작성되어 보기 좋은것 같습니다. API 명세가 상세해서 다른 개발자가 쉽게 사용할 수 있을 것 같습니다! |
|
이번 주차 과제도 고생 많으셨습니다! 특히 @Version을 활용한 동시성 제어나 자기 글 신고 불가 같은 세세한 비즈니스 예외 처리까지 챙기신 점이 인상 깊네용 👍 몇가지 의견을 드리자면 CommentController에서 userId를 직접 받기보다 @AuthenticationPrincipal을 활용해 인증된 정보를 쓰면 더 안전할 것 같아요! |
4 participants
[4주차] 임성현 /[feat] 추가 API 구현
1. 과제 요구사항 중 구현한 내용
구현한 기능
2. 핵심 변경 사항
return false;
}
3. 실행 및 검증 결과
4. 완료 사항
5. 추가 사항
#153
제출 체크리스트
{이름}/main브랜치다{이름}/{숫자}주차브랜치다