fix(core): some trace info is missing from the validation report (#24889)

Fixes an issue where we were not getting trace information all the way up the construct tree. Also refactored some logic to make it clearer.


----

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

Author: corymhall

packages/@aws-cdk/core/lib/private/tree-metadata.ts

@@ -85,6 +85,47 @@ export class TreeMetadata extends Construct {
     });
   }
 
+  /**
+   * Each node will only have 1 level up (node.parent.parent will always be undefined)
+   * so we need to reconstruct the node making sure the parents are set
+   */
+  private getNodeWithParents(node: Node): Node {
+    if (!this._tree) {
+      throw new Error(`attempting to get node branch for ${node.path}, but the tree has not been created yet!`);
+    }
+    let tree = node;
+    if (node.parent) {
+      tree = {
+        ...node,
+        parent: this.getNodeWithParents(this._tree[node.parent.path]),
+      };
+    }
+    return tree;
+  }
+
+  /**
+   * Construct a new tree with only the nodes that we care about.
+   * Normally each node can contain many child nodes, but we only care about the
+   * tree that leads to a specific construct so drop any nodes not in that path
+   *
+   * @param node Node the current tree node
+   * @param child Node the previous tree node and the current node's child node
+   * @returns Node the new tree
+   */
+  private renderTreeWithChildren(node: Node, child?: Node): Node {
+    if (node.parent) {
+      return this.renderTreeWithChildren(node.parent, node);
+    } else if (child) {
+      return {
+        ...node,
+        children: {
+          [child.id]: child,
+        },
+      };
+    }
+    return node;
+  }
+
   /**
    * This gets a specific "branch" of the tree for a given construct path.
    * It will return the root Node of the tree with non-relevant branches filtered
@@ -97,24 +138,8 @@ export class TreeMetadata extends Construct {
       throw new Error(`attempting to get node branch for ${constructPath}, but the tree has not been created yet!`);
     }
     const tree = this._tree[constructPath];
-    const newTree: Node = {
-      id: tree.id,
-      path: tree.path,
-      attributes: tree.attributes,
-      constructInfo: tree.constructInfo,
-      // need to re-add the parent because the current node
-      // won't have the parent's parent
-      parent: tree.parent ? this._tree[tree.parent.path] : undefined,
-    };
-    // need the properties to be mutable
-    let branch = newTree as any;
-    do {
-      branch.parent.children = {
-        [branch.id]: branch,
-      };
-      branch = branch.parent;
-    } while (branch.parent);
-    return branch;
+    const treeWithParents = this.getNodeWithParents(tree);
+    return this.renderTreeWithChildren(treeWithParents);
   }
 
   private synthAttributes(construct: IConstruct): { [key: string]: any } | undefined {

packages/@aws-cdk/core/lib/validation/private/construct-tree.ts

@@ -125,16 +125,28 @@ export class ConstructTree {
         } else {
           trace = construct.node.defaultChild?.node.metadata.find(meta => !!meta.trace)?.trace ?? [];
         }
-        // the top item is never pointing to anything relevant
-        trace.shift();
         // take just the items we need and reverse it since we are
-        // displaying to trace bottom up
+        // displaying the trace bottom up
         return Object.create(trace.slice(0, size));
       }
     }
     return [];
   }
 
+  /**
+   * Only the `CfnResource` constructs contain the trace information
+   * So we need to go down the tree and find that resource (its always the last one)
+   *
+   * @param node Node the entire tree where the bottom is the violating resource
+   * @return Node the bottom of the tree which will be the violating resource
+   */
+  private getNodeWithTrace(node: Node): Node {
+    if (node.children) {
+      return this.getNodeWithTrace(this.getChild(node.children));
+    }
+    return node;
+  }
+
   /**
    * Get a ConstructTrace from the cache for a given construct
    *
@@ -149,23 +161,11 @@ export class ConstructTree {
 
     const size = this.nodeSize(node);
 
-    // the first time through the node will
-    // be the root of the tree. We need to go
-    // down the tree until we get to the bottom which
-    // will be the resource with the violation and it
-    // will contain the trace info
-    let child = node;
-    if (!locations) {
-      do {
-        if (child.children) {
-          child = this.getChild(child.children);
-        }
-      } while (child.children);
-    }
-    const metadata = (locations ?? this.getTraceMetadata(size, child));
+    const nodeWithTrace = this.getNodeWithTrace(node);
+    const metadata = (locations ?? this.getTraceMetadata(size, nodeWithTrace));
     const thisLocation = metadata.pop();
 
-    let constructTrace: ConstructTrace = {
+    const constructTrace: ConstructTrace = {
       id: node.id,
       path: node.path,
       // the "child" trace will be the "parent" node
@@ -190,7 +190,7 @@ export class ConstructTree {
   }
 
   /**
-   * Get the size of a Node
+   * Get the size of a Node, i.e. how many levels is it
    */
   private nodeSize(node: Node): number {
     let size = 1;

packages/@aws-cdk/core/test/validation/trace.test.ts

@@ -1,5 +1,6 @@
 import { Construct } from 'constructs';
 import * as core from '../../lib';
+import { Resource, IResource } from '../../lib';
 import { ConstructTree } from '../../lib/validation/private/construct-tree';
 import { ReportTrace } from '../../lib/validation/private/trace';
 
@@ -40,6 +41,20 @@ describe('ReportTrace', () => {
           libraryVersion: expect.any(String),
           location: expect.stringMatching(/new MyStack \(.*\/trace.test.ts:[0-9]+:[0-9]+\)/),
           path: 'MyStack/MyConstruct',
+          child: {
+            id: 'MyL2Resource',
+            construct: expect.stringMatching(/Resource/),
+            libraryVersion: expect.any(String),
+            location: expect.stringMatching(/new MyConstruct \(.*\/trace.test.ts:[0-9]+:[0-9]+\)/),
+            path: 'MyStack/MyConstruct/MyL2Resource',
+            child: {
+              id: 'Resource',
+              construct: expect.stringMatching(/CfnResource/),
+              libraryVersion: expect.any(String),
+              location: expect.stringMatching(/new MyL2Resource \(.*\/trace.test.ts:[0-9]+:[0-9]+\)/),
+              path: 'MyStack/MyConstruct/MyL2Resource/Resource',
+            },
+          },
         },
       });
     } finally {
@@ -73,20 +88,47 @@ describe('ReportTrace', () => {
         libraryVersion: expect.any(String),
         location: "Run with '--debug' to include location info",
         path: 'MyStack/MyConstruct',
+        child: {
+          id: 'MyL2Resource',
+          construct: expect.stringMatching(/Resource/),
+          libraryVersion: expect.any(String),
+          location: "Run with '--debug' to include location info",
+          path: 'MyStack/MyConstruct/MyL2Resource',
+          child: {
+            id: 'Resource',
+            construct: expect.stringMatching(/CfnResource/),
+            libraryVersion: expect.any(String),
+            location: "Run with '--debug' to include location info",
+            path: 'MyStack/MyConstruct/MyL2Resource/Resource',
+          },
+        },
       },
     });
   });
 });
 
-class MyConstruct extends Construct {
+interface IMyL2Resource extends IResource {}
+
+class MyL2Resource extends Resource implements IMyL2Resource {
+  public readonly constructPath: string;
   constructor(scope: Construct, id: string) {
     super(scope, id);
-    new core.CfnResource(this, 'Resource', {
+    const resource = new core.CfnResource(this, 'Resource', {
       type: 'AWS::CDK::TestResource',
       properties: {
         testProp1: 'testValue',
       },
     });
+    this.constructPath = resource.node.path;
+  }
+}
+
+class MyConstruct extends Construct {
+  public readonly constructPath: string;
+  constructor(scope: Construct, id: string) {
+    super(scope, id);
+    const myResource = new MyL2Resource(this, 'MyL2Resource');
+    this.constructPath = myResource.constructPath;
   }
 }
 
@@ -95,6 +137,6 @@ class MyStack extends core.Stack {
   constructor(scope: Construct, id: string, props?: core.StackProps) {
     super(scope, id, props);
     const myConstruct = new MyConstruct(this, 'MyConstruct');
-    this.constructPath = myConstruct.node.path;
+    this.constructPath = myConstruct.constructPath;
   }
 }