Log a warning if JSON parsing of bodies fails

RESOLVED FIXED in mozilla10

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

(Blocks: 1 bug)

unspecified
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed in services][qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Right now we just silently fail to set .payload, which leads to obscure errors later.

  deserialize: function deserialize(json) {
    this.data = json.constructor.toString() == String ? JSON.parse(json) : json;

    try {
      // The payload is likely to be JSON, but if not, keep it as a string
      this.payload = JSON.parse(this.payload);
    } catch(ex) {}
  },

It would be really swell to, y'know, log something if we get a body we can't parse -- maybe only if the response headers indicate a Content-Type of JSON, if we want to be neat and tidy about it.
(Assignee)

Comment 1

6 years ago
Created attachment 565164 [details] [diff] [review]
About time. v1

Example output:

1317890048133	Sync.Resource	WARN	Got exception "JSON.parse: unexpected character JS Stack trace: ()@resource.js:392 < ()@XPCOMUtils.jsm:210 < run_test()@test_resource.js:192 < _execute_test()@head.js:326 < @-e:1" parsing response body.
1317890048133	Sync.Resource	DEBUG	Parse fail: Response body starts: "This path exists".
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #565164 - Flags: review?(philipp)
Comment on attachment 565164 [details] [diff] [review]
About time. v1

>+        this._log.debug("Parse fail: Response body starts: \"" + (ret + "").slice(0, 100) + "\".");

i asked about encoding the slice so it's log-safe and :rnewman suggests that wrapping slice(0, 100) in "JSON stringification, whilst ugly, would be non-printable-safe".
Attachment #565164 - Flags: feedback+
(Assignee)

Comment 3

6 years ago
Created attachment 565167 [details] [diff] [review]
JSONifying, some cleanup. v2
Attachment #565164 - Attachment is obsolete: true
Attachment #565164 - Flags: review?(philipp)
Attachment #565167 - Flags: review?(philipp)
Comment on attachment 565167 [details] [diff] [review]
JSONifying, some cleanup. v2

r=whisky
Attachment #565167 - Flags: review?(philipp) → review+
(Assignee)

Comment 5

6 years ago
Pushed:

https://hg.mozilla.org/services/services-central/rev/ffc15889906e
Whiteboard: [good first bug] → [fixed in services][qa-]
(Assignee)

Comment 6

6 years ago
Missed a qref after fixing tests to handle JSONifying. Fixxored.

https://hg.mozilla.org/services/services-central/rev/6270ac0e7f96
https://hg.mozilla.org/mozilla-central/rev/ffc15889906e
https://hg.mozilla.org/mozilla-central/rev/6270ac0e7f96
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.