fix(assertions): incorrect assertions when >1 messages on a resource (#…

…18948)

Previously we relied on the message id as a key to the internal `messages` object we maintain. However, the id is the construct path, and this is not unique if there are multiple messages attached to a particular construct. This would result in erroneous behavior from `Annotations`, as the newer message would overwrite the old one.

The fix here is to not depend on the id as the key at all. We don't need it, and it's there just to mold the messages into an object that `matchSection` can handle. Instead, we can index on `index`, and suddenly all our problems are solved.

I also redacted the stack trace from the `findXxx APIs; I don't see this as a breaking change because it is unfathomable that anyone is depending on the stack trace output, which is a long list of gibberish.

Fixes #18840. 

----

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

packages/@aws-cdk/assertions/lib/annotations.ts

@@ -102,10 +102,10 @@ function constructMessage(type: 'info' | 'warning' | 'error', message: any): {[k
 }
 
 function convertArrayToMessagesType(messages: SynthesisMessage[]): Messages {
-  return messages.reduce((obj, item) => {
+  return messages.reduce((obj, item, index) => {
     return {
       ...obj,
-      [item.id]: item,
+      [index]: item,
     };
   }, {}) as Messages;
 }

packages/@aws-cdk/assertions/lib/private/message.ts

@@ -1,5 +1,5 @@
 import { SynthesisMessage } from '@aws-cdk/cx-api';
 
 export type Messages = {
-  [logicalId: string]: SynthesisMessage;
+  [key: string]: SynthesisMessage;
 }

packages/@aws-cdk/assertions/lib/private/messages.ts

@@ -1,21 +1,22 @@
-import { MatchResult } from '../matcher';
+import { SynthesisMessage } from '@aws-cdk/cx-api';
 import { Messages } from './message';
-import { filterLogicalId, formatFailure, matchSection } from './section';
+import { formatFailure, matchSection } from './section';
 
-export function findMessage(messages: Messages, logicalId: string, props: any = {}): { [key: string]: { [key: string]: any } } {
-  const section: { [key: string]: {} } = messages;
-  const result = matchSection(filterLogicalId(section, logicalId), props);
+export function findMessage(messages: Messages, constructPath: string, props: any = {}): { [key: string]: { [key: string]: any } } {
+  const section: { [key: string]: SynthesisMessage } = messages;
+  const result = matchSection(filterPath(section, constructPath), props);
 
   if (!result.match) {
     return {};
   }
 
+  Object.values(result.matches).forEach((m) => handleTrace(m));
   return result.matches;
 }
 
-export function hasMessage(messages: Messages, logicalId: string, props: any): string | void {
-  const section: { [key: string]: {} } = messages;
-  const result = matchSection(filterLogicalId(section, logicalId), props);
+export function hasMessage(messages: Messages, constructPath: string, props: any): string | void {
+  const section: { [key: string]: SynthesisMessage } = messages;
+  const result = matchSection(filterPath(section, constructPath), props);
 
   if (result.match) {
     return;
@@ -25,17 +26,26 @@ export function hasMessage(messages: Messages, logicalId: string, props: any): s
     return 'No messages found in the stack';
   }
 
+  handleTrace(result.closestResult.target);
   return [
     `Stack has ${result.analyzedCount} messages, but none match as expected.`,
-    formatFailure(formatMessage(result.closestResult)),
+    formatFailure(result.closestResult),
   ].join('\n');
 }
 
 // We redact the stack trace by default because it is unnecessarily long and unintelligible.
 // If there is a use case for rendering the trace, we can add it later.
-function formatMessage(match: MatchResult, renderTrace: boolean = false): MatchResult {
-  if (!renderTrace) {
-    match.target.entry.trace = 'redacted';
-  }
-  return match;
+function handleTrace(match: any, redact: boolean = true): void {
+  if (redact && match.entry?.trace !== undefined) {
+    match.entry.trace = 'redacted';
+  };
+}
+
+function filterPath(section: { [key: string]: SynthesisMessage }, path: string): { [key: string]: SynthesisMessage } {
+  // default signal for all paths is '*'
+  if (path === '*') return section;
+
+  return Object.entries(section ?? {})
+    .filter(([_, v]) => v.id === path)
+    .reduce((agg, [k, v]) => { return { ...agg, [k]: v }; }, {});
 }

packages/@aws-cdk/assertions/test/annotations.test.ts

@@ -7,12 +7,13 @@ describe('Messages', () => {
   let annotations: _Annotations;
   beforeAll(() => {
     stack = new Stack();
-    new CfnResource(stack, 'Foo', {
+    const foo = new CfnResource(stack, 'Foo', {
       type: 'Foo::Bar',
       properties: {
         Fred: 'Thud',
       },
     });
+    foo.node.setContext('disable-stack-trace', false);
 
     new CfnResource(stack, 'Bar', {
       type: 'Foo::Bar',
@@ -53,12 +54,17 @@ describe('Messages', () => {
   describe('findError', () => {
     test('match', () => {
       const result = annotations.findError('*', Match.anyValue());
-      expect(Object.keys(result).length).toEqual(2);
+      expect(result.length).toEqual(2);
     });
 
     test('no match', () => {
       const result = annotations.findError('*', 'no message looks like this');
-      expect(Object.keys(result).length).toEqual(0);
+      expect(result.length).toEqual(0);
+    });
+
+    test('trace is redacted', () => {
+      const result = annotations.findError('/Default/Foo', Match.anyValue());
+      expect(result[0].entry.trace).toEqual('redacted');
     });
   });
 
@@ -75,12 +81,12 @@ describe('Messages', () => {
   describe('findWarning', () => {
     test('match', () => {
       const result = annotations.findWarning('*', Match.anyValue());
-      expect(Object.keys(result).length).toEqual(1);
+      expect(result.length).toEqual(1);
     });
 
     test('no match', () => {
       const result = annotations.findWarning('*', 'no message looks like this');
-      expect(Object.keys(result).length).toEqual(0);
+      expect(result.length).toEqual(0);
     });
   });
 
@@ -97,19 +103,19 @@ describe('Messages', () => {
   describe('findInfo', () => {
     test('match', () => {
       const result = annotations.findInfo('/Default/Qux', 'this is an info');
-      expect(Object.keys(result).length).toEqual(1);
+      expect(result.length).toEqual(1);
     });
 
     test('no match', () => {
       const result = annotations.findInfo('*', 'no message looks like this');
-      expect(Object.keys(result).length).toEqual(0);
+      expect(result.length).toEqual(0);
     });
   });
 
   describe('with matchers', () => {
     test('anyValue', () => {
       const result = annotations.findError('*', Match.anyValue());
-      expect(Object.keys(result).length).toEqual(2);
+      expect(result.length).toEqual(2);
     });
 
     test('not', () => {
@@ -123,6 +129,53 @@ describe('Messages', () => {
   });
 });
 
+describe('Multiple Messages on the Resource', () => {
+  let stack: Stack;
+  let annotations: _Annotations;
+  beforeAll(() => {
+    stack = new Stack();
+    new CfnResource(stack, 'Foo', {
+      type: 'Foo::Bar',
+      properties: {
+        Fred: 'Thud',
+      },
+    });
+
+    const bar = new CfnResource(stack, 'Bar', {
+      type: 'Foo::Bar',
+      properties: {
+        Baz: 'Qux',
+      },
+    });
+    bar.node.setContext('disable-stack-trace', false);
+
+    Aspects.of(stack).add(new MultipleAspectsPerNode());
+    annotations = _Annotations.fromStack(stack);
+  });
+
+  test('succeeds on hasXxx APIs', () => {
+    annotations.hasError('/Default/Foo', 'error: this is an error');
+    annotations.hasError('/Default/Foo', 'error: unsupported type Foo::Bar');
+    annotations.hasWarning('/Default/Foo', 'warning: Foo::Bar is deprecated');
+  });
+
+  test('succeeds on findXxx APIs', () => {
+    const result1 = annotations.findError('*', Match.stringLikeRegexp('error:.*'));
+    expect(result1.length).toEqual(4);
+    const result2 = annotations.findError('/Default/Bar', Match.stringLikeRegexp('error:.*'));
+    expect(result2.length).toEqual(2);
+    const result3 = annotations.findWarning('/Default/Bar', 'warning: Foo::Bar is deprecated');
+    expect(result3).toEqual([{
+      level: 'warning',
+      entry: {
+        type: 'aws:cdk:warning',
+        data: 'warning: Foo::Bar is deprecated',
+        trace: 'redacted',
+      },
+      id: '/Default/Bar',
+    }]);
+  });
+});
 class MyAspect implements IAspect {
   public visit(node: IConstruct): void {
     if (node instanceof CfnResource) {
@@ -147,4 +200,22 @@ class MyAspect implements IAspect {
   protected info(node: IConstruct, message: string): void {
     Annotations.of(node).addInfo(message);
   }
+}
+
+class MultipleAspectsPerNode implements IAspect {
+  public visit(node: IConstruct): void {
+    if (node instanceof CfnResource) {
+      this.error(node, 'error: this is an error');
+      this.error(node, `error: unsupported type ${node.cfnResourceType}`);
+      this.warn(node, `warning: ${node.cfnResourceType} is deprecated`);
+    }
+  }
+
+  protected warn(node: IConstruct, message: string): void {
+    Annotations.of(node).addWarning(message);
+  }
+
+  protected error(node: IConstruct, message: string): void {
+    Annotations.of(node).addError(message);
+  }
 }