1408108 - Rename "Sync ID" to "record ID" in the bookmarks engine and `PlacesSyncUtils`

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Lina Butler (ex-Mozilla)]

A synced bookmark's "sync ID" is just the Places GUID, except for the roots, where it's either the root name without trailing underscores for syncable roots, or "places" for "root________".

"Sync ID" in the rest of Sync means something different, though: it's a random ID stored in meta/global, for every engine and for the client. On every sync, we check if it's different than what we have locally, and, if so, discard all local state and start over as if it's a first sync.

Let's clean this up and rename the bookmark sync ID to something like "record ID" or "Sync record ID".

Comment 1

[Manish [:manishkk] [On Break]]

Hi, I would like to work on this as my first bug.
kit: any pointers as to where I should start?
I have already set up the dev environment and from what I can gather from the description I think I need to change sync ID in bookmarks.js and other places accordingly here: https://hg.mozilla.org/mozilla-central/file/tip/services/sync/modules/engines/bookmarks.js#l128

Thanks!

Comment 2

[Lina Butler (ex-Mozilla)]

Hi Manish, welcome, and thanks for picking this bug up! There are a few files we'll want to change:

* `bookmarks.js`, which you already found - https://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/services/sync/modules/engines/bookmarks.js#128-129,142,231,326,704-705,707,721-722,724,730-731,733,735,807,812,944-945,950
* `bookmark_repair.js` - https://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/services/sync/modules/bookmark_repair.js#640-641,650,670
* `bookmark_validator.js` - https://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/services/sync/modules/bookmark_validator.js#295,446
* One use in TPS - https://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm#930-931
* Most of the functions in `PlacesSyncUtils.jsm` - https://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/toolkit/components/places/PlacesSyncUtils.jsm
* The validators in `PlacesUtils.jsm` - https://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/toolkit/components/places/PlacesUtils.jsm#260-263.

In the `services/sync` files, let's replace `syncId` and `parentSyncId` with `recordId` and `parentRecordId`. Let's also rename the `syncID` variables to just `id`; I think that's what the other engines use. Finally, we'll need to fix up all mentions of `syncId` in the tests: https://searchfox.org/mozilla-central/search?q=%5BSs%5DyncId&case=true&regexp=true&path=services%2Fsync%2Ftests%2Fun You can run `./mach xpcshell-test services/sync` to make sure they still pass.

`PlacesSyncUtils` is the big one. As with `bookmarks.js`, let's rename `syncId` to `recordId` everywhere, including the method names (`guidToSyncId` becomes `guidToRecordId`, `fetchChildSyncIds` becomes `fetchChildRecordIds`, and so on), and comments. The only test we'll need to update for this is https://searchfox.org/mozilla-central/source/toolkit/components/places/tests/unit/test_sync_utils.js.

It's a pretty mechanical replace, there are just a lot of different files in different parts of the repo. :-) Feel free to ping me if you have questions. Thanks again!

Comment 3

[Manish [:manishkk] [On Break]]

kit: Thanks for this info.

I would like to work on this. 
can you assign this bug to me?

Comment 4

[Lina Butler (ex-Mozilla)]

Of course! We usually assign a bug after a patch has been uploaded, so please feel free to start working on this, and upload a first cut for feedback. We'll then assign the bug to you, and help you get it ready for landing.

Comment 5

[Manish [:manishkk] [On Break]]

Thanks, I will work on this weekend.

Comment 6

[Manish [:manishkk] [On Break]]

Attachment: Patch

Please once check this and let me know next step.

Thanking you in advance.

Comment 7

[Lina Butler (ex-Mozilla)]

Comment on attachment 8921407 [details] [diff] [review]
Patch

Review of attachment 8921407 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work, Manish; this is looking great! I see a few test failures when I run `./mach xpcshell-test services/sync`, but those should be an easy fix. Also, please update `toolkit/components/places/tests/unit/test_sync_utils.js` with the new method names. Once you fix those up and push a revised patch, we can get this landed. \o/

::: services/sync/modules/bookmark_repair.js
@@ +637,4 @@
>      let engine = this.service.engineManager.get("bookmarks");
>      // Determine every item that may be impacted by the requested IDs - eg,
>      // this may include children if a requested ID is a folder.
> +    // Turn an array of { syncId, syncable } into a map of recordId -> syncable.

Nit: `syncId` => `recordId` here, too.

::: services/sync/modules/engines/bookmarks.js
@@ +323,4 @@
>      for (let [node, parent] of walkBookmarksRoots(tree)) {
>        await maybeYield();
>        let {guid, type: placeType} = node;
> +      guid = PlacesSyncUtils.bookmarks.guidRecordId(guid);

Typo: `guidRecordId` => `guidToRecordId`. I think this should fix all the Sync test failures, too.

::: services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm
@@ +927,4 @@
>      } else {
>        expected_pos = this.props.last_item_pos + 1;
>      }
> +    // Note these are id instead of GUIDs, but that's ok here.

Nit: "these are IDs instead of GUIDs"

::: toolkit/components/places/PlacesSyncUtils.jsm
@@ +295,1 @@
>      return ROOT_GUID_TO_SYNC_ID[guid] || guid;

While you're here, please rename "ROOT_GUID_TO_SYNC_ID" to "ROOT_GUID_TO_RECORD_ID".

@@ +298,5 @@
>    /**
>     * Converts a Sync record ID to a Places GUID.
>     */
> +  recordIdToGuid(recordId) {
> +    return ROOT_SYNC_ID_TO_GUID[recordId] || recordId;

Ditto, rename to "ROOT_RECORD_ID_TO_GUID".

@@ +884,4 @@
>     * the following properties, depending on the item's kind:
>     *
>     *  - kind (all): A string representing the item's kind.
> +   *  - recordId (all): The item's sync ID.

Nit: please change "sync ID" to "record ID" in the comments, too.

::: toolkit/components/places/PlacesUtils.jsm
@@ +261,1 @@
>                                    (PlacesSyncUtils.bookmarks.ROOTS.includes(v) ||

Nit: Please update the indentation here and on the next line, so that the `(` lines up with the `t` on the first line.

Comment 8

[Lina Butler (ex-Mozilla)]

Assigning this bug to you. Also, congrats on the first patch! :-)

Comment 9

[Manish [:manishkk] [On Break]]

Kit: Thanks I will fix remaining things.

Comment 10

[Manish [:manishkk] [On Break]]

Attachment: patch

please review this patch

Comment 11

[Lina Butler (ex-Mozilla)]

Comment on attachment 8921845 [details] [diff] [review]
patch

Hmm, this looks identical to the first patch. Did you mean to upload a different one?

Comment 12

[Manish [:manishkk] [On Break]]

yes! last patch is updated one. Sorry for same name file.

Comment 13

[Lina Butler (ex-Mozilla)]

(In reply to Manish Kumar from comment #12)
> yes! last patch is updated one. Sorry for same name file.

Oh, sorry, I meant the last patch you uploaded (https://bug1408108.bmoattachments.org/attachment.cgi?id=8921845) doesn't include any of the fixes I requested in comment 7. It looks identical to the first patch (https://bug1408108.bmoattachments.org/attachment.cgi?id=8921407).

Comment 14

[Manish [:manishkk] [On Break]]

Attachment: latest_patch

Kit: am attaching a new patch which incorporates the changes you suggested and also I've updated the tests and now all the tests seem to passing when I run ./mach xpcshell-test services/sync

Comment 15

[Lina Butler (ex-Mozilla)]

Comment on attachment 8923208 [details] [diff] [review]
latest_patch

Review of attachment 8923208 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Manish, almost there! You need to update `toolkit/components/places/tests/unit/test_sync_utils.js`, too, so that `./mach xpcshell-test toolkit/components/places/tests/unit/test_sync_utils.js` passes.

Comment 16

[Manish [:manishkk] [On Break]]

I changed GUIDs in IDs.
Should I changes 'guids' also in 'ids'?

Comment 17

[Lina Butler (ex-Mozilla)]

(In reply to Manish Kumar from comment #16)
> Should I changes 'guids' also in 'ids'?

No, not in `test_sync_utils.js`.

Comment 18

[Manish [:manishkk] [On Break]]

After running test pass, getting this error but nothing is relevant in line #587

LOG: Thread-1 ERROR Unexpected exception Error: BookmarkSyncUtils: insert: The following properties were expected: parentRecordId at resource://gre/modules/PlacesUtils.jsm:587

Comment 19

[Lina Butler (ex-Mozilla)]

(In reply to Manish Kumar from comment #18)
> LOG: Thread-1 ERROR Unexpected exception Error: BookmarkSyncUtils: insert:
> The following properties were expected: parentRecordId at
> resource://gre/modules/PlacesUtils.jsm:587

Which test in `test_sync_utils.js` gives you that error?

Comment 20

[Manish [:manishkk] [On Break]]

Attachment: patch_final

Comment 21

[Lina Butler (ex-Mozilla)]

Comment on attachment 8925531 [details] [diff] [review]
patch_final

Thanks for your work on this, Manish! I pushed to our try server for you, fixed up the remaining test failure (https://treeherder.mozilla.org/#/jobs?repo=try&revision=2465aea1eddd0064b936c47384652a14e4e5aeb4), and landed on our integration branch.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4864989da37e
Rename "Sync ID" to "record ID" in the bookmarks engine and `PlacesSyncUtils`. r=kitcambridge

Comment 23

[Manish [:manishkk] [On Break]]

Thanks kit.

Comment 24

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4864989da37e