refactor: use unified connection [WPB-23775]#2303
Open
coriolinus wants to merge 18 commits into
Open
Conversation
We are now deprecating the old connection module in favor of the unified connection, and removing platform-specific code from the entity implementations. Unfortunately, we need to retain sufficient wasm code that we can still run enough of a migration to upgrade into the new unified entity database, and that means that we've got a whole rump of otherwise- useless code sitting in the legacy module. Can't be helped, unfortunately.
We're deprecating those in favor of the new `UnifiedEntity` traits and will eventually update the names to remove the `Unified*` prefix. The old traits still exist and are implemented in the `idb_migration::legacy` module.
We only need the new stuff now. Old implementations can live in the legacy module.
Just keeping things in parity with the old `Database` impl.
- rm now-obsolete trait implementations - move certain implementations out of the platform module - in general make the entities implement what they need inline
`Database` needs to be able to manage the `KeystoreTransaction` lifecycle, so we organize these methods centrally here.
At this point I can't even explain why that was necessary, but it was necessary to make everything work, so here it is.
Previously we'd been using the old `Database` and `Entity` trait family pervasively. We need to switch to the new stuff, so here we do.
We'd previously had a pointless trait which was implemented on the database and on nothing else, even for testing. That wasn't helping anything, so we got rid of it. Also updates the proteus-specific methods to use the new entity traits.
We'd previously had a pointless trait which was implemented on the database and on nothing else, even for testing. That wasn't helping anything, so we got rid of it. Also updates the mls-specific methods to use the new entity traits.
The code previously under test is now effectively unchanging; no new development will touch it. And it's now buried deep in the legacy migration module; it's not exposed publicly anymore. Therefore, we get rid of this test, because there's no longer a compelling reason why it should still exist.
This is mainly a convenience: it's not strictly necessary, but as consumers all accept an `Arc<Database>` anyway, there's no point making users construct the `Arc` themselves.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's new in this PR
Partially implements 23775. This accomplishes most of that PR, except for two more steps:
Unified*entity trait family traits to rm theUnifiedprefixOne issue I see right away: this new
Databasewraps a connection directly, where the old one wrapped aMutex<Option<Connection>>. This means we can no longer use theDatabase::closemethod, because it consumesselfand consumers all always have anArc<Database>.Whoever picks this up will need to either adjust that and all the access patterns, or determine that we can omit
closemethods entirely now.PR Submission Checklist for internal contributors
SQPIT-764feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.