From 2fb391f118b19c5f3d142fd0ba00bb3a2302fb66 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 26 Oct 2023 10:22:48 +0200 Subject: [PATCH] fix: paginating fetch multiple users request [WPB-4999] (#2158) (#2165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: paginating fetch multiple users request [WPB-4999] * fix logs to print only sizes * fix imports * add tests Co-authored-by: Michał Saleniuk <30429749+saleniuk@users.noreply.github.com> Co-authored-by: Tommaso Piazza <196761+tmspzz@users.noreply.github.com> --- .../kalium/logic/data/user/UserRepository.kt | 45 ++++++++++++------ .../logic/data/user/UserRepositoryTest.kt | 47 +++++++++++++++++++ 2 files changed, 77 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 f1b6eb31fc6..a7df43d8086 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 @@ -51,6 +52,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 @@ -219,23 +221,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(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 -> + 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 { userDAO.insertOrIgnoreUsers(usersFailed.map { userMapper.fromFailedUserToEntity(it) }) @@ -489,5 +503,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 7019af4aafe..72b3babf23d 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 @@ -25,6 +25,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 @@ -201,6 +202,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