storage.sync: first cut at the core sync infrastructure #2892

mhammond · 2020-04-01T03:27:44Z

Builds on #2810. This needs the trait from Lina's patch, but is setup to work with it. It implements the 3-way merge (although doesn't quite get deletions right yet, so there's still some work to do) - but comments welcome.

thomcc · 2020-04-01T22:26:43Z

Regarding how to do the two/three way merge, I'm pretty happy with how https://github.com/mozilla/application-services/blob/master/components/logins/src/update_plan.rs#L34-L60 works, and it's a high level description of the set of operations you perform for a three way merge.

https://github.com/mozilla/application-services/blob/master/components/logins/src/db.rs#L1061-L1114 is the code that calls it, and decides how to handle deletions vs missing mirror, etc. I think trying to handle it as you have is going to be tricky (but your comment says just about as much).

That said, handing things in bulk as this does is clearly not necessary, it could still be done piecemeal.

components/webext-storage/src/api.rs

components/webext-storage/src/schema.rs

components/webext-storage/src/sync/outgoing.rs

components/webext-storage/sql/create_schema.sql

components/webext-storage/src/sync/incoming.rs

rfk

Similar to my comments on #2810, I was poking around in here to see if I understood what's going on.

components/webext-storage/src/sync/outgoing.rs

components/webext-storage/src/api.rs

components/webext-storage/src/sync/incoming.rs

rfk · 2020-04-07T04:56:10Z

components/webext-storage/src/sync/mod.rs

+// XXX - this needs more thought, and probably needs significant changes.
+// Main problem is that it doesn't handle deletions - but to do that, we need
+// something other than a simple Option<JsonMap> - we need to differentiate
+// "doesn't exist" from "removed".


Naively, I would expect the three-way nature of three-way-merge to help us with deletions here. For example we could iterate over the keys in parent and find any that have been removed from other, indicating that they were deleted remotely.

Yes, we could do this, but TBH I'm not sure what the exact semantics are here - it's not documented, so we probably need to check how chrome behaves in these cases. I've just punted doing anything else here for now as hopefully @glasserc is tackling that as part of #2900

Summarizing the discussion from that thread, it sounds like Chrome's behavior is too coarse, and not really what we want...but also, the Kinto "client wins" strategy isn't right for us, and it's more important to maintain compatibility with what we had before than with Chrome's implementation. Is that accurate?

Since we have the mirror copy, @rfk's suggestion makes sense!

components/webext-storage/src/sync/outgoing.rs

linabutler

This looks great! I left some comments here and there, but I really like this overall!

components/webext-storage/src/api.rs

components/webext-storage/src/schema.rs

components/webext-storage/src/sync/incoming.rs

linabutler · 2020-04-08T05:03:08Z

components/webext-storage/src/sync/incoming.rs

+                }
+                (None, _, _) => {
+                    // Deleted remotely. Server wins.
+                    // XXX - WRONG - we want to 3 way merge here still!


components/webext-storage/src/sync/incoming.rs

linabutler · 2020-04-20T22:24:52Z

components/webext-storage/src/sync/mod.rs

+// XXX - need to work out how to consolidate "payload" vs "bso", particularly
+// the timestamp, etc. For now, we represent this in ServerPayload, even though
+// that's never actually stored in this way.
+// XXX - this isn't going to work without other changes.


Hmmm, yeah, we're going to need to pass that over in Desktop. I wonder if we can "just" pass the raw decrypted JSON over XPCOM, and have the Desktop integration layer parse it into a sync15_traits::Payload (which has id, deleted, and data already on it) before handing it over to webext_storage.

I spent a bit of time noodling on this, and the result is #3067 and https://phabricator.services.mozilla.com/D73581. Basically, we have this thing called an "envelope", which is BSO metadata fields + the unparsed payload. An incoming envelope has a payload() method, which you can use to get at the payload—which you can then either deserialize into a record, or check if it's a tombstone via is_deleted, like you can in a-s sync15.

I think we can land your patch as-is now, and, assuming you like the envelope idea, go back and tweak this after.

linabutler · 2020-04-20T22:29:54Z

components/webext-storage/src/sync/mod.rs

+// XXX - this needs more thought, and probably needs significant changes.
+// Main problem is that it doesn't handle deletions - but to do that, we need
+// something other than a simple Option<JsonMap> - we need to differentiate
+// "doesn't exist" from "removed".


Summarizing the discussion from that thread, it sounds like Chrome's behavior is too coarse, and not really what we want...but also, the Kinto "client wins" strategy isn't right for us, and it's more important to maintain compatibility with what we had before than with Chrome's implementation. Is that accurate?

Since we have the mirror copy, @rfk's suggestion makes sense!

components/webext-storage/src/sync/mod.rs

components/webext-storage/src/sync/outgoing.rs

components/webext-storage/src/sync/mod.rs

mhammond · 2020-04-28T03:38:26Z

phew! Thanks the the thorough review @linacambridge! I think I've addressed most review comments here - what's outstanding is:

Working out what the payload struct should look like - I figured it's best to do that as part of wiring up the sync bridge, and it should be quite easy to retrofit changes here.
A few outstanding items around merging, for which I'm going to wait for Ethan.

Currently, the only supported method is `wipe`, with all other methods returning `NotImplemented`. We can wire this up to Mark's syncing implementation in #2892. This also implements enough of the glue to where it can be vendored and used in Desktop.

linabutler

Thanks so much, @mhammond, this is fantastic work! 😍😍 I love the cleanup with DataState, and all the extra comments and tests! I think we should get this landed as soon as we can, and file follow-up bugs for anything else we need—and that way, we can target @glasserc's merging PR to master.

linabutler · 2020-05-03T22:26:22Z

components/webext-storage/src/sync/incoming.rs

+
+/// Details about an incoming item.
+#[derive(Debug, PartialEq)]
+pub struct IncomingItem {


That sounds perfect to me, thanks for taking a look and the thorough response! I agree that the alternatives are even worse, and the way you have this set up now is the cleanest.

linabutler · 2020-05-03T22:27:39Z

components/webext-storage/src/sync/incoming.rs

+/// record. This "state" is the input to calculating the IncomingAction.
+#[derive(Debug, PartialEq)]
+pub enum IncomingState {
+    /// There's an incoming item, but data for that extension doesn't exist


I love these comments, thank you so much for adding them! 😍

linabutler · 2020-05-03T22:31:25Z

components/webext-storage/src/sync/incoming.rs

+    IncomingOnly { incoming: DataState },
+    /// There's an incoming item and we have data for the same extension in
+    /// our local store - but not in our mirror. This should be relatively
+    /// uncoming as it means:


Nit: uncommon

linabutler · 2020-05-03T22:40:14Z

components/webext-storage/src/sync/mod.rs

+// XXX - need to work out how to consolidate "payload" vs "bso", particularly
+// the timestamp, etc. For now, we represent this in ServerPayload, even though
+// that's never actually stored in this way.
+// XXX - this isn't going to work without other changes.


I spent a bit of time noodling on this, and the result is #3067 and https://phabricator.services.mozilla.com/D73581. Basically, we have this thing called an "envelope", which is BSO metadata fields + the unparsed payload. An incoming envelope has a payload() method, which you can use to get at the payload—which you can then either deserialize into a record, or check if it's a tombstone via is_deleted, like you can in a-s sync15.

I think we can land your patch as-is now, and, assuming you like the envelope idea, go back and tweak this after.

components/webext-storage/src/sync/outgoing.rs

Currently, the only supported method is `wipe`, with all other methods returning `NotImplemented`. We can wire this up to Mark's syncing implementation in #2892. This also implements enough of the glue to where it can be vendored and used in Desktop.

mhammond requested review from thomcc and linabutler April 1, 2020 03:27

mhammond changed the title ~~First cut at the core sync infrastructure~~ storage.sync: first cut at the core sync infrastructure Apr 1, 2020

mhammond commented Apr 3, 2020

View reviewed changes

rfk reviewed Apr 7, 2020

View reviewed changes

linabutler requested changes Apr 20, 2020

View reviewed changes

glasserc reviewed Apr 22, 2020

View reviewed changes

components/webext-storage/src/sync/mod.rs Show resolved Hide resolved

linabutler changed the base branch from storage.sync-schema to master April 27, 2020 05:47

linabutler self-requested a review April 27, 2020 06:14

appsvc-moz mentioned this pull request Apr 29, 2020

Wire up the Golden Gate Sync bridge to Rust extension storage mozilla/application-services-bug-mirror#198

Closed

linabutler mentioned this pull request May 1, 2020

Implement a skeleton sync15::BridgedEngine for extension storage #3064

Merged

linabutler approved these changes May 3, 2020

View reviewed changes

mhammond force-pushed the storage.sync-sync branch from b51cbc6 to 151a36f Compare May 4, 2020 05:22

mhammond marked this pull request as ready for review May 4, 2020 05:22

mhammond added the checkin-needed Auto-merge this PR if there's an approved review and CI passes label May 4, 2020

Infrastructure for webext-storage syncing

8a38c70

mhammond force-pushed the storage.sync-sync branch from 151a36f to 8a38c70 Compare May 4, 2020 05:32

linabutler merged commit 5c52133 into master May 4, 2020

mhammond deleted the storage.sync-sync branch May 28, 2020 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage.sync: first cut at the core sync infrastructure #2892

storage.sync: first cut at the core sync infrastructure #2892

mhammond commented Apr 1, 2020

thomcc commented Apr 1, 2020

rfk left a comment

rfk Apr 7, 2020

mhammond Apr 9, 2020

linabutler Apr 20, 2020

linabutler left a comment

linabutler Apr 8, 2020

linabutler Apr 20, 2020

linabutler May 3, 2020

linabutler Apr 20, 2020

mhammond commented Apr 28, 2020

linabutler left a comment

linabutler May 3, 2020

linabutler May 3, 2020

linabutler May 3, 2020

linabutler May 3, 2020

storage.sync: first cut at the core sync infrastructure #2892

storage.sync: first cut at the core sync infrastructure #2892

Conversation

mhammond commented Apr 1, 2020

thomcc commented Apr 1, 2020

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linabutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhammond commented Apr 28, 2020

linabutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment