feat(common): add error options object #10460

thiagomini · 2022-10-25T21:15:17Z

add error options object to HttpException constructor to allow use of error cause along with custom message.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

We can only pass either a custom description or a cause error object.

Issue Number: #10392

What is the new behavior?

We can now use both custom messages and error cause with the following API:

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

Does this PR introduce a breaking change?

Yes
No

Other information

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 ?

add error options object to HttpException constructor to allow use of error cause along with custom message.

thiagomini · 2022-10-25T21:16:48Z

packages/common/exceptions/http.exception.ts

+   * 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'),
+   * })


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

coveralls · 2022-10-25T21:20:56Z

Pull Request Test Coverage Report for Build 47ec2c16-536f-4fcd-8acd-7e7f64581b60

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 93.804%

Totals
Change from base Build ee4c81ca-3d86-452b-91d8-8e84e7b63778:	0.02%
Covered Lines:	6116
Relevant Lines:	6520

💛 - Coveralls

kamilmysliwiec · 2022-10-26T08:31:43Z

packages/common/exceptions/http.exception.ts

+      this.cause = this.options.cause;
+      return;
+    }
+
    if (this.response instanceof Error) {


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)?

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?

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

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

add deprecated warning for HttpException class when using the first argument as the error cause.

add error cause option to bad request exception.

change BadRequestException constructor parameters assignment to be clearer.

thiagomini · 2022-10-26T17:23:57Z

packages/common/test/exceptions/http.exception.spec.ts

+      expect(cause).to.be.eql(causeError);
+    });
+
+    it('configures a cause when using a built-in exception with options', () => {


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?

Sounds great!

thiagomini · 2022-10-26T17:24:52Z

packages/common/exceptions/bad-request.exception.ts

-    description = 'Bad Request',
+    descriptionOrOptions:
+      | string
+      | (HttpExceptionOptions & { description?: string }) = 'Bad Request',


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

rearrange HttpException spec file test blocks to increase readability, better separating tests by methods and contexts.

rename HttpException createBody method parameter to better represent its usage. When this parameter is a string, it is used as a value to a 'message' key in the final object.

add test for HttpException's getResponse method when used with a built-in exception and providing the "description" parameter as part of the "options" object.

thiagomini · 2022-10-26T18:43:15Z

packages/common/test/exceptions/http.exception.spec.ts

-    };
-    expect(new HttpException(message, 404).getResponse()).to.be.eql(message);
-  });
+  describe('getResponse', () => {


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.

Sounds good!

thiagomini · 2022-10-26T18:45:07Z

packages/common/exceptions/http.exception.ts

@@ -112,15 +112,16 @@ export class HttpException extends Error {
  }

  public static createBody(
-    objectOrError: object | string,
+    objectOrErrorMessage: object | string,


I renamed this parameter to reflect its usage better. When this is a string, it's used as the message attribute below, so this new name seemed a better fit.

thiagomini · 2022-10-26T18:46:42Z

packages/common/test/exceptions/http.exception.spec.ts

+        const badRequestError = new BadRequestException('ErrorMessage', {
+          description: 'Some error description',
+        });


This test exemplifies how we can use the description parameter inside the options object. Before my changes, the BadRequestException second argument was a simple string for the error's description. With the addition of the cause attribute, I had to put them both under the same object literal.

packages/common/exceptions/http.exception.ts

add description attribute to HttpExceptionOptions interface so we can reuse it among children exceptions.

add error cause option to BadGatewayException.

extract functions that provide valid values for description and http exception options to the HttpException class

create HttpException#extractDescriptionAndOptionsFrom method to return both description and options at the same time

add docs for HttpException.getHttpExceptionOptionsFrom explaining its intended usage.

test error cause option for BadRequestException.

aggregate HttpException children classes tests in a single run to reduce the code duplication

add error cause option to ConflictException.

update HttpException children classes docs to reflect the changes to the "description" parameter

add error cause option to ForbiddenException.

add error cause option to GatewayTimeoutException.

add error cause option to GoneException.

add error cause option to HttpVersionNotSupportedException.

add error cause option to ImATeapotException.

add error cause option to InternalServerErrorException.

add error cause option to MethodNotAllowedException.

add error cause option to MisdirectedException.

add error cause option to NotAcceptableException.

add error cause option to NotFoundException.

add error cause option to NotImplementedException.

add error cause option to PayloadTooLargeException.

add error cause option to PreconditionFailedException.

add error cause option to RequestTimeoutException.

add error cause option to ServiceUnavailableException.

add error cause option to UnauthorizedException.

add error cause option to UnprocessableEntityException.

add error cause option to UnsupportedMediaTypeException.

reuse errorCause variable in HttpException test

add tests for all HttpException children classes and fix NotAcceptableException wrong status code.

add "getStatus" tests for all HttpException children classes

thiagomini · 2022-10-27T20:43:01Z

packages/common/test/exceptions/http.exception.spec.ts

+  describe('built-in exceptions', () => {
+    describe('getStatus', () => {
+      it('should return given status code', () => {
+        const testCases: [Type<HttpException>, number][] = [


Decided to add tests here to guarantee the changes I've made didn't break anything. Fortunately, this really prevented me from breaking NotAcceptableException that I mistakenly changed its status code.

thiagomini · 2022-10-27T20:43:21Z

packages/common/test/exceptions/http.exception.spec.ts

+
+    describe('getResponse', () => {
+      it('should return a response with default message and status code', () => {
+        const testCases: [Type<HttpException>, number, string][] = [


tests for all built-in exceptions just to make sure everything is still working.

thiagomini · 2022-10-27T20:55:11Z

Hey, @kamilmysliwiec , you might want to review this commit-by-commit now that the number of files changed increased a lot.

kamilmysliwiec · 2022-11-07T09:44:17Z

LGTM

feat(common): add error options object

b5fe06d

add error options object to HttpException constructor to allow use of error cause along with custom message.

thiagomini commented Oct 25, 2022

View reviewed changes

kamilmysliwiec reviewed Oct 26, 2022

View reviewed changes

kamilmysliwiec added type: enhancement 🐺 scope: common labels Oct 26, 2022

thiagomini added 3 commits October 26, 2022 09:35

chore(common): add deprecated warning

f5edb0f

add deprecated warning for HttpException class when using the first argument as the error cause.

feat(common): add error cause option

587fbae

add error cause option to bad request exception.

refactor(common): change parameter assignment

071c4c5

change BadRequestException constructor parameters assignment to be clearer.

thiagomini commented Oct 26, 2022

View reviewed changes

thiagomini requested a review from kamilmysliwiec October 26, 2022 17:38

thiagomini added 3 commits October 26, 2022 15:10

refactor(common): rearrange test blocks

d63b6df

rearrange HttpException spec file test blocks to increase readability, better separating tests by methods and contexts.

refactor(common): rename method parameter

7e763fa

rename HttpException createBody method parameter to better represent its usage. When this parameter is a string, it is used as a value to a 'message' key in the final object.

test(common): add http exception test

1a8ada5

add test for HttpException's getResponse method when used with a built-in exception and providing the "description" parameter as part of the "options" object.

thiagomini commented Oct 26, 2022

View reviewed changes

kamilmysliwiec reviewed Oct 27, 2022

View reviewed changes

packages/common/exceptions/http.exception.ts Outdated Show resolved Hide resolved

kamilmysliwiec reviewed Oct 27, 2022

View reviewed changes

packages/common/exceptions/http.exception.ts Outdated Show resolved Hide resolved

thiagomini added 10 commits October 27, 2022 12:12

refactor(common): add description attribute

4ded779

add description attribute to HttpExceptionOptions interface so we can reuse it among children exceptions.

chore(common): change deprecated message

c5c818e

feat(common): add error cause option

60cb953

add error cause option to BadGatewayException.

refactor(common): extract description and options

7a71035

extract functions that provide valid values for description and http exception options to the HttpException class

refactor(common): extract description and options

a4a8557

create HttpException#extractDescriptionAndOptionsFrom method to return both description and options at the same time

docs(common): add static method docs

5a186af

add docs for HttpException.getHttpExceptionOptionsFrom explaining its intended usage.

test(common): test error cause option

86c5084

test error cause option for BadRequestException.

test(common): aggregate exception tests

472c711

aggregate HttpException children classes tests in a single run to reduce the code duplication

feat(common): add error cause option

88ac6aa

add error cause option to ConflictException.

docs(common): update exception docs

1dff9c0

update HttpException children classes docs to reflect the changes to the "description" parameter

thiagomini added 21 commits October 27, 2022 16:19

feat(common): add error cause option

c3ced1d

add error cause option to ForbiddenException.

feat(common): add error cause option

f629dc1

add error cause option to GatewayTimeoutException.

feat(common): add error cause option

bd7daf1

add error cause option to GoneException.

feat(common): add error cause option

0328a5c

add error cause option to HttpVersionNotSupportedException.

feat(common): add error cause option

11d2763

add error cause option to ImATeapotException.

feat(common): add error cause option

b815143

add error cause option to InternalServerErrorException.

feat(common): add error cause option

789e99c

add error cause option to MethodNotAllowedException.

feat(common): add error cause option

cd44ff3

add error cause option to MisdirectedException.

feat(common): add error cause option

3a10258

add error cause option to NotAcceptableException.

feat(common): add error cause option

d569aaa

add error cause option to NotFoundException.

feat(common): add error cause option

2416df6

add error cause option to NotImplementedException.

feat(common): add error cause option

dfc28fc

add error cause option to PayloadTooLargeException.

feat(common): add error cause option

d40ebd1

add error cause option to PreconditionFailedException.

feat(common): add error cause option

5031f41

add error cause option to RequestTimeoutException.

feat(common): add error cause option

e8abd9a

add error cause option to ServiceUnavailableException.

feat(common): add error cause option

f62acac

add error cause option to UnauthorizedException.

feat(common): add error cause option

b4aca5d

add error cause option to UnprocessableEntityException.

feat(common): add error cause option

379b82c

add error cause option to UnsupportedMediaTypeException.

test(common): reuse error cause variable

971ce5f

reuse errorCause variable in HttpException test

test(common): add http exception tests

45b6cc1

add tests for all HttpException children classes and fix NotAcceptableException wrong status code.

test(common): add http exception tests

ab881e3

add "getStatus" tests for all HttpException children classes

thiagomini commented Oct 27, 2022

View reviewed changes

thiagomini requested a review from kamilmysliwiec October 27, 2022 20:54

kamilmysliwiec merged commit 7ec60ca into nestjs:master Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): add error options object #10460

feat(common): add error options object #10460

thiagomini commented Oct 25, 2022

thiagomini Oct 25, 2022

coveralls commented Oct 25, 2022 •

edited

Loading

kamilmysliwiec Oct 26, 2022

thiagomini Oct 26, 2022 •

edited

Loading

kamilmysliwiec Oct 26, 2022

thiagomini Oct 26, 2022

thiagomini Oct 26, 2022

kamilmysliwiec Oct 27, 2022

thiagomini Oct 26, 2022

thiagomini Oct 26, 2022

kamilmysliwiec Oct 27, 2022

thiagomini Oct 26, 2022

thiagomini Oct 26, 2022

thiagomini Oct 27, 2022 •

edited

Loading

thiagomini Oct 27, 2022

thiagomini commented Oct 27, 2022

kamilmysliwiec commented Nov 7, 2022

feat(common): add error options object #10460

feat(common): add error options object #10460

Conversation

thiagomini commented Oct 25, 2022

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

coveralls commented Oct 25, 2022 • edited Loading

Pull Request Test Coverage Report for Build 47ec2c16-536f-4fcd-8acd-7e7f64581b60

💛 - Coveralls

Choose a reason for hiding this comment

thiagomini Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagomini Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagomini commented Oct 27, 2022

kamilmysliwiec commented Nov 7, 2022

coveralls commented Oct 25, 2022 •

edited

Loading

thiagomini Oct 26, 2022 •

edited

Loading

thiagomini Oct 27, 2022 •

edited

Loading