Skip to content
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

feat: adds support to http problem details - rfc7807 #1786

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: adds support to http problem details - rfc7807 #1786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ deps/javascriptlint
deps/jsstyle
package-lock.json
benchmark/results
.idea
3 changes: 2 additions & 1 deletion lib/formatters/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ module.exports = {
'application/javascript; q=0.1': require('./jsonp'),
'application/json; q=0.4': require('./json'),
'text/plain; q=0.3': require('./text'),
'application/octet-stream; q=0.2': require('./binary')
'application/octet-stream; q=0.2': require('./binary'),
'application/problem+json; q=0.5': require('./json')
};
9 changes: 7 additions & 2 deletions lib/response.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ function patch(Response) {
// If the body is an error object and we were not given a status code,
// try to derive it from the error object, otherwise default to 500
if (!code && body instanceof Error) {
code = body.statusCode || 500;
code = body.statusCode || body.status || 500;
Copy link
Member

Choose a reason for hiding this comment

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

when is this status?

}

// Set sane defaults for optional arguments if they were not provided
Expand Down Expand Up @@ -441,7 +441,12 @@ function patch(Response) {
// Set Content-Type to application/json when
// res.send is called with an Object instead of calling res.json
if (!type && typeof body === 'object' && !Buffer.isBuffer(body)) {
type = 'application/json';
// if Error that conforms with rfc7807 use the correct type
if (body instanceof Error && body.status && !body.statusCode) {
type = 'application/problem+json';
} else {
type = 'application/json';
}
}

// Derive type if not provided by the user
Expand Down
2 changes: 1 addition & 1 deletion lib/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1435,7 +1435,7 @@ Server.prototype._routeErrorResponse = function _routeErrorResponse(
}

// only automatically send errors that are known (e.g., restify-errors)
if (err instanceof Error && _.isNumber(err.statusCode)) {
if (err instanceof Error && _.isNumber(err.statusCode || err.status)) {
Copy link
Member

Choose a reason for hiding this comment

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

when is this status? I
restify-errors does statusCode
https://github.com/restify/errors/blob/master/lib/baseClasses/HttpError.js#L54

Copy link
Member Author

Choose a reason for hiding this comment

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

exactly, and restify-errors doesn't conform to rfc7807, which defines status. The problem is that it assumes that users are indeed using restify-errors and don't give them the ability to follow the standard.

This PR keeps statusCode for backward compatibility with restify-errors but allows restify to return the correct content type application/problem+json when status is used and no statusCode is present

Copy link
Member Author

Choose a reason for hiding this comment

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

class NotFound extends Error {
  constructor(detail) {
    this.title = 'Not Found';
    this.type = 'https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/404';
    this.status = 404;
    this.detail = detail || 'Resource not found';
  }
  
  toJSON() {
    return this;
  }
}

this will conform with rfc7807 and will hit that code with status

Copy link
Member Author

Choose a reason for hiding this comment

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

You know what? It doesn't really need to "have a status property instead of a statusCode" to conform to the rfc. I was mainly using status property as a way to identify that was an error message complying with the rfc and sending the correct content-type.

But we can have a statusCode property in the error object and on the toJSON method that I serialize it to status instead, but I would need to find a way to set the application/problem+json content type and still keep backward compatibility. Any thoughts on this?

res.send(err);
return;
}
Expand Down
35 changes: 35 additions & 0 deletions test/response.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,3 +680,38 @@ test('GH-1607: should send numbers with explicit status code', function(t) {
});
});
});

test('should handle errors with a status property', function(t) {
var notFound = new Error('not found');
notFound.status = 404;
notFound.title = 'Not Found';

SERVER.get('/17', function handle(req, res, next) {
return next(notFound);
});

CLIENT.get(join(LOCALHOST, '/17'), function(err, _, res, body) {
t.equal(res.statusCode, 404);
t.equal(res.headers['content-type'], 'application/problem+json');
t.equal(body.title, notFound.title);
t.end();
});
});

test('should prefer error statusCode property over status', function(t) {
var notFound = new Error('not found');
notFound.statusCode = 404;
notFound.status = 500;
notFound.title = 'Not Found';

SERVER.get('/18', function handle(req, res, next) {
return next(notFound);
});

CLIENT.get(join(LOCALHOST, '/18'), function(err, _, res, body) {
t.equal(res.statusCode, 404);
t.equal(res.headers['content-type'], 'application/json');
t.equal(body.title, notFound.title);
t.end();
});
});
3 changes: 2 additions & 1 deletion test/server.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2333,7 +2333,8 @@ test('should show debug information', function(t) {
'application/javascript',
'application/json',
'text/plain',
'application/octet-stream'
'application/octet-stream',
'application/problem+json'
]);
t.equal(debugInfo.server.address, '127.0.0.1');
t.equal(typeof debugInfo.server.port, 'number');
Expand Down