From 923c9674783f7e46d24ee223db5881fc618cce2a Mon Sep 17 00:00:00 2001 From: Mohamad Jaara Date: Tue, 24 Dec 2024 14:29:52 +0100 Subject: [PATCH 1/2] Revert "fix: do not commit User and Client upserts if nothing has changed [WPB-15055] (#3188)" This reverts commit 4a84b761b5a0c7078a50881b65a6ad21f47226d8. --- .../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, 57 insertions(+), 209 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 52784b53025..709b3613d56 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,28 +46,15 @@ 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 -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(); - +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 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 3f508f405da..385d13f2602 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,40 +40,21 @@ 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 -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(); +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; 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 38dc64c4042..f601dcc701c 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,42 +224,33 @@ 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 { - 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() + 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 + ) + } } } } @@ -349,21 +340,9 @@ class UserDAOImpl internal constructor( } } - // 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 { + private fun safeMarkAsDeletedAndRemoveFromGroupConversation(qualifiedID: QualifiedIDEntity) { + userQueries.markUserAsDeleted(qualifiedID, UserTypeEntity.NONE) 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 00008c8a317..b30b2a8ebec 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,18 +75,10 @@ internal class ClientDAOImpl internal constructor( * then any new value will be ignored. */ override suspend fun insertClient(client: InsertClientParam): Unit = withContext(queriesContext) { - 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() - } - } + insert(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) { + private fun insert(client: InsertClientParam) = with(client) { clientsQueries.insertClient( user_id = userId, id = id, @@ -100,16 +92,11 @@ 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 { - 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() - } + clients.forEach { client -> insert(client) } } } @@ -129,13 +116,8 @@ internal class ClientDAOImpl internal constructor( override suspend fun insertClientsAndRemoveRedundant(clients: List) = withContext(queriesContext) { clientsQueries.transaction { clients.groupBy { it.userId }.forEach { (userId, clientsList) -> - val anyInsertedOrModified = clientsList.map { client -> insert(client) }.any { it } + clientsList.forEach { client -> insert(client) } 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 273b1bcf82f..e47c92385bf 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,7 +25,6 @@ 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 @@ -980,48 +979,6 @@ 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 a9503cdf50b..7d352251f6e 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,50 +453,12 @@ class ClientDAOTest : BaseDatabaseTest() { clientDAO.insertClients(listOf(insertClientWithNonNullValues)) - // null values should be overwritten with proper ones + // null values should not 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 ec3101dbd0a..5484ed6a4de 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) // updated draft is being saved that should replace the old one + messageDraftDAO.upsertMessageDraft(updatedDraft) // the same exact draft is being saved again // Then val updatedValue = awaitItem() // other query should be notified From f64e30eb8fee88db8ffc8bcd59859c445cfb93f4 Mon Sep 17 00:00:00 2001 From: Mohamad Jaara Date: Tue, 24 Dec 2024 14:39:55 +0100 Subject: [PATCH 2/2] Trigger CI