fix(mvc): Fix error 500 when endpoint return nothing and status is 204

tsedio · Dec 24, 2018 · 9ee0185 · 9ee0185
1 parent 1d2f0b0
commit 9ee0185
Show file tree

Hide file tree

Showing 7 changed files with 119 additions and 48 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -6,7 +6,7 @@ notifications:
   email: false
   webhooks:
     urls:
-      - https://webhooks.gitter.im/e/c28608f09c428355a2ef
+      - $GITTER_ROOM_URL
     on_success: change  # options: [always|never|change] default: always
     on_failure: always  # options: [always|never|change] default: always
     on_start: never     # options: [always|never|change] default: always

diff --git a/docs/docs/middlewares/override/send-response.md b/docs/docs/middlewares/override/send-response.md
@@ -3,33 +3,42 @@
 There is the current implementation of the [SendResponseMiddleware](/api/common/mvc/components/SendResponseMiddleware.md):
 
 ```typescript
+import * as Express from "express";
+import {ConverterService} from "../../converters/services/ConverterService";
+import {Response} from "../../filters/decorators/response";
+import {ResponseData} from "../../filters/decorators/responseData";
+
+import {Middleware} from "../decorators/class/middleware";
+import {IMiddleware} from "../interfaces/index";
+import {EndpointInfo, EndpointMetadata} from "@tsed/common";
+
 @Middleware()
 export class SendResponseMiddleware implements IMiddleware {
+  constructor(protected converterService: ConverterService) {
+  }
 
-    constructor(private converterService: ConverterService) {
+  public use(@ResponseData() data: any, @Response() response: Express.Response, @EndpointInfo() endpoint: EndpointMetadata) {
+    const type = typeof data;
 
-    }
-
-    public use(@ResponseData() data: any, @Response() response: Express.Response) {
+    if (endpoint.statusCode === 204) {
+      response.send();
 
-        if (response.headersSent) {
-            return;
-        }
+      return;
+    }
 
-        const type = typeof data;
+    if (data === undefined) {
+      return;
+    }
 
-        if (data === undefined) {
-            response.send("");
-        } else if (data === null || ["number", "boolean", "string"].indexOf(type) > -1) {
-            response.send(String(data));
-        } else {
-            response.setHeader("Content-Type", "text/json");
-            response.json(this.converterService.serialize(data));
-        }
+    if (data === null || ["number", "boolean", "string"].indexOf(type) > -1) {
+      response.send(data && String(data));
 
+      return;
     }
-}
 
+    response.json(this.converterService.serialize(data));
+  }
+}
 ```
 
 But for some reason, this implementation isn't enough to meet your needs.

diff --git a/packages/common/src/mvc/class/EndpointMetadata.ts b/packages/common/src/mvc/class/EndpointMetadata.ts
@@ -192,6 +192,10 @@ export class EndpointMetadata extends Storable {
     return this._inheritedEndpoint ? this._inheritedEndpoint.store : this._store;
   }
 
+  get statusCode() {
+    return this.store.get("statusCode") || 200;
+  }
+
   /**
    * Find the a value at the controller level. Let this value be extended or overridden by the endpoint itself.
    *
@@ -232,7 +236,7 @@ export class EndpointMetadata extends Storable {
       this.collectionType = collectionType;
     }
 
-    const expectedStatus = this.store.get("statusCode") || 200;
+    const expectedStatus = this.statusCode;
 
     if (+code === +expectedStatus) {
       const response = this.store.get("response");

diff --git a/packages/common/src/mvc/components/SendResponseMiddleware.ts b/packages/common/src/mvc/components/SendResponseMiddleware.ts
@@ -5,24 +5,31 @@ import {ResponseData} from "../../filters/decorators/responseData";
 
 import {Middleware} from "../decorators/class/middleware";
 import {IMiddleware} from "../interfaces/index";
+import {EndpointInfo, EndpointMetadata} from "@tsed/common";
 
-/**
- * @private
- * @middleware
- */
 @Middleware()
 export class SendResponseMiddleware implements IMiddleware {
   constructor(protected converterService: ConverterService) {}
 
-  public use(@ResponseData() data: any, @Response() response: Express.Response) {
+  public use(@ResponseData() data: any, @Response() response: Express.Response, @EndpointInfo() endpoint: EndpointMetadata) {
     const type = typeof data;
 
-    if (data !== undefined) {
-      if (data === null || ["number", "boolean", "string"].indexOf(type) > -1) {
-        response.send(String(data));
-      } else {
-        response.json(this.converterService.serialize(data));
-      }
+    if (endpoint.statusCode === 204) {
+      response.send();
+
+      return;
+    }
+
+    if (data === undefined) {
+      return;
     }
+
+    if (data === null || ["number", "boolean", "string"].indexOf(type) > -1) {
+      response.send(data && String(data));
+
+      return;
+    }
+
+    response.json(this.converterService.serialize(data));
   }
 }
diff --git a/test/integration/app/controllers/users/UserCtrl.ts b/test/integration/app/controllers/users/UserCtrl.ts
@@ -1,4 +1,4 @@
-import {BodyParams, Controller, Get, PathParams, Post, ProviderScope, Required, Scope, Status} from "@tsed/common";
+import {BodyParams, Controller, Get, PathParams, Post, ProviderScope, Required, Scope, Session, Status} from "@tsed/common";
 import {MultipartFile} from "../../../../../packages/multipartfiles/src";
 import {Docs, Hidden} from "../../../../../packages/swagger/src";
 import {IUser, User} from "../../models/User";
@@ -13,6 +13,17 @@ export class UserCtrl {
 
   constructor(public userService: UserService) {}
 
+  @Post("/connect")
+  @Status(204)
+  async connect(@Session() session: Express.Session, @BodyParams("user") user: User) {
+    session.user = user;
+  }
+
+  @Get("/get-connected")
+  async getConnected(@Session("user") user: User): Promise<IUser> {
+    return user;
+  }
+
   @Post("/")
   @Status(201)
   async createUser(@BodyParams() userData: User) {

diff --git a/test/units/di/decorators/injectable.spec.ts b/test/units/di/decorators/injectable.spec.ts
@@ -7,20 +7,40 @@ describe("@Injectable()", () => {
 
   class Test {}
 
-  before(() => {
-    sandbox.stub(ProviderRegistry, "registerProvider");
+  describe("with options", () => {
+    before(() => {
+      sandbox.stub(ProviderRegistry, "registerProvider");
 
-    Injectable({options: "options"})(Test);
-  });
+      Injectable({options: "options"})(Test);
+    });
 
-  after(() => {
-    sandbox.restore();
+    after(() => {
+      sandbox.restore();
+    });
+
+    it("should called registerProvider", () => {
+      ProviderRegistry.registerProvider.should.have.been.calledWithExactly({
+        options: "options",
+        provide: Test
+      });
+    });
   });
 
-  it("should called registerProvider", () => {
-    ProviderRegistry.registerProvider.should.have.been.calledWithExactly({
-      options: "options",
-      provide: Test
+  describe("without options", () => {
+    before(() => {
+      sandbox.stub(ProviderRegistry, "registerProvider");
+
+      Injectable()(Test);
+    });
+
+    after(() => {
+      sandbox.restore();
+    });
+
+    it("should called registerProvider", () => {
+      ProviderRegistry.registerProvider.should.have.been.calledWithExactly({
+        provide: Test
+      });
     });
   });
 });
diff --git a/test/units/mvc/components/SendResponseMiddleware.spec.ts b/test/units/mvc/components/SendResponseMiddleware.spec.ts
@@ -15,7 +15,7 @@ describe("SendResponseMiddleware", () => {
     before(() => {
       this.fakeResponse = new FakeResponse();
 
-      this.middleware.use(true, this.fakeResponse as any);
+      this.middleware.use(true, this.fakeResponse as any, {});
     });
 
     after(() => {
@@ -35,7 +35,7 @@ describe("SendResponseMiddleware", () => {
   describe("with number value", () => {
     before(() => {
       this.fakeResponse = new FakeResponse();
-      this.middleware.use(1, this.fakeResponse as any);
+      this.middleware.use(1, this.fakeResponse as any, {});
     });
 
     after(() => {
@@ -55,7 +55,7 @@ describe("SendResponseMiddleware", () => {
   describe("with null value", () => {
     before(() => {
       this.fakeResponse = new FakeResponse();
-      this.middleware.use(null, this.fakeResponse as any);
+      this.middleware.use(null, this.fakeResponse as any, {});
     });
 
     after(() => {
@@ -75,7 +75,7 @@ describe("SendResponseMiddleware", () => {
   describe("with undefined value", () => {
     before(() => {
       this.fakeResponse = new FakeResponse();
-      this.middleware.use(undefined, this.fakeResponse as any);
+      this.middleware.use(undefined, this.fakeResponse as any, {});
     });
 
     after(() => {
@@ -92,12 +92,32 @@ describe("SendResponseMiddleware", () => {
     });
   });
 
+  describe("with undefined value and 204 statusCode", () => {
+    before(() => {
+      this.fakeResponse = new FakeResponse();
+      Sinon.stub(this.fakeResponse, "send");
+      this.middleware.use(undefined, this.fakeResponse as any, {statusCode: 204});
+    });
+
+    after(() => {
+      this.serializeStub.reset();
+    });
+
+    it("should not call serialize method", () => {
+      return this.serializeStub.should.not.have.been.called;
+    });
+
+    it("should return a string of the value", () => {
+      return this.fakeResponse.send.should.have.been.called;
+    });
+  });
+
   describe("with date value", () => {
     before(() => {
       this.date = new Date();
       this.serializeStub.returns("dataSerialized");
       this.fakeResponse = new FakeResponse();
-      this.middleware.use(this.date, this.fakeResponse as any);
+      this.middleware.use(this.date, this.fakeResponse as any, {});
     });
 
     after(() => {
@@ -119,7 +139,7 @@ describe("SendResponseMiddleware", () => {
       this.data = {data: "data"};
       this.serializeStub.returns("dataSerialized");
       this.fakeResponse = new FakeResponse();
-      this.middleware.use(this.data, this.fakeResponse as any);
+      this.middleware.use(this.data, this.fakeResponse as any, {});
     });
 
     after(() => {
@@ -141,7 +161,7 @@ describe("SendResponseMiddleware", () => {
       this.data = {data: "data"};
       this.serializeStub.returns("dataSerialized");
       this.fakeResponse = new FakeResponse();
-      this.middleware.use(this.data, this.fakeResponse as any);
+      this.middleware.use(this.data, this.fakeResponse as any, {});
     });
 
     after(() => {