refactor(core): refactor API of Tokens #2757

rix0rrr · 2019-06-05T14:26:15Z

Tokens are now represented by the interface IResolvable. The class Token
still exists, but it is used to hold static routines for token encoding
(Token.asString()).

Actual token use is split into 2 categories:

Intrinsics, represented by static methods on Token: these are to represent values
that will be understood by the deployment language formalism (e.g.
CloudFormation), and can be used to escape language type checking.
Lazy values, represented by a Lazy class with static factories:
these will be used to represent type-checked but lazily-produced
values (evaluated at synthesis time).

In order to be JSII-compatible (which does not currently support
lambdas), Lazy.stringValue() et. al. take an interface with a single
method instead of a function.

Also changed in this commit: shoring up property names for encoded
tokens in classes like CfnParameter and CfnElement.

.stringValue => .valueAsString, etc so .value, .valueAsString,
.valueAsList etc. group together in autocomplete.
.ref now returns a Token, .refAsString returns the stringified
version that token.

To match the other standard the latter should actually be .refAsString
but .ref is used in so many places that this might be uncomfortable?

Fixes #1933.

BREAKING CHANGES:

Token can no longer be instantiated. You probably want to use
Lazy.stringValue and others, or Token.asString() and others.
Token.isToken()/Token.unresolved() => Token.isUnresolved().
CfnOutput: remove .ref, since they can't be referenced anyway.
CfnParameter: rename .stringValue => .valueAsString (and
similar for lists and numbers).
.ref now returns an IToken, use .refAsString to get a
string-encoded Token.
TokenMap is now no longer exported, some parts of its functionality
are available through the static Tokenization class.
IResolvedValuePostProcessor => IResolvableWithPostProcess.

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

Tokens are now represented by the interface `IToken`. The class `Token` still exists, but it is used to hold static routines for token encoding (`Token.encodeAsString()`), and as a potential base class for `IToken` implementations so some default functionality is inherited (this inheritance is an implementation optimization, it is not a requirement!). Actual token use is split into 2 categories: - *Intrinsics*, represented by `Intrinsic` class: these are to represent values that will be understood by the deployment language formalism (e.g. CloudFormation), and can be used to escape language type checking. - *Lazy values*, represented by a `Lazy` class with static factories: these will be used to represent type-checked but lazily-produced values (evaluated at synthesis time). In order to be JSII-compatible (which does not currently support lambdas), `Lazy.stringValue()` et. al. take an interface with a single method instead of a function. Also changed in this commit: shoring up property names for encoded tokens in classes like `CfnParameter` and `CfnElement`. * `.stringValue` => `.valueAsString`, etc so `.value`, `.valueAsString`, `.valueAsList` etc. group together in autocomplete. * `.ref` now returns a Token, `.refAsString` returns the stringified version that token. To match the other standard the latter should actually be `.refAsString` but `.ref` is used in so many places that this might be uncomfortable? Revert `Token.isToken()` to check if the passed object is literally an `IToken` (to match `Array.isArray()` and other related methods), and `Token.unresolved()` to check if the passed object might also be an encoded Token. Fixes #1933. BREAKING CHANGES: * `Token` can no longer be instantiated. Instead, instantiate `Intrinsic` or use `Lazy.stringValue` and others. * `Token.isToken()` will only return true if the object is a Token, for detecting arbitrary encoded tokens, use `Token.unresolved()`. * `CfnOutput`: remove `.ref`, since they can't be referenced anyway. * `CfnParameter`: rename `.stringValue` => `.valueAsString` (and similar for lists and numbers). * `.ref` now returns an `IToken`, use `.refAsString` to get a string-encoded Token.

RomainMuller · 2019-06-05T15:16:28Z

packages/@aws-cdk/cdk/lib/lazy.ts

+}
+
+/**
+ * Lazily produce a value


To me, Lazy means that we'll spare a bunch of expensive compute if some value is never used. That's not what we do here, more like deferring the compute to a later stage (the prepare or synth phase, I guess?).

I don't think Lazy is so fundamentally wrong that this is a deal breaker to me, however I kinda feel like something like Deferred or SynthesisTime would be a more accurate description of what happens.

RomainMuller · 2019-06-05T15:17:17Z

packages/@aws-cdk/cdk/lib/lazy.ts

+/**
+ * Interface for lazy string producers
+ */
+export interface IStringValue {


Is this not an IStringValueProducer? The same applies too... the other I*Value interfaces here.

RomainMuller · 2019-06-05T15:18:01Z

packages/@aws-cdk/cdk/lib/lazy.ts

+  /**
+   * Produce the list value
+   */
+  produce(context: IResolveContext): string[] | undefined;


That makes it an IStringListValue[Producer]...

RomainMuller · 2019-06-05T15:19:19Z

packages/@aws-cdk/cdk/lib/lazy.ts

+ * will only be calculated later, during synthesis.
+ */
+export class Lazy {
+  public static stringValue(producer: IStringValue, options: LazyStringValueOptions = {}) {


I kinda would like tho have the return types of those be spelled out on the signature. But I'm cool if you choose not to.

eladb · 2019-06-05T18:31:34Z

packages/@aws-cdk/aws-ec2/lib/security-group-rule.ts

@@ -239,7 +239,7 @@ export class UdpPort implements IPortRange {
 * A UDP port range
 */
 export class UdpPortRange implements IPortRange {
-  public readonly canInlineRule = !Token.isToken(this.startPort) && !Token.isToken(this.endPort);


why? I still think Token.isToken is better.

But it does something completely different than Array.isArray, Construct.isConstruct, and every other Class.isClass method we've added anywhere else.

eladb · 2019-06-06T07:30:31Z

packages/@aws-cdk/aws-apigateway/lib/deployment.ts

@@ -76,7 +76,7 @@ export class Deployment extends Resource {
    }

    this.api = props.api;
-    this.deploymentId = new Token(() => this.resource.deploymentId).toString();
+    this.deploymentId = Lazy.stringValue({ produce: () => this.resource.deploymentId });


Can we shorten? Lazy.string conveys the exact same meaning with less words...

I agree, but string will be a keyword in nearly all languages.

eladb · 2019-06-06T07:31:39Z

tools/cfn2ts/lib/genspec.ts

@@ -174,6 +174,8 @@ export function attributeDefinition(resourceName: CodeName, attributeName: strin
  let attrType: string;
  if ('PrimitiveType' in spec && spec.PrimitiveType === 'String') {
    attrType = 'string';
+  } else if ('PrimitiveType' in spec && spec.PrimitiveType === 'Integer') {


eladb · 2019-06-06T07:34:51Z

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

   *
   * @param obj The object to test.
   */
-  public static isToken(obj: any): boolean {
-    return unresolved(obj);
+  public static isToken(obj: any): obj is IToken {


I feel Token.isToken(x) should return true if x is a token or a token encoded as a primitive (list/string/number).
The test of whether something implements IToken should probably not even be a public API.

eladb · 2019-06-06T07:39:41Z

packages/@aws-cdk/cdk/lib/fn.ts

@@ -292,14 +293,14 @@ export class Fn {
   * @returns a token represented as a string array
   */
  public valueOfAll(parameterType: string, attribute: string): string[] {
-    return new FnValueOfAll(parameterType, attribute).toList();
+    return Token.encodeAsList(new FnValueOfAll(parameterType, attribute));


At this point I don't think we need all the FnXxx private classes any more. Let's just embed the intrinsic function directly.

Maybe, but they're all private so not really hurting.

eladb · 2019-06-06T07:42:09Z

packages/@aws-cdk/cdk/lib/intrinsic.ts

+/**
+ * Token subclass that represents values intrinsic to the target document language
+ */
+export class Intrinsic extends Token {


TBH I am not sure why this is needed? Kind of feels like encodeAsXxx should accept any which can be either an IToken or some primitive values that will be passed through as a token.

I also don't like the CFN semantics that "seeped through" to this layer. If you insist on keeping this class (again, I don't think it's really needed), then I would call it AnyValue or AnyToken or something like that.

The word "intrinsic" happens to exist in CloudFormation already, but I actually think it makes sense to have this as a concept in CDK: a value that makes sense in the target document language, which might not strictly adhere to the typing of the host language. The document language might be CFN, might be Google Cloud Manager, might be Kubernetes YAML, might be anything. There will still be value in the "intrinsic" concept, and nobody is forcing us to call it something else than it is called in CloudFormation.

As a concept name, I like "intrinsic" better than "AnyValue".

I also don't see why we need to bind the representation of the "AnyValue" and the encoding together. To me, it feels better to make them explicitly two operations: first represent a document language-level value, and maybe additionally encode it.

Users won't need to see any of this anyway, they will have the necessary operations pre-wrapped, and only use the Lazy constructors.

rix0rrr · 2019-06-06T08:54:20Z

TODO (just realized):

Lazy.anyValue() needs to return an IToken, otherwise jsii languages won't be able to pass the result in anywhere (any escapes typesystem is only valid in TypeScript).
cfn2ts and all other places that say they accept a Token need to accept an IToken.

eladb

Beautiful. Take it home!

Left some comments on the commits...

…ws-lambda-python-alpha/test/lambda-handler-poetry (#27381) Bumps [urllib3](https://github.com/urllib3/urllib3) from 2.0.3 to 2.0.6. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/urllib3/urllib3/releases">urllib3's releases</a>.</em></p> <blockquote> <h2>2.0.6</h2> <ul> <li>Added the <code>Cookie</code> header to the list of headers to strip from requests when redirecting to a different host. As before, different headers can be set via <code>Retry.remove_headers_on_redirect</code>. (GHSA-v845-jxx5-vc9f)</li> </ul> <h2>2.0.5</h2> <ul> <li>Allowed pyOpenSSL third-party module without any deprecation warning. <a href="https://redirect.github.com/urllib3/urllib3/issues/3126">#3126</a></li> <li>Fixed default <code>blocksize</code> of <code>HTTPConnection</code> classes to match high-level classes. Previously was 8KiB, now 16KiB. <a href="https://redirect.github.com/urllib3/urllib3/issues/3066%3E">#3066</a></li> </ul> <h2>2.0.4</h2> <ul> <li>Added support for union operators to <code>HTTPHeaderDict</code> (<a href="https://redirect.github.com/urllib3/urllib3/issues/2254">#2254</a>)</li> <li>Added <code>BaseHTTPResponse</code> to <code>urllib3.__all__</code> (<a href="https://redirect.github.com/urllib3/urllib3/issues/3078">#3078</a>)</li> <li>Fixed <code>urllib3.connection.HTTPConnection</code> to raise the <code>http.client.connect</code> audit event to have the same behavior as the standard library HTTP client (<a href="https://redirect.github.com/urllib3/urllib3/issues/2757">#2757</a>)</li> <li>Relied on the standard library for checking hostnames in supported PyPy releases (<a href="https://redirect.github.com/urllib3/urllib3/issues/3087">#3087</a>)</li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/urllib3/urllib3/blob/main/CHANGES.rst">urllib3's changelog</a>.</em></p> <blockquote> <h1>2.0.6 (2023-10-02)</h1> <ul> <li>Added the <code>Cookie</code> header to the list of headers to strip from requests when redirecting to a different host. As before, different headers can be set via <code>Retry.remove_headers_on_redirect</code>.</li> </ul> <h1>2.0.5 (2023-09-20)</h1> <ul> <li>Allowed pyOpenSSL third-party module without any deprecation warning. (<code>[#3126](urllib3/urllib3#3126) <https://github.com/urllib3/urllib3/issues/3126></code>__)</li> <li>Fixed default <code>blocksize</code> of <code>HTTPConnection</code> classes to match high-level classes. Previously was 8KiB, now 16KiB. (<code>[#3066](urllib3/urllib3#3066) <https://github.com/urllib3/urllib3/issues/3066></code>__)</li> </ul> <h1>2.0.4 (2023-07-19)</h1> <ul> <li>Added support for union operators to <code>HTTPHeaderDict</code> (<code>[#2254](urllib3/urllib3#2254) <https://github.com/urllib3/urllib3/issues/2254></code>__)</li> <li>Added <code>BaseHTTPResponse</code> to <code>urllib3.__all__</code> (<code>[#3078](urllib3/urllib3#3078) <https://github.com/urllib3/urllib3/issues/3078></code>__)</li> <li>Fixed <code>urllib3.connection.HTTPConnection</code> to raise the <code>http.client.connect</code> audit event to have the same behavior as the standard library HTTP client (<code>[#2757](urllib3/urllib3#2757) <https://github.com/urllib3/urllib3/issues/2757></code>__)</li> <li>Relied on the standard library for checking hostnames in supported PyPy releases (<code>[#3087](urllib3/urllib3#3087) <https://github.com/urllib3/urllib3/issues/3087></code>__)</li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/urllib3/urllib3/commit/262e3e332209ee93ff70e2b13502c8f20c105ac8"><code>262e3e3</code></a> Release 2.0.6</li> <li><a href="https://github.com/urllib3/urllib3/commit/644124ecd0b6e417c527191f866daa05a5a2056d"><code>644124e</code></a> Merge pull request from GHSA-v845-jxx5-vc9f</li> <li><a href="https://github.com/urllib3/urllib3/commit/740380c59ca2a7c2dceca19e5dba99f6b7060e62"><code>740380c</code></a> Bump cryptography from 41.0.3 to 41.0.4 (<a href="https://redirect.github.com/urllib3/urllib3/issues/3131">#3131</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/d9f85a749488188c286cd50606d159874db94d5f"><code>d9f85a7</code></a> Release 2.0.5</li> <li><a href="https://github.com/urllib3/urllib3/commit/d41f4122966f7f4f5f92001ad518e5d9dafcc886"><code>d41f412</code></a> Undeprecate pyOpenSSL module (<a href="https://redirect.github.com/urllib3/urllib3/issues/3127">#3127</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/b6c04cb3e62ef5a0e4947d037c12fb3ca79e024a"><code>b6c04cb</code></a> Fix a link to "absolute URI" definition (<a href="https://redirect.github.com/urllib3/urllib3/issues/3128">#3128</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/af7c78fa30f5a4e265911371d0c59b6baeddca0f"><code>af7c78f</code></a> refactor: change double conditional to one (<a href="https://redirect.github.com/urllib3/urllib3/issues/3118">#3118</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/34c13c8e68df6f89890ba08b9fc4fbf87ed21669"><code>34c13c8</code></a> Refer to current internet standards in docs on proxies (<a href="https://redirect.github.com/urllib3/urllib3/issues/3124">#3124</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/a3e94f218cd8297db73302eadae235f0c832a809"><code>a3e94f2</code></a> Fix a name of an attribute in docs (<a href="https://redirect.github.com/urllib3/urllib3/issues/3125">#3125</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/da69d4f4f95bc7ef9307fc8e0499c2121f1e4791"><code>da69d4f</code></a> Fix docs build (<a href="https://redirect.github.com/urllib3/urllib3/issues/3123">#3123</a>)</li> <li>Additional commits viewable in <a href="https://github.com/urllib3/urllib3/compare/2.0.3...2.0.6">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=urllib3&package-manager=pip&previous-version=2.0.3&new-version=2.0.6)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/aws/aws-cdk/network/alerts). </details>

rix0rrr added 3 commits June 5, 2019 12:39

Updating a bunch of usage

5fb1153

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

f7611ee

rix0rrr requested review from RomainMuller, skinny85, SoManyHs and a team as code owners June 5, 2019 14:26

RomainMuller mentioned this pull request Jun 5, 2019

chore(pkglint): Require stability setting in package.json #2736

Merged

RomainMuller reviewed Jun 5, 2019

View reviewed changes

eladb suggested changes Jun 6, 2019

View reviewed changes

eladb approved these changes Jun 7, 2019

View reviewed changes

rix0rrr added 2 commits June 7, 2019 14:58

Hiding more details

62f2cc2

Package locks

a056546

rix0rrr force-pushed the huijbers/token branch from f8c4b01 to a056546 Compare June 7, 2019 12:59

rix0rrr added 6 commits June 7, 2019 15:09

Fix test bugs

7a6abbb

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

1faaffc

Fix straggling import

4ea1251

Missed function rename

1fe30ac

Another missed refactor

245c82a

Fix cross-stack references

e37bb4d

rix0rrr added the p0 label Jun 10, 2019

rix0rrr mentioned this pull request Jun 10, 2019

Unable to reference a cdk.CfnParameter with Number type #2775

Closed

Rico Huijbers and others added 4 commits June 10, 2019 14:11

Update expectations

cd0dfb6

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

8aaf905

package-locks

c17e925

Fix build

9494e58

rix0rrr merged commit a12fcfa into master Jun 11, 2019

rix0rrr deleted the huijbers/token branch June 11, 2019 09:44

jogold mentioned this pull request Jun 28, 2019

Incorrect token resolution with RuleTargetInput/FieldAwareEventInput #3119

Closed

5 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): refactor API of Tokens #2757

refactor(core): refactor API of Tokens #2757

rix0rrr commented Jun 5, 2019 •

edited

Loading

RomainMuller Jun 5, 2019

RomainMuller Jun 5, 2019

RomainMuller Jun 5, 2019

RomainMuller Jun 5, 2019

eladb Jun 5, 2019

rix0rrr Jun 6, 2019

eladb Jun 6, 2019

rix0rrr Jun 6, 2019 •

edited

Loading

eladb Jun 6, 2019

eladb Jun 6, 2019

eladb Jun 6, 2019

rix0rrr Jun 6, 2019

eladb Jun 6, 2019

rix0rrr Jun 6, 2019

rix0rrr commented Jun 6, 2019

eladb left a comment

refactor(core): refactor API of Tokens #2757

refactor(core): refactor API of Tokens #2757

Conversation

rix0rrr commented Jun 5, 2019 • edited Loading

Pull Request Checklist

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 Jun 6, 2019 • 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

rix0rrr commented Jun 6, 2019

eladb left a comment

Choose a reason for hiding this comment

rix0rrr commented Jun 5, 2019 •

edited

Loading

rix0rrr Jun 6, 2019 •

edited

Loading