feat(aspects): priority-ordered aspect invocation

[sumupitchayan]

Closes #21341

This feature was designed in RFC648

Reason for this change

The current algorithm for invoking aspects (see invokeAspects in synthesis.ts) does not handle all use cases — specifically, when an Aspect adds a new node to the Construct tree and when Aspects are applied out of order.

Description of changes

This PR introduces a priority-based ordering system for aspects in the CDK to allow users to control the order in which aspects are applied on the construct tree. This PR also adds a stabilization loop for invoking aspects that can be enabled via the feature flag @aws-cdk/core:aspectStabilization - the stabilization loop ensures that newly added Aspects to the construct tree are visited and nested Aspects are invoked.

Description of how you validated changes

Plenty of unit tests - see aspects.test.ts.

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]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[rix0rrr]

I think this PR implicitly changes iteration order of different aspects at the same priority level.

Haven't decided yet how big of a deal that is. In principle we don't make any guarantees either way, so one could argue that it doesn't matter. In practice, as usual, it could be that someone somewhere has taken a dependency on that specific order (if only by accident)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.46%. Comparing base (0e79874) to head (ed93f22).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #32097      +/-   ##
==========================================
+ Coverage   78.42%   78.46%   +0.04%     
==========================================
  Files         106      106              
  Lines        7208     7208              
  Branches     1323     1323              
==========================================
+ Hits         5653     5656       +3     
+ Misses       1368     1364       -4     
- Partials      187      188       +1     

Flag	Coverage Δ
suite.unit	78.46% <ø> (+0.04%)	⬆️

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

Components	Coverage Δ
packages/aws-cdk	78.46% <ø> (+0.04%)	⬆️

[rix0rrr]

Good stuff! Nearly there. I have some comments on the documentation and some implementation cleanup, but after that I'm good to ship.

A question of a somewhat sizeable set of remaining work is how we're going to add priorities to our existing aspects. It needs to happen before our next release. But at the same time, applying priorities to all aspects we currently have the in code base probably changes infrastructure for someone somewhere.

Thoughts?

[rix0rrr]

💅 Nit: 500 seems like a more logical choice?

Also, these are not public constants, they are on a class, aren't they?

[rix0rrr]

We should be applying correct priorities to all aspects we apply in the construct library. Are we doing that in this PR? If not, it should be an extremely fast follow.

[sumupitchayan]

Also, these are not public constants, they are on a class, aren't they?

Yes I'll fix this and will change it to 500

We should be applying correct priorities to all aspects we apply in the construct library. Are we doing that in this PR? If not, it should be an extremely fast follow.

I can do it here or in a fast-follow. To respond to your point above Aspect priorities changing infrastructure, I've searched the CDK repo for Aspect applications (Aspects.of(<scope>).add()) and every application I found seems to be a mutating Aspect - none that are validation/readonly.

So I'm not sure if that'll change customers' infrastructure ... (I guess it potentially could if we define these as mutating and they run before Aspects with the default priority value).

[rix0rrr]

Let's do it!

[rix0rrr]

I'll give you a conditional approval given that you address everything in my previous comment

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: ed93f22
• 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.