1377246 - Move Sync deduping logic into profile storage

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Lina Butler [:lina]]

Splitting this out of bug 1363995.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1377246 - Move Sync deduping logic into profile storage.

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

Comment 2

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

Comment on attachment 8882360 [details]
Bug 1377246 - Move Sync deduping logic into profile storage.

https://reviewboard.mozilla.org/r/153460/#review158822

LGTM, thanks

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8882360 [details]
Bug 1377246 - Move Sync deduping logic into profile storage.

https://reviewboard.mozilla.org/r/153460/#review158850

::: browser/extensions/formautofill/ProfileStorage.jsm:679
(Diff revision 1)
> +   *          The remote record.
> +   * @returns {string|null}
> +   *          The GUID of the matching local record, or `null` if no records
> +   *          match.
> +   */
> +  findDuplicateGUID(record) {

It occurs to me that we can use the same logic as `mergeIfPossible` to find duplicates.

However, `mergeIfPossible` merges if *at least one field* matches, while `findDuplicateGUID` merges if *all* fields match. I'm not sure if we want the same behavior for both cases, or keep them separate.

Neither `findDuplicateGUID` nor `mergeIfPossible` know about records with higher versions, so I suspect we'll need to think about that in bug 1377204.

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8882360 [details]
Bug 1377246 - Move Sync deduping logic into profile storage.

https://reviewboard.mozilla.org/r/153460/#review158850

> It occurs to me that we can use the same logic as `mergeIfPossible` to find duplicates.
> 
> However, `mergeIfPossible` merges if *at least one field* matches, while `findDuplicateGUID` merges if *all* fields match. I'm not sure if we want the same behavior for both cases, or keep them separate.
> 
> Neither `findDuplicateGUID` nor `mergeIfPossible` know about records with higher versions, so I suspect we'll need to think about that in bug 1377204.

We had a discussion about it and realized that keeping them separate would be easier. We tend to merge profiles aggressively when submitting them locally, but I think it doesn't fit the sync behavior.

Comment 5

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

Comment on attachment 8882360 [details]
Bug 1377246 - Move Sync deduping logic into profile storage.

https://reviewboard.mozilla.org/r/153460/#review161104

The patch looks good to me. I just added the comments which are some questions more than issues.
Sorry for the delay since I have to read the patches cross bugs.

::: browser/extensions/formautofill/FormAutofillSync.jsm:151
(Diff revision 1)
>  
>    createRecord(id, collection) {
>      this._log.trace("Create record", id);
>      let record = new AutofillRecord(collection, id);
> -    record.entry = this.storage.get(id, {
> +    let entry = this.storage.get(id, {
>        noComputedFields: true,

Change to `rawData` if needed.

::: browser/extensions/formautofill/ProfileStorage.jsm:708
(Diff revision 1)
> +        keys.delete(field);
> +      }
> +      let same = true;
> +      for (let key of keys) {
> +        if (profile.hasOwnProperty(key) && record.hasOwnProperty(key)) {
> +          same = profile[key] == record[key];

Does this mean `same` variable will be never set to `false` for any missing properties?

e.g. the extremely case of `record = {}` will be the same with any profiles, right?

I agree with this behavior, and it's for finding the record is able to be merged easily. I suppose it's worth to protect the expected behavior with a xpcshell test.

The comment of this function might need to be modified as well since it mentioned:
```
   * Finds a new record with a different GUID but the same contents as the 
   * given record. 
```

Either of comment and test are fine with me to explain the behavior.

::: browser/extensions/formautofill/test/unit/test_sync.js:45
(Diff revision 1)
>    country: "US",
>  };
>  
>  function expectLocalProfiles(expected) {
>    let profiles = profileStorage.addresses.getAll({
> +    noComputedFields: true,

`noComputedFields` is changed to `rawData` in bug 1368008[1]. Please change it if you want to based on the top of that.

[1] https://reviewboard.mozilla.org/r/143180/diff/11#index_header

::: browser/extensions/formautofill/test/unit/test_sync.js:58
(Diff revision 1)
>      for (let i = 0; i < expected.length; i++) {
>        let thisExpected = expected[i];
>        let thisGot = profiles[i];
>        // always check "deleted".
>        equal(thisExpected.deleted, thisGot.deleted);
> -      doCheckObject(thisExpected, thisGot);
> +      objectMatches(thisGot, thisExpected);

Is there any reason to use `objectMatches` instead of `deepDqual`?

::: browser/extensions/formautofill/test/unit/test_sync.js:291
(Diff revision 1)
> +
> +    expectLocalProfiles([
> +      // guid1 had previously synced, so isn't touched.
> +      Object.assign({}, TEST_PROFILE_1, {guid: guid1}),
> +      // The incoming "dupe" of guid1 was applied.
> +      Object.assign({}, TEST_PROFILE_1, {guid: guid1_dupe}),

Is this duplicated to the above one? Why is this one not removed by the dedup function?

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8882360 [details]
Bug 1377246 - Move Sync deduping logic into profile storage.

https://reviewboard.mozilla.org/r/153460/#review161104

> Does this mean `same` variable will be never set to `false` for any missing properties?
> 
> e.g. the extremely case of `record = {}` will be the same with any profiles, right?
> 
> I agree with this behavior, and it's for finding the record is able to be merged easily. I suppose it's worth to protect the expected behavior with a xpcshell test.
> 
> The comment of this function might need to be modified as well since it mentioned:
> ```
>    * Finds a new record with a different GUID but the same contents as the 
>    * given record. 
> ```
> 
> Either of comment and test are fine with me to explain the behavior.

Good catch, and I think the correct behavior is to ensure both objects have the property once we build the set. Updated the comment and added a test to clarify.

> Is there any reason to use `objectMatches` instead of `deepDqual`?

This is similar to `deepEqual`, but not quite. We only want to compare a subset of properties, while `deepEqual` will compare the entire object: `deepEqual({ timeLastModified: 123, "given-name": "Mark", _sync: { ... } }, { "given-name": "Mark" })` won't work, even though I'm not interested in "timeLastModified" or "_sync". However, I can rewrite the helper to clone the object, filter the properties we want, and then call `deepEqual`. Thanks!

> Is this duplicated to the above one? Why is this one not removed by the dedup function?

In this case, it's not deduplicated because `guid1` already exists on the server. (We uploaded it the first time we called `engine.sync()`, and, at that point, `guid1_dupe` wasn't on the server). For deduping, we only consider records that haven't been uploaded yet, so that's why `guid2` is deduplicated to `guid2_dupe`. I'll add some more comments to clarify.

Comment 7

[Lina Butler [:lina]]

Attachment: Bug 1377246 - Always filter Sync metadata from profiles.

We don't want to expose Sync metadata via `{rawData: true}` for two
reasons. First, Sync's `createRecord` method passes this option to
filter computed fields. We don't want to include computed fields in
the record, but we also don't want to upload the Sync metadata.
Second, `_clone` uses a shallow copy, so the `_sync` object can still
be mutated.

The only callers that rely on `{rawData: true}` returning `_sync` are
tests, so we can add a test-only helper instead of exposing the
object.

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

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8882360 [details]
Bug 1377246 - Move Sync deduping logic into profile storage.

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

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8885553 [details]
Bug 1377246 - Always filter Sync metadata from profiles.

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

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8882360 [details]
Bug 1377246 - Move Sync deduping logic into profile storage.

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

Comment 11

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

Comment on attachment 8885553 [details]
Bug 1377246 - Always filter Sync metadata from profiles.

https://reviewboard.mozilla.org/r/156410/#review161518

That looks fine to me, but I think we should do the right thing and make sure that the autofill team reviews the profilestorage part - I also wonder if that helper might be better off in head.js, but I don't really care either way.

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8885553 [details]
Bug 1377246 - Always filter Sync metadata from profiles.

Cool, I wanted to make sure this made sense.

Luke, could you take a look as well, please, since you're reviewing bug 1363999? This changes `_clone` so that it never returns `_sync`, even if `{rawData: true}` is passed. I explained a bit more in the commit message.

Comment 13

[Lina Butler [:lina]]

Comment on attachment 8885553 [details]
Bug 1377246 - Always filter Sync metadata from profiles.

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

Comment 14

[Lina Butler [:lina]]

Comment on attachment 8882360 [details]
Bug 1377246 - Move Sync deduping logic into profile storage.

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

Comment 15

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8885553 [details]
Bug 1377246 - Always filter Sync metadata from profiles.

https://reviewboard.mozilla.org/r/156410/#review163308

Putting the helper in head.js makes sense to me, but it's up to you. Overall looks good. Thanks.

Comment 16

[Lina Butler [:lina]]

Comment on attachment 8885553 [details]
Bug 1377246 - Always filter Sync metadata from profiles.

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

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8882360 [details]
Bug 1377246 - Move Sync deduping logic into profile storage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153460/diff/4-5/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1051372286ca
Always filter Sync metadata from profiles. r=markh,lchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6a93a94e7d0
Move Sync deduping logic into profile storage. r=markh,seanlee

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1051372286ca
https://hg.mozilla.org/mozilla-central/rev/b6a93a94e7d0