806656 - Query records generated incorrectly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Richard Newman [:rnewman]]

We're getting records without a URI, and with a blank folderName and queryId. Android Sync is at fault.

Comment 1

[Richard Newman [:rnewman]]

https://github.com/mozilla-services/android-sync/pull/265

Comment 2

[Richard Newman [:rnewman]]

After the fixes in that branch, we:

* Correctly include bmkUri in produced bookmark records for query URIs.
* Omit fields which were encoded as empty strings prior to these fixes (e.g., if a query had no queryId, it now doesn't produce a blank string in the JSON output).
* No longer encode missing fields as empty strings in the first place.

This will prevent the upload of records which caused Bug 806460.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Same as the GitHub branch.

Comment 4

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/759b0e64b8b7

See Bug 806580 Comment 12 for how to verify this.

Will wait for verification before requesting uplift.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Aurora: fix.

Reviewed on GitHub. liuche, please rubberstamp here.

Setting Aurora and Beta flags here for release drivers to take a look at. Will wait for verification before landing, of course.

This is the counterpart to Bug 806460, the fix for which was just uplifted.

This is a pretty straightforward fix, and now comes with tests in the services codebase.

Comment 6

[Tracy Walker [:tracy]]

(In reply to Richard Newman [:rnewman] from comment #4)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/759b0e64b8b7
> 
> See Bug 806580 Comment 12 for how to verify this.
> 
> Will wait for verification before requesting uplift.

I believe you meant https://bugzilla.mozilla.org/show_bug.cgi?id=806460#c12. How is this the same verification?

Comment 7

[Richard Newman [:rnewman]]

(In reply to Tracy Walker [:tracy] from comment #6)
> (In reply to Richard Newman [:rnewman] from comment #4)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/759b0e64b8b7
> > 
> > See Bug 806580 Comment 12 for how to verify this.
> > 
> > Will wait for verification before requesting uplift.
> 
> I believe you meant https://bugzilla.mozilla.org/show_bug.cgi?id=806460#c12.
> How is this the same verification?

"and those queries won't appear (e.g., smart bookmarks on your toolbar won't appear). After Bug 806656 lands, those queries should sync successfully."

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 676767 [details] [diff] [review]
Aurora: fix.

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

Like rnewman said, reviewed on github. Looks good here.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/759b0e64b8b7

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 676767 [details] [diff] [review]
Aurora: fix.

Approving for uplift to go out with the fix in bug 806460 so that we stop creating bad records in the first place (as explained by rnewman in IRC).

Comment 11

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/267a86de82df
https://hg.mozilla.org/releases/mozilla-beta/rev/0ddd071d9ff2