catches JSON parsing errors by eawooten · Pull Request #3 · AgencyPMG/fbgraph

eawooten · 2026-03-16T21:07:41Z

This pull request migrates the test suite from Vows to Jest, updates test configuration, and improves JSON parsing robustness in the graph.js library. The most important changes include replacing legacy tests with Jest-based tests, updating package dependencies, and enhancing error handling for malformed JSON responses.

Testing migration and improvements:

Replaced the Vows-based test suite in tests/graph.test.js with a new Jest-based suite in tests/graph.jest.test.js, including improved tests for API behavior and hardening JSON parsing. [1] [2]
Added a new Jest test file tests/hardening.jest.test.js specifically for validating robust JSON parsing and error handling in API responses.

Configuration and dependency updates:

Updated package.json to use Jest for testing instead of Vows, added Jest configuration for test matching, and changed devDependencies accordingly.
Removed the legacy Makefile test targets for Vows, reflecting the switch to Jest.

Library robustness enhancements:

Improved JSON parsing and error handling in lib/graph.js for both GET and POST requests, ensuring malformed responses are handled gracefully and headers are attached to all responses. [1] [2]

Copilot

Pull request overview

This PR modernizes the project’s testing setup by migrating from Vows to Jest and strengthens lib/graph.js response handling so malformed JSON doesn’t crash requests and response headers are consistently surfaced to callers.

Changes:

Replaced legacy Vows integration tests with Jest-based tests and added new helper-focused unit tests.
Updated npm test configuration/dependencies to use Jest and removed the old Makefile test targets.
Hardened lib/graph.js GET/POST parsing to catch JSON parse failures and attach response headers to returned data/errors.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`tests/testUsers.test.js`	Removed legacy Vows-based test-user integration suite.
`tests/testUsers.jest.test.js`	Added Jest replacement for test-user integration flow (create users, friend, post/query).
`tests/graph.test.js`	Removed legacy Vows-based graph integration tests.
`tests/graph.jest.test.js`	Added Jest replacement graph tests + JSON parsing hardening tests via `request` mocks.
`tests/helpers.jest.test.js`	Added Jest tests for internal helper functions used to attach headers / gate callbacks.
`lib/graph.js`	Added header-wrapping helper, JSON parse guarding in GET/POST, and exported test helpers.
`package.json`	Switched `npm test` to Jest and added Jest config/dependency updates.
`Makefile`	Removed Vows-oriented test targets.

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

Copilot · 2026-03-19T19:56:26Z

package.json

+    "jest": {
+      "testMatch": [
+        "**/tests/*.jest.test.js"
+      ]
+    },


PR description mentions a dedicated tests/hardening.jest.test.js file, but the repository currently only contains hardening tests embedded in tests/graph.jest.test.js and no hardening.jest.test.js file. Either add the missing test file or update the PR description to reflect the actual test layout.

Copilot · 2026-03-19T19:56:26Z

lib/graph.js

+                handleRequestError(
+                    self,
+                    callbackCalled,
+                    { message: 'Error parsing JSON response', exception: parseError, body: body },


The library now produces two different JSON parse error messages: Graph.prototype.end sets message: 'Error parsing json', while get/post use message: 'Error parsing JSON response'. For API consumers this is inconsistent and makes it harder to match/handle parse failures. Consider standardizing on a single message (and casing) across all parse-error paths.

Suggested change

{ message: 'Error parsing JSON response', exception: parseError, body: body },

{ message: 'Error parsing json', exception: parseError, body: body },

Copilot · 2026-03-19T19:56:27Z

lib/graph.js

+        }
+
+        wrapCallbackWithHeaders(self, res);
+


Graph.prototype.get only treats responses as images when the content-type header contains image, but Facebook /.../picture endpoints commonly respond with a 3xx redirect + Location header (often with content-type: text/html). In that case this code will fall through to self.end(body) and the library won’t produce the documented { image: true, location: ... } shape. Consider detecting redirects (e.g., statusCode 3xx with res.headers.location) and returning { image: true, location: res.headers.location } (and headers) to match the README/tests.

Suggested change

// Handle redirect responses (e.g., Facebook /picture endpoints) as image locations

if (

res &&

typeof res.statusCode === 'number' &&

res.statusCode >= 300 &&

res.statusCode < 400 &&

res.headers &&

res.headers.location

) {

return self.end({

image: true,

location: res.headers.location,

headers: res.headers,

});

}

Copilot · 2026-03-19T19:56:27Z

lib/graph.js

+        if (typeof body === 'string' && (body.charAt(0) === '{' || body.charAt(0) === '[')) {
+            try {
+                body = JSON.parse(body);
+            } catch (parseError) {
+                handleRequestError(
+                    self,
+                    callbackCalled,
+                    { message: 'Error parsing JSON response', exception: parseError, body: body },
+                );
+                callbackCalled = true;
+                return;


JSON detection in get/post only checks body.charAt(0) for { or [. If the server returns valid JSON with leading whitespace/newlines (common), this won’t parse here; Graph.prototype.end also won’t parse JSON arrays ([...]) at all, so responses like \n [ ... ] would be misinterpreted as a querystring. Consider trimming leading whitespace before the {/[ check (or using a more robust JSON detection) so valid JSON isn’t misclassified.

Suggested change

if (typeof body === 'string' && (body.charAt(0) === '{' || body.charAt(0) === '[')) {

try {

body = JSON.parse(body);

} catch (parseError) {

handleRequestError(

self,

callbackCalled,

{ message: 'Error parsing JSON response', exception: parseError, body: body },

);

callbackCalled = true;

return;

if (typeof body === 'string') {

// Trim leading whitespace before checking for JSON structures

var trimmedBody = (typeof body.trimStart === 'function')

? body.trimStart()

: body.replace(/^\s+/, '');

if (trimmedBody.charAt(0) === '{' || trimmedBody.charAt(0) === '[') {

try {

body = JSON.parse(trimmedBody);

} catch (parseError) {

handleRequestError(

self,

callbackCalled,

{ message: 'Error parsing JSON response', exception: parseError, body: body },

);

callbackCalled = true;

return;

}

Copilot · 2026-03-19T19:56:27Z

tests/graph.jest.test.js

+const request = require("request");
+// Mock must happen before requiring graph/index
+jest.mock("request", () => {
+    const originalModule = jest.requireActual("request");
+    const mock = jest.fn((options, callback) => {
+        return originalModule(options, callback);
+    });
+    mock.get = jest.fn((options, callback) => {
+        return originalModule.get(options, callback);
+    });
+    return mock;
+});


This test file requires request before calling jest.mock('request', ...). Unless Jest hoists this mock (which depends on configuration/transform), request may already be the real module here, making request.get.mockImplementationOnce(...) and request.mockImplementationOnce(...) fail. To make the mocking deterministic, call jest.mock('request', ...) before requiring request (or require it after the mock).

Copilot · 2026-03-19T19:56:27Z

tests/graph.jest.test.js

+        test("and searching for public data via username", (done) => {
+            graph.get("/btaylor", (err, res) => {
+                if (res && res.error) {
+                    expect(res).toHaveProperty("error");
+                } else if (res) {
+                    expect(res).toHaveProperty("name");
+                    expect(res.name).toBe("Bret Taylor");
+                }
+                done();
+            });


Several tests can pass without asserting anything if the callback yields (err, undefined) or (undefined, undefined). For example here, if res is falsy the test just calls done() with no expectations. Please ensure the test asserts an expected outcome in all branches (e.g., require either res or err to be defined, and/or fail the test when neither is present).

Copilot · 2026-03-19T19:56:27Z

tests/testUsers.jest.test.js

+                if (err || (res1 && res1.error)) {
+                    expect(err || res1.error).toBeDefined();
+                    done();


On failure to create the first test user, this test currently calls done() after asserting the error is defined, which makes the test succeed even though the main behavior wasn’t exercised. If this is a real failure (not an intentional skip), it should fail the test (e.g., by calling done(err || res1.error) or throwing) so CI catches regressions.

Suggested change

if (err || (res1 && res1.error)) {

expect(err || res1.error).toBeDefined();

done();

const error = err || (res1 && res1.error);

if (error) {

expect(error).toBeDefined();

done(error);

catches JSON parsing errors

a9f3aa9

eawooten requested a review from Copilot March 16, 2026 21:07

Copilot started reviewing on behalf of eawooten March 16, 2026 21:08 View session

This comment was marked as resolved.

Sign in to view

better testing and handling of response types

098983c

eawooten requested a review from Copilot March 18, 2026 14:47

Copilot started reviewing on behalf of eawooten March 18, 2026 14:48 View session

This comment was marked as outdated.

Sign in to view

eawooten added 3 commits March 18, 2026 10:17

fixes issues with callbacks and logic

f74f7d3

updates tests to jest and remove legacy functionality

236b8fb

abstract header handling to its own method

c0d6cc0

eawooten requested a review from Copilot March 19, 2026 16:21

This comment was marked as outdated.

Sign in to view

improve tests and export test object

7858de2

eawooten requested a review from Copilot March 19, 2026 19:49

Copilot started reviewing on behalf of eawooten March 19, 2026 19:50 View session

Copilot AI reviewed Mar 19, 2026

View reviewed changes

abstract out the common functionality to parse and validate json

4cb9def

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

catches JSON parsing errors#3

catches JSON parsing errors#3
eawooten wants to merge 7 commits intomasterfrom
catch-json-parse-errors

eawooten commented Mar 16, 2026 •

edited

Loading

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 19, 2026

Uh oh!

Copilot AI Mar 19, 2026

Uh oh!

Copilot AI Mar 19, 2026

Uh oh!

Copilot AI Mar 19, 2026

Uh oh!

Copilot AI Mar 19, 2026

Uh oh!

Copilot AI Mar 19, 2026

Uh oh!

Copilot AI Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	{ message: 'Error parsing JSON response', exception: parseError, body: body },
	{ message: 'Error parsing json', exception: parseError, body: body },

+        // Handle redirect responses (e.g., Facebook /picture endpoints) as image locations
+        if (
+            res &&
+            typeof res.statusCode === 'number' &&
+            res.statusCode >= 300 &&
+            res.statusCode < 400 &&
+            res.headers &&
+            res.headers.location
+        ) {
+            return self.end({
+                image: true,
+                location: res.headers.location,
+                headers: res.headers,
+            });
+        }

-                if (err || (res1 && res1.error)) {
-                    expect(err || res1.error).toBeDefined();
-                    done();
+                const error = err || (res1 && res1.error);
+                if (error) {
+                    expect(error).toBeDefined();
+                    done(error);

Conversation

eawooten commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eawooten commented Mar 16, 2026 •

edited

Loading