[PM-1724] Sqlite WASM #924
Conversation
# Conflicts: # Cargo.lock # crates/bitwarden/src/vault/cipher/mod.rs
# Conflicts: # Cargo.lock # crates/bitwarden-core/src/.gitignore # crates/bitwarden-vault/src/cipher/repository.rs # crates/bitwarden-vault/src/sync.rs # crates/bitwarden/Cargo.toml # crates/bitwarden/src/client/client.rs # crates/bitwarden/src/client/mod.rs # crates/bitwarden/src/error.rs # crates/bitwarden/src/vault/client_vault.rs
|
No New Or Fixed Issues Found |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
# Conflicts: # Cargo.lock # crates/bitwarden-core/Cargo.toml # crates/bitwarden-core/src/client/internal.rs # crates/bitwarden-vault/Cargo.toml
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #924 +/- ##
==========================================
+ Coverage 57.96% 58.78% +0.81%
==========================================
Files 197 203 +6
Lines 13651 14085 +434
==========================================
+ Hits 7913 8280 +367
- Misses 5738 5805 +67 ☔ View full report in Codecov by Sentry. |
## 🎟️ Tracking <!-- Paste the link to the Jira or GitHub issue or otherwise describe / point to where this change is coming from. --> ## 📔 Objective <!-- Describe what the purpose of this PR is, for example what bug you're fixing or new feature you're adding. --> The #972 PR requires MSRV 1.73, and #924 will require. MSRV 1.75. To avoid upgrading twice I suggest immediately changing to 1.75. This is noted as a change in both the CLI and bitwarden change logs. ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+1:`) or similar for great changes - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info - ❓ (`:question:`) for questions - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (`:art:`) for suggestions / improvements - ❌ (`:x:`) or⚠️ (`:warning:`) for more significant problems or concerns needing attention - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt - ⛏ (`:pick:`) for minor or nitpick changes
# Conflicts: # .github/workflows/minimum-rust-version.yml # crates/bitwarden-c/src/c.rs # crates/bitwarden-core/Cargo.toml # crates/bitwarden-core/src/mobile/crypto.rs # crates/bitwarden-py/src/client.rs # crates/bitwarden-uniffi/src/lib.rs # crates/bitwarden-vault/Cargo.toml # crates/bitwarden/Cargo.toml
crates/bitwarden-uniffi/src/lib.rs
Outdated
| @@ -32,13 +32,13 @@ pub struct Client(bitwarden::Client); | |||
| impl Client { | |||
| /// Initialize a new instance of the SDK client | |||
| #[uniffi::constructor] | |||
| pub fn new(settings: Option<ClientSettings>) -> Arc<Self> { | |||
| pub async fn factory(settings: Option<ClientSettings>) -> Arc<Self> { | |||
There was a problem hiding this comment.
factory seems like a confusing name, I would almost expect this function to return a factory itself. Thoughts on using something else like create?
| @@ -79,6 +117,8 @@ impl Client { | |||
| })), | |||
| external_client, | |||
| encryption_settings: RwLock::new(None), | |||
| #[cfg(feature = "state")] | |||
| db: Arc::new(Mutex::new(db)), | |||
There was a problem hiding this comment.
I don't think the Mutex around the Database here is needed, all the functions in the DatabaseTrait only take &self as far as I could see.
There was a problem hiding this comment.
Maybe, but ideally you shouldn't run multiple queries at the same time if we want to support transactions.
|
|
||
| #[tokio::main(flavor = "current_thread")] | ||
| async fn main() { | ||
| run_tests().await; |
There was a problem hiding this comment.
These tests seem to leave the test.sqlite file hanging around after they finish when I run them with cargo run --bin bitwarden_db_tests.
That then causes the next run of the tests for me to fail, should we remove it?
There was a problem hiding this comment.
Will this crate be a part of the SDK or is it just a test to check that the POC works? Because if it's the former it would be nice to have it checked in CI
There was a problem hiding this comment.
I think we should keep it around since it's a good suite to test the database behavior.
| async fn get_version(&self) -> Result<usize, DatabaseError>; | ||
| async fn set_version(&self, version: usize) -> Result<(), DatabaseError>; | ||
|
|
||
| async fn execute_batch(&self, sql: &str) -> Result<(), DatabaseError>; |
There was a problem hiding this comment.
It would be nice to have execute_batch and query_map documented here, instead of having to click though into the rusqlite documentation
| @@ -66,6 +91,19 @@ impl Client { | |||
| api_key: None, | |||
| }; | |||
|
|
|||
| #[cfg(feature = "state")] | |||
| let db = { | |||
| let db = Database::default() | |||
There was a problem hiding this comment.
Should we pass some path to the database here? Otherwise two simultaneous SDK clients will overwrite each other.
| pub async fn replace_all(&self, ciphers: &[Cipher]) -> Result<(), CipherRepositoryError> { | ||
| let guard = self.db.lock().await; | ||
|
|
||
| guard.execute("DELETE FROM ciphers", []).await?; |
There was a problem hiding this comment.
Would be nice to be able to run the DELETE FROM and INSERT INTO in the same transaction, to be able to rollback the whole thing in cases there's an error.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-1724
📔 Objective
Implement persisted state backed by SQLite on our supported platforms. It provides an abstraction layer with a similar interface as the rusqlite crate with some minor differences required to support the web assembly interface.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes