Conversation
* chore: 안드로이드 프로젝트 폴더 생성 (#2) * feat: 안드로이드 프로젝트 초기 세팅 * chore: gitignore 수정 및 불필요한 파일 삭제
* feat: 프로젝트 초기 세팅 * chore: .DS_Store 파일 삭제 및 gitignore 추가 * chore: .DS_Store 파일 삭제 및 gitignore 추가
* chore: 프로퍼티 파일 수정 Co-authored-by: dooboocookie <[email protected]> Co-authored-by: chaewon121 <[email protected]> Co-authored-by: zillionme <[email protected]> * feat: Member 엔티티 생성 Co-authored-by: dooboocookie <[email protected]> Co-authored-by: chaewon121 <[email protected]> Co-authored-by: zillionme <[email protected]> * feat: Place 엔티티 생성 - Position 도메인 생성 Co-authored-by: dooboocookie <[email protected]> Co-authored-by: chaewon121 <[email protected]> Co-authored-by: zillionme <[email protected]> * feat: Game 엔티티 생성 Co-authored-by: dooboocookie <[email protected]> Co-authored-by: chaewon121 <[email protected]> Co-authored-by: zillionme <[email protected]> * feat: Position의 동등성 기능 추가 --------- Co-authored-by: dooboocookie <[email protected]> Co-authored-by: chaewon121 <[email protected]> Co-authored-by: zillionme <[email protected]>
* feat: 위치를 나타내는 Dto 생성 * feat: 목적지를 나타내는 Dto 생성 * chore: gitkeep 파일 삭제
* refactor: 좌표를 나타내는 Dto 이름 변경 * feat: 좌표를 저장하는 도메인 클래스 생성 * feat: 목적지를 저장하는 도메인 클래스 생성 * feat: 도메인 repository 인터페이스 생성
* feat: 네이버 맵을 사용하기 위한 의존성을 gradle에 추가 * feat: manifest에 네이버 맵을 사용하기 위한 메타 데이터 추가 * feat: 액티비티 네이밍 변경, 네이버 맵을 화면에 띄우는 기능 구현 * chore: .gitignore 수정 * chore: 네이버 지도 사용을 위해 gradle.properties에 제티파이어를 추가하여 버전 차이 문제를 해결
* feat: 회원정보 추출 기능 구현 * feat: 회원 인증 어노테이션 커스텀 구현 * feat: 인터셉터 추가 * refactor: argumentResolver 리팩토링 * feat: 설정정보 인터셉터 추가 * refactor: 회원 정보 조회 예외처리 중복 수정
* chore: repository 패키지 위치 이동 * feat: 모험시작 액티비티 생성 - 필요한 위치권한 Manifest에 등록 - 위치권한 요청 * design: UI관련 resource 추가 * feat: 게임 시작 기능 액티비티 추가 및 권한 요청 기능 구현 * design: 문자열 strings 리소스화 * design: 배경 및 버튼 색상 변경
* feat: 위치 권한을 승인한 경우 모험 액티비티로 이동하는 기능 구현 * feat: 내 위치를 잡으면 트래킹하도록 기능 구현 * feat: isLocationButtonEnabled 활성화 구현
* chore: 프로퍼티 파일 수정 * feat: 위치 기준으로 반경 거리내의 장소 추천 쿼리 작성 * feat: Game 엔티티 빈 생성자 추가 * refactor: 패키지명 수정 * chore: 더미 데이터 추가 * feat: Game 엔티티 생성 * feat: Place 서비스 계층 생성 feat: 장소 선정 로직 기능 구현 * feat: GameRepository 생성 * feat: Game 서비스 계층 생성 feat: 장소 선정 기능 구현 * feat: Place 응답 DTO 구현 * feat: Game 컨트롤러 계층 생성 * style: 코드 정렬
* feat: repository 목 구현체 생성 * feat: OnAdventureViewModel 생성 * refactor: 거리 반환값의 자료형을 Int로 수정 * feat: 목적지와 현재위치 사이 거리를 계산하여 사용자에게 보여주는 기능 구현
- 목적지를 임시로 상수처리 - 추후 목적지를 서버에서 받아 마커를 찍는 방식으로 수정 필요
* design: close icon 추가 * feat: 화면에 관련된 유틸 파일과 메서드 생성 * design: OnAdventure 액티비티에서 사진을 보여주기 위한 다이얼로그 UI 구현 * feat: 목적지에 대한 사진을 띄워주는 DestinationPhotoDialog 구현 * refactor: LocationDialogFragment의 메서드를 DisplayUtil로 분리 * style: 코드 정렬 * feat: 생성시 액티비티로부터 사진을 받아오는 기능 구현 * design: 사진 아이콘 버튼 추가 * feat: 액티비티에서 사진 아이콘 버튼 클릭시 다이얼로그를 띄우는 로직 구현 * feat: 사진이 로딩중 문구 strings 리소스화 * feat: 뷰모델로부터 얻어온 사진을 넘기도록 로직 수정 * refactor: 다이얼로그 및 변수 네이밍 변경 * refactor: strings 리소스화 * refactor: 객체 네이밍 변경
* design: 비활성화 버튼 color resource 추가 * design: 버튼의 상태에 따라 버튼 UI가 변경 되는 selector 생성 * feat: 목적지 도달 시 모험 종료 버튼 활성화 기능 추가 * refactor: 기존에 있는 drawable 활용해 selector 생성 * refactor: 도착 버튼 string을 resource에 추가
* refactor: 조회 쿼리의 결과를 List로 받도록 수정 * feat: 인증 관련 예외 기능 추가 * feat: 게임 관련 예외 기능 추가 * feat: 장소 관련 예외 기능 추가 * feat: 회원 관련 예외 기능 추가 * feat: 수정된 API 명세에 맞춰 게임 서비스 로직 수정 * feat: 수정된 API 명세에 맞춰 게임 서비스 로직 수정 * feat: 수정된 API 명세에 맞춰 장소 서비스 로직 수정 * feat: 수정된 API 명세에 맞춰 회원 서비스 로직 수정 * feat: 수정된 API 명세에 맞춰 게임 서비스 로직 수정 * feat: 게임에 대한 응답 DTO 생성 * feat: 수정된 API 명세에 맞게 컨트롤러 수정 * feat: 수정된 API 명세에 맞게 컨트롤러 수정 * style: import문 최적화 * refactor: 기존 런타임 예외를 인증 커스텀 예외로 수정 * refactor: 사용하지 않은 메서드 삭제 * refactor: 상수 이름 변경 * refactor: final 키워드 추가 * refactor: @transactional 어노테이션에 readOnly 속성 추가 * refactor: 패키지 수정 --------- Co-authored-by: kokodak <[email protected]>
* feat: Adventure 도메인 클래스 생성 * feat: Adventure 상태를 갖는 enum 클래스 생성 * feat: 기능 변경에 따른 repository 변경 * chore: 사진 다이얼로그 패키지 위치 변경 * feat: AdventureDto 클래스 생성 * feat: Dto와 domain 클래스간의 mapper 구현 * refactor: repository 이름 변경 * feat: repository 기능 변경으로 인한 mock repository 변경 * feat: retrofit service 인터페이스 생성 * feat: adventure ERROR 상태 클래스 추가 * feat: Repository 변경에 따른 ViewModel 변경 및 서버 통신 오류 처리 * feat: Retrofit 객체 생성 및 interceptor 구현 * chore: AdventureService 패키지 위치 변경
* feat: 좌표사이의 거리를 키로미터로 계산하는 로직 구현 * feat: 도착여부 판단 및 접근 권한 검증 기능 구현 * feat: 장소의 유효범위 여부 검증 구현 * feat: 게임 종료기능 api 구현 * refactor: 게임 관련 예외를 사용하도록 변경 * refactor: 인증 관련 예외 추가 및 적용 * style: final 키워드 추가 및 코드 정렬 * fix: 파라미터명 수정
* chore: step 이름 추가 * fix: default working directory 추가
* chore: step 이름 추가 * fix: default working directory 추가 * fix: working directory 상대경로로 변경
* refactor: AdventureRepository 수정 * feat: Coordinate 객체의 Domain to Dto 매핑 함수 생성 * feat: Result의 failure에서 사용할 나아가 팀만의 Throwable 생성 * feat: 서버통신이 성공적이지 않을 때에 대한 객체와 서버통신 코드를 단순화할 메서드 생성 * feat: AdventureRepository의 beginAdventure 함수 구현 * refactor: 코드 방식 변경 get() -> [] * feat: MockRepository의 beginAdventure 함수 구현 * refactor: 타입을 표현해줌 * feat: 실제 base url 등록 * feat: http 통신 허용 * fix: json 변환 과정 수정 * fix: TODO 주석처리 * feat: 코루틴 의존성 추가 * feat: 모험을 시작하는 서버통신 구현
* feat: 모험 종료 시 서버에게 데이터를 받아 올 Dto 생성 * feat: 모험 종료 시 서버에게 받아 온 Dto mapper 생성 * refactor: 데이터 반환 값 타입 변경 * feat: AdventureRepository의 endAdventure 구현 * feat: 모험 종료 시 서버와 통신해 현재 모험 상태를 받아온 후 상황에 맞게 분기 처리 해주는 기능 구현 * refactor: mock Repository의 endAdventure 함수 구현 * fix: onClick에서 사용된 함수에게 인자를 넣어 Databinding 오류 해결
* refactor: 게임 정보를 가져오는 서비스 수정 * feat: 서버에서 목적지를 받아오는 기능 구현
* fix: RetrofitService Path 수정 * fix: Serializable 어노테이션 추가 * refactor: iteration 명칭 변경 및 타입 명시 제거
- 정상적인 리프레시를 위함
- authDataSource의 메서드를 사용하는 것에서 DefaultAuthRepository의 메서드를 사용하도록
- Repository 테스트를 하지 않아 불필요하다고 생각
There was a problem hiding this comment.
고생하셨습니다 크롱! AuthService의 몇몇 함수에 직접 헤더를 넣어주는 식으로 이전에 하던 고민을 스스로 잘 해결하셨네요!
그럼 질문에 대한 답변 및 제 생각을 적어보겠습니다.
AuthInterceptor가 AuthRepository를 갖게 하는 것에 대해 어떻게 생각하는지 궁금합니다.
우선 코드 분리 자체로는 괜찮은 것 같습니다. 다만 이 것이Respository인가? 라고 한다면 물음표인 것 같습니다.
기본적으로 데이터는 Data - Domain - Presentation 으로 흐릅니다. 여기서 우리는 도메인에 Repository 인터페이스를 두면서 DIP를 통해 마치 Data가 Domain에 의존하도록 만들죠.
이 관점에서 바라봤을 때 우선 AuthRepository의 행동들이 우리의 Domain에 드러나야 하는가 생각을 하게 됩니다.
아래는 AuthRepository의 함수들입니다.
suspend fun logIn(platformAuth: PlatformAuth): Boolean
suspend fun withdrawalMember()
suspend fun logout()
suspend fun refreshAccessToken()
fun getAccessToken(): String?
fun getRefreshToken(): String
fun storeToken(accessToken: String, refreshToken: String)저는 여기서 로그인, 로그아웃, 회원탈퇴를 제외하고는 서비스의 기능이라고 보기 어렵다고 생각합니다.
즉, refreshAccessToken, getAccessToken, getRefreshToken, storeToken 모두 Domain과 관련 없이 Data Layer 안에서 수행되는 행동들이라고 생각합니다.
이 부분을 해결하려면 AuthInterceptor에서 LocalAuthDataSource와 RemoteAuthDataSource를 모두 주입 받게 됩니다. 이 것을 보고 이럴거면 그냥 AuthRepository로 쓰는게 낫지 않아? 라고 할 수도 있다고 생각합니다.
하지만 우리가 제이슨에게 들었던 짬통 강의를 생각한다면 저는 도메인에 드러나지 않는 쪽이 더 맞다고 생각합니다!
로그아웃 or 회원탈퇴를 진행하는 경우 액세스 토큰 대신 리프레시 토큰을 사용하는 것은 어떻게 생각하는지 궁금합니다.
저는 엑세스 토큰으로 로그아웃 하는 것이 맞다고 생각합니다.
우리 팀 백엔드에선 엑세스 토큰이 만료되지 않은 상태에서 리프레시 요청을 보낼 시 탈취된 토큰으로 간주하고 에러를 던집니다. 이 것은 리프레시 토큰이 탈취된 상황을 위한 방어 로직으로 생각됩니다.
그런데 리프레시 토큰으로 회원 탈퇴와 같은 중요한 api를 호출한다는 것은 리프레시 토큰이 탈취된 상황에 대한 방어로직의 의미를 잃게 합니다.
또한 의미적으로도 맞지 않다고 생각합니다. 리프레시 토큰은 단순히 리프레시를 위한 토큰이고, 엑세스 토큰이 우리가 백엔드에 요청을 보내 접근할 수 있는 인증 정보라고 생각합니다.
추가 리뷰
리프레시 로직과 관련되지 않은 내용들이 조금 있었던 것 같습니다. pr 설명에 없던 갑자기 다른 내용이 등장해서 컨텍스트 스위칭이 잘 되지 않았던 경우가 조금 있었습니다!
| @Qualifier | ||
| @Retention(AnnotationRetention.BINARY) | ||
| annotation class AuthRetrofit | ||
|
|
||
| @Qualifier | ||
| @Retention(AnnotationRetention.BINARY) | ||
| annotation class NormalRetrofit |
There was a problem hiding this comment.
[P2]
네이밍에서 혼란이 올 수 있을 것 같습니다.
사실 저는 처음에 AuthRetrofit이 Auth관련 로직에서 사용하는 retrofit인 줄 알고 오랫동안 해멨습니다.
제가 느끼기엔 의미가 반대로 된 것 같은데 다른 사람은 어떨지 뽀또의 의견도 궁금하네요.
더해서 제 시선(현재 상태에서 반대로 된)으로 간다면 NormalRetrofit 보다는 그냥 Common 같은 용어가 조금 더 어울릴 것 같습니다.
There was a problem hiding this comment.
어디에 중점을 두고 네이밍을 하냐에 따라 달라질 것 같습니다!
빅스는 아마 사용되는 위치에 중점을 두고 생각해 현재와 같은 네이밍이 헷갈린다고 말한 것 같네요!
전 개인적으로 현재처럼 auth를 담고 있는지에 대한 여부로 네이밍 된 것이 괜찮다 생각합니다
만일 사용되는 위치에 중점을 담아 네이밍이 된다면?
auth 를 담고 있는 NormalRetrofit과 auth를 담지 않고 있는 AuthRetrofit이라는 네이밍이 왠지 전 더 헷갈릴 것 같은 느낌ㅎㅎ
추가로 빅스가 말한 Normal을 Common 같은 용어로 바꾸자는 의견에는 적극 동의합니다!
There was a problem hiding this comment.
이 부분에서 혼란이 생길줄은 몰랐는데, 저는 Auth를 담고있다는 의미로 사용했던 것입니다. 확실히 '사용처'의 시선으로 본다면 그렇게 생각할 수도 있을 것 같습니다. 어떤 네이밍을 사용하던 저희가 인지하고 있는 내용이 같도록 하는 것이 중요한 것 같습니다.
일단은 뽀또의 의견도 저와 같은 것 같아서 그대로 두었는데, 뽀또 의견을 들은 빅스가 바꾸는 것이 나을 것 같다고 이야기한다면 따라가도록 하겠습니다.
추가로 Normal대신 Common으로 변경했습니다!
| is IOException -> { | ||
| _throwable.value = DataThrowable.AuthorizationThrowable(EXPIRATION_AUTH_ERROR_CODE, "") | ||
| } |
There was a problem hiding this comment.
[P3]
IOException이면 전부 엑세스 토큰 만료인가요? 몰라서 여쭤봅니다
There was a problem hiding this comment.
정확히 그런것은 아닙니다. Interceptor에서는 IOException만 던질 수 있습니다. 그래서 뷰모델에서 IOException을 캐치하면 토큰 만료인 것으로 만들었는데요.
빅스의 코멘트를 보고 이 처리는 충분히 문제가 있을 것 같다는 생각을 하고, 인증 관련 문제로 예외가 발생되었을 때만 AuthorizationThrowable 로 만들어주도록 하였습니다.
| DataThrowable.NETWORK_THROWABLE_CODE -> { showToast(getString(R.string.network_error_message)) } | ||
| EXPIRATION_AUTH_ERROR_CODE -> { showToast(getString(R.string.splash_re_login_message)) } |
There was a problem hiding this comment.
[P3]
한 줄이라면 중괄호를 없애보는 건 어떨까요?
| fun getRefreshToken(): String | ||
| fun storeToken(accessToken: String, refreshToken: String) |
There was a problem hiding this comment.
[P2]
혹시 인터페이스에 정의된 이유가 있을까요? 외부에서 사용되지 않은 것처럼 보여서 여쭤봅니다! (혹시 잘못 본 거라면 알려주세요!)
There was a problem hiding this comment.
오 그랬네요! 꼼꼼한 리뷰 감사합니다~
| class TestAuthDataSource() : AuthDataSource { | ||
| override fun getAccessToken(): String? { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override fun setAccessToken(newToken: String) { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override fun getRefreshToken(): String? { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override fun setRefreshToken(newToken: String) { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override fun resetToken() { | ||
| TODO("Not yet implemented") | ||
| } | ||
| } | ||
|
|
||
| @Module | ||
| @InstallIn(SingletonComponent::class) | ||
| class TestDataSourceModule { | ||
| @Singleton | ||
| @Provides | ||
| fun provideTestAuthDatasource(@ApplicationContext context: Context): AuthDataSource { | ||
| return TestAuthDataSource() | ||
| } | ||
| } |
There was a problem hiding this comment.
[P2]
위 테스트에서 일어난 변경 전체적으로 설명이 필요할 것 같습니다. 이해가 어렵네요 ㅠ
There was a problem hiding this comment.
Robolectric을 사용하여 테스트를 해도 실제로 앱이 구동되는 것처럼 테스트가 되어 힐트의 의존성 주입도 실제로 해줍니다.
현재 저희의 AuthDataSource는 EncryptedSharedPreferences를 사용하고 있죠. 그렇지만 Robolectric은 이의 키를 알 수 없습니다. 실제로 변경되기 전에 테스트를 실행해보면 다음과 같은 오류가 발생합니다.
이전에 깨지지 않았던 이유는 AuthDataSource를 사용하는 AuthInterceptor에서 의존성 주입이 아닌 애플리케이션에 선언되어 있는 AuthDataSource를 사용했기 때문입니다. 이번 PR에서 그 부분을 변경했기 때문에 테스트가 깨지게 되었습니다.
이제 테스트를 어떻게 해결했는지 말씀드릴게요.
UploadActivity는 AuthDataSource를 사용하지 않습니다. 그래서 어떤 객체가 주입되건 상관없죠.
@UninstallModules(DataSourceModule::class)을 사용해서 현재 주입해주는 모듈을 이 테스트에서 사용하지 않도록 합니다. 그러면 주입해줄 AuthDataSource가 없으니 다시 오류가 발생하겠죠.
그래서 아래 AuthDataSource를 제공하는 새로운 모듈을 만들어준 것입니다.
간단하게 설명하면 Fake객체를 직접 주입 해주는 것이 아니라 힐트가 주입해주도록 변경했다 라고 생각하시면 됩니다.
| showToast(getString(R.string.upload_image_orientation_error)) | ||
| showToast(getString(R.string.upload_image_orientation_error_message)) |
There was a problem hiding this comment.
이 부분을 변경할까 해도될까에 대해 고민을 좀 하다가 변경하게 되었습니다.
프로젝트의 통일성이 유지되었으면 좋겠다는 생각을 하고 있습니다. 그런데 저희는 다른 사람이다보니 다른 부분이 있을 수 밖에 없고, 리뷰를 하지만 거기서 고치지 못하는 부분도 분명히 있던 것 같습니다.
그래서 통일성을 맞추기 위해 새로운 이슈를 등록해서 하는 것보다 현재 PR에서 변경하는 것이 낫지 않을까 하는 생각으로 변경했습니다.
There was a problem hiding this comment.
크롱🦖 고생많았어요!
17시간 진짜.. 리프레시 딥다이브 하셨네요... 힘들었을 크롱의 모습이 눈에 그려져 마음이 아픕니다...
전반적으로 문제 되던 부분들 잘 해결하고 수정해주셨네요!!
크롱이 물어본 질문에 대답해보자면...
첫번째, AuthInterceptor가 AuthRepository를 갖게 하는 것에 대해 어떻게 생각하는지?
저는 저번에 리팩된 이 코드를 보고 크롱이 말했던 것 처럼 역할과 책임이 분리되어 해당 코드들에 대한 가독성이 좋아진 것 같아 너무 좋다 생각했습니다!
하지만 위에서 빅스가 말한 것처럼
로그인, 로그아웃, 회원탈퇴를 제외하고는 서비스의 기능이라고 보기 어렵다고 생각합니다.
즉, refreshAccessToken, getAccessToken, getRefreshToken, storeToken 모두 Domain과 관련 없이 Data Layer 안에서 수행되는 행동들이라고 생각함
이런 부분에 대해서는 미처 생각해보지 못했던 부분이었어서 저도 덩달아 함께 생각을 해보았습니다.
전 개인적으로 가독성과 역할 분리 면에서 현재와 같은 구조가 좋다고 생각합니다. 하지만 빅스의 의견도 충분히 고려해볼만한 의견인 것 같아 크롱의 의견도 궁금합니다!
두번째, 로그아웃 or 회원탈퇴를 진행하는 경우 액세스 토큰 대신 리프레시 토큰을 사용하는 것은 어떻게 생각하는지?
크롱의 의견을 들어보며 생각해보았는데, 만일 진짜로 리프레시 토큰을 사용한다고 하면 사용해도 괜찮다 생각합니다.
하지만 개인적인 생각을 말해보자면 저는 액세스 토큰으로 로그아웃과 회원탈퇴를 진행하는게 좋다고 생각합니다!
리프레시 토큰이 만료된 액세스 토큰을 갱신해주는 일을 한다는 본래의 역할이 유지되어야 된다고 생각하기 때문입니다😄
아 그리고 마지막으로 테스트가 깨져있어요🥹 테스트 깨진거 고쳐주시면 될 것 같아요!
정말 고생 많았어요!!
| @Qualifier | ||
| @Retention(AnnotationRetention.BINARY) | ||
| annotation class AuthRetrofit | ||
|
|
||
| @Qualifier | ||
| @Retention(AnnotationRetention.BINARY) | ||
| annotation class NormalRetrofit |
There was a problem hiding this comment.
어디에 중점을 두고 네이밍을 하냐에 따라 달라질 것 같습니다!
빅스는 아마 사용되는 위치에 중점을 두고 생각해 현재와 같은 네이밍이 헷갈린다고 말한 것 같네요!
전 개인적으로 현재처럼 auth를 담고 있는지에 대한 여부로 네이밍 된 것이 괜찮다 생각합니다
만일 사용되는 위치에 중점을 담아 네이밍이 된다면?
auth 를 담고 있는 NormalRetrofit과 auth를 담지 않고 있는 AuthRetrofit이라는 네이밍이 왠지 전 더 헷갈릴 것 같은 느낌ㅎㅎ
추가로 빅스가 말한 Normal을 Common 같은 용어로 바꾸자는 의견에는 적극 동의합니다!
이게 무슨 말인가.. 한참 고민했는데 이제 이해가 되었네요. AuthRepository에서 불필요한 부분들을 삭제하기 위한 방법이겠죠?
어떤 부분인지 예를 들어 말씀해주시면 더 좋을 것 같네요 :) |
dooboocookie
force-pushed
the
dev_android
branch
from
84742c6
to
e27ac4d
Compare
📌 관련 이슈
🛠️ 작업 내용
🎯 리뷰 포인트
AuthInterceptor
⏳ 작업 시간
추정 시간: 4h
실제 시간: 17h
AuthInterceptor에 있는 리프레시 로직을 AuthRepository로 이동시키는 과정에서 부수적인 문제들이 발생해서 고민하고 고치느라 시간이 더 소요되었습니다.
정말 고려할 것이 많군요. 리프레시,,,