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
Changes from 4 commits
b5fe06d
f5edb0f
587fbae
071c4c5
d63b6df
7e763fa
1a8ada5
4ded779
c5c818e
60cb953
7a71035
a4a8557
5a186af
86c5084
472c711
88ac6aa
1dff9c0
c3ced1d
f629dc1
bd7daf1
0328a5c
11d2763
b815143
789e99c
cd44ff3
3a10258
d569aaa
2416df6
dfc28fc
d40ebd1
5031f41
e8abd9a
f62acac
b4aca5d
379b82c
971ce5f
45b6cc1
ab881e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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(); | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Following up on this ☝️ here. Since we now have a dedicated options object, perhaps we can just log the warning down when There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,5 +139,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', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a POC for how we could use the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
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.