Skip to content

Bug 2056071 - remove BridgedEngine, app-services side. #7483

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
mhammond wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Bug 2056071 - remove BridgedEngine, app-services side. #7483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 1 addition & 7 deletions components/autofill/src/autofill.udl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,6 @@ interface AddressesBridgedEngine {
[Throws=AutofillApiError]
i64 last_sync();

[Throws=AutofillApiError]
void set_last_sync(i64 last_sync);

[Throws=AutofillApiError]
string? sync_id();

Expand All @@ -297,17 +294,14 @@ interface AddressesBridgedEngine {
[Throws=AutofillApiError]
string ensure_current_sync_id([ByRef]string new_sync_id);

[Throws=AutofillApiError]
void prepare_for_sync([ByRef]string client_data);

[Throws=AutofillApiError]
void sync_started();

[Throws=AutofillApiError]
void store_incoming(sequence<string> incoming_envelopes_as_json);

[Throws=AutofillApiError]
sequence<string> apply();
sequence<string> apply(i64 server_modified_millis);

[Throws=AutofillApiError]
void set_uploaded(i64 new_timestamp, sequence<string> uploaded_ids);
Expand Down
64 changes: 14 additions & 50 deletions components/autofill/src/sync/bridge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,53 +2,22 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use crate::db::models::address::InternalAddress;
use crate::sync::engine::ConfigSyncEngine;
use crate::Store;
use anyhow::Result;
use std::sync::Arc;
use sync15::engine::BridgedEngineAdaptor;

impl Store {
/// Returns a bridged sync engine for addresses, for use by Desktop's Sync
/// framework. Constructing a `ConfigSyncEngine` only assembles structs and
/// never touches the DB, so this cannot fail.
pub fn addresses_bridged_engine(self: Arc<Self>) -> Arc<AddressesBridgedEngine> {
let engine = crate::sync::address::create_engine(self);
Arc::new(AddressesBridgedEngine::new(Box::new(
AddressesBridgedEngineAdaptor { engine },
)))
}
}

/// `ConfigSyncEngine` implements `sync15::SyncEngine`, which is what the sync
/// manager drives. Desktop instead speaks `mozIBridgedSyncEngine`, whose Rust
/// shape is `sync15::BridgedEngine`. The two differ only in that the bridge owns
/// the last-sync timestamp explicitly, so this adaptor supplies that and the
/// blanket `impl<A: BridgedEngineAdaptor> BridgedEngine for A` provides the rest.
struct AddressesBridgedEngineAdaptor {
engine: ConfigSyncEngine<InternalAddress>,
}

impl BridgedEngineAdaptor for AddressesBridgedEngineAdaptor {
fn last_sync(&self) -> Result<i64> {
Ok(self.engine.get_last_sync_millis()?)
}

fn set_last_sync(&self, last_sync_millis: i64) -> Result<()> {
self.engine.set_last_sync_millis(last_sync_millis)?;
Ok(())
}

fn engine(&self) -> &dyn sync15::engine::SyncEngine {
&self.engine
Arc::new(AddressesBridgedEngine::new(Box::new(engine)))
}
}

// Generates the UniFFI-exposed `AddressesBridgedEngine`, a newtype around
// `sync15::engine::BridgedEngineWrapper`. The UDL's `set_uploaded` takes
// `sequence<string>`, hence the `String` id type.
sync15::uniffi_bridged_engine!(AddressesBridgedEngine, String);
// `sync15::engine::BridgedEngineWrapper`.
sync15::uniffi_bridged_engine!(AddressesBridgedEngine);

#[cfg(test)]
mod tests {
Expand All @@ -64,9 +33,10 @@ mod tests {
let store = Arc::new(Store::new_shared_memory("addresses-bridge").unwrap());
let bridge = store.addresses_bridged_engine();

bridge.sync_started().unwrap();
// Fresh DB: never synced.
assert_eq!(bridge.last_sync().unwrap(), 0);
bridge.set_last_sync(3).unwrap();
bridge.set_uploaded(3, vec![]).unwrap();
assert_eq!(bridge.last_sync().unwrap(), 3);

assert!(bridge.sync_id().unwrap().is_none());
Expand All @@ -75,12 +45,12 @@ mod tests {
assert_eq!(bridge.sync_id().unwrap(), Some("some_guid".to_string()));
// changing the sync ID resets the timestamp
assert_eq!(bridge.last_sync().unwrap(), 0);
bridge.set_last_sync(3).unwrap();
bridge.set_uploaded(3, vec![]).unwrap();

bridge.reset_sync_id().unwrap();
assert_ne!(bridge.sync_id().unwrap(), Some("some_guid".to_string()));
assert_eq!(bridge.last_sync().unwrap(), 0);
bridge.set_last_sync(3).unwrap();
bridge.set_uploaded(3, vec![]).unwrap();

// `reset` clears the guid and the timestamp.
bridge.reset().unwrap();
Expand Down Expand Up @@ -113,12 +83,8 @@ mod tests {

let bridge = store.clone().addresses_bridged_engine();

// `prepare_for_sync` is what creates the sync staging tables; the client
// data it is given is unused by this engine.
bridge
.prepare_for_sync(r#"{"local_client_id":"my-client","recent_clients":{}}"#)
.expect("should prepare for sync");
bridge.sync_started().unwrap();
// `sync_started` is what creates the sync staging tables.
bridge.sync_started().expect("should prepare for sync");

// An incoming remote address that isn't known locally. We build the
// envelope as raw JSON, exactly as the JS bridge hands it to us.
Expand All @@ -144,7 +110,7 @@ mod tests {

// Applying stores the remote record locally and returns the local-only
// address for upload.
let outgoing = bridge.apply().expect("should apply");
let outgoing = bridge.apply(1234).expect("should apply");
let changes: HashMap<String, serde_json::Value> = outgoing
.into_iter()
.map(|s| {
Expand All @@ -169,15 +135,13 @@ mod tests {
.expect("remote address should have been stored");
assert_eq!(stored.street_address, "99 Remote Road");

// `apply` deliberately stamps last_sync with 0 - Desktop applies without
// telling us the server timestamp and sends it separately afterwards.
assert_eq!(bridge.last_sync().unwrap(), 0);
bridge.set_uploaded(1234, vec![local.guid.clone()]).unwrap();
bridge.sync_finished().unwrap();
assert_eq!(bridge.last_sync().unwrap(), 1234);
bridge.set_uploaded(5678, vec![local.guid.clone()]).unwrap();
bridge.sync_finished().unwrap();
assert_eq!(bridge.last_sync().unwrap(), 5678);

// Acknowledging the upload cleared the record's change counter, so a
// subsequent sync has nothing to send.
assert!(bridge.apply().expect("should apply again").is_empty());
assert!(bridge.apply(5678).expect("should apply again").is_empty());
}
}
37 changes: 19 additions & 18 deletions components/autofill/src/sync/engine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,6 @@ impl<T> ConfigSyncEngine<T> {
let key = format!("{}.{}", self.config.namespace, tail);
crate::db::store::delete_meta(conn, &key)
}
/// The last-sync timestamp in milliseconds, 0 if never synced.
pub(crate) fn get_last_sync_millis(&self) -> Result<i64> {
let db = self.store.lock_db()?;
Ok(self
.get_meta::<i64>(&db.writer, LAST_SYNC_META_KEY)?
.unwrap_or_default())
}

pub(crate) fn set_last_sync_millis(&self, millis: i64) -> Result<()> {
let db = self.store.lock_db()?;
self.put_meta(&db.writer, LAST_SYNC_META_KEY, &millis)
}

// Reset the local sync data so the next server request fetches all records.
pub fn reset_local_sync_data(&self) -> Result<()> {
Expand Down Expand Up @@ -119,10 +107,7 @@ impl<T: SyncRecord + std::fmt::Debug> SyncEngine for ConfigSyncEngine<T> {
Ok(())
}

fn prepare_for_sync(
&self,
_get_client_data: &dyn Fn() -> sync15::ClientData,
) -> anyhow::Result<()> {
fn sync_started(&self) -> anyhow::Result<()> {
let db = self.store.lock_db()?;
let signal = db.begin_interrupt_scope()?;
crate::db::schema::create_empty_sync_temp_tables(&db.writer)?;
Expand Down Expand Up @@ -169,10 +154,13 @@ impl<T: SyncRecord + std::fmt::Debug> SyncEngine for ConfigSyncEngine<T> {
super::apply_incoming_action(&*incoming_impl, &tx, action)?;
}

// write the timestamp now, so if we are interrupted merging or
// The timestamp value is handled differently in desktop v mobile. Record a
// timestamp if we are given one now, so if we are interrupted merging or
// creating outgoing changesets we don't need to re-download the same
// records.
self.put_meta(&tx, LAST_SYNC_META_KEY, &timestamp.as_millis())?;
if timestamp != ServerTimestamp(0) {
self.put_meta(&tx, LAST_SYNC_META_KEY, &timestamp.as_millis())?;
}

incoming_impl.finish_incoming(&tx)?;

Expand Down Expand Up @@ -256,6 +244,19 @@ impl<T: SyncRecord + std::fmt::Debug> SyncEngine for ConfigSyncEngine<T> {
warn!("not implemented as there isn't a valid use case for it");
Ok(())
}

fn last_sync(&self) -> anyhow::Result<Option<ServerTimestamp>> {
let db = self.store.lock_db()?;
Ok(self
.get_meta::<i64>(&db.writer, LAST_SYNC_META_KEY)?
.map(ServerTimestamp::from_millis))
}

fn reset_last_sync(&self) -> anyhow::Result<()> {
let db = self.store.lock_db()?;
self.delete_meta(&db.writer, LAST_SYNC_META_KEY)?;
Ok(())
}
}

#[cfg(test)]
Expand Down
5 changes: 3 additions & 2 deletions components/logins/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,9 @@ impl GetErrorHandling for Error {
}

// The bridged sync engine (`sync::bridge`) deals in `anyhow::Result`, as that's
// what the `sync15` BridgedEngine traits use. This lets UniFFI map those errors
// onto our public error type when the bridge methods are exposed via the UDL.
// what the `sync15` `SyncEngine`/`BridgedEngineWrapper` use. This lets UniFFI map
// those errors onto our public error type when the bridge methods are exposed
// via the UDL.
impl From<anyhow::Error> for LoginsApiError {
fn from(value: anyhow::Error) -> Self {
LoginsApiError::UnexpectedLoginsApiError {
Expand Down
9 changes: 5 additions & 4 deletions components/logins/src/logins.udl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,16 +324,17 @@ interface LoginStore {
void shutdown();
};

/// The Desktop-facing bridged sync engine. The canonical docs are in
/// https://searchfox.org/mozilla-central/source/services/interfaces/mozIBridgedSyncEngine.idl
/// The Desktop-facing bridged sync engine - a thin wrapper over the
/// `sync15::engine::SyncEngine` implemented by this component (see
/// `sync15::engine::BridgedEngineWrapper`).
/// It's only actually used on Desktop, but it's fine to expose this everywhere.
/// NOTE: all timestamps here are milliseconds.
interface LoginsBridgedEngine {
[Throws=LoginsApiError]
i64 last_sync();

[Throws=LoginsApiError]
void set_last_sync(i64 last_sync);
void reset_last_sync();

[Throws=LoginsApiError]
string? sync_id();
Expand All @@ -351,7 +352,7 @@ interface LoginsBridgedEngine {
void store_incoming(sequence<string> incoming_envelopes_as_json);

[Throws=LoginsApiError]
sequence<string> apply();
sequence<string> apply(i64 server_modified_millis);

[Throws=LoginsApiError]
void set_uploaded(i64 new_timestamp, sequence<string> uploaded_ids);
Expand Down
60 changes: 10 additions & 50 deletions components/logins/src/sync/bridge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ use crate::sync::engine::LoginsSyncEngine;
use crate::LoginStore;
use anyhow::Result;
use std::sync::Arc;
use sync15::engine::BridgedEngineAdaptor;
use sync15::ServerTimestamp;

impl LoginStore {
/// Returns a bridged sync engine for Desktop for this store.
Expand All @@ -18,55 +16,15 @@ impl LoginStore {
/// `LoginsApiError` through `From<anyhow::Error>`.
pub fn bridged_engine(self: Arc<Self>) -> Result<Arc<LoginsBridgedEngine>> {
let engine = LoginsSyncEngine::new(self)?;
let bridged_engine = LoginsBridgedEngineAdaptor { engine };
Ok(Arc::new(LoginsBridgedEngine::new(Box::new(bridged_engine))))
}
}

/// `LoginsSyncEngine` only implements the internal `sync15::SyncEngine` trait,
/// which is what the mobile (Android/iOS) sync manager drives. Desktop's Sync
/// framework instead speaks the `mozIBridgedSyncEngine` interface, whose Rust
/// shape is `sync15::BridgedEngine`. This adaptor wraps our `SyncEngine` and,
/// via the blanket `impl<A: BridgedEngineAdaptor> BridgedEngine for A`, gives
/// us a `BridgedEngine` for free. The adaptor exists only because these two
/// sync-engine traits still live side by side; it can go away if they're ever
/// unified.
struct LoginsBridgedEngineAdaptor {
engine: LoginsSyncEngine,
}

/// see sync15/src/engine/bridged_engine.rs for required functions for the trait
impl BridgedEngineAdaptor for LoginsBridgedEngineAdaptor {
fn last_sync(&self) -> Result<i64> {
// `get_last_sync` takes the `&LoginDb` to avoid deadlocking when called
// mid-sync (while the lock is already held). The bridge methods are
// always called outside a sync transaction, so we can lock here.
let db = self.engine.store.lock_db()?;
Ok(self
.engine
.get_last_sync(&db)?
.unwrap_or_default()
.as_millis())
}

fn set_last_sync(&self, last_sync_millis: i64) -> Result<()> {
let db = self.engine.store.lock_db()?;
self.engine
.set_last_sync(&db, ServerTimestamp::from_millis(last_sync_millis))?;
Ok(())
}

fn engine(&self) -> &dyn sync15::engine::SyncEngine {
&self.engine
Ok(Arc::new(LoginsBridgedEngine::new(Box::new(engine))))
}
}

// The UniFFI-exposed `LoginsBridgedEngine` (a thin newtype around
// `sync15::engine::BridgedEngineWrapper`) is generated by this macro, which
// removes the facade + BSO marshalling boilerplate that used to live here.
// logins' `set_uploaded` UDL row is `sequence<string>`, so the id element type
// is `String`. See services/interfaces/mozIBridgedSyncEngine.idl for the contract.
sync15::uniffi_bridged_engine!(LoginsBridgedEngine, String);
// removes the facade + BSO marshalling boilerplate. The wrapper drives our
// `LoginsSyncEngine`'s `SyncEngine` impl directly.
sync15::uniffi_bridged_engine!(LoginsBridgedEngine);

#[cfg(not(feature = "keydb"))]
#[cfg(test)]
Expand All @@ -89,7 +47,7 @@ mod tests {

// Fresh DB: never synced.
assert_eq!(bridge.last_sync().unwrap(), 0);
bridge.set_last_sync(3).unwrap();
bridge.set_uploaded(3, vec![]).unwrap();
assert_eq!(bridge.last_sync().unwrap(), 3);

assert!(bridge.sync_id().unwrap().is_none());
Expand All @@ -98,14 +56,16 @@ mod tests {
assert_eq!(bridge.sync_id().unwrap(), Some("some_guid".to_string()));
// changing the sync ID should reset the timestamp
assert_eq!(bridge.last_sync().unwrap(), 0);
bridge.set_last_sync(3).unwrap();
// Advance the engine-owned last_sync
bridge.set_uploaded(3, vec![]).unwrap();

bridge.reset_sync_id().unwrap();
// should now be a random guid.
assert_ne!(bridge.sync_id().unwrap(), Some("some_guid".to_string()));
// should have reset the last sync timestamp.
assert_eq!(bridge.last_sync().unwrap(), 0);
bridge.set_last_sync(3).unwrap();
// Advance the engine-owned last_sync
bridge.set_uploaded(3, vec![]).unwrap();

// `reset` clears the guid and the timestamp
bridge.reset().unwrap();
Expand Down Expand Up @@ -162,7 +122,7 @@ mod tests {

// Applying stores the remote record locally and returns the local-only
// login for upload.
let outgoing = bridge.apply().expect("should apply");
let outgoing = bridge.apply(0).expect("should apply");
let changes: HashMap<String, serde_json::Value> = outgoing
.into_iter()
.map(|s| {
Expand Down
Loading