1354016 - Forms validator should ignore clientMissing

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P3)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

We never delete form history records from the server as we don't get notified when that happens. Sad! Therefore we should probably have the form history validator ignore clientMissing as it will be expected they are missing (ie, there were deleted locally but we didn't record the deletion on the server).

Comment 1

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

The needinfo was intended for me to look for a dupe which I cannot find.

This is a very easy bug and would be a good candidate for someone getting ramped up on desktop (*cough* :Grisha *cough*), although it's a fine first bug otherwise.

To ensure it's actionable either way, I'll say what probably needs to be done:

The approach I'd recommend would be to add a property to CollectionValidator that basically equates to "does this collection support tombstones" (defaulting to true), and if it doesn't, somehow ensure that problems.clientMissing is empty when you return from compareClientWithServer (probably either by clearing it out at the end, or never pushing into it). Then set this property to true in FormValidator's constructor.

We don't currently have any FormValidator or general CollectionValidator tests, so the most natural place to test this would be in the PasswordValidator tests, which basically is the stand-in for the latter.

Comment 2

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

Whoops, didn't mean to assign myself.

Comment 3

[Santiago Paez [:tiago]]

Attachment: Bug 1354016 - Forms validator ignore clientMissing.

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

Comment 4

[Santiago Paez [:tiago]]

(In reply to Santiago Paez [:tiago] from comment #3)
> Created attachment 8871983 [details]
> Bug 1354016 - Forms validator ignore clientMissing.
> 
> Review commit: https://reviewboard.mozilla.org/r/143528/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/143528/

Hi!

This is a first try at the bug. If there is any improvement or change, I'll add a new version, and after that I'll add the tests.

All comments welcome, thanks!

Comment 5

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

Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

https://reviewboard.mozilla.org/r/143528/#review147942

With tests and the issue addressed this looks more or less right.

::: services/sync/modules/engines/forms.js:269
(Diff revision 1)
>  }
>  
>  class FormValidator extends CollectionValidator {
>    constructor() {
>      super("forms", "id", ["name", "value"]);
> +    super.setIgnoresMissingClients(true);

If we decided to override this in the subclass (and I'm not sure why we would, which argues against having this as a setter), this would bypass the override.

Honestly, IMO you should just do `this.ignoresMissingClients = true` here, and get rid of the setter. But if you're really attached to using a setter, you should call it on `this` and not on `super`.

Comment 6

[Santiago Paez [:tiago]]

Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

https://reviewboard.mozilla.org/r/143528/#review148136

::: services/sync/modules/engines/forms.js:269
(Diff revision 1)
>  }
>  
>  class FormValidator extends CollectionValidator {
>    constructor() {
>      super("forms", "id", ["name", "value"]);
> +    super.setIgnoresMissingClients(true);

Not attached to the setter, it's gone :)

Comment 7

[Santiago Paez [:tiago]]

Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

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

Comment 8

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

Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

https://reviewboard.mozilla.org/r/143528/#review148444

::: services/sync/tests/unit/test_password_validator.js:160
(Diff revision 2)
>  
> +add_test(function test_formValidatorIgnoresMissingClients() {
> +  // Since history form records are not deleted from the server, the
> +  // |FormValidator| shouldn't set the |missingClient| flag in |problemData|.
> +  let validator = new FormValidator();
> +  let { server, client } = getDummyServerAndClient();

Unfortunately, these give password records, which have a (very) different format from form records. And so it's not really supported to send it to the form validator.

This isn't actually a problem at the moment (obviously, since your test passes), but I don't like the thought of our tests relying on the forms validator not reporting errors when given password records.

The easiest fix is to test the ignoresMissingClients functionality on the password validator instead -- that is, do `let validator = new PasswordValidator(); validator.ignoresMissingClients = true;` (and also remove the forms import from the top).

Thanks for your work so far!

Comment 9

[Santiago Paez [:tiago]]

You are correct, I didn't realize that.

I can make the fix you suggested, but what do you think about creating a |test_form_validator.js| file with a |getDummyServerAndClient| that gives form records? If I'm not wrong the structure of the code would be pretty similar to the |test_password_validator.js|.

Thanks for the feedback!

Comment 10

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

Adding one wouldn't bother me and couldn't hurt. If you feel like it, go for it, but also don't feel like you need to do it if it ends up being unexpectedly difficult (note that you will have to add the new filename to the xpcshell.ini in the same directory, if you've never added new test files before).

(For some context, the reason we don't have one is that it would mostly just serve to test the common collection_validator code, which is already exercised by the password validator test, but again, having it couldn't hurt).

Comment 11

[Santiago Paez [:tiago]]

Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

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

Comment 12

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

Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

https://reviewboard.mozilla.org/r/143528/#review148838

Thanks, this looks great.

Fix the nit and resubmit and it should be good to go.

::: services/sync/tests/unit/test_form_validator.js:75
(Diff revision 3)
> +
> +add_test(function test_formValidatorIgnoresMissingClients() {
> +  // Since history form records are not deleted from the server, the
> +  // |FormValidator| shouldn't set the |missingClient| flag in |problemData|.
> +  let { server, client } = getDummyServerAndClient();
> +  

nit: trailing whitespace on this line.

Comment 13

[Santiago Paez [:tiago]]

Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

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

Comment 14

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

Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

https://reviewboard.mozilla.org/r/143528/#review149826

Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be7694b72c8a
Forms validator ignore clientMissing. r=tcsc

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/be7694b72c8a