Skip to content

Better error handling of non-conformant json:api responses #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 26, 2016
Merged

Better error handling of non-conformant json:api responses #14

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1440146
Better error handling of non-conformant json:api responses
Jan 26, 2016
d4fc605
Use Array.isArray instead of instanceof
Jan 26, 2016
64fa102
Use Array.isArray instead of instanceof
Jan 26, 2016
8de8d15
Wrap a JSON.parse in a try-catch block
Jan 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
2015-12-17 - v0.1.0 Adding authentication
2015-12-17 - v0.2.0 Better validation when changing relations
2015-12-17 - v0.3.0 Fixes to enable usage in web browsers
2015-12-17 - v0.4.0 Better handling of non-json:api payloads
35 changes: 34 additions & 1 deletion dist/jsonapi-client-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,15 @@ Transport.prototype._attachAuthToRequest = function(someRequest) {
}
};

Transport._defaultError = function(response) {
return {
status: "500",
code: "EUNKNOWN",
title: "An unknown error has occured",
detail: response
};
};

Transport.prototype._action = function(method, url, data, callback) {
// console.log(method, url, JSON.stringify(data, null, 2));
var someRequest = request[method](url);
Expand All @@ -435,7 +444,12 @@ Transport.prototype._action = function(method, url, data, callback) {
response = JSON.parse(err.response.text);
} catch(e) {
console.error("Transport Error: " + JSON.stringify(err));
return callback(new Error("Invalid response from server"));
return callback(Transport._defaultError(response));
}

if (!(response.errors instanceof Array)) {
Copy link
Contributor

@pmcnr-hx pmcnr-hx Jan 26, 2016

Choose a reason for hiding this comment

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

Should we use Array.isArray instead? Or is the difference largely irrelevant?

Copy link
Contributor Author

@theninj4 theninj4 Jan 26, 2016

Choose a reason for hiding this comment

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

Apparently Array.isArray goes as far back as IE9, so I guess we could?

Copy link
Contributor

@pmcnr-hx pmcnr-hx Jan 26, 2016

Choose a reason for hiding this comment

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

I didn't even think about Array.isArray vs instanceof Array until now, so I'm not sure. I'll leave it to your better judgement. :)

console.error("Invalid Error payload!", response);
return callback(Transport._defaultError(response));
}

var realErrors = response.errors.map(function(apiError) {
Expand All @@ -454,6 +468,10 @@ Transport.prototype._action = function(method, url, data, callback) {
payload.body = JSON.parse(payload.text);
Copy link
Contributor

@pmcnr-hx pmcnr-hx Jan 26, 2016

Choose a reason for hiding this comment

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

Out of scope of this change, but is there any danger JSON.parse(payload.text) will fail to parse and throw and exception? Should we put in a try..catch block?

Copy link
Contributor Author

@theninj4 theninj4 Jan 26, 2016

Choose a reason for hiding this comment

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

Good call, I'll add it.

}

if (!payload.body) {
return callback(Transport._defaultError(payload));
}

return callback(null, payload.body, payload.body.data, payload.body.included);
});
};
Expand Down Expand Up @@ -16868,6 +16886,21 @@ describe("Testing jsonapi-client", function() {

});

describe("testing invalid payloads", function() {
it("doesnt crash when we get a non-conformant response", function(done) {
Copy link
Contributor

@pmcnr-hx pmcnr-hx Jan 26, 2016

Choose a reason for hiding this comment

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

You can actually spell doesn't correctly since the string is double quoted ;)

Copy link
Contributor Author

@theninj4 theninj4 Jan 26, 2016

Choose a reason for hiding this comment

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

Haha, too true!

var badClient = new Client("http://localhost:12345");
badClient.find("articles", { }, function(err) {
assert.deepEqual(err, {
"status": "500",
"code": "EUNKNOWN",
"title": "An unknown error has occured",
"detail": undefined
});
done();
});
});
});

});

},{"../.":1,"./_testServer.js":30,"assert":6}],34:[function(require,module,exports){
Expand Down
20 changes: 19 additions & 1 deletion lib/Transport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@ Transport.prototype._attachAuthToRequest = function(someRequest) {
}
};

Transport._defaultError = function(response) {
return {
status: "500",
code: "EUNKNOWN",
title: "An unknown error has occured",
detail: response
};
};

Transport.prototype._action = function(method, url, data, callback) {
// console.log(method, url, JSON.stringify(data, null, 2));
var someRequest = request[method](url);
Expand All @@ -49,7 +58,12 @@ Transport.prototype._action = function(method, url, data, callback) {
response = JSON.parse(err.response.text);
} catch(e) {
console.error("Transport Error: " + JSON.stringify(err));
return callback(new Error("Invalid response from server"));
return callback(Transport._defaultError(response));
}

if (!(response.errors instanceof Array)) {
console.error("Invalid Error payload!", response);
return callback(Transport._defaultError(response));
}

var realErrors = response.errors.map(function(apiError) {
Expand All @@ -68,6 +82,10 @@ Transport.prototype._action = function(method, url, data, callback) {
payload.body = JSON.parse(payload.text);
}

if (!payload.body) {
return callback(Transport._defaultError(payload));
}

return callback(null, payload.body, payload.body.data, payload.body.included);
});
};
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "jsonapi-client",
"version": "0.3.0",
"version": "0.4.0",
"description": "A clientside module designed to make it really easy to consume a json:api service.",
"keywords": [
"jsonapi",
Expand Down
15 changes: 15 additions & 0 deletions test/read.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,4 +255,19 @@ describe("Testing jsonapi-client", function() {

});

describe("testing invalid payloads", function() {
it("doesnt crash when we get a non-conformant response", function(done) {
var badClient = new Client("http://localhost:12345");
badClient.find("articles", { }, function(err) {
assert.deepEqual(err, {
"status": "500",
"code": "EUNKNOWN",
"title": "An unknown error has occured",
"detail": undefined
});
done();
});
});
});

});