From 9983871db7cea4035aa18e9113e57426aa756c34 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 23 Oct 2025 09:32:52 +0200 Subject: [PATCH 1/2] Introduce WorkerDataConverter to avoid hard coded Json key and ensure serializing/deserializing is performed at the same place. --- .../NotificationResolverQueue.kt | 6 +- .../workmanager/FetchNotificationsWorker.kt | 16 +---- .../SyncNotificationWorkManagerRequest.kt | 38 ++-------- .../impl/workmanager/WorkerDataConverter.kt | 72 +++++++++++++++++++ .../push/impl/push/DefaultPushHandlerTest.kt | 3 +- .../FetchNotificationWorkerTest.kt | 2 +- .../SyncNotificationWorkManagerRequestTest.kt | 2 +- .../workmanager/WorkerDataConverterTest.kt | 53 ++++++++++++++ 8 files changed, 140 insertions(+), 52 deletions(-) create mode 100644 libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt create mode 100644 libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationResolverQueue.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationResolverQueue.kt index 928f7bfee1..58ff8a5064 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationResolverQueue.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationResolverQueue.kt @@ -11,13 +11,13 @@ import dev.zacsweers.metro.AppScope import dev.zacsweers.metro.ContributesBinding import dev.zacsweers.metro.Inject import dev.zacsweers.metro.SingleIn -import io.element.android.libraries.androidutils.json.JsonProvider import io.element.android.libraries.di.annotations.AppCoroutineScope import io.element.android.libraries.featureflag.api.FeatureFlagService import io.element.android.libraries.featureflag.api.FeatureFlags import io.element.android.libraries.push.api.push.NotificationEventRequest import io.element.android.libraries.push.impl.notifications.model.ResolvedPushEvent import io.element.android.libraries.push.impl.workmanager.SyncNotificationWorkManagerRequest +import io.element.android.libraries.push.impl.workmanager.WorkerDataConverter import io.element.android.libraries.workmanager.api.WorkManagerScheduler import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -50,7 +50,7 @@ class DefaultNotificationResolverQueue( private val appCoroutineScope: CoroutineScope, private val workManagerScheduler: WorkManagerScheduler, private val featureFlagService: FeatureFlagService, - private val json: JsonProvider, + private val workerDataConverter: WorkerDataConverter, ) : NotificationResolverQueue { companion object { private const val BATCH_WINDOW_MS = 250L @@ -101,7 +101,7 @@ class DefaultNotificationResolverQueue( SyncNotificationWorkManagerRequest( sessionId = sessionId, notificationEventRequests = requests, - json = json, + workerDataConverter = workerDataConverter, ) ) } diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/FetchNotificationsWorker.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/FetchNotificationsWorker.kt index 4839bc193f..fadd1a377c 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/FetchNotificationsWorker.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/FetchNotificationsWorker.kt @@ -18,7 +18,6 @@ import dev.zacsweers.metro.ContributesIntoMap import dev.zacsweers.metro.binding import io.element.android.features.networkmonitor.api.NetworkMonitor import io.element.android.features.networkmonitor.api.NetworkStatus -import io.element.android.libraries.androidutils.json.JsonProvider import io.element.android.libraries.core.coroutine.CoroutineDispatchers import io.element.android.libraries.core.extensions.runCatchingExceptions import io.element.android.libraries.di.annotations.ApplicationContext @@ -47,20 +46,11 @@ class FetchNotificationsWorker( private val workManagerScheduler: WorkManagerScheduler, private val syncOnNotifiableEvent: SyncOnNotifiableEvent, private val coroutineDispatchers: CoroutineDispatchers, - private val json: JsonProvider, + private val workerDataConverter: WorkerDataConverter, ) : CoroutineWorker(context, workerParams) { override suspend fun doWork(): Result = withContext(coroutineDispatchers.io) { Timber.d("FetchNotificationsWorker started") - val rawRequestsJson = inputData.getString("requests") ?: return@withContext Result.failure() - val requests = runCatchingExceptions { - json().decodeFromString>(rawRequestsJson).map { it.toRequest() } - }.getOrElse { - Timber.e(it, "Failed to deserialize notification requests") - return@withContext Result.failure() - } - - Timber.d("Deserialized ${requests.size} requests") - + val requests = workerDataConverter.deserialize(inputData) ?: return@withContext Result.failure() // Wait for network to be available, but not more than 10 seconds val hasNetwork = withTimeoutOrNull(10.seconds) { networkMonitor.connectivity.first { it == NetworkStatus.Connected } @@ -97,7 +87,7 @@ class FetchNotificationsWorker( SyncNotificationWorkManagerRequest( sessionId = failedSessionId, notificationEventRequests = requestsToRetry, - json = json, + workerDataConverter = workerDataConverter, ) ) } diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt index db08b0041d..b0aabe1cc7 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt @@ -10,11 +10,6 @@ package io.element.android.libraries.push.impl.workmanager import androidx.work.OneTimeWorkRequestBuilder import androidx.work.OutOfQuotaPolicy import androidx.work.WorkRequest -import androidx.work.workDataOf -import io.element.android.libraries.androidutils.json.JsonProvider -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 import io.element.android.libraries.push.api.push.NotificationEventRequest import io.element.android.libraries.workmanager.api.WorkManagerRequest @@ -28,24 +23,19 @@ import java.security.InvalidParameterException class SyncNotificationWorkManagerRequest( private val sessionId: SessionId, private val notificationEventRequests: List, - private val json: JsonProvider, + private val workerDataConverter: WorkerDataConverter, ) : WorkManagerRequest { override fun build(): Result { if (notificationEventRequests.isEmpty()) { return Result.failure(InvalidParameterException("notificationEventRequests cannot be empty")) } - - val json = runCatchingExceptions { json().encodeToString(notificationEventRequests.map { it.toData() }) } - .getOrElse { - Timber.e(it, "Failed to serialize notification requests") - return Result.failure(it) - } - + val data = workerDataConverter.serialize(notificationEventRequests).getOrElse { + return Result.failure(it) + } Timber.d("Scheduling ${notificationEventRequests.size} notification requests with WorkManager for $sessionId") - return Result.success( OneTimeWorkRequestBuilder() - .setInputData(workDataOf("requests" to json)) + .setInputData(data) .setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) .setTraceTag(workManagerTag(sessionId, WorkManagerRequestType.NOTIFICATION_SYNC)) // TODO investigate using this instead of the resolver queue @@ -64,23 +54,5 @@ class SyncNotificationWorkManagerRequest( val eventId: String, @SerialName("provider_info") val providerInfo: String, - ) { - fun toRequest(): NotificationEventRequest { - return NotificationEventRequest( - sessionId = SessionId(sessionId), - roomId = RoomId(roomId), - eventId = EventId(eventId), - providerInfo = providerInfo, - ) - } - } -} - -private fun NotificationEventRequest.toData(): SyncNotificationWorkManagerRequest.Data { - return SyncNotificationWorkManagerRequest.Data( - sessionId = sessionId.value, - roomId = roomId.value, - eventId = eventId.value, - providerInfo = providerInfo, ) } diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt new file mode 100644 index 0000000000..ce961e1dc2 --- /dev/null +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt @@ -0,0 +1,72 @@ +/* + * Copyright 2025 New Vector 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.push.impl.workmanager + +import androidx.work.Data +import androidx.work.workDataOf +import dev.zacsweers.metro.Inject +import io.element.android.libraries.androidutils.json.JsonProvider +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 +import io.element.android.libraries.push.api.push.NotificationEventRequest +import timber.log.Timber + +@Inject +class WorkerDataConverter( + private val json: JsonProvider, +) { + fun serialize(notificationEventRequests: List): Result { + return runCatchingExceptions { json().encodeToString(notificationEventRequests.map { it.toData() }) } + .onFailure { + Timber.e(it, "Failed to serialize notification requests") + } + .map { str -> + workDataOf(REQUESTS_KEY to str) + } + } + + fun deserialize(data: Data): List? { + val rawRequestsJson = data.getString(REQUESTS_KEY) ?: return null + return runCatchingExceptions { + json().decodeFromString>(rawRequestsJson).map { it.toRequest() } + }.fold( + onSuccess = { + Timber.d("Deserialized ${it.size} requests") + it + }, + onFailure = { + Timber.e(it, "Failed to deserialize notification requests") + null + } + ) + } + + companion object { + private const val REQUESTS_KEY = "requests" + } +} + +private fun NotificationEventRequest.toData(): SyncNotificationWorkManagerRequest.Data { + return SyncNotificationWorkManagerRequest.Data( + sessionId = sessionId.value, + roomId = roomId.value, + eventId = eventId.value, + providerInfo = providerInfo, + ) +} + +private fun SyncNotificationWorkManagerRequest.Data.toRequest(): NotificationEventRequest { + return NotificationEventRequest( + sessionId = SessionId(sessionId), + roomId = RoomId(roomId), + eventId = EventId(eventId), + providerInfo = providerInfo, + ) +} diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandlerTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandlerTest.kt index 4bfb944202..870b9e1e1a 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandlerTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandlerTest.kt @@ -46,6 +46,7 @@ import io.element.android.libraries.push.impl.notifications.model.NotifiableEven import io.element.android.libraries.push.impl.notifications.model.ResolvedPushEvent import io.element.android.libraries.push.impl.test.DefaultTestPush import io.element.android.libraries.push.impl.troubleshoot.DiagnosticPushHandler +import io.element.android.libraries.push.impl.workmanager.WorkerDataConverter import io.element.android.libraries.pushproviders.api.PushData import io.element.android.libraries.pushstore.api.UserPushStore import io.element.android.libraries.pushstore.api.clientsecret.PushClientSecret @@ -715,7 +716,7 @@ class DefaultPushHandlerTest { appCoroutineScope = backgroundScope, workManagerScheduler = workManagerScheduler, featureFlagService = featureFlagService, - json = DefaultJsonProvider(), + workerDataConverter = WorkerDataConverter(DefaultJsonProvider()), ), appCoroutineScope = backgroundScope, fallbackNotificationFactory = FallbackNotificationFactory( diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/FetchNotificationWorkerTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/FetchNotificationWorkerTest.kt index 4a98ed970a..74241fecd8 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/FetchNotificationWorkerTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/FetchNotificationWorkerTest.kt @@ -175,7 +175,7 @@ class FetchNotificationWorkerTest { workManagerScheduler = workManagerScheduler, syncOnNotifiableEvent = syncOnNotifiableEvent, coroutineDispatchers = testCoroutineDispatchers(), - json = DefaultJsonProvider(), + workerDataConverter = WorkerDataConverter(DefaultJsonProvider()), ) private fun TestScope.createWorkerParams( diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt index 91937a2a67..fcefd8ab8b 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt @@ -58,5 +58,5 @@ private fun createSyncNotificationWorkManagerRequest( ) = SyncNotificationWorkManagerRequest( sessionId = sessionId, notificationEventRequests = notificationEventRequests, - json = DefaultJsonProvider(), + workerDataConverter = WorkerDataConverter(DefaultJsonProvider()), ) diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt new file mode 100644 index 0000000000..a4a53208b3 --- /dev/null +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt @@ -0,0 +1,53 @@ +/* + * Copyright 2025 New Vector 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.push.impl.workmanager + +import com.google.common.truth.Truth.assertThat +import io.element.android.libraries.androidutils.json.DefaultJsonProvider +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_ROOM_ID +import io.element.android.libraries.matrix.test.A_ROOM_ID_2 +import io.element.android.libraries.matrix.test.A_SESSION_ID +import io.element.android.libraries.matrix.test.A_SESSION_ID_2 +import io.element.android.libraries.push.api.push.NotificationEventRequest +import org.junit.Test + +class WorkerDataConverterTest { + @Test + fun `ensure identity when serializing - deserializing an empty list`() { + testIdentity(emptyList()) + } + + @Test + fun `ensure identity when serializing - deserializing a list`() { + testIdentity( + listOf( + NotificationEventRequest( + sessionId = A_SESSION_ID, + roomId = A_ROOM_ID, + eventId = AN_EVENT_ID, + providerInfo = "info1", + ), + NotificationEventRequest( + sessionId = A_SESSION_ID_2, + roomId = A_ROOM_ID_2, + eventId = AN_EVENT_ID_2, + providerInfo = "info2", + ), + ) + ) + } + + private fun testIdentity(data: List) { + val sut = WorkerDataConverter(DefaultJsonProvider()) + val serialized = sut.serialize(data).getOrThrow() + val result = sut.deserialize(serialized) + assertThat(result).isEqualTo(data) + } +} From 937519639df02b903a6d23d66a54ebd235ff6ba0 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 23 Oct 2025 09:59:24 +0200 Subject: [PATCH 2/2] Add missing test. --- .../libraries/androidutils/json/JsonProvider.kt | 2 +- .../SyncNotificationWorkManagerRequestTest.kt | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/json/JsonProvider.kt b/libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/json/JsonProvider.kt index 08226060db..2e91cd6784 100644 --- a/libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/json/JsonProvider.kt +++ b/libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/json/JsonProvider.kt @@ -17,7 +17,7 @@ import kotlinx.serialization.json.Json /** * Provides a Json instance configured to ignore unknown keys. */ -interface JsonProvider : Provider +fun interface JsonProvider : Provider @ContributesBinding(AppScope::class) @SingleIn(AppScope::class) diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt index fcefd8ab8b..ebbe4eb865 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt @@ -49,14 +49,24 @@ class SyncNotificationWorkManagerRequestTest { assertThat(result.isFailure).isTrue() } - // TODO add test for invalid serialization (how?) + @Test + fun `build - invalid serialization`() = runTest { + val request = createSyncNotificationWorkManagerRequest( + sessionId = A_SESSION_ID, + notificationEventRequests = listOf(aNotificationEventRequest()), + workerDataConverter = WorkerDataConverter({ error("error during serialization") }) + ) + val result = request.build() + assertThat(result.isFailure).isTrue() + } } private fun createSyncNotificationWorkManagerRequest( sessionId: SessionId, notificationEventRequests: List, + workerDataConverter: WorkerDataConverter = WorkerDataConverter(DefaultJsonProvider()) ) = SyncNotificationWorkManagerRequest( sessionId = sessionId, notificationEventRequests = notificationEventRequests, - workerDataConverter = WorkerDataConverter(DefaultJsonProvider()), + workerDataConverter = workerDataConverter, )