Closed Bug 685945 Opened 9 years ago Closed 8 years ago

Log a warning if JSON parsing of bodies fails

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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.
Attached patch About time. v1 (obsolete) — Splinter Review
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+
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+
Pushed:

https://hg.mozilla.org/services/services-central/rev/ffc15889906e
Whiteboard: [good first bug] → [fixed in services][qa-]
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.