From 26eecb33dd55db29b9402ef642a1cdbd5bd1819f Mon Sep 17 00:00:00 2001 From: Marco Romano Date: Wed, 8 Nov 2023 07:46:00 +0100 Subject: [PATCH] Voice messages: Don't crash if mxc uri is invalid (#1756) ## Type of change - [ ] Feature - [x] Bugfix - [ ] Technical - [ ] Other : ## Content Code now checks whether the mx uri is invalid and handles the error condition properly. ## Motivation and context https://github.com/vector-im/element-x-android/pull/1748#discussion_r1384557443 ## Screenshots / GIFs ## Tests - Step 1 - Step 2 - Step ... ## Tested devices - [ ] Physical - [ ] Emulator - OS version(s): ## Checklist - [ ] Changes have been tested on an Android device or Android emulator with API 23 - [ ] UI change has been tested on both light and dark themes - [ ] Accessibility has been taken into account. See https://github.com/vector-im/element-x-android/blob/develop/CONTRIBUTING.md#accessibility - [ ] Pull request is based on the develop branch - [ ] Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-x-android/blob/develop/CONTRIBUTING.md#changelog - [ ] Pull request includes screenshots or videos if containing UI changes - [ ] Pull request includes a [sign off](https://matrix-org.github.io/synapse/latest/development/contributing_guide.html#sign-off) - [ ] You've made a self review of your PR --- .../timeline/VoiceMessageMediaRepo.kt | 23 ++++++++----------- .../DefaultVoiceMessageMediaRepoTest.kt | 21 +++++++++++++++-- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessageMediaRepo.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessageMediaRepo.kt index a559183261..98b43ae415 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessageMediaRepo.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessageMediaRepo.kt @@ -83,13 +83,15 @@ class DefaultVoiceMessageMediaRepo @AssistedInject constructor( ): DefaultVoiceMessageMediaRepo } - override suspend fun getMediaFile(): Result = if (!isInCache()) { - matrixMediaLoader.downloadMediaFile( + override suspend fun getMediaFile(): Result = when { + cachedFile == null -> Result.failure(IllegalStateException("Invalid mxcUri.")) + cachedFile.exists() -> Result.success(cachedFile) + else -> matrixMediaLoader.downloadMediaFile( source = mediaSource, mimeType = mimeType, body = body, ).mapCatching { - val dest = cachedFilePath.apply { parentFile?.mkdirs() } + val dest = cachedFile.apply { parentFile?.mkdirs() } // TODO By not closing the MediaFile we're leaking the rust file handle here. // Not that big of a deal but better to avoid it someday. if (it.toFile().renameTo(dest)) { @@ -98,13 +100,11 @@ class DefaultVoiceMessageMediaRepo @AssistedInject constructor( error("Failed to move file to cache.") } } - } else { - Result.success(cachedFilePath) } - private val cachedFilePath: File = File("${cacheDir.path}/$CACHE_VOICE_SUBDIR/${mxcUri2FilePath(mediaSource.url)}") - - private fun isInCache(): Boolean = cachedFilePath.exists() + private val cachedFile: File? = mxcUri2FilePath(mediaSource.url)?.let { + File("${cacheDir.path}/$CACHE_VOICE_SUBDIR/$it") + } } /** @@ -123,12 +123,9 @@ private val mxcRegex = Regex("""^mxc:\/\/([^\/]+)\/([^\/]+)$""") * Sanitizes an mxcUri to be used as a relative file path. * * @param mxcUri the Matrix Content (mxc://) URI of the voice message. - * @return the relative file path as "/". - * @throws IllegalStateException if the mxcUri is invalid. + * @return the relative file path as "/" or null if the mxcUri is invalid. */ -private fun mxcUri2FilePath(mxcUri: String): String = checkNotNull(mxcRegex.matchEntire(mxcUri)) { - "mxcUri2FilePath: Invalid mxcUri: $mxcUri" -}.let { match -> +private fun mxcUri2FilePath(mxcUri: String): String? = mxcRegex.matchEntire(mxcUri)?.let { match -> buildString { append(match.groupValues[1]) append("/") diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/DefaultVoiceMessageMediaRepoTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/DefaultVoiceMessageMediaRepoTest.kt index 3b19e66450..152090b14c 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/DefaultVoiceMessageMediaRepoTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/DefaultVoiceMessageMediaRepoTest.kt @@ -63,7 +63,7 @@ class DefaultVoiceMessageMediaRepoTest { repo.getMediaFile().let { result -> Truth.assertThat(result.isFailure).isTrue() - result.exceptionOrNull()?.let { exception -> + result.exceptionOrNull()!!.let { exception -> Truth.assertThat(exception).isInstanceOf(RuntimeException::class.java) } } @@ -116,16 +116,32 @@ class DefaultVoiceMessageMediaRepoTest { } } } + + @Test + fun `invalid mxc uri returns a failure`() = runTest { + val repo = createDefaultVoiceMessageMediaRepo( + temporaryFolder = temporaryFolder, + mxcUri = INVALID_MXC_URI, + ) + repo.getMediaFile().let { result -> + Truth.assertThat(result.isFailure).isTrue() + result.exceptionOrNull()!!.let { exception -> + Truth.assertThat(exception).isInstanceOf(RuntimeException::class.java) + Truth.assertThat(exception).hasMessageThat().isEqualTo("Invalid mxcUri.") + } + } + } } private fun createDefaultVoiceMessageMediaRepo( temporaryFolder: TemporaryFolder, matrixMediaLoader: MatrixMediaLoader = FakeMediaLoader(), + mxcUri: String = MXC_URI, ) = DefaultVoiceMessageMediaRepo( cacheDir = temporaryFolder.root, matrixMediaLoader = matrixMediaLoader, mediaSource = MediaSource( - url = MXC_URI, + url = mxcUri, json = null ), mimeType = "audio/ogg", @@ -133,6 +149,7 @@ private fun createDefaultVoiceMessageMediaRepo( ) private const val MXC_URI = "mxc://matrix.org/1234567890abcdefg" +private const val INVALID_MXC_URI = "notAnMxcUri" private val TemporaryFolder.cachedFilePath get() = "${this.root.path}/temp/voice/matrix.org/1234567890abcdefg" private fun TemporaryFolder.createCachedFile() = File(cachedFilePath).apply { parentFile?.mkdirs()