Conversation
📝 WalkthroughWalkthrough프리사인드(Presigned) URL을 통한 이미지 업로드 기능을 구현하는 변경사항입니다. 도메인 계층에서 PhotoLogUploadInfo 모델과 PhotoLogRepository 인터페이스를 정의하고, 데이터 계층에서 이를 구현하는 DefaultPhotoLogRepository를 추가합니다. 네트워크 계층에서는 업로드 URL을 조회하는 PhotoLogService API, S3 업로드를 수행하는 PresignedUploader 클래스, 그리고 네트워크 오류를 일관되게 처리하는 safeUploadCall 함수를 추가하며, 각 계층의 DI 설정을 확장합니다. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 상세 리뷰 의견전반적인 평가매우 깔끔하고 일관성 있는 계층별 구조입니다. 도메인 → 데이터 → 네트워크 계층이 명확하게 분리되어 있고, 각 계층이 자신의 책임을 잘 수행하고 있습니다. 특히 DI 구성이 체계적이고 예외 처리 전략이 명확한 점이 좋습니다. 칭찬할 점들✅ 일관된 에러 처리 전략: ✅ 명확한 책임 분리: PhotoLogRepository 인터페이스는 두 가지 기능( ✅ DI 설정의 정확성: 네임드 바인딩( 개선 가능한 영역🔍 타임아웃 설정값의 일관성 검토 // UploadHttpClientProvider.kt의 타임아웃 설정
request: 120_000 ms
connect: 30_000 ms
socket: 120_000 ms이러한 타임아웃 값들이:
검토 제안: 만약 일반 API용 HttpClient의 타임아웃과 다르다면, 주석으로 그 이유를 명시하면 향후 유지보수 시 도움이 될 것 같습니다. 🔍 PresignedUploader의 오류 처리 // uploadPhotoLogImage 메서드
on success, return info.fileName as AppResult.Success;
propagate any upload errors.현재 제안: 필요하다면 fileName 유효성 검증을 추가하거나, 명시적으로 이를 수행하지 않는다는 주석을 남기면 좋겠습니다. 🔍 DefaultPhotoLogRepository의 워크플로우 1) fetch upload info using getUploadUrl(goalId) and propagate errors.
2) upload to S3 using PresignedUploader이 두 단계가 순차적으로 수행되는데, 첫 번째 단계에서 오류가 발생하면 두 번째 단계가 실행되지 않습니다. 이는 의도된 설계인지 확인이 필요합니다. 질문: 마이너 제안
종합하면, 이 PR은 구조적으로 우수하고 실행 가능한 변경입니다. 위의 제안사항들을 선택적으로 검토하시고, 특히 타임아웃 설정값과 워크플로우 재시도 정책에 대한 의도를 명시하면 더욱 견고한 코드가 될 것 같습니다. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/network/src/main/java/com/twix/network/upload/PresignedUploader.kt`:
- Around line 20-30: The upload client currently handles non-2xx responses by
manually checking response.status in PresignedUploader.safeUploadCall which
causes thrown IllegalStateException to lose HTTP info; instead set expectSuccess
= true in the upload client provider (UploadHttpClientProvider) so Ktor throws
ResponseException that safeUploadCall can map to AppError.Http, and then remove
the manual status check in PresignedUploader (the client.put block) so you rely
on Ktor's automatic exception handling.
🧹 Nitpick comments (1)
domain/src/main/java/com/twix/domain/model/photo/PhotoLogUploadInfo.kt (1)
3-6: 기본값으로 빈 문자열("")을 사용하는 것이 적절한지 검토가 필요합니다.
uploadUrl과fileName이 빈 문자열로 기본값이 설정되어 있으면, 서버 응답 매핑 실패 시에도 유효하지 않은 빈 문자열 객체가 조용히 생성될 수 있습니다. 이 경우 이후 업로드 단계에서 원인 파악이 어려운 오류로 이어질 수 있습니다.기본값 없이 필수 파라미터로 두거나, 혹은 현재 구조를 유지하되 사용하는 쪽에서 빈 문자열 검증을 수행하는 방식을 고려해 보시면 좋겠습니다. As per coding guidelines, "도메인 모델이 불변 객체로 설계되었는가?" — 불변성은 준수하고 있으나, 유효성 보장 측면에서 개선 여지가 있습니다.
| safeUploadCall { | ||
| val response = | ||
| client.put(uploadUrl) { | ||
| contentType(ContentType.parse(contentType)) | ||
| setBody(bytes) | ||
| } | ||
|
|
||
| if (!response.status.isSuccess()) { | ||
| throw IllegalStateException("S3 upload failed: ${response.status.value}") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Ktor HttpClient expectSuccess default behavior 2.x
💡 Result:
In Ktor HttpClient 2.x, expectSuccess is off by default—meaning Ktor does not automatically treat non-2xx HTTP statuses as failures (it won’t throw just because the status is 3xx/4xx/5xx). You must explicitly enable it. [1]
If you set expectSuccess = true (client-wide or per-request), Ktor enables its default response validation and will throw on non-2xx responses: RedirectResponseException (3xx), ClientRequestException (4xx), ServerResponseException (5xx). [1][2]
You can configure it:
- Client-wide:
HttpClient { expectSuccess = true }[2] - Per-request override:
HttpRequestBuilder.expectSuccess = ...[3]
Sources: [1] [2] [3]
🏁 Script executed:
# Find build configuration files
fd -e "build.gradle" -e "build.gradle.kts" -e "pom.xml" | head -20Repository: YAPP-Github/Twix-Android
Length of output: 50
🏁 Script executed:
# Search for Ktor version and HttpClient configuration
rg "ktor|HttpClient|expectSuccess" --type kotlin --type java -B 2 -A 2Repository: YAPP-Github/Twix-Android
Length of output: 16294
🏁 Script executed:
cat -n core/network/src/main/java/com/twix/network/UploadHttpClientProvider.ktRepository: YAPP-Github/Twix-Android
Length of output: 618
🏁 Script executed:
cat -n core/network/src/main/java/com/twix/network/execute/UploadCall.ktRepository: YAPP-Github/Twix-Android
Length of output: 1579
업로드 클라이언트를 expectSuccess = true로 설정하여 에러 처리를 개선하세요.
Ktor 2.x는 기본적으로 expectSuccess = false이므로, 현재 코드의 isSuccess() 검사는 실제로 도달 가능합니다. 하지만 IllegalStateException을 throw하면 safeUploadCall의 Throwable 핸들러에 잡혀 HTTP 상태 정보가 손실됩니다.
메인 API 클라이언트(HttpClientProvider)는 이미 expectSuccess = true로 설정되어 있습니다. 업로드 클라이언트도 동일하게 설정하면 Ktor가 자동으로 ResponseException을 throw하며, safeUploadCall이 이를 잡아 AppError.Http로 매핑하므로 HTTP 상태 코드 정보가 보존됩니다.
✅ 개선 방안
UploadHttpClientProvider.kt에서:
object UploadHttpClientProvider {
fun create(): HttpClient =
HttpClient(OkHttp) {
+ expectSuccess = true
install(HttpTimeout) {
requestTimeoutMillis = 120_000
connectTimeoutMillis = 30_000
socketTimeoutMillis = 120_000
}
}
}그 후 PresignedUploader.kt에서 수동 검사 제거:
safeUploadCall {
- val response =
- client.put(uploadUrl) {
- contentType(ContentType.parse(contentType))
- setBody(bytes)
- }
-
- if (!response.status.isSuccess()) {
- throw IllegalStateException("S3 upload failed: ${response.status.value}")
- }
+ client.put(uploadUrl) {
+ contentType(ContentType.parse(contentType))
+ setBody(bytes)
+ }
}🤖 Prompt for AI Agents
In `@core/network/src/main/java/com/twix/network/upload/PresignedUploader.kt`
around lines 20 - 30, The upload client currently handles non-2xx responses by
manually checking response.status in PresignedUploader.safeUploadCall which
causes thrown IllegalStateException to lose HTTP info; instead set expectSuccess
= true in the upload client provider (UploadHttpClientProvider) so Ktor throws
ResponseException that safeUploadCall can map to AppError.Http, and then remove
the manual status check in PresignedUploader (the client.put block) so you rely
on Ktor's automatic exception handling.
chanho0908
left a comment
chanho0908
left a comment
There was a problem hiding this comment.
고생했어 현수야 ! 실제 업로드 구현하면서 의견남길거 있으면 디엠 남길게 !
2 participants
이슈 번호
작업내용
리뷰어에게 추가로 요구하는 사항 (선택)
사용 방법은 Repository에 있는 uploadPhotoLogImage을 호출해서 서버에서 fileName을 받아오고 인증샷 등록할 때 fileName을 다시 서버에 전달하면 됩니다!