(core): Questioning the Assets visiting order

[cmaster11]

Describe the bug

Hi, I've been using Aspects for a while for various purposes, and I've had a specific issue multiple times. My flow is:

1. I declare tags for some children constructs but not for the entire application or Stack, e.g. I want to apply and enforce a specific tag for SNS topics only:

Tags.of(mySNSTopic).add('MyRequiredTag', 'Hello world!');`)

2. I have an app-level Aspect that performs validations on required tags, e.g.:

export class ValidateSNSTopicAspect implements cdk.IAspect {  
  visit(node: IConstruct) {  
    if (!(node instanceof aws_sns.CfnTopic)) {  
      return;  
    }  
  
    if (!('MyRequiredTag' in (TagManager.of(node)?.tagValues() ?? {}))) {  
      Annotations.of(node).addError(  
        `Missing required tag MyRequiredTag in CfnTopic ${node.logicalId}!`  
      );  
    }  
  }  
}

Now, because of the visiting order forced in https://github.com/aws/aws-cdk/blob/dd912daf2b91a4a32064341e92863afbd9eeebdd/packages/aws-cdk-lib/core/lib/private/synthesis.ts#L226C5-L226C72, outer aspects are invoked BEFORE the ones in their children nodes.

This means that if I add tags on a stack level, they'll be added before my validation Aspect is invoked, but if I declare them in children constructs, they will be added AFTER my validation Aspect is invoked, causing it to fail.

I'm now wondering whether this visiting order is a strong requirement, or if it can be improved by causing Aspects in children nodes to be visited BEFORE the ones in the parents'.

P.S. I know I can apply tags for specific resource types at the Stack level. This post is about the logic behind the order of the visiting. I'm not trying to solve the issue in the example in another way.

Expected Behavior

Aspects in children nodes are visited before the ones in their parent nodes.

Current Behavior

Aspects in children nodes are visited AFTER the ones in their parent nodes.

Reproduction Steps

test('Validate MyRequiredTag aspect in children node', async () => {
  const app = new App();
  const stack = new Stack(app, 'Test');

  const topic = new aws_sns.Topic(stack, 'MyTopic');
  Tags.of(topic).add('MyRequiredTag', 'Hello world!');

  Aspects.of(app).add({
    visit(node: IConstruct) {
      if (!(node instanceof aws_sns.CfnTopic)) {
        return;
      }

      if (!('MyRequiredTag' in (TagManager.of(node)?.tagValues() ?? {}))) {
        Annotations.of(node).addError(
          `Missing required tag MyRequiredTag in CfnTopic ${node.logicalId}!`
        );
      }
    }
  });

  const template = Template.fromStack(stack);
  expect(template).toMatchSnapshot();

  // Expect an ERROR annotation to NOT exist on the SNS topic created with the MyRequiredTag
  expect(
    topic.node.defaultChild!.node.metadata.find(
      (e) => e.type == ArtifactMetadataEntryType.ERROR
    )
  ).toBeUndefined();
});

Possible Solution

Turn

https://github.com/aws/aws-cdk/blob/dd912daf2b91a4a32064341e92863afbd9eeebdd/packages/aws-cdk-lib/core/lib/private/synthesis.ts#L226C5-L226C72

from

 const allAspectsHere = [...inheritedAspects ?? [], ...aspects.all];

to

 const allAspectsHere = [...aspects.all, ...inheritedAspects ?? []];

Additional Information/Context

No response

CDK CLI Version

2.140.0 (build 46168aa)

Framework Version

No response

Node.js Version

v20.10.0

OS

Windows

Language

TypeScript

Language Version

No response

Other information

No response

[pahud]

Thank you for bringing it to our attention. I am not sure what's the logic behind that but this doesn't seem to be a bug to me. I am leaving this for the core team for more inputs.

[cmaster11]

I think this has been fixed by #21341 :D I'm closing it!

[cmaster11]

Nice stuff!!

[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.