From 3e25c7801f69dca08791c29fe0f9f35dfbdc67ce Mon Sep 17 00:00:00 2001 From: Rashmi Hunt Date: Thu, 25 May 2017 07:11:16 -0700 Subject: [PATCH] Extract function for parsing operation arguments (#295) --- packages/core/package.json | 2 + packages/core/src/index.ts | 4 + packages/core/src/parser.ts | 92 +++++++++++++++ packages/core/src/promisify.ts | 37 ++++++ packages/core/src/router/SwaggerRouter.ts | 107 ++++-------------- .../router/SwaggerRouter.integration.ts | 22 +++- packages/core/test/unit/parser.test.ts | 62 ++++++++++ 7 files changed, 242 insertions(+), 84 deletions(-) create mode 100644 packages/core/src/parser.ts create mode 100644 packages/core/src/promisify.ts create mode 100644 packages/core/test/unit/parser.test.ts diff --git a/packages/core/package.json b/packages/core/package.json index b708d2d76d7d..dcaef7e0b116 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -30,7 +30,9 @@ "devDependencies": { "@loopback/openapi-spec-builder": "^4.0.0-alpha.2", "@loopback/testlab": "^4.0.0-alpha.3", + "@types/shot": "^3.4.0", "mocha": "^3.2.0", + "shot": "^3.4.0", "typescript": "^2.3.2" }, "files": [ diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2bc791770db0..4ab54272d1fe 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -16,3 +16,7 @@ export * from '@loopback/openapi-spec'; // external dependencies export {ServerRequest, ServerResponse} from 'http'; + +// internals used by unit-tests +export {parseOperationArgs} from './parser'; +export {ParsedRequest, parseRequestUrl} from './router/SwaggerRouter'; diff --git a/packages/core/src/parser.ts b/packages/core/src/parser.ts new file mode 100644 index 000000000000..54f183f8ee1b --- /dev/null +++ b/packages/core/src/parser.ts @@ -0,0 +1,92 @@ +// Copyright IBM Corp. 2017. All Rights Reserved. +// Node module: @loopback/core +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +import {ServerRequest as Request} from 'http'; +import { + PathParameterValues, + OperationArgs, + ParsedRequest, + HttpError, + createHttpError, +} from './router/SwaggerRouter'; +import {OperationObject, ParameterObject} from '@loopback/openapi-spec'; +import {promisify} from './promisify'; + +type jsonBodyFn = (req: Request, cb: (err?: Error, body?: {}) => void) => void; +const jsonBody: jsonBodyFn = require('body/json'); + +// tslint:disable:no-any +type MaybeBody = any | undefined; +// tslint:enable:no-any + +const parseJsonBody: (req: Request) => Promise = promisify(jsonBody); + +export async function parseOperationArgs(request: ParsedRequest, operationSpec: OperationObject, pathParams: PathParameterValues): Promise { + const args: OperationArgs = []; + const body = await loadRequestBodyIfNeeded(operationSpec, request); + return buildOperationArguments(operationSpec, request, pathParams, body); +} + +function loadRequestBodyIfNeeded(operationSpec: OperationObject, request: Request): Promise { + if (!hasArgumentsFromBody(operationSpec)) + return Promise.resolve(); + + const contentType = request.headers['content-type']; + if (contentType && !/json/.test(contentType)) { + const err = createHttpError(415, `Content-type ${contentType} is not supported.`); + return Promise.reject(err); + } + + return parseJsonBody(request).catch((err: HttpError) => { + err.statusCode = 400; + return Promise.reject(err); + }); +} + +function hasArgumentsFromBody(operationSpec: OperationObject): boolean { + if (!operationSpec.parameters || !operationSpec.parameters.length) + return false; + + for (const paramSpec of operationSpec.parameters) { + if ('$ref' in paramSpec) continue; + const source = (paramSpec as ParameterObject).in; + if (source === 'formData' || source === 'body') + return true; + } + return false; +} + +function buildOperationArguments(operationSpec: OperationObject, request: ParsedRequest, + pathParams: PathParameterValues, body?: MaybeBody): OperationArgs { + const args: OperationArgs = []; + + for (const paramSpec of operationSpec.parameters || []) { + if ('$ref' in paramSpec) { + // TODO(bajtos) implement $ref parameters + throw new Error('$ref parameters are not supported yet.'); + } + const spec = paramSpec as ParameterObject; + switch (spec.in) { + case 'query': + args.push(request.query[spec.name]); + break; + case 'path': + args.push(pathParams[spec.name]); + break; + case 'header': + args.push(request.headers[spec.name.toLowerCase()]); + break; + case 'formData': + args.push(body ? body[spec.name] : undefined); + break; + case 'body': + args.push(body); + break; + default: + throw createHttpError(501, 'Parameters with "in: ' + spec.in + '" are not supported yet.'); + } + } + return args; +} diff --git a/packages/core/src/promisify.ts b/packages/core/src/promisify.ts new file mode 100644 index 000000000000..e29592da3c2a --- /dev/null +++ b/packages/core/src/promisify.ts @@ -0,0 +1,37 @@ +// Copyright IBM Corp. 2017. All Rights Reserved. +// Node module: @loopback/core +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +// TODO(bajtos) Move this file to a standalone module, or find an existing +// npm module that we could use instead. Just make sure the existing +// module is using native utils.promisify() when available. + +// tslint:disable:no-any + +import * as util from 'util'; + +const nativePromisify = (util as any).promisify; + +export function promisify(func: (callback: (err: any, result: T) => void) => void): () => Promise; +export function promisify(func: (arg1: A1, callback: (err: any, result: T) => void) => void): (arg1: A1) => Promise; +export function promisify(func: (arg1: A1, arg2: A2, callback: (err: any, result: T) => void) => void): (arg1: A1, arg2: A2) => Promise; + +export function promisify(func: (...args: any[]) => void): (...args: any[]) => Promise { + if (nativePromisify) + return nativePromisify(func); + + // The simplest implementation of Promisify + return (...args) => { + return new Promise((resolve, reject) => { + try { + func(...args, (err?: any, result?: any) => { + if (err) reject(err); + else resolve(result); + }); + } catch (err) { + reject(err); + } + }); + }; +} diff --git a/packages/core/src/router/SwaggerRouter.ts b/packages/core/src/router/SwaggerRouter.ts index 1e12eab2028d..dc94cdcb4a73 100644 --- a/packages/core/src/router/SwaggerRouter.ts +++ b/packages/core/src/router/SwaggerRouter.ts @@ -6,24 +6,18 @@ import {ServerRequest as Request, ServerResponse as Response} from 'http'; import {OpenApiSpec, OperationObject, ParameterObject} from '@loopback/openapi-spec'; import {invoke} from '../invoke'; +import {parseOperationArgs} from '../parser'; import * as assert from 'assert'; import * as url from 'url'; import * as pathToRegexp from 'path-to-regexp'; -import * as Promise from 'bluebird'; const debug = require('debug')('loopback:SwaggerRouter'); -type jsonBodyFn = (req: Request, cb: (err?: Error, body?: {}) => void) => void; -const jsonBody: jsonBodyFn = require('body/json'); - // tslint:disable:no-any -type MaybeBody = any | undefined; -type OperationArgs = any[]; -type PathParameterValues = {[key: string]: any}; +export type OperationArgs = any[]; +export type PathParameterValues = {[key: string]: any}; type OperationRetval = any; // tslint:enable:no-any -const parseJsonBody: (req: Request) => Promise = Promise.promisify(jsonBody); - export type HandlerCallback = (err?: Error | string) => void; export type RequestHandler = (req: Request, res: Response, cb?: HandlerCallback) => void; @@ -73,7 +67,7 @@ export class SwaggerRouter { */ public controller(factory: ControllerFactory, spec: OpenApiSpec): void { assert(typeof factory === 'function', 'Controller factory must be a function.'); - assert(typeof spec === 'object' && !!spec, 'API speciification must be a non-null object'); + assert(typeof spec === 'object' && !!spec, 'API specification must be a non-null object'); if (!spec.paths || !Object.keys(spec.paths).length) { return; } @@ -92,12 +86,7 @@ export class SwaggerRouter { } private _handleRequest(request: Request, response: Response, next: HandlerCallback): void { - // TODO(bajtos) The following parsing can be skipped when the router - // is mounted on an express app - const parsedRequest = request as ParsedRequest; - const parsedUrl = url.parse(parsedRequest.url, true); - parsedRequest.path = parsedUrl.pathname || '/'; - parsedRequest.query = parsedUrl.query; + const parsedRequest = parseRequestUrl(request); debug('Handle request "%s %s"', request.method, parsedRequest.path); @@ -134,7 +123,7 @@ export class SwaggerRouter { } } -interface ParsedRequest extends Request { +export interface ParsedRequest extends Request { // see http://expressjs.com/en/4x/api.html#req.path path: string; // see http://expressjs.com/en/4x/api.html#req.query @@ -185,10 +174,10 @@ class Endpoint { } const operationName = this._spec['x-operation-name']; + // tslint:disable-next-line:no-floating-promises Promise.resolve(this._controllerFactory(request, response, operationName)) .then(controller => { - loadRequestBodyIfNeeded(this._spec, request) - .then(body => buildOperationArguments(this._spec, request, pathParams, body)) + return parseOperationArgs(request, this._spec, pathParams) .then( args => { invoke(controller, operationName, args, response, next); @@ -197,79 +186,31 @@ class Endpoint { debug('Cannot parse arguments of operation %s: %s', operationName, err.stack || err); next(err); }); + }, + err => { + debug('Cannot resolve controller instance for operation %s: %s', operationName, err.stack || err); + next(err); }); } } -function loadRequestBodyIfNeeded(operationSpec: OperationObject, request: Request): Promise { - if (!hasArgumentsFromBody(operationSpec)) - return Promise.resolve(); - - const contentType = request.headers['content-type']; - if (contentType && !/json/.test(contentType)) { - const err = createHttpError(415, `Content-type ${contentType} is not supported.`); - return Promise.reject(err); - } - - return parseJsonBody(request).catch((err: HttpError) => { - err.statusCode = 400; - return Promise.reject(err); - }); -} - -function hasArgumentsFromBody(operationSpec: OperationObject): boolean { - if (!operationSpec.parameters || !operationSpec.parameters.length) - return false; - - for (const paramSpec of operationSpec.parameters) { - if ('$ref' in paramSpec) continue; - const source = (paramSpec as ParameterObject).in; - if (source === 'formData' || source === 'body') - return true; - } - return false; -} - -function buildOperationArguments(operationSpec: OperationObject, request: ParsedRequest, - pathParams: PathParameterValues, body?: MaybeBody): OperationArgs { - const args: OperationArgs = []; - - for (const paramSpec of operationSpec.parameters || []) { - if ('$ref' in paramSpec) { - // TODO(bajtos) implement $ref parameters - throw new Error('$ref parameters are not supported yet.'); - } - const spec = paramSpec as ParameterObject; - switch (spec.in) { - case 'query': - args.push(request.query[spec.name]); - break; - case 'path': - args.push(pathParams[spec.name]); - break; - case 'header': - args.push(request.headers[spec.name.toLowerCase()]); - break; - case 'formData': - args.push(body ? body[spec.name] : undefined); - break; - case 'body': - args.push(body); - break; - default: - throw createHttpError(501, 'Parameters with "in: ' + spec.in + '" are not supported yet.'); - } - } - return args; -} - -interface HttpError extends Error { +export interface HttpError extends Error { statusCode?: number; status?: number; } -function createHttpError(statusCode: number, message: string) { +export function createHttpError(statusCode: number, message: string) { const err = new Error(message) as HttpError; err.statusCode = statusCode; return err; } + +export function parseRequestUrl(request: Request): ParsedRequest { + // TODO(bajtos) The following parsing can be skipped when the router + // is mounted on an express app + const parsedRequest = request as ParsedRequest; + const parsedUrl = url.parse(parsedRequest.url, true); + parsedRequest.path = parsedUrl.pathname || '/'; + parsedRequest.query = parsedUrl.query; + return parsedRequest; +} diff --git a/packages/core/test/integration/router/SwaggerRouter.integration.ts b/packages/core/test/integration/router/SwaggerRouter.integration.ts index f519e894c8a9..93e94dac2bbb 100644 --- a/packages/core/test/integration/router/SwaggerRouter.integration.ts +++ b/packages/core/test/integration/router/SwaggerRouter.integration.ts @@ -351,6 +351,26 @@ context('with an operation echoing a string parameter from query', () => { }); }); + context('error handling', () => { + it('handles errors throws by controller constructor', () => { + const spec = givenOpenApiSpec() + .withOperationReturningString('get', '/hello', 'greet') + .build(); + + class ThrowingController { + constructor() { + throw new Error('Thrown from constructor.'); + } + } + + givenControllerClass(ThrowingController, spec); + + logErrorsExcept(500); + return client.get('/hello') + .expect(500); + }); + }); + let router: SwaggerRouter; function givenRouter() { router = new SwaggerRouter(); @@ -358,7 +378,7 @@ context('with an operation echoing a string parameter from query', () => { // tslint:disable-next-line:no-any function givenControllerClass(ctor: new (...args: any[]) => Object, spec: OpenApiSpec) { - router.controller((req, res) => new ctor(), spec); + router.controller((req, res) => Promise.resolve().then(() => new ctor()), spec); } function logErrorsExcept(ignoreStatusCode: number) { diff --git a/packages/core/test/unit/parser.test.ts b/packages/core/test/unit/parser.test.ts new file mode 100644 index 000000000000..47241acee396 --- /dev/null +++ b/packages/core/test/unit/parser.test.ts @@ -0,0 +1,62 @@ +// Copyright IBM Corp. 2013,2017. All Rights Reserved. +// Node module: @loopback/core +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +import { + parseOperationArgs, + ServerRequest, + ParsedRequest, + parseRequestUrl, +} from '../..'; +import {expect} from '@loopback/testlab'; +import {OperationObject, ParameterObject} from '@loopback/openapi-spec'; + +import {RequestOptions as ShotRequestOptions} from 'shot'; +type ShotRequestCtor = new(options: ShotRequestOptions) => ServerRequest; +// tslint:disable-next-line:variable-name +const ShotRequest: ShotRequestCtor = require('shot/lib/request'); + +describe('operationArgsParser', () => { + it('parses path parameters', async () => { + const spec = givenOperationWithParameters([{ + name: 'id', + type: 'number', + in: 'path', + }]); + const req = givenRequest(); + + const args = await parseOperationArgs(req, spec, {id: 1}); + + expect(args).to.eql([1]); + }); + + it('parsed body parameter', async () => { + const spec = givenOperationWithParameters([{ + name: 'data', + type: 'object', + in: 'body', + }]); + + const req = givenRequest({ + url: '/', + payload: {key: 'value'}, + }); + + const args = await parseOperationArgs(req, spec, {}); + + expect(args).to.eql([{key: 'value'}]); + }); + + function givenOperationWithParameters(params?: ParameterObject[]) { + return { + 'x-operation-name': 'testOp', + parameters: params, + responses: {}, + }; + } + + function givenRequest(options?: ShotRequestOptions): ParsedRequest { + return parseRequestUrl(new ShotRequest(options || {url: '/'})); + } +});