Framework: Tokens can be converted to strings #518

rix0rrr · 2018-08-07T13:01:15Z

Tokens (such as resource attributes) can now be implicitly converted
into strings. Parameters and attributes can now be combined into larger
strings using the native string facilities of the host language.

The process will be reversed by the resolve() function during synthesis,
which will split the string up again into string literals and
CloudFormation intrinsics.

The same transformation is applied during JSON.stringify(), so that
Tokens in a complex JSON structure are preserved.

Fixes #24 and #168.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Tokens (such as resource attributes) can now be implicitly converted into strings. Parameters and attributes can now be combined into larger strings using the native string facilities of the host language. The process will be reversed by the resolve() function during synthesis, which will split the string up again into string literals and CloudFormation intrinsics. The same transformation is applied during JSON.stringify(), so that Tokens in a complex JSON structure are preserved. Fixes #24 and #168.

rix0rrr · 2018-08-07T13:03:25Z

I'm not 100% happy with the current implementation yet, as the resolve() function now needs to have knowledge of the FnConcat primitive, and is restricted to CloudFormation.

However, barring a more complicated rework of Token to make it aware of the context its being used in, I don't see a good solution to this. Right now, Tokens are free-floating and not attached to the construct tree in any way.

eladb · 2018-08-07T13:11:36Z

packages/@aws-cdk/cdk/lib/cloudformation/token-aware-jsonify.ts

@@ -1,112 +0,0 @@
-import { istoken, resolve, Token } from '../core/tokens';


Hell yeah! This whole thing was worth just to get rid of this function!

eladb · 2018-08-07T13:13:03Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ * returns a string representation for the Token pre-resolution, NOT the
+ * resolved value of the Token.
+ */
+ public toJSON(): any {


Why is this needed?

This gets called as custom serialization in JSON.stringify().

Wow! That's cool. Good to know.

So now I am thinking if we can simplify resolve by simply saying that tokens are objects that implement toJSON instead of resolve, but then why are you returning the string representation here....

JSON.stringify() must always return a string.

Structural reason: if we returned a CloudFormation intrinsic here, it might work if our Token is the top-level object, but it might also be 3 levels deep... but we still want to wrap the WHOLE return value in a FnConcat. That cannot be done in the toJSON, because that only has a local view of what it's producing. So we must first return a string that nicely embeds into the larger JSONified string, and only later split that string up again and wrap it in a FnConcat at a later point when we're calling resolve() on the entire string.

And also the simple reason: API doesn't allow it. Whatever toJSON returns is stringified according to the normal stringification rules. So if we returned an object here, we would end up with "{ Fn::GetAtt: [...] }" inside our JSON string, not in a position where CloudFormation would evaluate it.

Yeah, got it. The name is quite confusing :-)

eladb · 2018-08-07T13:14:26Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ * The string representation is used to embed token into strings,
+ * and stored to be able to
+ *
+ * All instances of TokenStringMap share the same storage, so that this process


eladb · 2018-08-07T13:20:01Z

packages/@aws-cdk/custom-resources/lib/resource.d.ts

+ * object with the properties your provider accepts.
+ */
+export declare class CustomResource extends cloudformation.CustomResource {
+ private readonly userProperties?;


Are these changes supposed to be here?

eladb · 2018-08-07T13:27:18Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ * sense to humans.
+ */
+export class HintedToken extends Token {
+ constructor(public readonly representationHint: string, valueOrFunction?: any) {


Maybe just add an optional 2nd argument to the Token ctor and call it displayName or something like that, so this capability will be more discoverable by anyone defining a Token.

Would also simplify the whole duck-type-checks: if (token.displayName) { bla }

eladb · 2018-08-07T13:30:25Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+
+type Span = StringSpan | TokenSpan;
+
+const BEGIN_TOKEN_MARKER = '🔸🔹';


Cute idea of using emojis, but I am not 100% sure it's sufficient (and elegant enough). I think it's important that when people look at this, they can reference it to something in the code base, search google/stack-overflow, etc, and have some intuition has to what's going on.

I would do something like ${Token[key]}:

The ${} is a signal that this is going to be substituted/expanded at some point.

It points to the Token class, which is responsible for this (maybe we can even do cdk.Token[key]

The [] indicate that this is a key to some map.

I'm afraid of such simple strings in that they might naturally occur in strings where we DON'T want to replace them. So I'm looking for a string that's highly unlikely to occur (hence, the odd unicode codepoints).

I wouldn't say "${Token[*]}" is a common string...

eladb · 2018-08-07T13:31:08Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ throw new Error(`Invalid characters in token representation: ${key}`);
+ }
+
+ this.tokenMap[key] = token;


Defensive: check that key doesn't already exist?

Can't really happen since every one gets a unique number that's guaranteed to not occur in the map yet.

eladb · 2018-08-07T13:37:11Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ */
+ private split(s: string): Span[] {
+ const re = new RegExp(`${BEGIN_TOKEN_MARKER}([${VALID_KEY_CHARS}]+)${END_TOKEN_MARKER}`, 'gu');
+ const ret: Span[] = [];


I prefer = new Array<Span>

eladb · 2018-08-07T13:39:52Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ return span.value;
+ }
+ if (!(span.key in this.tokenMap)) {
+ throw new Error(`Unrecognized token representation: ${span.key}`);


"token key"

eladb · 2018-08-07T13:40:41Z

packages/@aws-cdk/cdk/test/core/test.tokens.ts

+ // THEN
+ test.deepEqual(resolved, {'Fn::Join': ['',
+ ['{"name":"Fido","speaks":"', 'woof woof', '"}']]});
+ test.done();
 }


I think some more testing is needed to cover various edge cases of defining tokens, display names and parsing.

eladb · 2018-08-08T03:00:12Z

packages/@aws-cdk/aws-cloudwatch/lib/dashboard.ts

@@ -33,7 +33,7 @@ export class Dashboard extends Construct {
 dashboardBody: new Token(() => {
 const column = new Column(...this.rows);
 column.position(0, 0);
- return tokenAwareJsonify({ widgets: column.toJson() });
+ return JSON.stringify({ widgets: column.toJson() });


It would be ideal if we can get rid of tokenAwareJsonify, but I am wondering if the new approach takes care of tokens that resolve to strings and may contain characters that require JSON escaping performed by stringifyStrings

Okay, it depends a little on what we want here. For example, do we want to strip undefineds from stringified objects if it turns out that the Token in there evaluates to undefined? For example, would you expect this:

const obj1 = { literal: "okay", gone: new Token(() => undefined), } const obj2 = { literal: "okay", gone: undefined, } // true or no? CFN(resolve(JSON.stringify(obj1))) == CFN(resolve(JSON.stringify(obj2)))

(where CFN is a magic function that implements the CloudFormation intrinsics evaluation engine)

If so, then we're going to need to delay the JSON.stringify() operation until resolve(), which means that we need tokenAwareJsonify() after all, to wrap the stringification operation in a Token itself.

For my part, I think we WOULD want the equivalence that I posited. Otherwise, users have to be aware where in a CloudFormation template their property values are used which is exactly the kind of the thing we're trying to get rid of.

Here is the test that covers the case I was talking about: https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/cdk/test/test.token-aware-jsonify.ts#L60

In the FnConcat version, the expected result should be something like:

{ 'Fn::Join': [ "", [ '{"test1":"', { 'Fn::Join': [ '', [ 'Hello', 'This\\nIs', 'Very \\"cool\\"' ] ] }, '"}' ] ] }

Notice that \n and " are escaped.

Oh sorry, I know, I got it. While working on this I just thought of some more edge cases we might want to cover, though. Sorry for not being more clear.

eladb · 2018-08-08T03:02:56Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ * when resolve() is called on the string.
+ */
+ public toString(): string {
+ if (this.stringRepr === undefined) {


If typeof(valueOfFunction) is a primitive (string, number, boolean), then just return it or it's .toString() representation (dah!). Basically the only situation where you need to tokenize is when valueOrFunction is a function or an object.

eladb · 2018-08-08T03:06:38Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ * returns a string representation for the Token pre-resolution, NOT the
+ * resolved value of the Token.
+ */
+ public toJSON(): any {


Wow! That's cool. Good to know.

So now I am thinking if we can simplify resolve by simply saying that tokens are objects that implement toJSON instead of resolve, but then why are you returning the string representation here....

eladb · 2018-08-08T03:13:12Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

@@ -133,3 +179,128 @@ export function resolve(obj: any, prefix?: string[]): any {

 return result;
 }
+
+import { FnConcat } from "../cloudformation/fn";


I actually thought of an approach to decouple - the core idea is that a token should be expanded to FnConcat only if it's a CFN intrinsic function (or Ref). Otherwise, we don't really know what to do with it.

So basically the heuristics are if resolve(x) returns an object that looks like a CFN intrinsic (i.e. key starts with Fn:: or Ref), then we can expand using FnConcat. Otherwise, we should probably fail resolution because that's probably not what the user wanted, unless someone can tell us how to expand this token.

So we can employ a "plug in" pattern - resolve can consult with a chain of "expanders" (?) when it needs to expand a token. And the CloudFormation library can register an expander that knows how to resolve {Fn::} and Ref objects. Some other libraries can use this to implement other expansion strategies.

MAJOR CHANGES - Reintroducing tokenAwareJsonify; it needs to be there because otherwise we cannot have enough knowledge to do escaping properly; see documentation in the code. - Introducing markAsIntrinsic() which sets a hidden property on an object which is used as a hint to the rest of the engine that (a) the value doesn't need to be resolved anymore and (b) we can use it as a type hint to reject non-intrinsic complex types during stringification. A wrapper class would have been nicer but this entails fewer code changes (deepEqual everywhere...). A convenience class has been introduced in the CloudFormation section to make impact minimal. - Implement inversion-of-control on the FnConcat thing, execution engines (name?) can now register themselves to handle string combining. Still not perfect because if there are not intrinsics we can't really guess the engine (so we "default" to CFN anyway) but we have the initial trappings of CFN decoupling. ALSO - Add a run config and a helper script to debug a unit test from VSCode (breakpoints \o/) I expect this to generate a healthy amount of debate. Also I would appreciate suggestions on naming of the various pieces, I'm afraid I haven't been 100% consistent just yet. - What do we call CloudFormation/Terraform/...? - What do we call stringified Tokens?

rix0rrr · 2018-08-09T14:50:33Z

After a giant slough, reintroducing tokenAwareJsonify()

MAJOR CHANGES

Reintroducing tokenAwareJsonify; it needs to be there because
otherwise we cannot have enough knowledge to do escaping properly;
see documentation in the code.
Introducing markAsIntrinsic() which sets a hidden property on an
object which is used as a hint to the rest of the engine that
(a) the value doesn't need to be resolved anymore and (b) we can
use it as a type hint to reject non-intrinsic complex types during
stringification. A wrapper class would have been nicer but this
entails fewer code changes (deepEqual everywhere...). A convenience
class has been introduced in the CloudFormation section to make
impact minimal.
Implement inversion-of-control on the FnConcat thing, execution
engines (name?) can now register themselves to handle string
combining. Still not perfect because if there are not intrinsics
we can't really guess the engine (so we "default" to CFN anyway)
but we have the initial trappings of CFN decoupling.

ALSO

Add a run config and a helper script to debug a unit test from VSCode
(breakpoints \o/)

I expect this to generate a healthy amount of debate. Also I would
appreciate suggestions on naming of the various pieces, I'm afraid
I haven't been 100% consistent just yet.

What do we call CloudFormation/Terraform/...?
What do we call stringified Tokens?

rix0rrr · 2018-08-09T15:03:33Z

Just realized this is still wrong when putting a token that lazily resolves to a number in a string first and then into a JSON structure :(

rix0rrr · 2018-08-09T15:18:45Z

Just realized this is still wrong when putting a token that lazily resolves to a number in a string first and then into a JSON structure :(

I guess the correct solution is still to use a different marker for "full value" JSONifications and threat them differently than string-inlined Token values. The difference is illustrated here:

const token = new Token(() => 1);

const embedded = `the number is ${token}`;

// Should lead to "the number is 1"
CFN(resolve(embedded));

// Logically, must lead to { "embedded": "the number is 1" }
// (but right now will throw)
CFN(resolve({ embedded }));

// However, a naked use of the token should preserve the type
// Should lead to { "the number": 1 }
CFN(resolve(tokenAwareJsonify({"the number": token})))

Changed tokenAwareJsonify() -> TokenJSON.stringify() to mimic JSON.stringify() API name. Implementation does not depend on override of toJSON() anymore, because there's no way to do that correctly. Instead, the function has a custom implementation. Split new code over more files to separate them out.

…-TypeScript languages anyway

…ngification

eladb · 2018-08-13T12:43:34Z

packages/@aws-cdk/cdk/test/core/test.token-json.ts

+
+ // THEN
+ const context = {MyBucket: 'TheName'};
+ test.equal(evaluateCFN(resolved, context), '{"theBucket":"TheName"}');


Why do we need evaluateCFN here? I actually expected to see raw CloudFormation JSON in the expected value. This might hide bugs...

The reason why we have this: I actually don't care about the CFN specifics. Use either {Fn::Join} or {Fn::Sub} or anything else--as long as it evaluates to whatever I care about. If we decide to change the mechanism that is used, I expect these tests to still work, because the feature still works.

If this is an issue of trust in the specific implementation of evaluateCFN, that is remedied with either tests on the implementation of it (but seems like overkill to me), or deploying a couple of stacks that use the current Token stringification implementation and seeing that they work (and thereby increasing the trust that evaluateCFN does what CloudFormation does), which is done in the integ test of CloudWatch dashboards.

Case in point: I just refactor the code to combine string literals while evaluating, so that if there were 2 fragments it was guaranteed that one of them was an intrinsic, and the only test that broke was one that did NOT use evaluateCFN. The test that broke was the one that looked at the structure of the returned data, not the thing it was representing (which is what is interesting about it).

What I'm concerned with here is testing at the right level of concern: I have a feeling that many of our tests that are testing "output is exactly this" are testing too much, not testing the actual behavior that they're interested in. This means they are less readable and prone to breakage. That, in turn, leads to more time to make changes (because you need to fix all tests that now complain) and also change fatigue, where after a while I will either (a) stop making changes or (b) ignore the cause of the failing test, assume it's because of something trivial and just copy/paste in a new example. Both would be bad.

:-)

I understand what you are talking about. It is a pragmatic choice we have been making throughout the CDK (and jsii to be honest). I think it's a cost/benefit question. It's much much easier to write those types of heavy-handed "diff" tests then granular, pure, meticulous testing you are talking about. Of course those would be more precise, but since we are still at a stage where things are moving quite a lot, the efficiency of using "diff tests" is proving itself. It might be annoying to need to update the test expectations, but it's much simpler to do that then to start changing dozens of potential assertions.

It's also about tools. I think our assertion library started providing some tools such as expect(to(haveResource( that make it easier to author more precise tests. But in a sense, we might be missing things because it's not testing for === but rather for >=.

At any rate, as much as this is unpure, I am quite okay with this trade off so far. It's proven itself quite good in protecting us from regression, which is, at the end, the sole purpose of those tests.

I know and I agree.

But the evaluateCFN function is both a really rather low investment AND allows us to express tests at the right level of abstraction ("if the result of this were to be evaluated by CloudFormation, would it give us back the value we intended?").

So I think it is the right way to test in this case.

I disagree that this is the right level of abstraction. These are unit tests, the boundary should be the output of the function, and not how CloudFormation will treat those results.

I subscribe to the "one concern per test" philosophy, so that a test has one reason to break (because that concern is no longer satisfied).

If you really really care, I will add a "String concatenation required by Token stringification is implemented using {Fn::Join}" test, so that we can guarantee that {Fn::Join} is used. Then, if we ever change that to {Fn::Sub}, that new test will rightly break because it was asserting that we were using {Fn::Join} and that's no longer true.

But I'm not going to rewrite the other tests to make the same assertions, because they're testing different concerns (embedding and escaping and such).

eladb · 2018-08-13T12:45:00Z

packages/@aws-cdk/cdk/lib/cloudformation/intrinsics.ts

+};
+
+ProvisioningEngine.register(CLOUDFORMATION_ENGINE, cloudFormationEngine);
+ProvisioningEngine.register(DEFAULT_ENGINE_NAME, cloudFormationEngine);


Why do we need the default?

We used to at some point because we could at some point stitch values together where none of them would be an intrinsic (and so it would not be obvious which engine to use) but I guess at this point that's something I can do in the framework.

eladb · 2018-08-13T12:46:00Z

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

@@ -391,8 +392,8 @@ export abstract class Referenceable extends StackElement {
 /**
 * Returns a token to a CloudFormation { Ref } that references this entity based on it's logical ID.
 */
- public get ref() {
- return new Token(() => ({ Ref: this.logicalId }));
+ public get ref(): Token {


Maybe we should just define a ‘Ref’ class for this?

No need for that. It doesn't add a lot of value and no user is ever going to instantiate it.

eladb · 2018-08-13T12:52:50Z

packages/@aws-cdk/cdk/lib/cloudformation/intrinsics.ts

@@ -0,0 +1,28 @@
+import { DEFAULT_ENGINE_NAME, ProvisioningEngine, StringFragment } from "../core/engine-strings";


This whole “provisioning engine framework” feels like it’s future proofing for something we really don’t know how to design yet. May I suggest that the token plugin system will be kept within the confines of the “tokens framework” and use domain-specific semantics.

For example, if the CDK will be used to synthesize pure Amazon State Language documents, then there is no provisioning engine involved, but there still might be tokenization heuristics.

Quite possibly, we might want to extract the tokens framework into a separate general-purpose and independent npm module, unrelated to cloud resource provisioning.

Well I did have it all in one file first, but it became quite large and mixed too many different concerns for my tastes. I just need some place to put this logic which is preferably a separate file, so it is "string handling for engines in an IoC fashion", which is this.

then there is no provisioning engine involved, but there still might be tokenization heuristics.

Regardless of the language that is being stringified (ASL or otherwise), if there is Token stringification to be done it will involve stitching string literals and intrinsics together. This must be done with an engine-specific FnConcat mechanism, which is what we model here.

If you use Amazon State Language on Terraform, we need terraform.concat, etc, and there would be a terraform/intrinsic.ts module.

Granted, maybe the filename is not fantastic here, and maybe the interface could be replaced with a single function, but that's about all the undesigning leeway we have.

I am not referring to the case where some other provisioning engine is used, I am referring to the case where the CDK is used to just synthesize other types of documents. I agree, tokenization/concat will still need to happen in those cases, but the provider will not be a "provisioning engine" but some other document format.

That's why I suggested to bind this terminology to the resolve/tokens domain and not to the resource provisioning domain. Then perhaps some of the abstractions will be an overkill. For example IProvisioningEngine.

Fix an issue where the wrong resolution method was used to resolve policy documents.

…on-intrinsics

eladb · 2018-08-14T07:25:28Z

packages/@aws-cdk/aws-logs/lib/cross-account-destination.ts

 // In the underlying model, the name is not optional, but we make it so anyway.
 const destinationName = props.destinationName || new cdk.Token(() => this.generateUniqueName());

 this.resource = new cloudformation.DestinationResource(this, 'Resource', {
 destinationName,
- destinationPolicy: new cdk.Token(() => !this.policyDocument.isEmpty ? JSON.stringify(this.policyDocument.resolve()) : ""),
+ destinationPolicy: new cdk.Token(() => !this.policyDocument.isEmpty ? JSON.stringify(cdk.resolve(this.policyDocument)) : ""),


I think this should have the exact same effect:

destinationPolicy: this.policyDocument

eladb · 2018-08-14T07:28:26Z

packages/@aws-cdk/cdk/lib/cloudformation/intrinsics.ts

+ * Base class for CloudFormation built-ins
+ */
+export class CloudFormationIntrinsicToken extends IntrinsicToken {
+ protected readonly engine: string = CLOUDFORMATION_ENGINE;


Instead of using a string value for engine, why not just point to a token resolver function?

It's not a resolver function, it's a join strings together in the engine function. Feels overly specific to me for intrinsics to carry around a function pointer to that.

But I guess I'm with ya on them having a direct reference to the engine, instead of indirected through a string pointing to a table.

eladb · 2018-08-14T07:30:37Z

packages/@aws-cdk/cdk/lib/core/engine-strings.ts

+ /**
+ * The intrinsic engine
+ */
+ intrinsicEngine?: string;


Make this a function resolver instead of a string.

eladb · 2018-08-14T07:30:52Z

scripts/runtest.js

@@ -0,0 +1,16 @@
+#!/usr/bin/env node


What's that?

It's a helper script for running unit tests in the debugger directly from VSCode. Breakpoints!

eladb · 2018-08-14T07:32:48Z

packages/@aws-cdk/cdk/test/core/test.token-json.ts

+ // WHEN
+ const resolved = resolve(TokenJSON.stringify({
+ literal: 'I can also "contain" quotes',
+ token


Can you please embed token into a string like this:

`I can also "contain" quotes: ${token}`

I'd like to make sure that this also works :-)

https://github.com/awslabs/aws-cdk/pull/518/files/4206b85da3627ba586e8d773edbda076f2c41bf7#diff-9b9d373dbf84ff1ee826d2b7cdb757ffR135

and

https://github.com/awslabs/aws-cdk/pull/518/files/4206b85da3627ba586e8d773edbda076f2c41bf7#diff-9b9d373dbf84ff1ee826d2b7cdb757ffR151

both test variants of this

eladb · 2018-08-14T07:34:54Z

packages/@aws-cdk/cdk/lib/core/util.ts

+export function splitOnMarkers(s: string, beginMarker: string, idPattern: string, endMarker: string): MarkerSpan[] {
+ // tslint:disable-next-line:max-line-length
+ const re = new RegExp(`${regexQuote(beginMarker)}(${idPattern})${regexQuote(endMarker)}`, 'g');


Can we just limit what beginMarker and endMarker accept instead of yet another escaping? There's no real need for those markers to be super flexible, and also, they should be picked based on their aesthetic form when embedded in strings, so picking something that will eventually be escaped kinda defeats the purpose, no?

Your previous favorite pick, ${Token[ and ]} contain 5 characters that need to be escaped when used in a regex. The alternative is to build our markers completely out of alphanumerics, which is not going to improve aesthetics and going to make collissions more likely.

Got it. Didn't realize this was in order to formulate the regex. The resulting marker is not going to be escaped. Sorry.

… cross-account-destination policy usage

… engines

eladb · 2018-08-14T13:57:28Z

packages/@aws-cdk/cdk/lib/cloudformation/cloudformation-json.ts

+ */
+export class CloudFormationJSON {
+ /**
+ * Turn an arbitrary structure potentially containing Tokens into JSON.


Return type should be string correct?
Also, in the description say "into a JSON string"

It actually returns a Token<string>.

eladb · 2018-08-14T13:58:47Z

packages/@aws-cdk/aws-cloudwatch/lib/dashboard.ts

@@ -33,7 +33,7 @@ export class Dashboard extends Construct {
 dashboardBody: new Token(() => {


This wraps a token and CloudFormationJSON.stringify also returns a Token. Can we eliminate one layer?

eladb · 2018-08-14T13:59:04Z

packages/@aws-cdk/cdk/lib/cloudformation/cloudformation-json.ts

+ return new Token(() => {
+ // Resolve inner value so that if they evaluate to literals, we maintain the type.
+ //
+ // Then replace intrinsics with a special sublcass of Token that overrides toJSON() to


"sublcass" => "subclass", "to and" => "and" (i think)

eladb · 2018-08-14T14:00:12Z

packages/@aws-cdk/cdk/lib/cloudformation/cloudformation-json.ts

+ });
+
+ /**
+ * Recurse into a structure, replace all intrinsics with


eladb · 2018-08-14T14:14:39Z

packages/@aws-cdk/cdk/lib/cloudformation/cloudformation-token.ts

+ joinerName: 'CloudFormation',
+
+ joinStringFragments(fragments: any[]): any {
+ return new FnConcat(...fragments.map(x => isIntrinsic(x) ? x : `${x}`));


Instead of ${x} maybe x.toString() will be more readable, no?

That fails in the case of undefined and null, whereas the quoting will work correctly for those cases.

eladb · 2018-08-14T14:17:56Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ /**
+ * Function used to concatenate strings and Token results together
+ *
+ * @default No joining


Describe what are the implications of not specifying a joiner (i.e. "it won't be possible to embed this token in strings")

eladb · 2018-08-14T14:18:30Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

@@ -67,7 +88,7 @@ export class Token {
 }



I am assuming you want to fail if there is no joiner, no?

Not necessarily, since it's not unlikely that people will lazily return literals.

eladb · 2018-08-14T14:20:27Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ /**
+ * The name of the joiner.
+ *
+ * Must be unique per joiner, because it will be used.


used for what...

eladb · 2018-08-14T14:20:52Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ /**
+ * Return the language intrinsic that will combine the strings in the given engine
+ */
+ joinStringFragments(fragments: any[]): any;


eladb · 2018-08-14T14:20:55Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ *
+ * Must be unique per joiner, because it will be used.
+ */
+ joinerName: string;


To avoid an undue number of {Fn::Join}s in the output, add an optimization into FnConcat() to inline nested FnConcats.

…ngification

eladb

Freaking awesome!

eladb · 2018-08-15T10:13:22Z

packages/@aws-cdk/cdk/lib/cloudformation/cloudformation-token.ts

- joiner: CLOUDFORMATION_JOINER,
- displayName
- });
+ super(valueOrFunction, displayName);


Ctor can be omitted then

eladb · 2018-08-15T10:16:50Z

packages/@aws-cdk/cdk/lib/cloudformation/pseudo.ts

 constructor(name: string) {
- super(() => ({ Ref: name }));
+ super(() => ({ Ref: name }), name);


Why does this need to be lazy? We can plug in a concrete value

eladb · 2018-08-15T10:17:41Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

@@ -82,7 +83,7 @@ export class Resource extends Referenceable {
 * @param attributeName The name of the attribute.
 */
 public getAtt(attributeName: string): Token {
- return new Token(() => ({ 'Fn::GetAtt': [this.logicalId, attributeName] }));


Doesn’t need to be lazy

eladb · 2018-08-15T10:18:11Z

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

- public get ref() {
- return new Token(() => ({ Ref: this.logicalId }));
+ public get ref(): Token {
+ return new CloudFormationToken(() => ({ Ref: this.logicalId }), `${this.logicalId}.Ref`);


Doesn’t need to be lazy

eladb · 2018-08-15T10:18:44Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

@@ -7,16 +7,35 @@ import { Construct } from "./construct";
 export const RESOLVE_METHOD = 'resolve';

 /**
- * Represents a lazy-evaluated value. Can be used to delay evaluation of a certain value
- * in case, for example, that it requires some context or late-bound data.
+ * Represents a lazy-evaluated value.


Not nesesarily lazy

eladb · 2018-08-15T10:21:16Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ * This gets called by JSON.stringify(). We want to prohibit this, because
+ * it's not possible to do this properly, so we just throw an error here.
+ */
+ public toJSON(): any {


There’s a leak in the docs... talks about CfN?

Yes. I was unsure of how to phrase this error message to be helpful without mentioning the specifics, so I erred on the side of helpful.

eladb · 2018-08-15T10:22:33Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ */
+ public concat(left: any | undefined, right: any | undefined): Token {
+ const parts = [left, resolve(this), right].filter(x => x !== undefined);
+ return new Token(() => parts.map(x => `${x}`).join(''));


eladb · 2018-08-15T10:23:08Z

packages/@aws-cdk/cdk/lib/core/tokens.ts

+ *
+ * The default implementation of this combines strings, but specialized
+ * implements of Token can return a more appropriate value.
+ */


Does it make sense to provide a default? In what use case it will be invoked?

It will be invoked when Tokenized string literals are being used (as in new Token("literal")). Having this behavior makes sense when detached from any particular document literal, as well as simplifying the implementation of join().

@rix0rrr

### Features * __@aws-cdk/cdk__: Tokens can now be transparently embedded into strings and encoded into JSON without losing their semantics. This makes it possible to treat late-bound (deploy-time) values as if they were regular strings ([@rix0rrr] in [#518](#518)). * __@aws-cdk/aws-s3__: add support for bucket notifications to Lambda, SNS, and SQS targets ([@eladb] in [#201](#201), [#560](#560), [#561](#561), [#564](#564)) * __@aws-cdk/cdk__: non-alphanumeric characters can now be used as construct identifiers ([@eladb] in [#556](#556)) * __@aws-cdk/aws-iam__: add support for `maxSessionDuration` for Roles ([@eladb] in [#545](#545)). ### Changes * __@aws-cdk/aws-lambda__ (_**BREAKING**_): most classes renamed to be shorter and more in line with official service naming (`Lambda` renamed to `Function` or ommitted) ([@eladb] in [#550](#550)) * __@aws-cdk/aws-codepipeline__ (_**BREAKING**_): move all CodePipeline actions from `@aws-cdk/aws-xxx-codepipeline` packages into the regular `@aws-cdk/aws-xxx` service packages ([@skinny85] in [#459](#459)). * __@aws-cdk/aws-custom-resources__ (_**BREAKING**_): package was removed, and the Custom Resource construct added to the __@aws-cdk/aws-cloudformation__ package ([@rix0rrr] in [#513](#513)) ### Fixes * __@aws-cdk/aws-lambda__: Lambdas that are triggered by CloudWatch Events now show up in the console, and can only be triggered the indicated Event Rule. _**BREAKING**_ for middleware writers (as this introduces an API change), but transparent to regular consumers ([@eladb] in [#558](#558)) * __@aws-cdk/aws-codecommit__: fix a bug where `pollForSourceChanges` could not be set to `false` ([@maciejwalkowiak] in [#534](#534)) * __aws-cdk__: don't fail if the `~/.aws/credentials` file is missing ([@RomainMuller] in [#541](#541)) * __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions to correctly support TemplateConfiguration ([@mindstorms6] in [#571](#571)). * __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions to correctly support ParameterOverrides ([@mindstorms6] in [#574](#574)). ### Known Issues * `cdk init` will try to init a `git` repository and fail if no global `user.name` and `user.email` have been configured.

…g types (#712) This change removes all strong-types generated to represent CloudFormation resource attributes that return a string (such as `QueueArn`, `DeploymentGroupId`) and replaces them with `string` that contains a stringified token (via #518). This allows working with these attributes as if they were regular strings, and dramatically simplifies the type-system and unneeded wrapping when assigning values to such attributes. The ".ref" property of each resource has been replaced with a property named according to the actual semantic meaning of the intrinsic reference (such as `queueArn`), and also returns a `string`. Users can test if a string contains stringified tokens using the function `unresolved(o)`, which can be applied to either strings or objects (previously was `isToken(o)`). Fixes #695 (opened #744 to follow up on non-string attributes)

__NOTICE__: This release includes a framework-wide [__breaking change__](#712) which changes the type of all the string resource attributes across the framework. Instead of using strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we now represent all these attributes as normal `string`s, and codify the tokens into the string (using the feature introduced in [#168](#168)). Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs, use the static methods on `cdk.ArnUtils`. See motivation and discussion in [#695](#695). * **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744) * **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189)) * **aws-codebuild:** fix typo "priviledged" -> "privileged * **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706) * **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged ([#734](#734)) ([72fec36](72fec36)) * **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5)) * **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708) * **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714) * **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672) * **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233) * **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857)) * **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130) * **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818)) * **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0)) * **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719) * **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918)) * **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)

* v0.9.2 __NOTICE__: This release includes a framework-wide [__breaking change__](#712) which changes the type of all the string resource attributes across the framework. Instead of using strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we now represent all these attributes as normal `string`s, and codify the tokens into the string (using the feature introduced in [#168](#168)). Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs, use the static methods on `cdk.ArnUtils`. See motivation and discussion in [#695](#695). * **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744) * **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189)) * **aws-codebuild:** fix typo "priviledged" -> "privileged * **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706) * **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged ([#734](#734)) ([72fec36](72fec36)) * **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5)) * **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708) * **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714) * **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672) * **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233) * **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857)) * **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130) * **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818)) * **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0)) * **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719) * **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918)) * **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)

rix0rrr requested review from eladb and RomainMuller August 7, 2018 13:03

eladb suggested changes Aug 7, 2018

View reviewed changes

eladb suggested changes Aug 8, 2018

View reviewed changes

Rico Huijbers added 5 commits August 8, 2018 15:32

Remove incorrectly staged custom-resources directory

9469a71

WIP

e47741b

Merge branch 'master' into huijbers/token-stringification

f6d01a8

Remove HintedToken, change markers, add quoting

6e79bb8

Update testing

a72d14d

rix0rrr mentioned this pull request Aug 9, 2018

Assets: upload to and grant permissions on prefix #510

Merged

Rico Huijbers added 2 commits August 9, 2018 10:52

WIP

42becf3

Make CloudWatch use tokenAwareJsonify again

d1fc358

moofish32 mentioned this pull request Aug 9, 2018

Create A Tag Management for Constructs #538

Merged

Rico Huijbers added 3 commits August 13, 2018 14:18

SecretParameter no longer duckily implements Token, won't work in non…

74f4fe7

…-TypeScript languages anyway

Merge remote-tracking branch 'origin/master' into huijbers/token-stri…

b15684d

…ngification

eladb reviewed Aug 13, 2018

View reviewed changes

Rico Huijbers added 2 commits August 13, 2018 15:20

fix(aws-logs): resolving of policy

5753e31

Fix an issue where the wrong resolution method was used to resolve policy documents.

Combine string literals while resolving fragments, so we never have n…

4206b85

…on-intrinsics

eladb reviewed Aug 14, 2018

View reviewed changes

Rico Huijbers added 2 commits August 14, 2018 10:32

Make intrinsics carry a direct object reference to the engine, simply…

85cfa81

… cross-account-destination policy usage

Move JSONification to CloudFormation library, remove any reference to…

a3ec610

… engines

eladb approved these changes Aug 14, 2018

View reviewed changes

eladb mentioned this pull request Aug 14, 2018

Impossible to generate correct parameter overrides #566

Closed

Rico Huijbers added 3 commits August 15, 2018 09:40

Explain better in comments, fix cross-account-destination

7361815

Remove joiner(), make Tokens concat() themselves

a7b41e2

To avoid an undue number of {Fn::Join}s in the output, add an optimization into FnConcat() to inline nested FnConcats.

Merge remote-tracking branch 'origin/master' into huijbers/token-stri…

6aa9b77

…ngification

eladb approved these changes Aug 15, 2018

View reviewed changes

Remove laziness of many existing Token use cases

c5b3b68

eladb approved these changes Aug 15, 2018

View reviewed changes

rix0rrr merged commit 939015a into master Aug 15, 2018

rix0rrr deleted the huijbers/token-stringification branch August 15, 2018 11:25

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

		@@ -1,112 +0,0 @@
		import { istoken, resolve, Token } from '../core/tokens';


		type Span = StringSpan \| TokenSpan;

		const BEGIN_TOKEN_MARKER = '🔸🔹';

		@@ -0,0 +1,28 @@
		import { DEFAULT_ENGINE_NAME, ProvisioningEngine, StringFragment } from "../core/engine-strings";

		@@ -33,7 +33,7 @@ export class Dashboard extends Construct {
		dashboardBody: new Token(() => {

Framework: Tokens can be converted to strings #518

Framework: Tokens can be converted to strings #518

Conversation

rix0rrr commented Aug 7, 2018

rix0rrr commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Aug 9, 2018

rix0rrr commented Aug 9, 2018

rix0rrr commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

rix0rrr Aug 8, 2018 •

edited

Loading

eladb Aug 9, 2018 •

edited

Loading

rix0rrr Aug 13, 2018 •

edited

Loading

rix0rrr Aug 14, 2018 •

edited

Loading