fix(rest): Improve rest metadata inheritance

packages/rest/package.json

@@ -26,8 +26,6 @@
     "@loopback/context": "^4.0.0-alpha.20",
     "@loopback/core": "^4.0.0-alpha.22",
     "@loopback/openapi-spec": "^4.0.0-alpha.15",
-    "@types/http-errors": "^1.5.34",
-    "@types/js-yaml": "^3.9.1",
     "body": "^5.1.0",
     "debug": "^3.1.0",
     "http-errors": "^1.6.1",
@@ -39,7 +37,10 @@
   "devDependencies": {
     "@loopback/build": "^4.0.0-alpha.5",
     "@loopback/openapi-spec-builder": "^4.0.0-alpha.12",
-    "@loopback/testlab": "^4.0.0-alpha.14"
+    "@loopback/testlab": "^4.0.0-alpha.14",
+    "@types/http-errors": "^1.5.34",
+    "@types/js-yaml": "^3.9.1",
+    "@types/lodash": "^4.14.85"
   },
   "files": [
     "README.md",

packages/rest/src/router/metadata.ts

@@ -4,6 +4,8 @@
 // License text available at https://opensource.org/licenses/MIT
 
 import * as assert from 'assert';
+import * as _ from 'lodash';
+
 import {Reflector} from '@loopback/context';
 import {
   OperationObject,
@@ -21,6 +23,17 @@ const API_SPEC_KEY = 'rest:api-spec';
 
 // tslint:disable:no-any
 
+function cloneDeep<T>(val: T): T {
+  if (val === undefined) {
+    return {} as T;
+  }
+  return _.cloneDeepWith(val, v => {
+    // Do not clone functions
+    if (typeof v === 'function') return v;
+    return undefined;
+  });
+}
+
 export interface ControllerSpec {
   /**
    * The base path on which the Controller API is served.
@@ -73,6 +86,20 @@ interface RestEndpoint {
   target: any;
 }
 
+function getEndpoints(
+  target: any,
+): {[property: string]: Partial<RestEndpoint>} {
+  let endpoints = Reflector.getOwnMetadata(ENDPOINTS_KEY, target);
+  if (!endpoints) {
+    // Clone the endpoints so that subclasses won't mutate the metadata
+    // in the base class
+    const baseEndpoints = Reflector.getMetadata(ENDPOINTS_KEY, target);
+    endpoints = cloneDeep(baseEndpoints);
+    Reflector.defineMetadata(ENDPOINTS_KEY, endpoints, target);
+  }
+  return endpoints;
+}
+
 /**
  * Build the api spec from class and method level decorations
  * @param constructor Controller class
@@ -86,33 +113,39 @@ function resolveControllerSpec(
 
   if (spec) {
     debug('  using class-level spec defined via @api()', spec);
-    spec = Object.assign({}, spec);
+    spec = cloneDeep(spec);
   } else {
     spec = {paths: {}};
   }
 
-  const endpoints =
-    Reflector.getMetadata(ENDPOINTS_KEY, constructor.prototype) || {};
+  const endpoints = getEndpoints(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})`;
+    const verb = endpoint.verb!;
+    const path = endpoint.path!;
+
+    let endpointName = '';
+    if (debug.enabled) {
+      const className =
+        endpoint.target.constructor.name ||
+        constructor.name ||
+        '<AnonymousClass>';
+      const fullMethodName = `${className}.${op}`;
+      endpointName = `${fullMethodName} (${verb} ${path})`;
+    }
 
     let operationSpec = endpoint.spec;
     if (!operationSpec) {
       // The operation was defined via @operation(verb, path) with no spec
       operationSpec = {
         responses: {},
       };
+      endpoint.spec = operationSpec;
     }
 
+    operationSpec['x-operation-name'] = op;
+
     if (!spec.paths[path]) {
       spec.paths[path] = {};
     }
@@ -123,9 +156,7 @@ function resolveControllerSpec(
     }
 
     debug(`  adding ${endpointName}`, operationSpec);
-    spec.paths[path][verb] = Object.assign({}, operationSpec, {
-      'x-operation-name': op,
-    });
+    spec.paths[path][verb] = operationSpec;
   }
   return spec;
 }
@@ -135,7 +166,7 @@ function resolveControllerSpec(
  * @param constructor Controller class
  */
 export function getControllerSpec(constructor: Function): ControllerSpec {
-  let spec = Reflector.getMetadata(API_SPEC_KEY, constructor);
+  let spec = Reflector.getOwnMetadata(API_SPEC_KEY, constructor);
   if (!spec) {
     spec = resolveControllerSpec(constructor, spec);
     Reflector.defineMetadata(API_SPEC_KEY, spec, constructor);
@@ -222,14 +253,9 @@ export function operation(verb: string, path: string, spec?: OperationObject) {
       '@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) {
+    const endpoints = getEndpoints(target);
+    let endpoint = endpoints[propertyKey];
+    if (!endpoint || endpoint.target !== target) {
       // Add the new endpoint metadata for the method
       endpoint = {verb, path, spec, target};
       endpoints[propertyKey] = endpoint;
@@ -305,16 +331,13 @@ export function param(paramSpec: ParameterObject) {
       '@param 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) {
+    const endpoints = getEndpoints(target);
+    let endpoint = endpoints[propertyKey];
+    if (!endpoint || endpoint.target !== target) {
+      const baseEndpoint = endpoint;
       // Add the new endpoint metadata for the method
-      endpoint = {target};
+      endpoint = cloneDeep(baseEndpoint);
+      endpoint.target = target;
       endpoints[propertyKey] = endpoint;
     }
 

packages/rest/test/unit/router/metadata.test.ts

@@ -12,6 +12,7 @@ import {
   put,
   patch,
   del,
+  param,
 } from '../../..';
 import {expect} from '@loopback/testlab';
 import {anOpenApiSpec, anOperationSpec} from '@loopback/openapi-spec-builder';
@@ -272,4 +273,85 @@ describe('Routing metadata', () => {
       'getChildName',
     );
   });
+
+  it('allows children to override parent REST operations', () => {
+    const operationSpec = anOperationSpec()
+      .withStringResponse()
+      .build();
+
+    class Parent {
+      @get('/parent-name', operationSpec)
+      getName() {
+        return 'The Parent';
+      }
+    }
+
+    class Child extends Parent {
+      @get('/child-name', operationSpec)
+      getName() {
+        return 'The Child';
+      }
+    }
+
+    const childSpec = getControllerSpec(Child);
+    const parentSpec = getControllerSpec(Parent);
+
+    expect(childSpec.paths['/child-name']['get']).to.have.property(
+      'x-operation-name',
+      'getName',
+    );
+
+    // The parent endpoint has been overridden
+    expect(childSpec.paths).to.not.have.property('/parent-name');
+
+    expect(parentSpec.paths['/parent-name']['get']).to.have.property(
+      'x-operation-name',
+      'getName',
+    );
+
+    // The parent endpoint should not be polluted
+    expect(parentSpec.paths).to.not.have.property('/child-name');
+  });
+
+  it('allows children to override parent REST parameters', () => {
+    const operationSpec = anOperationSpec()
+      .withStringResponse()
+      .build();
+
+    class Parent {
+      @get('/greet', operationSpec)
+      greet(@param.query.string('msg') msg: string) {
+        return `Parent: ${msg}`;
+      }
+    }
+
+    class Child extends Parent {
+      greet(@param.query.string('message') msg: string) {
+        return `Child: ${msg}`;
+      }
+    }
+
+    const childSpec = getControllerSpec(Child);
+    const parentSpec = getControllerSpec(Parent);
+
+    const childGreet = childSpec.paths['/greet']['get'];
+    expect(childGreet).to.have.property('x-operation-name', 'greet');
+
+    expect(childGreet.parameters).to.have.property('length', 1);
+
+    expect(childGreet.parameters[0]).to.containEql({
+      name: 'message',
+      in: 'query',
+    });
+
+    const parentGreet = parentSpec.paths['/greet']['get'];
+    expect(parentGreet).to.have.property('x-operation-name', 'greet');
+
+    expect(parentGreet.parameters).to.have.property('length', 1);
+
+    expect(parentGreet.parameters[0]).to.containEql({
+      name: 'msg',
+      in: 'query',
+    });
+  });
 });