Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}
}
Expand Down Expand Up @@ -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>) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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()
}
}
}

Expand All @@ -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()
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down