Skip to content

Commit

Permalink
chore: don't add the same Aspect more than once (#32880)
Browse files Browse the repository at this point in the history
There is no observable behavior from adding the same Aspect more than
once at the same priority, but we are adding multiple
`AspectApplication`s for it (and then only executing once).

Make the proptest results a bit easier to read by not allowing this in
the first place.

----

*By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Jan 13, 2025
1 parent 5f060f4 commit 3fa5b23
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 2 deletions.
6 changes: 5 additions & 1 deletion packages/aws-cdk-lib/core/lib/aspect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,11 @@ export class Aspects {
* @param options Options to apply on this aspect.
*/
public add(aspect: IAspect, options?: AspectOptions) {
this._appliedAspects.push(new AspectApplication(this._scope, aspect, options?.priority ?? AspectPriority.DEFAULT));
const newApplication = new AspectApplication(this._scope, aspect, options?.priority ?? AspectPriority.DEFAULT);
if (this._appliedAspects.some(a => a.aspect === newApplication.aspect && a.priority === newApplication.priority)) {
return;
}
this._appliedAspects.push(newApplication);
}

/**
Expand Down
16 changes: 15 additions & 1 deletion packages/aws-cdk-lib/core/test/aspect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,20 @@ describe('aspect', () => {
expect(root.visitCounter).toEqual(1);
});

test('Adding the same aspect twice to the same construct only adds 1', () => {
// GIVEN
const app = new App();
const root = new MyConstruct(app, 'MyConstruct');

// WHEN
const asp = new VisitOnce();
Aspects.of(root).add(asp);
Aspects.of(root).add(asp);

// THEN
expect(Aspects.of(root).all.length).toEqual(1);
});

test('if stabilization is disabled, warn if an Aspect is added via another Aspect', () => {
const app = new App({ context: { '@aws-cdk/core:aspectStabilization': false } });
const root = new MyConstruct(app, 'MyConstruct');
Expand All @@ -91,7 +105,7 @@ describe('aspect', () => {
expect(root.node.metadata[0].type).toEqual(cxschema.ArtifactMetadataEntryType.WARN);
expect(root.node.metadata[0].data).toEqual('We detected an Aspect was added via another Aspect, and will not be applied [ack: @aws-cdk/core:ignoredAspect]');
// warning is not added to child construct
expect(child.node.metadata.length).toEqual(0);
expect(child.node.metadata).toEqual([]);
});

test('Do not warn if an Aspect is added directly (not by another aspect)', () => {
Expand Down

0 comments on commit 3fa5b23

Please sign in to comment.