-
Notifications
You must be signed in to change notification settings - Fork 0
[Refactor] API 호출 오류 핸들링 개선 #391
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
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.
Pull Request Overview
This PR refactors the API error handling structure by introducing an ApiResult sealed class and Retrofit CallAdapter to replace the existing BaseResponse<T> pattern with Flow. The changes simplify error handling by having repositories return nullable types or Boolean success indicators instead of wrapped Flow responses.
Key changes:
- Introduced
ApiResult<T>sealed class with CallAdapter pattern for centralized API response handling - Replaced repository Flow patterns with direct nullable/Boolean return types
- Updated all ViewModels to handle new repository return patterns directly
Reviewed Changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
app/src/main/res/values/strings.xml |
Removed trailing newline from login_failed string |
app/src/main/java/com/eatssu/android/presentation/*/ |
Updated ViewModels to handle new nullable/Boolean repository patterns |
app/src/main/java/com/eatssu/android/domain/usecase/*/ |
Modified use cases to return nullable/Boolean types instead of Flow |
app/src/main/java/com/eatssu/android/domain/repository/*/ |
Updated repository interfaces to return nullable/Boolean types |
app/src/main/java/com/eatssu/android/data/service/*/ |
Modified service interfaces to use ApiResult<T> |
app/src/main/java/com/eatssu/android/data/repository/*/ |
Implemented new repository patterns using ApiResult extensions |
app/src/main/java/com/eatssu/android/di/network/*/ |
Added ApiResult CallAdapter infrastructure |
app/src/main/java/com/eatssu/android/data/model/ApiResult.kt |
New sealed class for API response handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...src/main/java/com/eatssu/android/presentation/cafeteria/review/write/ReviewWriteViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/data/service/MealService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/di/network/TokenAuthenticator.kt
Outdated
Show resolved
Hide resolved
|
IntroActivity에서 최초 시작될 때, BaseActivity.onResume 때 Health Check 한 뒤 |
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.
Pull Request Overview
Copilot reviewed 67 out of 67 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewViewModel.kt
Show resolved
Hide resolved
...in/java/com/eatssu/android/presentation/cafeteria/review/write/menu/VariableMenuViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/presentation/login/LoginViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/data/repository/MealRepositoryImpl.kt
Show resolved
Hide resolved
HI-JIN2
left a comment
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.
동작 관련해서 체크해보아야할게 있어보여서 코멘트 달았습니다!!
app/src/main/java/com/eatssu/android/data/repository/PartnershipRepositoryImpl.kt
Show resolved
Hide resolved
| is UiEvent.NavigateToServerError -> { | ||
| val intent = | ||
| Intent(this@IntroActivity, ServerErrorActivity::class.java).apply { | ||
| putExtra(ServerErrorActivity.EXTRA_TITLE, event.title) | ||
| putExtra(ServerErrorActivity.EXTRA_MESSAGE, event.message) | ||
| } | ||
| startActivity(intent) | ||
| finish() | ||
| } |
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.
지금 도메인을 바꾸지 않은채로 빌드를 해보면, UnknownHostException이 뜨죠.
intro 에서 UiEvent.NavigateToServerError으로 떨궈주지도 않고 baseActivity를 상속하는것도 아니어서
intro -> login 화면으로 갑니다 (intro > error 화면으로 가는게 적합하겟쬬?)
문제점을 해결해주지 못하고 있어요.. 요부분 개선이 필요할 것 같아요
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.
이 부분 캐치 못했네요 감사합니다!
UiEvent.NavigateToServerError 자체가 필요하지 않은 것 같아서 삭제하고, IntroActivity에서 NetworkErrorEventBus를 쓰게끔 수정할게요!
| async { | ||
| when (restaurant.menuType) { | ||
| MenuType.FIXED -> { | ||
| when (val result = menuService.getFixMenu(restaurant.toString())) { |
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.
옴... 제가 말한 클린아키텍쳐 리팩토링은 뷰모델에서 바로 서비스를 호출하는걸 레이어를 나눠서 repository > usecase로 나누어달라는 거였는데..!
이번 PR에서 작업해야할 부분은 아닌것 같아서 넘길게유
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.
클린 코드만 생각하다보니 구조만 갈아엎고 service 호출 분리를 깜빡했네요!
이미 건드려서 커밋이 올라간 이상 어차피 하긴 해야하니 같이 작업해서 올리겠습니다!
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.
Pull Request Overview
Copilot reviewed 77 out of 77 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
app/src/main/java/com/eatssu/android/presentation/login/LoginViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/presentation/intro/IntroViewModel.kt
Show resolved
Hide resolved
|
kangyuri1114
left a comment
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.
Repository가 ApiResult를 리턴하는 구조로 모두 변경되면서 viewmodel 단의 runcatching도 웬만하면 필요가 없어졌네요! 이부분도 차차 필요없어진 부분은 걷어내면 좋을 것 같습니다
저도 후다닥 만들다보니 Repository 호출하는 부분만 바꾸고 그 위아래 주변 코드 자체는 아직 기존 형태인데가 꽤 있더라구요. 말씀해주신대로 추후에 수정하겠습니다! |
|
유리님께서 말씀해주셔서 runCatching 쪽을 한번 훑어봤는데 throw가 안됨 = onFailure가 절대 호출 안됨 = 오류를 값으로 반환했지만 UI는 오류로 인식 안될 것 같아서 전부 리팩토링 하겠습니다 하하 |
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.
Pull Request Overview
Copilot reviewed 80 out of 80 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| val menuReviewInfo = getMenuReviewInfoUseCase(menuId) | ||
| _uiState.update { | ||
| it.copy( | ||
| loading = false, | ||
| error = false, | ||
| reviewInfo = menuReviewInfo, | ||
| isEmpty = menuReviewInfo == null, | ||
| ) | ||
| } |
Copilot
AI
Oct 21, 2025
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.
Setting error = false unconditionally is incorrect when menuReviewInfo is null. A null result could indicate an API failure, not just an empty state. The error flag should be set to true when the API call fails, distinguishing between 'no reviews exist' and 'failed to fetch reviews'.
| val mealReviewInfo = getMealReviewInfoUseCase(mealId) | ||
| _uiState.update { | ||
| it.copy( | ||
| loading = false, | ||
| error = false, | ||
| reviewInfo = mealReviewInfo, | ||
| isEmpty = mealReviewInfo == null, | ||
| ) | ||
| } |
Copilot
AI
Oct 21, 2025
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.
Setting error = false unconditionally is incorrect when mealReviewInfo is null. A null result could indicate an API failure, not just an empty state. The error flag should be set to true when the API call fails, distinguishing between 'no reviews exist' and 'failed to fetch reviews'.
Summary
ApiResult sealed class와 retrofit의 CallAdapter를 활용해 오류에 대응하는 새로운 구조로 변경합니다
Describe your changes
Service: BaseResponse 를 ApiResult로 변경
Repository: Flow<BaseResponse를 T?로 변경
ViewModel, UseCase:
Issue
To reviewers
local.properties 에서 백엔드 서버 도메인을 임의로 변경하고 앱을 켜보면로그인이 되어있던 경우) 토큰 값이 유효한지 확인하지 못해 로그인 창으로 이동 > 로그인 시도하면 "로그인이 실패했습니다"
로그인이 안되어있던 경우) 로그인 창에서 로그인 시도하면 "로그인이 실패했습니다"
앱이 죽지는 않지만 ApiResult.NetworkError인 경우에는 어떻게 할 것인지 대응이 필요해요!
제 생각에 서버가 아예 죽었을 때 서버 오류 스플래시로 이동하는 걸 구현하는 제일 깔끔한 구조는 역시 health check용 use-case를 만들고, IntroActivity에서 + 정기적으로 실행하는거에요.
이 use-case는 ApiResult를 매핑하지 않고 NetworkError를 그대로 반환해서 Presentation 레이어에서 ErrorActivity로 이동시키기만 하면 될 것 같습니다.
EDIT: 20251010 ServerErrorActivity 추가했습니다!
Repository에서 ApiResult를 완전히 가리고 결과값만 던져주는 구조와, ApiResult의 data만 도메인 객체로 바꾸고 반환해주는 구조 중 무엇이 더 좋을까요? 호출하는 입장에서 '어떤 오류'가 발생했는가를 상세히 알아야할 필요가 있는지에 따라 결정하면 될 것 같습니다.
Presentation 레이어에서 Loading 상태 변경 > API 호출 > 결과의 null 여부에 따라 Error 상태, Success 상태 변경하고 있는 구조로 변경했습니다.
그러나 상당히 반복적인 부분이 많아서 UiState를 모두 사용하게 리팩토링한 후, 자동으로 Loading과 Error 상태를 관리해주는 유틸 함수를 만들어서 쓰면 좋을 것 같아요.