fix: paginating fetch multiple users request [WPB-4999] (#2158) (#2165)

* fix: paginating fetch multiple users request [WPB-4999] * fix logs to print only sizes * fix imports * add tests Co-authored-by: Michał Saleniuk <[email protected]> Co-authored-by: Tommaso Piazza <[email protected]>
wireapp · Oct 26, 2023 · 2fb391f · 2fb391f
1 parent b8dfcb5
commit 2fb391f
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 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
@@ -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<UserId>): Either<CoreFailure, Unit> {
+    @Suppress("MagicNumber")
+    override suspend fun fetchUsersByIds(qualifiedUserIdList: Set<UserId>): Either<CoreFailure, Unit> =
         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<NetworkQualifiedId>) = 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
@@ -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