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

[Global/aos] Bearer Interceptor 구현 #110

Merged
merged 3 commits into from
Nov 23, 2023

Conversation

yy0ung
Copy link
Collaborator

@yy0ung yy0ung commented Nov 23, 2023

주요 작업

  • refresh token 재발급 api 연동
  • access token 만료 시 재발급 후 다시 요청 보내는 로직 구현

완료한 task 명세

  • BearerInterceptor 구현

결과 화면

  • access token 만료되지 않았을 경우
2023-11-23.3.44.11.mov
  • access token 만료되었을 경우 재발급 로직 (임시로 값을 이상하게 줬음)
2023-11-23.3.45.04.mov

리뷰 요청 사항

  • 일단 Application context 쓰는건 좀 더 알아봐야 할 것 같아서 그부분 빼고 구현했습니다~!

Related to issue #86

@yy0ung yy0ung added feat 기능 구현 AOS 작업범위 : Android labels Nov 23, 2023
@yy0ung yy0ung added this to the etc milestone Nov 23, 2023
@yy0ung yy0ung requested review from BENDENG1 and plashdof November 23, 2023 14:24
@yy0ung yy0ung self-assigned this Nov 23, 2023
Copy link
Collaborator

@BENDENG1 BENDENG1 left a comment

Choose a reason for hiding this comment

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

util 패키지에 constants관리하는데 같이 관리하는거는 어떻게 생각하는지!

고생했어 👍

Comment on lines +72 to +78
val retrofit = Retrofit.Builder()
.baseUrl(Constants.BASE_URL)
.addConverterFactory(GsonConverterFactory.create())
.client(okHttpClient)
.build()
val api = retrofit.create(RefreshApi::class.java)
return runRemote { api.refreshToken(RefreshTokenRequest(refreshToken)) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분에 대해서 왜 retrofit을 di를 안했지? 라는 의문이 들었는데

    @Provides
    fun provideRetrofit(okHttpClient: OkHttpClient): Retrofit {

        return Retrofit.Builder()
            .baseUrl(BASE_URL)
            .client(okHttpClient)
            .addConverterFactory(GsonConverterFactory.create())
            .build()
    }

내가 이해한 바로는 retrofit -> okhttpclient -> bearerinterceptor -> retrofit 이렇게 순환참조가 일어나서 이렇게 선언을 했다고 생각하는데 생각하는 바가 맞는걸까?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞아!! 나도 di 로 하려고했는데 사이클이 돌아서 안되더라고..

Comment on lines +81 to +85
companion object {
const val TOKEN_ERROR = 401
const val AUTHORIZATION = "Authorization"
const val BEARER = "Bearer"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분 util패키지 -> constants에서 관리하고 있는데 거기다가 진행하는 거는 어떨까?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

constants 에 지금 상수값이 거의 다 들어가있어서 좀 나눠서 관리하는건 어떨까 싶어. 예를들어 Constant 라고만 하면 나중에 무슨 상수값인지 모르니까 값을 불러오려면 파일에 들어가서 확인해야하는데, Error, Network, Token 뭐 이런식으로 하면 쉽게 찾을 수 있을것 같아. 장단이 있을거 같은데 얘기해보면 좋을듯!

Comment on lines +48 to +55
@Provides
fun provideAccessTokenInterceptor(dataStoreManager: DataStoreManager): AccessTokenInterceptor =
AccessTokenInterceptor(dataStoreManager)

@Provides
fun provideBearerInterceptor(dataStoreManager: DataStoreManager): BearerInterceptor =
BearerInterceptor(dataStoreManager)

Copy link
Collaborator

Choose a reason for hiding this comment

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

accesstokenInterceptor 없었는데 같이 추가 👍👍👍👍

Copy link
Collaborator

@plashdof plashdof left a comment

Choose a reason for hiding this comment

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

👍👍👍

(newAccessToken)?.let {
val newRequest = originalRequest.newBuilder()
.addHeader(AUTHORIZATION, "$BEARER $newAccessToken")
.build()
Copy link
Collaborator

Choose a reason for hiding this comment

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

기존 request 가 열려있으면 에러가 나서
여기서 response.close() 를 해줘야 정상적으로 새로운 request가 내려갔던 것으로 기억함!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

같은 파일 41 줄에 response.close() 넣어뒀어! 어디 넣어하는건지 잘 몰라서 일단 재발급 성공한 직후에 넣어뒀는데 여기로 옮기는게 나을까?

Copy link
Collaborator

Choose a reason for hiding this comment

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

아! 못봤다ㅜㅜ 기존 위치가 좋은거 같아

}
}
}
(newAccessToken)?.let {
Copy link
Collaborator

Choose a reason for hiding this comment

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

사소한 컨밴션 이지만 이거 괄호 왜 썼는지?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

연산을 하려고 했는데 연산을 지우면서 같이 안 지운거같아 ㅋㅋㅋㅋㅋ 다음 커밋때 지울게요!

@plashdof plashdof merged commit 107f165 into boostcampwm2023:feature/aos-data Nov 23, 2023
@yy0ung yy0ung deleted the feature/aos-data branch November 25, 2023 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AOS 작업범위 : Android feat 기능 구현
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants