Skip to content

Commit

Permalink
chore: don't add the same Aspect more than once
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.
  • Loading branch information
rix0rrr committed Jan 13, 2025
1 parent bd38861 commit 162cfb6
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 162cfb6

Please sign in to comment.