1363999 - Add "sync tracker" support to ProfileStorage

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Mark Hammond [:markh] [:mhammond]]

Sync needs to track various changes to profilestorage items so it can work out what to sync and when. We've prototyped this, but it should land as its own bug with its own tests. It will wait for Luke's refactor.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1363999 - Add sync metadata to formautofill records.

This patch lands the metadata and related functions to the ProfileStorage
necessary for Sync to function.

* New "public" functions which are intended to be used only by Sync:
** pullSyncChanges: Gets metadata about what Sync needs to upload to the
   server.
** pushSyncChanges: Apply the changes made by Sync.
** resetSync: Reset all Sync metadata, used when Sync is disconnected from a
   device.
** changeGUID to change the GUID of an item that has no sync metadata.

* New optional Sync metdata for a record.
  Once a record is marked as Syncing, it carries around a new _sync field.
  Currently this contains only a changeCounter. All records are marked as
  Syncing once pullSyncChanges is called - after this call, all local
  records will have Sync metadata.

* Changes to tombstones semantics: If an item carries no Sync metadata (ie,
  hasn't previously been synced), then no tombstone will be left behind when
  it is deleted.

* Many existing functions get an optional {includePrivateFields} option, which
  will include all fields with a leading underscore (_) - which obviously
  includes this new _sync field - to be returned in the record.

* Many existing functions get an optional {sourceSync} option, which
  indicates Sync itself is making the change. This significantly changes
  the semantics of some operations (eg, counters will not be incremented,
  tombstones can be resurrected, etc)

Review commit: https://reviewboard.mozilla.org/r/149748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149748/

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

The sync metadata we need - hopefully the commit message and tests are self-explanatory. For additional context, see https://github.com/mhammond/gecko/tree/formautofill

I think this is actually ready for review, but I fully expect some things you'd all like changing, so only requesting feedback for now.

(This is mostly Thom's work, but he's on PTO, so I thought I'd get it up anyway)

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review154436

::: browser/extensions/formautofill/ProfileStorage.jsm:219
(Diff revision 1)
> +        } else {
> +          throw new Error(`Record ${record.guid} already exists`);
> +        }
> +      }
> +      let recordToSave = Object.assign({
> +        // `timeLastUsed` and `timesUsed` are always local.

I think values from `record` will win here, and I think we carry them in sync (which we maybe shouldn't!)

::: browser/extensions/formautofill/ProfileStorage.jsm:223
(Diff revision 1)
> +      let recordToSave = Object.assign({
> +        // `timeLastUsed` and `timesUsed` are always local.
> +        timeLastUsed: 0,
> +        timesUsed: 0,
> +      }, record);
> +      return this._saveRecord(recordToSave, {sourceSync});

should we call `_recordWriteProcessor()` here? A normal "update" does (via normalizeRecord - but that won't allow us to specify a GUID)

::: browser/extensions/formautofill/ProfileStorage.jsm:482
(Diff revision 1)
> +      let tombstone = {
> +        guid,
> +        timeLastModified: Date.now(),
> +        deleted: true,
> +      };
> +      this._store.data[this._collectionName].push(tombstone);

the 2 lines below are still manipulating tombstone, so this should move below them for "cleanliness" sake :)

::: browser/extensions/formautofill/test/unit/test_storage_syncfields.js:64
(Diff revision 1)
> +  address = profileStorage.addresses.getAll({includePrivateFields: true})[0];
> +  equal(address._sync.changeCounter, 1);
> +});
> +
> +add_task(async function test_changeCounter_update() {
> +  // This test should probably also check .status, and need splitting so

also, there's no .status anymore. Probably just kill this test.

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review157634

The patch looks good to me and needs a rebase for ProfileStorage. The rebase task should be controlled in a reasonable and expected scope, so we need a discussion to figure out the order of Sync and ProfileStorage patches during all-hands.

::: browser/extensions/formautofill/ProfileStorage.jsm:246
(Diff revision 1)
> +    recordToSave.timesUsed = 0;
> +
> +    return this._saveRecord(recordToSave);
> +  }
> +
> +  _saveRecord(record, {sourceSync = false} = {}) {

`_saveRecord` seems for adding operation only. Do we need a more specific name for `add` only?

After saving a record, `_computeFields` (see bug 1368008, it was _recordReadProcessor) should be invoked to make sure the following `get` or `getAll` operation will provide the computed values.

::: browser/extensions/formautofill/ProfileStorage.jsm:402
(Diff revision 1)
> +   * @param   {boolean} [options.includePrivateFields = false]
> +   *          Should fields starting with underscore be included in the result?
>     * @returns {Object}
>     *          A clone of the record.
>     */
> -  get(guid) {
> +  get(guid, {includePrivateFields = false} = {}) {

Just curious. Is there any use-case of `includePrivateFields = true`? IIUC, it's only for test. BTW, I am fine with this option.

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review158704

Overall it looks good. Sorry for the late reply.

::: browser/extensions/formautofill/ProfileStorage.jsm:430
(Diff revision 1)
> +   * @param   {boolean} [config.includePrivateFields = false]
> +   *          Should fields starting with underscore be included in the result?
>     * @returns {Array.<Object>}
>     *          An array containing clones of all records.
>     */
> -  getAll({noComputedFields = false, includeDeleted = false} = {}) {
> +  getAll({noComputedFields = false, includeDeleted = false, includePrivateFields = false} = {}) {

I'm going to introduce an option `rawData` in bug 1368008 to replace `noComputedFields`. It indicates that the returned data is untouched, without the modifications by `_recordReadProcessor` and the computed fields. I think this option will be used by sync function only so I suspect that `rawData` can also cover `includePrivateFields`. What do you think?

::: browser/extensions/formautofill/ProfileStorage.jsm:680
(Diff revision 1)
>        }
>      }
>    }
>  
> +  // A test-only helper.
> +  _nukeAllRecords() {

It's not used in this patch. Will it be used in the follow-up patches?

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review158986

I don't think I can close these issues so I'll just respond to them. Rebased and updated patch incoming.

::: browser/extensions/formautofill/ProfileStorage.jsm:223
(Diff revision 1)
> +      let recordToSave = Object.assign({
> +        // `timeLastUsed` and `timesUsed` are always local.
> +        timeLastUsed: 0,
> +        timesUsed: 0,
> +      }, record);
> +      return this._saveRecord(recordToSave, {sourceSync});

That (well, the renamed version) is now called in saveRecord.

::: browser/extensions/formautofill/ProfileStorage.jsm:402
(Diff revision 1)
> +   * @param   {boolean} [options.includePrivateFields = false]
> +   *          Should fields starting with underscore be included in the result?
>     * @returns {Object}
>     *          A clone of the record.
>     */
> -  get(guid) {
> +  get(guid, {includePrivateFields = false} = {}) {

I believe it's used in later patches as well as tests, but it's also helpful for tests.

::: browser/extensions/formautofill/ProfileStorage.jsm:430
(Diff revision 1)
> +   * @param   {boolean} [config.includePrivateFields = false]
> +   *          Should fields starting with underscore be included in the result?
>     * @returns {Array.<Object>}
>     *          An array containing clones of all records.
>     */
> -  getAll({noComputedFields = false, includeDeleted = false} = {}) {
> +  getAll({noComputedFields = false, includeDeleted = false, includePrivateFields = false} = {}) {

I've made rawData cover includePrivateFields to meet this request (this will require a slightly tricky rebase for the other sync patches but worse things have happened).

::: browser/extensions/formautofill/ProfileStorage.jsm:482
(Diff revision 1)
> +      let tombstone = {
> +        guid,
> +        timeLastModified: Date.now(),
> +        deleted: true,
> +      };
> +      this._store.data[this._collectionName].push(tombstone);

Got it.

::: browser/extensions/formautofill/ProfileStorage.jsm:680
(Diff revision 1)
>        }
>      }
>    }
>  
> +  // A test-only helper.
> +  _nukeAllRecords() {

Removed.

::: browser/extensions/formautofill/test/unit/test_storage_syncfields.js:64
(Diff revision 1)
> +  address = profileStorage.addresses.getAll({includePrivateFields: true})[0];
> +  equal(address._sync.changeCounter, 1);
> +});
> +
> +add_task(async function test_changeCounter_update() {
> +  // This test should probably also check .status, and need splitting so

Removed the test.

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1363999 - Add sync metadata to formautofill records.

This patch lands the metadata and related functions to the ProfileStorage
necessary for Sync to function.

* New "public" functions which are intended to be used only by Sync:
** pullSyncChanges: Gets metadata about what Sync needs to upload to the
   server.
** pushSyncChanges: Apply the changes made by Sync.
** resetSync: Reset all Sync metadata, used when Sync is disconnected from a
   device.
** changeGUID to change the GUID of an item that has no sync metadata.

* New optional Sync metdata for a record.
  Once a record is marked as Syncing, it carries around a new _sync field.
  Currently this contains only a changeCounter. All records are marked as
  Syncing once pullSyncChanges is called - after this call, all local
  records will have Sync metadata.

* Changes to tombstones semantics: If an item carries no Sync metadata (ie,
  hasn't previously been synced), then no tombstone will be left behind when
  it is deleted.

* Many existing functions get an optional {includePrivateFields} option, which
  will include all fields with a leading underscore (_) - which obviously
  includes this new _sync field - to be returned in the record.

* Many existing functions get an optional {sourceSync} option, which
  indicates Sync itself is making the change. This significantly changes
  the semantics of some operations (eg, counters will not be incremented,
  tombstones can be resurrected, etc)

Review commit: https://reviewboard.mozilla.org/r/153820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153820/

Comment 8

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

Unsure about who should review this, let me know if I should change who I flagged.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/153820/#review159396

Awesome, thanks!

Comment 10

[Lina Butler (ex-Mozilla)]

I think this bitrotted again. :-(

Comment 11

[Lina Butler (ex-Mozilla)]

Sorry, it didn't; I forgot to cherry-pick bug 1368008 first.

Comment 12

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/153820/#review161884

Looks great! Thanks.

::: browser/extensions/formautofill/ProfileStorage.jsm:45
(Diff revision 1)
>   *       // metadata
>   *       timeCreated,          // in ms
>   *       timeLastUsed,         // in ms
>   *       timeLastModified,     // in ms
>   *       timesUsed
> + *       _sync: { ... sync metadata },

nit: "optional sync metadata" to align with the other one.

::: browser/extensions/formautofill/test/unit/test_storage_syncfields.js:212
(Diff revision 1)
> +
> +  let tombstone = findGUID(profileStorage.addresses, guid, {
> +    rawData: true,
> +    includeDeleted: true,
> +  });
> +  ok(tombstone.deleted, 0);

`ok(tombstone.deleted);`

::: browser/extensions/formautofill/test/unit/test_storage_syncfields.js:236
(Diff revision 1)
> +
> +  let tombstone = findGUID(profileStorage.addresses, guid, {
> +    rawData: true,
> +    includeDeleted: true,
> +  });
> +  ok(tombstone.deleted, 0);

`ok(tombstone.deleted);`

::: browser/extensions/formautofill/test/unit/test_storage_syncfields.js:253
(Diff revision 1)
> +  let testAddr = Object.assign({guid: addedDirectGUID},
> +                               TEST_ADDRESS_1, TEST_ADDRESS_3);

Curious why we need `TEST_ADDRESS_1` here?

Comment 13

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153820/diff/1-2/

Comment 14

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153820/diff/2-3/

Comment 15

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/153820/#review161884

> Curious why we need `TEST_ADDRESS_1` here?

For whatever reason, I remember there being an issue due to `TEST_ADDRESS_3` being a very small record. It doesn't seem to cause any problems now though, so I've removed the use of `TEST_ADDRESS_1` from that expression.

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8882728 [details]
Bug 1363999 - Add sync metadata to formautofill records.

Luke's r+ somehow got dropped here.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149748/diff/1-2/

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

stooopid mozreview - this version of the patch just has the eslint failure fixed - not sure why mozreview thinks it is something different.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review163908

Comment 20

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8878406 [details]
Bug 1363999 - Add sync metadata to formautofill records.

https://reviewboard.mozilla.org/r/149748/#review163918

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f38f4687b6f
Add sync metadata to formautofill records. r=lchang

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f38f4687b6f