Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Framework: Tokens can be converted to strings #518

Merged
merged 20 commits into from
Aug 15, 2018

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Aug 7, 2018

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
Copy link
Contributor Author

rix0rrr commented Aug 7, 2018

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.

@rix0rrr rix0rrr requested review from eladb and RomainMuller August 7, 2018 13:03
@@ -1,112 +0,0 @@
import { istoken, resolve, Token } from '../core/tokens';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice touch

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these changes supposed to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crap

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }


type Span = StringSpan | TokenSpan;

const BEGIN_TOKEN_MARKER = '🔸🔹';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]}:

  1. The ${} is a signal that this is going to be substituted/expanded at some point.
  2. It points to the Token class, which is responsible for this (maybe we can even do cdk.Token[key]
  3. The [] indicate that this is a key to some map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new Error(`Invalid characters in token representation: ${key}`);
}

this.tokenMap[key] = token;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defensive: check that key doesn't already exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer = new Array<Span>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"token key"

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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() });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@rix0rrr rix0rrr Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@eladb eladb Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....

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

return result;
}

import { FnConcat } from "../cloudformation/fn";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Rico Huijbers added 2 commits August 9, 2018 10:52
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
Copy link
Contributor Author

rix0rrr commented Aug 9, 2018

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
Copy link
Contributor Author

rix0rrr commented Aug 9, 2018

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
Copy link
Contributor Author

rix0rrr commented Aug 9, 2018

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

Rico Huijbers added 3 commits August 13, 2018 14:18
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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-)

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.

Copy link
Contributor Author

@rix0rrr rix0rrr Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

};

ProvisioningEngine.register(CLOUDFORMATION_ENGINE, cloudFormationEngine);
ProvisioningEngine.register(DEFAULT_ENGINE_NAME, cloudFormationEngine);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Rico Huijbers added 2 commits August 13, 2018 15:20
Fix an issue where the wrong resolution method was used to resolve policy documents.
// 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)) : ""),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should have the exact same effect:

destinationPolicy: this.policyDocument

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

joiner?

/**
* The intrinsic engine
*/
intrinsicEngine?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a function resolver instead of a string.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually returns a Token<string>.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

/**
* Recurse into a structure, replace all intrinsics with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"with..."?

joinerName: 'CloudFormation',

joinStringFragments(fragments: any[]): any {
return new FnConcat(...fragments.map(x => isIntrinsic(x) ? x : `${x}`));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* Function used to concatenate strings and Token results together
*
* @default No joining
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@rix0rrr rix0rrr Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used for what...

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

join?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id

Rico Huijbers added 3 commits August 15, 2018 09:40
To avoid an undue number of {Fn::Join}s in the output, add an optimization
into FnConcat() to inline nested FnConcats.
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Freaking awesome!

joiner: CLOUDFORMATION_JOINER,
displayName
});
super(valueOrFunction, displayName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ctor can be omitted then

constructor(name: string) {
super(() => ({ Ref: name }));
super(() => ({ Ref: name }), name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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] }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn’t need to be lazy

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn’t need to be lazy

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not nesesarily lazy

* 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
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(''));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why lazy?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@rix0rrr rix0rrr Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rix0rrr merged commit 939015a into master Aug 15, 2018
@rix0rrr rix0rrr deleted the huijbers/token-stringification branch August 15, 2018 11:25
rix0rrr added a commit that referenced this pull request Aug 15, 2018
### 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.
eladb pushed a commit that referenced this pull request Sep 19, 2018
…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)
eladb pushed a commit that referenced this pull request Sep 20, 2018
__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)
eladb pushed a commit that referenced this pull request Sep 20, 2018
* 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)
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Token.toString()
3 participants