From bc4433403fc0f14dfd3fe848ba50df84621c7b42 Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Wed, 1 Dec 2021 12:01:48 +0100 Subject: [PATCH 1/6] refactor(Variables): Improve tests reliability --- test/unit/lib/configuration/variables/resolve.test.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/unit/lib/configuration/variables/resolve.test.js b/test/unit/lib/configuration/variables/resolve.test.js index 1b516f283b5..e985e7be4d1 100644 --- a/test/unit/lib/configuration/variables/resolve.test.js +++ b/test/unit/lib/configuration/variables/resolve.test.js @@ -79,7 +79,9 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { resolve: ({ params }) => ({ value: params.join('|') }), }, sourceAddress: { - resolve: ({ address }) => ({ value: address }), + resolve: ({ address }) => ({ + value: typeof address === 'string' ? address.split('').reverse().join('') : address, + }), }, sourceDirect: { resolve: () => ({ value: 234 }), @@ -200,7 +202,7 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { }); it('should pass address to source resolvers', () => { - expect(configuration.address).to.equal('fooaddress-result'); + expect(configuration.address).to.equal('footluser-sserdda'); }); it('should resolve variables in params', () => { @@ -223,7 +225,7 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { it('should clear escapes', () => { expect(configuration.escape).to.equal( - 'e${sourceDirect:}n\\$fooaddress-resultqe\\${sourceProperty(direct)}qn\\fooaddress-result' + 'e${sourceDirect:}n\\$footluser-sserddaqe\\${sourceProperty(direct)}qn\\footluser-sserdda' ); }); From aab453f0de52df7276555f46ac8eb2eff0a3eda6 Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Wed, 1 Dec 2021 12:10:21 +0100 Subject: [PATCH 2/6] refactor(Variables): Improve reliability of tests --- .../lib/configuration/variables/resolve.test.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/unit/lib/configuration/variables/resolve.test.js b/test/unit/lib/configuration/variables/resolve.test.js index e985e7be4d1..af0b56406b0 100644 --- a/test/unit/lib/configuration/variables/resolve.test.js +++ b/test/unit/lib/configuration/variables/resolve.test.js @@ -76,7 +76,7 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { let variablesMeta; const sources = { sourceParam: { - resolve: ({ params }) => ({ value: params.join('|') }), + resolve: ({ params }) => ({ value: params.join('|').split('').reverse().join('') }), }, sourceAddress: { resolve: ({ address }) => ({ @@ -198,7 +198,7 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { }); it('should pass params to source resolvers', () => { - expect(configuration.foo.params).to.equal('param1|param2'); + expect(configuration.foo.params).to.equal('2marap|1marap'); }); it('should pass address to source resolvers', () => { @@ -206,7 +206,7 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { }); it('should resolve variables in params', () => { - expect(configuration.foo.varParam).to.equal('234'); + expect(configuration.foo.varParam).to.equal('432'); }); it('should resolve variables in address', () => { @@ -218,8 +218,8 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { expect(configuration.otherProperty).to.equal('foo234'); expect(configuration.static).to.equal(true); expect(configuration.deepProperty).to.deep.equal({ - params: 'param1|param2', - varParam: '234', + params: '2marap|1marap', + varParam: '432', }); }); @@ -263,8 +263,8 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { it('should provide working resolveVariablesInString util', () => { expect(configuration.resolveVariablesInString).to.deep.equal({ - params: 'param1|param2', - varParam: '234', + params: '2marap|1marap', + varParam: '432', }); }); @@ -451,7 +451,7 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { describe('"resolveVariable" source util', () => { it('should resolve variable', () => { - expect(configuration.resolvesVariables).to.equal('234'); + expect(configuration.resolvesVariables).to.equal('432'); }); it('should support multiple sources', () => { expect(configuration.resolvesVariablesFallback).to.equal(null); From 2b97d68d15bdc546d6f65479bad92e27db128533 Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Wed, 1 Dec 2021 12:43:57 +0100 Subject: [PATCH 3/6] refactor(Variables): Cleanup logic --- lib/configuration/variables/resolve.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/configuration/variables/resolve.js b/lib/configuration/variables/resolve.js index f253c48883d..a1e94f99ed1 100644 --- a/lib/configuration/variables/resolve.js +++ b/lib/configuration/variables/resolve.js @@ -386,14 +386,8 @@ Object.defineProperties( const propertyPathKeys = propertyPath.split('\0'); const { value, sourceValues } = valueMeta; - const valueEntries = (() => { - if (isPlainObject(value)) return Object.entries(value); - return Array.isArray(value) ? value.entries() : null; - })(); let propertyVariablesMeta; - if (valueEntries) { - propertyVariablesMeta = parseEntries(valueEntries, propertyPathKeys, new Map()); - } else if (typeof value === 'string') { + if (typeof value === 'string') { const valueVariables = (() => { const silentParse = (sourceValue) => { try { @@ -444,6 +438,14 @@ Object.defineProperties( } else if (valueMeta.error) { return; } + } else { + const valueEntries = (() => { + if (isPlainObject(value)) return Object.entries(value); + return Array.isArray(value) ? value.entries() : null; + })(); + if (valueEntries) { + propertyVariablesMeta = parseEntries(valueEntries, propertyPathKeys, new Map()); + } } if (propertyVariablesMeta && propertyVariablesMeta.size) { // Register variables found in resolved value From e0af1b46c36ec8de231052b9fdcf779c4948f84f Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Wed, 1 Dec 2021 12:53:28 +0100 Subject: [PATCH 4/6] refactor(Variables): Seclude internal logic for reuse --- lib/configuration/variables/resolve.js | 78 ++++++++++++-------------- 1 file changed, 37 insertions(+), 41 deletions(-) diff --git a/lib/configuration/variables/resolve.js b/lib/configuration/variables/resolve.js index a1e94f99ed1..707bcedde1e 100644 --- a/lib/configuration/variables/resolve.js +++ b/lib/configuration/variables/resolve.js @@ -27,6 +27,33 @@ const variableProcessingErrorNames = new Set(['ServerlessError', 'VariableSource let lastResolutionBatchId = 0; +const resolveSourceValuesVariables = (sourceValues) => { + // Value is a result of concatenation of string values coming from multiple sources. + // Parse variables in each string part individually - it's to avoid accidental + // resolution of variable-like notation which may surface after joining two values. + // Also that way we do not accidentally re-parse an escaped variables notation + let baseIndex = 0; + let resolvedValueVariables = null; + for (const sourceValue of sourceValues) { + const sourceValueVariables = parse(sourceValue); + if (sourceValueVariables) { + for (const sourceValueVariable of sourceValueVariables) { + if (sourceValueVariable.end) { + sourceValueVariable.start += baseIndex; + sourceValueVariable.end += baseIndex; + } else { + sourceValueVariable.start = baseIndex; + sourceValueVariable.end = baseIndex + sourceValue.length; + } + } + if (!resolvedValueVariables) resolvedValueVariables = []; + resolvedValueVariables.push(...sourceValueVariables); + } + baseIndex += sourceValue.length; + } + return resolvedValueVariables; +}; + class VariablesResolver { constructor({ serviceDir, @@ -389,48 +416,17 @@ Object.defineProperties( let propertyVariablesMeta; if (typeof value === 'string') { const valueVariables = (() => { - const silentParse = (sourceValue) => { - try { - return parse(sourceValue); - } catch (error) { - error.message = `Cannot resolve variable at "${humanizePropertyPath( - propertyPathKeys - )}": Approached variable syntax error in resolved value "${value}": ${ - error.message - }`; - delete valueMeta.value; - valueMeta.error = error; - return null; - } - }; - if (sourceValues) { - // Value is a result of concatenation of string values coming from multiple sources. - // Parse variables in each string part individually - it's to avoid accidental - // resolution of variable-like notation which may surface after joining two values. - // Also that way we do not accidentally re-parse an escaped variables notation - let baseIndex = 0; - let resolvedValueVariables = null; - for (const sourceValue of sourceValues) { - const sourceValueVariables = silentParse(sourceValue); - if (valueMeta.error) return null; - if (sourceValueVariables) { - for (const sourceValueVariable of sourceValueVariables) { - if (sourceValueVariable.end) { - sourceValueVariable.start += baseIndex; - sourceValueVariable.end += baseIndex; - } else { - sourceValueVariable.start = baseIndex; - sourceValueVariable.end = baseIndex + sourceValue.length; - } - } - if (!resolvedValueVariables) resolvedValueVariables = []; - resolvedValueVariables.push(...sourceValueVariables); - } - baseIndex += sourceValue.length; - } - return resolvedValueVariables; + try { + if (sourceValues) return resolveSourceValuesVariables(sourceValues); + return parse(value); + } catch (error) { + error.message = `Cannot resolve variable at "${humanizePropertyPath( + propertyPathKeys + )}": Approached variable syntax error in resolved value "${value}": ${error.message}`; + delete valueMeta.value; + valueMeta.error = error; + return null; } - return silentParse(value); })(); if (valueVariables) { From 2bf9f7b30cc7ba1770bca6a157be35ed32672238 Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Wed, 1 Dec 2021 14:14:34 +0100 Subject: [PATCH 5/6] test(Variables): Improve test error reporting --- .../configuration/variables/resolve.test.js | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/test/unit/lib/configuration/variables/resolve.test.js b/test/unit/lib/configuration/variables/resolve.test.js index af0b56406b0..2d39721dbe8 100644 --- a/test/unit/lib/configuration/variables/resolve.test.js +++ b/test/unit/lib/configuration/variables/resolve.test.js @@ -429,24 +429,22 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { }); it('should correctly record encountered variable sources', () => { - expect(variableSourcesInConfig).to.deep.equal( - new Set([ - 'sourceParam', - 'sourceDirect', - 'sourceAddress', - 'sourceProperty', - 'sourceResultVariables', - 'sourceResolveVariablesInString', - 'sourceResolveVariable', - 'sourceIncomplete', - 'sourceMissing', - 'sourceUnrecognized', - 'sourceError', - 'sourceInfinite', - 'sourceShared', - 'sourceSharedProperty', - ]) - ); + expect(Array.from(variableSourcesInConfig)).to.deep.equal([ + 'sourceParam', + 'sourceDirect', + 'sourceAddress', + 'sourceProperty', + 'sourceResultVariables', + 'sourceResolveVariablesInString', + 'sourceResolveVariable', + 'sourceIncomplete', + 'sourceMissing', + 'sourceUnrecognized', + 'sourceError', + 'sourceInfinite', + 'sourceShared', + 'sourceSharedProperty', + ]); }); describe('"resolveVariable" source util', () => { From a6b2a35a7a9f90e94ee4423d7aa1c1147639c311 Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Wed, 1 Dec 2021 14:16:53 +0100 Subject: [PATCH 6/6] fix(Variables): Resolve variables in resolved address & params values --- lib/configuration/variables/resolve.js | 82 +++++++++++++++---- .../configuration/variables/resolve.test.js | 19 +++++ 2 files changed, 85 insertions(+), 16 deletions(-) diff --git a/lib/configuration/variables/resolve.js b/lib/configuration/variables/resolve.js index 707bcedde1e..bef1748f827 100644 --- a/lib/configuration/variables/resolve.js +++ b/lib/configuration/variables/resolve.js @@ -256,20 +256,14 @@ class VariablesResolver { sourceData.params.map(async (param) => { if (!param.variables) return; await this.resolveVariables(resolutionBatchId, propertyPath, param); - if (param.error) throw param.error; + await this.resolveInternalResult(resolutionBatchId, propertyPath, param); }) ); - if (sourceData.params.some((param) => param.variables)) { - throw new ServerlessError('Cannot resolve variable', 'MISSING_VARIABLE_DEPENDENCY'); - } } if (sourceData.address && sourceData.address.variables) { // Ensure to have all eventual variables in address resolved await this.resolveVariables(resolutionBatchId, propertyPath, sourceData.address); - if (sourceData.address.error) throw sourceData.address.error; - if (sourceData.address.variables) { - throw new ServerlessError('Cannot resolve variable', 'MISSING_VARIABLE_DEPENDENCY'); - } + await this.resolveInternalResult(resolutionBatchId, propertyPath, sourceData.address); } return JSON.parse( JSON.stringify( @@ -309,6 +303,68 @@ class VariablesResolver { ) ); } + + async resolveInternalResult(resolutionBatchId, propertyPath, valueMeta, nestTracker = 10) { + if (valueMeta.error) throw valueMeta.error; + if (valueMeta.variables) { + throw new ServerlessError('Cannot resolve variable', 'MISSING_VARIABLE_DEPENDENCY'); + } + if (!nestTracker) { + throw new ServerlessError( + `Cannot resolve variable at "${humanizePropertyPath( + propertyPath.split('\0') + )}": Excessive variables nest depth`, + 'EXCESSIVE_RESOLVED_VARIABLES_NEST_DEPTH' + ); + } + if (typeof valueMeta.value === 'string') { + const valueVariables = (() => { + try { + if (valueMeta.sourceValues) return resolveSourceValuesVariables(valueMeta.sourceValues); + return parse(valueMeta.value); + } catch (error) { + error.message = `Cannot resolve variable at "${humanizePropertyPath( + propertyPath.split('\0') + )}": Approached variable syntax error in resolved value "${valueMeta.value}": ${ + error.message + }`; + delete valueMeta.value; + valueMeta.error = error; + throw error; + } + })(); + + if (!valueVariables) return; + valueMeta.variables = valueVariables; + delete valueMeta.sourceValues; + await this.resolveVariables(resolutionBatchId, propertyPath, valueMeta); + await this.resolveInternalResult(resolutionBatchId, propertyPath, valueMeta, --nestTracker); + return; + } + const valueEntries = (() => { + if (isPlainObject(valueMeta.value)) return Object.entries(valueMeta.value); + return Array.isArray(valueMeta.value) ? valueMeta.value.entries() : null; + })(); + if (!valueEntries) return; + const propertyVariablesMeta = parseEntries(valueEntries, [], new Map()); + for (const [propertyKeyPath, propertyValueMeta] of propertyVariablesMeta) { + await this.resolveVariables(resolutionBatchId, propertyPath, propertyValueMeta); + await this.resolveInternalResult( + resolutionBatchId, + propertyPath, + propertyValueMeta, + --nestTracker + ); + const propertyKeyPathKeys = propertyKeyPath.split('\0'); + const targetKey = propertyKeyPathKeys[propertyKeyPathKeys.length - 1]; + let targetObject = valueMeta.value; + for (const parentKey of propertyKeyPathKeys.slice(0, -1)) { + targetObject = targetObject[parentKey]; + } + targetObject[targetKey] = propertyValueMeta.value; + } + } + validateCrossPropertyDependency(dependentPropertyPath, dependencyPropertyPath) { if (dependentPropertyPath === dependencyPropertyPath) { throw new ServerlessError( @@ -531,10 +587,7 @@ Object.defineProperties( variables: variableData, }; await this.resolveVariables(resolutionBatchId, propertyPath, meta); - if (meta.error) throw meta.error; - if (meta.variables) { - throw new ServerlessError('Cannot resolve variable', 'MISSING_VARIABLE_DEPENDENCY'); - } + await this.resolveInternalResult(resolutionBatchId, propertyPath, meta); return meta.value; }, resolveVariablesInString: async (stringValue) => { @@ -550,10 +603,7 @@ Object.defineProperties( variables: variableData, }; await this.resolveVariables(resolutionBatchId, propertyPath, meta); - if (meta.error) throw meta.error; - if (meta.variables) { - throw new ServerlessError('Cannot resolve variable', 'MISSING_VARIABLE_DEPENDENCY'); - } + await this.resolveInternalResult(resolutionBatchId, propertyPath, meta); return meta.value; }, }); diff --git a/test/unit/lib/configuration/variables/resolve.test.js b/test/unit/lib/configuration/variables/resolve.test.js index 2d39721dbe8..e16239204e6 100644 --- a/test/unit/lib/configuration/variables/resolve.test.js +++ b/test/unit/lib/configuration/variables/resolve.test.js @@ -40,6 +40,12 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { resolvesResultVariablesStringInvalid: '${sourceResultVariables(stringInvalid)}', resolveDeepVariablesConcat: '${sourceResultVariables(string)}foo${sourceResultVariables(string)}', + resolveDeepVariablesConcatInParam: + '${sourceParam(${sourceResultVariables(string)}foo${sourceResultVariables(string)})}', + resolveDeepVariablesConcatInAddress: + '${sourceAddress:${sourceResultVariables(string)}foo${sourceResultVariables(string)}}', + infiniteDeepVariablesConcat: + '${sourceAddress:${sourceInfiniteString:}foo${sourceResultVariables(string)}}', resolveVariablesInString: "${sourceResolveVariablesInString('${sourceProperty(foo)}')}", resolvesVariables: '${sourceResolveVariable("sourceParam(${sourceDirect:})")}', resolvesVariablesFallback: '${sourceResolveVariable("sourceMissing:, null"), null}', @@ -156,6 +162,9 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { sourceInfinite: { resolve: () => ({ value: { nest: '${sourceInfinite:}' } }), }, + sourceInfiniteString: { + resolve: () => ({ value: '${sourceInfiniteString:}' }), + }, sourceShared: { resolve: () => ({ value: { @@ -259,6 +268,8 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { it('should resolve variables in resolved strings which are subject to concatenation', () => { expect(configuration.resolveDeepVariablesConcat).to.equal('234foo234'); + expect(configuration.resolveDeepVariablesConcatInParam).to.equal('432oof432'); + expect(configuration.resolveDeepVariablesConcatInAddress).to.equal('432oof432'); }); it('should provide working resolveVariablesInString util', () => { @@ -344,6 +355,12 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { expect(valueMeta.error.code).to.equal('VARIABLE_RESOLUTION_ERROR'); }); + it('should error on infinite variables resolution recursion', () => { + const valueMeta = variablesMeta.get('infiniteDeepVariablesConcat'); + expect(valueMeta).to.not.have.property('variables'); + expect(valueMeta.error.code).to.equal('EXCESSIVE_RESOLVED_VARIABLES_NEST_DEPTH'); + }); + it('should mark deep circular dependency among properties with error', () => { const valueMeta = variablesMeta.get('propertyDeepCircularA'); expect(valueMeta).to.not.have.property('variables'); @@ -410,6 +427,7 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { 'propertyDeepCircularC', 'propertyRoot', 'resolvesResultVariablesStringInvalid', + 'infiniteDeepVariablesConcat', 'resolvesVariablesInvalid1', 'resolvesVariablesInvalid2', 'missing', @@ -435,6 +453,7 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => { 'sourceAddress', 'sourceProperty', 'sourceResultVariables', + 'sourceInfiniteString', 'sourceResolveVariablesInString', 'sourceResolveVariable', 'sourceIncomplete',