From 91e3367af43c752e4a613bdf383c8a4dc476a1d9 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Thu, 16 Nov 2017 11:58:11 -0800 Subject: [PATCH] fix(rest): Improve rest metadata inheritance Make sure metadata inherited from base classes are cloned to avoid pollution of base information --- packages/rest/package.json | 7 +- packages/rest/src/router/metadata.ts | 87 ++++++++++++------- .../rest/test/unit/router/metadata.test.ts | 82 +++++++++++++++++ 3 files changed, 141 insertions(+), 35 deletions(-) diff --git a/packages/rest/package.json b/packages/rest/package.json index 0862bd14cf64..fa993027a8ca 100644 --- a/packages/rest/package.json +++ b/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", diff --git a/packages/rest/src/router/metadata.ts b/packages/rest/src/router/metadata.ts index e861e6bad7c0..912afbda4502 100644 --- a/packages/rest/src/router/metadata.ts +++ b/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(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} { + 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,24 +113,27 @@ 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 || - ''; - 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 || + ''; + const fullMethodName = `${className}.${op}`; + endpointName = `${fullMethodName} (${verb} ${path})`; + } let operationSpec = endpoint.spec; if (!operationSpec) { @@ -111,8 +141,11 @@ function resolveControllerSpec( 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 = 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 = 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; } diff --git a/packages/rest/test/unit/router/metadata.test.ts b/packages/rest/test/unit/router/metadata.test.ts index 7ecb1b5ccc12..c00653602e08 100644 --- a/packages/rest/test/unit/router/metadata.test.ts +++ b/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', + }); + }); });