-
-
Notifications
You must be signed in to change notification settings - Fork 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
Replace Gson library with Moshi #4309
Conversation
…nce.Configuration.Urls.streamingApi
@@ -67,7 +64,8 @@ fun TimelineAccountEntity.toAccount(gson: Gson): TimelineAccount { | |||
url = url, | |||
avatar = avatar, | |||
bot = bot, | |||
emojis = gson.fromJson(emojis, emojisListType) | |||
// Handle nullable lists for backward compatibility | |||
emojis = moshi.adapter<List<Emoji>?>().fromJson(emojis) ?: emptyList() |
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.
emojis = moshi.adapter<List<Emoji>?>().fromJson(emojis) ?: emptyList() | |
emojis = moshi.adapter<List<Emoji>?>().fromJson(emojis).orEmpty() |
fun jsonToEmojiList(emojiListJson: String?): List<Emoji>? { | ||
return gson.fromJson(emojiListJson, object : TypeToken<List<Emoji>>() {}.type) | ||
fun jsonToEmojiList(emojiListJson: String?): List<Emoji> { | ||
return emojiListJson?.let { moshi.adapter<List<Emoji>?>().fromJson(it) } ?: emptyList() |
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.
return emojiListJson?.let { moshi.adapter<List<Emoji>?>().fromJson(it) } ?: emptyList() | |
return emojiListJson?.let { moshi.adapter<List<Emoji>?>().fromJson(it) } .orEmpty() |
object : TypeToken<List<ConversationAccountEntity>>() {}.type | ||
) | ||
fun jsonToAccountList(accountListJson: String?): List<ConversationAccountEntity> { | ||
return accountListJson?.let { moshi.adapter<List<ConversationAccountEntity>?>().fromJson(it) } ?: emptyList() |
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.
return accountListJson?.let { moshi.adapter<List<ConversationAccountEntity>?>().fromJson(it) } ?: emptyList() | |
return accountListJson?.let { moshi.adapter<List<ConversationAccountEntity>?>().fromJson(it) }.orEmpty() |
fun jsonToAttachmentList(attachmentListJson: String?): List<Attachment>? { | ||
return gson.fromJson(attachmentListJson, object : TypeToken<List<Attachment>>() {}.type) | ||
fun jsonToAttachmentList(attachmentListJson: String?): List<Attachment> { | ||
return attachmentListJson?.let { moshi.adapter<List<Attachment>?>().fromJson(it) } ?: emptyList() |
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.
return attachmentListJson?.let { moshi.adapter<List<Attachment>?>().fromJson(it) } ?: emptyList() | |
return attachmentListJson?.let { moshi.adapter<List<Attachment>?>().fromJson(it) }.orEmpty() |
fun jsonToMentionArray(mentionListJson: String?): List<Status.Mention>? { | ||
return gson.fromJson(mentionListJson, object : TypeToken<List<Status.Mention>>() {}.type) | ||
fun jsonToMentionArray(mentionListJson: String?): List<Status.Mention> { | ||
return mentionListJson?.let { moshi.adapter<List<Status.Mention>?>().fromJson(it) } ?: emptyList() |
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.
return mentionListJson?.let { moshi.adapter<List<Status.Mention>?>().fromJson(it) } ?: emptyList() | |
return mentionListJson?.let { moshi.adapter<List<Status.Mention>?>().fromJson(it) }.orEmpty() |
object : TypeToken<List<DraftAttachment>>() {}.type | ||
) | ||
fun jsonToDraftAttachmentList(draftAttachmentListJson: String?): List<DraftAttachment> { | ||
return draftAttachmentListJson?.let { moshi.adapter<List<DraftAttachment>?>().fromJson(it) } ?: emptyList() |
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.
return draftAttachmentListJson?.let { moshi.adapter<List<DraftAttachment>?>().fromJson(it) } ?: emptyList() | |
return draftAttachmentListJson?.let { moshi.adapter<List<DraftAttachment>?>().fromJson(it) }.orEmpty() |
get() { | ||
return text == null && attachments.isEmpty() | ||
} |
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.
get() { | |
return text == null && attachments.isEmpty() | |
} | |
get() = text == null && attachments.isEmpty() |
get() { | ||
return when (this) { | ||
PUBLIC -> "public" | ||
UNLISTED -> "unlisted" | ||
PRIVATE -> "private" | ||
DIRECT -> "direct" | ||
UNKNOWN -> "unknown" | ||
} |
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.
get() { | |
return when (this) { | |
PUBLIC -> "public" | |
UNLISTED -> "unlisted" | |
PRIVATE -> "private" | |
DIRECT -> "direct" | |
UNKNOWN -> "unknown" | |
} | |
get() { | |
return when (this) { | |
PUBLIC -> "public" | |
UNLISTED -> "unlisted" | |
PRIVATE -> "private" | |
DIRECT -> "direct" | |
UNKNOWN -> "unknown" | |
} |
get() { | |
return when (this) { | |
PUBLIC -> "public" | |
UNLISTED -> "unlisted" | |
PRIVATE -> "private" | |
DIRECT -> "direct" | |
UNKNOWN -> "unknown" | |
} | |
get() = when (this) { | |
PUBLIC -> "public" | |
UNLISTED -> "unlisted" | |
PRIVATE -> "private" | |
DIRECT -> "direct" | |
UNKNOWN -> "unknown" | |
} |
If we are going to use codegen anyway, why not Kotlin Serialization? |
this is genuine question btw, amazing work too! |
I don't know kotlinx serialization well enough to be able to write a custom streaming deserializer or implement something equivalent to I'm going to travel for 3 weeks without a laptop, so I won't be able to update this branch until March 27. Fell free to do more tests and reviews in the meantime. |
It could be something like import kotlinx.serialization.KSerializer
import kotlinx.serialization.Serializer
import kotlinx.serialization.encoding.Decoder
import kotlinx.serialization.encoding.Encoder
import kotlinx.serialization.json.Json
import kotlinx.serialization.json.JsonElement
import kotlinx.serialization.json.JsonNull
@Serializer(forClass = T::class)
object GuardedSerializer : KSerializer<T?> {
private val json = Json { ignoreUnknownKeys = true }
override val descriptor = SerialDescriptor("WithCustomDefault", StructureKind.OBJECT)
override fun serialize(encoder: Encoder, value: T?) {
// Write the value to the encoder
encoder.encodeSerializableValue(json.encodeToJsonElement(value))
}
override fun deserialize(decoder: Decoder): T? {
return try {
// Try to decode the value
json.decodeFromJsonElement(decoder.decodeSerializableValue(JsonElement.serializer()))
} catch (e: Exception) {
// If there's an error, return null
null
}
}
} |
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.
I found one bug with my mastodon.social account
Failed to load timeline com.squareup.moshi.JsonDataException: Required value 'keywords' missing at $[0].filtered[0].filter
description = mediaDescriptions[i], | ||
focus = mediaFocus[i], | ||
type = types[i] | ||
val attachments: List<DraftAttachment> = buildList { |
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.
val attachments: List<DraftAttachment> = buildList { | |
val attachments: List<DraftAttachment> = buildList(mediaUris.size) { |
* Tusky 15: uriString = e, description = f, type = g | ||
* Tusky 16 beta: uriString = i, description = j, type = k | ||
*/ | ||
class DraftAttachmentJsonAdapter(moshi: Moshi) : JsonAdapter<DraftAttachment>() { |
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.
I wonder if this is still necessary. It only helps users that have unedited drafts that are over two years old. 🤔
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.
This is indeed legacy code that adds unnecessary complexity to the project. I'll remove it.
# Conflicts: # app/proguard-rules.pro # app/src/main/java/com/keylesspalace/tusky/adapter/StatusBaseViewHolder.java # app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeViewModel.kt # app/src/main/java/com/keylesspalace/tusky/components/login/LoginWebViewViewModel.kt # app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelineTypeMappers.kt # app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt # app/src/main/java/com/keylesspalace/tusky/components/viewthread/ViewThreadViewModel.kt # app/src/main/java/com/keylesspalace/tusky/entity/Status.kt # app/src/main/java/com/keylesspalace/tusky/fragment/SFragment.kt # gradle/libs.versions.toml
# Conflicts: # app/src/main/java/com/keylesspalace/tusky/entity/Translation.kt
I made the "keywords" field optional with a default value of empty list. This is one of the cases where a non-null Kotlin field ends up null at runtime with Gson. I already fixed one or two more in the initial code I submitted. |
Note that contrary to Moshi, the Kotlin Serialization adapter for Retrofit currently does not support streaming: the entire network response is first loaded into a big String, then that String is parsed. Moshi works directly at the byte stream level using Okio, which makes it more efficient in theory. |
this is a good point, thank you |
Now I'm getting |
The two list fields in |
Loading the timeline works now and I can't find any other problems! If they are unused we should probably comment them out to save some deserializing time and memory. |
… from snake case to camel case
I went trough the entire Mastodon API entities documentation to find remaining optional fields that were declared as mandatory, as well as nullable fields that were declared as non-null. |
Huh, I'm still getting the |
…API doc says it's not
Oops, I just realized I reverted my own change in the recent commit. Turns out the documentation says this field is mandatory but it's in fact optional. I added a comment to avoid a future mistake. |
! ! Warning: Do not merge before testing every API call and database read involving JSON !
Gson is obsolete and has been superseded by Moshi. But more importantly, parsing Kotlin objects using Gson is dangerous because Gson uses Java serialization and is not Kotlin-aware. This has two main consequences:
NullPointerException
when trying to access a field member;On the other hand, Kotlin-aware parsers like Moshi or Kotlin Serialization will validate at parsing time that all received fields comply with the Kotlin contract and avoid errors at runtime, making apps more stable and schema mismatches easier to detect (as long as logs are accessible):
Migrating the entity declarations from Gson to Moshi will make the code more robust but is not an easy task because of the semantic differences.
With Gson, both nullable and optional fields are represented with a null value. After converting to Moshi, some nullable entities can become non-null with a default value (if they are optional and not nullable), others can stay nullable with no default value (if they are mandatory and nullable), and others can become nullable with a default value of null (if they are optional or nullable or both). That third option is the safest bet when it's not clear if a field is optional or not, except for lists which can usually be declared as non-null with a default value of an empty list (I have yet to see a nullable array type in the Mastodon API).
Fields that are currently declared as non-null present another challenge. In theory, they should remain as-is and everything will work fine. In practice, because Gson is not aware of nullable types at all, it's possible that some non-null fields currently hold a null value in some cases but the app does not report any error because the field is not accessed by Kotlin code in that scenario. After migrating to Moshi however, parsing such a field will now fail early if a null value or no value is received.
These fields will have to be identified by heavily testing the app and looking for parsing errors (
JsonDataException
) and/or by going through the Mastodon documentation. A default value needs to be added for missing optional fields, and their type could optionally be changed to nullable, depending on the case.Gson is also currently used to serialize and deserialize objects to and from the local database, which is also challenging because backwards compatibility needs to be preserved. Fortunately, by default Gson omits writing null fields, so a field of type
List<T>?
could be replaced with a field of typeList<T>
with a default value ofemptyList()
and reading back the old data should still work. However, nullable lists that are written directly (not as a field of another object) will still be serialized to JSON as"null"
so the deserializing code must still be handling null properly.Finally, changing the database schema is out of scope for this pull request, so database entities that also happen to be serialized with Gson will keep their original types even if they could be made non-null as an improvement.
In the end this is all for the best, because the app will be more reliable and errors will be easier to detect by showing up earlier with a clear error message. Not to mention the performance benefits of using Moshi compared to Gson.
Rfc3339DateJsonAdapter
with the one provided by moshi-adapters.JsonDeserializer
classes for Enum types withEnumJsonAdapter.create(T).withUnknownFallback()
from moshi-adapters to support fallback values.GuardedBooleanAdapter
with the more genericGuardedAdapter
which works with any type. Any nullable field may now be annotated with@Guarded
.@JsonClass
with no exception, and adding this annotation will ensure that R8/Proguard will handle the entities properly.emptyList()
where possible.TimelineDao
to perform all Json conversions internally usingConverters
so no Gson or Moshi instance has to be passed to its methods.Create a customRemove alternate names for someDraftAttachmentJsonAdapter
to serialize and deserializeDraftAttachment
which is a special entity that supports more than one json name per field. A custom adapter is necessary because there is not direct equivalent of@SerializedName(alternate = [...])
in Moshi.DraftAttachment
fields which were used as a workaround to deserialize local data in 2-years old builds of Tusky.equals()
implementations.val
sNetworkModule
into anobject
(since it contains no abstract methods).Please test the app thoroughly before merging. There may be some fields currently declared as mandatory that are actually optional.