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.

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 {
+    ): Result<NotificationData> {
-        return notificationItem.use { item ->
+        return runCatchingExceptions {
-            val isDm = isDm(
+            notificationItem.use { item ->
-                isDirect = item.roomInfo.isDirect,
+                val isDm = isDm(
-                activeMembersCount = item.roomInfo.joinedMembersCount.toInt(),
+                    isDirect = item.roomInfo.isDirect,
-            )
+                    activeMembersCount = item.roomInfo.joinedMembersCount.toInt(),
-            NotificationData(
+                )
-                sessionId = sessionId,
+                NotificationData(
-                eventId = eventId,
+                    sessionId = sessionId,
-                // FIXME once the `NotificationItem` in the SDK returns the thread id
+                    eventId = eventId,
-                threadId = null,
+                    // FIXME once the `NotificationItem` in the SDK returns the thread id
-                roomId = roomId,
+                    threadId = null,
-                senderAvatarUrl = item.senderInfo.avatarUrl,
+                    roomId = roomId,
-                senderDisplayName = item.senderInfo.displayName,
+                    senderAvatarUrl = item.senderInfo.avatarUrl,
-                senderIsNameAmbiguous = item.senderInfo.isNameAmbiguous,
+                    senderDisplayName = item.senderInfo.displayName,
-                roomAvatarUrl = item.roomInfo.avatarUrl ?: item.senderInfo.avatarUrl.takeIf { isDm },
+                    senderIsNameAmbiguous = item.senderInfo.isNameAmbiguous,
-                roomDisplayName = item.roomInfo.displayName,
+                    roomAvatarUrl = item.roomInfo.avatarUrl ?: item.senderInfo.avatarUrl.takeIf { isDm },
-                isDirect = item.roomInfo.isDirect,
+                    roomDisplayName = item.roomInfo.displayName,
-                isDm = isDm,
+                    isDirect = item.roomInfo.isDirect,
-                isEncrypted = item.roomInfo.isEncrypted.orFalse(),
+                    isDm = isDm,
-                isNoisy = item.isNoisy.orFalse(),
+                    isEncrypted = item.roomInfo.isEncrypted.orFalse(),
-                timestamp = item.timestamp() ?: clock.epochMillis(),
+                    isNoisy = item.isNoisy.orFalse(),
-                content = item.event.use { notificationContentMapper.map(it) },
+                    timestamp = item.timestamp() ?: clock.epochMillis(),
-                hasMention = item.hasMention.orFalse(),
+                    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<NotificationContent> =
         when (notificationEvent) {
             is NotificationEvent.Timeline -> timelineEventToNotificationContentMapper.map(notificationEvent.event)
-            is NotificationEvent.Invite -> NotificationContent.Invite(
+            is NotificationEvent.Invite -> Result.success(
-                senderId = UserId(notificationEvent.sender),
+                NotificationContent.Invite(
+                    senderId = UserId(notificationEvent.sender),
+                )
             )
         }
 }

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")

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 {
+    fun map(timelineEvent: TimelineEvent): Result<NotificationContent> {
-        return timelineEvent.use {
+        return runCatchingExceptions {
-            timelineEvent.eventType().use { eventType ->
+            timelineEvent.use {
-                eventType.toContent(senderId = UserId(timelineEvent.senderId()))
+                timelineEvent.eventType().use { eventType ->
+                    eventType.toContent(senderId = UserId(timelineEvent.senderId()))
+                }
             }
         }
     }

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,

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(