From 4a84b761b5a0c7078a50881b65a6ad21f47226d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= <30429749+saleniuk@users.noreply.github.com> Date: Mon, 23 Dec 2024 14:14:15 +0100 Subject: [PATCH] fix: do not commit User and Client upserts if nothing has changed [WPB-15055] (#3188) * fix: do not commit User and Client upserts if nothing has changed [WPB-15055] * fix unused parameter * fix detekt --- .../com/wire/kalium/persistence/Clients.sq | 31 +++++--- .../com/wire/kalium/persistence/Users.sq | 49 ++++++++---- .../kalium/persistence/dao/UserDAOImpl.kt | 75 ++++++++++++------- .../persistence/dao/client/ClientDAOImpl.kt | 26 ++++++- .../kalium/persistence/dao/UserDAOTest.kt | 43 +++++++++++ .../persistence/dao/client/ClientDAOTest.kt | 40 +++++++++- .../dao/message/draft/MessageDraftDAOTest.kt | 2 +- 7 files changed, 209 insertions(+), 57 deletions(-) diff --git a/persistence/src/commonMain/db_user/com/wire/kalium/persistence/Clients.sq b/persistence/src/commonMain/db_user/com/wire/kalium/persistence/Clients.sq index 709b3613d56..52784b53025 100644 --- a/persistence/src/commonMain/db_user/com/wire/kalium/persistence/Clients.sq +++ b/persistence/src/commonMain/db_user/com/wire/kalium/persistence/Clients.sq @@ -46,15 +46,28 @@ insertClient: INSERT INTO Client(user_id, id, device_type, client_type, is_valid, registration_date, label, model, last_active, mls_public_keys, is_mls_capable) VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(id, user_id) DO UPDATE SET -device_type = coalesce(excluded.device_type, device_type), -client_type = coalesce(excluded.client_type, client_type), -registration_date = coalesce(excluded.registration_date, registration_date), -label = coalesce(excluded.label, label), -model = coalesce(excluded.model, model), -is_valid = is_valid, -last_active = coalesce(excluded.last_active, last_active), -mls_public_keys = excluded.mls_public_keys, -is_mls_capable = excluded.is_mls_capable OR is_mls_capable; -- it's not possible to remove mls capability once added + device_type = coalesce(excluded.device_type, device_type), + client_type = coalesce(excluded.client_type, client_type), + registration_date = coalesce(excluded.registration_date, registration_date), + label = coalesce(excluded.label, label), + model = coalesce(excluded.model, model), + is_valid = is_valid, + last_active = coalesce(excluded.last_active, last_active), + mls_public_keys = excluded.mls_public_keys, + is_mls_capable = excluded.is_mls_capable OR is_mls_capable -- it's not possible to remove mls capability once added +WHERE -- execute the update only if any of the fields changed + Client.device_type IS NOT coalesce(excluded.device_type, Client.device_type) + OR Client.client_type IS NOT coalesce(excluded.client_type, Client.client_type) + OR Client.registration_date IS NOT coalesce(excluded.registration_date, Client.registration_date) + OR Client.label IS NOT coalesce(excluded.label, Client.label) + OR Client.model IS NOT coalesce(excluded.model, Client.model) + OR Client.last_active IS NOT coalesce(excluded.last_active, Client.last_active) + OR Client.mls_public_keys IS NOT excluded.mls_public_keys + OR Client.is_mls_capable IS NOT (excluded.is_mls_capable OR Client.is_mls_capable); + +selectChanges: +SELECT changes(); + selectAllClients: SELECT * FROM Client; diff --git a/persistence/src/commonMain/db_user/com/wire/kalium/persistence/Users.sq b/persistence/src/commonMain/db_user/com/wire/kalium/persistence/Users.sq index 385d13f2602..3f508f405da 100644 --- a/persistence/src/commonMain/db_user/com/wire/kalium/persistence/Users.sq +++ b/persistence/src/commonMain/db_user/com/wire/kalium/persistence/Users.sq @@ -40,21 +40,40 @@ insertUser: INSERT INTO User(qualified_id, name, handle, email, phone, accent_id, team, connection_status, preview_asset_id, complete_asset_id, user_type, bot_service, deleted, incomplete_metadata, expires_at, supported_protocols, active_one_on_one_conversation_id) VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(qualified_id) DO UPDATE SET -name = excluded.name, -handle = excluded.handle, -email = excluded.email, -phone = excluded.phone, -accent_id = excluded.accent_id, -team = excluded.team, -preview_asset_id = excluded.preview_asset_id, -complete_asset_id = excluded.complete_asset_id, -user_type = excluded.user_type, -bot_service = excluded.bot_service, -deleted = excluded.deleted, -incomplete_metadata = excluded.incomplete_metadata, -expires_at = excluded.expires_at, -defederated = 0, -supported_protocols = excluded.supported_protocols; + name = excluded.name, + handle = excluded.handle, + email = excluded.email, + phone = excluded.phone, + accent_id = excluded.accent_id, + team = excluded.team, + preview_asset_id = excluded.preview_asset_id, + complete_asset_id = excluded.complete_asset_id, + user_type = excluded.user_type, + bot_service = excluded.bot_service, + deleted = excluded.deleted, + incomplete_metadata = excluded.incomplete_metadata, + expires_at = excluded.expires_at, + defederated = 0, + supported_protocols = excluded.supported_protocols +WHERE -- execute the update only if any of the fields changed + User.name != excluded.name + OR User.handle != excluded.handle + OR User.email != excluded.email + OR User.phone != excluded.phone + OR User.accent_id != excluded.accent_id + OR User.team != excluded.team + OR User.preview_asset_id != excluded.preview_asset_id + OR User.complete_asset_id != excluded.complete_asset_id + OR User.user_type != excluded.user_type + OR User.bot_service != excluded.bot_service + OR User.deleted != excluded.deleted + OR User.incomplete_metadata != excluded.incomplete_metadata + OR User.expires_at != excluded.expires_at + OR User.defederated != 0 + OR User.supported_protocols != excluded.supported_protocols; + +selectChanges: +SELECT changes(); insertOrIgnoreUser: INSERT OR IGNORE INTO User(qualified_id, name, handle, email, phone, accent_id, team, connection_status, preview_asset_id, complete_asset_id, user_type, bot_service, deleted, incomplete_metadata, expires_at, supported_protocols) diff --git a/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/UserDAOImpl.kt b/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/UserDAOImpl.kt index f601dcc701c..38dc64c4042 100644 --- a/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/UserDAOImpl.kt +++ b/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/UserDAOImpl.kt @@ -224,33 +224,42 @@ class UserDAOImpl internal constructor( } } + private fun insertUser(user: UserEntity): Boolean { + userQueries.insertUser( + qualified_id = user.id, + name = user.name, + handle = user.handle, + email = user.email, + phone = user.phone, + accent_id = user.accentId, + team = user.team, + preview_asset_id = user.previewAssetId, + complete_asset_id = user.completeAssetId, + user_type = user.userType, + bot_service = user.botService, + incomplete_metadata = user.hasIncompleteMetadata, + expires_at = user.expiresAt, + connection_status = user.connectionStatus, + deleted = user.deleted, + supported_protocols = user.supportedProtocols, + active_one_on_one_conversation_id = user.activeOneOnOneConversationId + ) + return userQueries.selectChanges().executeAsOne() > 0 + } + override suspend fun upsertUsers(users: List) = withContext(queriesContext) { userQueries.transaction { - for (user: UserEntity in users) { - if (user.deleted) { - // mark as deleted and remove from groups - safeMarkAsDeletedAndRemoveFromGroupConversation(user.id) - } else { - userQueries.insertUser( - qualified_id = user.id, - name = user.name, - handle = user.handle, - email = user.email, - phone = user.phone, - accent_id = user.accentId, - team = user.team, - preview_asset_id = user.previewAssetId, - complete_asset_id = user.completeAssetId, - user_type = user.userType, - bot_service = user.botService, - incomplete_metadata = user.hasIncompleteMetadata, - expires_at = user.expiresAt, - connection_status = user.connectionStatus, - deleted = user.deleted, - supported_protocols = user.supportedProtocols, - active_one_on_one_conversation_id = user.activeOneOnOneConversationId - ) - } + val anyInsertedOrModified = users.map { user -> + if (user.deleted) { + // mark as deleted and remove from groups + safeMarkAsDeletedAndRemoveFromGroupConversation(user.id) + } else { + insertUser(user) + } + }.any { it } + if (!anyInsertedOrModified) { + // rollback the transaction if no changes were made so that it doesn't notify other queries if not needed + this.rollback() } } } @@ -340,9 +349,21 @@ class UserDAOImpl internal constructor( } } - private fun safeMarkAsDeletedAndRemoveFromGroupConversation(qualifiedID: QualifiedIDEntity) { - userQueries.markUserAsDeleted(qualifiedID, UserTypeEntity.NONE) + // returns true if any row has been inserted or modified, false if exactly the same data already exists + private fun markUserAsDeleted(qualifiedID: QualifiedIDEntity, userType: UserTypeEntity): Boolean { + userQueries.markUserAsDeleted(qualifiedID, userType) + return userQueries.selectChanges().executeAsOne() > 0 + } + + // returns true if any row has been inserted or modified, false if exactly the same data already exists + private fun deleteUserFromGroupConversations(qualifiedID: QualifiedIDEntity): Boolean { userQueries.deleteUserFromGroupConversations(qualifiedID) + return userQueries.selectChanges().executeAsOne() > 0 + } + + // returns true if any row has been inserted or modified, false if exactly the same data already exists + private fun safeMarkAsDeletedAndRemoveFromGroupConversation(qualifiedID: QualifiedIDEntity): Boolean { + return markUserAsDeleted(qualifiedID, UserTypeEntity.NONE) or deleteUserFromGroupConversations(qualifiedID) } override suspend fun markAsDeleted(userId: List) { diff --git a/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/client/ClientDAOImpl.kt b/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/client/ClientDAOImpl.kt index b30b2a8ebec..00008c8a317 100644 --- a/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/client/ClientDAOImpl.kt +++ b/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/client/ClientDAOImpl.kt @@ -75,10 +75,18 @@ internal class ClientDAOImpl internal constructor( * then any new value will be ignored. */ override suspend fun insertClient(client: InsertClientParam): Unit = withContext(queriesContext) { - insert(client) + clientsQueries.transaction { + insert(client) + val changes = clientsQueries.selectChanges().executeAsOne() + if (changes == 0L) { + // rollback the transaction if no changes were made so that it doesn't notify other queries if not needed + this.rollback() + } + } } - private fun insert(client: InsertClientParam) = with(client) { + // returns true if any row has been inserted or modified, false if exactly the same data already exists + private fun insert(client: InsertClientParam): Boolean = with(client) { clientsQueries.insertClient( user_id = userId, id = id, @@ -92,11 +100,16 @@ internal class ClientDAOImpl internal constructor( label = label, mls_public_keys = mlsPublicKeys ) + clientsQueries.selectChanges().executeAsOne() > 0 } override suspend fun insertClients(clients: List) = withContext(queriesContext) { clientsQueries.transaction { - clients.forEach { client -> insert(client) } + val anyInsertedOrModified = clients.map { client -> insert(client) }.any { it } + if (!anyInsertedOrModified) { + // rollback the transaction if no changes were made so that it doesn't notify other queries if not needed + this.rollback() + } } } @@ -116,8 +129,13 @@ internal class ClientDAOImpl internal constructor( override suspend fun insertClientsAndRemoveRedundant(clients: List) = withContext(queriesContext) { clientsQueries.transaction { clients.groupBy { it.userId }.forEach { (userId, clientsList) -> - clientsList.forEach { client -> insert(client) } + val anyInsertedOrModified = clientsList.map { client -> insert(client) }.any { it } clientsQueries.deleteClientsOfUserExcept(userId, clientsList.map { it.id }) + val anyDeleted = clientsQueries.selectChanges().executeAsOne() > 0 + if (!anyInsertedOrModified && !anyDeleted) { + // rollback the transaction if no changes were made so that it doesn't notify other queries if not needed + this.rollback() + } } } } diff --git a/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/UserDAOTest.kt b/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/UserDAOTest.kt index e47c92385bf..273b1bcf82f 100644 --- a/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/UserDAOTest.kt +++ b/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/UserDAOTest.kt @@ -25,6 +25,7 @@ import com.wire.kalium.persistence.dao.member.MemberEntity import com.wire.kalium.persistence.db.UserDatabaseBuilder import com.wire.kalium.persistence.utils.stubs.TestStubs import com.wire.kalium.persistence.utils.stubs.newConversationEntity +import com.wire.kalium.persistence.utils.stubs.newUserDetailsEntity import com.wire.kalium.persistence.utils.stubs.newUserEntity import com.wire.kalium.util.DateTimeUtil import kotlinx.coroutines.flow.first @@ -979,6 +980,48 @@ class UserDAOTest : BaseDatabaseTest() { assertFalse { db.userDAO.isAtLeastOneUserATeamMember(users.map { it.id }, teamId) } } + @Test + fun givenPersistedUser_whenUpsertingTheSameExactUser_thenItShouldIgnoreAndNotNotifyOtherQueries() = runTest(dispatcher) { + // Given + val user = newUserEntity() + val userDetails = newUserDetailsEntity() + db.userDAO.upsertUser(user) + val updatedUser = user.copy(name = "new_name") + + db.userDAO.observeUserDetailsByQualifiedID(user.id).test { + val initialValue = awaitItem() + assertEquals(userDetails, initialValue) + + // When + db.userDAO.upsertUser(updatedUser) // the same exact user is being saved again + + // Then + expectNoEvents() // other query should not be notified + } + } + + @Test + fun givenPersistedUser_whenUpsertingUpdatedUser_thenItShouldBeSavedAndOtherQueriesShouldBeUpdated() = runTest(dispatcher) { + // Given + val user = newUserEntity() + val userDetails = newUserDetailsEntity() + db.userDAO.upsertUser(user) + val updatedUser = user.copy(name = "new_name") + val updatedUserDetails = userDetails.copy(name = "new_name") + + db.userDAO.observeUserDetailsByQualifiedID(user.id).test { + val initialValue = awaitItem() + assertEquals(userDetails, initialValue) + + // When + db.userDAO.upsertUser(updatedUser) // updated user is being saved + + // Then + val updatedValue = awaitItem() // other query should be notified + assertEquals(updatedUserDetails, updatedValue) + } + } + private companion object { val USER_ENTITY_1 = newUserEntity(QualifiedIDEntity("1", "wire.com")) val USER_ENTITY_2 = newUserEntity(QualifiedIDEntity("2", "wire.com")) diff --git a/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/client/ClientDAOTest.kt b/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/client/ClientDAOTest.kt index 7d352251f6e..a9503cdf50b 100644 --- a/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/client/ClientDAOTest.kt +++ b/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/client/ClientDAOTest.kt @@ -453,12 +453,50 @@ class ClientDAOTest : BaseDatabaseTest() { clientDAO.insertClients(listOf(insertClientWithNonNullValues)) - // null values should not be overwritten with proper ones + // null values should be overwritten with proper ones clientDAO.getClientsOfUserByQualifiedIDFlow(userId).first().also { resultList -> assertEquals(listOf(clientWithNonNullValues), resultList) } } + @Test + fun givenPersistedClient_whenUpsertingTheSameExactClient_thenItShouldIgnoreAndNotNotifyOtherQueries() = runTest { + // Given + userDAO.upsertUser(user) + clientDAO.insertClient(insertedClient) + + clientDAO.observeClient(user.id, insertedClient.id).test { + val initialValue = awaitItem() + assertEquals(insertedClient.toClient(), initialValue) + + // When + clientDAO.insertClient(insertedClient) // the same exact client is being saved again + + // Then + expectNoEvents() // other query should not be notified + } + } + + @Test + fun givenPersistedClient_whenUpsertingUpdatedClient_thenItShouldBeSavedAndOtherQueriesShouldBeUpdated() = runTest { + // Given + userDAO.upsertUser(user) + clientDAO.insertClient(insertedClient) + val updatedInsertedClient = insertedClient.copy(label = "new_label") + + clientDAO.observeClient(user.id, insertedClient.id).test { + val initialValue = awaitItem() + assertEquals(insertedClient.toClient(), initialValue) + + // When + clientDAO.insertClient(updatedInsertedClient) // updated client is being saved that should replace the old one + + // Then + val updatedValue = awaitItem() // other query should be notified + assertEquals(updatedInsertedClient.toClient(), updatedValue) + } + } + private companion object { val userId = QualifiedIDEntity("test", "domain") val user = newUserEntity(userId) diff --git a/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/message/draft/MessageDraftDAOTest.kt b/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/message/draft/MessageDraftDAOTest.kt index 5484ed6a4de..ec3101dbd0a 100644 --- a/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/message/draft/MessageDraftDAOTest.kt +++ b/persistence/src/commonTest/kotlin/com/wire/kalium/persistence/dao/message/draft/MessageDraftDAOTest.kt @@ -282,7 +282,7 @@ class MessageDraftDAOTest : BaseDatabaseTest() { assertEquals(listOf(draft), initialValue) // When - messageDraftDAO.upsertMessageDraft(updatedDraft) // the same exact draft is being saved again + messageDraftDAO.upsertMessageDraft(updatedDraft) // updated draft is being saved that should replace the old one // Then val updatedValue = awaitItem() // other query should be notified