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] swagger 관련 코드 controller에서 분리 #141

Merged
merged 7 commits into from
Aug 21, 2024

Conversation

kssumin
Copy link
Contributor

@kssumin kssumin commented Aug 21, 2024

📌 관련 이슈

closed #135

✒️ 작업 내용

swagger 관련 코드를 interface에서 분리했습니다.

  • presentation 디렉터리를 보니 각 도메인마다 분리가 다르게 되고 있음을 확인했습니다. 따라서 아래와 같은 기준으로 분리했습니다.
    • presentation /docs : swagger 코드
    • presentation/controller : controller 코드
  • 다만 program 쪽은 Program, GuestProgram 을 분리하는 게 각 클래스에서 맞는 api를 찾기가 더 쉬울 것이라고 판단하여 분리했습니다.

swagger에서 memberId 파라미터 노출을 제거

  • 현 상황은 swagger에서 memberId 파라미터를 노출합니다. 하지만 이는 헤더에서 토큰을 추출하여 memberid 파라미터 값으로 변경하는 저희 서비스 내부의 구현 방법을 노출하고 있었습니다.
  • 즉 클라이언트 측에서는 토큰을 추출하여 memberId로 변경하고 있는지를 알고 있을 필요가 없다고 생각합니다.
  • 다만 헤더에 값을 넣어야 한다만 알아야 하기 때문에 헤더에 토큰 값이 필요할 때는 헤더에 토큰 값을 받도록 변경하였습니다.

Guest 권한을 나타내는 객체 디렉터리 위치 변경커밋

  • 본래 presentation 아래에 있었지만 게스트의 권한을 어느정도로 부여할 지 결정하는 것도 비즈니스의 일부라고 생각했습니다.(우리 서비스의 정책이기 때문)
  • 또한 GuestAccesRight -> GuestRight로 변경하여 추후 접근 권한 말고 다른 권한도 부여할 수 있는 책임을 가진 객체로 변경했습니다.

스크린샷 🏞️ (선택)

  • memberId를 노출했던 이전 swagger
    image

  • memberId를 노출하지 않는 swagger
    image

💬 REVIEWER에게 요구사항 💬

전체 테스트가 돌아가지 않아 확인을 보니 실패하는 테스트들이 존재했습니다.
하지만 이는 메서드 시그니처가 변경되어 발생하는 문제였고 추가된 기능에 대한 파악이 우선적으로 되어야 한다고 생각했습니다.
따라서 통과하지 않는 테스트, 테스트 코드 추가는 별도의 이슈를 통해 해결하고자 합니다.

@kssumin kssumin added the 🔧refactor 코드 수정 label Aug 21, 2024
@kssumin kssumin self-assigned this Aug 21, 2024
@rlajm1203
Copy link
Contributor

그러네요, 메소드 파라미터 부분에 memberId 값을 바인딩 하도록 해놓은 게 스웨거에서도 그대로 보이는군요...
제대로 확인을 못했는데 찾아주셔서 감사합니다!

Copy link

Unit Test Results

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ±0 

Results for commit ac05135. ± Comparison against base commit 04dd928.

Comment on lines 57 to 61
@PostMapping("/login/{oauthServerType}")
ApiResponse<SuccessBody<TokenResponse>> login(
public ApiResponse<SuccessBody<TokenResponse>> login(
@PathVariable String oauthServerType,
@RequestParam("code") String code,
@RequestParam("redirect_uri") String uri,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이전에 Slack으로 보내주신 사진을 보면, RequestMapping과 PathVariable, RequestParam도 모두 Interface로 빼놓았는데, 지금은 안 빼놓으신 이유가 Controller 메소드 코드를 봤을 때, 어떻게 동작하는지 바로 알 수 있게 하기 위함인가요?!

Copy link
Contributor Author

@kssumin kssumin Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵!! 맞아요!!

RequestMapping과 PathVariable, RequestParam은 web에 의존적인 것이라고 판단했어요!!

swagger가 아닌 다른 문서로 변경되었을 때 web 코드가 변경되어야 하는가?

만약 추후 swagger가 아닌 다른 문서로 변경이 된다고 했을 때 RequestMapping과 PathVariable, RequestParam와 같은 web 계층의 변화는 없어야 한다고 생각했어요.

해당 라이브러리들의 패키지를 기준

  • RequestMapping과 PathVariable, RequestParam는 package org.springframework.web.bind.annotation; 패키지에 속한다.
  • 즉 web 계층에 의존적이라고 생각했습니다.
image
  • 반면 swagger 어노테이션은 package io.swagger.v3.oas.annotations;에 속한다.
  • 즉 swagger 에 의존적이라고 생각했습니다.
image

이러한 패키지의 위치를 기준으로 interface로 나누었습니다.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 그렇네요 Swagger에 Web 어노테이션을 붙이면, Web 어노테이션이 Swagger와 공존해서, 의존성이 생겨버리니까 분리를 한 거군요! 이해됐습니다! 감사합니다!

@kssumin
Copy link
Contributor Author

kssumin commented Aug 21, 2024

@rlajm1203 주석 처리한 테스트를 통과시켜야지만 Ci가 통과할 것 같아요... ㅠㅠㅠㅠㅠㅠㅠ
별도의 이슈에서 테스트 코드를 추가해도 될까요?

@kssumin kssumin merged commit 4ad5a26 into develop Aug 21, 2024
1 of 3 checks passed
@kssumin kssumin deleted the MD/refactor/135/seperate-swagger branch August 21, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧refactor 코드 수정
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REFACTOR] swagger 관련 코드를 컨트롤러에서 인터페이스로 분리한다
2 participants