fix: do not commit User and Client upserts if nothing has changed [WP…

…B-15055] (#3188)

* fix: do not commit User and Client upserts if nothing has changed [WPB-15055]

* fix unused parameter

* fix detekt

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;

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)

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<UserEntity>) = 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<UserIDEntity>) {

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<InsertClientParam>) = 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<InsertClientParam>) = 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()
+                }
             }
         }
     }

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"))

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)

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