From 76a9cd1e17b252c3fe58e3a662a17e6840f4edc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= Date: Fri, 20 Oct 2023 19:08:07 +0200 Subject: [PATCH] fix: paginating fetch multiple users request [WPB-4999] --- .../kalium/logic/data/user/UserRepository.kt | 36 ++++++++++---- .../logic/data/user/UserRepositoryTest.kt | 48 +++++++++++++++++++ 2 files changed, 75 insertions(+), 9 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 795e31fc05e..4fc700a074e 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 @@ -43,6 +43,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 @@ -53,6 +54,7 @@ import com.wire.kalium.network.api.base.authenticated.self.ChangeHandleRequest import com.wire.kalium.network.api.base.authenticated.self.SelfApi import com.wire.kalium.network.api.base.authenticated.self.UserUpdateRequest 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 @@ -198,9 +200,11 @@ internal class UserDataSource internal constructor( override suspend fun fetchUsersByIds(qualifiedUserIdList: Set): Either { val selfUserDomain = selfUserId.domain - qualifiedUserIdList.groupBy { it.domain } + return qualifiedUserIdList + .groupBy { it.domain } .filter { it.value.isNotEmpty() } - .map { (domain: String, usersOnDomain: List) -> + .entries + .foldToEitherWhileRight(Unit) { (domain: String, usersOnDomain: List), _ -> when (selfUserDomain == domain) { true -> fetchMultipleUsers(usersOnDomain) false -> { @@ -213,15 +217,28 @@ internal class UserDataSource internal constructor( } } } - - return Either.Right(Unit) } - private suspend fun fetchMultipleUsers(qualifiedUsersOnSameDomainList: List) = wrapApiRequest { - userDetailsApi.getMultipleUsers( - ListUserRequest.qualifiedIds(qualifiedUsersOnSameDomainList.map { userId -> userId.toApi() }) - ) - }.flatMap { listUserProfileDTO -> persistUsers(listUserProfileDTO.usersFound) } + private suspend fun fetchMultipleUsers(qualifiedUsersOnSameDomainList: List): Either = + qualifiedUsersOnSameDomainList + .chunked(BATCH_SIZE) + .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.size} users and ${it.usersFailed.size} failed users") + acc.copy( + usersFound = (acc.usersFound + it.usersFound).distinct(), + usersFailed = (acc.usersFailed + it.usersFailed).distinct(), + ) + } + } + .flatMap { listUserProfileDTO -> + persistUsers(listUserProfileDTO.usersFound) + } override suspend fun fetchUserInfo(userId: UserId) = wrapApiRequest { userDetailsApi.getUserInfo(userId.toApi()) } @@ -466,5 +483,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 d8a34d36fc7..c229c7b0866 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 @@ -20,6 +20,7 @@ package com.wire.kalium.logic.data.user import com.wire.kalium.logic.data.id.QualifiedIdMapper 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 @@ -52,6 +53,7 @@ import io.mockative.eq import io.mockative.given import io.mockative.mock import io.mockative.once +import io.mockative.twice import io.mockative.verify import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.channels.Channel @@ -211,6 +213,52 @@ class UserRepositoryTest { .wasNotInvoked() } + @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