feat(core): warn if an aspect was added via another aspect

[NetaNir]

Updated PR

Since we introduced stages 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:

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):

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

[eladb]

Please add a unit tests

[NetaNir]

Please add a unit tests

@eladb Before I add a test, I just want to make sure we are good with the current behavior since the stage PR introduce a change in the way aspects are invoked.

Prior to the change, if AspectB was added via AspectA (meaning when AspectA visit is called), AspectB will be invoked on the node AspectA was applied to and all of its children. In the bellow code, both AspectA and AspectB will be invoked on the root and the child constructs.

class AspectB implements IAspect {
  public visit(construct: IConstruct): void {}
}

class AspectA implements IAspect {
  public visit(construct: IConstruct): void {
    construct.node.applyAspect(new AspectB())
  }
}

const app = new App();
const root = new Construct(app, 'MyConstruct');
root.node.applyAspect(new AspectA());
const child = new Construct(root, 'ChildConstruct');

After the change AspectB will not be invoked on the root or the child constructs.

[eladb]

I am okay with detecting this and issuing a warning.

[rix0rrr]

We can be more specific than the vague "may result in unexpected behavior."

How about something like?

Aspects added by other Aspects will not be applied; add all Aspects before 'synth' is called.

[rix0rrr]

If we wanted to go with restoring the original behavior, it would look something like this:

const appliedHere = new Set<IAspect>();
let allAspectsHere: IAspect[];
while (true) {
  allAspectsHere = [...inheritedAspects ?? [], ...node._aspects];
  const yetToApply = allAspectsHere.filter(x => !appliedHere.has(x));
  if (yetToApply.length === 0) { break; }
  
  for (const aspect of yetToApply) { 
    // ...
    appliedHere.add(aspect);
  }
}

recurse(child, allAspectsHere);

[NetaNir]

@eladb @rix0rrr I think we should restore the previous behavior, if only to maintain backward compatibility, if no objection I'll create the PR.

[eladb]

No objections

[eladb]

? Any updates?

[NetaNir]

on it!

[eladb]

Any updates?

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 8e264a7
• 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).