Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Variables: Ensure complete resolution of address and params values #10296

Merged
merged 6 commits into from
Dec 2, 2021
Merged

Variables: Ensure complete resolution of address and params values #10296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc44334
refactor(Variables): Improve tests reliability
medikoo Dec 1, 2021
aab453f
refactor(Variables): Improve reliability of tests
medikoo Dec 1, 2021
2b97d68
refactor(Variables): Cleanup logic
medikoo Dec 1, 2021
e0af1b4
refactor(Variables): Seclude internal logic for reuse
medikoo Dec 1, 2021
2bf9f7b
test(Variables): Improve test error reporting
medikoo Dec 1, 2021
a6b2a35
fix(Variables): Resolve variables in resolved address & params values
medikoo Dec 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
176 changes: 112 additions & 64 deletions lib/configuration/variables/resolve.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -229,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(
Expand Down Expand Up @@ -282,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(
Expand Down Expand Up @@ -386,64 +469,35 @@ 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 {
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) {
propertyVariablesMeta = new Map([[propertyPath, { value, variables: valueVariables }]]);
} 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
Expand Down Expand Up @@ -533,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) => {
Expand All @@ -552,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;
},
});
Expand Down
77 changes: 48 additions & 29 deletions test/unit/lib/configuration/variables/resolve.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}',
Expand Down Expand Up @@ -76,10 +82,12 @@ 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 }) => ({ value: address }),
resolve: ({ address }) => ({
value: typeof address === 'string' ? address.split('').reverse().join('') : address,
}),
},
sourceDirect: {
resolve: () => ({ value: 234 }),
Expand Down Expand Up @@ -154,6 +162,9 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => {
sourceInfinite: {
resolve: () => ({ value: { nest: '${sourceInfinite:}' } }),
},
sourceInfiniteString: {
resolve: () => ({ value: '${sourceInfiniteString:}' }),
},
sourceShared: {
resolve: () => ({
value: {
Expand Down Expand Up @@ -196,15 +207,15 @@ 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', () => {
expect(configuration.address).to.equal('fooaddress-result');
expect(configuration.address).to.equal('footluser-sserdda');
});

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', () => {
Expand All @@ -216,14 +227,14 @@ 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',
});
});

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'
);
});

Expand Down Expand Up @@ -257,12 +268,14 @@ 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', () => {
expect(configuration.resolveVariablesInString).to.deep.equal({
params: 'param1|param2',
varParam: '234',
params: '2marap|1marap',
varParam: '432',
});
});

Expand Down Expand Up @@ -342,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');
Expand Down Expand Up @@ -408,6 +427,7 @@ describe('test/unit/lib/configuration/variables/resolve.test.js', () => {
'propertyDeepCircularC',
'propertyRoot',
'resolvesResultVariablesStringInvalid',
'infiniteDeepVariablesConcat',
'resolvesVariablesInvalid1',
'resolvesVariablesInvalid2',
'missing',
Expand All @@ -427,29 +447,28 @@ 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',
'sourceInfiniteString',
'sourceResolveVariablesInString',
'sourceResolveVariable',
'sourceIncomplete',
'sourceMissing',
'sourceUnrecognized',
'sourceError',
'sourceInfinite',
'sourceShared',
'sourceSharedProperty',
]);
});

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);
Expand Down