diff --git a/toolkit/components/places/SyncedBookmarksMirror.jsm b/toolkit/components/places/SyncedBookmarksMirror.jsm index 632872767965..7d9a91ef6075 100644 --- a/toolkit/components/places/SyncedBookmarksMirror.jsm +++ b/toolkit/components/places/SyncedBookmarksMirror.jsm @@ -81,7 +81,7 @@ const DB_TITLE_LENGTH_MAX = 4096; // The current mirror database schema version. Bump for migrations, then add // migration code to `migrateMirrorSchema`. -const MIRROR_SCHEMA_VERSION = 8; +const MIRROR_SCHEMA_VERSION = 7; const DEFAULT_MAX_FRECENCIES_TO_RECALCULATE = 400; @@ -1514,15 +1514,6 @@ async function migrateMirrorSchema(db, currentSchemaVersion) { await db.execute(`CREATE INDEX IF NOT EXISTS mirror.structurePositions ON structure(parentGuid, position)`); } - if (currentSchemaVersion < 8) { - // Not really a "schema" update, but addresses the defect from bug 1635859. - // In short, every bookmark with a corresponding entry in the mirror should - // have syncStatus = NORMAL. - await db.execute(`UPDATE moz_bookmarks AS b - SET syncStatus = ${PlacesUtils.bookmarks.SYNC_STATUS.NORMAL} - WHERE EXISTS (SELECT 1 FROM mirror.items - WHERE guid = b.guid)`); - } } /** diff --git a/toolkit/components/places/bookmark_sync/src/store.rs b/toolkit/components/places/bookmark_sync/src/store.rs index 2f90e1a6162d..9cb7300187d6 100644 --- a/toolkit/components/places/bookmark_sync/src/store.rs +++ b/toolkit/components/places/bookmark_sync/src/store.rs @@ -26,19 +26,6 @@ fn total_sync_changes() -> i64 { unsafe { NS_NavBookmarksTotalSyncChanges() } } -// Return all the non-root-roots as a 'sql set' (ie, suitable for use in an -// IN statement) -fn user_roots_as_sql_set() -> String { - format!( - "('{0}', '{1}', '{2}', '{3}', '{4}')", - dogear::MENU_GUID, - dogear::MOBILE_GUID, - dogear::TAGS_GUID, - dogear::TOOLBAR_GUID, - dogear::UNFILED_GUID - ) -} - pub struct Store<'s> { db: &'s mut Conn, driver: &'s Driver, @@ -92,11 +79,15 @@ impl<'s> Store<'s> { ) AND NOT EXISTS( SELECT 1 FROM moz_bookmarks b JOIN moz_bookmarks p ON p.id = b.parent - WHERE b.guid IN {user_roots} AND - p.guid <> '{root}' + WHERE b.guid IN ('{1}', '{2}', '{3}', '{4}', '{5}') AND + p.guid <> '{0}' )", - root = dogear::ROOT_GUID, - user_roots = user_roots_as_sql_set(), + dogear::ROOT_GUID, + dogear::MENU_GUID, + dogear::MOBILE_GUID, + dogear::TAGS_GUID, + dogear::TOOLBAR_GUID, + dogear::UNFILED_GUID, ))?; let has_valid_roots = match statement.step()? { Some(row) => row.get_by_index::(0)? == 1, @@ -900,33 +891,18 @@ fn apply_remote_items(db: &Conn, driver: &Driver, controller: &AbortController) /* The last modified date should always be newer than the date added, so we pick the newer of the two here. */ MAX(lastModifiedMicroseconds, remoteDateAddedMicroseconds), - {syncStatusNormal}, 0 + {}, 0 FROM itemsToApply WHERE 1 ON CONFLICT(id) DO UPDATE SET title = excluded.title, dateAdded = excluded.dateAdded, lastModified = excluded.lastModified, - syncStatus = {syncStatusNormal}, - syncChangeCounter = 0, /* It's important that we update the URL *after* removing old keywords and *before* inserting new ones, so that the above DELETEs select the correct affected items. */ fk = excluded.fk", - syncStatusNormal = nsINavBookmarksService::SYNC_STATUS_NORMAL - ))?; - // The roots are never in `itemsToApply` but still need to (well, at least - // *should*) have a syncStatus of NORMAL after a reconcilliation. The - // ROOT_GUID doesn't matter in practice, but we include it to be consistent. - db.exec(format!( - "UPDATE moz_bookmarks SET - syncStatus={syncStatusNormal} - WHERE guid IN {user_roots} OR - guid = '{root}' - ", - syncStatusNormal = nsINavBookmarksService::SYNC_STATUS_NORMAL, - root = dogear::ROOT_GUID, - user_roots = user_roots_as_sql_set(), + nsINavBookmarksService::SYNC_STATUS_NORMAL ))?; // Flag frecencies for recalculation. This is a multi-index OR that uses the diff --git a/toolkit/components/places/tests/sync/head_sync.js b/toolkit/components/places/tests/sync/head_sync.js index 37639eb557ad..7f5c00a617df 100644 --- a/toolkit/components/places/tests/sync/head_sync.js +++ b/toolkit/components/places/tests/sync/head_sync.js @@ -66,7 +66,7 @@ const MobileBookmarksTitle = "mobile"; function run_test() { let bufLog = Log.repository.getLogger("Sync.Engine.Bookmarks.Mirror"); - bufLog.level = Log.Level.All; + bufLog.level = Log.Level.Error; let sqliteLog = Log.repository.getLogger("Sqlite"); sqliteLog.level = Log.Level.Error; diff --git a/toolkit/components/places/tests/sync/test_bookmark_mirror_migration.js b/toolkit/components/places/tests/sync/test_bookmark_mirror_migration.js index 9951983f7853..3499846bfa6a 100644 --- a/toolkit/components/places/tests/sync/test_bookmark_mirror_migration.js +++ b/toolkit/components/places/tests/sync/test_bookmark_mirror_migration.js @@ -2,7 +2,7 @@ * http://creativecommons.org/publicdomain/zero/1.0/ */ // Keep in sync with `SyncedBookmarksMirror.jsm`. -const CURRENT_MIRROR_SCHEMA_VERSION = 8; +const CURRENT_MIRROR_SCHEMA_VERSION = 7; // The oldest schema version that we support. Any databases with schemas older // than this will be dropped and recreated. @@ -158,71 +158,3 @@ add_task(async function test_database_corrupt() { ok(buf.wasCorrupt, "Opening corrupt database should mark it as such"); await buf.finalize(); }); - -add_task(async function test_migrate_v8() { - let buf = await openMirror("test_migrate_v8"); - - await PlacesUtils.bookmarks.insertTree({ - guid: PlacesUtils.bookmarks.menuGuid, - children: [ - { - guid: "bookmarkAAAA", - url: "http://example.com/a", - title: "A", - }, - { - guid: "bookmarkBBBB", - url: "http://example.com/b", - title: "B", - }, - ], - }); - - await buf.db.execute( - `UPDATE moz_bookmarks - SET syncChangeCounter = 0, - syncStatus = ${PlacesUtils.bookmarks.SYNC_STATUS.NEW}` - ); - - // setup the mirror. - await storeRecords(buf, [ - { - id: "bookmarkAAAA", - parentid: "menu", - type: "bookmark", - title: "B", - bmkUri: "http://example.com/b", - }, - { - id: "menu", - parentid: "places", - type: "folder", - children: [], - }, - ]); - - await buf.db.setSchemaVersion(7, "mirror"); - await buf.finalize(); - - // reopen it. - buf = await openMirror("test_migrate_v8"); - Assert.equal(await buf.db.getSchemaVersion("mirror"), 8, "did upgrade"); - - let fields = await PlacesTestUtils.fetchBookmarkSyncFields( - "bookmarkAAAA", - "bookmarkBBBB", - PlacesUtils.bookmarks.menuGuid - ); - let [fieldsA, fieldsB, fieldsMenu] = fields; - - // 'A' was in the mirror - should now be _NORMAL - Assert.equal(fieldsA.guid, "bookmarkAAAA"); - Assert.equal(fieldsA.syncStatus, PlacesUtils.bookmarks.SYNC_STATUS.NORMAL); - // 'B' was not in the mirror so should be untouched. - Assert.equal(fieldsB.guid, "bookmarkBBBB"); - Assert.equal(fieldsB.syncStatus, PlacesUtils.bookmarks.SYNC_STATUS.NEW); - // 'menu' was in the mirror - should now be _NORMAL - Assert.equal(fieldsMenu.guid, PlacesUtils.bookmarks.menuGuid); - Assert.equal(fieldsMenu.syncStatus, PlacesUtils.bookmarks.SYNC_STATUS.NORMAL); - await buf.finalize(); -}); diff --git a/toolkit/components/places/tests/sync/test_bookmark_reconcile.js b/toolkit/components/places/tests/sync/test_bookmark_reconcile.js deleted file mode 100644 index 218e84beb640..000000000000 --- a/toolkit/components/places/tests/sync/test_bookmark_reconcile.js +++ /dev/null @@ -1,191 +0,0 @@ -// Get bookmarks which aren't marked as normally syncing and with no pending -// changes. -async function getBookmarksNotMarkedAsSynced() { - let db = await PlacesUtils.promiseDBConnection(); - let rows = await db.executeCached( - ` - SELECT guid, syncStatus, syncChangeCounter FROM moz_bookmarks - WHERE syncChangeCounter > 1 OR syncStatus != :syncStatus - ORDER BY guid - `, - { syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL } - ); - return rows.map(row => { - return { - guid: row.getResultByName("guid"), - syncStatus: row.getResultByName("syncStatus"), - syncChangeCounter: row.getResultByName("syncChangeCounter"), - }; - }); -} - -add_task(async function test_reconcile_metadata() { - let buf = await openMirror("test_reconcile_metadata"); - - let olderDate = new Date(Date.now() - 100000); - info("Set up local tree"); - await PlacesUtils.bookmarks.insertTree({ - guid: PlacesUtils.bookmarks.menuGuid, - children: [ - { - // this folder is going to reconcile exactly - guid: "folderAAAAAA", - type: PlacesUtils.bookmarks.TYPE_FOLDER, - title: "A", - children: [ - { - guid: "bookmarkBBBB", - url: "http://example.com/b", - title: "B", - }, - ], - }, - { - // this folder's existing child isn't on the server (so will be - // outgoing) and also will take a new child from the server. - guid: "folderCCCCCC", - type: PlacesUtils.bookmarks.TYPE_FOLDER, - title: "C", - children: [ - { - guid: "bookmarkEEEE", - url: "http://example.com/e", - title: "E", - }, - ], - }, - { - // This bookmark is going to take the remote title. - guid: "bookmarkFFFF", - url: "http://example.com/f", - title: "f", - dateAdded: olderDate, - lastModified: olderDate, - }, - ], - }); - // And a single, local-only bookmark in the toolbar. - await PlacesUtils.bookmarks.insertTree({ - guid: PlacesUtils.bookmarks.toolbarGuid, - children: [ - { - guid: "bookmarkTTTT", - url: "http://example.com/t", - title: "in the toolbar", - dateAdded: olderDate, - lastModified: olderDate, - }, - ], - }); - // Reset to prepare for our reconciled sync. - await PlacesSyncUtils.bookmarks.reset(); - // setup the mirror. - await storeRecords( - buf, - shuffle([ - { - id: "menu", - parentid: "places", - type: "folder", - children: ["folderAAAAAA", "folderCCCCCC", "bookmarkFFFF"], - modified: Date.now() / 1000 - 60, - }, - { - id: "folderAAAAAA", - parentid: "menu", - type: "folder", - title: "A", - children: ["bookmarkBBBB"], - modified: Date.now() / 1000 - 60, - }, - { - id: "bookmarkBBBB", - parentid: "folderAAAAAA", - type: "bookmark", - title: "B", - bmkUri: "http://example.com/b", - modified: Date.now() / 1000 - 60, - }, - { - id: "folderCCCCCC", - parentid: "menu", - type: "folder", - title: "C", - children: ["bookmarkDDDD"], - modified: Date.now() / 1000 - 60, - }, - { - id: "bookmarkDDDD", - parentid: "folderCCCCCC", - type: "bookmark", - title: "D", - bmkUri: "http://example.com/d", - modified: Date.now() / 1000 - 60, - }, - { - id: "bookmarkFFFF", - parentid: "menu", - type: "bookmark", - title: "F", - bmkUri: "http://example.com/f", - dateAdded: olderDate, - modified: Date.now() / 1000 + 60, - }, - { - id: "toolbar", - parentid: "places", - type: "folder", - children: [], - index: 1, - }, - { - id: "unfiled", - parentid: "places", - type: "folder", - children: [], - index: 3, - }, - ]) - ); - info("Applying"); - let changesToUpload = await buf.apply(); - // We need to upload a bookmark and the parent as they didn't exist on the - // server. Since we always use the local state for roots (bug 1472241), we'll - // reupload them too. - let idsToUpload = inspectChangeRecords(changesToUpload); - deepEqual( - idsToUpload, - { - updated: [ - "bookmarkEEEE", - "bookmarkTTTT", - "folderCCCCCC", - "menu", - "mobile", - "toolbar", - "unfiled", - ], - deleted: [], - }, - "Should upload the 2 local-only bookmarks and their parents" - ); - // Check it took the remote thing we were expecting. - Assert.equal((await PlacesUtils.bookmarks.fetch("bookmarkFFFF")).title, "F"); - // Most things should be synced and have no change counter. - let badGuids = await getBookmarksNotMarkedAsSynced(); - Assert.deepEqual(badGuids, [ - { - // The bookmark that was only on the server. Still have SYNC_STATUS_NEW - // as it's yet to be uploaded. - guid: "bookmarkEEEE", - syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NEW, - syncChangeCounter: 1, - }, - { - // This bookmark is local only so is yet to be uploaded. - guid: "bookmarkTTTT", - syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NEW, - syncChangeCounter: 1, - }, - ]); -}); diff --git a/toolkit/components/places/tests/sync/xpcshell.ini b/toolkit/components/places/tests/sync/xpcshell.ini index b8eaad5b6662..62f21b960e5c 100644 --- a/toolkit/components/places/tests/sync/xpcshell.ini +++ b/toolkit/components/places/tests/sync/xpcshell.ini @@ -18,7 +18,6 @@ support-files = [test_bookmark_mirror_meta.js] [test_bookmark_mirror_migration.js] [test_bookmark_observer_recorder.js] -[test_bookmark_reconcile.js] [test_bookmark_structure_changes.js] [test_bookmark_value_changes.js] [test_sync_utils.js]