Skip to content

Commit d3005c0

Browse files
NoNameProvidedNoNameProvided
authored
Merge pull request #241 from 19majkel94/404-return-type-fix
Fix 404 text content-type for JSON controllers
2 parents 23a3af7 + e289d11 commit d3005c0

File tree

4 files changed

+163
-104
lines changed

4 files changed

+163
-104
lines changed

‎package.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,12 @@
99
"name": "Umed Khudoiberdiev",
1010
"email": "pleerock.me@gmail.com"
1111
},
12+
"contributors": [
13+
{
14+
"name": "Michał Lytek",
15+
"url": "https://github.com/19majkel94"
16+
}
17+
],
1218
"repository": {
1319
"type": "git",
1420
"url": "https://github.com/pleerock/routing-controllers.git"

‎src/driver/express/ExpressDriver.ts‎

Lines changed: 38 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {AuthorizationCheckerNotDefinedError} from "../../error/AuthorizationChec
1313
import {isPromiseLike} from "../../util/isPromiseLike";
1414
import {getFromContainer} from "../../container";
1515
import {AuthorizationRequiredError} from "../../error/AuthorizationRequiredError";
16+
import { NotFoundError } from "../../index";
1617
const cookie = require("cookie");
1718
const templateUrl = require("template-url");
1819

@@ -226,25 +227,26 @@ export class ExpressDriver extends BaseDriver implements Driver {
226227
handleSuccess(result: any, action: ActionMetadata, options: Action): void {
227228

228229
// check if we need to transform result and do it
229-
if (this.useClassTransformer && result && result instanceof Object) {
230+
if (result && result instanceof Object && this.useClassTransformer) {
230231
const options = action.responseClassTransformOptions || this.classToPlainTransformOptions;
231232
result = classToPlain(result, options);
232233
}
233234

234235
// set http status code
235-
if (action.undefinedResultCode && result === undefined) {
236-
if (action.undefinedResultCode instanceof Function)
237-
throw new (action.undefinedResultCode as any)(options);
238-
239-
options.response.status(action.undefinedResultCode);
240-
241-
} else if (action.nullResultCode && result === null) {
242-
if (action.nullResultCode instanceof Function)
243-
throw new (action.nullResultCode as any)(options);
244-
245-
options.response.status(action.nullResultCode);
246-
247-
} else if (action.successHttpCode) {
236+
if (result === undefined && action.undefinedResultCode && action.undefinedResultCode instanceof Function) {
237+
throw new (action.undefinedResultCode as any)(options);
238+
}
239+
else if (result === null) {
240+
if (action.nullResultCode) {
241+
if (action.nullResultCode instanceof Function) {
242+
throw new (action.nullResultCode as any)(options);
243+
}
244+
options.response.status(action.nullResultCode);
245+
} else {
246+
options.response.status(204);
247+
}
248+
}
249+
else if (action.successHttpCode) {
248250
options.response.status(action.successHttpCode);
249251
}
250252

@@ -263,8 +265,8 @@ export class ExpressDriver extends BaseDriver implements Driver {
263265
}
264266

265267
options.next();
266-
267-
} else if (action.renderedTemplate) { // if template is set then render it
268+
}
269+
else if (action.renderedTemplate) { // if template is set then render it
268270
const renderOptions = result && result instanceof Object ? result : {};
269271

270272
this.express.render(action.renderedTemplate, renderOptions, (err: any, html: string) => {
@@ -279,29 +281,28 @@ export class ExpressDriver extends BaseDriver implements Driver {
279281
}
280282
options.next();
281283
});
282-
283-
} else if (result !== undefined || action.undefinedResultCode) { // send regular result
284-
if (result === null || (result === undefined && action.undefinedResultCode)) {
285-
if (result === null && !action.nullResultCode) {
286-
options.response.status(204);
287-
}
288-
289-
if (action.isJsonTyped) {
290-
options.response.json();
291-
} else {
292-
options.response.send();
293-
}
294-
options.next();
284+
}
285+
else if (result === undefined) { // throw NotFoundError on undefined response
286+
const notFoundError = new NotFoundError();
287+
if (action.undefinedResultCode) {
288+
notFoundError.httpCode = action.undefinedResultCode as number;
289+
}
290+
throw notFoundError;
291+
}
292+
else if (result === null) { // send null response
293+
if (action.isJsonTyped) {
294+
options.response.json(null);
295295
} else {
296-
if (action.isJsonTyped) {
297-
options.response.json(result);
298-
} else {
299-
options.response.send(result);
300-
}
301-
options.next();
296+
options.response.send(null);
297+
}
298+
options.next();
299+
}
300+
else { // send regular result
301+
if (action.isJsonTyped) {
302+
options.response.json(result);
303+
} else {
304+
options.response.send(result);
302305
}
303-
304-
} else {
305306
options.next();
306307
}
307308
}

‎src/driver/koa/KoaDriver.ts‎

Lines changed: 62 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {isPromiseLike} from "../../util/isPromiseLike";
1313
import {getFromContainer} from "../../container";
1414
import {RoleChecker} from "../../RoleChecker";
1515
import {AuthorizationRequiredError} from "../../error/AuthorizationRequiredError";
16+
import { NotFoundError, HttpError } from "../../index";
1617
const cookie = require("cookie");
1718
const templateUrl = require("template-url");
1819

@@ -215,18 +216,13 @@ export class KoaDriver extends BaseDriver implements Driver {
215216
}
216217

217218
// set http status code
218-
if (action.undefinedResultCode && result === undefined) {
219-
if (action.undefinedResultCode instanceof Function)
220-
throw new (action.undefinedResultCode as any)(options);
221-
222-
options.response.status = action.undefinedResultCode;
223-
224-
} else if (action.nullResultCode && result === null) {
225-
if (action.nullResultCode instanceof Function)
219+
if (result === undefined && action.undefinedResultCode && action.undefinedResultCode instanceof Function) {
220+
throw new (action.undefinedResultCode as any)(options);
221+
}
222+
else if (result === null && action.nullResultCode) {
223+
if (action.nullResultCode instanceof Function) {
226224
throw new (action.nullResultCode as any)(options);
227-
228-
options.response.status = action.nullResultCode;
229-
225+
}
230226
} else if (action.successHttpCode) {
231227
options.response.status = action.successHttpCode;
232228
}
@@ -247,81 +243,81 @@ export class KoaDriver extends BaseDriver implements Driver {
247243

248244
return options.next();
249245

250-
} else if (action.renderedTemplate) { // if template is set then render it // todo: not working in koa
246+
} else if (action.renderedTemplate) { // if template is set then render it // TODO: not working in koa
251247
const renderOptions = result && result instanceof Object ? result : {};
252248

253249
this.koa.use(async function (ctx: any, next: any) {
254250
await ctx.render(action.renderedTemplate, renderOptions);
255251
});
256252

257253
return options.next();
258-
259-
} else if (result !== undefined || action.undefinedResultCode) { // send regular result
260-
if (result === null || (result === undefined && action.undefinedResultCode)) {
261-
262-
if (action.isJsonTyped) {
263-
options.response.body = null;
264-
} else {
265-
options.response.body = null;
266-
}
267-
268-
// todo: duplication. we make it here because after we set null to body koa seems overrides status
269-
if (action.nullResultCode) {
270-
options.response.status = action.nullResultCode;
271-
272-
} else if (result === undefined && action.undefinedResultCode) {
273-
options.response.status = action.undefinedResultCode;
274-
}
275-
276-
return options.next();
254+
}
255+
else if (result === undefined) { // throw NotFoundError on undefined response
256+
const notFoundError = new NotFoundError();
257+
if (action.undefinedResultCode) {
258+
notFoundError.httpCode = action.undefinedResultCode as number;
259+
}
260+
throw notFoundError;
261+
}
262+
else if (result === null) { // send null response
263+
if (action.isJsonTyped) {
264+
options.response.body = null;
277265
} else {
278-
if (result instanceof Object) {
279-
options.response.body = result;
280-
} else {
281-
options.response.body = result;
282-
}
283-
return options.next();
266+
options.response.body = null;
267+
}
268+
269+
// Setting `null` as a `response.body` means to koa that there is no content to return
270+
// so we must reset the status codes here.
271+
if (action.nullResultCode) {
272+
options.response.status = action.nullResultCode;
273+
} else {
274+
options.response.status = 204;
275+
}
276+
277+
return options.next();
278+
}
279+
else { // send regular result
280+
if (result instanceof Object) {
281+
options.response.body = result;
282+
} else {
283+
options.response.body = result;
284284
}
285285

286-
} else {
287286
return options.next();
288287
}
289288
}
290289

291290
/**
292291
* Handles result of failed executed controller action.
293292
*/
294-
handleError(error: any, action: ActionMetadata | undefined, options: Action): any {
295-
if (this.isDefaultErrorHandlingEnabled) {
296-
const response: any = options.response;
297-
298-
// set http status
299-
// note that we can't use error instanceof HttpError properly anymore because of new typescript emit process
300-
if (error.httpCode) {
301-
options.context.status = error.httpCode;
302-
response.status = error.httpCode;
303-
} else {
304-
options.context.status = 500;
305-
response.status = 500;
306-
}
293+
handleError(error: any, action: ActionMetadata | undefined, options: Action) {
294+
return new Promise((resolve, reject) => {
295+
if (this.isDefaultErrorHandlingEnabled) {
296+
// set http status
297+
if (error instanceof HttpError && error.httpCode) {
298+
options.response.status = error.httpCode;
299+
} else {
300+
options.response.status = 500;
301+
}
307302

308-
// apply http headers
309-
if (action) {
310-
Object.keys(action.headers).forEach(name => {
311-
response.set(name, action.headers[name]);
312-
});
313-
}
303+
// apply http headers
304+
if (action) {
305+
Object.keys(action.headers).forEach(name => {
306+
options.response.set(name, action.headers[name]);
307+
});
308+
}
314309

315-
// send error content
316-
if (action && action.isJsonTyped) {
317-
response.body = this.processJsonError(error);
318-
} else {
319-
response.body = this.processJsonError(error);
320-
}
310+
// send error content
311+
if (action && action.isJsonTyped) {
312+
options.response.body = this.processJsonError(error);
313+
} else {
314+
options.response.body = this.processTextError(error);
315+
}
321316

322-
return Promise.resolve();
323-
}
324-
return Promise.reject(error);
317+
return resolve();
318+
}
319+
return reject(error);
320+
});
325321
}
326322

327323
// -------------------------------------------------------------------------

‎test/functional/controller-methods.spec.ts‎

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import {Head} from "../../src/decorator/Head";
77
import {Delete} from "../../src/decorator/Delete";
88
import {Patch} from "../../src/decorator/Patch";
99
import {Put} from "../../src/decorator/Put";
10-
import {createExpressServer, createKoaServer, getMetadataArgsStorage} from "../../src/index";
10+
import { createExpressServer, createKoaServer, getMetadataArgsStorage, JsonController } from "../../src/index";
1111
import {assertRequest} from "./test-utils";
1212
const chakram = require("chakram");
1313
const expect = chakram.expect;
@@ -83,6 +83,30 @@ describe("controller methods", () => {
8383
}
8484
}
8585

86+
@JsonController("/return/json")
87+
class ReturnJsonController {
88+
@Get("/undefined")
89+
returnUndefined(): undefined {
90+
return undefined;
91+
}
92+
@Get("/null")
93+
returnNull(): null {
94+
return null;
95+
}
96+
}
97+
98+
@Controller("/return/normal")
99+
class ReturnNormalController {
100+
@Get("/undefined")
101+
returnUndefined(): undefined {
102+
return undefined;
103+
}
104+
@Get("/null")
105+
returnNull(): null {
106+
return null;
107+
}
108+
}
109+
86110
});
87111

88112
let expressApp: any, koaApp: any;
@@ -207,4 +231,36 @@ describe("controller methods", () => {
207231
});
208232
});
209233

234+
describe("should respond with 204 No Content when null returned in action", () => {
235+
assertRequest([3001, 3002], "get", "return/normal/null", response => {
236+
expect(response).to.have.status(204);
237+
expect(response).to.not.have.header("content-type");
238+
expect(response.body).to.not.exist;
239+
});
240+
assertRequest([3001, 3002], "get", "return/json/null", response => {
241+
expect(response).to.have.status(204);
242+
expect(response).to.not.have.header("content-type");
243+
expect(response.body).to.not.exist;
244+
});
245+
});
246+
247+
describe("should respond with 404 Not Found text when undefined returned in action", () => {
248+
assertRequest([3001, 3002], "get", "return/normal/undefined", response => {
249+
expect(response).to.have.status(404);
250+
expect(response).to.have.header("content-type", (contentType: string) => {
251+
expect(contentType).to.match(/text/);
252+
});
253+
});
254+
});
255+
256+
describe("should respond with 404 Not Found JSON when undefined returned in action", () => {
257+
assertRequest([3001, 3002], "get", "return/json/undefined", response => {
258+
expect(response).to.have.status(404);
259+
expect(response).to.have.header("content-type", (contentType: string) => {
260+
expect(contentType).to.match(/application\/json/);
261+
});
262+
});
263+
});
264+
265+
210266
});

0 commit comments

Comments
 (0)