Skip to content

catch JSON parsing errors in fbgraph#4

Open
eawooten wants to merge 5 commits intomasterfrom
catch-json-parse-errors-minimal
Open

catch JSON parsing errors in fbgraph#4
eawooten wants to merge 5 commits intomasterfrom
catch-json-parse-errors-minimal

Conversation

@eawooten
Copy link

@eawooten eawooten commented Mar 19, 2026

This pull request improves the robustness of JSON parsing in the Graph class by adding error handling for malformed JSON responses. It also introduces new tests to verify that the application handles non-JSON responses gracefully instead of crashing.

Error handling improvements:

  • Updated Graph.prototype.get and Graph.prototype.post in lib/graph.js to wrap JSON parsing in a try/catch block, ensuring that parsing errors do not crash the application and that raw bodies are passed to end() if parsing fails. [1] [2]

Testing enhancements:

  • Added a new test batch in tests/graph.test.js to simulate receiving a malformed JSON response and verify that the error is handled correctly, returning an error instead of crashing.
  • Imported the request module in tests/graph.test.js to enable mocking of HTTP requests for the new tests.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens fbgraph’s HTTP response handling by preventing unhandled exceptions when parsing response bodies as JSON, and adds a regression test to ensure malformed JSON doesn’t crash the client.

Changes:

  • Wrap JSON.parse in Graph.prototype.get/post with try/catch to avoid crashing on invalid JSON.
  • Check for image responses before attempting JSON parsing in get.
  • Add a Vows test that stubs request.get to return malformed JSON and asserts an error is returned.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
lib/graph.js Adds defensive try/catch around JSON parsing in get/post and reorders image detection to avoid parsing binary/image responses.
tests/graph.test.js Adds a new batch that stubs request.get to validate malformed JSON is surfaced as an error instead of throwing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

eawooten and others added 4 commits March 19, 2026 15:20
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@eawooten eawooten marked this pull request as ready for review March 19, 2026 20:22
var callback = this.callback
, originalGet = request.get;

request.get = function (options, cb) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to somehow reset this back to what it was?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that's line 296 below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants