refactor(rest): tidy up controller related methods

- move controllFactory param next to controllerCtor - split createControllerFactory to three functions
loopbackio · Apr 11, 2018 · 5446cea · 5446cea
1 parent a1cbab3
commit 5446cea
Show file tree

Hide file tree

Showing 12 changed files with 136 additions and 81 deletions.
diff --git a/packages/core/src/keys.ts b/packages/core/src/keys.ts
@@ -5,7 +5,6 @@
 
 import {BindingKey} from '@loopback/context';
 import {Application, ControllerClass} from './application';
-import {ControllerInstance} from '../../rest/src';
 
 /**
  * Namespace for core binding keys
@@ -59,7 +58,5 @@ export namespace CoreBindings {
    * Binding key for the controller instance resolved in the current request
    * context
    */
-  export const CONTROLLER_CURRENT = BindingKey.create<ControllerInstance>(
-    'controller.current',
-  );
+  export const CONTROLLER_CURRENT = BindingKey.create('controller.current');
 }
diff --git a/packages/rest/src/http-handler.ts b/packages/rest/src/http-handler.ts
@@ -35,11 +35,11 @@ export class HttpHandler {
   }
 
   registerController<T>(
-    controllerCtor: ControllerClass<T>,
     spec: ControllerSpec,
+    controllerCtor: ControllerClass<T>,
     controllerFactory?: ControllerFactory<T>,
   ) {
-    this._routes.registerController(controllerCtor, spec, controllerFactory);
+    this._routes.registerController(spec, controllerCtor, controllerFactory);
   }
 
   registerRoute(route: RouteEntry) {

diff --git a/packages/rest/src/index.ts b/packages/rest/src/index.ts
@@ -17,7 +17,9 @@ export {
   ControllerClass,
   ControllerInstance,
   ControllerFactory,
-  createControllerFactory,
+  createControllerFactoryForBinding,
+  createControllerFactoryForClass,
+  createControllerFactoryForInstance,
 } from './router/routing-table';
 
 export * from './providers';

diff --git a/packages/rest/src/rest.application.ts b/packages/rest/src/rest.application.ts
@@ -100,16 +100,16 @@ export class RestApplication extends Application implements HttpServerLike {
    * @param path URL path of the endpoint
    * @param spec The OpenAPI spec describing the endpoint (operation)
    * @param controllerCtor Controller constructor
-   * @param methodName The name of the controller method
    * @param controllerFactory A factory function to create controller instance
+   * @param methodName The name of the controller method
    */
   route<T>(
     verb: string,
     path: string,
     spec: OperationObject,
     controllerCtor: ControllerClass<T>,
+    controllerFactory: ControllerFactory<T>,
     methodName: string,
-    controllerFactory?: ControllerFactory<T>,
   ): Binding;
 
   /**
@@ -132,8 +132,8 @@ export class RestApplication extends Application implements HttpServerLike {
     path?: string,
     spec?: OperationObject,
     controllerCtor?: ControllerClass<T>,
-    methodName?: string,
     controllerFactory?: ControllerFactory<T>,
+    methodName?: string,
   ): Binding {
     const server = this.restServer;
     if (typeof routeOrVerb === 'object') {
@@ -144,8 +144,8 @@ export class RestApplication extends Application implements HttpServerLike {
         path!,
         spec!,
         controllerCtor!,
+        controllerFactory!,
         methodName!,
-        controllerFactory,
       );
     }
   }

diff --git a/packages/rest/src/rest.server.ts b/packages/rest/src/rest.server.ts
@@ -10,10 +10,10 @@ import {
   Route,
   ControllerRoute,
   RouteEntry,
-  createControllerFactory,
   ControllerFactory,
   ControllerClass,
   ControllerInstance,
+  createControllerFactoryForBinding,
 } from './router/routing-table';
 import {ParsedRequest} from './internal-types';
 import {OpenApiSpec, OperationObject} from '@loopback/openapi-v3-types';
@@ -246,8 +246,8 @@ export class RestServer extends Context implements Server, HttpServerLike {
       if (apiSpec.components && apiSpec.components.schemas) {
         this._httpHandler.registerApiDefinitions(apiSpec.components.schemas);
       }
-      const controllerFactory = createControllerFactory(b.key);
-      this._httpHandler.registerController(ctor, apiSpec, controllerFactory);
+      const controllerFactory = createControllerFactoryForBinding(b.key);
+      this._httpHandler.registerController(apiSpec, ctor, controllerFactory);
     }
 
     for (const b of this.find('routes.*')) {
@@ -296,13 +296,12 @@ export class RestServer extends Context implements Server, HttpServerLike {
         );
       }
 
-      const controllerFactory = createControllerFactory(b.key);
+      const controllerFactory = createControllerFactoryForBinding(b.key);
       const route = new ControllerRoute(
         verb,
         path,
         spec,
         ctor,
-        undefined,
         controllerFactory,
       );
       this._httpHandler.registerRoute(route);
@@ -385,16 +384,16 @@ export class RestServer extends Context implements Server, HttpServerLike {
    * @param path URL path of the endpoint
    * @param spec The OpenAPI spec describing the endpoint (operation)
    * @param controllerCtor Controller constructor
-   * @param methodName The name of the controller method
    * @param controllerFactory A factory function to create controller instance
+   * @param methodName The name of the controller method
    */
   route<I>(
     verb: string,
     path: string,
     spec: OperationObject,
     controllerCtor: ControllerClass<I>,
+    controllerFactory: ControllerFactory<I>,
     methodName: string,
-    controllerFactory?: ControllerFactory<I>,
   ): Binding;
 
   /**
@@ -417,8 +416,8 @@ export class RestServer extends Context implements Server, HttpServerLike {
     path?: string,
     spec?: OperationObject,
     controllerCtor?: ControllerClass<I>,
-    methodName?: string,
     controllerFactory?: ControllerFactory<I>,
+    methodName?: string,
   ): Binding {
     if (typeof routeOrVerb === 'object') {
       const r = routeOrVerb;
@@ -459,8 +458,8 @@ export class RestServer extends Context implements Server, HttpServerLike {
         path,
         spec,
         controllerCtor,
-        methodName,
         controllerFactory,
+        methodName,
       ),
     );
   }

diff --git a/packages/rest/src/router/routing-table.ts b/packages/rest/src/router/routing-table.ts
@@ -90,13 +90,13 @@ export class RoutingTable {
 
   /**
    * Register a controller as the route
-   * @param controllerCtor
    * @param spec
+   * @param controllerCtor
    * @param controllerFactory
    */
   registerController<T>(
-    controllerCtor: ControllerClass<T>,
     spec: ControllerSpec,
+    controllerCtor: ControllerClass<T>,
     controllerFactory?: ControllerFactory<T>,
   ) {
     assert(
@@ -119,7 +119,6 @@ export class RoutingTable {
           fullPath,
           opSpec,
           controllerCtor,
-          undefined,
           controllerFactory,
         );
         this.registerRoute(route);
@@ -358,16 +357,16 @@ export class ControllerRoute<T> extends BaseRoute {
    * @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
+   * @param methodName Controller method name, default to `x-operation-name`
    */
   constructor(
     verb: string,
     path: string,
     spec: OperationObject,
     controllerCtor: ControllerClass<T>,
-    methodName?: string,
     controllerFactory?: ControllerFactory<T>,
+    methodName?: string,
   ) {
     const controllerName = spec['x-controller-name'] || controllerCtor.name;
     methodName = methodName || spec['x-operation-name'];
@@ -395,14 +394,11 @@ export class ControllerRoute<T> extends BaseRoute {
       ),
     );
 
+    this._controllerFactory =
+      controllerFactory || createControllerFactoryForClass(controllerCtor);
     this._controllerCtor = controllerCtor;
     this._controllerName = controllerName || controllerCtor.name;
     this._methodName = methodName;
-
-    if (controllerFactory == null) {
-      controllerFactory = createControllerFactory(controllerCtor);
-    }
-    this._controllerFactory = controllerFactory;
   }
 
   describe(): string {
@@ -449,31 +445,41 @@ function describeOperationParameters(opSpec: OperationObject) {
 }
 
 /**
- * Create a controller factory function
- * @param source The source can be one of the following:
- * - A binding key
- * - A controller class
- * - A controller instance
+ * Create a controller factory function for a given binding key
+ * @param key Binding key
  */
-export function createControllerFactory<T>(
-  source: ControllerClass<T> | string | T,
+export function createControllerFactoryForBinding<T>(
+  key: string,
 ): ControllerFactory<T> {
-  if (typeof source === 'string') {
-    return ctx => ctx.get<T>(source);
-  } else if (typeof source === 'function') {
-    const ctor = source as ControllerClass<T>;
-    return async ctx => {
-      // By default, we get an instance of the controller from the context
-      // using `controllers.<controllerName>` as the key
-      let inst = await ctx.get<T>(`controllers.${ctor.name}`, {
-        optional: true,
-      });
-      if (inst === undefined) {
-        inst = await instantiateClass<T>(ctor, ctx);
-      }
-      return inst;
-    };
-  } else {
-    return ctx => source;
-  }
+  return ctx => ctx.get<T>(key);
+}
+
+/**
+ * Create a controller factory function for a given class
+ * @param controllerCtor Controller class
+ */
+export function createControllerFactoryForClass<T>(
+  controllerCtor: ControllerClass<T>,
+): ControllerFactory<T> {
+  return async ctx => {
+    // By default, we get an instance of the controller from the context
+    // using `controllers.<controllerName>` as the key
+    let inst = await ctx.get<T>(`controllers.${controllerCtor.name}`, {
+      optional: true,
+    });
+    if (inst === undefined) {
+      inst = await instantiateClass<T>(controllerCtor, ctx);
+    }
+    return inst;
+  };
+}
+
+/**
+ * Create a controller factory function for a given instance
+ * @param controllerCtor Controller instance
+ */
+export function createControllerFactoryForInstance<T>(
+  controllerInst: T,
+): ControllerFactory<T> {
+  return ctx => controllerInst;
 }
diff --git a/packages/rest/test/acceptance/routing/routing.acceptance.ts b/packages/rest/test/acceptance/routing/routing.acceptance.ts
@@ -14,9 +14,9 @@ import {
   SequenceActions,
   HttpServerLike,
   ControllerClass,
-  createControllerFactory,
-  ControllerFactory,
   ControllerInstance,
+  createControllerFactoryForClass,
+  createControllerFactoryForInstance,
 } from '../../..';
 
 import {api, get, post, param, requestBody} from '@loopback/openapi-v3';
@@ -554,7 +554,14 @@ describe('Routing', () => {
       .withParameter({name: 'name', in: 'query', type: 'string'})
       .build();
 
-    server.route('get', '/greet', spec, MyController, 'greet');
+    server.route(
+      'get',
+      '/greet',
+      spec,
+      MyController,
+      createControllerFactoryForClass(MyController),
+      'greet',
+    );
 
     const client = whenIMakeRequestTo(server);
     await client.get('/greet?name=world').expect(200, 'hello world');
@@ -574,8 +581,8 @@ describe('Routing', () => {
       .withParameter({name: 'name', in: 'query', type: 'string'})
       .build();
 
-    const factory = createControllerFactory(MyController);
-    server.route('get', '/greet', spec, MyController, 'greet', factory);
+    const factory = createControllerFactoryForClass(MyController);
+    server.route('get', '/greet', spec, MyController, factory, 'greet');
 
     const client = whenIMakeRequestTo(server);
     await client.get('/greet?name=world').expect(200, 'hello world');
@@ -650,7 +657,8 @@ describe('Routing', () => {
         .withParameter({name: 'name', in: 'query', type: 'string'})
         .build();
 
-      app.route('get', '/greet', spec, MyController, 'greet');
+      const factory = createControllerFactoryForClass(MyController);
+      app.route('get', '/greet', spec, MyController, factory, 'greet');
 
       await whenIMakeRequestTo(app)
         .get('/greet?name=world')
@@ -676,9 +684,8 @@ describe('Routing', () => {
         .withParameter({name: 'name', in: 'query', type: 'string'})
         .build();
 
-      const factory: ControllerFactory<MyController> = ctx =>
-        new MySubController();
-      app.route('get', '/greet', spec, MyController, 'greet', factory);
+      const factory = createControllerFactoryForInstance(new MySubController());
+      app.route('get', '/greet', spec, MyController, factory, 'greet');
 
       await whenIMakeRequestTo(app)
         .get('/greet?name=world')

diff --git a/packages/rest/test/integration/http-handler.integration.ts b/packages/rest/test/integration/http-handler.integration.ts
@@ -454,7 +454,7 @@ describe('HttpHandler', () => {
     ctor: new (...args: any[]) => Object,
     spec: ControllerSpec,
   ) {
-    handler.registerController(ctor, spec);
+    handler.registerController(spec, ctor);
   }
 
   function givenClient() {

diff --git a/packages/rest/test/unit/rest.server/rest.server.open-api-spec.unit.ts b/packages/rest/test/unit/rest.server/rest.server.open-api-spec.unit.ts
@@ -5,7 +5,12 @@
 
 import {expect, validateApiSpec} from '@loopback/testlab';
 import {Application} from '@loopback/core';
-import {RestServer, Route, RestComponent} from '../../..';
+import {
+  RestServer,
+  Route,
+  RestComponent,
+  createControllerFactoryForClass,
+} from '../../..';
 import {get, post, requestBody} from '@loopback/openapi-v3';
 import {anOpenApiSpec} from '@loopback/openapi-spec-builder';
 import {model, property} from '@loopback/repository';
@@ -63,6 +68,7 @@ describe('RestServer.getApiSpec()', () => {
       '/greet.json',
       {responses: {}},
       MyController,
+      createControllerFactoryForClass(MyController),
       'greet',
     );
     expect(binding.key).to.eql('routes.get %2Fgreet%2Ejson');
@@ -88,7 +94,14 @@ describe('RestServer.getApiSpec()', () => {
       greet() {}
     }
 
-    server.route('get', '/greet', {responses: {}}, MyController, 'greet');
+    server.route(
+      'get',
+      '/greet',
+      {responses: {}},
+      MyController,
+      createControllerFactoryForClass(MyController),
+      'greet',
+    );
 
     const spec = server.getApiSpec();
     expect(spec.paths).to.eql({