From 16b9440048acd76bc9fb2867167406586cb5381a Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Fri, 8 Aug 2025 11:56:15 +0200 Subject: [PATCH] When mapping invalid notification event, only drop that one (#5137) Previously, this meant the code processing the whole notification batch result failed and other notifications would be lost too. --- .../impl/notification/NotificationMapper.kt | 61 ++++++++++--------- .../notification/RustNotificationService.kt | 4 +- ...imelineEventToNotificationContentMapper.kt | 11 ++-- .../fixtures/fakes/FakeFfiTimelineEvent.kt | 2 +- .../RustNotificationServiceTest.kt | 33 ++++++++++ 5 files changed, 77 insertions(+), 34 deletions(-) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/NotificationMapper.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/NotificationMapper.kt index 9130df5c7a..e7831bc492 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/NotificationMapper.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/NotificationMapper.kt @@ -8,6 +8,7 @@ package io.element.android.libraries.matrix.impl.notification import io.element.android.libraries.core.bool.orFalse +import io.element.android.libraries.core.extensions.runCatchingExceptions import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.api.core.SessionId @@ -30,31 +31,33 @@ class NotificationMapper( eventId: EventId, roomId: RoomId, notificationItem: NotificationItem - ): NotificationData { - return notificationItem.use { item -> - val isDm = isDm( - isDirect = item.roomInfo.isDirect, - activeMembersCount = item.roomInfo.joinedMembersCount.toInt(), - ) - NotificationData( - sessionId = sessionId, - eventId = eventId, - // FIXME once the `NotificationItem` in the SDK returns the thread id - threadId = null, - roomId = roomId, - senderAvatarUrl = item.senderInfo.avatarUrl, - senderDisplayName = item.senderInfo.displayName, - senderIsNameAmbiguous = item.senderInfo.isNameAmbiguous, - roomAvatarUrl = item.roomInfo.avatarUrl ?: item.senderInfo.avatarUrl.takeIf { isDm }, - roomDisplayName = item.roomInfo.displayName, - isDirect = item.roomInfo.isDirect, - isDm = isDm, - isEncrypted = item.roomInfo.isEncrypted.orFalse(), - isNoisy = item.isNoisy.orFalse(), - timestamp = item.timestamp() ?: clock.epochMillis(), - content = item.event.use { notificationContentMapper.map(it) }, - hasMention = item.hasMention.orFalse(), - ) + ): Result { + return runCatchingExceptions { + notificationItem.use { item -> + val isDm = isDm( + isDirect = item.roomInfo.isDirect, + activeMembersCount = item.roomInfo.joinedMembersCount.toInt(), + ) + NotificationData( + sessionId = sessionId, + eventId = eventId, + // FIXME once the `NotificationItem` in the SDK returns the thread id + threadId = null, + roomId = roomId, + senderAvatarUrl = item.senderInfo.avatarUrl, + senderDisplayName = item.senderInfo.displayName, + senderIsNameAmbiguous = item.senderInfo.isNameAmbiguous, + roomAvatarUrl = item.roomInfo.avatarUrl ?: item.senderInfo.avatarUrl.takeIf { isDm }, + roomDisplayName = item.roomInfo.displayName, + isDirect = item.roomInfo.isDirect, + isDm = isDm, + isEncrypted = item.roomInfo.isEncrypted.orFalse(), + isNoisy = item.isNoisy.orFalse(), + timestamp = item.timestamp() ?: clock.epochMillis(), + content = item.event.use { notificationContentMapper.map(it) }.getOrThrow(), + hasMention = item.hasMention.orFalse(), + ) + } } } } @@ -62,11 +65,13 @@ class NotificationMapper( class NotificationContentMapper { private val timelineEventToNotificationContentMapper = TimelineEventToNotificationContentMapper() - fun map(notificationEvent: NotificationEvent): NotificationContent = + fun map(notificationEvent: NotificationEvent): Result = when (notificationEvent) { is NotificationEvent.Timeline -> timelineEventToNotificationContentMapper.map(notificationEvent.event) - is NotificationEvent.Invite -> NotificationContent.Invite( - senderId = UserId(notificationEvent.sender), + is NotificationEvent.Invite -> Result.success( + NotificationContent.Invite( + senderId = UserId(notificationEvent.sender), + ) ) } } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/RustNotificationService.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/RustNotificationService.kt index 4b4693c73b..1379392ae3 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/RustNotificationService.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/RustNotificationService.kt @@ -53,7 +53,9 @@ class RustNotificationService( is BatchNotificationResult.Ok -> { when (val status = result.status) { is NotificationStatus.Event -> { - put(eventId, Result.success(notificationMapper.map(sessionId, eventId, roomId, status.item))) + val result = notificationMapper.map(sessionId, eventId, roomId, status.item) + result.onFailure { Timber.e(it, "Could not map notification event $eventId") } + put(eventId, result) } is NotificationStatus.EventNotFound -> { Timber.e("Could not retrieve event for notification with $eventId - event not found") diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/TimelineEventToNotificationContentMapper.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/TimelineEventToNotificationContentMapper.kt index cf914abe8d..85f87b271f 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/TimelineEventToNotificationContentMapper.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/notification/TimelineEventToNotificationContentMapper.kt @@ -7,6 +7,7 @@ package io.element.android.libraries.matrix.impl.notification +import io.element.android.libraries.core.extensions.runCatchingExceptions import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.core.UserId import io.element.android.libraries.matrix.api.notification.CallNotifyType @@ -21,10 +22,12 @@ import org.matrix.rustcomponents.sdk.TimelineEventType import org.matrix.rustcomponents.sdk.use class TimelineEventToNotificationContentMapper { - fun map(timelineEvent: TimelineEvent): NotificationContent { - return timelineEvent.use { - timelineEvent.eventType().use { eventType -> - eventType.toContent(senderId = UserId(timelineEvent.senderId())) + fun map(timelineEvent: TimelineEvent): Result { + return runCatchingExceptions { + timelineEvent.use { + timelineEvent.eventType().use { eventType -> + eventType.toContent(senderId = UserId(timelineEvent.senderId())) + } } } } diff --git a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiTimelineEvent.kt b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiTimelineEvent.kt index e514cef7a8..41eb9c798e 100644 --- a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiTimelineEvent.kt +++ b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiTimelineEvent.kt @@ -14,7 +14,7 @@ import org.matrix.rustcomponents.sdk.NoPointer import org.matrix.rustcomponents.sdk.TimelineEvent import org.matrix.rustcomponents.sdk.TimelineEventType -class FakeFfiTimelineEvent( +open class FakeFfiTimelineEvent( val timestamp: ULong = A_FAKE_TIMESTAMP.toULong(), val timelineEventType: TimelineEventType = aRustTimelineEventTypeMessageLike(), val senderId: String = A_USER_ID_2.value, diff --git a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/notification/RustNotificationServiceTest.kt b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/notification/RustNotificationServiceTest.kt index 117d164ab9..2b6717910b 100644 --- a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/notification/RustNotificationServiceTest.kt +++ b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/notification/RustNotificationServiceTest.kt @@ -12,8 +12,12 @@ import io.element.android.libraries.matrix.api.exception.NotificationResolverExc import io.element.android.libraries.matrix.api.notification.NotificationContent import io.element.android.libraries.matrix.api.timeline.item.event.TextMessageType import io.element.android.libraries.matrix.impl.fixtures.factories.aRustBatchNotificationResult +import io.element.android.libraries.matrix.impl.fixtures.factories.aRustNotificationEventTimeline +import io.element.android.libraries.matrix.impl.fixtures.factories.aRustNotificationItem import io.element.android.libraries.matrix.impl.fixtures.fakes.FakeFfiNotificationClient +import io.element.android.libraries.matrix.impl.fixtures.fakes.FakeFfiTimelineEvent import io.element.android.libraries.matrix.test.AN_EVENT_ID +import io.element.android.libraries.matrix.test.AN_EVENT_ID_2 import io.element.android.libraries.matrix.test.A_MESSAGE import io.element.android.libraries.matrix.test.A_ROOM_ID import io.element.android.libraries.matrix.test.A_SESSION_ID @@ -26,6 +30,8 @@ import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.runTest import org.junit.Test import org.matrix.rustcomponents.sdk.NotificationClient +import org.matrix.rustcomponents.sdk.NotificationStatus +import org.matrix.rustcomponents.sdk.TimelineEventType class RustNotificationServiceTest { @Test @@ -49,6 +55,33 @@ class RustNotificationServiceTest { ) } + @Test + fun `test mapping invalid item only drops that item`() = runTest { + val error = IllegalStateException("This event type is not supported") + val faultyEvent = object : FakeFfiTimelineEvent() { + override fun eventType(): TimelineEventType { + throw error + } + } + val notificationClient = FakeFfiNotificationClient( + notificationItemResult = mapOf( + AN_EVENT_ID.value to aRustBatchNotificationResult( + notificationStatus = NotificationStatus.Event(aRustNotificationItem(aRustNotificationEventTimeline(faultyEvent))) + ), + AN_EVENT_ID_2.value to aRustBatchNotificationResult() + ), + ) + val sut = createRustNotificationService( + notificationClient = notificationClient, + ) + val result = sut.getNotifications(mapOf(A_ROOM_ID to listOf(AN_EVENT_ID, AN_EVENT_ID_2))).getOrThrow() + val exception = result[AN_EVENT_ID]!!.exceptionOrNull() + assertThat(exception).isEqualTo(error) + + val successfulResult = result[AN_EVENT_ID_2] + assertThat(successfulResult?.isSuccess).isTrue() + } + @Test fun `test unable to resolve event`() = runTest { val notificationClient = FakeFfiNotificationClient(