From 0355c0bda99402a85759e32ce5cb198b6e489fb3 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 19 Nov 2025 17:53:12 +0100 Subject: [PATCH] Improve code again --- .../workmanager/DataForWorkManagerIsTooBig.kt | 10 +++++++ .../SyncNotificationWorkManagerRequest.kt | 4 +-- .../impl/workmanager/WorkerDataConverter.kt | 29 +++++++++++-------- .../SyncNotificationWorkManagerRequestTest.kt | 12 ++++---- .../workmanager/WorkerDataConverterTest.kt | 8 ++--- .../workmanager/api/WorkManagerRequest.kt | 2 +- .../impl/DefaultWorkManagerScheduler.kt | 18 +++++------- 7 files changed, 48 insertions(+), 35 deletions(-) create mode 100644 libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/DataForWorkManagerIsTooBig.kt diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/DataForWorkManagerIsTooBig.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/DataForWorkManagerIsTooBig.kt new file mode 100644 index 0000000000..e700a58a7e --- /dev/null +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/DataForWorkManagerIsTooBig.kt @@ -0,0 +1,10 @@ +/* + * Copyright (c) 2025 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.push.impl.workmanager + +class DataForWorkManagerIsTooBig : Exception() 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 cff12cc17d..a529beba4b 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 @@ -29,9 +29,9 @@ class SyncNotificationWorkManagerRequest( private val workerDataConverter: WorkerDataConverter, private val buildVersionSdkIntProvider: BuildVersionSdkIntProvider, ) : WorkManagerRequest { - override fun build(): List> { + override fun build(): Result> { if (notificationEventRequests.isEmpty()) { - return listOf(Result.failure(InvalidParameterException("notificationEventRequests cannot be empty"))) + return Result.failure(InvalidParameterException("notificationEventRequests cannot be empty")) } Timber.d("Scheduling ${notificationEventRequests.size} notification requests with WorkManager for $sessionId") return workerDataConverter.serialize(notificationEventRequests).map { 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 index 98aea3f528..5efda9647f 100644 --- 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 @@ -24,21 +24,22 @@ import timber.log.Timber class WorkerDataConverter( private val json: JsonProvider, ) { - fun serialize(notificationEventRequests: List): List> { + fun serialize(notificationEventRequests: List): Result> { // First try to serialize all requests at once. In the vast majority of cases this will work. return serializeRequests(notificationEventRequests) - .fold( - onSuccess = { - listOf(Result.success(it)) - }, - onFailure = { - // Perform serialization on sublists, workDataOf may have failed because of size limit + .map { listOf(it) } + .recoverCatching { + if (it is DataForWorkManagerIsTooBig) { + // Perform serialization on sublists, workDataOf have failed because of size limit Timber.w(it, "Failed to serialize ${notificationEventRequests.size} notification requests, trying with chunks of $CHUNK_SIZE.") - notificationEventRequests.chunked(CHUNK_SIZE).map { chunk -> - serializeRequests(chunk) + // TODO Do not split rooms + notificationEventRequests.chunked(CHUNK_SIZE).mapNotNull { chunk -> + serializeRequests(chunk).getOrNull() } - }, - ) + } else { + throw it + } + } } private fun serializeRequests(notificationEventRequests: List): Result { @@ -48,7 +49,11 @@ class WorkerDataConverter( } .mapCatchingExceptions { str -> // Note: workDataOf can fail if the data is too large - workDataOf(REQUESTS_KEY to str) + try { + workDataOf(REQUESTS_KEY to str) + } catch (_: IllegalStateException) { + throw DataForWorkManagerIsTooBig() + } } } 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 cc4890d93f..9dae435a9a 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 @@ -33,8 +33,8 @@ class SyncNotificationWorkManagerRequestTest { ) val result = request.build() - assertThat(result.first().isSuccess).isTrue() - result.first().getOrNull()!!.run { + assertThat(result.isSuccess).isTrue() + result.getOrNull()!!.first().run { assertThat(this).isInstanceOf(OneTimeWorkRequest::class.java) assertThat(workSpec.input.hasKeyWithValueOfType("requests")).isTrue() // True in API 33+ @@ -52,8 +52,8 @@ class SyncNotificationWorkManagerRequestTest { ) val result = request.build() - assertThat(result.first().isSuccess).isTrue() - result.first().getOrNull()!!.run { + assertThat(result.isSuccess).isTrue() + result.getOrNull()!!.first().run { assertThat(this).isInstanceOf(OneTimeWorkRequest::class.java) assertThat(workSpec.input.hasKeyWithValueOfType("requests")).isTrue() // False before API 33 @@ -70,7 +70,7 @@ class SyncNotificationWorkManagerRequestTest { ) val result = request.build() - assertThat(result.first().isFailure).isTrue() + assertThat(result.isFailure).isTrue() } @Test @@ -81,7 +81,7 @@ class SyncNotificationWorkManagerRequestTest { workerDataConverter = WorkerDataConverter({ error("error during serialization") }) ) val result = request.build() - assertThat(result.first().isFailure).isTrue() + assertThat(result.isFailure).isTrue() } } 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 index 9c3f346a28..82e47078f5 100644 --- 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 @@ -58,14 +58,14 @@ class WorkerDataConverterTest { } val sut = WorkerDataConverter(DefaultJsonProvider()) val serialized = sut.serialize(data) - assertThat(serialized.size).isGreaterThan(1) - assertThat(serialized.size).isEqualTo(100 / WorkerDataConverter.CHUNK_SIZE) + assertThat(serialized.getOrNull()?.size).isGreaterThan(1) + assertThat(serialized.getOrNull()?.size).isEqualTo(100 / WorkerDataConverter.CHUNK_SIZE) } private fun testIdentity(data: List) { val sut = WorkerDataConverter(DefaultJsonProvider()) - val serialized = sut.serialize(data).first().getOrThrow() - val result = sut.deserialize(serialized) + val serialized = sut.serialize(data).getOrThrow() + val result = sut.deserialize(serialized.first()) assertThat(result).isEqualTo(data) } } diff --git a/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt b/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt index a6e4a26e3d..af49c3dc87 100644 --- a/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt +++ b/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt @@ -11,5 +11,5 @@ package io.element.android.libraries.workmanager.api import androidx.work.WorkRequest interface WorkManagerRequest { - fun build(): List> + fun build(): Result> } diff --git a/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt b/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt index 6b19e48208..6943c1432e 100644 --- a/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt +++ b/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt @@ -27,16 +27,14 @@ class DefaultWorkManagerScheduler( private val workManager by lazy { WorkManager.getInstance(context) } override fun submit(workManagerRequest: WorkManagerRequest) { - workManagerRequest.build().forEach { - it.fold( - onSuccess = { workRequest -> - workManager.enqueue(workRequest) - }, - onFailure = { - Timber.e(it, "Failed to build WorkManager request $workManagerRequest") - } - ) - } + workManagerRequest.build().fold( + onSuccess = { workRequests -> + workManager.enqueue(workRequests) + }, + onFailure = { + Timber.e(it, "Failed to build WorkManager request $workManagerRequest") + } + ) } override fun cancel(sessionId: SessionId) {