refactor(trace): simplify how construct traces get built (#34355)

This change is motivated by the tree metadata containing a `parent` pointer that used to be stripped before writing it to disk in `tree.json`. I removed the `parent` pointer because shouldn't be in this data structure at all.

The TreeMetadata `parent` pointer was being used to build a `ConstructTrace` for the purposes of reporting template validation errors, so that code had to be refactored as well. It was doing a complicated dance that I honestly didn't quite understand going up and down the construct tree trace and holding cached copies of various data structures in memory.

I removed the dependency of the trace reporting on `TreeMetadata` and replaced it with a path traversal from the root of the construct tree, and progressively building the `ConstructTrace` from the constructs we find along the way: first building a chain of `ConstructTrace` objects without `child` and `location` information, and then adding it on later.

One note on what's left here: 

- As far as I can tell we are peeling apart a stack trace from the lowest-level `CfnResource` into a different single frames, to be reported in the `ConstructTrace`. I *think* I did a faithful conversion, but what I have only works if child constructs are always created in constructors. Our counting of stack frames will be off by one for every method call involved (in other words, if more than 1 stack frame separates 2 levels in the construct tree).

I'm not fixing this right now because that's not what I want to focus on, but this could/should probably be a backlog item?

------

As an example of the latter, let's say this construct tree:

```
StaticWebsite
  |
  +-- Bucket
        |
        +--- CfnResource   <-- stack trace only available here
```

The way we will do this is we'll take the stack trace from `CfnResource`, and use it as follows:

- `stackTrace[0]` -> that must be where CfnResource is created
- `stackTrace[1]` -> that must be where Bucket is created
- `stackTrace[2]` -> that must be where StaticWebsite is created

The above assumptions hold for the following code:

```ts
// static-website.ts
class StaticWebsite {
  constructor() {
    new Bucket(this, 'Bucket');
   }
}

// app.ts
new StaticWebsite();
```

But *don't* hold for the following code:

```ts
// static-website.ts
class StaticWebsite {
  constructor() 
    this.createStaticHtmlBucket();
  }
  private createStaticHtmlBucket() {
    new Bucket(this, 'Bucket');
  }
}

// app.ts
new StaticWebsite();
```

Because the initialization of `StaticWebsite` now comprises 2 stack frames instead of 1.

----

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

Author: rix0rrr

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

@@ -10,17 +10,14 @@ import { ISynthesisSession } from '../stack-synthesizers';
 import { IInspectable, TreeInspector } from '../tree';
 
 const FILE_PATH = 'tree.json';
-type Mutable<T> = {
-  -readonly [P in keyof T]: Mutable<T[P]>;
-};
+
 /**
  * Construct that is automatically attached to the top-level `App`.
  * This generates, as part of synthesis, a file containing the construct tree and the metadata for each node in the tree.
  * The output is in a tree format so as to preserve the construct hierarchy.
  *
  */
 export class TreeMetadata extends Construct {
-  private _tree?: { [path: string]: Node };
   constructor(scope: Construct) {
     super(scope, 'Tree');
   }
@@ -45,15 +42,9 @@ export class TreeMetadata extends Construct {
         .filter((child) => child !== undefined)
         .reduce((map, child) => Object.assign(map, { [child!.id]: child }), {});
 
-      const parent = construct.node.scope;
       const node: Node = {
         id: construct.node.id || 'App',
         path: construct.node.path,
-        parent: parent && parent.node.path ? {
-          id: parent.node.id,
-          path: parent.node.path,
-          constructInfo: constructInfoFromConstruct(parent),
-        } : undefined,
         children: Object.keys(childrenMap).length === 0 ? undefined : childrenMap,
         attributes: this.synthAttributes(construct),
         constructInfo: constructInfoFromConstruct(construct),
@@ -68,16 +59,8 @@ export class TreeMetadata extends Construct {
       version: 'tree-0.1',
       tree: visit(this.node.root),
     };
-    this._tree = lookup;
-
     const builder = session.assembly;
-    fs.writeFileSync(path.join(builder.outdir, FILE_PATH), JSON.stringify(tree, (key: string, value: any) => {
-      // we are adding in the `parent` attribute for internal use
-      // and it doesn't make much sense to include it in the
-      // tree.json
-      if (key === 'parent') return undefined;
-      return value;
-    }), { encoding: 'utf-8' });
+    fs.writeFileSync(path.join(builder.outdir, FILE_PATH), JSON.stringify(tree), { encoding: 'utf-8' });
 
     builder.addArtifact('Tree', {
       type: ArtifactType.CDK_TREE,
@@ -87,76 +70,6 @@ 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
-   * @returns Node the root node of the new tree
-   */
-  private renderTreeWithChildren(node: Node): Node {
-    /**
-     * @param currentNode - The current node being evaluated
-     * @param currentNodeChild - The previous node which should be the only child of the current node
-     * @returns The node with all children removed except for the path to the current node
-     */
-    function renderTreeWithSingleChild(currentNode: Mutable<Node>, currentNodeChild: Mutable<Node>) {
-      currentNode.children = {
-        [currentNodeChild.id]: currentNodeChild,
-      };
-      if (currentNode.parent) {
-        currentNode.parent = renderTreeWithSingleChild(currentNode.parent, currentNode);
-      }
-      return currentNode;
-    }
-
-    const currentNode = node.parent ? renderTreeWithSingleChild(node.parent, node) : node;
-    // now that we have the new tree we need to return the root node
-    let root = currentNode;
-    do {
-      if (root.parent) {
-        root = root.parent;
-      }
-    } while (root.parent);
-
-    return root;
-  }
-
-  /**
-   * 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
-   * out (i.e. node children that don't traverse to the given construct path)
-   *
-   * @internal
-   */
-  public _getNodeBranch(constructPath: string): Node | undefined {
-    if (!this._tree) {
-      throw new Error(`attempting to get node branch for ${constructPath}, but the tree has not been created yet!`);
-    }
-    const tree = this._tree[constructPath];
-    const treeWithParents = this.getNodeWithParents(tree);
-    return this.renderTreeWithChildren(treeWithParents);
-  }
-
   private synthAttributes(construct: IConstruct): { [key: string]: any } | undefined {
     // check if a construct implements IInspectable
     function canInspect(inspectable: any): inspectable is IInspectable {
@@ -177,7 +90,6 @@ export class TreeMetadata extends Construct {
 export interface Node {
   readonly id: string;
   readonly path: string;
-  readonly parent?: Node;
   readonly children?: { [key: string]: Node };
   readonly attributes?: { [key: string]: any };
 

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

@@ -1,7 +1,7 @@
 import { Construct, IConstruct } from 'constructs';
 import { App } from '../../app';
 import { CfnResource } from '../../cfn-resource';
-import { TreeMetadata, Node } from '../../private/tree-metadata';
+import { constructInfoFromConstruct } from '../../helpers-internal';
 import { Stack } from '../../stack';
 
 /**
@@ -65,19 +65,15 @@ export class ConstructTree {
   /**
    * A cache of the ConstructTrace by node.path. Each construct
    */
-  private readonly _traceCache = new Map<string, ConstructTrace>();
   private readonly _constructByPath = new Map<string, Construct>();
   private readonly _constructByTemplatePathAndLogicalId = new Map<string, Map<string, Construct>>();
-  private readonly treeMetadata: TreeMetadata;
+  private readonly root: IConstruct;
 
   constructor(
-    private readonly root: IConstruct,
+    root: IConstruct,
   ) {
-    if (App.isApp(this.root)) {
-      this.treeMetadata = this.root.node.tryFindChild('Tree') as TreeMetadata;
-    } else {
-      this.treeMetadata = App.of(this.root)?.node.tryFindChild('Tree') as TreeMetadata;
-    }
+    this.root = App.of(root) ?? root;
+
     this._constructByPath.set(this.root.node.path, root);
     // do this once at the start so we don't have to traverse
     // the entire tree everytime we want to find a nested node
@@ -110,126 +106,91 @@ export class ConstructTree {
   }
 
   /**
-   * Get the stack trace from the construct node metadata.
-   * The stack trace only gets recorded if the node is a `CfnResource`,
-   * but the stack trace will have entries for all types of parent construct
-   * scopes
-   */
-  private getTraceMetadata(size: number, node?: Node): string[] {
-    if (node) {
-      const construct = this.getConstructByPath(node.path);
-      if (construct) {
-        let trace;
-        if (CfnResource.isCfnResource(construct)) {
-          trace = construct.node.metadata.find(meta => !!meta.trace)?.trace ?? [];
-        } else {
-          trace = construct.node.defaultChild?.node.metadata.find(meta => !!meta.trace)?.trace ?? [];
-        }
-        // take just the items we need and reverse it since we are
-        // 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)
+   * Turn a construct path into a trace
    *
-   * @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));
+   * The trace contains all constructs from the root to the construct indicated
+   * by the given path. It does not include the root itself.
+   */
+  public traceFromPath(constructPath: string): ConstructTrace {
+    // Deal with a path starting with `/`.
+    const components = constructPath.replace(/^\//, '').split('/');
+
+    const rootPath: Array<ReturnType<ConstructTree['constructTraceLevelFromTreeNode']>> = [];
+    const stackTraces: Array<string[] | undefined> = [];
+    let node: IConstruct | undefined = this.root;
+    while (node) {
+      rootPath.push(this.constructTraceLevelFromTreeNode(node));
+      stackTraces.push(this.stackTrace(node));
+
+      const component = components.shift()!;
+      node = component !== undefined ? node.node.tryFindChild(component) : undefined;
     }
-    return node;
-  }
-
-  /**
-   * @param node - the root node of the tree
-   * @returns the terminal node in the tree
-   */
-  private lastChild(node: Node): Node {
-    if (node.children) {
-      return this.lastChild(this.getChild(node.children));
+    // Remove the root from the levels
+    rootPath.shift();
+    stackTraces.shift();
+
+    // Now turn the rootpath into a proper ConstructTrace tree by making every next element in the
+    // array the 'child' of the previous one, and adding stack traces.
+    let ret: ConstructTrace = {
+      ...rootPath[rootPath.length - 1],
+      location: stackLocationAt(rootPath.length - 1),
+    };
+    for (let i = rootPath.length - 2; i >= 0; i--) {
+      ret = {
+        ...rootPath[i],
+        child: ret,
+        location: stackLocationAt(i),
+      };
+    }
+    return ret;
+
+    /**
+     * Get the stack location for construct `i` in the list
+     *
+     * If there is a stack trace for the given construct, then use it and get the
+     * first frame from it. Otherwise, find the first stack trace below this
+     * construct and use the N'th frame from it, where N is the distance from this
+     * construct to the construct with stack trace.
+     *
+     * This assumes all constructs are created without additional functions
+     * calls in the middle.
+     */
+    function stackLocationAt(i: number) {
+      let j = i;
+      while (j < rootPath.length && !stackTraces[j]) {
+        j++;
+      }
+      if (j === rootPath.length) {
+        return STACK_TRACE_HINT;
+      }
+      // Stack traces are closest frame first
+      return stackTraces[j]?.[0 + j - i] ?? STACK_TRACE_HINT;
     }
-    return node;
   }
 
   /**
-   * Get a ConstructTrace from the cache for a given construct
+   * Convert a Tree Metadata Node into a ConstructTrace object, except its child and stack trace info
    *
-   * Construct the stack trace of constructs. This will start with the
-   * root of the tree and go down to the construct that has the violation
+   * FIXME: This could probably use the construct tree directly.
    */
-  public getTrace(node: Node, locations?: string[]): ConstructTrace | undefined {
-    const lastChild = this.lastChild(node);
-    const trace = this._traceCache.get(lastChild.path);
-    if (trace) {
-      return trace;
-    }
+  public constructTraceLevelFromTreeNode(node: IConstruct): Omit<ConstructTrace, 'child' | 'location'> {
+    const constructInfo = constructInfoFromConstruct(node);
 
-    const size = this.nodeSize(node);
-
-    const nodeWithTrace = this.getNodeWithTrace(node);
-    const metadata = (locations ?? this.getTraceMetadata(size, nodeWithTrace));
-    const thisLocation = metadata.pop();
-
-    const constructTrace: ConstructTrace = {
-      id: node.id,
-      path: node.path,
-      // the "child" trace will be the "parent" node
-      // since we are going bottom up
-      child: node.children
-        ? this.getTrace(this.getChild(node.children), metadata)
-        : undefined,
-      construct: node.constructInfo?.fqn,
-      libraryVersion: node.constructInfo?.version,
-      location: thisLocation ?? "Run with '--debug' to include location info",
+    return {
+      id: node.node.id,
+      path: node.node.path,
+      construct: constructInfo?.fqn,
+      libraryVersion: constructInfo?.version,
     };
-    // set the cache for the last child path. If the last child path is different then
-    // we have a different tree and need to retrieve the trace again
-    this._traceCache.set(lastChild.path, constructTrace);
-    return constructTrace;
-  }
-
-  /**
-   * Each node will only have a single child so just
-   * return that
-   */
-  private getChild(children: { [key: string]: Node }): Node {
-    return Object.values(children)[0];
   }
 
   /**
-   * Get the size of a Node, i.e. how many levels is it
-   */
-  private nodeSize(node: Node): number {
-    let size = 1;
-    if (!node.children) {
-      return size;
-    }
-    let children: Node | undefined = this.getChild(node.children);
-    do {
-      size++;
-      children = children.children
-        ? this.getChild(children.children)
-        : undefined;
-    } while (children);
-
-    return size;
-  }
-
-  /**
-   * Get a specific node in the tree by construct path
+   * Return the stack trace for a given construct path
    *
-   * @param path the construct path of the node to return
-   * @returns the TreeMetadata Node
+   * Returns a stack trace if stack trace information is found, or `undefined` if not.
    */
-  public getTreeNode(path: string): Node | undefined {
-    return this.treeMetadata._getNodeBranch(path);
+  private stackTrace(construct: IConstruct): string[] | undefined {
+    return construct?.node.metadata.find(meta => !!meta.trace)?.trace;
   }
 
   /**
@@ -253,3 +214,5 @@ export class ConstructTree {
     return this._constructByTemplatePathAndLogicalId.get(templateFile)?.get(logicalId);
   }
 }
+
+const STACK_TRACE_HINT = 'Run with \'--debug\' to include location info';

packages/aws-cdk-lib/core/lib/validation/private/report.ts

@@ -212,7 +212,7 @@ export class PolicyValidationReportFormatter {
                 resource.resourceLogicalId,
               )?.node.path;
               return {
-                constructStack: this.reportTrace.formatJson(constructPath),
+                constructStack: constructPath ? this.reportTrace.formatJson(constructPath) : undefined,
                 constructPath: constructPath,
                 locations: resource.locations,
                 resourceLogicalId: resource.resourceLogicalId,