From cc4ad0f450a5eeb3c7e1d55da57a461ed1cf36c5 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 30 Oct 2020 17:12:53 +0100 Subject: [PATCH 1/6] refactor(core): refactor `CloudFormationLang.toJSON()` Our previous implementation of `toJSON()` was quite hacky. It replaced values inside the structure with objects that had a custom `toJSON()` serializer, and then called `JSON.stringify()` on the result. The resulting JSON would have special markers in it where the Token values would be string-substituted back in. It's actually easier and gives us more control to just implement JSONification ourselves in a Token-aware recursive function. This change has been split off from a larger, upcoming PR in order to make the individual reviews smaller. --- .../core/lib/private/cloudformation-lang.ts | 333 +++++++++++++----- .../@aws-cdk/core/lib/private/token-map.ts | 3 +- .../@aws-cdk/core/lib/string-fragments.ts | 41 +++ .../core/test/cloudformation-json.test.ts | 243 +++++++------ packages/@aws-cdk/core/test/evaluate-cfn.ts | 2 +- 5 files changed, 437 insertions(+), 185 deletions(-) diff --git a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts index 4a74665b8f338..fe3f15c54f954 100644 --- a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts +++ b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts @@ -1,10 +1,7 @@ import { Lazy } from '../lazy'; -import { Reference } from '../reference'; -import { DefaultTokenResolver, IFragmentConcatenator, IPostProcessor, IResolvable, IResolveContext } from '../resolvable'; -import { TokenizedStringFragments } from '../string-fragments'; -import { Token } from '../token'; -import { Intrinsic } from './intrinsic'; -import { resolve } from './resolve'; +import { DefaultTokenResolver, IFragmentConcatenator, IResolveContext } from '../resolvable'; +import { isResolvableObject, Token } from '../token'; +import { TokenMap } from './token-map'; /** * Routines that know how to do operations at the CloudFormation document language level @@ -24,59 +21,12 @@ export class CloudFormationLang { * @param space Indentation to use (default: no pretty-printing) */ public static toJSON(obj: any, space?: number): string { - // This works in two stages: - // - // First, resolve everything. This gets rid of the lazy evaluations, evaluation - // to the real types of things (for example, would a function return a string, an - // intrinsic, or a number? We have to resolve to know). - // - // We then to through the returned result, identify things that evaluated to - // CloudFormation intrinsics, and re-wrap those in Tokens that have a - // toJSON() method returning their string representation. If we then call - // JSON.stringify() on that result, that gives us essentially the same - // string that we started with, except with the non-token characters quoted. - // - // {"field": "${TOKEN}"} --> {\"field\": \"${TOKEN}\"} - // - // A final resolve() on that string (done by the framework) will yield the string - // we're after. - // - // Resolving and wrapping are done in go using the resolver framework. - class IntrinsincWrapper extends DefaultTokenResolver { - constructor() { - super(CLOUDFORMATION_CONCAT); - } - - public resolveToken(t: IResolvable, context: IResolveContext, postProcess: IPostProcessor) { - // Return References directly, so their type is maintained and the references will - // continue to work. Only while preparing, because we do need the final value of the - // token while resolving. - if (Reference.isReference(t) && context.preparing) { return wrap(t); } - - // Deep-resolve and wrap. This is necessary for Lazy tokens so we can see "inside" them. - return wrap(super.resolveToken(t, context, postProcess)); - } - public resolveString(fragments: TokenizedStringFragments, context: IResolveContext) { - return wrap(super.resolveString(fragments, context)); - } - public resolveList(l: string[], context: IResolveContext) { - return wrap(super.resolveList(l, context)); - } - } - - // We need a ResolveContext to get started so return a Token return Lazy.stringValue({ - produce: (ctx: IResolveContext) => - JSON.stringify(resolve(obj, { - preparing: ctx.preparing, - scope: ctx.scope, - resolver: new IntrinsincWrapper(), - }), undefined, space), + // We used to do this by hooking into `JSON.stringify()` by adding in objects + // with custom `toJSON()` functions, but it's ultimately simpler just to + // reimplement the `stringify()` function from scratch. + produce: (ctx) => tokenAwareStringify(obj, space ?? 0, ctx), }); - - function wrap(value: any): any { - return isIntrinsic(value) ? new JsonToken(deepQuoteStringsForJSON(value)) : value; - } } /** @@ -97,44 +47,213 @@ export class CloudFormationLang { // Otherwise return a Join intrinsic (already in the target document language to avoid taking // circular dependencies on FnJoin & friends) - return { 'Fn::Join': ['', minimalCloudFormationJoin('', parts)] }; + return fnJoinConcat(parts); } } /** - * Token that also stringifies in the toJSON() operation. + * Return a CFN intrinsic mass concatting any number of CloudFormation expressions */ -class JsonToken extends Intrinsic { - /** - * Special handler that gets called when JSON.stringify() is used. - */ - public toJSON() { - return this.toString(); - } +function fnJoinConcat(parts: any[]) { + return { 'Fn::Join': ['', minimalCloudFormationJoin('', parts)] }; } /** - * Deep escape strings for use in a JSON context + * Perform a JSON.stringify()-like operation, except aware of Tokens and CloudFormation intrincics + * + * Tokens will be resolved and if they resolve to CloudFormation intrinsics, the intrinsics + * will be lifted to the top of a giant `{ Fn::Join}` expression. + * + * We are looking to do the following transforms: + * + * (a) Token in a string context + * + * { "field": "a${TOKEN}b" } -> "{ \"field\": \"a" ++ resolve(TOKEN) ++ "b\" }" + * { "a${TOKEN}b": "value" } -> "{ \"a" ++ resolve(TOKEN) ++ "b\": \"value\" }" + * + * (b) Standalone token + * + * { "field": TOKEN } -> + * + * if TOKEN resolves to a string (or is a non-encoded or string-encoded intrinsic) -> + * "{ \"field\": \"" ++ resolve(TOKEN) ++ "\" }" + * if TOKEN resolves to a non-string (or is a non-string-encoded intrinsic) -> + * "{ \"field\": " ++ resolve(TOKEN) ++ " }" + * + * (Where ++ is the CloudFormation string-concat operation (`{ Fn::Join }`). + * + * ------------------- + * + * Here come complex type interpretation rules, which we are unable to simplify because + * some clients our there are already taking dependencies on the unintended side effects + * of the old implementation. + * + * 1. If TOKEN is embedded in a string with a prefix or postfix, we'll render the token + * as a string regardless of whether it returns an intrinsic or a literal. + * + * 2. If TOKEN resolves to an intrinsic: + * - We'll treat it as a string if the TOKEN itself was not encoded or string-encoded + * (this covers the 99% case of what CloudFormation intrinsics resolve to). + * - We'll treat it as a non-string otherwise; the intrinsic MUST resolve to a number; + * * if resolves to a list { Fn::Join } will fail + * * if it resolves to a string after all the JSON will be malformed at API call time. + * + * 3. Otherwise, the type of the value it resolves to (string, number, complex object, ...) + * determines how the value is rendered. */ -function deepQuoteStringsForJSON(x: any): any { - if (typeof x === 'string') { - // Whenever we escape a string we strip off the outermost quotes - // since we're already in a quoted context. - const stringified = JSON.stringify(x); - return stringified.substring(1, stringified.length - 1); +function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) { + let indent = 0; + + const ret = new Array(); + recurse(root); + switch (ret.length) { + case 0: return ''; + case 1: return renderSegment(ret[0]); + default: + return fnJoinConcat(ret.map(renderSegment)); + } + + /** + * Stringify a JSON element + */ + function recurse(obj: any): void { + if (Token.isUnresolved(obj)) { + return handleToken(obj); + } + if (Array.isArray(obj)) { + return renderCollection('[', ']', obj, recurse); + } + if (typeof obj === 'object' && obj != null && !(obj instanceof Date)) { + return renderCollection('{', '}', definedEntries(obj), ([key, value]) => { + recurse(key); + pushLiteral(prettyPunctuation(':')); + recurse(value); + }); + } + // Otherwise we have a scalar, defer to JSON.stringify()s serialization + pushLiteral(JSON.stringify(obj)); } - if (Array.isArray(x)) { - return x.map(deepQuoteStringsForJSON); + /** + * Render an object or list + */ + function renderCollection(pre: string, post: string, xs: Iterable, each: (x: A) => void) { + pushLiteral(pre); + indent += space; + let atLeastOne = false; + for (const [comma, item] of sepIter(xs)) { + if (comma) { pushLiteral(','); } + pushLineBreak(); + each(item); + atLeastOne = true; + } + indent -= space; + if (atLeastOne) { pushLineBreak(); } + pushLiteral(post); } - if (typeof x === 'object') { - for (const key of Object.keys(x)) { - x[key] = deepQuoteStringsForJSON(x[key]); + /** + * Handle a Token. + * + * Can be any of: + * + * - Straight up IResolvable + * - Encoded string, number or list + */ + function handleToken(token: any) { + if (typeof token === 'string') { + // Encoded string, treat like a string if it has a token and at least one other + // component, otherwise treat like a regular token and base the output quoting on the + // type of the result. + const fragments = TokenMap.instance().splitString(token); + if (fragments.length > 1) { + pushLiteral('"'); + fragments.visit({ + visitLiteral: pushLiteral, + visitToken: (tok) => { + const resolved = ctx.resolve(tok); + if (isIntrinsic(resolved)) { + pushIntrinsic(quoteInsideIntrinsic(resolved)); + } else { + // We're already in a string context, so stringify and escape + pushLiteral(quoteString(`${resolved}`)); + } + }, + // This potential case is the result of poor modeling in the tokenized string, it should not happen + visitIntrinsic: () => { throw new Error('Intrinsic not expected in a freshly-split string'); }, + }); + pushLiteral('"'); + return; + } } + + const resolved = ctx.resolve(token); + if (isIntrinsic(resolved)) { + if (isResolvableObject(token) || typeof token === 'string') { + // If the input was an unencoded IResolvable or a string-encoded value, + // treat it like it was a string (for the 99% case) + pushLiteral('"'); + pushIntrinsic(quoteInsideIntrinsic(resolved)); + pushLiteral('"'); + } else { + pushIntrinsic(resolved); + } + return; + } + + // Otherwise we got an arbitrary JSON structure from the token, recurse + recurse(resolved); } - return x; + /** + * Push a literal onto the current segment if it's also a literal, otherwise open a new Segment + */ + function pushLiteral(lit: string) { + let last = ret[ret.length - 1]; + if (last?.type !== 'literal') { + last = { type: 'literal', parts: [] }; + ret.push(last); + } + last.parts.push(lit); + } + + /** + * Add a new intrinsic segment + */ + function pushIntrinsic(intrinsic: any) { + ret.push({ type: 'intrinsic', intrinsic }); + } + + /** + * Push a line break if we are pretty-printing, otherwise don't + */ + function pushLineBreak() { + if (space > 0) { + pushLiteral(`\n${' '.repeat(indent)}`); + } + } + + /** + * Add a space after the punctuation if we are pretty-printing, no space if not + */ + function prettyPunctuation(punc: string) { + return space > 0 ? `${punc} ` : punc; + } +} + +/** + * A Segment is either a literal string or a CloudFormation intrinsic + */ +type Segment = { type: 'literal'; parts: string[] } | { type: 'intrinsic'; intrinsic: any }; + +/** + * Render a segment + */ +function renderSegment(s: Segment): NonNullable { + switch (s.type) { + case 'literal': return s.parts.join(''); + case 'intrinsic': return s.intrinsic; + } } const CLOUDFORMATION_CONCAT: IFragmentConcatenator = { @@ -204,3 +323,59 @@ export function isNameOfCloudFormationIntrinsic(name: string): boolean { // these are 'fake' intrinsics, only usable inside the parameter overrides of a CFN CodePipeline Action return name !== 'Fn::GetArtifactAtt' && name !== 'Fn::GetParam'; } + +/** + * Separated iterator + */ +function* sepIter(xs: Iterable): IterableIterator<[boolean, A]> { + let comma = false; + for (const item of xs) { + yield [comma, item]; + comma = true; + } +} + +/** + * Object.entries() but skipping undefined values + */ +function* definedEntries(xs: A): IterableIterator<[string, any]> { + for (const [key, value] of Object.entries(xs)) { + if (value !== undefined) { + yield [key, value]; + } + } +} + +/** + * Quote string literals inside an intrinsic + */ +function quoteInsideIntrinsic(x: any): any { + if (typeof x === 'object' && x != null && Object.keys(x).length === 1) { + const key = Object.keys(x)[0]; + const params = x[key]; + switch (key) { + case 'Fn::If': + return { 'Fn::If': [params[0], quoteInsideIntrinsic(params[1]), quoteInsideIntrinsic(params[2])] }; + case 'Fn::Join': + return { 'Fn::Join': [quoteInsideIntrinsic(params[0]), params[1].map(quoteInsideIntrinsic)] }; + case 'Fn::Sub': + if (Array.isArray(params)) { + return { 'Fn::Sub': [quoteInsideIntrinsic(params[0]), params[1]] }; + } else { + return { 'Fn::Sub': quoteInsideIntrinsic(params[0]) }; + } + } + } + if (typeof x === 'string') { + return quoteString(x); + } + return x; +} + +/** + * Quote the characters inside a string, for use inside toJSON + */ +function quoteString(s: string) { + s = JSON.stringify(s); + return s.substring(1, s.length - 1); +} \ No newline at end of file diff --git a/packages/@aws-cdk/core/lib/private/token-map.ts b/packages/@aws-cdk/core/lib/private/token-map.ts index 1b4ea48c04440..c526a9b0e69cc 100644 --- a/packages/@aws-cdk/core/lib/private/token-map.ts +++ b/packages/@aws-cdk/core/lib/private/token-map.ts @@ -1,6 +1,6 @@ import { IResolvable } from '../resolvable'; import { TokenizedStringFragments } from '../string-fragments'; -import { Token } from '../token'; +import { isResolvableObject, Token } from '../token'; import { BEGIN_LIST_TOKEN_MARKER, BEGIN_STRING_TOKEN_MARKER, createTokenDouble, END_TOKEN_MARKER, extractTokenDouble, TokenString, VALID_KEY_CHARS, @@ -77,6 +77,7 @@ export class TokenMap { * Lookup a token from an encoded value */ public tokenFromEncoding(x: any): IResolvable | undefined { + if (isResolvableObject(x)) { return x; } if (typeof x === 'string') { return this.lookupString(x); } if (Array.isArray(x)) { return this.lookupList(x); } if (Token.isUnresolved(x)) { return x; } diff --git a/packages/@aws-cdk/core/lib/string-fragments.ts b/packages/@aws-cdk/core/lib/string-fragments.ts index b92fd3628a28d..81606bf7b4c8d 100644 --- a/packages/@aws-cdk/core/lib/string-fragments.ts +++ b/packages/@aws-cdk/core/lib/string-fragments.ts @@ -84,6 +84,25 @@ export class TokenizedStringFragments { return ret; } + /** + * Visit all fragments of the string + */ + public visit(visitor: ITokenVisitor) { + for (const f of this.fragments) { + switch (f.type) { + case 'literal': + visitor.visitLiteral(f.lit); + break; + case 'token': + visitor.visitToken(f.token); + break; + case 'intrinsic': + visitor.visitIntrinsic(f.value); + break; + } + } + } + /** * Combine the string fragments using the given joiner. * @@ -116,6 +135,28 @@ export interface ITokenMapper { mapToken(t: IResolvable): any; } +/** + * Interface to visit parts of an encoded token string + * + * Interface so it can be exported via jsii. + */ +export interface ITokenVisitor { + /** + * Visit a literal + */ + visitLiteral(lit: string): void; + + /** + * Visit a token + */ + visitToken(tok: IResolvable): void; + + /** + * Visit an intrinsic + */ + visitIntrinsic(intrinsic: any): void; +} + /** * Resolve the value from a single fragment * diff --git a/packages/@aws-cdk/core/test/cloudformation-json.test.ts b/packages/@aws-cdk/core/test/cloudformation-json.test.ts index 8d7e571501462..813bfdd27e102 100644 --- a/packages/@aws-cdk/core/test/cloudformation-json.test.ts +++ b/packages/@aws-cdk/core/test/cloudformation-json.test.ts @@ -1,12 +1,32 @@ -import { nodeunitShim, Test } from 'nodeunit-shim'; import { App, CfnOutput, Fn, Lazy, Stack, Token } from '../lib'; import { Intrinsic } from '../lib/private/intrinsic'; import { evaluateCFN } from './evaluate-cfn'; -nodeunitShim({ - 'string tokens can be JSONified and JSONification can be reversed'(test: Test) { - const stack = new Stack(); +let app: App; +let stack: Stack; +beforeEach(() => { + app = new App(); + stack = new Stack(app, 'Stack'); +}); + +test('JSONification of literals looks like JSON.stringify', () => { + const structure = { + undefinedProp: undefined, + nestedObject: { + prop1: undefined, + prop2: 'abc', + prop3: 42, + prop4: [1, 2, 3], + }, + }; + + expect(stack.resolve(stack.toJsonString(structure))).toEqual(JSON.stringify(structure)); + expect(stack.resolve(stack.toJsonString(structure, 2))).toEqual(JSON.stringify(structure, undefined, 2)); +}); +describe('tokens that return literals', () => { + + test('string tokens can be JSONified and JSONification can be reversed', () => { for (const token of tokensThatResolveTo('woof woof')) { // GIVEN const fido = { name: 'Fido', speaks: token }; @@ -15,15 +35,11 @@ nodeunitShim({ const resolved = stack.resolve(stack.toJsonString(fido)); // THEN - test.deepEqual(evaluateCFN(resolved), '{"name":"Fido","speaks":"woof woof"}'); + expect(evaluateCFN(resolved)).toEqual('{"name":"Fido","speaks":"woof woof"}'); } + }); - test.done(); - }, - - 'string tokens can be embedded while being JSONified'(test: Test) { - const stack = new Stack(); - + test('string tokens can be embedded while being JSONified', () => { for (const token of tokensThatResolveTo('woof woof')) { // GIVEN const fido = { name: 'Fido', speaks: `deep ${token}` }; @@ -32,57 +48,93 @@ nodeunitShim({ const resolved = stack.resolve(stack.toJsonString(fido)); // THEN - test.deepEqual(evaluateCFN(resolved), '{"name":"Fido","speaks":"deep woof woof"}'); + expect(evaluateCFN(resolved)).toEqual('{"name":"Fido","speaks":"deep woof woof"}'); } + }); - test.done(); - }, - - 'constant string has correct amount of quotes applied'(test: Test) { - const stack = new Stack(); - + test('constant string has correct amount of quotes applied', () => { const inputString = 'Hello, "world"'; // WHEN const resolved = stack.resolve(stack.toJsonString(inputString)); // THEN - test.deepEqual(evaluateCFN(resolved), JSON.stringify(inputString)); + expect(evaluateCFN(resolved)).toEqual(JSON.stringify(inputString)); + }); - test.done(); - }, - - 'integer Tokens behave correctly in stringification and JSONification'(test: Test) { + test('integer Tokens behave correctly in stringification and JSONification', () => { // GIVEN - const stack = new Stack(); const num = new Intrinsic(1); const embedded = `the number is ${num}`; // WHEN - test.equal(evaluateCFN(stack.resolve(embedded)), 'the number is 1'); - test.equal(evaluateCFN(stack.resolve(stack.toJsonString({ embedded }))), '{"embedded":"the number is 1"}'); - test.equal(evaluateCFN(stack.resolve(stack.toJsonString({ num }))), '{"num":1}'); + expect(evaluateCFN(stack.resolve(embedded))).toEqual('the number is 1'); + expect(evaluateCFN(stack.resolve(stack.toJsonString({ embedded })))).toEqual('{"embedded":"the number is 1"}'); + expect(evaluateCFN(stack.resolve(stack.toJsonString({ num })))).toEqual('{"num":1}'); + }); + + test('String-encoded lazies do not have quotes applied if they return objects', () => { + // This is unfortunately crazy behavior, but we have some clients already taking a + // dependency on the fact that `Lazy.stringValue({ produce: () => [...some list...] })` + // does not apply quotes but just renders the list. + + // GIVEN + const someList = Lazy.stringValue({ produce: () => [1, 2, 3] as any }); + + // WHEN + expect(evaluateCFN(stack.resolve(stack.toJsonString({ someList })))).toEqual('{"someList":[1,2,3]}'); + }); + + test('List Tokens do not have quotes applied', () => { + // GIVEN + const someList = Token.asList([1, 2, 3]); - test.done(); - }, + // WHEN + expect(evaluateCFN(stack.resolve(stack.toJsonString({ someList })))).toEqual('{"someList":[1,2,3]}'); + }); - 'tokens in strings survive additional TokenJSON.stringification()'(test: Test) { + test('tokens in strings survive additional TokenJSON.stringification()', () => { // GIVEN - const stack = new Stack(); for (const token of tokensThatResolveTo('pong!')) { // WHEN const stringified = stack.toJsonString(`ping? ${token}`); // THEN - test.equal(evaluateCFN(stack.resolve(stringified)), '"ping? pong!"'); + expect(evaluateCFN(stack.resolve(stringified))).toEqual('"ping? pong!"'); } + }); + + test('Doubly nested strings evaluate correctly in JSON context', () => { + // WHEN + const fidoSays = Lazy.stringValue({ produce: () => 'woof' }); - test.done(); - }, + // WHEN + const resolved = stack.resolve(stack.toJsonString({ + information: `Did you know that Fido says: ${fidoSays}`, + })); + + // THEN + expect(evaluateCFN(resolved)).toEqual('{"information":"Did you know that Fido says: woof"}'); + }); + + test('Quoted strings in embedded JSON context are escaped', () => { + // GIVEN + const fidoSays = Lazy.stringValue({ produce: () => '"woof"' }); + + // WHEN + const resolved = stack.resolve(stack.toJsonString({ + information: `Did you know that Fido says: ${fidoSays}`, + })); + + // THEN + expect(evaluateCFN(resolved)).toEqual('{"information":"Did you know that Fido says: \\"woof\\""}'); + }); + +}); - 'intrinsic Tokens embed correctly in JSONification'(test: Test) { +describe('tokens returning CloudFormation intrinsics', () => { + test('intrinsic Tokens embed correctly in JSONification', () => { // GIVEN - const stack = new Stack(); const bucketName = new Intrinsic({ Ref: 'MyBucket' }); // WHEN @@ -90,13 +142,10 @@ nodeunitShim({ // THEN const context = { MyBucket: 'TheName' }; - test.equal(evaluateCFN(resolved, context), '{"theBucket":"TheName"}'); + expect(evaluateCFN(resolved, context)).toEqual('{"theBucket":"TheName"}'); + }); - test.done(); - }, - - 'fake intrinsics are serialized to objects'(test: Test) { - const stack = new Stack(); + test('fake intrinsics are serialized to objects', () => { const fakeIntrinsics = new Intrinsic({ a: { 'Fn::GetArtifactAtt': { @@ -112,16 +161,13 @@ nodeunitShim({ }); const stringified = stack.toJsonString(fakeIntrinsics); - test.equal(evaluateCFN(stack.resolve(stringified)), + expect(evaluateCFN(stack.resolve(stringified))).toEqual( '{"a":{"Fn::GetArtifactAtt":{"key":"val"}},"b":{"Fn::GetParam":["val1","val2"]}}'); + }); - test.done(); - }, - - 'embedded string literals in intrinsics are escaped when calling TokenJSON.stringify()'(test: Test) { + test('embedded string literals in intrinsics are escaped when calling TokenJSON.stringify()', () => { // GIVEN - const stack = new Stack(); - const token = Fn.join('', ['Hello', 'This\nIs', 'Very "cool"']); + const token = Fn.join('', ['Hello ', Token.asString({ Ref: 'Planet' }), ', this\nIs', 'Very "cool"']); // WHEN const resolved = stack.resolve(stack.toJsonString({ @@ -130,15 +176,13 @@ nodeunitShim({ })); // THEN - const expected = '{"literal":"I can also \\"contain\\" quotes","token":"HelloThis\\nIsVery \\"cool\\""}'; - test.equal(evaluateCFN(resolved), expected); + const context = { Planet: 'World' }; + const expected = '{"literal":"I can also \\"contain\\" quotes","token":"Hello World, this\\nIsVery \\"cool\\""}'; + expect(evaluateCFN(resolved, context)).toEqual(expected); + }); - test.done(); - }, - - 'Tokens in Tokens are handled correctly'(test: Test) { + test('Tokens in Tokens are handled correctly', () => { // GIVEN - const stack = new Stack(); const bucketName = new Intrinsic({ Ref: 'MyBucket' }); const combinedName = Fn.join('', ['The bucket name is ', bucketName.toString()]); @@ -147,31 +191,12 @@ nodeunitShim({ // THEN const context = { MyBucket: 'TheName' }; - test.equal(evaluateCFN(resolved, context), '{"theBucket":"The bucket name is TheName"}'); - - test.done(); - }, - - 'Doubly nested strings evaluate correctly in JSON context'(test: Test) { - // WHEN - const stack = new Stack(); - const fidoSays = Lazy.stringValue({ produce: () => 'woof' }); - - // WHEN - const resolved = stack.resolve(stack.toJsonString({ - information: `Did you know that Fido says: ${fidoSays}`, - })); + expect(evaluateCFN(resolved, context)).toEqual('{"theBucket":"The bucket name is TheName"}'); + }); - // THEN - test.deepEqual(evaluateCFN(resolved), '{"information":"Did you know that Fido says: woof"}'); - - test.done(); - }, - - 'Doubly nested intrinsics evaluate correctly in JSON context'(test: Test) { + test('Doubly nested intrinsics evaluate correctly in JSON context', () => { // GIVEN - const stack = new Stack(); - const fidoSays = Lazy.anyValue({ produce: () => ({ Ref: 'Something' }) }); + const fidoSays = Lazy.anyValue({ produce: () => Token.asAny({ Ref: 'Something' }) }); // WHEN const resolved = stack.resolve(stack.toJsonString({ @@ -180,30 +205,11 @@ nodeunitShim({ // THEN const context = { Something: 'woof woof' }; - test.deepEqual(evaluateCFN(resolved, context), '{"information":"Did you know that Fido says: woof woof"}'); + expect(evaluateCFN(resolved, context)).toEqual('{"information":"Did you know that Fido says: woof woof"}'); + }); - test.done(); - }, - - 'Quoted strings in embedded JSON context are escaped'(test: Test) { + test('cross-stack references are also properly converted by toJsonString()', () => { // GIVEN - const stack = new Stack(); - const fidoSays = Lazy.stringValue({ produce: () => '"woof"' }); - - // WHEN - const resolved = stack.resolve(stack.toJsonString({ - information: `Did you know that Fido says: ${fidoSays}`, - })); - - // THEN - test.deepEqual(evaluateCFN(resolved), '{"information":"Did you know that Fido says: \\"woof\\""}'); - - test.done(); - }, - - 'cross-stack references are also properly converted by toJsonString()'(test: Test) { - // GIVEN - const app = new App(); const stack1 = new Stack(app, 'Stack1'); const stack2 = new Stack(app, 'Stack2'); @@ -217,7 +223,7 @@ nodeunitShim({ // THEN const asm = app.synth(); - test.deepEqual(asm.getStackByName('Stack2').template, { + expect(asm.getStackByName('Stack2').template).toEqual({ Outputs: { Stack1Id: { Value: { @@ -232,9 +238,38 @@ nodeunitShim({ }, }, }); + }); - test.done(); - }, + test('Intrinsics can occur in key position', () => { + // GIVEN + const bucketName = Token.asString({ Ref: 'MyBucket' }); + + // WHEN + const resolved = stack.resolve(stack.toJsonString({ + [bucketName]: 'Is Cool', + [`${bucketName} Is`]: 'Cool', + })); + + // THEN + const context = { MyBucket: 'Harry' }; + expect(evaluateCFN(resolved, context)).toEqual('{"Harry":"Is Cool","Harry Is":"Cool"}'); + }); + + test('toJsonString() can be used recursively', () => { + // GIVEN + const bucketName = Token.asString({ Ref: 'MyBucket' }); + + // WHEN + const embeddedJson = stack.toJsonString({ message: `the bucket name is ${bucketName}` }); + const outerJson = stack.toJsonString({ embeddedJson }); + + // THEN + const evaluatedJson = evaluateCFN(stack.resolve(outerJson), { + MyBucket: 'Bucky', + }); + expect(evaluatedJson).toEqual('{"embeddedJson":"{\\"message\\":\\"the bucket name is Bucky\\"}"}'); + expect(JSON.parse(JSON.parse(evaluatedJson).embeddedJson).message).toEqual('the bucket name is Bucky'); + }); }); /** diff --git a/packages/@aws-cdk/core/test/evaluate-cfn.ts b/packages/@aws-cdk/core/test/evaluate-cfn.ts index 7dfa66328c319..ec8c3c7baff45 100644 --- a/packages/@aws-cdk/core/test/evaluate-cfn.ts +++ b/packages/@aws-cdk/core/test/evaluate-cfn.ts @@ -5,7 +5,7 @@ */ import { isNameOfCloudFormationIntrinsic } from '../lib/private/cloudformation-lang'; -export function evaluateCFN(object: any, context: {[key: string]: string} = {}): any { +export function evaluateCFN(object: any, context: {[key: string]: any} = {}): any { const intrinsics: any = { 'Fn::Join'(separator: string, args: string[]) { return args.map(evaluate).join(separator); From d5a96db6b04ad5709eb04a42d6a4cb6a0849c9d7 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 9 Mar 2021 16:01:10 +0000 Subject: [PATCH 2/6] Change approach a bit --- .../core/lib/private/cloudformation-lang.ts | 159 +++++++++--------- packages/@aws-cdk/core/lib/private/resolve.ts | 131 +++++++++++++-- packages/@aws-cdk/core/lib/resolvable.ts | 14 +- .../core/test/cloudformation-json.test.ts | 46 ++++- 4 files changed, 253 insertions(+), 97 deletions(-) diff --git a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts index fe3f15c54f954..4ffdaf5545387 100644 --- a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts +++ b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts @@ -1,7 +1,7 @@ import { Lazy } from '../lazy'; import { DefaultTokenResolver, IFragmentConcatenator, IResolveContext } from '../resolvable'; -import { isResolvableObject, Token } from '../token'; -import { TokenMap } from './token-map'; +import { Token } from '../token'; +import { INTRINSIC_KEY_PREFIX, ResolutionTypeHint, resolvedTypeHint } from './resolve'; /** * Routines that know how to do operations at the CloudFormation document language level @@ -21,7 +21,7 @@ export class CloudFormationLang { * @param space Indentation to use (default: no pretty-printing) */ public static toJSON(obj: any, space?: number): string { - return Lazy.stringValue({ + return Lazy.uncachedString({ // We used to do this by hooking into `JSON.stringify()` by adding in objects // with custom `toJSON()` functions, but it's ultimately simpler just to // reimplement the `stringify()` function from scratch. @@ -61,51 +61,77 @@ function fnJoinConcat(parts: any[]) { /** * Perform a JSON.stringify()-like operation, except aware of Tokens and CloudFormation intrincics * - * Tokens will be resolved and if they resolve to CloudFormation intrinsics, the intrinsics - * will be lifted to the top of a giant `{ Fn::Join}` expression. + * Tokens will be resolved and if any resolve to CloudFormation intrinsics, the intrinsics + * will be lifted to the top of a giant `{ Fn::Join }` expression. * - * We are looking to do the following transforms: + * If Tokens resolve to primitive types (for example, by using Lazies), we'll + * use the primitive type to determine how to encode the value into the JSON. * - * (a) Token in a string context + * If Tokens resolve to CloudFormation intrinsics, we'll use the type of the encoded + * value as a type hint to determine how to encode the value into the JSON. The difference + * is that we add quotes (") around strings, and don't add anything around non-strings. * - * { "field": "a${TOKEN}b" } -> "{ \"field\": \"a" ++ resolve(TOKEN) ++ "b\" }" - * { "a${TOKEN}b": "value" } -> "{ \"a" ++ resolve(TOKEN) ++ "b\": \"value\" }" + * The following structure: * - * (b) Standalone token + * { SomeAttr: resource.someAttr } * - * { "field": TOKEN } -> + * Will JSONify to either: * - * if TOKEN resolves to a string (or is a non-encoded or string-encoded intrinsic) -> - * "{ \"field\": \"" ++ resolve(TOKEN) ++ "\" }" - * if TOKEN resolves to a non-string (or is a non-string-encoded intrinsic) -> - * "{ \"field\": " ++ resolve(TOKEN) ++ " }" + * '{ "SomeAttr": "' ++ { Fn::GetAtt: [Resource, SomeAttr] } ++ '" }' + * or '{ "SomeAttr": ' ++ { Fn::GetAtt: [Resource, SomeAttr] } ++ ' }' + * + * Depending on whether `someAttr` is type-hinted to be a string or not. * * (Where ++ is the CloudFormation string-concat operation (`{ Fn::Join }`). * - * ------------------- + * ----------------------- + * + * This work requires 2 features from the `resolve()` function: + * + * - INTRINSICS TYPE HINTS: intrinsics only have values like `{ Ref: 'XYZ' }`. This value can reference either + * a string or a list/number at deploy time, and from the value alone there's no way to know which. The difference + * will be whether we JSONify this to: + * + * '{ "referencedValue": "' ++ { Ref: XYZ } ++ '"}' + * or '{ "referencedValue": ' ++ { Ref: XYZ } ++ '}' + * + * I.e., whether or not we need to enclose the reference in quotes or not. + * + * We COULD have done this by resolving one token at a time, and looking at the + * type of the encoded token we were resolving to obtain a type hint. However, + * the `resolve()` and Token system resists a level-at-a-time resolve + * operation; because of the existence of post-processors, we must have done a + * complete recursive resolution of a token before we can look at its result + * (after which any type information about the sources of nested resolved + * values is lost). * - * Here come complex type interpretation rules, which we are unable to simplify because - * some clients our there are already taking dependencies on the unintended side effects - * of the old implementation. + * To fix this, "type hints" have been added to the `resolve()` function, + * giving an idea of the type of the source value for compplex result values. + * This only works for complex values (not strings and numbers) but fortunately + * we only care about the types of intrinsics, which are always complex values. * - * 1. If TOKEN is embedded in a string with a prefix or postfix, we'll render the token - * as a string regardless of whether it returns an intrinsic or a literal. + * Type Hints could have been added to `IResolvable` as well, but for now we just + * use the type of an encoded value as a type hint. * - * 2. If TOKEN resolves to an intrinsic: - * - We'll treat it as a string if the TOKEN itself was not encoded or string-encoded - * (this covers the 99% case of what CloudFormation intrinsics resolve to). - * - We'll treat it as a non-string otherwise; the intrinsic MUST resolve to a number; - * * if resolves to a list { Fn::Join } will fail - * * if it resolves to a string after all the JSON will be malformed at API call time. + * - COMPLEX KEYS: contrary to regular JavaScript objects, it is feasible in a JSON string to use an intrinsic in + * the key position of an object. * - * 3. Otherwise, the type of the value it resolves to (string, number, complex object, ...) - * determines how the value is rendered. + * The simplest implementation of `tokenAwareStringify` would do a complete `resolve()` of the data structure, and + * afterwards encode the resolved object to a string, taking care to properly encode intrinsics. + * + * However, the intermediary representation (the result of `resolve()`) could not hold complex key values in memory + * in a JavaScript object. We therefore extend the intermediary representation to be able to store these, deviating + * from straight JavaScript object semantics somewhat by using special key prefixes. */ function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) { let indent = 0; const ret = new Array(); - recurse(root); + + // First completely resolve the tree, then encode to JSON while respecting the type + // hints we got for the resolved intrinsics. + recurse(ctx.resolve(root, { allowIntrinsicKeys: true })); + switch (ret.length) { case 0: return ''; case 1: return renderSegment(ret[0]); @@ -118,13 +144,24 @@ function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) { */ function recurse(obj: any): void { if (Token.isUnresolved(obj)) { - return handleToken(obj); + throw new Error('This shouldnt happen anymore'); } if (Array.isArray(obj)) { return renderCollection('[', ']', obj, recurse); } if (typeof obj === 'object' && obj != null && !(obj instanceof Date)) { + // Treat as an intrinsic if this LOOKS like a CFN intrinsic (`{ Ref: ... }`) + // AND it's the result of a token resolution. Otherwise, we just treat this + // value as a regular old JSON object (that happens to look a lot like an intrinsic). + if (isIntrinsic(obj) && resolvedTypeHint(obj)) { + return renderIntrinsic(obj); + } + return renderCollection('{', '}', definedEntries(obj), ([key, value]) => { + if (key.startsWith(INTRINSIC_KEY_PREFIX)) { + [key, value] = value; + } + recurse(key); pushLiteral(prettyPunctuation(':')); recurse(value); @@ -152,57 +189,18 @@ function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) { pushLiteral(post); } - /** - * Handle a Token. - * - * Can be any of: - * - * - Straight up IResolvable - * - Encoded string, number or list - */ - function handleToken(token: any) { - if (typeof token === 'string') { - // Encoded string, treat like a string if it has a token and at least one other - // component, otherwise treat like a regular token and base the output quoting on the - // type of the result. - const fragments = TokenMap.instance().splitString(token); - if (fragments.length > 1) { + function renderIntrinsic(intrinsic: any) { + switch (resolvedTypeHint(intrinsic)) { + case ResolutionTypeHint.STRING: pushLiteral('"'); - fragments.visit({ - visitLiteral: pushLiteral, - visitToken: (tok) => { - const resolved = ctx.resolve(tok); - if (isIntrinsic(resolved)) { - pushIntrinsic(quoteInsideIntrinsic(resolved)); - } else { - // We're already in a string context, so stringify and escape - pushLiteral(quoteString(`${resolved}`)); - } - }, - // This potential case is the result of poor modeling in the tokenized string, it should not happen - visitIntrinsic: () => { throw new Error('Intrinsic not expected in a freshly-split string'); }, - }); + pushIntrinsic(quoteInsideIntrinsic(intrinsic)); pushLiteral('"'); - return; - } - } + break; - const resolved = ctx.resolve(token); - if (isIntrinsic(resolved)) { - if (isResolvableObject(token) || typeof token === 'string') { - // If the input was an unencoded IResolvable or a string-encoded value, - // treat it like it was a string (for the 99% case) - pushLiteral('"'); - pushIntrinsic(quoteInsideIntrinsic(resolved)); - pushLiteral('"'); - } else { - pushIntrinsic(resolved); - } - return; + default: + pushIntrinsic(intrinsic); + break; } - - // Otherwise we got an arbitrary JSON structure from the token, recurse - recurse(resolved); } /** @@ -348,6 +346,11 @@ function* definedEntries(xs: A): IterableIterator<[string, any /** * Quote string literals inside an intrinsic + * + * Formally, this should only match string literals that will be interpreted as + * string literals. Fortunately, the strings that should NOT be quoted are + * Logical IDs and attribute names, which cannot contain quotes anyway. Hence, + * we can get away not caring about the distinction and just quoting everything. */ function quoteInsideIntrinsic(x: any): any { if (typeof x === 'object' && x != null && Object.keys(x).length === 1) { diff --git a/packages/@aws-cdk/core/lib/private/resolve.ts b/packages/@aws-cdk/core/lib/private/resolve.ts index d6ae73cdb8796..75d063eb74aa7 100644 --- a/packages/@aws-cdk/core/lib/private/resolve.ts +++ b/packages/@aws-cdk/core/lib/private/resolve.ts @@ -1,5 +1,5 @@ import { IConstruct } from 'constructs'; -import { DefaultTokenResolver, IPostProcessor, IResolvable, IResolveContext, ITokenResolver, StringConcat } from '../resolvable'; +import { DefaultTokenResolver, IPostProcessor, IResolvable, IResolveContext, ITokenResolver, ResolveChangeContextOptions, StringConcat } from '../resolvable'; import { TokenizedStringFragments } from '../string-fragments'; import { containsListTokenElement, TokenString, unresolved } from './encoding'; import { TokenMap } from './token-map'; @@ -9,9 +9,32 @@ import { TokenMap } from './token-map'; import { IConstruct as ICoreConstruct } from '../construct-compat'; // This file should not be exported to consumers, resolving should happen through Construct.resolve() - const tokenMap = TokenMap.instance(); +/** + * Resolved complex values will have a type hint applied. + * + * The type hint will be based on the type of the input value that was resolved. + * + * If the value was encoded, the type hint will be the type of the encoded value. In case + * of a plain `IResolvable`, a type hint of 'string' will be assumed. + */ +const RESOLUTION_TYPEHINT_SYM = Symbol.for('@aws-cdk/core.resolvedTypeHint'); + +/** + * Prefix used for intrinsic keys + */ +export const INTRINSIC_KEY_PREFIX = '$IntrinsicKey$'; + +/** + * Type hints for resolved values + */ +export enum ResolutionTypeHint { + STRING = 'string', + NUMBER = 'number', + LIST = 'list', +} + /** * Options to the resolve() operation * @@ -25,6 +48,36 @@ export interface IResolveOptions { preparing: boolean; resolver: ITokenResolver; prefix?: string[]; + + /** + * Whether or not to allow intrinsics in keys of an object + * + * Because keys of an object must be strings, a (resolved) intrinsic, which + * is an object, cannot be stored in that position. By default, we reject these + * intrinsics if we encounter them. + * + * If this is set to `true`, in order to store the complex value in a map, + * keys that happen to evaluate to intrinsics will be added with a unique key + * identified by an uncomming prefix, mapped to a tuple that represents the + * actual key/value-pair. The map will look like this: + * + * { + * '$IntrinsicKey$0': [ { Ref: ... }, 'value1' ], + * '$IntrinsicKey$1': [ { Ref: ... }, 'value2' ], + * 'regularKey': 'value3', + * ... + * } + * + * Callers should only set this option to `true` if they are prepared to deal with + * the object in this weird shape, and massage it back into a correct object afterwards. + * + * (A regular but uncommon string was chosen over something like symbols or + * other ways of tagging the extra values in order to simplify the implementation which + * maintains the desired behavior `resolve(resolve(x)) == resolve(x)`). + * + * @default false + */ + allowIntrinsicKeys?: boolean; } /** @@ -50,7 +103,7 @@ export function resolve(obj: any, options: IResolveOptions): any { preparing: options.preparing, scope: options.scope as ICoreConstruct, registerPostProcessor(pp) { postProcessor = pp; }, - resolve(x: any) { return resolve(x, { ...options, prefix: newPrefix }); }, + resolve(x: any, changeOptions?: ResolveChangeContextOptions) { return resolve(x, { ...options, ...changeOptions, prefix: newPrefix }); }, }; return [context, { postProcess(x) { return postProcessor ? postProcessor.postProcess(x, context) : x; } }]; @@ -98,7 +151,7 @@ export function resolve(obj: any, options: IResolveOptions): any { const str = TokenString.forString(obj); if (str.test()) { const fragments = str.split(tokenMap.lookupToken.bind(tokenMap)); - return options.resolver.resolveString(fragments, makeContext()[0]); + return tagResolvedValue(options.resolver.resolveString(fragments, makeContext()[0]), ResolutionTypeHint.STRING); } return obj; } @@ -107,7 +160,7 @@ export function resolve(obj: any, options: IResolveOptions): any { // number - potentially decode Tokenized number // if (typeof(obj) === 'number') { - return resolveNumberToken(obj, makeContext()[0]); + return tagResolvedValue(resolveNumberToken(obj, makeContext()[0]), ResolutionTypeHint.NUMBER); } // @@ -124,7 +177,7 @@ export function resolve(obj: any, options: IResolveOptions): any { if (Array.isArray(obj)) { if (containsListTokenElement(obj)) { - return options.resolver.resolveList(obj, makeContext()[0]); + return tagResolvedValue(options.resolver.resolveList(obj, makeContext()[0]), ResolutionTypeHint.LIST); } const arr = obj @@ -140,7 +193,8 @@ export function resolve(obj: any, options: IResolveOptions): any { if (unresolved(obj)) { const [context, postProcessor] = makeContext(); - return options.resolver.resolveToken(obj, context, postProcessor); + const ret = tagResolvedValue(options.resolver.resolveToken(obj, context, postProcessor), ResolutionTypeHint.STRING); + return ret; } // @@ -155,24 +209,40 @@ export function resolve(obj: any, options: IResolveOptions): any { } const result: any = { }; + let intrinsicKeyCtr = 0; for (const key of Object.keys(obj)) { - const resolvedKey = makeContext()[0].resolve(key); - if (typeof(resolvedKey) !== 'string') { - // eslint-disable-next-line max-len - throw new Error(`"${key}" is used as the key in a map so must resolve to a string, but it resolves to: ${JSON.stringify(resolvedKey)}. Consider using "CfnJson" to delay resolution to deployment-time`); - } - - const value = makeContext(key)[0].resolve(obj[key]); + const value = makeContext(String(key))[0].resolve(obj[key]); // skip undefined if (typeof(value) === 'undefined') { continue; } - result[resolvedKey] = value; + // Simple case -- not an unresolved key + if (!unresolved(key)) { + result[key] = value; + continue; + } + + const resolvedKey = makeContext()[0].resolve(key); + if (typeof(resolvedKey) === 'string') { + result[resolvedKey] = value; + } else { + if (!options.allowIntrinsicKeys) { + // eslint-disable-next-line max-len + throw new Error(`"${String(key)}" is used as the key in a map so must resolve to a string, but it resolves to: ${JSON.stringify(resolvedKey)}. Consider using "CfnJson" to delay resolution to deployment-time`); + } + + // Can't represent this object in a JavaScript key position, but we can store it + // in value position. Use a unique symbol as the key. + result[`${INTRINSIC_KEY_PREFIX}${intrinsicKeyCtr++}`] = [resolvedKey, value]; + } } - return result; + // Because we may be called to recurse on already resolved values (that already have type hints applied) + // and we just copied those values into a fresh object, be sure to retain any type hints. + const previousTypeHint = resolvedTypeHint(obj); + return previousTypeHint ? tagResolvedValue(result, previousTypeHint) : result; } /** @@ -222,3 +292,32 @@ function resolveNumberToken(x: number, context: IResolveContext): any { if (token === undefined) { return x; } return context.resolve(token); } + +/** + * Apply a type hint to a resolved value + * + * The type hint will only be applied to objects. + * + * These type hints are used for correct JSON-ification of intrinsic values. + */ +function tagResolvedValue(value: any, typeHint: ResolutionTypeHint): any { + if (typeof value !== 'object' || value == null) { return value; } + Object.defineProperty(value, RESOLUTION_TYPEHINT_SYM, { + value: typeHint, + configurable: true, + }); + return value; +} + +/** + * Return the type hint from the given value + * + * If the value is not a resolved value (i.e, the result of resolving a token), + * `undefined` will be returned. + * + * These type hints are used for correct JSON-ification of intrinsic values. + */ +export function resolvedTypeHint(value: any): ResolutionTypeHint | undefined { + if (typeof value !== 'object' || value == null) { return undefined; } + return value[RESOLUTION_TYPEHINT_SYM]; +} \ No newline at end of file diff --git a/packages/@aws-cdk/core/lib/resolvable.ts b/packages/@aws-cdk/core/lib/resolvable.ts index 2ddbd544ffbbb..9004cd111bb33 100644 --- a/packages/@aws-cdk/core/lib/resolvable.ts +++ b/packages/@aws-cdk/core/lib/resolvable.ts @@ -20,7 +20,7 @@ export interface IResolveContext { /** * Resolve an inner object */ - resolve(x: any): any; + resolve(x: any, options?: ResolveChangeContextOptions): any; /** * Use this postprocessor after the entire token structure has been resolved @@ -28,6 +28,18 @@ export interface IResolveContext { registerPostProcessor(postProcessor: IPostProcessor): void; } +/** + * Options that can be changed while doing a recursive resolve + */ +export interface ResolveChangeContextOptions { + /** + * Change the 'allowIntrinsicKeys' option + * + * @default - Unchanged + */ + readonly allowIntrinsicKeys?: boolean; +} + /** * Interface for values that can be resolvable later * diff --git a/packages/@aws-cdk/core/test/cloudformation-json.test.ts b/packages/@aws-cdk/core/test/cloudformation-json.test.ts index 7ad4c2554098f..61d65fb5f178d 100644 --- a/packages/@aws-cdk/core/test/cloudformation-json.test.ts +++ b/packages/@aws-cdk/core/test/cloudformation-json.test.ts @@ -1,4 +1,4 @@ -import { App, CfnOutput, Fn, Lazy, Stack, Token } from '../lib'; +import { App, CfnOutput, Fn, IPostProcessor, IResolvable, IResolveContext, Lazy, Stack, Token } from '../lib'; import { Intrinsic } from '../lib/private/intrinsic'; import { evaluateCFN } from './evaluate-cfn'; @@ -85,7 +85,7 @@ describe('tokens that return literals', () => { expect(evaluateCFN(stack.resolve(stack.toJsonString({ someList })))).toEqual('{"someList":[1,2,3]}'); }); - test('List Tokens do not have quotes applied', () => { + test('Literal-resolving List Tokens do not have quotes applied', () => { // GIVEN const someList = Token.asList([1, 2, 3]); @@ -93,6 +93,17 @@ describe('tokens that return literals', () => { expect(evaluateCFN(stack.resolve(stack.toJsonString({ someList })))).toEqual('{"someList":[1,2,3]}'); }); + test('Intrinsic-resolving List Tokens do not have quotes applied', () => { + // GIVEN + const someList = Token.asList(new Intrinsic({ Ref: 'Thing' })); + + // WHEN + expect(stack.resolve(stack.toJsonString({ someList }))).toEqual({ + 'Fn::Join': ['', ['{"someList":', { Ref: 'Thing' }, '}']], + }); + }); + + test('tokens in strings survive additional TokenJSON.stringification()', () => { // GIVEN for (const token of tokensThatResolveTo('pong!')) { @@ -194,6 +205,20 @@ describe('tokens returning CloudFormation intrinsics', () => { expect(evaluateCFN(resolved, context)).toEqual('{"theBucket":"The bucket name is TheName"}'); }); + test('Intrinsics in postprocessors are handled correctly', () => { + // GIVEN + const bucketName = new Intrinsic({ Ref: 'MyBucket' }); + const combinedName = new DummyPostProcessor(['this', 'is', bucketName]); + + // WHEN + const resolved = stack.resolve(stack.toJsonString({ theBucket: combinedName })); + + // THEN + expect(resolved).toEqual({ + 'Fn::Join': ['', ['{"theBucket":["this","is","', { Ref: 'MyBucket' }, '"]}']], + }); + }); + test('Doubly nested strings evaluate correctly in JSON context', () => { // WHEN const fidoSays = Lazy.string({ produce: () => 'woof' }); @@ -324,3 +349,20 @@ function tokensThatResolveTo(value: any): Token[] { Lazy.any({ produce: () => value }), ]; } + +class DummyPostProcessor implements IResolvable, IPostProcessor { + public readonly creationStack: string[]; + + constructor(private readonly value: any) { + this.creationStack = ['test']; + } + + public resolve(context: IResolveContext) { + context.registerPostProcessor(this); + return context.resolve(this.value); + } + + public postProcess(o: any, _context: IResolveContext): any { + return o; + } +} \ No newline at end of file From c483b973e5b04f1248e2d556ea19e330a8aa8958 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 9 Mar 2021 16:32:24 +0000 Subject: [PATCH 3/6] Empty value returns undefined --- packages/@aws-cdk/core/lib/private/cloudformation-lang.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts index 4ffdaf5545387..6a9c080a5dd6e 100644 --- a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts +++ b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts @@ -133,7 +133,7 @@ function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) { recurse(ctx.resolve(root, { allowIntrinsicKeys: true })); switch (ret.length) { - case 0: return ''; + case 0: return undefined; case 1: return renderSegment(ret[0]); default: return fnJoinConcat(ret.map(renderSegment)); From f98881221d6b938501f10ebea86355da0addbc72 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 9 Mar 2021 17:12:46 +0000 Subject: [PATCH 4/6] Remove dead code, add better explanations, fix undefined case --- .../core/lib/private/cloudformation-lang.ts | 46 +++++++++++++------ packages/@aws-cdk/core/lib/private/resolve.ts | 6 +++ .../@aws-cdk/core/lib/string-fragments.ts | 41 ----------------- .../core/test/cloudformation-json.test.ts | 4 ++ 4 files changed, 41 insertions(+), 56 deletions(-) diff --git a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts index 6a9c080a5dd6e..ebd08ef149a79 100644 --- a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts +++ b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts @@ -88,9 +88,10 @@ function fnJoinConcat(parts: any[]) { * * This work requires 2 features from the `resolve()` function: * - * - INTRINSICS TYPE HINTS: intrinsics only have values like `{ Ref: 'XYZ' }`. This value can reference either - * a string or a list/number at deploy time, and from the value alone there's no way to know which. The difference - * will be whether we JSONify this to: + * - INTRINSICS TYPE HINTS: intrinsics are represented by values like + * `{ Ref: 'XYZ' }`. These values can reference either a string or a list/number at + * deploy time, and from the value alone there's no way to know which. We need + * to know the type to know whether to JSONify this reference to: * * '{ "referencedValue": "' ++ { Ref: XYZ } ++ '"}' * or '{ "referencedValue": ' ++ { Ref: XYZ } ++ '}' @@ -99,29 +100,42 @@ function fnJoinConcat(parts: any[]) { * * We COULD have done this by resolving one token at a time, and looking at the * type of the encoded token we were resolving to obtain a type hint. However, - * the `resolve()` and Token system resists a level-at-a-time resolve - * operation; because of the existence of post-processors, we must have done a + * the `resolve()` and Token system resist a level-at-a-time resolve + * operation: because of the existence of post-processors, we must have done a * complete recursive resolution of a token before we can look at its result * (after which any type information about the sources of nested resolved * values is lost). * * To fix this, "type hints" have been added to the `resolve()` function, * giving an idea of the type of the source value for compplex result values. - * This only works for complex values (not strings and numbers) but fortunately + * This only works for objects (not strings and numbers) but fortunately * we only care about the types of intrinsics, which are always complex values. * - * Type Hints could have been added to `IResolvable` as well, but for now we just - * use the type of an encoded value as a type hint. + * Type hinting could have been added to the `IResolvable` protocol as well, + * but for now we just use the type of an encoded value as a type hint. That way + * we don't need to annotate anything more at the L1 level--we will use the type + * encodings added by construct authors at the L2 levels. L1 users can escape the + * default decision of "string" by using `Token.asList()`. * - * - COMPLEX KEYS: contrary to regular JavaScript objects, it is feasible in a JSON string to use an intrinsic in - * the key position of an object. + * - COMPLEX KEYS: since tokens can be string-encoded, we can use string-encoded tokens + * as the keys in JavaScript objects. However, after resolution, those string-encoded + * tokens could resolve to intrinsics (`{ Ref: ... }`), which CANNOT be stored in + * JavaScript objects anymore. * - * The simplest implementation of `tokenAwareStringify` would do a complete `resolve()` of the data structure, and - * afterwards encode the resolved object to a string, taking care to properly encode intrinsics. + * We therefore need a protocol to store the resolved values somewhere in the JavaScript + * type model, which can be returned by `resolve()`, and interpreted by `tokenAwareStringify()` + * to produce the correct JSON. * - * However, the intermediary representation (the result of `resolve()`) could not hold complex key values in memory - * in a JavaScript object. We therefore extend the intermediary representation to be able to store these, deviating - * from straight JavaScript object semantics somewhat by using special key prefixes. + * And example will quickly show the point: + * + * User writes: + * { [resource.resourceName]: 'SomeValue' } + * ------ string actually looks like ------> + * { '${Token[1234]}': 'SomeValue' } + * ------ resolve -------> + * { '$IntrinsicKey$0': [ {Ref: Resource}, 'SomeValue' ] } + * ------ tokenAwareStringify -------> + * '{ "' ++ { Ref: Resource } ++ '": "SomeValue" }' */ function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) { let indent = 0; @@ -143,6 +157,8 @@ function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) { * Stringify a JSON element */ function recurse(obj: any): void { + if (obj == null) { return; } + if (Token.isUnresolved(obj)) { throw new Error('This shouldnt happen anymore'); } diff --git a/packages/@aws-cdk/core/lib/private/resolve.ts b/packages/@aws-cdk/core/lib/private/resolve.ts index 75d063eb74aa7..5f9620ecb759c 100644 --- a/packages/@aws-cdk/core/lib/private/resolve.ts +++ b/packages/@aws-cdk/core/lib/private/resolve.ts @@ -23,6 +23,12 @@ const RESOLUTION_TYPEHINT_SYM = Symbol.for('@aws-cdk/core.resolvedTypeHint'); /** * Prefix used for intrinsic keys + * + * If a key with this prefix is found in an object, the actual value of the + * key doesn't matter. The value of this key will be an `[ actualKey, actualValue ]` + * tuple, and the `actualKey` will be a value which otherwise couldn't be represented + * in the types of `string | number | symbol`, which are the only possible JavaScript + * object keys. */ export const INTRINSIC_KEY_PREFIX = '$IntrinsicKey$'; diff --git a/packages/@aws-cdk/core/lib/string-fragments.ts b/packages/@aws-cdk/core/lib/string-fragments.ts index 81606bf7b4c8d..b92fd3628a28d 100644 --- a/packages/@aws-cdk/core/lib/string-fragments.ts +++ b/packages/@aws-cdk/core/lib/string-fragments.ts @@ -84,25 +84,6 @@ export class TokenizedStringFragments { return ret; } - /** - * Visit all fragments of the string - */ - public visit(visitor: ITokenVisitor) { - for (const f of this.fragments) { - switch (f.type) { - case 'literal': - visitor.visitLiteral(f.lit); - break; - case 'token': - visitor.visitToken(f.token); - break; - case 'intrinsic': - visitor.visitIntrinsic(f.value); - break; - } - } - } - /** * Combine the string fragments using the given joiner. * @@ -135,28 +116,6 @@ export interface ITokenMapper { mapToken(t: IResolvable): any; } -/** - * Interface to visit parts of an encoded token string - * - * Interface so it can be exported via jsii. - */ -export interface ITokenVisitor { - /** - * Visit a literal - */ - visitLiteral(lit: string): void; - - /** - * Visit a token - */ - visitToken(tok: IResolvable): void; - - /** - * Visit an intrinsic - */ - visitIntrinsic(intrinsic: any): void; -} - /** * Resolve the value from a single fragment * diff --git a/packages/@aws-cdk/core/test/cloudformation-json.test.ts b/packages/@aws-cdk/core/test/cloudformation-json.test.ts index 61d65fb5f178d..c56b065fde8c3 100644 --- a/packages/@aws-cdk/core/test/cloudformation-json.test.ts +++ b/packages/@aws-cdk/core/test/cloudformation-json.test.ts @@ -24,6 +24,10 @@ test('JSONification of literals looks like JSON.stringify', () => { expect(stack.resolve(stack.toJsonString(structure, 2))).toEqual(JSON.stringify(structure, undefined, 2)); }); +test('JSONification of undefined leads to undefined', () => { + expect(stack.resolve(stack.toJsonString(undefined))).toEqual(undefined); +}); + describe('tokens that return literals', () => { test('string tokens can be JSONified and JSONification can be reversed', () => { From a5275a3f887b54dff634ac0aa17455401a28d755 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 9 Mar 2021 18:31:33 +0000 Subject: [PATCH 5/6] undefined is not the same as null --- packages/@aws-cdk/core/lib/private/cloudformation-lang.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts index ebd08ef149a79..0a6fe0ce70360 100644 --- a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts +++ b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts @@ -157,7 +157,7 @@ function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) { * Stringify a JSON element */ function recurse(obj: any): void { - if (obj == null) { return; } + if (obj === undefined) { return; } if (Token.isUnresolved(obj)) { throw new Error('This shouldnt happen anymore'); From 80731c1d84f2f8a790dfe21a39b34d453b09b7a8 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 10 Mar 2021 10:44:25 +0000 Subject: [PATCH 6/6] Add some more tests around quoting, fix bugs --- .../core/lib/private/cloudformation-lang.ts | 26 ++++------ .../core/test/cloudformation-json.test.ts | 51 ++++++++++++++++++- packages/@aws-cdk/core/test/evaluate-cfn.ts | 14 ++--- 3 files changed, 63 insertions(+), 28 deletions(-) diff --git a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts index 0a6fe0ce70360..310a4632f4e8f 100644 --- a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts +++ b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts @@ -209,7 +209,7 @@ function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) { switch (resolvedTypeHint(intrinsic)) { case ResolutionTypeHint.STRING: pushLiteral('"'); - pushIntrinsic(quoteInsideIntrinsic(intrinsic)); + pushIntrinsic(deepQuoteStringLiterals(intrinsic)); pushLiteral('"'); break; @@ -368,22 +368,16 @@ function* definedEntries(xs: A): IterableIterator<[string, any * Logical IDs and attribute names, which cannot contain quotes anyway. Hence, * we can get away not caring about the distinction and just quoting everything. */ -function quoteInsideIntrinsic(x: any): any { - if (typeof x === 'object' && x != null && Object.keys(x).length === 1) { - const key = Object.keys(x)[0]; - const params = x[key]; - switch (key) { - case 'Fn::If': - return { 'Fn::If': [params[0], quoteInsideIntrinsic(params[1]), quoteInsideIntrinsic(params[2])] }; - case 'Fn::Join': - return { 'Fn::Join': [quoteInsideIntrinsic(params[0]), params[1].map(quoteInsideIntrinsic)] }; - case 'Fn::Sub': - if (Array.isArray(params)) { - return { 'Fn::Sub': [quoteInsideIntrinsic(params[0]), params[1]] }; - } else { - return { 'Fn::Sub': quoteInsideIntrinsic(params[0]) }; - } +function deepQuoteStringLiterals(x: any): any { + if (Array.isArray(x)) { + return x.map(deepQuoteStringLiterals); + } + if (typeof x === 'object' && x != null) { + const ret: any = {}; + for (const [key, value] of Object.entries(x)) { + ret[deepQuoteStringLiterals(key)] = deepQuoteStringLiterals(value); } + return ret; } if (typeof x === 'string') { return quoteString(x); diff --git a/packages/@aws-cdk/core/test/cloudformation-json.test.ts b/packages/@aws-cdk/core/test/cloudformation-json.test.ts index c56b065fde8c3..cb96020e04904 100644 --- a/packages/@aws-cdk/core/test/cloudformation-json.test.ts +++ b/packages/@aws-cdk/core/test/cloudformation-json.test.ts @@ -1,4 +1,4 @@ -import { App, CfnOutput, Fn, IPostProcessor, IResolvable, IResolveContext, Lazy, Stack, Token } from '../lib'; +import { App, Aws, CfnOutput, Fn, IPostProcessor, IResolvable, IResolveContext, Lazy, Stack, Token } from '../lib'; import { Intrinsic } from '../lib/private/intrinsic'; import { evaluateCFN } from './evaluate-cfn'; @@ -196,6 +196,35 @@ describe('tokens returning CloudFormation intrinsics', () => { expect(evaluateCFN(resolved, context)).toEqual(expected); }); + test('embedded string literals are escaped in Fn.sub (implicit references)', () => { + // GIVEN + const token = Fn.sub('I am in account "${AWS::AccountId}"'); + + // WHEN + const resolved = stack.resolve(stack.toJsonString({ token })); + + // THEN + const context = { 'AWS::AccountId': '1234' }; + const expected = '{"token":"I am in account \\"1234\\""}'; + expect(evaluateCFN(resolved, context)).toEqual(expected); + }); + + test('embedded string literals are escaped in Fn.sub (explicit references)', () => { + // GIVEN + const token = Fn.sub('I am in account "${Acct}", also wanted to say: ${Also}', { + Acct: Aws.ACCOUNT_ID, + Also: '"hello world"', + }); + + // WHEN + const resolved = stack.resolve(stack.toJsonString({ token })); + + // THEN + const context = { 'AWS::AccountId': '1234' }; + const expected = '{"token":"I am in account \\"1234\\", also wanted to say: \\"hello world\\""}'; + expect(evaluateCFN(resolved, context)).toEqual(expected); + }); + test('Tokens in Tokens are handled correctly', () => { // GIVEN const bucketName = new Intrinsic({ Ref: 'MyBucket' }); @@ -344,6 +373,26 @@ describe('tokens returning CloudFormation intrinsics', () => { }); }); +test('JSON strings nested inside JSON strings have correct quoting', () => { + // GIVEN + const payload = stack.toJsonString({ + message: Fn.sub('I am in account "${AWS::AccountId}"'), + }); + + // WHEN + const resolved = stack.resolve(stack.toJsonString({ payload })); + + // THEN + const context = { 'AWS::AccountId': '1234' }; + const expected = '{"payload":"{\\"message\\":\\"I am in account \\\\\\"1234\\\\\\"\\"}"}'; + const evaluated = evaluateCFN(resolved, context); + expect(evaluated).toEqual(expected); + + // Is this even correct? Let's ask JavaScript because I have trouble reading this many backslashes. + expect(JSON.parse(JSON.parse(evaluated).payload).message).toEqual('I am in account "1234"'); +}); + + /** * Return two Tokens, one of which evaluates to a Token directly, one which evaluates to it lazily */ diff --git a/packages/@aws-cdk/core/test/evaluate-cfn.ts b/packages/@aws-cdk/core/test/evaluate-cfn.ts index 6d60949cc3193..af07209c7e5a7 100644 --- a/packages/@aws-cdk/core/test/evaluate-cfn.ts +++ b/packages/@aws-cdk/core/test/evaluate-cfn.ts @@ -42,16 +42,8 @@ export function evaluateCFN(object: any, context: {[key: string]: string} = {}): return context[key]; }, - 'Fn::Sub'(argument: string | [string, Record]) { - let template; - let placeholders: Record; - if (Array.isArray(argument)) { - template = argument[0]; - placeholders = evaluate(argument[1]); - } else { - template = argument; - placeholders = context; - } + 'Fn::Sub'(template: string, explicitPlaceholders?: Record) { + const placeholders = explicitPlaceholders ? evaluate(explicitPlaceholders) : context; if (typeof template !== 'string') { throw new Error('The first argument to {Fn::Sub} must be a string literal (cannot be the result of an expression)'); @@ -79,7 +71,7 @@ export function evaluateCFN(object: any, context: {[key: string]: string} = {}): const ret: {[key: string]: any} = {}; for (const key of Object.keys(obj)) { - ret[key] = evaluateCFN(obj[key]); + ret[key] = evaluate(obj[key]); } return ret; }