diff --git a/Cargo.lock b/Cargo.lock index 52423983fe..8284b215ac 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3183,6 +3183,7 @@ name = "sync_manager" version = "0.1.0" dependencies = [ "anyhow", + "autofill", "error-support", "ffi-support 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)", "interrupt-support", diff --git a/components/autofill/Cargo.toml b/components/autofill/Cargo.toml index a9f525e68f..017385557b 100644 --- a/components/autofill/Cargo.toml +++ b/components/autofill/Cargo.toml @@ -11,6 +11,7 @@ license = "MPL-2.0" anyhow = "1.0" error-support = { path = "../support/error" } interrupt-support = { path = "../support/interrupt" } +lazy_static = "1.4" log = "0.4" serde = "1" serde_derive = "1" @@ -30,7 +31,6 @@ features = ["functions", "bundled", "serde_json", "unlock_notify"] [dev-dependencies] env_logger = { version = "0.7", default-features = false } -lazy_static = "1.4" libsqlite3-sys = "0.20.1" [build-dependencies] diff --git a/components/autofill/src/autofill.udl b/components/autofill/src/autofill.udl index bcc8ef005a..d0fc762135 100644 --- a/components/autofill/src/autofill.udl +++ b/components/autofill/src/autofill.udl @@ -67,6 +67,11 @@ enum Error { "SqlError", "IoError", "InterruptedError", "IllegalDatabasePath", "Utf8Error", "JsonError", "InvalidSyncPayload", }; +// We don't really *need* this to be `Threadsafe` because we have a mutex to +// manage concurrent DB connections - but we might as well so our Mutexes +// do what we thing they are doing rather than being controlled by the hidden +// mutexes non-threadsafe uniffi gives us. +[Threadsafe] interface Store { [Throws=Error] constructor(string dbpath); @@ -106,4 +111,6 @@ interface Store { [Throws=Error] void touch_address(string guid); + + void register_with_sync_manager(); }; diff --git a/components/autofill/src/db/store.rs b/components/autofill/src/db/store.rs index 9c6feb2ec0..af52ff20e9 100644 --- a/components/autofill/src/db/store.rs +++ b/components/autofill/src/db/store.rs @@ -11,35 +11,143 @@ use rusqlite::{ Connection, }; use sql_support::{self, ConnExt}; +use std::cell::RefCell; use std::path::Path; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, Weak}; use sync15_traits::SyncEngine; use sync_guid::Guid; -#[allow(dead_code)] +// Our "sync manager" will use whatever is stashed here. +lazy_static::lazy_static! { + // Mutex: just taken long enough to update the inner stuff - needed + // to wrap the RefCell as they aren't `Sync` + // RefCell: So we can replace what it holds. Normally you'd use `get_ref()` + // on the mutex and avoid the RefCell entirely, but that requires + // the mutex to be declared as `mut` which is apparently + // impossible in a `lazy_static` + // [Arc/Weak]: What the sync manager actually needs. + pub static ref STORE_FOR_MANAGER: Mutex>> = Mutex::new(RefCell::new(Weak::new())); +} + +// This is the type that uniffi exposes. It holds an `Arc<>` around the +// actual implementation, because we need to hand a clone of this `Arc<>` to +// the sync manager and to sync engines. One day +// https://github.com/mozilla/uniffi-rs/issues/417 will give us access to the +// `Arc<>` uniffi owns, which means we can drop this entirely (ie, `Store` and +// `StoreImpl` could be re-unified) +// Sadly, this is `pub` because our `autofill-utils` example uses it. pub struct Store { - db: Arc>, + store_impl: Arc, } -#[allow(dead_code)] impl Store { pub fn new(db_path: impl AsRef) -> Result { Ok(Self { - db: Arc::new(Mutex::new(AutofillDb::new(db_path)?)), + store_impl: Arc::new(StoreImpl::new(db_path)?), }) } - /// Creates a store backed by an in-memory database. - #[cfg(test)] - pub fn new_memory(db_path: &str) -> Result { + pub fn add_credit_card(&self, fields: UpdatableCreditCardFields) -> Result { + self.store_impl.add_credit_card(fields) + } + + pub fn get_credit_card(&self, guid: String) -> Result { + self.store_impl.get_credit_card(guid) + } + + pub fn get_all_credit_cards(&self) -> Result> { + self.store_impl.get_all_credit_cards() + } + + pub fn update_credit_card( + &self, + guid: String, + credit_card: UpdatableCreditCardFields, + ) -> Result<()> { + self.store_impl.update_credit_card(guid, credit_card) + } + + pub fn delete_credit_card(&self, guid: String) -> Result { + self.store_impl.delete_credit_card(guid) + } + + pub fn touch_credit_card(&self, guid: String) -> Result<()> { + self.store_impl.touch_credit_card(guid) + } + + pub fn add_address(&self, new_address: UpdatableAddressFields) -> Result
{ + self.store_impl.add_address(new_address) + } + + pub fn get_address(&self, guid: String) -> Result
{ + self.store_impl.get_address(guid) + } + + pub fn get_all_addresses(&self) -> Result> { + self.store_impl.get_all_addresses() + } + + pub fn update_address(&self, guid: String, address: UpdatableAddressFields) -> Result<()> { + self.store_impl.update_address(guid, address) + } + + pub fn delete_address(&self, guid: String) -> Result { + self.store_impl.delete_address(guid) + } + + pub fn touch_address(&self, guid: String) -> Result<()> { + self.store_impl.touch_address(guid) + } + + // This allows the embedding app to say "make this instance available to + // the sync manager". The implementation is more like "offer to sync mgr" + // (thereby avoiding us needing to link with the sync manager) but + // `register_with_sync_manager()` is logically what's happening so that's + // the name it gets. + pub fn register_with_sync_manager(&self) { + STORE_FOR_MANAGER + .lock() + .unwrap() + .replace(Arc::downgrade(&self.store_impl)); + } + + // These 2 are odd ones out - they don't just delegate but instead + // hand off the Arc. + // Currently the only consumer of this is our "example" (and hence why they + // are `pub` and not `pub(crate)`) - the sync manager duplicates it (because + // it doesn't have a reference to us, just to the store_impl) + // We could probably make the example work with the sync manager - but then + // our example would link with places and logins etc. + pub fn create_credit_cards_sync_engine(&self) -> Box { + Box::new(crate::sync::credit_card::create_engine( + self.store_impl.clone(), + )) + } + + pub fn create_addresses_sync_engine(&self) -> Box { + Box::new(crate::sync::address::create_engine(self.store_impl.clone())) + } +} + +// This is the actual implementation. All code in this crate works with this. +// Sadly, it's forced to be `pub` because the SyncManager also uses it. +pub struct StoreImpl { + pub(crate) db: Mutex, +} + +impl StoreImpl { + pub fn new(db_path: impl AsRef) -> Result { Ok(Self { - db: Arc::new(Mutex::new(AutofillDb::new_memory(db_path)?)), + db: Mutex::new(AutofillDb::new(db_path)?), }) } + /// Creates a store backed by an in-memory database. #[cfg(test)] - pub fn db(&self) -> Arc> { - self.db.clone() + pub fn new_memory() -> Self { + Self { + db: Mutex::new(crate::db::test::new_mem_db()), + } } pub fn add_credit_card(&self, fields: UpdatableCreditCardFields) -> Result { @@ -108,14 +216,6 @@ impl Store { pub fn touch_address(&self, guid: String) -> Result<()> { addresses::touch(&self.db.lock().unwrap().writer, &Guid::new(&guid)) } - - pub fn create_credit_cards_sync_engine(&self) -> Box { - Box::new(crate::sync::credit_card::create_engine(self.db.clone())) - } - - pub fn create_addresses_sync_engine(&self) -> Box { - Box::new(crate::sync::address::create_engine(self.db.clone())) - } } pub(crate) fn put_meta(conn: &Connection, key: &str, value: &dyn ToSql) -> Result<()> { diff --git a/components/autofill/src/lib.rs b/components/autofill/src/lib.rs index ba3a4b5582..cae5777e7e 100644 --- a/components/autofill/src/lib.rs +++ b/components/autofill/src/lib.rs @@ -10,6 +10,9 @@ pub mod db; pub mod error; pub mod sync; +// Re-export stuff the sync manager needs. +pub use crate::db::store::{StoreImpl, STORE_FOR_MANAGER}; + // Expose stuff needed by the uniffi generated code. use crate::db::models::address::*; use crate::db::models::credit_card::*; diff --git a/components/autofill/src/sync/address/mod.rs b/components/autofill/src/sync/address/mod.rs index b305b4f47b..f9a6bd1b54 100644 --- a/components/autofill/src/sync/address/mod.rs +++ b/components/autofill/src/sync/address/mod.rs @@ -17,14 +17,14 @@ use incoming::IncomingAddressesImpl; use outgoing::OutgoingAddressesImpl; use rusqlite::Transaction; use serde::{Deserialize, Serialize}; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use sync_guid::Guid; use types::Timestamp; // The engine. -pub fn create_engine(db: Arc>) -> ConfigSyncEngine { +pub fn create_engine(store: Arc) -> ConfigSyncEngine { ConfigSyncEngine { - db, + store, config: EngineConfig { namespace: "addresses".to_string(), collection: "addresses", diff --git a/components/autofill/src/sync/credit_card/mod.rs b/components/autofill/src/sync/credit_card/mod.rs index 582a1464a5..1b268f871c 100644 --- a/components/autofill/src/sync/credit_card/mod.rs +++ b/components/autofill/src/sync/credit_card/mod.rs @@ -17,16 +17,14 @@ use incoming::IncomingCreditCardsImpl; use outgoing::OutgoingCreditCardsImpl; use rusqlite::Transaction; use serde::{Deserialize, Serialize}; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use sync_guid::Guid; use types::Timestamp; // The engine. -pub fn create_engine( - db: Arc>, -) -> ConfigSyncEngine { +pub fn create_engine(store: Arc) -> ConfigSyncEngine { ConfigSyncEngine { - db, + store, config: EngineConfig { namespace: "credit_cards".to_string(), collection: "creditcards", diff --git a/components/autofill/src/sync/engine.rs b/components/autofill/src/sync/engine.rs index 1659bcfccd..f35ca662fc 100644 --- a/components/autofill/src/sync/engine.rs +++ b/components/autofill/src/sync/engine.rs @@ -3,13 +3,13 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use super::{plan_incoming, ProcessIncomingRecordImpl, ProcessOutgoingRecordImpl, SyncRecord}; -use crate::db::AutofillDb; use crate::error::*; +use crate::StoreImpl as Store; use rusqlite::{ types::{FromSql, ToSql}, Connection, Transaction, }; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use sync15::{ telemetry, CollSyncIds, CollectionRequest, EngineSyncAssociation, IncomingChangeset, OutgoingChangeset, ServerTimestamp, SyncEngine, @@ -38,7 +38,7 @@ pub trait SyncEngineStorageImpl { // A sync engine that gets functionality from an EngineConfig. pub struct ConfigSyncEngine { pub(crate) config: EngineConfig, - pub(crate) db: Arc>, + pub(crate) store: Arc, pub(crate) storage_impl: Box>, } @@ -71,7 +71,7 @@ impl SyncEngine for ConfigSyncEngine { assert_eq!(inbound.len(), 1, "we only request one item"); let inbound = inbound.into_iter().next().unwrap(); - let db = self.db.lock().unwrap(); + let db = &self.store.db.lock().unwrap(); crate::db::schema::create_empty_sync_temp_tables(&db.writer)?; let signal = db.begin_interrupt_scope(); @@ -122,7 +122,7 @@ impl SyncEngine for ConfigSyncEngine { new_timestamp: ServerTimestamp, records_synced: Vec, ) -> anyhow::Result<()> { - let db = self.db.lock().unwrap(); + let db = &self.store.db.lock().unwrap(); self.put_meta( &db.writer, LAST_SYNC_META_KEY, @@ -139,7 +139,7 @@ impl SyncEngine for ConfigSyncEngine { &self, server_timestamp: ServerTimestamp, ) -> anyhow::Result> { - let db = self.db.lock().unwrap(); + let db = &self.store.db.lock().unwrap(); let since = ServerTimestamp( self.get_meta::(&db.writer, LAST_SYNC_META_KEY)? .unwrap_or_default(), @@ -154,7 +154,7 @@ impl SyncEngine for ConfigSyncEngine { } fn get_sync_assoc(&self) -> anyhow::Result { - let db = self.db.lock().unwrap(); + let db = &self.store.db.lock().unwrap(); let global = self.get_meta(&db.writer, GLOBAL_SYNCID_META_KEY)?; let coll = self.get_meta(&db.writer, COLLECTION_SYNCID_META_KEY)?; Ok(if let (Some(global), Some(coll)) = (global, coll) { @@ -165,7 +165,7 @@ impl SyncEngine for ConfigSyncEngine { } fn reset(&self, assoc: &EngineSyncAssociation) -> anyhow::Result<()> { - let db = self.db.lock().unwrap(); + let db = &self.store.db.lock().unwrap(); let tx = db.unchecked_transaction()?; self.storage_impl.reset_storage(&tx)?; // Reset the last sync time, so that the next sync fetches fresh records @@ -200,12 +200,13 @@ mod tests { use crate::db::credit_cards::add_internal_credit_card; use crate::db::credit_cards::tests::{get_all, insert_mirror_record, insert_tombstone_record}; use crate::db::models::credit_card::InternalCreditCard; - use crate::db::{schema::create_empty_sync_temp_tables, test::new_mem_db}; + use crate::db::schema::create_empty_sync_temp_tables; use sql_support::ConnExt; // We use the credit-card engine here. - fn create_engine(db: AutofillDb) -> ConfigSyncEngine { - crate::sync::credit_card::create_engine(Arc::new(Mutex::new(db))) + fn create_engine() -> ConfigSyncEngine { + let store = crate::db::store::StoreImpl::new_memory(); + crate::sync::credit_card::create_engine(Arc::new(store)) } pub fn clear_cc_tables(conn: &Connection) -> rusqlite::Result<(), rusqlite::Error> { @@ -219,10 +220,10 @@ mod tests { #[test] fn test_credit_card_engine_sync_finished() -> Result<()> { - let db = new_mem_db(); - create_empty_sync_temp_tables(&db).expect("should create temp tables"); - - let credit_card_engine = create_engine(db); + let credit_card_engine = create_engine(); + { + create_empty_sync_temp_tables(&credit_card_engine.store.db.lock().unwrap())?; + } let last_sync = 24; let result = @@ -230,7 +231,7 @@ mod tests { assert!(result.is_ok()); // check that last sync metadata was set - let conn = &credit_card_engine.db.lock().unwrap().writer; + let conn = &credit_card_engine.store.db.lock().unwrap().writer; assert_eq!( credit_card_engine.get_meta::(conn, LAST_SYNC_META_KEY)?, @@ -242,8 +243,7 @@ mod tests { #[test] fn test_credit_card_engine_get_sync_assoc() -> Result<()> { - let db = new_mem_db(); - let credit_card_engine = create_engine(db); + let credit_card_engine = create_engine(); let result = credit_card_engine.get_sync_assoc(); assert!(result.is_ok()); @@ -259,7 +259,7 @@ mod tests { coll: coll_guid, }; { - let conn = &credit_card_engine.db.lock().unwrap().writer; + let conn = &credit_card_engine.store.db.lock().unwrap().writer; credit_card_engine.put_meta(conn, GLOBAL_SYNCID_META_KEY, &ids.global)?; credit_card_engine.put_meta(conn, COLLECTION_SYNCID_META_KEY, &ids.coll)?; } @@ -274,10 +274,8 @@ mod tests { #[test] fn test_engine_sync_reset() -> Result<()> { - let db = new_mem_db(); + let engine = create_engine(); - let tx = db.writer.unchecked_transaction()?; - // create a normal record, a mirror record and a tombstone. let cc = InternalCreditCard { guid: Guid::random(), cc_name: "Ms Jane Doe".to_string(), @@ -287,12 +285,18 @@ mod tests { cc_type: "visa".to_string(), ..Default::default() }; - add_internal_credit_card(&tx, &cc)?; - insert_mirror_record(&tx, cc.clone()); - insert_tombstone_record(&tx, Guid::random().to_string())?; - tx.commit()?; - let engine = create_engine(db); + { + // temp scope for the mutex lock. + let db = &engine.store.db.lock().unwrap(); + + let tx = db.writer.unchecked_transaction()?; + // create a normal record, a mirror record and a tombstone. + add_internal_credit_card(&tx, &cc)?; + insert_mirror_record(&tx, cc.clone()); + insert_tombstone_record(&tx, Guid::random().to_string())?; + tx.commit()?; + } // create sync metadata let global_guid = Guid::new("AAAA"); @@ -302,7 +306,7 @@ mod tests { coll: coll_guid.clone(), }; { - let conn = &engine.db.lock().unwrap().writer; + let conn = &engine.store.db.lock().unwrap().writer; engine.put_meta(conn, GLOBAL_SYNCID_META_KEY, &ids.global)?; engine.put_meta(conn, COLLECTION_SYNCID_META_KEY, &ids.coll)?; } @@ -313,7 +317,7 @@ mod tests { .expect("should work"); { - let conn = &engine.db.lock().unwrap().writer; + let conn = &engine.store.db.lock().unwrap().writer; // check that the mirror and tombstone tables have no records assert!(get_all(conn, "credit_cards_mirror".to_string())?.is_empty()); @@ -351,7 +355,7 @@ mod tests { .reset(&EngineSyncAssociation::Connected(ids)) .expect("should work"); - let conn = &engine.db.lock().unwrap().writer; + let conn = &engine.store.db.lock().unwrap().writer; // check that the meta records were set let retrieved_global_sync_id = engine.get_meta::(conn, GLOBAL_SYNCID_META_KEY)?; assert_eq!( diff --git a/components/sync_manager/Cargo.toml b/components/sync_manager/Cargo.toml index 13e56bfdf4..4965878e0e 100644 --- a/components/sync_manager/Cargo.toml +++ b/components/sync_manager/Cargo.toml @@ -7,6 +7,7 @@ license = "MPL-2.0" exclude = ["/android", "/ios"] [dependencies] +autofill = { path = "../autofill" } sync15 = { path = "../sync15" } places = { path = "../places" } logins = { path = "../logins" } diff --git a/components/sync_manager/src/error.rs b/components/sync_manager/src/error.rs index 2e6f9a9b8f..ad1dae790c 100644 --- a/components/sync_manager/src/error.rs +++ b/components/sync_manager/src/error.rs @@ -29,6 +29,10 @@ pub enum ErrorKind { LoginsError(#[from] logins::Error), #[error("Places error: {0}")] PlacesError(#[from] places::Error), + // We should probably upgrade this crate to anyhow, which would mean this + // gets replaced with AutofillError or similar. + #[error("External error: {0}")] + AnyhowError(#[from] anyhow::Error), } error_support::define_error! { @@ -41,5 +45,6 @@ error_support::define_error! { (JsonError, serde_json::Error), (LoginsError, logins::Error), (PlacesError, places::Error), + (AnyhowError, anyhow::Error), } } diff --git a/components/sync_manager/src/manager.rs b/components/sync_manager/src/manager.rs index 03326cccdc..e9daec0885 100644 --- a/components/sync_manager/src/manager.rs +++ b/components/sync_manager/src/manager.rs @@ -15,7 +15,7 @@ use std::time::SystemTime; use sync15::{ self, clients::{self, Command, CommandProcessor, CommandStatus, Settings}, - MemoryCachedState, + EngineSyncAssociation, MemoryCachedState, SyncEngine, }; use tabs::TabsStore; @@ -64,6 +64,23 @@ impl SyncManager { self.tabs = Arc::downgrade(&tabs); } + pub fn autofill_engine(engine: &str) -> Result>> { + let cell = autofill::STORE_FOR_MANAGER.lock().unwrap(); + // The cell holds a `Weak` - borrow it (which is safe as we have the + // mutex) and upgrade it to a real Arc. + let r = cell.borrow(); + match r.upgrade() { + None => Ok(None), + Some(arc) => match engine { + "addresses" => Ok(Some(Box::new(autofill::sync::address::create_engine(arc)))), + "creditcards" => Ok(Some(Box::new(autofill::sync::credit_card::create_engine( + arc, + )))), + _ => Err(ErrorKind::UnknownEngine(engine.into()).into()), + }, + } + } + pub fn wipe(&mut self, engine: &str) -> Result<()> { match engine { "logins" => { @@ -95,6 +112,12 @@ impl SyncManager { Err(ErrorKind::ConnectionClosed(engine.into()).into()) } } + "addresses" | "creditcards" => { + if let Some(engine) = Self::autofill_engine(engine)? { + engine.wipe()?; + } + Ok(()) + } _ => Err(ErrorKind::UnknownEngine(engine.into()).into()), } } @@ -142,6 +165,12 @@ impl SyncManager { Err(ErrorKind::ConnectionClosed(engine.into()).into()) } } + "addresses" | "creditcards" => { + if let Some(engine) = Self::autofill_engine(engine)? { + engine.reset(&EngineSyncAssociation::Disconnected)?; + } + Ok(()) + } _ => Err(ErrorKind::UnknownEngine(engine.into()).into()), } }