refactor: only have one main query for inserting/updating users #2124

typfel · 2023-10-09T23:01:47Z

PR Submission Checklist for internal contributors

The PR Title
- conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
- contains a reference JIRA issue number like SQPIT-764
- answers the question: If merged, this PR will: ... ³
The PR Description
- is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

We have several queries for updating/inserting users, which increases the risk for bugs when introducing a new field since it's easy to forget to to add the new field to all queries.

Solutions

Have one main "upsert" query for inserting and updating the common fields of a user.

Any updates to special fields which doesn't come directly from the UserDTO are updated via special queries with a limited scope. This avoids the dangerous pattern creating a UserEntity with mostly null values in order to update some specific field, passing such a UserEntity to the wrong query would delete data. See: UserEventReceiver, TeamRepository.

Testing

I have added automated test to this contribution

PR Post Submission Checklist for internal contributors (Optional)

Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References

https://sparkbox.com/foundry/semantic_commit_messages
https://github.com/wireapp/.github#usage
E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

github-actions · 2023-10-09T23:16:54Z

Unit Test Results

  416 files ±0   416 suites ±0 20s ⏱️ +4s
2 340 tests - 1 2 262 ✔️ - 1 78 💤 ±0 0 ❌ ±0

Results for commit 140c8c6. ± Comparison against base commit 9e46a61.

♻️ This comment has been updated with latest results.

datadog-wireapp · 2023-10-09T23:30:33Z

Datadog Report

All test runs 961a0fa 🔗

❌ 2 Total Test Services: 1 Failed, 0 with New Flaky, 1 Passed

Test Services

Service Name	Failed	Known Flaky	New Flaky	Passed	Skipped	Wall Time	Branch View
`kalium-jvm`	1	1	0	612	39	10m 36.55s	Link
`kalium-ios`	0	0	0	2262	78	6m 16.13s	Link

❌ Failed Tests (1)

givenEmailAndPasswordWhenLoggingInThenRegisterClientAndLogout - PocIntegrationTest - ❄️ Known flaky - Details

Expand for error

 java.lang.AssertionError: no expected response was found for POST:https://test.api.com/v1/access/logout
 
 java.lang.AssertionError: no expected response was found for POST:https://test.api.com/v1/access/logout
 	at org.junit.Assert.fail(Assert.java:89)
 	at kotlin.test.junit.JUnitAsserter.fail(JUnitSupport.kt:56)
 	at kotlin.test.AssertionsKt__AssertionsKt.fail(Assertions.kt:520)
 	at kotlin.test.AssertionsKt.fail(Unknown Source)
 	at util.MockUnboundNetworkClient$createMockEngine$1.invokeSuspend(MockUnboundNetworkClient.kt:88)
 	at util.MockUnboundNetworkClient$createMockEngine$1.invoke(MockUnboundNetworkClient.kt)
 	at util.MockUnboundNetworkClient$createMockEngine$1.invoke(MockUnboundNetworkClient.kt)
 ...

codecov-commenter · 2023-10-09T23:34:08Z

Codecov Report

❗ No coverage uploaded for pull request base (epic/mls-one2one@8c3cd30). Click here to learn what that means.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##             epic/mls-one2one    #2124   +/-   ##
===================================================
  Coverage                    ?   57.85%           
  Complexity                  ?       24           
===================================================
  Files                       ?     1045           
  Lines                       ?    39484           
  Branches                    ?     3670           
===================================================
  Hits                        ?    22843           
  Misses                      ?    15065           
  Partials                    ?     1576

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c3cd30...e58cf83. Read the comment docs.

typfel · 2023-10-10T09:30:24Z

persistence/src/commonMain/db_user/com/wire/kalium/persistence/Users.sq

 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,
-active_one_on_one_conversation_id = excluded.active_one_on_one_conversation_id;


This fixes the issue with blocked conversations disappearing.

On when we change the connection status we upsert the user which was resetting the active_one_on_one_conversation_id to null.

typfel · 2023-10-10T09:31:10Z

persistence/src/commonMain/db_user/com/wire/kalium/persistence/Users.sq

 preview_asset_id = excluded.preview_asset_id,
 complete_asset_id = excluded.complete_asset_id,
-user_type = excluded.user_type,


It's only relevant to update this field for team users and then it should be done with upsertTeamMemberUserType query.

Ok, turns out this is not only relevant for team users and thus I've reverted this and now it's instead mandatory to provide a correct userType when upserting a user. We weren't providing a correct userType when syncing team users in the UserRepository, which I've fixed for now by fetching it from the local db.

cc @vitorhugods @yamilmedina @mchenani

typfel · 2023-10-10T09:32:24Z

persistence/src/commonMain/db_user/com/wire/kalium/persistence/Users.sq

@@ -46,36 +46,39 @@ email = excluded.email,
 phone = excluded.phone,
 accent_id = excluded.accent_id,
 team = excluded.team,
-connection_status = excluded.connection_status,


When updating the connection status we should be using the upsertUserConnectionStatus query.

vitorhugods

Nicely done. Just a small suggestion

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/team/TeamRepository.kt

persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/UserDAOImpl.kt

yamilmedina

Amazing work! A huge debt we had is being paid. This brings much more clean and less orchestration from the caller side. 👏

typfel · 2023-10-10T14:59:45Z

@vitorhugods @yamilmedina I had do some additional refactoring to fix the ConnectionRepository since it was relying on updating the connection status via an upsertUser and not via the upsertUserConnectionStatus query. Also generally now the ConnectionRepository is no longer responsible for fetching user profiles that's now done higher up, this has performance impacts during slow sync where were fetching user profiles many times over.

Went down the rabbit hole of only calling upsertUserConnectionStatus from only one location. Increased the number of lines changed but I think the code is a lot easier to follow now.

* refactor: only have one main query for inserting/updating users * fix: incorrect user mapping for team member * chore: inline function which is only used once * fix: updating connection status * fix: user type not updating for non-team contacts * fix: typo in partial user update query * chore: update tests after rebase

* feat(mls-migration): fetch migration configuration every 24 hours #1 (#1728) * feat(mls-migration): migrate proteus conversations to "mixed" protocol #2 (#1729) * feat: add api support for updating the conversation protocol * fix: rollback when we lost the race to establish a MLS group * feat: add method for migrating from proteus to mixed * feat: allow controlling migration update interval in CLI * fix: use correct use case for checking if we can register an MLS client * chore: add debug response since BE is not implemented yet * test: add test for aborting when losing the race to establish an mls group * test: add network test for updating protocol * test: add MLSMigrationManager tests * test: add MLSMigrator tests * chore: fix detekt * chore: make method private * refactor: add sql query for fetching proteus team conversations * feat(mls-migration): support the mixed protocol #3 (#1747) * feat(mls-migration): supported protocols for users #4 (#1786) * feat: add API support for supported-protocols * feat: support persisting supported protocols * feat: add mapping of supported protocols in logic module * chore: fix detekt * refactor: rename UserProtocol to SupportedProtocolDTO * chore: move BaseApi interface to its own file * chore: fix imports after rebase * fix: persist supported protocols locally when API request succeeds * chore: remove debug line * fix: mapsupported protocols when fetching one-to-one conversationd details * refactor: move default value for supportedProtocols to the logic mapping layer * feat(mls-migration): add use case for updating self supported protocols #5 (#1797) * feat: add use case for calculating & updating your supported protocols * test: add UpdateSupportedProtocolsUseCase tests * refactor: simplify UpdateSupportedProtocolsUseCase * test: update feature config test data * chore: add mlsPublicKeys to NewClientDTO * feat: persist if a self client is mls capable or not * feat: update supported protocols from user.update events * fix: migration (altering column positions not supported) * chore: clean up disabled migration configuration * feat(mls-migration): update conversation protocol to MLS when every member supports MLS #6 (#1802) * feat: support fetching conversation which can be finalised * feat: add function for finalising migration of team group conversations * refactor: move methods for updating conversation protocol to the conversation repository * chore: fix copy & paste error * refactor: don't return a flow when fetching conversations for migration * chore: rebase clean up * performance: replace string matching by listing all cases * feat(mls-migration): handle protocol changed events #7 (#1811) * feat: support persisting protocol changed system messages * feat: process protocol change events and insert system messages * feat: insert history lost system message if protocol change is discovered during slow sync * chore: add migration of new system message table * fix: check if protocol was updated inside sql query * fix: only fetch group conversations by protocol * chore: document return value * chore: fix test naming * refactor: rename SystemMessageBuilder to SystemMessageInserter * chore: fix logging inconsistency * fix: import * chore: fix compilation * fix(mls-migration): migration feature config #8 (#1822) * fix: mls migration config fields are all optional * chore: update tests after removing no longer necessary fields * chore: fix detekt * feat(mls-migration): update self supported protocols during slow sync (#1826) #9 * feat(mls-migration): force migration when migration deadline arrives #10 (#1831) * feat: end migration regardless when migration deadline arrives * refactor: generalise methods for fetching conversations ids * refactor: better naming * feat(mls-migration): ignore inactive clients when calculating supported protocols #11 (#1904) * test: add test cases for ignoring inactive clients when calculating supported protocols * feat: add isActive computed property on Client model * feat: ignore inactive clients when checking if all self clients are mls capable * fix: updating isMLSCapable (#1905) * fix: updating isMLSCapable It should be possible to update isMlsCapable from false to true but not the other way around. * chore: add comment explaining update logic for is_mls_capable * chore: fix detekt * fix: fetch group id on protocol change event (#2051) * chore: fix rebase issues * feat(mls-one2one): support fetching mls 1-1 conversation details (#1848) * feat: support fetching mls 1-1 conversation details * refactor: better naming of function * chore: remove another magic number * feat: support migrating to mls 1-1 (#1875) * refactor: allow fetching whole conversations by a member (#1940) * feat(mls): establish one-to-one conversation [WPB-2258] (#1953) * feat(mls): migrate 1:1 connections from Proteus to MLS [WPB-2258] (#1968) * feat: choose 1:1 protocol [WPB-2181] (#1993) Co-authored-by: Mojtaba Chenani <[email protected]> * feat: check supported protocols when creating/fetching team 1-1 (#1995) * feat: establish mls 1-1 for team members if mls is the default protocol * refactor: update GetOrCreateOneToOneConversationUseCaseTest to use the arrangement pattern * test: update with test cases covering mls as the default protocol * feat: support persisting the default protocol from mls feature config * test: update & add tests for persisting the default protocol * chore: EstablishMLSOneToOneUseCase was renamed into MLSOneOnOneConversationResolver * test: don't expect team 1-1 to be established * fix: use protocol selector for deciding which protocol use for team 1-1 * chore: fix detekt * feat: active one-on-one conversations (#2010) * feat(persistance): active/inactive one-on-one conversations * fix: more efficient mapping Co-authored-by: Mojtaba Chenani <[email protected]> * refactor: join with Conversation table instead Co-authored-by: Mojtaba Chenani <[email protected]> * refactor: rename oneOnOneConversationId to activeOneOnOneconversationId Co-authored-by: Vitor Hugo Schwaab <[email protected]> --------- Co-authored-by: Mojtaba Chenani <[email protected]> Co-authored-by: Vitor Hugo Schwaab <[email protected]> * feat: resolve one-on-one conversation to either proteus or mls (#2012) * chore: default selfUser when creator field is missing (#2022) * feat: resolve one-on-one conversaions when receiving a welcome message (#2033) * feat: resolve one-on-on conversation when the connection is accepted (#2034) * feat: resolve active 1-1 after accepting a connection request (#2042) * feat: support parsing quantum safe ciphersuite (#2049) * feat: avoid race condition when establishing mls 1-1 (#2048) * feat: add live property to Event model Distinguish between live events arriving via the websocket vs events fetched when catching up when connectivity is restored. * feat: delay resolving active one-on-one when live Delay resolving active one-on-one when a connection request is accepted and we are live. This avoids a race to establish the mls group when multiple clients are online, which is wasteful. * chore: update tests after adding live propperty * chore: fix detekt * fix: always schedule resolving active 1-1 to avoid discarding welcome msg * fix: improve stability of MLS 1-1 conversations (#2063) * fix: don't fail migration query if messages has already been copied * fix: don't fail the slow sync on a non-recoverable error when resolving 1-1s * fix: don't fail the slow sync when etablishing 1-1 fails due to missing key packages * fix: re-use existing mls group if it exists * fix: establish 1-1 also with other self clients * test: add missing test for establishing 1-1 * chore: fix detekt * feat: limit re-trying commit to 2 retry attemps (#2068) * feat: limit re-trying commit to 2 retry attemps * refactor: rename retryCount to remainingAttempts for better readability * chore: fix rebase issues * fix: map and ignore BufferedFutureMessage (#2084) * feat: update supported protocols after deleting a client (#2083) * feat(mls): recover from stale epoch on message sending (#2076) * feat: parse epoch timestamp in conversation response * feat: persist epoch timestamp * feat: verify if we have lost commits when sending fails * test: add tests for stale epoch handler * fix: don't compare against last processed event instead use time heuristic * fix: verify epoch using CC in WrongEpochHandler * refactor: replace MLSWrongEpochHandler with StaleEpochVerifier since they are identical * fix: fail re-try if verifying epoch fails * Revert "feat: persist epoch timestamp" This reverts commit e968af6. * Revert "feat: parse epoch timestamp in conversation response" This reverts commit f1bf8b4. * chore: remove any remaining trace of epochTimestamp * chore: fix rebase issues * feat(debug): add use case for disabling event processing (#2096) * chore: fix rebase issues * refactor: mls feature config handling (#2114) * refactor: mls feature config handling * chore: remove debug println * chore: use named arguments for better readability * refactor: extract handlers into variables and re-use them * fix: assign supported_protocols when migration user db (#2126) * chore(mls): propagate activeOneOnOneConversation on ConversationDetails [WPB-4705] (#2130) * refactor: only have one main query for inserting/updating users (#2124) * refactor: only have one main query for inserting/updating users * fix: incorrect user mapping for team member * chore: inline function which is only used once * fix: updating connection status * fix: user type not updating for non-team contacts * fix: typo in partial user update query * chore: update tests after rebase * fix: supported protocols is only available since API v5 (#2128) * fix: skip updating supported protocols when mls is not supported (#2131) * chore: fix rebase issues * chore: squash migrations when possible --------- Co-authored-by: Vitor Hugo Schwaab <[email protected]> Co-authored-by: Mojtaba Chenani <[email protected]>

github-actions bot assigned typfel Oct 9, 2023

github-actions bot added 🚨 Potential breaking changes 👕 size: XL type: refactoring 🛠 labels Oct 9, 2023

typfel requested review from yamilmedina, vitorhugods and mchenani October 10, 2023 08:38

typfel force-pushed the epic/mls-one2one branch from fc867fa to fec8f05 Compare October 10, 2023 08:49

typfel force-pushed the refactor/user-dao branch from 723dd25 to 030a715 Compare October 10, 2023 08:50

typfel commented Oct 10, 2023

View reviewed changes

vitorhugods approved these changes Oct 10, 2023

View reviewed changes

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/team/TeamRepository.kt Outdated Show resolved Hide resolved

persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/UserDAOImpl.kt Outdated Show resolved Hide resolved

yamilmedina approved these changes Oct 10, 2023

View reviewed changes

github-actions bot added 👕 size: XXL and removed 👕 size: XL labels Oct 10, 2023

typfel force-pushed the refactor/user-dao branch from fef4052 to e2dc6dd Compare October 10, 2023 15:21

typfel force-pushed the epic/mls-one2one branch from 5d47b90 to 8c3cd30 Compare October 11, 2023 09:32

typfel force-pushed the refactor/user-dao branch 2 times, most recently from 29e1c0c to 3244972 Compare October 13, 2023 05:53

typfel added 6 commits October 13, 2023 10:44

refactor: only have one main query for inserting/updating users

38adf98

fix: incorrect user mapping for team member

6ee6c73

chore: inline function which is only used once

85be5ec

fix: updating connection status

9b0c349

fix: user type not updating for non-team contacts

4aae1fa

fix: typo in partial user update query

567c58e

chore: update tests after rebase

140c8c6

typfel force-pushed the refactor/user-dao branch from 3244972 to 140c8c6 Compare October 13, 2023 08:46

typfel merged commit 438ec5b into epic/mls-one2one Oct 13, 2023

typfel deleted the refactor/user-dao branch October 13, 2023 09:21

typfel mentioned this pull request Oct 15, 2023

feat: mls (epic) #2015

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: only have one main query for inserting/updating users #2124

refactor: only have one main query for inserting/updating users #2124

typfel commented Oct 9, 2023

github-actions bot commented Oct 9, 2023 •

edited

Loading

datadog-wireapp bot commented Oct 9, 2023 •

edited

Loading

codecov-commenter commented Oct 9, 2023 •

edited

Loading

typfel Oct 10, 2023

typfel Oct 10, 2023 •

edited

Loading

typfel Oct 11, 2023 •

edited

Loading

typfel Oct 11, 2023

typfel Oct 10, 2023

vitorhugods left a comment

yamilmedina left a comment

typfel commented Oct 10, 2023

refactor: only have one main query for inserting/updating users #2124

refactor: only have one main query for inserting/updating users #2124

Conversation

typfel commented Oct 9, 2023

PR Submission Checklist for internal contributors

What's new in this PR?

Issues

Solutions

Testing

PR Post Submission Checklist for internal contributors (Optional)

PR Post Merge Checklist for internal contributors

References

github-actions bot commented Oct 9, 2023 • edited Loading

Unit Test Results

datadog-wireapp bot commented Oct 9, 2023 • edited Loading

Datadog Report

❌ Failed Tests (1)

codecov-commenter commented Oct 9, 2023 • edited Loading

Codecov Report

typfel Oct 10, 2023

Choose a reason for hiding this comment

typfel Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

typfel Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

typfel Oct 11, 2023

Choose a reason for hiding this comment

typfel Oct 10, 2023

Choose a reason for hiding this comment

vitorhugods left a comment

Choose a reason for hiding this comment

yamilmedina left a comment

Choose a reason for hiding this comment

typfel commented Oct 10, 2023

github-actions bot commented Oct 9, 2023 •

edited

Loading

datadog-wireapp bot commented Oct 9, 2023 •

edited

Loading

codecov-commenter commented Oct 9, 2023 •

edited

Loading

typfel Oct 10, 2023 •

edited

Loading

typfel Oct 11, 2023 •

edited

Loading