Closed Bug 1148252 Opened 5 years ago Closed 5 years ago

Reading-list sync engine should gracefully handle local item already existing on the server

Categories

(Firefox Graveyard :: Reading List, defect)

defect
Not set

Tracking

(firefox38 fixed, firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: markh, Assigned: markh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Currently we fail to handle an attempt to upload a new item when the server already has it.  This scenario could happen due to our bugs, and possibly even due to a network error in the *response* when we first attempt to upload it (ie, request succeeds but we fail to get a response).

Manually testing with this patch, I saw the problematic article get handled correctly (ie, get the correct syncStatus and GUID) and the log indicate:

> 1427421112074   readinglist.sync        INFO    Attempting to upload a new item found the server already had it: {"status":200,"path":"/v1/articles","body":{"archived":false,"marked_read_on":null,"excerpt":"A Marseille prosecutor says the co-pilot of the Germanwings flight that perished in the French Alps appears to have crashed the plane deliberately.","resolved_url":"http://www.abc.net.au/news/2015-03-26/germanwings-co-pilot-andreas-lubitz-crashed-plane-deliberately/6351854","resolved_title":"Germanwings co-pilot crashed deliberately: prosecutor","title":"Germanwings co-pilot crashed deliberately: prosecutor","url":"http://www.abc.net.au/news/2015-03-26/germanwings-co-pilot-andreas-lubitz-crashed-plane-deliberately/6351854","is_article":true,"stored_on":1427416672690,"favorite":true,"word_count":null,"read_position":0,"marked_read_by":null,"added_on":1427416366270,"last_modified":1427416672693,"preview":"http://www.abc.net.au/news/image/6351948-1x1-700x700.jpg","unread":true,"id":"75ea03f0e5d04b48ba0bd54e0ac57c0a","added_by":"%USERNAME%'s Nightly on eden"},"headers":{"Content-Length":"955","Backoff":"None","Content-Type":"application/json; charset=UTF-8","Access-Control-Expose-Headers":"Backoff, Retry-After, Alert"}}
Flags: qe-verify-
Flags: firefox-backlog+
Comment on attachment 8584237 [details] [diff] [review]
0006-Bug-XXXXXXX-gracefully-handle-an-attempt-to-upload-a.patch

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

::: browser/components/readinglist/Sync.jsm
@@ +283,5 @@
> +      // So allow 200 but log a warning.
> +      if (response.status == 200) {
> +        log.info("Attempting to upload a new item found the server already had it", response);
> +      }
> +      if (response.status != 201 && response.status != 200) {

else if (response.status != 201) {

?
Attachment #8584237 - Flags: review+
oops - I guess it's not fixed on 39 yet - it's only on fx-team.
Blocks: 1132074
Iteration: --- → 39.3 - 30 Mar
https://hg.mozilla.org/mozilla-central/rev/024efc08edd8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.