From 8e65480bd060d5e96f2c5b07070cdf95bd5e335d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= <30429749+saleniuk@users.noreply.github.com> Date: Mon, 2 Dec 2024 14:19:30 +0000 Subject: [PATCH 1/2] fix: crash when saving bottom sheet state for message with reply [WPB-14433] (#3689) --- .../com/wire/android/model/ImageAsset.kt | 1 + .../ui/home/conversations/model/UIMessage.kt | 3 +- .../conversations/model/UIQuotedMessage.kt | 15 ++- .../kotlin/com/wire/android/util/ui/UIText.kt | 6 +- core/ui-common/build.gradle.kts | 7 +- .../common/bottomsheet/WireModalSheetState.kt | 24 ++-- .../util/AnyPrimitiveAsStringSerializer.kt | 62 +++++++++++ .../wire/android/ui/common/ExampleUnitTest.kt | 5 +- .../bottomsheet/WireModalSheetStateTest.kt | 105 ++++++++++++++++++ 9 files changed, 206 insertions(+), 22 deletions(-) create mode 100644 core/ui-common/src/main/kotlin/com/wire/android/util/AnyPrimitiveAsStringSerializer.kt create mode 100644 core/ui-common/src/test/kotlin/com/wire/android/ui/common/bottomsheet/WireModalSheetStateTest.kt diff --git a/app/src/main/kotlin/com/wire/android/model/ImageAsset.kt b/app/src/main/kotlin/com/wire/android/model/ImageAsset.kt index b39c78d5095..8735b109d27 100644 --- a/app/src/main/kotlin/com/wire/android/model/ImageAsset.kt +++ b/app/src/main/kotlin/com/wire/android/model/ImageAsset.kt @@ -58,6 +58,7 @@ sealed class ImageAsset { val idKey: String ) : ImageAsset() + @Serializable sealed class Remote : ImageAsset() { /** diff --git a/app/src/main/kotlin/com/wire/android/ui/home/conversations/model/UIMessage.kt b/app/src/main/kotlin/com/wire/android/ui/home/conversations/model/UIMessage.kt index e9c53a7269a..f61768edad0 100644 --- a/app/src/main/kotlin/com/wire/android/ui/home/conversations/model/UIMessage.kt +++ b/app/src/main/kotlin/com/wire/android/ui/home/conversations/model/UIMessage.kt @@ -286,7 +286,8 @@ sealed interface UIMessageContent { @Serializable data object IncompleteAssetMessage : UIMessageContent - interface PartialDeliverable { + @Serializable + sealed interface PartialDeliverable { val deliveryStatus: DeliveryStatusContent } diff --git a/app/src/main/kotlin/com/wire/android/ui/home/conversations/model/UIQuotedMessage.kt b/app/src/main/kotlin/com/wire/android/ui/home/conversations/model/UIQuotedMessage.kt index 04b90d310ab..8193ee55afc 100644 --- a/app/src/main/kotlin/com/wire/android/ui/home/conversations/model/UIQuotedMessage.kt +++ b/app/src/main/kotlin/com/wire/android/ui/home/conversations/model/UIQuotedMessage.kt @@ -39,25 +39,34 @@ sealed class UIQuotedMessage { val quotedContent: Content ) : UIQuotedMessage() { + @Serializable sealed interface Content + @Serializable data class Text(val value: String) : Content + @Serializable data class GenericAsset( val assetName: String?, val assetMimeType: String ) : Content + @Serializable data class DisplayableImage( val displayable: ImageAsset.PrivateAsset ) : Content + @Serializable data class Location(val locationName: String) : Content - object AudioMessage : Content + @Serializable + data object AudioMessage : Content - object Deleted : Content - object Invalid : Content + @Serializable + data object Deleted : Content + + @Serializable + data object Invalid : Content } } diff --git a/app/src/main/kotlin/com/wire/android/util/ui/UIText.kt b/app/src/main/kotlin/com/wire/android/util/ui/UIText.kt index 14bdb4faea3..1734807af18 100644 --- a/app/src/main/kotlin/com/wire/android/util/ui/UIText.kt +++ b/app/src/main/kotlin/com/wire/android/util/ui/UIText.kt @@ -25,8 +25,8 @@ import androidx.compose.runtime.Composable import androidx.compose.ui.res.pluralStringResource import androidx.compose.ui.res.stringResource import com.wire.android.appLogger +import com.wire.android.util.AnyPrimitiveAsStringSerializer import com.wire.kalium.logic.data.message.mention.MessageMention -import com.wire.kalium.util.serialization.AnyPrimitiveValueSerializer import kotlinx.serialization.Serializable @Serializable @@ -41,14 +41,14 @@ sealed class UIText { @Serializable class StringResource( @StringRes val resId: Int, - vararg val formatArgs: @Serializable(with = AnyPrimitiveValueSerializer::class) Any + vararg val formatArgs: @Serializable(with = AnyPrimitiveAsStringSerializer::class) Any ) : UIText() @Serializable class PluralResource( @PluralsRes val resId: Int, val count: Int, - vararg val formatArgs: @Serializable(with = AnyPrimitiveValueSerializer::class) Any + vararg val formatArgs: @Serializable(with = AnyPrimitiveAsStringSerializer::class) Any ) : UIText() @Suppress("SpreadOperator") diff --git a/core/ui-common/build.gradle.kts b/core/ui-common/build.gradle.kts index 5f6cb899b8e..664adf293e8 100644 --- a/core/ui-common/build.gradle.kts +++ b/core/ui-common/build.gradle.kts @@ -2,6 +2,7 @@ plugins { id(libs.plugins.wire.android.library.get().pluginId) id(libs.plugins.wire.kover.get().pluginId) alias(libs.plugins.kotlin.serialization) + id(BuildPlugins.junit5) } android { @@ -35,7 +36,11 @@ dependencies { implementation(libs.coil.gif) implementation(libs.coil.compose) - testImplementation(libs.junit4) + testImplementation(libs.junit5.core) + testImplementation(libs.junit5.params) + testImplementation(libs.mockk.core) + testImplementation(libs.kluent.core) + testRuntimeOnly(libs.junit5.engine) androidTestImplementation(libs.androidx.test.extJunit) androidTestImplementation(libs.androidx.espresso.core) } diff --git a/core/ui-common/src/main/kotlin/com/wire/android/ui/common/bottomsheet/WireModalSheetState.kt b/core/ui-common/src/main/kotlin/com/wire/android/ui/common/bottomsheet/WireModalSheetState.kt index ffcdacf1cce..624601e3807 100644 --- a/core/ui-common/src/main/kotlin/com/wire/android/ui/common/bottomsheet/WireModalSheetState.kt +++ b/core/ui-common/src/main/kotlin/com/wire/android/ui/common/bottomsheet/WireModalSheetState.kt @@ -24,9 +24,9 @@ import androidx.compose.material3.SheetValue import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.saveable.Saver -import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.platform.LocalSoftwareKeyboardController @@ -89,13 +89,14 @@ open class WireModalSheetState( companion object { const val DELAY_TO_SHOW_BOTTOM_SHEET_WHEN_KEYBOARD_IS_OPEN = 300L + @Suppress("TooGenericExceptionCaught") @OptIn(InternalSerializationApi::class) inline fun saver( density: Density, softwareKeyboardController: SoftwareKeyboardController?, noinline onDismissAction: () -> Unit, scope: CoroutineScope - ): Saver, *> = Saver( + ): Saver, List> = Saver( save = { when (it.currentValue) { is WireSheetValue.Hidden -> listOf(false) // hidden @@ -109,7 +110,13 @@ open class WireModalSheetState( listOf(true, SavedType.Regular, value) T::class.serializerOrNull() != null -> // expanded and with non-Unit value that can be serialized - listOf(true, SavedType.SerializedBundle, Bundlizer.bundle(T::class.serializer(), value)) + try { + val serializedBundleValue = Bundlizer.bundle(T::class.serializer(), value) + listOf(true, SavedType.SerializedBundle, serializedBundleValue) + } catch (e: Exception) { + e.printStackTrace() + listOf(false) // hidden because value cannot be serialized properly + } else -> listOf(false) // hidden because value cannot be saved } @@ -159,14 +166,9 @@ inline fun rememberWireModalSheetState( val softwareKeyboardController = LocalSoftwareKeyboardController.current val density = LocalDensity.current val scope = rememberCoroutineScope() - return rememberSaveable( - saver = WireModalSheetState.saver( - density = density, - softwareKeyboardController = softwareKeyboardController, - onDismissAction = onDismissAction, - scope = scope - ) - ) { + // TODO: we can use rememberSaveable instead of remember to save the state but first we need to make sure that we don't store too much, + // especially for conversations and messages to not keep such data unencrypted anywhere + return remember { WireModalSheetState( density = density, scope = scope, diff --git a/core/ui-common/src/main/kotlin/com/wire/android/util/AnyPrimitiveAsStringSerializer.kt b/core/ui-common/src/main/kotlin/com/wire/android/util/AnyPrimitiveAsStringSerializer.kt new file mode 100644 index 00000000000..60275a95b59 --- /dev/null +++ b/core/ui-common/src/main/kotlin/com/wire/android/util/AnyPrimitiveAsStringSerializer.kt @@ -0,0 +1,62 @@ +/* + * Wire + * Copyright (C) 2024 Wire Swiss GmbH + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see http://www.gnu.org/licenses/. + */ +package com.wire.android.util + +import kotlinx.serialization.KSerializer +import kotlinx.serialization.SerialName +import kotlinx.serialization.Serializable +import kotlinx.serialization.descriptors.SerialDescriptor +import kotlinx.serialization.encoding.Decoder +import kotlinx.serialization.encoding.Encoder + +object AnyPrimitiveAsStringSerializer : KSerializer { + override val descriptor: SerialDescriptor = AnyPrimitiveSurrogate.serializer().descriptor + override fun serialize(encoder: Encoder, value: Any) = + encoder.encodeSerializableValue(AnyPrimitiveSurrogate.serializer(), AnyPrimitiveSurrogate(value)) + override fun deserialize(decoder: Decoder): Any = + decoder.decodeSerializableValue(AnyPrimitiveSurrogate.serializer()).value +} + +@Serializable +@SerialName("AnyPrimitive") +private data class AnyPrimitiveSurrogate(private val kind: AnyPrimitiveKind, private val stringValue: String) { + constructor(value: Any) : this( + kind = when (value) { + is String -> AnyPrimitiveKind.STRING + is Int -> AnyPrimitiveKind.INT + is Long -> AnyPrimitiveKind.LONG + is Float -> AnyPrimitiveKind.FLOAT + is Double -> AnyPrimitiveKind.DOUBLE + is Boolean -> AnyPrimitiveKind.BOOLEAN + else -> throw IllegalArgumentException("Unsupported type: ${value::class}") + }, + stringValue = value.toString() + ) + + val value: Any + get() = when (kind) { + AnyPrimitiveKind.STRING -> stringValue + AnyPrimitiveKind.INT -> stringValue.toInt() + AnyPrimitiveKind.LONG -> stringValue.toLong() + AnyPrimitiveKind.FLOAT -> stringValue.toFloat() + AnyPrimitiveKind.DOUBLE -> stringValue.toDouble() + AnyPrimitiveKind.BOOLEAN -> stringValue.toBoolean() + } +} + +private enum class AnyPrimitiveKind { STRING, INT, LONG, FLOAT, DOUBLE, BOOLEAN } diff --git a/core/ui-common/src/test/kotlin/com/wire/android/ui/common/ExampleUnitTest.kt b/core/ui-common/src/test/kotlin/com/wire/android/ui/common/ExampleUnitTest.kt index b0896ca1a45..09521343967 100644 --- a/core/ui-common/src/test/kotlin/com/wire/android/ui/common/ExampleUnitTest.kt +++ b/core/ui-common/src/test/kotlin/com/wire/android/ui/common/ExampleUnitTest.kt @@ -17,9 +17,8 @@ */ package com.wire.android.ui.common -import org.junit.Test - -import org.junit.Assert.* +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test /** * Example local unit test, which will execute on the development machine (host). diff --git a/core/ui-common/src/test/kotlin/com/wire/android/ui/common/bottomsheet/WireModalSheetStateTest.kt b/core/ui-common/src/test/kotlin/com/wire/android/ui/common/bottomsheet/WireModalSheetStateTest.kt new file mode 100644 index 00000000000..8b2cd8de657 --- /dev/null +++ b/core/ui-common/src/test/kotlin/com/wire/android/ui/common/bottomsheet/WireModalSheetStateTest.kt @@ -0,0 +1,105 @@ +/* + * Wire + * Copyright (C) 2024 Wire Swiss GmbH + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see http://www.gnu.org/licenses/. + */ +package com.wire.android.ui.common.bottomsheet + +import androidx.compose.runtime.saveable.SaverScope +import com.wire.android.util.AnyPrimitiveAsStringSerializer +import io.mockk.mockk +import kotlinx.serialization.Serializable +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertInstanceOf + +class WireModalSheetStateTest { + + @Suppress("LongParameterList") + @Serializable + class SerializableTestModel( + val boolean: Boolean, + val int: Int, + val long: Long, + val float: Float, + val double: Double, + val char: Char, + val string: String, + val nullable: String?, + val list: List, + vararg val any: @Serializable(with = AnyPrimitiveAsStringSerializer::class) Any + ) { + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is SerializableTestModel) return false + + if (boolean != other.boolean) return false + if (int != other.int) return false + if (long != other.long) return false + if (float != other.float) return false + if (double != other.double) return false + if (char != other.char) return false + if (string != other.string) return false + if (nullable != other.nullable) return false + if (list != other.list) return false + if (!any.contentEquals(other.any)) return false + + return true + } + + override fun hashCode(): Int { + var result = boolean.hashCode() + result = 31 * result + int + result = 31 * result + long.hashCode() + result = 31 * result + float.hashCode() + result = 31 * result + double.hashCode() + result = 31 * result + char.hashCode() + result = 31 * result + string.hashCode() + result = 31 * result + (nullable?.hashCode() ?: 0) + result = 31 * result + list.hashCode() + result = 31 * result + any.contentHashCode() + return result + } + } + + @Test + fun givenSerializableModel_whenSavingState_thenStateIsSavedAndRestoredProperly() { + // given + val model = SerializableTestModel( + boolean = true, + int = 1, + long = 2L, + float = 3.0f, + double = 4.0, + char = 'c', + string = "string", + nullable = null, + list = listOf("a", "b", "c"), + any = arrayOf(1, 2L, 3.0, 4f, true, false, 'c', "string") + ) + val sheetValue = WireSheetValue.Expanded(model) + with(WireModalSheetState.saver(mockk(), mockk(), mockk(), mockk())) { + // when + val saved = SaverScope { true }.save(WireModalSheetState(mockk(), mockk(), mockk(), mockk(), sheetValue)) + // then + assertInstanceOf>(saved).let { + val restored = restore(it) + assertInstanceOf>(restored?.currentValue).let { + assertEquals(sheetValue.value, it.value) + } + } + } + } +} From 5864ecc70b97329429492dfdc8d23d514854231c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= Date: Tue, 3 Dec 2024 09:58:23 +0100 Subject: [PATCH 2/2] trigger build