fix(rest): Tidy up rest decorator metadata

raymondfeng · raymondfeng · commit 7d15bfeaa25d · 2017-11-09T10:14:12.000-08:00
Simplify method level metadata processing
Add support for basePath
diff --git a/packages/rest/src/router/metadata.ts b/packages/rest/src/router/metadata.ts
@@ -16,6 +16,9 @@ import {
 
 const debug = require('debug')('loopback:core:router:metadata');
 
+const ENDPOINTS_KEY = 'rest:endpoints';
+const API_SPEC_KEY = 'rest:api-spec';
+
 // tslint:disable:no-any
 
 export interface ControllerSpec {
@@ -36,6 +39,14 @@ export interface ControllerSpec {
  * Decorate the given Controller constructor with metadata describing
  * the HTTP/REST API the Controller implements/provides.
  *
+ * `@api` can be applied to controller classes. For example,
+ * ```
+ * @api({basePath: '/my'})
+ * class MyController {
+ *   // ...
+ * }
+ * ```
+ *
  * @param spec OpenAPI specification describing the endpoints
  * handled by this controller
  *
@@ -47,95 +58,89 @@ export function api(spec: ControllerSpec) {
       typeof constructor === 'function',
       'The @api decorator can be applied to constructors only.',
     );
-    Reflector.defineMetadata('loopback:api-spec', spec, constructor);
+    const apiSpec = resolveControllerSpec(constructor, spec);
+    Reflector.defineMetadata(API_SPEC_KEY, apiSpec, constructor);
   };
 }
 
+/**
+ * Data structure for REST related metadata
+ */
 interface RestEndpoint {
   verb: string;
   path: string;
+  spec?: OperationObject;
+  target: any;
 }
 
-export function getControllerSpec(constructor: Function): ControllerSpec {
+/**
+ * Build the api spec from class and method level decorations
+ * @param constructor Controller class
+ * @param spec API spec
+ */
+function resolveControllerSpec(
+  constructor: Function,
+  spec?: ControllerSpec,
+): ControllerSpec {
   debug(`Retrieving OpenAPI specification for controller ${constructor.name}`);
 
-  let spec: ControllerSpec = Reflector.getMetadata(
-    'loopback:api-spec',
-    constructor,
-  );
-
   if (spec) {
     debug('  using class-level spec defined via @api()', spec);
-    return spec;
+    spec = Object.assign({}, spec);
+  } else {
+    spec = {paths: {}};
   }
 
-  spec = {paths: {}};
-  for (
-    let proto = constructor.prototype;
-    proto && proto !== Object.prototype;
-    proto = Object.getPrototypeOf(proto)
-  ) {
-    addPrototypeMethodsToSpec(spec, proto);
-  }
-  return spec;
-}
-
-function addPrototypeMethodsToSpec(spec: ControllerSpec, proto: any) {
-  const controllerMethods = Object.getOwnPropertyNames(proto).filter(
-    key => key !== 'constructor' && typeof proto[key] === 'function',
-  );
-  for (const methodName of controllerMethods) {
-    addControllerMethodToSpec(spec, proto, methodName);
-  }
-}
+  const endpoints =
+    Reflector.getMetadata(ENDPOINTS_KEY, constructor.prototype) || {};
+
+  for (const op in endpoints) {
+    const endpoint = endpoints[op];
+    const className =
+      endpoint.target.constructor.name ||
+      constructor.name ||
+      '<AnonymousClass>';
+    const fullMethodName = `${className}.${op}`;
+
+    const {verb, path} = endpoint;
+    const endpointName = `${fullMethodName} (${verb} ${path})`;
+
+    let operationSpec = endpoint.spec;
+    if (!operationSpec) {
+      // The operation was defined via @operation(verb, path) with no spec
+      operationSpec = {
+        responses: {},
+      };
+    }
 
-function addControllerMethodToSpec(
-  spec: ControllerSpec,
-  proto: any,
-  methodName: string,
-) {
-  const className = proto.constructor.name || '<UnknownClass>';
-  const fullMethodName = `${className}.${methodName}`;
-
-  const endpoint: RestEndpoint = Reflector.getMetadata(
-    'loopback:operation-endpoint',
-    proto,
-    methodName,
-  );
-
-  if (!endpoint) {
-    debug(`  skipping ${fullMethodName} - no endpoint is defined`);
-    return;
-  }
+    if (!spec.paths[path]) {
+      spec.paths[path] = {};
+    }
 
-  const {verb, path} = endpoint;
-  const endpointName = `${fullMethodName} (${verb} ${path})`;
-
-  let operationSpec = Reflector.getMetadata(
-    'loopback:operation-spec',
-    proto,
-    methodName,
-  );
-  if (!operationSpec) {
-    // The operation was defined via @operation(verb, path) with no spec
-    operationSpec = {
-      responses: {},
-    };
-  }
+    if (spec.paths[path][verb]) {
+      // Operations from subclasses override those from the base
+      debug(`  Overriding ${endpointName} - endpoint was already defined`);
+    }
 
-  if (!spec.paths[path]) {
-    spec.paths[path] = {};
+    debug(`  adding ${endpointName}`, operationSpec);
+    spec.paths[path][verb] = Object.assign({}, operationSpec, {
+      'x-operation-name': op,
+    });
   }
+  return spec;
+}
 
-  if (spec.paths[path][verb]) {
-    debug(`  skipping ${endpointName} - endpoint was already defined`);
-    return;
+/**
+ * Get the controller spec for the given class
+ * @param constructor Controller class
+ */
+export function getControllerSpec(constructor: Function): ControllerSpec {
+  let spec = Reflector.getMetadata(API_SPEC_KEY, constructor);
+  if (!spec) {
+    spec = resolveControllerSpec(constructor, spec);
+    Reflector.defineMetadata(API_SPEC_KEY, spec, constructor);
   }
-
-  debug(`  adding ${endpointName}`, operationSpec);
-  spec.paths[path][verb] = Object.assign({}, operationSpec, {
-    'x-operation-name': methodName,
-  });
+  return spec;
 }
 
 /**
@@ -207,20 +212,35 @@ export function del(path: string, spec?: OperationObject) {
  *   of this operation.
  */
 export function operation(verb: string, path: string, spec?: OperationObject) {
-  // tslint:disable-next-line:no-any
   return function(
-    target: object,
+    target: any,
     propertyKey: string,
     descriptor: PropertyDescriptor,
   ) {
-    const endpoint: RestEndpoint = {verb, path};
-    Reflector.defineMetadata(
-      'loopback:operation-endpoint',
-      endpoint,
-      target,
-      propertyKey,
+    assert(
+      typeof target[propertyKey] === 'function',
+      '@operation decorator can be applied to methods only',
     );
 
+    let endpoints = Object.assign(
+      {},
+      Reflector.getMetadata(ENDPOINTS_KEY, target),
+    );
+    Reflector.defineMetadata(ENDPOINTS_KEY, endpoints, target);
+
+    let endpoint: Partial<RestEndpoint> = endpoints[propertyKey];
+    if (!endpoint) {
+      // Add the new endpoint metadata for the method
+      endpoint = {verb, path, spec, target};
+      endpoints[propertyKey] = endpoint;
+    } else {
+      // Update the endpoint metadata
+      // It can be created by @param
+      endpoint.verb = verb;
+      endpoint.path = path;
+      endpoint.target = target;
+    }
+
     if (!spec) {
       // Users can define parameters and responses using decorators
       return;
@@ -231,7 +251,7 @@ export function operation(verb: string, path: string, spec?: OperationObject) {
     // will invoke param() decorator first and operation() second.
     // As a result, we need to preserve any partial definitions
     // already provided by other decorators.
-    editOperationSpec(target, propertyKey, overrides => {
+    editOperationSpec(endpoint, overrides => {
       const mergedSpec = Object.assign({}, spec, overrides);
 
       // Merge "responses" definitions
@@ -285,7 +305,20 @@ export function param(paramSpec: ParameterObject) {
       '@param decorator can be applied to methods only',
     );
 
-    editOperationSpec(target, propertyKey, operationSpec => {
+    let endpoints = Object.assign(
+      {},
+      Reflector.getMetadata(ENDPOINTS_KEY, target),
+    );
+    Reflector.defineMetadata(ENDPOINTS_KEY, endpoints, target);
+
+    let endpoint: Partial<RestEndpoint> = endpoints[propertyKey];
+    if (!endpoint) {
+      // Add the new endpoint metadata for the method
+      endpoint = {target};
+      endpoints[propertyKey] = endpoint;
+    }
+
+    editOperationSpec(endpoint, operationSpec => {
       let decoratorStyle;
       if (typeof descriptorOrParameterIndex === 'number') {
         decoratorStyle = 'parameter';
@@ -318,30 +351,18 @@ export function param(paramSpec: ParameterObject) {
 }
 
 function editOperationSpec(
-  target: any,
-  propertyKey: string,
+  endpoint: Partial<RestEndpoint>,
   updateFn: (spec: OperationObject) => OperationObject,
 ) {
-  let spec: OperationObject = Reflector.getMetadata(
-    'loopback:operation-spec',
-    target,
-    propertyKey,
-  );
-
+  let spec = endpoint.spec;
   if (!spec) {
     spec = {
       responses: {},
     };
   }
 
   spec = updateFn(spec);
-
-  Reflector.defineMetadata(
-    'loopback:operation-spec',
-    spec,
-    target,
-    propertyKey,
-  );
+  endpoint.spec = spec;
 }
 
 export namespace param {
diff --git a/packages/rest/src/router/routing-table.ts b/packages/rest/src/router/routing-table.ts
@@ -66,15 +66,26 @@ export class RoutingTable {
 
     debug('Registering Controller with API', spec);
 
-    for (const path in spec.paths) {
-      for (const verb in spec.paths[path]) {
-        const opSpec: OperationObject = spec.paths[path][verb];
-        const route = new ControllerRoute(verb, path, opSpec, controller);
+    const basePath = spec.basePath || '/';
+    for (const p in spec.paths) {
+      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);
         this.registerRoute(route);
       }
     }
   }
 
+  static joinPath(basePath: string, path: string) {
+    const fullPath = [basePath, path]
+      .join('/') // Join by /
+      .replace(/(\/){2,}/g, '/') // Remove extra /
+      .replace(/\/$/, '') // Remove trailing /
+      .replace(/^(\/)?/, '/'); // Add leading /
+    return fullPath;
+  }
+
   registerRoute(route: RouteEntry) {
     // TODO(bajtos) handle the case where opSpec.parameters contains $ref
     // See https://github.com/strongloop/loopback-next/issues/435
diff --git a/packages/rest/test/acceptance/routing/routing.acceptance.ts b/packages/rest/test/acceptance/routing/routing.acceptance.ts
@@ -344,6 +344,24 @@ describe('Routing', () => {
     await client.get('/greet?name=world').expect(200, 'hello world');
   });
 
+  it('supports controller routes declared via API spec with basePath', async () => {
+    const app = givenAnApplication();
+    const server = await givenAServer(app);
+
+    @api({basePath: '/my', paths: {}})
+    class MyController {
+      @get('/greet')
+      greet(@param.query.string('name') name: string) {
+        return `hello ${name}`;
+      }
+    }
+
+    app.controller(MyController);
+
+    const client = whenIMakeRequestTo(server);
+    await client.get('/my/greet?name=world').expect(200, 'hello world');
+  });
+
   it('reports operations bound to unknown controller', async () => {
     const app = givenAnApplication();
     const server = await givenAServer(app);
diff --git a/packages/rest/test/unit/router/routing-table.test.ts b/packages/rest/test/unit/router/routing-table.test.ts
