fix(core): message including tokens from annotations cannot output correctly

[go-to-k]

Issue # (if applicable)

Closes #33707

Reason for this change

If a stack with name 'some-stack' includes an info annotation

Annotations.of(this).addInfo(`stackId: ${this.stackId}`);

then the following output results:

[Info at /some-stack] [object Object]

That's because data comes from Annotations and the data can be of object type containing 'Fn::Join' or 'Ref' when tokens are included in Annotations.

The issue mentioned a proposal to output the data in the form of tokens like [Info at /CdkSampleStack] ${Token[AWS::StackId.1116]}.

Description of changes

Approach 1 for now. (I am still wondering if approach 3 would be better...)

See below:

Approach 1

The PR makes messages with tokens by annotations unresolved.

NOTE

This change would also output a token format in manifest.json.

If users run integ tests with annotations including tokens, the manifest.json would change for every run. (like ${Token[AWS::StackId.1119]} -> ${Token[AWS::StackId.123]} -> ${Token[AWS::StackId.521]} -> ...)

{
  // ...
  "CdkSampleStack": {
    // ...
      "metadata": {
        "/CdkSampleStack": [
          {
            "type": "aws:cdk:info",
            "data": "stackId: ${Token[AWS::StackId.1119]}",

Approach 2

Change the type for the msg.entry.data (MetadataEntryData for MetadataEntry) to a string type with JSON.stringify if the type is an objective type in cdk-cli.

https://github.com/aws/aws-cdk-cli/blob/cdk%40v2.1003.0/packages/%40aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts#L771

Then I had submitted the PR in aws-cdk-cli.

But talked with Rico that the change should be made inside cdk-lib and leave the token unrendered.

aws/aws-cdk-cli#101 (comment)

Approach 3

Change the data type to a string type after resolve if the data is by annotations with tokens.

This approach doesn't make differences in manifest.json for every run and the original format (with 'Ref' or 'Fn::Join') is kept.

However, the issue for this PR and comments in the PR submitted (aws-cdk-cli) has proposed the approach with unresolved tokens, I decided the approach 1 for now.

63fd78b

    if (node.node.metadata.length > 0) {
      // Make the path absolute
      output[Node.PATH_SEP + node.node.path] = node.node.metadata.map(md => {
        const resolved = stack.resolve(md) as cxschema.MetadataEntry;

        const isAnnotation = [
          cxschema.ArtifactMetadataEntryType.ERROR,
          cxschema.ArtifactMetadataEntryType.WARN,
          cxschema.ArtifactMetadataEntryType.INFO,
        ].includes(md.type as cxschema.ArtifactMetadataEntryType);

        // Transform the data to a string for the case where Annotations include a token.
        // Otherwise, the message is resolved and output as `[object Object]` after synth
        // because the message will be object type using 'Ref' or 'Fn::Join'.
        const mdWithStringData: cxschema.MetadataEntry = {
          ...resolved,
          data: (isAnnotation && typeof resolved.data === 'object') ? JSON.stringify(resolved.data) : resolved.data,
        };
        return mdWithStringData;
      });
    }

This approach outputs the message as the following style:

{"Fn::Join":["",["Cannot add a resource policy to your dead letter queue associated with rule ",{"Ref":"Rule4C995B7F"}," because the queue is in a different account. You must add the resource policy manually to the dead letter queue in account 444455556666. [ack: @aws-cdk/aws-events-targets:manuallyAddDLQResourcePolicy]"]]}

Additional Information

see:

#33707 (comment)

aws/aws-cdk-cli#101 (comment)

Describe any new or updated permissions being added

Description of how you validated changes

Unit tests.

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[aws-cdk-automation]

(This review is outdated)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.38%. Comparing base (3f1fecf) to head (3c2f8d7).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #33706      +/-   ##
==========================================
+ Coverage   82.37%   82.38%   +0.01%     
==========================================
  Files         120      120              
  Lines        6933     6937       +4     
  Branches     1169     1170       +1     
==========================================
+ Hits         5711     5715       +4     
  Misses       1119     1119              
  Partials      103      103              

Flag	Coverage Δ
suite.unit	82.38% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	∅ <ø> (∅)
packages/aws-cdk-lib/core	82.38% <100.00%> (+0.01%)	⬆️

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[go-to-k]

Exemption Request:

This PR could covered with unit tests only.

And I think the integration test is unnecessary, as it should originally not allow tokens to be included in annotations. (CDK CLI would expect the type of string.)

Also integ tests for annotations with tokens will make differences in manifest.json for every run.

aws/aws-cdk-cli#101 (comment)

[go-to-k]

Dear team,

I came up with the approach to change the data type to a string type after resolve if the data is by annotations with tokens. (see Approach 3 in the description in this PR.)

This approach doesn't make differences in manifest.json for every run and the original format (with 'Ref' or 'Fn::Join') is kept.

However, the issue for this PR and comments in the PR submitted (aws-cdk-cli) has proposed the approach with unresolved tokens, I decided the approach 1 for now.

Please let me know if the approach would be better.

63fd78b

    if (node.node.metadata.length > 0) {
      // Make the path absolute
      output[Node.PATH_SEP + node.node.path] = node.node.metadata.map(md => {
        const resolved = stack.resolve(md) as cxschema.MetadataEntry;

        const isAnnotation = [
          cxschema.ArtifactMetadataEntryType.ERROR,
          cxschema.ArtifactMetadataEntryType.WARN,
          cxschema.ArtifactMetadataEntryType.INFO,
        ].includes(md.type as cxschema.ArtifactMetadataEntryType);

        // Transform the data to a string for the case where Annotations include a token.
        // Otherwise, the message is resolved and output as `[object Object]` after synth
        // because the message will be object type using 'Ref' or 'Fn::Join'.
        const mdWithStringData: cxschema.MetadataEntry = {
          ...resolved,
          data: (isAnnotation && typeof resolved.data === 'object') ? JSON.stringify(resolved.data) : resolved.data,
        };
        return mdWithStringData;
      });
    }

This approach outputs the message as the following style:

{"Fn::Join":["",["Cannot add a resource policy to your dead letter queue associated with rule ",{"Ref":"Rule4C995B7F"}," because the queue is in a different account. You must add the resource policy manually to the dead letter queue in account 444455556666. [ack: @aws-cdk/aws-events-targets:manuallyAddDLQResourcePolicy]"]]}

[GavinZZ]

LGTM, thank you!

[GavinZZ]

@go-to-k I think Option 1 makes sense to me.

If users run integ tests with annotations including tokens, the manifest.json would change for every run

I think this is not a concern for existing tests (if I'm not mistaken) since existing tests would not fail for manifest file change as long as the other snapshots template remain the same.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 3c2f8d7
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.