From 6916fcf962ff1b1e582d9be3315a88fb463706de Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Fri, 9 Feb 2018 21:34:13 -0800 Subject: [PATCH] feat(rest): allow factory for controller routes --- packages/rest/src/http-handler.ts | 9 +- packages/rest/src/index.ts | 4 + packages/rest/src/rest-server.ts | 32 ++- packages/rest/src/router/routing-table.ts | 201 ++++++++++++++---- .../acceptance/routing/routing.acceptance.ts | 55 ++++- .../unit/router/controller-factory.test.ts | 41 ++++ .../test/unit/router/controller-route.test.ts | 54 ++++- 7 files changed, 350 insertions(+), 46 deletions(-) create mode 100644 packages/rest/test/unit/router/controller-factory.test.ts diff --git a/packages/rest/src/http-handler.ts b/packages/rest/src/http-handler.ts index d934a0f689a2..4218228922b5 100644 --- a/packages/rest/src/http-handler.ts +++ b/packages/rest/src/http-handler.ts @@ -15,6 +15,7 @@ import { ResolvedRoute, RouteEntry, ControllerClass, + ControllerFactory, } from './router/routing-table'; import {ParsedRequest} from './internal-types'; @@ -33,8 +34,12 @@ export class HttpHandler { this.handleRequest = (req, res) => this._handleRequest(req, res); } - registerController(name: ControllerClass, spec: ControllerSpec) { - this._routes.registerController(name, spec); + registerController( + controllerCtor: ControllerClass, + spec: ControllerSpec, + factory?: ControllerFactory, + ) { + this._routes.registerController(controllerCtor, spec); } registerRoute(route: RouteEntry) { diff --git a/packages/rest/src/index.ts b/packages/rest/src/index.ts index d730f186c957..da7b0e92e91e 100644 --- a/packages/rest/src/index.ts +++ b/packages/rest/src/index.ts @@ -14,6 +14,10 @@ export { ResolvedRoute, createResolvedRoute, parseRequestUrl, + ControllerClass, + ControllerInstance, + ControllerFactory, + createControllerFactory, } from './router/routing-table'; export * from './providers'; diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 03aa0c1c14a9..5261214d3aef 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -6,7 +6,13 @@ import {AssertionError} from 'assert'; import {safeDump} from 'js-yaml'; import {Binding, Context, Constructor, inject} from '@loopback/context'; -import {Route, ControllerRoute, RouteEntry} from './router/routing-table'; +import { + Route, + ControllerRoute, + RouteEntry, + createControllerFactory, + ControllerFactory, +} from './router/routing-table'; import {ParsedRequest} from './internal-types'; import {OpenApiSpec, OperationObject} from '@loopback/openapi-v3-types'; import {ServerRequest, ServerResponse, createServer} from 'http'; @@ -228,7 +234,8 @@ export class RestServer extends Context implements Server { if (apiSpec.components && apiSpec.components.schemas) { this._httpHandler.registerApiDefinitions(apiSpec.components.schemas); } - this._httpHandler.registerController(ctor, apiSpec); + const controllerFactory = createControllerFactory(b.key); + this._httpHandler.registerController(ctor, apiSpec, controllerFactory); } for (const b of this.find('routes.*')) { @@ -277,7 +284,15 @@ export class RestServer extends Context implements Server { ); } - const route = new ControllerRoute(verb, path, spec, ctor); + const controllerFactory = createControllerFactory(b.key); + const route = new ControllerRoute( + verb, + path, + spec, + ctor, + undefined, + controllerFactory, + ); this._httpHandler.registerRoute(route); return; } @@ -366,6 +381,7 @@ export class RestServer extends Context implements Server { spec: OperationObject, controller: ControllerClass, methodName: string, + factory?: ControllerFactory, ): Binding; /** @@ -389,6 +405,7 @@ export class RestServer extends Context implements Server { spec?: OperationObject, controller?: ControllerClass, methodName?: string, + controllerFactory?: ControllerFactory, ): Binding { if (typeof routeOrVerb === 'object') { const r = routeOrVerb; @@ -424,7 +441,14 @@ export class RestServer extends Context implements Server { } return this.route( - new ControllerRoute(routeOrVerb, path, spec, controller, methodName), + new ControllerRoute( + routeOrVerb, + path, + spec, + controller, + methodName, + controllerFactory, + ), ); } diff --git a/packages/rest/src/router/routing-table.ts b/packages/rest/src/router/routing-table.ts index a23eb96fcda1..41e8cc5d40df 100644 --- a/packages/rest/src/router/routing-table.ts +++ b/packages/rest/src/router/routing-table.ts @@ -9,10 +9,12 @@ import { PathsObject, } from '@loopback/openapi-v3-types'; import { + BindingScope, Context, Constructor, - instantiateClass, invokeMethod, + instantiateClass, + ValueOrPromise, } from '@loopback/context'; import {ServerRequest} from 'http'; import * as HttpErrors from 'http-errors'; @@ -60,13 +62,42 @@ export function parseRequestUrl(request: ServerRequest): ParsedRequest { return parsedRequest; } +/** + * A controller instance with open properties/methods + */ // tslint:disable-next-line:no-any -export type ControllerClass = Constructor; +export type ControllerInstance = {[name: string]: any}; +/** + * A factory function to create controller instances synchronously or + * asynchronously + */ +export type ControllerFactory = ( + ctx: Context, +) => ValueOrPromise; + +/** + * Controller class + */ +export type ControllerClass = Constructor; + +/** + * Routing table + */ export class RoutingTable { private readonly _routes: RouteEntry[] = []; - registerController(controller: ControllerClass, spec: ControllerSpec) { + /** + * Register a controller as the route + * @param controller + * @param spec + * @param controllerFactory + */ + registerController( + controller: ControllerClass, + spec: ControllerSpec, + controllerFactory?: ControllerFactory, + ) { assert( typeof spec === 'object' && !!spec, 'API specification must be a non-null object', @@ -82,7 +113,14 @@ export class RoutingTable { for (const verb in spec.paths[p]) { const opSpec: OperationObject = spec.paths[p][verb]; const fullPath = RoutingTable.joinPath(basePath, p); - const route = new ControllerRoute(verb, fullPath, opSpec, controller); + const route = new ControllerRoute( + verb, + fullPath, + opSpec, + controller, + undefined, + controllerFactory, + ); this.registerRoute(route); } } @@ -97,6 +135,10 @@ export class RoutingTable { return fullPath; } + /** + * Register a route + * @param route A route entry + */ registerRoute(route: RouteEntry) { // TODO(bajtos) handle the case where opSpec.parameters contains $ref // See https://github.com/strongloop/loopback-next/issues/435 @@ -126,6 +168,10 @@ export class RoutingTable { return paths; } + /** + * Map a request to a route + * @param request + */ find(request: ParsedRequest): ResolvedRoute { for (const entry of this._routes) { const match = entry.match(request); @@ -138,14 +184,40 @@ export class RoutingTable { } } +/** + * An entry in the routing table + */ export interface RouteEntry { + /** + * http verb + */ readonly verb: string; + /** + * http path + */ readonly path: string; + /** + * OpenAPI operation spec + */ readonly spec: OperationObject; + /** + * Map an http request to a route + * @param request + */ match(request: ParsedRequest): ResolvedRoute | undefined; + /** + * Update bindings for the request context + * @param requestContext + */ updateBindings(requestContext: Context): void; + + /** + * A handler to invoke the resolved controller method + * @param requestContext + * @param args + */ invokeHandler( requestContext: Context, args: OperationArgs, @@ -154,15 +226,27 @@ export interface RouteEntry { describe(): string; } +/** + * A route with path parameters resolved + */ export interface ResolvedRoute extends RouteEntry { readonly pathParams: PathParameterValues; } +/** + * Base implementation of RouteEntry + */ export abstract class BaseRoute implements RouteEntry { public readonly verb: string; private readonly _keys: pathToRegexp.Key[] = []; private readonly _pathRegexp: RegExp; + /** + * Construct a new route + * @param verb http verb + * @param path http request path pattern + * @param spec OpenAPI operation spec + */ constructor( verb: string, public readonly path: string, @@ -258,55 +342,78 @@ export class Route extends BaseRoute { } } -type ControllerInstance = {[opName: string]: Function}; - +/** + * A route backed by a controller + */ export class ControllerRoute extends BaseRoute { + protected readonly _controllerCtor: ControllerClass; + protected readonly _controllerName: string; protected readonly _methodName: string; - + protected readonly _controllerFactory: ControllerFactory; + + /** + * Construct a controller based route + * @param verb http verb + * @param path http request path + * @param spec OpenAPI operation spec + * @param controllerCtor Controller class + * @param methodName Controller method name, default to `x-operation-name` + * @param controllerFactory A factory function to create a controller instance + */ constructor( verb: string, path: string, spec: OperationObject, - protected readonly _controllerCtor: ControllerClass, + controllerCtor: ControllerClass, methodName?: string, + controllerFactory?: ControllerFactory, ) { + const controllerName = spec['x-controller-name'] || controllerCtor.name; + methodName = methodName || spec['x-operation-name']; + + if (!methodName) { + throw new Error( + 'methodName must be provided either via the ControllerRoute argument ' + + 'or via "x-operation-name" extension field in OpenAPI spec. ' + + `Operation: "${verb} ${path}" ` + + `Controller: ${controllerName}.`, + ); + } + super( verb, path, // Add x-controller-name and x-operation-name if not present Object.assign( { - 'x-controller-name': _controllerCtor.name, + 'x-controller-name': controllerName, 'x-operation-name': methodName, - tags: [_controllerCtor.name], + tags: [controllerName], }, spec, ), ); - if (!methodName) { - methodName = this.spec['x-operation-name']; - } + this._controllerCtor = controllerCtor; + this._controllerName = controllerName || controllerCtor.name; + this._methodName = methodName; - if (!methodName) { - throw new Error( - 'methodName must be provided either via the ControllerRoute argument ' + - 'or via "x-operation-name" extension field in OpenAPI spec. ' + - `Operation: "${verb} ${path}" ` + - `Controller: ${this._controllerCtor.name}.`, - ); + if (controllerFactory == null) { + controllerFactory = createControllerFactory(controllerCtor); } - - this._methodName = methodName; + this._controllerFactory = controllerFactory; } describe(): string { - return `${this._controllerCtor.name}.${this._methodName}`; + return `${this._controllerName}.${this._methodName}`; } updateBindings(requestContext: Context) { - const ctor = this._controllerCtor; - requestContext.bind('controller.current.ctor').to(ctor); + requestContext + .bind('controller.current') + .toDynamicValue(() => this._controllerFactory(requestContext)) + .inScope(BindingScope.SINGLETON); + requestContext.bind('controller.current.ctor').to(this._controllerCtor); requestContext.bind('controller.current.operation').to(this._methodName); } @@ -314,7 +421,9 @@ export class ControllerRoute extends BaseRoute { requestContext: Context, args: OperationArgs, ): Promise { - const controller = await this._createControllerInstance(requestContext); + const controller = await requestContext.get( + 'controller.current', + ); if (typeof controller[this._methodName] !== 'function') { throw new HttpErrors.NotFound( `Controller method not found: ${this.describe()}`, @@ -328,16 +437,6 @@ export class ControllerRoute extends BaseRoute { args, ); } - - private async _createControllerInstance( - requestContext: Context, - ): Promise { - const valueOrPromise = instantiateClass( - this._controllerCtor, - requestContext, - ); - return (await Promise.resolve(valueOrPromise)) as ControllerInstance; - } } function describeOperationParameters(opSpec: OperationObject) { @@ -345,3 +444,33 @@ function describeOperationParameters(opSpec: OperationObject) { .map(p => (p && p.name) || '') .join(', '); } + +/** + * Create a controller factory function + * @param source The source can be one of the following: + * - A binding key + * - A controller class + * - A controller instance + */ +export function createControllerFactory( + source: ControllerClass | string | ControllerInstance, +): ControllerFactory { + if (typeof source === 'string') { + return ctx => ctx.get(source); + } else if (typeof source === 'function') { + const ctor = source as ControllerClass; + return async ctx => { + // By default, we get an instance of the controller from the context + // using `controllers.` as the key + let inst = await ctx.get(`controllers.${ctor.name}`, { + optional: true, + }); + if (inst === undefined) { + inst = await instantiateClass(ctor, ctx); + } + return inst; + }; + } else { + return ctx => source; + } +} diff --git a/packages/rest/test/acceptance/routing/routing.acceptance.ts b/packages/rest/test/acceptance/routing/routing.acceptance.ts index 5297b13847d9..7c1b6e057d57 100644 --- a/packages/rest/test/acceptance/routing/routing.acceptance.ts +++ b/packages/rest/test/acceptance/routing/routing.acceptance.ts @@ -12,6 +12,8 @@ import { RestComponent, RestApplication, SequenceActions, + ControllerClass, + createControllerFactory, } from '../../..'; import {api, get, post, param, requestBody} from '@loopback/openapi-v3'; @@ -26,8 +28,8 @@ import { import {expect, Client, createClientForHandler} from '@loopback/testlab'; import {anOpenApiSpec, anOperationSpec} from '@loopback/openapi-spec-builder'; -import {inject, Context} from '@loopback/context'; -import {ControllerClass} from '../../../src/router/routing-table'; +import {inject, Context, BindingScope} from '@loopback/context'; + import {createUnexpectedHttpErrorLogger} from '../../helpers'; /* # Feature: Routing @@ -356,6 +358,32 @@ describe('Routing', () => { }); }); + it('binds the current controller', async () => { + const app = givenAnApplication(); + const server = await givenAServer(app); + const spec = anOpenApiSpec() + .withOperationReturningString('get', '/name', 'checkController') + .build(); + + @api(spec) + class GetCurrentController { + async checkController( + @inject('controllers.GetCurrentController') inst: GetCurrentController, + ): Promise { + return { + result: this === inst, + }; + } + } + givenControllerInApp(app, GetCurrentController); + + return whenIMakeRequestTo(server) + .get('/name') + .expect({ + result: true, + }); + }); + it('supports function-based routes', async () => { const app = givenAnApplication(); const server = await givenAServer(app); @@ -529,6 +557,27 @@ describe('Routing', () => { await client.get('/greet?name=world').expect(200, 'hello world'); }); + it('supports controller routes with factory defined via server.route()', async () => { + const app = givenAnApplication(); + const server = await givenAServer(app); + + class MyController { + greet(name: string) { + return `hello ${name}`; + } + } + + const spec = anOperationSpec() + .withParameter({name: 'name', in: 'query', type: 'string'}) + .build(); + + const factory = createControllerFactory(MyController); + server.route('get', '/greet', spec, MyController, 'greet', factory); + + const client = whenIMakeRequestTo(server); + await client.get('/greet?name=world').expect(200, 'hello world'); + }); + describe('RestApplication', () => { it('supports function-based routes declared via app.route()', async () => { const app = new RestApplication(); @@ -628,7 +677,7 @@ describe('Routing', () => { } function givenControllerInApp(app: Application, controller: ControllerClass) { - app.controller(controller); + app.controller(controller).inScope(BindingScope.CONTEXT); } function whenIMakeRequestTo(server: RestServer): Client { diff --git a/packages/rest/test/unit/router/controller-factory.test.ts b/packages/rest/test/unit/router/controller-factory.test.ts new file mode 100644 index 000000000000..dbfed0064892 --- /dev/null +++ b/packages/rest/test/unit/router/controller-factory.test.ts @@ -0,0 +1,41 @@ +// Copyright IBM Corp. 2017,2018. All Rights Reserved. +// Node module: @loopback/rest +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +import {expect} from '@loopback/testlab'; +import {ControllerFactory, createControllerFactory} from '../../..'; +import {Context} from '@loopback/core'; + +describe('createControllerFactory', () => { + let ctx: Context; + + beforeEach(() => { + ctx = new Context(); + }); + + it('creates a factory with binding key', async () => { + ctx.bind('controllers.my-controller').toClass(MyController); + const factory = createControllerFactory('controllers.my-controller'); + const inst = await factory(ctx); + expect(inst).to.be.instanceof(MyController); + }); + + it('creates a factory with class', async () => { + const factory = createControllerFactory(MyController); + const inst = await factory(ctx); + expect(inst).to.be.instanceof(MyController); + }); + + it('creates a factory with an instance', async () => { + const factory = createControllerFactory(new MyController()); + const inst = await factory(ctx); + expect(inst).to.be.instanceof(MyController); + }); + + class MyController { + greet() { + return 'Hello'; + } + } +}); diff --git a/packages/rest/test/unit/router/controller-route.test.ts b/packages/rest/test/unit/router/controller-route.test.ts index ec4fec151182..8b1f3bab16c5 100644 --- a/packages/rest/test/unit/router/controller-route.test.ts +++ b/packages/rest/test/unit/router/controller-route.test.ts @@ -6,14 +6,66 @@ import {ControllerRoute} from '../../..'; import {expect} from '@loopback/testlab'; import {anOperationSpec} from '@loopback/openapi-spec-builder'; +import {ControllerFactory, createControllerFactory} from '../../..'; describe('ControllerRoute', () => { it('rejects routes with no methodName', () => { - class MyController {} const spec = anOperationSpec().build(); expect( () => new ControllerRoute('get', '/greet', spec, MyController), ).to.throw(/methodName must be provided.*"get \/greet".*MyController/); }); + + it('creates a factory', () => { + const spec = anOperationSpec().build(); + + const route = new MyRoute('get', '/greet', spec, MyController, 'greet'); + + expect(route._controllerFactory).to.be.a.Function(); + }); + + it('honors a factory', () => { + const spec = anOperationSpec().build(); + + const factory = createControllerFactory('controllers.my-controller'); + const route = new MyRoute( + 'get', + '/greet', + spec, + MyController, + 'greet', + factory, + ); + + expect(route._controllerFactory).to.be.exactly(factory); + }); + + it('infers controllerName from the class', () => { + const spec = anOperationSpec().build(); + + const route = new MyRoute('get', '/greet', spec, MyController, 'greet'); + + expect(route._controllerName).to.eql(MyController.name); + }); + + it('honors controllerName from the spec', () => { + const spec = anOperationSpec().build(); + spec['x-controller-name'] = 'my-controller'; + + const route = new MyRoute('get', '/greet', spec, MyController, 'greet'); + + expect(route._controllerName).to.eql('my-controller'); + }); + + class MyController { + greet() { + return 'Hello'; + } + } + + class MyRoute extends ControllerRoute { + _controllerFactory: ControllerFactory; + _controllerName: string; + } });