1201110 - Schema changes to support bookmark sync

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Richard Newman [:rnewman]]

We need to track local changes, just as we did for passwords in Bug 1171657.

Comment 1

[Richard Newman [:rnewman]]

Interested parties can follow along here.

https://github.com/mozilla/firefox-ios/pull/1224

Goddamn favicons.

Comment 2

[Richard Newman [:rnewman]]

Note to self: structure extraction will need is_overridden; make sure this is indexed, probably a partial index.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Pull req.

High-level feedback and questions, please.


This is the same approach we take to logins -- the idea of a local overlay on a reconciled mirror -- extended to bookmarks.

See <https://gist.github.com/rnewman/f95720050df3d5515351#gistcomment-1566012> for some notes.

Bookmarks are much more complicated than logins:

* There are more types and more fields.
* We have structure as well as state.
* We need to buffer downloaded records.

For these reasons we end up with six tables (three pairs of value + structure). The 'mirror' table introduced in Bug 1201104 is renamed to 'buffer', because that's what it really is.

This PR implements upgrading (albeit without a test yet) and new DB creation. It implements adding a new local bookmark and isBookmarked. We preserve our previous sad icon support for local bookmarks.

Notable omissions:

* Syncing of any kind. Downloaded records should be applied correctly to the buffer (though I haven't checked yet), but those won't end up being merged into the mirror. As a result they won't be displayed, but that would be a relatively easy change if we wanted to get this landed -- just point at (buffer + local) instead of (mirror + local).

We can create views for these, making that a one-line switch.

* Deletion and clearing. This is rote work that I haven't got to yet. It'll 'work' right now, but it won't work with syncing.

* Indexing of tables.


The approach we'll use when implementing syncing itself is a merger that can compare (buffer + mirror) to figure out server changes, and (local + mirror) to get local changes, and can lazily compare subtrees to yield a new merged state. The production of a merged state gives us a stream of changes that can be applied to the server and to the local (mirror + buffer + local) tables, typically putting everything in (mirror) with empty (buffer) and (local).

That, as you can imagine, is a whole other bug -- Bug 1141850.

Comment 4

[Stephan Leroux [:sleroux]]

The code itself makes sense but I'm having a hard time understanding how the big picture works. If I understand it correctly,

1. We have a local, mirror (remote), and buffer table for both the Bookmarks themselves and the Bookmark hierarchy (structure).
2. Incoming changes from the server get received in the buffer
3. Conflict resolution happens when merging between the buffered data and the mirror (remote) table.

For clarification, could you go through some scenarios from a high level that happens when,

1. A local bookmark is changed
2. A remote bookmark is changed on the client side
3. A remote bookmark is changed on the server from sync

Comment 5

[Richard Newman [:rnewman]]

Yup, I'll put together another gist.

Comment 6

[Emily Toop (:fluffyemily)]

Comment on attachment 8689915 [details] [review]
Pull req.

If I am understanding this correctly, this feels like a much simpler and more logical separation of remote and local bookmarks which will make dealing with them a lot easier in future.

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #5)
> Yup, I'll put together another gist.

I'm quite comfortable with the schema, but I'll need this gist as well.  My two big questions:

1) What do we gain from maintaining the structure as a separate table?  It appears isomorphic to keeping additional fields in the existing tables to me, and requires cross-table transactions to be safe.  Right?

2) Explain is_overridden.  The code is too "in flight" for me to work this out, and I did try.

Comment 8

[Richard Newman [:rnewman]]

Nick raised an excellent question in his PR comments.

+extension SQLiteBookmarks {
-    public func addToMobileBookmarks(url: NSURL, title: String, favicon: Favicon?) -> Success {
+    /**
+     * Assumption: the provided GUID exists in either the local table or the mirror table.

> If not, what happens?


If the GUID is entirely unknown, the insert into the structure table will fail because the foreign key constraint will be violated.

If the GUID is present in the target table but the destination is not a folder, the newly inserted bookmark is basically lost to the world for navigation — it doesn't have a meaningful parent, so it's never re-found, but the page will show as bookmarked.

In the course of answering this comment I noticed that each structure table permits only GUIDs that are already present in the appropriate 'values' table — that is, mirror structure can only refer to mirror records (which makes sense), and the local structure can only refer to local records.

Either we need to loosen that constraint for local records — they can refer to mirror folders — or we need to answer my TODO below with "we need to copy it so we can refer to the folder". I think the former makes more sense.

Note that this constraint also makes it impossible to represent in the mirror structure table bookmarks that have no known parent. It's not an issue for the buffer tables, because by definition the structure is carried by a non-deleted parent.

Comment 9

[Richard Newman [:rnewman]]

A quick progress update:

Lots of changes in the PR. More TODOs, but many more TODOs eliminated.

A mini blog post that describes the current structure is here:

https://gist.github.com/rnewman/82c17460161047db38d5

I've tested migration of existing data. I ripped out most of the merged bookmark view, replacing it with the overlay-based view onto local + mirror. Eventually it will go entirely.

Having manually schlepped buffer data into the mirror:

  insert into bookmarksMirror (id, guid, type, server_modified, is_deleted, hasDupe, parentid, parentName, feedUri, siteUri, pos, title, description, bmkUri, tags, keyword, folderName, queryId) select * from bookmarksBuffer;
  insert into bookmarksMirrorStructure select * from bookmarksBufferStructure;
  insert or ignore into bookmarksMirrorStructure select * from bookmarksLocalStructure;
  delete from bookmarksLocalStructure;
  delete from bookmarksLocal;

the new bookmark view correctly shows Desktop Bookmarks.

I've also mostly implemented separators and stubs for queries and livemarks, and I've tested bookmarking of new items into an existing synced Mobile Bookmarks folder. Everything's working nicely.

Next up is implementing a testing framework, new tests for various kinds of structural and value-based reconciling and conflict resolution, and then the merger that makes those tests pass.

Because the merged view was getting in the way and was consequently ripped out, I won't land this PR until I've built the merger -- to do so would remove access to synced bookmarks altogether -- but we can finish the review process when everyone's back from Orlando.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8689915 [details] [review]
Pull req.

I promised you some pictures to go with these, so that's next. In the mean time, the textual explanation is here:

https://gist.github.com/rnewman/82c17460161047db38d5

and if you'd like me to give you a spoken/guided romp through these 27 commits, I'd be happy to do so.

Tests pass and the browser works with this sequence applied. Notably you won't be able to see your desktop bookmarks until I implement Bug 1141850.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8689915 [details] [review]
Pull req.

Holy crap, long PR series is long.  I reviewed up to and including part J.

I tried to refrain from nits; in general, you and I have very similar styles anyway :)

I had a few substantive questions, but nothing critical.  There's so much to be built on top of this; we'll have time to wiggle out some tricky cases.

Comment 12

[Richard Newman [:rnewman]]

4956641