Merge pull request nestjs#3053 from nestjs/refactor/http-message-property

kamilmysliwiec · web-flow · commit 0e661c88ac0b · 2020-03-02T15:44:13.000+01:00
refactor(core) make message property (http errors) serializable
diff --git a/integration/graphql-code-first/e2e/pipes.spec.ts b/integration/graphql-code-first/e2e/pipes.spec.ts
@@ -32,24 +32,7 @@ describe('GraphQL Pipes', () => {
             extensions: {
               code: 'INTERNAL_SERVER_ERROR',
               exception: {
-                message: {
-                  error: 'Bad Request',
-                  message: [
-                    {
-                      children: [],
-                      constraints: {
-                        length:
-                          'description must be longer than or equal to 30 characters',
-                      },
-                      property: 'description',
-                      target: {
-                        ingredients: [],
-                        title: 'test',
-                      },
-                    },
-                  ],
-                  statusCode: 400,
-                },
+                message: 'Bad Request Exception',
                 response: {
                   error: 'Bad Request',
                   message: [
@@ -77,24 +60,7 @@ describe('GraphQL Pipes', () => {
                 line: 2,
               },
             ],
-            message: {
-              error: 'Bad Request',
-              message: [
-                {
-                  children: [],
-                  constraints: {
-                    length:
-                      'description must be longer than or equal to 30 characters',
-                  },
-                  property: 'description',
-                  target: {
-                    ingredients: [],
-                    title: 'test',
-                  },
-                },
-              ],
-              statusCode: 400,
-            },
+            message: 'Bad Request Exception',
             path: ['addRecipe'],
           },
         ],
diff --git a/integration/hello-world/e2e/fastify-adapter.spec.ts b/integration/hello-world/e2e/fastify-adapter.spec.ts
@@ -59,7 +59,7 @@ describe('Hello world (fastify adapter)', () => {
           fail(`Unexpected success: ${payload}`);
         },
         err => {
-          expect(err.message).to.be.eql({
+          expect(err.getResponse()).to.be.eql({
             error: 'Internal Server Error',
             message:
               'HTTP adapter does not support filtering on host: ":tenant.example.com"',
diff --git a/packages/common/exceptions/http.exception.ts b/packages/common/exceptions/http.exception.ts
@@ -9,7 +9,6 @@ import { isObject, isString } from '../utils/shared.utils';
  * @publicApi
  */
 export class HttpException extends Error {
-  public readonly message: any;
   /**
    * Instantiate a plain HTTP Exception.
    *
@@ -36,11 +35,26 @@ export class HttpException extends Error {
    * @param status HTTP response status code
    */
   constructor(
-    private readonly response: string | object,
+    private readonly response: string | Record<string, any>,
     private readonly status: number,
   ) {
     super();
-    this.message = response;
+    this.initMessage();
+  }
+
+  public initMessage() {
+    if (isString(this.response)) {
+      this.message = this.response;
+    } else if (
+      isObject(this.response) &&
+      isString((this.response as Record<string, any>).message)
+    ) {
+      this.message = (this.response as Record<string, any>).message;
+    } else if (this.constructor) {
+      this.message = this.constructor.name
+        .match(/[A-Z][a-z]+|[0-9]+/g)
+        .join(' ');
+    }
   }
 
   public getResponse(): string | object {
@@ -51,15 +65,6 @@ export class HttpException extends Error {
     return this.status;
   }
 
-  public toString(): string {
-    const message = this.getErrorString(this.message);
-    return `Error: ${message}`;
-  }
-
-  private getErrorString(target: string | object): string {
-    return isString(target) ? target : JSON.stringify(target);
-  }
-
   public static createBody(
     message: object | string,
     error?: string,
diff --git a/packages/common/test/exceptions/http.exception.spec.ts b/packages/common/test/exceptions/http.exception.spec.ts
@@ -6,33 +6,33 @@ import {
 } from '../../exceptions';
 
 describe('HttpException', () => {
-  it('should return a message as a string when input is a string', () => {
+  it('should return a response as a string when input is a string', () => {
     const message = 'My error message';
-    expect(new HttpException(message, 404).message).to.be.eql(
+    expect(new HttpException(message, 404).getResponse()).to.be.eql(
       'My error message',
     );
   });
 
-  it('should return a message as an object when input is an object', () => {
-    const message: object = {
+  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).message).to.be.eql(message);
+    expect(new HttpException(message, 404).getResponse()).to.be.eql(message);
   });
 
   it('should return a message from a built-in exception as an object', () => {
     const message = 'My error message';
-    expect(new BadRequestException(message).message).to.be.eql({
+    expect(new BadRequestException(message).getResponse()).to.be.eql({
       statusCode: 400,
       error: 'Bad Request',
       message: 'My error message',
     });
   });
 
   it('should return an object even when the message is undefined', () => {
-    expect(new BadRequestException().message).to.be.eql({
+    expect(new BadRequestException().getResponse()).to.be.eql({
       statusCode: 400,
       error: 'Bad Request',
     });
@@ -65,17 +65,24 @@ describe('HttpException', () => {
     expect(`${error}`).to.be.eql(`Error: ${message}`);
   });
 
-  describe('when "message" is an object', () => {
-    it('should serialize an object', () => {
+  describe('when "response" is an object', () => {
+    it('should use default message', () => {
       const obj = { foo: 'bar' };
       const error = new HttpException(obj, 400);
-      expect(`${error}`).to.be.eql(`Error: ${JSON.stringify(obj)}`);
+      const badRequestError = new BadRequestException(obj);
+
+      expect(`${error}`).to.be.eql(`Error: Http Exception`);
+      expect(`${badRequestError}`).to.be.eql(`Error: Bad Request Exception`);
       expect(`${error}`.includes('[object Object]')).to.not.be.true;
+      expect(`${badRequestError}`.includes('[object Object]')).to.not.be.true;
     });
-
-    it('should serialize sub errors', () => {
-      const error = new NotFoundException();
-      expect(`${error}`.includes('Not Found')).to.be.true;
+    describe('otherwise', () => {
+      it('should concat strings', () => {
+        const test = 'test message';
+        const error = new HttpException(test, 400);
+        expect(`${error}`).to.be.eql(`Error: ${test}`);
+        expect(`${error}`.includes('[object Object]')).to.not.be.true;
+      });
     });
   });
 
diff --git a/packages/core/test/router/router-execution-context.spec.ts b/packages/core/test/router/router-execution-context.spec.ts
@@ -1,7 +1,7 @@
 import { ForbiddenException } from '@nestjs/common/exceptions/forbidden.exception';
 import { expect } from 'chai';
 import * as sinon from 'sinon';
-import { HttpStatus, RouteParamMetadata } from '../../../common';
+import { HttpException, HttpStatus, RouteParamMetadata } from '../../../common';
 import { CUSTOM_ROUTE_AGRS_METADATA } from '../../../common/constants';
 import { RouteParamtypes } from '../../../common/enums/route-paramtypes.enum';
 import { AbstractHttpAdapter } from '../../adapters';
@@ -137,14 +137,16 @@ describe('RouterExecutionContext', () => {
           });
           it('should throw exception when "tryActivate" returns false', async () => {
             tryActivateStub.callsFake(async () => false);
-            let error: Error;
+
+            let error: HttpException;
             try {
               await proxyContext(request, response, next);
             } catch (e) {
               error = e;
             }
             expect(error).to.be.instanceOf(ForbiddenException);
-            expect(error.message).to.be.eql({
+            expect(error.message).to.be.eql('Forbidden resource');
+            expect(error.getResponse()).to.be.eql({
               statusCode: HttpStatus.FORBIDDEN,
               error: 'Forbidden',
               message: FORBIDDEN_MESSAGE,
@@ -297,14 +299,17 @@ describe('RouterExecutionContext', () => {
     it('should throw ForbiddenException when "tryActivate" returns false', async () => {
       const guardsFn = contextCreator.createGuardsFn([null], null, null);
       sinon.stub(guardsConsumer, 'tryActivate').callsFake(async () => false);
+
       let error: ForbiddenException;
       try {
         await guardsFn([]);
       } catch (e) {
         error = e;
       }
+
       expect(error).to.be.instanceOf(ForbiddenException);
-      expect(error.message).to.be.eql({
+      expect(error.message).to.be.eql('Forbidden resource');
+      expect(error.getResponse()).to.be.eql({
         statusCode: HttpStatus.FORBIDDEN,
         error: 'Forbidden',
         message: FORBIDDEN_MESSAGE,
