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

refactor: comments API URI 수정 #544

Closed
4 of 5 tasks
linirini opened this issue Nov 8, 2024 · 2 comments · Fixed by #549
Closed
4 of 5 tasks

refactor: comments API URI 수정 #544

linirini opened this issue Nov 8, 2024 · 2 comments · Fixed by #549
Assignees
Labels
confirm need confirmation! refactor 리팩토링 (변수 및 메서드 네이밍 변경)
Milestone

Comments

@linirini
Copy link
Contributor

linirini commented Nov 8, 2024

🤮 As Is

리팩터링하고자 하는 파트와 이유를 구체적으로 설명해주세요.

  • 댓글 조회 API의 URI를 수정하고자 합니다.
  • PUT, DELETE /comments?commentId={commentId} -> PUT, DELETE /comments/{commentId}
  • 필터링이 아닌, 특정 자원을 식별하기 위함이므로 Query String보다는 path variable이 적합하다고 판단하여 리팩토링하고자 합니다.

🤩 To Be

리팩터링 방향을 구체적으로 공유해주세요.

  • PUT, DELETE /comments?commentId={commentId} -> PUT, DELETE /comments/{commentId}
  • /v2/comments/{commentId}로 버저닝하여 안드로이드 개발 전까지 지원 예정입니다.
  • 운영 배포 시 강제 업데이트로 배포할 것인지, 아닌지에 따라 버저닝을 유지할 지 결정할 것 같습니다.

😇 이때까지 끝낼게요!

리팩터링 완료 예상 날짜를 작성해주세요. 신중하게 생각해요!

11/10까지 마무리 예정입니다. (이슈 컨펌 날짜 생각하여 넉넉하게 잡았어요)

😵 참고 자료(선택)

🙇‍♀️이슈 확인했어요:)

팀원에게 이슈 확인을 부탁해요!

@linirini linirini added the refactor 리팩토링 (변수 및 메서드 네이밍 변경) label Nov 8, 2024
@linirini linirini added this to the sprint-7 milestone Nov 8, 2024
@linirini linirini self-assigned this Nov 8, 2024
@linirini linirini added the confirm need confirmation! label Nov 9, 2024
@linirini linirini linked a pull request Nov 10, 2024 that will close this issue
@Ho-Tea
Copy link
Contributor

Ho-Tea commented Nov 11, 2024

궁금한 부분

기존에 존재하던 쿼리파라미터 형태에서 PathVariable형태로 변경하면서 버저닝이 아닌 아래와 같이 구성할 수 있을 것 같은데,
버저닝을 도입하신 이유가 궁금합니다.

    @GetMapping
    public ResponseEntity<CommentResponses> readCommentsByMomentId(
            @LoginMember Member member,
            @RequestParam @Min(value = 1L, message = "스타카토 식별자는 양수로 이루어져야 합니다.") long momentId
    ) {
        CommentResponses commentResponses = commentService.readAllCommentsByMomentId(member, momentId);
        return ResponseEntity.ok().body(commentResponses);
    }

    @GetMapping(path = "/{momentId}")
    public ResponseEntity<CommentResponses> readCommentsByMomentIdPathVariable(
            @LoginMember Member member,
            @PathVariable @Min(value = 1L, message = "스타카토 식별자는 양수로 이루어져야 합니다.") long momentId) {
        CommentResponses commentResponses = commentService.readAllCommentsByMomentId(member, momentId);
        return ResponseEntity.ok().body(commentResponses);
    }

@linirini
Copy link
Contributor Author

궁금한 부분

기존에 존재하던 쿼리파라미터 형태에서 PathVariable형태로 변경하면서 버저닝이 아닌 아래와 같이 구성할 수 있을 것 같은데, 버저닝을 도입하신 이유가 궁금합니다.

    @GetMapping
    public ResponseEntity<CommentResponses> readCommentsByMomentId(
            @LoginMember Member member,
            @RequestParam @Min(value = 1L, message = "스타카토 식별자는 양수로 이루어져야 합니다.") long momentId
    ) {
        CommentResponses commentResponses = commentService.readAllCommentsByMomentId(member, momentId);
        return ResponseEntity.ok().body(commentResponses);
    }

    @GetMapping(path = "/{momentId}")
    public ResponseEntity<CommentResponses> readCommentsByMomentIdPathVariable(
            @LoginMember Member member,
            @PathVariable @Min(value = 1L, message = "스타카토 식별자는 양수로 이루어져야 합니다.") long momentId) {
        CommentResponses commentResponses = commentService.readAllCommentsByMomentId(member, momentId);
        return ResponseEntity.ok().body(commentResponses);
    }

동일한 목적의 API가 2가지가 존재하는 것이므로, 명시해줄 필요가 있다고 생각했습니다. API를 사용하는 클라이언트 측에서도 어떤 API를 대신할 새로운 버전임을 분명하게 인지하고 다른 목적으로 API를 혼용하는 문제를 방지할 수 있다고 생각했습니다.

linirini added a commit that referenced this issue Nov 13, 2024
* feat: comment uri 변경 및 버저닝

* refactor: 불필요한 코드 변경을 최소화하기 위해 uri 상에서 버저닝 위치 변경
@linirini linirini closed this as completed by moving to Done in 2024-staccato Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirm need confirmation! refactor 리팩토링 (변수 및 메서드 네이밍 변경)
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants