From cdd850d4dd5606587c948e76fa3c964993997b91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Fri, 27 Feb 2026 09:33:03 +0100 Subject: [PATCH] Apply suggestion: - Added `MediaSource.safeUrl` property replacing `withCleanUrl` method. - Made `url` private so it can't be used externally. - Reverted code in `CoilMediaFetcher` - Also add tests --- .../model/event/TimelineItemStickerContent.kt | 2 +- .../libraries/matrix/api/media/MediaSource.kt | 35 +++++++------------ .../matrix/api/media/MediaSourceTest.kt | 26 ++++++++++++++ .../matrix/impl/media/RustMediaLoader.kt | 3 +- .../matrix/ui/media/CoilMediaFetcher.kt | 13 +++---- .../matrix/ui/media/MediaRequestDataKeyer.kt | 2 +- .../impl/viewer/MediaViewerDataSource.kt | 6 ++-- .../notifications/NotificationMediaRepo.kt | 2 +- .../voiceplayer/impl/VoiceMessageMediaRepo.kt | 2 +- 9 files changed, 50 insertions(+), 41 deletions(-) create mode 100644 libraries/matrix/api/src/test/kotlin/io/element/android/libraries/matrix/api/media/MediaSourceTest.kt diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemStickerContent.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemStickerContent.kt index d333c0b6e7..9e2f08d5c7 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemStickerContent.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemStickerContent.kt @@ -30,5 +30,5 @@ data class TimelineItemStickerContent( /* Stickers are supposed to be small images so we allow using the mediaSource (unless the url is empty) */ - val preferredMediaSource = if (mediaSource.url.isEmpty()) thumbnailSource else mediaSource + val preferredMediaSource = if (mediaSource.safeUrl.isEmpty()) thumbnailSource else mediaSource } diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MediaSource.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MediaSource.kt index 9d0ea7fd13..2f7fff84ef 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MediaSource.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MediaSource.kt @@ -9,7 +9,7 @@ package io.element.android.libraries.matrix.api.media import android.os.Parcelable -import androidx.core.net.toUri +import kotlinx.parcelize.IgnoredOnParcel import kotlinx.parcelize.Parcelize @Parcelize @@ -17,31 +17,20 @@ data class MediaSource( /** * Url of the media. */ - val url: String, + private val url: String, /** * This is used to hold data for encrypted media. */ val json: String? = null, -) : Parcelable - -/** - * Returns a new [MediaSource] with a valid URL. - */ -fun MediaSource.withCleanUrl(): MediaSource { - val uri = this.url.toUri() - if (uri.scheme != "mxc") return this - - // We've seen some MXC urls in the wild having some `mxc://foo/bar#auto` fragment suffix, which is invalid - val cleanedUrl = buildString { - append(uri.scheme) - if (!this.endsWith("://")) { - append("://") - } - append(uri.host) - if (uri.path != null) { - append(uri.path) - } +) : Parcelable { + /** + * A URL with invalid parts (like `#fragment`, if it's an MXC url) removed. + */ + @IgnoredOnParcel + val safeUrl = if (url.startsWith("mxc")) { + // We've seen some MXC urls in the wild having some `mxc://foo/bar#auto` fragment suffix, which is invalid + url.substringBefore("#") + } else { + url } - - return this.copy(url = cleanedUrl) } diff --git a/libraries/matrix/api/src/test/kotlin/io/element/android/libraries/matrix/api/media/MediaSourceTest.kt b/libraries/matrix/api/src/test/kotlin/io/element/android/libraries/matrix/api/media/MediaSourceTest.kt new file mode 100644 index 0000000000..55c1381a4d --- /dev/null +++ b/libraries/matrix/api/src/test/kotlin/io/element/android/libraries/matrix/api/media/MediaSourceTest.kt @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2026 Element Creations Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial. + * Please see LICENSE files in the repository root for full details. + */ + +package io.element.android.libraries.matrix.api.media + +import com.google.common.truth.Truth.assertThat +import io.element.android.libraries.matrix.test.media.aMediaSource +import org.junit.Test + +class MediaSourceTest { + @Test + fun `safeUrl removes the fragment part in MXC urls`() { + val mediaSource = aMediaSource(url = "mxc://matrix.org/url#fragment") + assertThat(mediaSource.safeUrl).isEqualTo("mxc://matrix.org/url") + } + + @Test + fun `safeUrl keeps the fragment part in a non-MXC url`() { + val mediaSource = aMediaSource(url = "https://matrix.org/url#fragment") + assertThat(mediaSource.safeUrl).isEqualTo("https://matrix.org/url#fragment") + } +} diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt index 653b4d055d..81946980e3 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt @@ -14,7 +14,6 @@ import io.element.android.libraries.core.mimetype.MimeTypes import io.element.android.libraries.matrix.api.media.MatrixMediaLoader import io.element.android.libraries.matrix.api.media.MediaFile import io.element.android.libraries.matrix.api.media.MediaSource -import io.element.android.libraries.matrix.api.media.withCleanUrl import kotlinx.coroutines.withContext import org.matrix.rustcomponents.sdk.Client import org.matrix.rustcomponents.sdk.use @@ -92,7 +91,7 @@ class RustMediaLoader( return if (json != null) { RustMediaSource.fromJson(json) } else { - RustMediaSource.fromUrl(withCleanUrl().url) + RustMediaSource.fromUrl(safeUrl) } } } diff --git a/libraries/matrixmedia/impl/src/main/kotlin/io/element/android/libraries/matrix/ui/media/CoilMediaFetcher.kt b/libraries/matrixmedia/impl/src/main/kotlin/io/element/android/libraries/matrix/ui/media/CoilMediaFetcher.kt index e08746f8df..39f67b41d7 100644 --- a/libraries/matrixmedia/impl/src/main/kotlin/io/element/android/libraries/matrix/ui/media/CoilMediaFetcher.kt +++ b/libraries/matrixmedia/impl/src/main/kotlin/io/element/android/libraries/matrix/ui/media/CoilMediaFetcher.kt @@ -16,7 +16,6 @@ import coil3.fetch.SourceFetchResult import io.element.android.libraries.matrix.api.media.MatrixMediaLoader import io.element.android.libraries.matrix.api.media.MediaSource import io.element.android.libraries.matrix.api.media.toFile -import io.element.android.libraries.matrix.api.media.withCleanUrl import okio.Buffer import okio.FileSystem import okio.Path.Companion.toOkioPath @@ -28,14 +27,10 @@ internal class CoilMediaFetcher( private val mediaData: MediaRequestData, ) : Fetcher { override suspend fun fetch(): FetchResult? { - val source = mediaData.source - val mediaSource = when { - source == null -> { - Timber.e("MediaData source is null") - return null - } - source.url.startsWith("mxc:") -> source.withCleanUrl() - else -> source + val mediaSource = mediaData.source + if (mediaSource == null) { + Timber.e("MediaData source is null") + return null } return when (val kind = mediaData.kind) { is MediaRequestData.Kind.Content -> fetchContent(mediaSource) diff --git a/libraries/matrixmedia/impl/src/main/kotlin/io/element/android/libraries/matrix/ui/media/MediaRequestDataKeyer.kt b/libraries/matrixmedia/impl/src/main/kotlin/io/element/android/libraries/matrix/ui/media/MediaRequestDataKeyer.kt index 488803e933..660b80500a 100644 --- a/libraries/matrixmedia/impl/src/main/kotlin/io/element/android/libraries/matrix/ui/media/MediaRequestDataKeyer.kt +++ b/libraries/matrixmedia/impl/src/main/kotlin/io/element/android/libraries/matrix/ui/media/MediaRequestDataKeyer.kt @@ -25,5 +25,5 @@ internal class MediaRequestDataKeyer : Keyer { } private fun MediaRequestData.toKey(): String? { - return source?.let { "${it.url}_$kind" } + return source?.let { "${it.safeUrl}_$kind" } } diff --git a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSource.kt b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSource.kt index ae7b54ccdd..928e5d9ca8 100644 --- a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSource.kt +++ b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSource.kt @@ -126,7 +126,7 @@ class MediaViewerDataSource( when (mediaItem) { is MediaItem.DateSeparator -> Unit is MediaItem.Event -> { - val sourceUrl = mediaItem.mediaSource().url + val sourceUrl = mediaItem.mediaSource().safeUrl val localMedia = localMediaStates.getOrPut(sourceUrl) { mutableStateOf(AsyncData.Uninitialized) } @@ -153,7 +153,7 @@ class MediaViewerDataSource( }.toImmutableList() fun clearLoadingError(data: MediaViewerPageData.MediaViewerData) { - localMediaStates[data.mediaSource.url]?.value = AsyncData.Uninitialized + localMediaStates[data.mediaSource.safeUrl]?.value = AsyncData.Uninitialized } suspend fun loadMore(direction: Timeline.PaginationDirection) { @@ -162,7 +162,7 @@ class MediaViewerDataSource( suspend fun loadMedia(data: MediaViewerPageData.MediaViewerData) { Timber.d("loadMedia for ${data.eventId}") - val localMediaState = localMediaStates.getOrPut(data.mediaSource.url) { + val localMediaState = localMediaStates.getOrPut(data.mediaSource.safeUrl) { mutableStateOf(AsyncData.Uninitialized) } localMediaState.value = AsyncData.Loading() diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationMediaRepo.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationMediaRepo.kt index f309f754c3..39b3f3fda2 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationMediaRepo.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationMediaRepo.kt @@ -101,7 +101,7 @@ class DefaultNotificationMediaRepo( } } - private fun MediaSource.cachedFile(): File? = mxcTools.mxcUri2FilePath(url)?.let { + private fun MediaSource.cachedFile(): File? = mxcTools.mxcUri2FilePath(safeUrl)?.let { File("${cacheDir.path}/$CACHE_NOTIFICATION_SUBDIR/$it") } } diff --git a/libraries/voiceplayer/impl/src/main/kotlin/io/element/android/libraries/voiceplayer/impl/VoiceMessageMediaRepo.kt b/libraries/voiceplayer/impl/src/main/kotlin/io/element/android/libraries/voiceplayer/impl/VoiceMessageMediaRepo.kt index 7cc09c4009..2d1c9f7277 100644 --- a/libraries/voiceplayer/impl/src/main/kotlin/io/element/android/libraries/voiceplayer/impl/VoiceMessageMediaRepo.kt +++ b/libraries/voiceplayer/impl/src/main/kotlin/io/element/android/libraries/voiceplayer/impl/VoiceMessageMediaRepo.kt @@ -95,7 +95,7 @@ class DefaultVoiceMessageMediaRepo( } } - private val cachedFile: File? = mxcTools.mxcUri2FilePath(mediaSource.url)?.let { + private val cachedFile: File? = mxcTools.mxcUri2FilePath(mediaSource.safeUrl)?.let { File("${cacheDir.path}/$CACHE_VOICE_SUBDIR/$it") } }