-
Notifications
You must be signed in to change notification settings - Fork 0
[Refactor] kotlinx-serialization 마이그레이션 및 enum 자동 매핑 #467
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
base: develop
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @HI-JIN2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 애플리케이션의 JSON 직렬화 메커니즘을 Gson에서 Kotlinx Serialization으로 마이그레이션하여 리팩토링합니다. 이 변경은 앱 전반의 데이터 직렬화를 표준화하고, 성능을 향상시키며, 타입 안정성을 강화합니다. 또한 새로운 직렬화 라이브러리에 맞춰 네트워크 구성, 데이터 모델 및 UI 컴포넌트를 업데이트하고, 식당/상점 유형 enum을 리팩토링하여 일관성을 높였습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
이 PR은 기존의 Gson 라이브러리를 kotlinx.serialization으로 마이그레이션하는 대규모 리팩토링입니다. 전반적으로 변경 사항이 잘 적용되었으며, 코드의 가독성과 유지보수성이 크게 향상되었습니다. 특히 DTO에 @Serializable 어노테이션을 추가하고, Retrofit 설정에서 GsonConverterFactory를 KotlinxSerializationConverterFactory로 교체한 점, 그리고 enum 타입을 JSON 문자열과 자동으로 매핑하여 수동 변환 로직을 제거한 점이 인상적입니다. 몇 가지 추가적인 개선 사항을 제안합니다.
app/src/main/java/com/eatssu/android/data/remote/dto/response/BaseResponse.kt
Outdated
Show resolved
Hide resolved
| } catch (e: Exception) { | ||
| Timber.e(e, "Failed to parse cafeteria JSON") |
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.
try-catch 블록에서 제네릭 Exception을 잡고 있습니다. 이는 코루틴의 취소 메커니즘을 방해할 수 있는 CancellationException과 같은 중요한 예외까지 가로챌 수 있습니다. JSON 파싱 오류를 처리하기 위해서는 kotlinx.serialization.SerializationException과 같이 더 구체적인 예외를 잡는 것이 좋습니다. 이 패턴은 ApiResultCall.kt와 MealInfoStateDefinition.kt에서도 발견되므로, 프로젝트 전반에 걸쳐 일관되게 수정하는 것을 권장합니다.
| } catch (e: Exception) { | |
| Timber.e(e, "Failed to parse cafeteria JSON") | |
| } catch (e: kotlinx.serialization.SerializationException) { | |
| Timber.e(e, "Failed to parse cafeteria JSON") |
| fun provideJson(): Json = Json { | ||
| ignoreUnknownKeys = true | ||
| coerceInputValues = true | ||
| isLenient = true |
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.
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.
저도 이 부분에 동의합니다! coerceInputValues랑 isLenient가 loose하게 처리해주는 덕분에 앱 크래시를 줄일 수는 있겠지만 이 부분은 직렬화 레이어가 아니라 서비스 레이어에서 처리하다가 발생한 오류를 우아하게 해결해주어 크래시가 없게끔 하는게 좋은 것 같아요.
서버에서 이상하게 보내주는지 우리쪽에선 오류가 안나니까 알 수 없는데, silent하게 오류가 발생하는게 가장 무서운거잖아요?
직렬화 규칙은 빡세게 잡되(위 옵션들 false로), 전역적으로 serialization 관련 오류를 처리하는 로직을 추가하는게 어떨까요?
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.
268e2a0 확인부탁합니다!
Summary
Gson 기반 JSON 파싱 구조에서 서버 응답의 null / 누락 필드로 인한 NPE 가능성과 리플렉션 기반 파싱으로 인한 성능·안정성 한계가 지속적으로 존재해 kotlinx.serialization으로 전환했습니다.
핵심 효과
1. 기본값(Default Value) 처리 안정성 (가장 큰 장점)
Gson
리플렉션(Reflection) 기반으로 객체를 생성하면서 코틀린 생성자를 우회하는 경우가 많음
JSON 필드가 누락되면
val list: List<String> = emptyList()같은 기본값이 무시되고null이 들어가는 문제가 빈번=> 결과적으로 예상치 못한 NPE 발생 가능
Kotlin Serialization
생성자를 명확히 호출 → 기본값이 정상 적용
NetworkModule.kt의coerceInputValues = true옵션 덕분에 JSON에null이 내려와도 Non-null + 기본값이 있는 프로퍼티는 안전하게 기본값으로 변환됨=> 서버에서 null을 내려주거나 필드를 빼먹어도 앱이 죽을 확률을 크게 낮춤
2. 런타임 성능 향상
3. 순수 코틀린 친화성
Nullable 타입, Sealed class, Value class 등 코틀린 고유 문법을 자연스럽게 지원
4. 라이브러리 크기 감소
Retrofit Gson Converter 대비 Kotlin Serialization Converter + Core 라이브러리가 일반적으로 더 가벼움
Describe your changes
주의사항: kotlinx.serialization의 엄격함
null로 처리하거나 조용히 넘어가는 반면,MissingFieldException발생예시 로그:
대응 방법
null을 허용하거나 기본값을 지정 ✅관련 커밋: cee31fd, 62c3af8
(해당 커밋 이전에도 DTO 보수가 어느 정도 되어 있어 앱 크래시로 이어지지는 않았습니다.)
Issue
To reviewers
coerceInputValues설정 의도가 잘 반영되었는지