From cd165d9bba62ee6ef3b1cd2a2e1923aef6173d55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= Date: Fri, 20 Oct 2023 14:08:51 +0200 Subject: [PATCH 1/4] fix: paginating fetch multiple users request [WPB-4999] --- .../kalium/logic/data/user/UserRepository.kt | 42 ++++++++++++------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt index 4314b3a6a7a..d770d301eac 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt @@ -209,23 +209,35 @@ internal class UserDataSource internal constructor( wrapApiRequest { userDetailsApi.getUserInfo(userId.toApi()) } .flatMap { userProfileDTO -> persistUsers(listOf(userProfileDTO)) } - override suspend fun fetchUsersByIds(qualifiedUserIdList: Set): Either { + @Suppress("MagicNumber") + override suspend fun fetchUsersByIds(qualifiedUserIdList: Set): Either = if (qualifiedUserIdList.isEmpty()) { - return Either.Right(Unit) - } - - return wrapApiRequest { - userDetailsApi.getMultipleUsers( - ListUserRequest.qualifiedIds(qualifiedUserIdList.map { userId -> userId.toApi() }) - ) - }.flatMap { listUserProfileDTO -> - if (listUserProfileDTO.usersFailed.isNotEmpty()) { - kaliumLogger.d("Handling ${listUserProfileDTO.usersFailed.size} failed users") - persistIncompleteUsers(listUserProfileDTO.usersFailed) - } - persistUsers(listUserProfileDTO.usersFound) + Either.Right(Unit) + } else { + qualifiedUserIdList + .chunked(500) + .foldToEitherWhileRight(ListUsersDTO(emptyList(), emptyList())) { chunk, acc -> + wrapApiRequest { + kaliumLogger.d("Fetching ${chunk.size} users") + userDetailsApi.getMultipleUsers( + ListUserRequest.qualifiedIds(chunk.map { userId -> userId.toApi() }) + ) + }.map { + kaliumLogger.d("Found ${it.usersFound} users and ${it.usersFailed} failed users") + acc.copy( + usersFound = (acc.usersFound + it.usersFound).distinct(), + usersFailed = (acc.usersFailed + it.usersFailed).distinct(), + ) + } + } + .flatMap { listUserProfileDTO -> + if (listUserProfileDTO.usersFailed.isNotEmpty()) { + kaliumLogger.d("Handling ${listUserProfileDTO.usersFailed.size} failed users") + persistIncompleteUsers(listUserProfileDTO.usersFailed) + } + persistUsers(listUserProfileDTO.usersFound) + } } - } private suspend fun persistIncompleteUsers(usersFailed: List) = wrapStorageRequest { usersFailed.map { userMapper.fromFailedUserToEntity(it) }.forEach { From fc20b9fe0fdd18ae18fac37ff738cdb1abb6b997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= Date: Fri, 20 Oct 2023 14:14:22 +0200 Subject: [PATCH 2/4] fix logs to print only sizes --- .../kotlin/com/wire/kalium/logic/data/user/UserRepository.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt index d770d301eac..12d49f5e633 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt @@ -223,7 +223,7 @@ internal class UserDataSource internal constructor( ListUserRequest.qualifiedIds(chunk.map { userId -> userId.toApi() }) ) }.map { - kaliumLogger.d("Found ${it.usersFound} users and ${it.usersFailed} failed users") + kaliumLogger.d("Found ${it.usersFound.size} users and ${it.usersFailed.size} failed users") acc.copy( usersFound = (acc.usersFound + it.usersFound).distinct(), usersFailed = (acc.usersFailed + it.usersFailed).distinct(), From b2a8c32f81f407b6a0d2bae5e7a4a5396d74e688 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= Date: Fri, 20 Oct 2023 14:31:32 +0200 Subject: [PATCH 3/4] fix imports --- .../kotlin/com/wire/kalium/logic/data/user/UserRepository.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt index 12d49f5e633..9068ae85137 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt @@ -44,6 +44,7 @@ import com.wire.kalium.logic.feature.SelfTeamIdProvider import com.wire.kalium.logic.functional.Either import com.wire.kalium.logic.functional.flatMap import com.wire.kalium.logic.functional.fold +import com.wire.kalium.logic.functional.foldToEitherWhileRight import com.wire.kalium.logic.functional.getOrNull import com.wire.kalium.logic.functional.map import com.wire.kalium.logic.functional.mapRight @@ -52,6 +53,7 @@ import com.wire.kalium.logic.wrapApiRequest import com.wire.kalium.logic.wrapStorageRequest import com.wire.kalium.network.api.base.authenticated.self.SelfApi import com.wire.kalium.network.api.base.authenticated.userDetails.ListUserRequest +import com.wire.kalium.network.api.base.authenticated.userDetails.ListUsersDTO import com.wire.kalium.network.api.base.authenticated.userDetails.UserDetailsApi import com.wire.kalium.network.api.base.authenticated.userDetails.qualifiedIds import com.wire.kalium.network.api.base.model.SelfUserDTO @@ -74,7 +76,6 @@ import kotlinx.coroutines.flow.flatMapMerge import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach import kotlinx.datetime.Instant -import kotlinx.serialization.decodeFromString import kotlinx.serialization.encodeToString import kotlinx.serialization.json.Json import kotlin.time.Duration.Companion.minutes From 84b56789910295f432ab69743b37184b6d76bc00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= Date: Fri, 20 Oct 2023 15:12:36 +0200 Subject: [PATCH 4/4] add tests --- .../kalium/logic/data/user/UserRepository.kt | 3 +- .../logic/data/user/UserRepositoryTest.kt | 48 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt index 9068ae85137..79ff9fa0d87 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt @@ -216,7 +216,7 @@ internal class UserDataSource internal constructor( Either.Right(Unit) } else { qualifiedUserIdList - .chunked(500) + .chunked(BATCH_SIZE) .foldToEitherWhileRight(ListUsersDTO(emptyList(), emptyList())) { chunk, acc -> wrapApiRequest { kaliumLogger.d("Fetching ${chunk.size} users") @@ -480,5 +480,6 @@ internal class UserDataSource internal constructor( companion object { internal const val SELF_USER_ID_KEY = "selfUserID" internal val FEDERATED_USER_TTL = 5.minutes + internal const val BATCH_SIZE = 500 } } diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/user/UserRepositoryTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/user/UserRepositoryTest.kt index ef8a5ace6c5..fbb3aaabf9f 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/user/UserRepositoryTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/user/UserRepositoryTest.kt @@ -24,6 +24,7 @@ import com.wire.kalium.logic.data.id.QualifiedIdMapper import com.wire.kalium.logic.data.id.toApi import com.wire.kalium.logic.data.id.toDao import com.wire.kalium.logic.data.session.SessionRepository +import com.wire.kalium.logic.data.user.UserDataSource.Companion.BATCH_SIZE import com.wire.kalium.logic.data.user.UserDataSource.Companion.SELF_USER_ID_KEY import com.wire.kalium.logic.failure.SelfUserDeleted import com.wire.kalium.logic.feature.SelfTeamIdProvider @@ -61,6 +62,7 @@ import io.mockative.given import io.mockative.matching import io.mockative.mock import io.mockative.once +import io.mockative.twice import io.mockative.verify import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.Flow @@ -216,6 +218,52 @@ class UserRepositoryTest { .wasInvoked(exactly = once) } + @Test + fun givenAUserIdListSmallerThanBatchSize_whenFetchingUsers_thenShouldExecuteRequestsOnce() = runTest { + // given + val requestedUserIds = buildSet { + repeat(BATCH_SIZE - 1) { add(UserId(value = "id$it", domain = "domain")) } + } + val (arrangement, userRepository) = Arrangement() + .withSuccessfulGetMultipleUsersApiRequest( + ListUsersDTO( + usersFailed = emptyList(), + usersFound = listOf(TestUser.USER_PROFILE_DTO) + ) + ) + .arrange() + // when + userRepository.fetchUsersByIds(requestedUserIds).shouldSucceed() + // then + verify(arrangement.userDetailsApi) + .suspendFunction(arrangement.userDetailsApi::getMultipleUsers) + .with(any()) + .wasInvoked(exactly = once) + } + + @Test + fun givenAUserIdListLargerThanBatchSize_whenFetchingUsers_thenShouldExecuteRequestsTwice() = runTest { + // given + val requestedUserIds = buildSet { + repeat(BATCH_SIZE + 1) { add(UserId(value = "id$it", domain = "domain")) } + } + val (arrangement, userRepository) = Arrangement() + .withSuccessfulGetMultipleUsersApiRequest( + ListUsersDTO( + usersFailed = emptyList(), + usersFound = listOf(TestUser.USER_PROFILE_DTO) + ) + ) + .arrange() + // when + userRepository.fetchUsersByIds(requestedUserIds).shouldSucceed() + // then + verify(arrangement.userDetailsApi) + .suspendFunction(arrangement.userDetailsApi::getMultipleUsers) + .with(any()) + .wasInvoked(exactly = twice) + } + @Test fun givenARemoteUserIsDeleted_whenFetchingSelfUser_thenShouldFailWithProperError() = runTest { // given