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.

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(common): add error options object #10460

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
b5fe06d
feat(common): add error options object
thiagomini Oct 25, 2022
f5edb0f
chore(common): add deprecated warning
thiagomini Oct 26, 2022
587fbae
feat(common): add error cause option
thiagomini Oct 26, 2022
071c4c5
refactor(common): change parameter assignment
thiagomini Oct 26, 2022
d63b6df
refactor(common): rearrange test blocks
thiagomini Oct 26, 2022
7e763fa
refactor(common): rename method parameter
thiagomini Oct 26, 2022
1a8ada5
test(common): add http exception test
thiagomini Oct 26, 2022
4ded779
refactor(common): add description attribute
thiagomini Oct 27, 2022
c5c818e
chore(common): change deprecated message
thiagomini Oct 27, 2022
60cb953
feat(common): add error cause option
thiagomini Oct 27, 2022
7a71035
refactor(common): extract description and options
thiagomini Oct 27, 2022
a4a8557
refactor(common): extract description and options
thiagomini Oct 27, 2022
5a186af
docs(common): add static method docs
thiagomini Oct 27, 2022
86c5084
test(common): test error cause option
thiagomini Oct 27, 2022
472c711
test(common): aggregate exception tests
thiagomini Oct 27, 2022
88ac6aa
feat(common): add error cause option
thiagomini Oct 27, 2022
1dff9c0
docs(common): update exception docs
thiagomini Oct 27, 2022
c3ced1d
feat(common): add error cause option
thiagomini Oct 27, 2022
f629dc1
feat(common): add error cause option
thiagomini Oct 27, 2022
bd7daf1
feat(common): add error cause option
thiagomini Oct 27, 2022
0328a5c
feat(common): add error cause option
thiagomini Oct 27, 2022
11d2763
feat(common): add error cause option
thiagomini Oct 27, 2022
b815143
feat(common): add error cause option
thiagomini Oct 27, 2022
789e99c
feat(common): add error cause option
thiagomini Oct 27, 2022
cd44ff3
feat(common): add error cause option
thiagomini Oct 27, 2022
3a10258
feat(common): add error cause option
thiagomini Oct 27, 2022
d569aaa
feat(common): add error cause option
thiagomini Oct 27, 2022
2416df6
feat(common): add error cause option
thiagomini Oct 27, 2022
dfc28fc
feat(common): add error cause option
thiagomini Oct 27, 2022
d40ebd1
feat(common): add error cause option
thiagomini Oct 27, 2022
5031f41
feat(common): add error cause option
thiagomini Oct 27, 2022
e8abd9a
feat(common): add error cause option
thiagomini Oct 27, 2022
f62acac
feat(common): add error cause option
thiagomini Oct 27, 2022
b4aca5d
feat(common): add error cause option
thiagomini Oct 27, 2022
379b82c
feat(common): add error cause option
thiagomini Oct 27, 2022
971ce5f
test(common): reuse error cause variable
thiagomini Oct 27, 2022
45b6cc1
test(common): add http exception tests
thiagomini Oct 27, 2022
ab881e3
test(common): add http exception tests
thiagomini Oct 27, 2022
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
21 changes: 18 additions & 3 deletions packages/common/exceptions/bad-request.exception.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { HttpStatus } from '../enums/http-status.enum';
import { HttpException } from './http.exception';
import { isString } from '../utils/shared.utils';
import { HttpException, HttpExceptionOptions } from './http.exception';

/**
* Defines an HTTP exception for *Bad Request* type errors.
Expand Down Expand Up @@ -31,19 +32,33 @@ export class BadRequestException extends HttpException {
* and return it as the JSON response body.
*
* @param objectOrError string or object describing the error condition.
* @param description a short description of the HTTP error.
* @param descriptionOrOptions a short description of the HTTP error.
*/
constructor(
objectOrError?: string | object | any,
description = 'Bad Request',
descriptionOrOptions:
| string
| (HttpExceptionOptions & { description?: string }) = 'Bad Request',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inline type here is provisory. I may change it as soon as we validate an interface for these errors.

) {
let description: string;
let httpExceptionOptions: HttpExceptionOptions;

if (isString(descriptionOrOptions)) {
description = descriptionOrOptions;
httpExceptionOptions = {};
} else {
description = descriptionOrOptions.description;
httpExceptionOptions = descriptionOrOptions;
}

super(
HttpException.createBody(
objectOrError,
description,
HttpStatus.BAD_REQUEST,
),
HttpStatus.BAD_REQUEST,
httpExceptionOptions,
);
}
}
33 changes: 29 additions & 4 deletions packages/common/exceptions/http.exception.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { Logger } from '../services';
import { isObject, isString } from '../utils/shared.utils';

export type HttpExceptionOptions = {
thiagomini marked this conversation as resolved.
Show resolved Hide resolved
cause?: Error;
};

/**
* Defines the base Nest HTTP exception, which is handled by the default
* Exceptions Handler.
Expand All @@ -13,12 +18,22 @@ export class HttpException extends Error {
* Instantiate a plain HTTP Exception.
*
* @example
* `throw new HttpException()`
* throw new HttpException()
* throw new HttpException('message', HttpStatus.BAD_REQUEST)
* throw new HttpException({ reason: 'this can be a human readable reason' }, HttpStatus.BAD_REQUEST)
* throw new HttpException(new Error('Cause Error'), HttpStatus.BAD_REQUEST)
* throw new HttpException('custom message', HttpStatus.BAD_REQUEST, {
* cause: new Error('Cause Error'),
* })
Comment on lines +27 to +33
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a bunch of examples to better illustrate the possibilities here.

*
*
* @usageNotes
* The constructor arguments define the response and the HTTP response status code.
* - The `response` argument (required) defines the JSON response body.
* - The `response` argument (required) defines the JSON response body. alternatively, it can also be
* an error object that is used to define an error [cause](https://nodejs.org/en/blog/release/v16.9.0/#error-cause).
* - The `status` argument (required) defines the HTTP Status Code.
* - The `options` argument (optional) defines additional error options. Currently, it supports the `cause` attribute,
* and can be used as an alternative way to specify the error cause: `const error = new HttpException('description', 400, { cause: new Error() });`
*
* By default, the JSON response body contains two properties:
* - `statusCode`: the Http Status Code.
Expand All @@ -31,12 +46,14 @@ export class HttpException extends Error {
* The `status` argument is required, and should be a valid HTTP status code.
* Best practice is to use the `HttpStatus` enum imported from `nestjs/common`.
*
* @param response string or object describing the error condition.
* @param response string, object describing the error condition or the error cause.
* @param status HTTP response status code.
* @param options An object used to add an error cause.
*/
constructor(
private readonly response: string | Record<string, any>,
private readonly status: number,
private readonly options?: HttpExceptionOptions,
) {
super();
this.initMessage();
Expand All @@ -53,8 +70,16 @@ export class HttpException extends Error {
* - https://nodejs.org/en/blog/release/v16.9.0/#error-cause
* - https://github.com/microsoft/TypeScript/issues/45167
*/
public initCause() {
public initCause(): void {
if (this.options?.cause) {
this.cause = this.options.cause;
return;
}

if (this.response instanceof Error) {
Copy link
Member

Choose a reason for hiding this comment

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

We have yet to define what is the expected behavior when someone mistakenly passes both the first argument as an Error and also the options object with a cause. In that case, should we always consider the cause property or throw an error, @kamilmysliwiec ?

Following up on this ☝️ here.

Since we now have a dedicated options object, perhaps we can just log the warning down when this.response instanceof Error === true saying that from now on, the { cause: ... } option should be used instead (and the previous syntax is deprecated)?

Copy link
Contributor Author

@thiagomini thiagomini Oct 26, 2022

Choose a reason for hiding this comment

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

Yes, we could do that, but then this would be a breaking change to the users that are already using the first parameter as the error cause. Is it okay to break this interface and just log a warning?

Copy link
Member

Choose a reason for hiding this comment

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

@thiagomini the idea is to log the warning now and introduce a breaking change as part of the next major release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmm I see, alright then, I'll keep both options and just log the deprecated message.

Logger.warn(
'Deprecated: Passing the error cause as the first argument to HttpException constructor is deprecated. You should use the "options" parameter instead: new HttpException("message", 400, { cause: new Error("Some Error") }) ',
thiagomini marked this conversation as resolved.
Show resolved Hide resolved
);
this.cause = this.response;
}
}
Expand Down
150 changes: 91 additions & 59 deletions packages/common/test/exceptions/http.exception.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,51 +6,59 @@ import {
} from '../../exceptions';

describe('HttpException', () => {
it('should return a response as a string when input is a string', () => {
const message = 'My error message';
expect(new HttpException(message, 404).getResponse()).to.be.eql(
'My error message',
);
});

it('should return a response as an object when input is an object', () => {
const message = {
msg: 'My error message',
reason: 'this can be a human readable reason',
anything: 'else',
};
expect(new HttpException(message, 404).getResponse()).to.be.eql(message);
});
describe('getResponse', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, I decided to group the related tests together under a common describe block. Many tests were actually testing the result for getResponse() method, while others were related to serialization. This aggregation made it easier for me to understand what was being tested. If you have strong feelings against it @kamilmysliwiec , let me know and I can revert it. However, this separation helped me notice there was a duplicated test for serialization. the test under the otherwise describe block was basically the same as the it('should be serializable', () => { }. So, in conclusion, I think this was a positive change.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

it('should return a response as a string when input is a string', () => {
const message = 'My error message';
expect(new HttpException(message, 404).getResponse()).to.be.eql(
'My error message',
);
});

it('should return a message from a built-in exception as an object', () => {
const message = 'My error message';
expect(new BadRequestException(message).getResponse()).to.be.eql({
statusCode: 400,
error: 'Bad Request',
message: 'My error message',
it('should return a response as an object when input is an object', () => {
const message = {
msg: 'My error message',
reason: 'this can be a human readable reason',
anything: 'else',
};
expect(new HttpException(message, 404).getResponse()).to.be.eql(message);
});
});

it('should return an object even when the message is undefined', () => {
expect(new BadRequestException().getResponse()).to.be.eql({
statusCode: 400,
message: 'Bad Request',
it('should return a message from a built-in exception as an object', () => {
const message = 'My error message';
expect(new BadRequestException(message).getResponse()).to.be.eql({
statusCode: 400,
error: 'Bad Request',
message: 'My error message',
});
});
});

it('should return a status code', () => {
expect(new BadRequestException().getStatus()).to.be.eql(400);
expect(new NotFoundException().getStatus()).to.be.eql(404);
it('should return an object even when the message is undefined', () => {
expect(new BadRequestException().getResponse()).to.be.eql({
statusCode: 400,
message: 'Bad Request',
});
});
});

it('should return a response', () => {
expect(new BadRequestException().getResponse()).to.be.eql({
message: 'Bad Request',
statusCode: 400,
describe('built-in exceptions', () => {
describe('getStatus', () => {
it('should return given status code', () => {
expect(new BadRequestException().getStatus()).to.be.eql(400);
expect(new NotFoundException().getStatus()).to.be.eql(404);
});
});
expect(new NotFoundException().getResponse()).to.be.eql({
message: 'Not Found',
statusCode: 404,

describe('getResponse', () => {
it('should return a response with default message and status code', () => {
expect(new BadRequestException().getResponse()).to.be.eql({
message: 'Bad Request',
statusCode: 400,
});
expect(new NotFoundException().getResponse()).to.be.eql({
message: 'Not Found',
statusCode: 404,
});
});
});
});

Expand All @@ -59,31 +67,28 @@ describe('HttpException', () => {
expect(error instanceof Error).to.be.true;
});

it('should be serializable', () => {
const message = 'Some Error';
const error = new HttpException(message, 400);
expect(`${error}`).to.be.eql(`HttpException: ${message}`);
});
describe('when serializing', () => {
describe('and "response" parameter is a string', () => {
it('should concatenate HttpException with the given message', () => {
const responseAsString = 'Some Error';
const error = new HttpException(responseAsString, 400);
expect(`${error}`).to.be.eql(`HttpException: ${responseAsString}`);
expect(`${error}`.includes('[object Object]')).to.not.be.true;
});
});

describe('when "response" is an object', () => {
it('should use default message', () => {
const obj = { foo: 'bar' };
const error = new HttpException(obj, 400);
const badRequestError = new BadRequestException(obj);
describe('and "response" parameter is an object', () => {
it('should use default message', () => {
const responseAsObject = { foo: 'bar' };
const error = new HttpException(responseAsObject, 400);
const badRequestError = new BadRequestException(responseAsObject);

expect(`${error}`).to.be.eql(`HttpException: Http Exception`);
expect(`${badRequestError}`).to.be.eql(
`BadRequestException: Bad Request Exception`,
);
expect(`${error}`.includes('[object Object]')).to.not.be.true;
expect(`${badRequestError}`.includes('[object Object]')).to.not.be.true;
});
describe('otherwise', () => {
it('should concat strings', () => {
const test = 'test message';
const error = new HttpException(test, 400);
expect(`${error}`).to.be.eql(`HttpException: ${test}`);
expect(`${error}`).to.be.eql(`HttpException: Http Exception`);
expect(`${badRequestError}`).to.be.eql(
`BadRequestException: Bad Request Exception`,
);
expect(`${error}`.includes('[object Object]')).to.not.be.true;
expect(`${badRequestError}`.includes('[object Object]')).to.not.be.true;
});
});
});
Expand Down Expand Up @@ -139,5 +144,32 @@ describe('HttpException', () => {

expect(cause).to.be.eql(message);
});

it('configures a cause when message is a string and the options object is passed', () => {
const causeError = new Error('Some Error');

const customDescription = 'custom description';
const error = new HttpException(customDescription, 400, {
cause: causeError,
});

expect(`${error}`).to.be.eql(`HttpException: ${customDescription}`);
const { cause } = error;

expect(cause).to.be.eql(causeError);
});

it('configures a cause when using a built-in exception with options', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a POC for how we could use the options object with other built-in exceptions, like the BadRequestException. The modification here, as you can see in the class implementation, was to make the second argument either an object or a string. When it's an object, then we consider it as an option, otherwise, we consider it to be the old description parameter. @kamilmysliwiec , what do you think about that interface?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds great!

const causeError = new Error('Some Error');

const customDescription = 'custom description';
const error = new BadRequestException(customDescription, {
cause: causeError,
});

const { cause } = error;

expect(cause).to.be.eql(causeError);
});
});
});