-
Notifications
You must be signed in to change notification settings - Fork 0
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
[FEAT/#18] 소셜 로그인 유즈케이스 구현 #19
base: dev
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.
고생하셨습니다 !
자잘한 궁금점은 리뷰에 적어놓았습니다 ! 바쁜 일정인데도 고생이 많으세요..
src/main/java/sopt/makers/authentication/application/auth/api/AuthApiController.java
Outdated
Show resolved
Hide resolved
|
||
import lombok.RequiredArgsConstructor; | ||
|
||
@Repository |
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.
P3
해당 어노테이션은 UserJpaRepository <>에 붙이는게 더 적절하다고 생각하는데, 성은님 의견이 궁금합니다!
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.
옹..!! 저는 성은이 방식이 적합하다고 생각했는데!!
본래 @Controller
, @Service
, @Repository
어노테이션들은 물론 각자마다의 기능이 조금씩 포함되어 있지만
"역할"을 나타내는게 강한 친구라고 생각해요!
(만약 빈 등록만이 목적이라면 @Component
를 사용해도 무관하니까요!!)
본 객체는 Usercase In 포트를 구현하는 Service 객체에 주입이 필요하니 빈 등록이 필요함과 동시에 "저장소"라는 의미를 전달할 수 있어야 하기 때문에 @Component
보다는 @Repository
가 더 적합해 보이구요!!
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.
JpaRepository를 상속받을 경우 스프링부트에서는 자동으로 빈 등록을 해준다고 하더라고요
그래서 중복해서 어노테이션을 붙일 필요는 없다는 입장이었습니다!
다만 in 포트를 구현해야하는 객체인 UserRepositoryImpl은 빈 등록을 해야하기에 고민하다가 Repository를 선택하게 되었습니다! 동규오빠와 같은 의견이었던 것 같아요 ㅎㅎ
src/main/java/sopt/makers/authentication/database/rdb/repository/UserJpaRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/sopt/makers/authentication/support/constant/OAuthConstant.java
Outdated
Show resolved
Hide resolved
src/main/java/sopt/makers/authentication/application/auth/api/AuthApi.java
Outdated
Show resolved
Hide resolved
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.
바쁜 와중에 고생했어요!! :)
몇 가지 궁금증과 제안을 적어놨는데 편하게 보시구
현욱이가 남겨준 내용 논의 끝나면 머지해도 큰 문제 없어보입니다!!
(그래서 우선 Request changes 요청이 되어있으니 저는 approve를 하도록 하겠습니다!)
@PostMapping("/web/login") | ||
public ResponseEntity<BaseResponse<?>> authenticateSocialAuthInfoFromWeb( | ||
AuthRequest.AuthenticateSocialAuthInfo socialAuthInfo) { | ||
AuthenticateTokenInfo tokenInfo = | ||
authenticateSocialAccountUsecase.authenticate(socialAuthInfo.toCommand()); | ||
HttpHeaders headers = cookieUtil.setRefreshToken(tokenInfo.refreshToken()); | ||
|
||
return ResponseEntity.ok() | ||
.headers(headers) | ||
.body( | ||
BaseResponse.ofSuccess( | ||
AUTHENTICATE_SOCIAL_ACCOUNT, | ||
AuthResponse.AuthenticateSocialAuthInfoForWeb.of(tokenInfo.accessToken()))); | ||
} | ||
|
||
@Override | ||
@PostMapping("/web/app") |
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.
p3
사소한 것이긴 한데
web/login
/ app/login
으로 지정하신 이유가 따로 있는지 궁금합니다!!
저는 보통 path를 정할 때, 패키징을 많이 생각하는데요!
login
까지는 공통 관심사이고 플랫폼(web/app)에서 관심사가 나뉘어지는 논리적인 상황에서
플랫폼에 대한 path가 먼저 나오게 된다면 플랫폼이 먼저 구분지어진다고 느껴져요!!
또한 Controller의 정의 규칙도 보통 path 기반으로 관심사를 분리하고 정의하게 되는데
위와 같은 방식으로 진행한다면 앞으로의 플랫폼 구분이 필요하 기능들은 플랫폼 path가 앞에 오게될거에요!!
그로 인해 Controller 분리가 필요해진다면 login 기능만 분리한 AuthLoginController 로 분리되기 보다는 AppController, WebController로 분리될 가능성이 커보이구요!!
물론 정답은 없는 요소이지만 정책을 정하면 좋을 것 같아 얘기해봅니다!!
(cc. @hyunw9 씨의 의견도 궁금해요!)
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.
이전에 전달받은 토큰 운용 관련 문서(cc. https://www.notion.so/sopt-makers/API-Token-045310ea8c5749e3b27846e2384b1c5b?pvs=4) 대로 구현을 하긴 했는데요!
다시 여쭤보지 않았던 이유는 이 문서를 전달해주실 때 제가 없는 자리에서 관련된 이야기가 나왔는데 그 내용을 제가 인지하지 못하고 있어서 오빠가 관련 내용을 작성해서 보내주신다고 하셨던거로 기억해요. 그래서 이미 논의가 끝난 사안이라고 생각하여 문서에 적힌 경로 및 Response DTO 및 토큰을 그대로 구현했습니다. 만약 클라이언트와 협의된 내용이 아니고 오빠가 작성해주신 내용이 클라이언트 분들과 경로에 대해 협의가 된 사안이 아니라 변경해도 된다면 저도 변경하는 것이 더 좋다고 생각합니다!
|
||
import lombok.RequiredArgsConstructor; | ||
|
||
@Repository |
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.
옹..!! 저는 성은이 방식이 적합하다고 생각했는데!!
본래 @Controller
, @Service
, @Repository
어노테이션들은 물론 각자마다의 기능이 조금씩 포함되어 있지만
"역할"을 나타내는게 강한 친구라고 생각해요!
(만약 빈 등록만이 목적이라면 @Component
를 사용해도 무관하니까요!!)
본 객체는 Usercase In 포트를 구현하는 Service 객체에 주입이 필요하니 빈 등록이 필요함과 동시에 "저장소"라는 의미를 전달할 수 있어야 하기 때문에 @Component
보다는 @Repository
가 더 적합해 보이구요!!
Related Issue 🚀
Work Description ✏️
usercase
: 유저로부터 role & id 값을 찾아와 CustomAuthentication을 생성했습니다application
: web, app 경로를 나누었고 resonse도 구분하여 작성해두었습니다return ResponseEntity~~
에 대한 코드가 중복된다고 생각했습니다.PR Point 📸