Skip to content

Commit

Permalink
feat(core): warn if an aspect was added via another aspect (#8639)
Browse files Browse the repository at this point in the history
### Updated PR

Since we introduced [`stages`](#8423) which had the unintended side effect of the CDK not supporting adding an Aspect via another Aspect, we have not seen any issues reported beside #8536, that was resolved without requiring this capability.
Given that, and the fact this features has many sharp edges we decided to leave it unsupported, and add a warning in the meantime. 

-----------

Following up on #8536

If an aspect is added via another aspect, the inner aspect  will  not be invoked. Take for example this code:

```typescript
const app = new cdk.App();

app.node.applyAspect({
  visit(construct: cdk.IConstruct) {
    construct.node.applyAspect({
      visit(construct: cdk.IConstruct) {
        console.info("Invoking aspect on construct: " + construct.node.id);  // This will not be called
      }
    })
  }
});
```

Since aspects are added only on the top level node,  if an aspect is added while `InvokeAspects` is called on that node,  it will be ignored since it will not be added to list of aspects to invoke (`allAspectsHere` in the bellow code):
```typescript
function invokeAspects(root: IConstruct) {
  recurse(root, []);

  function recurse(construct: IConstruct, inheritedAspects: constructs.IAspect[]) {
    // hackery to be able to access some private members with strong types (yack!)
    const node: NodeWithAspectPrivatesHangingOut = construct.node._actualNode as any;

    const allAspectsHere = [...inheritedAspects ?? [], ...node._aspects];

    for (const aspect of allAspectsHere) {
      if (node.invokedAspects.includes(aspect)) { continue; }

      aspect.visit(construct); <-- an aspect that was added here will not be added to `allAspectsHere` and will be ignored

      node.invokedAspects.push(aspect);
    }

    for (const child of construct.node.children) {
      if (!Stage.isStage(child)) {
        recurse(child, allAspectsHere);
      }
    }
  }
}
 
```

Assuming this is not something we want to support**, we can detect it by comparing the size of `node._aspects` before and after the call to `aspect.visit`, and emit a warning if there has been a change. Note that while the aspect will not be invoked it will be added to every child construct during the recursive visit. Emitting a warning for each child construct will result in a noisy console, to prevent this I have added a flag that will only allow adding **one warning per application**, given this limitation I'm not sure  there is a lot of value in adding the warning, thoughts?
 
If we decide to add it I will add tests.



 (** theoretically we could support it by adding the aspects to `allAspectsHere`  during the loop, but this will require a non trivial implementation in order to avoid infinite recursion) 



----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
NetaNir authored Aug 3, 2020
1 parent 2451fa9 commit 9d7bef7
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 1 deletion.
10 changes: 9 additions & 1 deletion packages/@aws-cdk/core/lib/private/synthesis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,17 +58,25 @@ function synthNestedAssemblies(root: IConstruct, options: StageSynthesisOptions)
* twice for the same construct.
*/
function invokeAspects(root: IConstruct) {
let nestedAspectWarning = false;
recurse(root, []);

function recurse(construct: IConstruct, inheritedAspects: constructs.IAspect[]) {
// hackery to be able to access some private members with strong types (yack!)
const node: NodeWithAspectPrivatesHangingOut = construct.node._actualNode as any;

const allAspectsHere = [...inheritedAspects ?? [], ...node._aspects];

const nodeAspectsCount = node._aspects.length;
for (const aspect of allAspectsHere) {
if (node.invokedAspects.includes(aspect)) { continue; }

aspect.visit(construct);
// if an aspect was added to the node while invoking another aspect it will not be invoked, emit a warning
// the `nestedAspectWarning` flag is used to prevent the warning from being emitted for every child
if (!nestedAspectWarning && nodeAspectsCount !== node._aspects.length) {
construct.node.addWarning('We detected an Aspect was added via another Aspect, and will not be applied');
nestedAspectWarning = true;
}
node.invokedAspects.push(aspect);
}

Expand Down
46 changes: 46 additions & 0 deletions packages/@aws-cdk/core/test/test.aspect.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { Test } from 'nodeunit';
import { App } from '../lib';
import { IAspect } from '../lib/aspect';
Expand All @@ -17,6 +18,13 @@ class VisitOnce implements IAspect {
}
}
}

class MyAspect implements IAspect {
public visit(node: IConstruct): void {
node.node.addMetadata('foo', 'bar');
}
}

export = {
'Aspects are invoked only once'(test: Test) {
const app = new App();
Expand All @@ -28,4 +36,42 @@ export = {
test.deepEqual(root.visitCounter, 1);
test.done();
},

'Warn if an Aspect is added via another Aspect'(test: Test) {
const app = new App();
const root = new MyConstruct(app, 'MyConstruct');
const child = new MyConstruct(root, 'ChildConstruct');
root.node.applyAspect({
visit(construct: IConstruct) {
construct.node.applyAspect({
visit(inner: IConstruct) {
inner.node.addMetadata('test', 'would-be-ignored');
},
});
},
});
app.synth();
test.deepEqual(root.node.metadata[0].type, cxschema.ArtifactMetadataEntryType.WARN);
test.deepEqual(root.node.metadata[0].data, 'We detected an Aspect was added via another Aspect, and will not be applied');
// warning is not added to child construct
test.equal(child.node.metadata.length, 0);
test.done();
},

'Do not warn if an Aspect is added directly (not by another aspect)'(test: Test) {
const app = new App();
const root = new MyConstruct(app, 'Construct');
const child = new MyConstruct(root, 'ChildConstruct');
root.node.applyAspect(new MyAspect());
app.synth();
test.deepEqual(root.node.metadata[0].type, 'foo');
test.deepEqual(root.node.metadata[0].data, 'bar');
test.deepEqual(root.node.metadata[0].type, 'foo');
test.deepEqual(child.node.metadata[0].data, 'bar');
// no warning is added
test.equal(root.node.metadata.length, 1);
test.equal(child.node.metadata.length, 1);
test.done();
},

};

0 comments on commit 9d7bef7

Please sign in to comment.