From 46646a9bff35d4e7b18c0de774c9a81be46b347b Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Wed, 26 Apr 2017 21:44:35 +0200 Subject: [PATCH 1/5] Replace self references with "${self:}" on save state --- .../aws/package/lib/saveServiceState.js | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/plugins/aws/package/lib/saveServiceState.js b/lib/plugins/aws/package/lib/saveServiceState.js index 8ebcb1b0f..395c136e5 100644 --- a/lib/plugins/aws/package/lib/saveServiceState.js +++ b/lib/plugins/aws/package/lib/saveServiceState.js @@ -4,6 +4,39 @@ const BbPromise = require('bluebird'); const path = require('path'); const _ = require('lodash'); +/** + * Find all self references within a given object. + * The search is implemented non-recursive to prevent stackoverflows and will + * do a complete deep search including arrays. + * @param root {Object} Root object for search + * @param self {Object} Object reference to be treated as self + * @returns {Array} Paths to all self references found within the object + */ +function findSelfReferences(root, self) { + const resourcePaths = []; + const stack = [{ parent: null, value: root, path: '' }]; + + while (!_.isEmpty(stack)) { + const property = stack.pop(); + + _.forOwn(property.value, (value, key) => { + if (value === self) { + resourcePaths.push(`${property.path}.${key}`); + } else if (_.isObject(value)) { + let propKey; + if (_.isArray(property.value)) { + propKey = `[${key}]`; + } else { + propKey = _.isEmpty(property.path) ? `${key}` : `.${key}`; + } + stack.push({ parent: property, value, path: `${property.path}${propKey}` }); + } + }); + } + + return resourcePaths; +} + module.exports = { saveServiceState() { const serviceStateFileName = this.provider.naming.getServiceStateFileName(); @@ -20,8 +53,14 @@ module.exports = { ) ); + const strippedService = _.assign( + {}, _.omit(this.serverless.service, ['serverless', 'package']) + ); + const selfReferences = findSelfReferences(strippedService, this.serverless.service); + _.forEach(selfReferences, refPath => _.set(strippedService, refPath, '${self:}')); + const state = { - service: _.assign({}, _.omit(this.serverless.service, ['serverless', 'package'])), + service: strippedService, package: { individually: this.serverless.service.package.individually, artifactDirectoryName: this.serverless.service.package.artifactDirectoryName, From 33a93e7a2886954675f6699ca5eb0baade648bd4 Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Wed, 26 Apr 2017 21:54:24 +0200 Subject: [PATCH 2/5] Move findReferences to utils. It will be used in extendedValidate too. --- .../aws/package/lib/saveServiceState.js | 36 +----------------- lib/plugins/aws/utils/findReferences.js | 38 +++++++++++++++++++ 2 files changed, 40 insertions(+), 34 deletions(-) create mode 100644 lib/plugins/aws/utils/findReferences.js diff --git a/lib/plugins/aws/package/lib/saveServiceState.js b/lib/plugins/aws/package/lib/saveServiceState.js index 395c136e5..46756bd62 100644 --- a/lib/plugins/aws/package/lib/saveServiceState.js +++ b/lib/plugins/aws/package/lib/saveServiceState.js @@ -3,39 +3,7 @@ const BbPromise = require('bluebird'); const path = require('path'); const _ = require('lodash'); - -/** - * Find all self references within a given object. - * The search is implemented non-recursive to prevent stackoverflows and will - * do a complete deep search including arrays. - * @param root {Object} Root object for search - * @param self {Object} Object reference to be treated as self - * @returns {Array} Paths to all self references found within the object - */ -function findSelfReferences(root, self) { - const resourcePaths = []; - const stack = [{ parent: null, value: root, path: '' }]; - - while (!_.isEmpty(stack)) { - const property = stack.pop(); - - _.forOwn(property.value, (value, key) => { - if (value === self) { - resourcePaths.push(`${property.path}.${key}`); - } else if (_.isObject(value)) { - let propKey; - if (_.isArray(property.value)) { - propKey = `[${key}]`; - } else { - propKey = _.isEmpty(property.path) ? `${key}` : `.${key}`; - } - stack.push({ parent: property, value, path: `${property.path}${propKey}` }); - } - }); - } - - return resourcePaths; -} +const findReferences = require('../../utils/findReferences'); module.exports = { saveServiceState() { @@ -56,7 +24,7 @@ module.exports = { const strippedService = _.assign( {}, _.omit(this.serverless.service, ['serverless', 'package']) ); - const selfReferences = findSelfReferences(strippedService, this.serverless.service); + const selfReferences = findReferences(strippedService, this.serverless.service); _.forEach(selfReferences, refPath => _.set(strippedService, refPath, '${self:}')); const state = { diff --git a/lib/plugins/aws/utils/findReferences.js b/lib/plugins/aws/utils/findReferences.js new file mode 100644 index 000000000..570e74c87 --- /dev/null +++ b/lib/plugins/aws/utils/findReferences.js @@ -0,0 +1,38 @@ +'use strict'; + +const _ = require('lodash'); + +/** + * Find all objects with a given value within a given root object. + * The search is implemented non-recursive to prevent stackoverflows and will + * do a complete deep search including arrays. + * @param root {Object} Root object for search + * @param value {Object} Value to search + * @returns {Array} Paths to all self references found within the object + */ +function findReferences(root, value) { + const resourcePaths = []; + const stack = [{ parent: null, propValue: root, path: '' }]; + + while (!_.isEmpty(stack)) { + const property = stack.pop(); + + _.forOwn(property.propValue, (propValue, key) => { + if (propValue === value) { + resourcePaths.push(`${property.path}.${key}`); + } else if (_.isObject(propValue)) { + let propKey; + if (_.isArray(property.propValue)) { + propKey = `[${key}]`; + } else { + propKey = _.isEmpty(property.path) ? `${key}` : `.${key}`; + } + stack.push({ parent: property, propValue, path: `${property.path}${propKey}` }); + } + }); + } + + return resourcePaths; +} + +module.exports = findReferences; From 9257256e7a3ece7f3725c87abb0fba64f71d65a1 Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Wed, 26 Apr 2017 22:09:51 +0200 Subject: [PATCH 3/5] Restore self references on state restore --- lib/plugins/aws/deploy/lib/extendedValidate.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/plugins/aws/deploy/lib/extendedValidate.js b/lib/plugins/aws/deploy/lib/extendedValidate.js index 557692a81..86221dd33 100644 --- a/lib/plugins/aws/deploy/lib/extendedValidate.js +++ b/lib/plugins/aws/deploy/lib/extendedValidate.js @@ -3,6 +3,7 @@ const path = require('path'); const BbPromise = require('bluebird'); const _ = require('lodash'); +const findReferences = require('../../utils/findReferences'); module.exports = { extendedValidate() { @@ -17,7 +18,11 @@ module.exports = { } const state = this.serverless .utils.readFileSync(serviceStateFilePath); + const selfReferences = findReferences(state.service, '${self:}'); + _.forEach(selfReferences, ref => _.set(state.service, ref, this.serverless.service)); + _.assign(this.serverless.service, state.service); + this.serverless.service.package.artifactDirectoryName = state.package.artifactDirectoryName; if (!_.isEmpty(state.package.artifact)) { this.serverless.service.package.artifact = path From cc91ca256375c8024c170fc1166ecbdb4bbbe30a Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Wed, 26 Apr 2017 23:56:01 +0200 Subject: [PATCH 4/5] Prevent circular references during search. --- lib/plugins/aws/utils/findReferences.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/plugins/aws/utils/findReferences.js b/lib/plugins/aws/utils/findReferences.js index 570e74c87..59b8ff609 100644 --- a/lib/plugins/aws/utils/findReferences.js +++ b/lib/plugins/aws/utils/findReferences.js @@ -11,6 +11,7 @@ const _ = require('lodash'); * @returns {Array} Paths to all self references found within the object */ function findReferences(root, value) { + const visitedObjects = []; const resourcePaths = []; const stack = [{ parent: null, propValue: root, path: '' }]; @@ -21,6 +22,11 @@ function findReferences(root, value) { if (propValue === value) { resourcePaths.push(`${property.path}.${key}`); } else if (_.isObject(propValue)) { + // Prevent circular references + if (_.includes(visitedObjects, propValue)) { + return; + } + visitedObjects.push(propValue); let propKey; if (_.isArray(property.propValue)) { propKey = `[${key}]`; From e8bdd0ca3365c9309e51499320ed0e7a085ea927 Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Thu, 27 Apr 2017 02:28:13 +0200 Subject: [PATCH 5/5] Added unit tests and a small fix with property paths --- .../aws/package/lib/saveServiceState.test.js | 34 +++++++ lib/plugins/aws/utils/findReferences.js | 18 ++-- lib/plugins/aws/utils/findReferences.test.js | 88 +++++++++++++++++++ 3 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 lib/plugins/aws/utils/findReferences.test.js diff --git a/lib/plugins/aws/package/lib/saveServiceState.test.js b/lib/plugins/aws/package/lib/saveServiceState.test.js index e4a2e4a61..33f7ab1ac 100644 --- a/lib/plugins/aws/package/lib/saveServiceState.test.js +++ b/lib/plugins/aws/package/lib/saveServiceState.test.js @@ -66,4 +66,38 @@ describe('#saveServiceState()', () => { .to.equal(true); }); }); + + it('should remove self references correctly', () => { + const filePath = path.join( + awsPackage.serverless.config.servicePath, + '.serverless', + 'service-state.json' + ); + + serverless.service.custom = { + mySelfRef: serverless.service, + }; + + return awsPackage.saveServiceState().then(() => { + const expectedStateFileContent = { + service: { + provider: { + compiledCloudFormationTemplate: 'compiled content', + }, + custom: { + mySelfRef: '${self:}', + }, + }, + package: { + individually: false, + artifactDirectoryName: 'artifact-directory', + artifact: 'service.zip', + }, + }; + + expect(getServiceStateFileNameStub.calledOnce).to.equal(true); + expect(writeFileSyncStub.calledWithExactly(filePath, expectedStateFileContent)) + .to.equal(true); + }); + }); }); diff --git a/lib/plugins/aws/utils/findReferences.js b/lib/plugins/aws/utils/findReferences.js index 59b8ff609..0f70c70e7 100644 --- a/lib/plugins/aws/utils/findReferences.js +++ b/lib/plugins/aws/utils/findReferences.js @@ -13,27 +13,27 @@ const _ = require('lodash'); function findReferences(root, value) { const visitedObjects = []; const resourcePaths = []; - const stack = [{ parent: null, propValue: root, path: '' }]; + const stack = [{ propValue: root, path: '' }]; while (!_.isEmpty(stack)) { const property = stack.pop(); _.forOwn(property.propValue, (propValue, key) => { + let propKey; + if (_.isArray(property.propValue)) { + propKey = `[${key}]`; + } else { + propKey = _.isEmpty(property.path) ? `${key}` : `.${key}`; + } if (propValue === value) { - resourcePaths.push(`${property.path}.${key}`); + resourcePaths.push(`${property.path}${propKey}`); } else if (_.isObject(propValue)) { // Prevent circular references if (_.includes(visitedObjects, propValue)) { return; } visitedObjects.push(propValue); - let propKey; - if (_.isArray(property.propValue)) { - propKey = `[${key}]`; - } else { - propKey = _.isEmpty(property.path) ? `${key}` : `.${key}`; - } - stack.push({ parent: property, propValue, path: `${property.path}${propKey}` }); + stack.push({ propValue, path: `${property.path}${propKey}` }); } }); } diff --git a/lib/plugins/aws/utils/findReferences.test.js b/lib/plugins/aws/utils/findReferences.test.js new file mode 100644 index 000000000..cc994712d --- /dev/null +++ b/lib/plugins/aws/utils/findReferences.test.js @@ -0,0 +1,88 @@ +'use strict'; + +const expect = require('chai').expect; +const _ = require('lodash'); +const findReferences = require('./findReferences'); + +describe('#findReferences()', () => { + it('should succeed on invalid input', () => { + const withoutArgs = findReferences(); + const nullArgs = findReferences(null); + + expect(withoutArgs).to.be.a('Array').to.have.lengthOf(0); + expect(nullArgs).to.be.a('Array').have.lengthOf(0); + }); + + it('should return paths', () => { + const testObject = { + prop1: 'test', + array1: [ + { + prop1: 'hit', + prop2: 4, + }, + 'hit', + [ + { + prop1: null, + prop2: 'hit', + }, + ], + ], + prop2: { + prop1: 'foo', + prop2: { + prop1: 'hit', + }, + }, + }; + + const expectedResult = [ + 'array1[0].prop1', + 'array1[1]', + 'array1[2][0].prop2', + 'prop2.prop2.prop1', + ]; + const paths = findReferences(testObject, 'hit'); + + expect(paths).to.be.a('Array').to.have.lengthOf(4); + expect(_.every(paths, path => _.includes(expectedResult, path))).to.equal(true); + }); + + it('should not fail with circular references', () => { + const testObject = { + prop1: 'test', + array1: [ + { + prop1: 'hit', + prop2: 4, + }, + 'hit', + [ + { + prop1: null, + prop2: 'hit', + }, + ], + ], + prop2: { + prop1: 'foo', + prop2: { + prop1: 'hit', + }, + }, + }; + testObject.array1.push(testObject.prop2); + + const expectedResult = [ + 'array1[0].prop1', + 'array1[1]', + 'array1[2][0].prop2', + 'prop2.prop2.prop1', + ]; + const paths = findReferences(testObject, 'hit'); + + expect(paths).to.be.a('Array').to.have.lengthOf(4); + expect(_.every(paths, path => _.includes(expectedResult, path))).to.equal(true); + }); +});