An aspect system for the construct tree

[rix0rrr]

(Follow-up discussion in response to #1007)

We should add an Aspect system to the construct tree, doing similar things as
Entity/Component/System systems do in games. (I'm mostly ignoring the System
part of ECS in the rest of this discussion)

Bear with me on (inconsistent use of) the proper names, and I'm open to suggestions.

Background

EC systems have the same purpose in game/physics modeling as they do in our system: make sense of a mess of objects that
share common functionality, but for which it becomes problematic to derive a correct inheritance hierarchy. For example, a game
consists of many different "objects" in a very large list of objects, and on every game tick:

• All the objects that can have physics applied to them should move.
• All the objects that can collide should do so.
• All the objects that have AI applied to them should do so.
• All the objects that are controlled by the player have controls applied to them.
• Etc.

Modeling these different aspects every object can have correctly requires a complex class hierarchy,
and sometimes even becomes inexpressible.

In our system, we have something something similar: we have a giant tree of objects (similar to the list
in game land, but hierarchically organized for added fun) that we can walk over, and we have several behaviors
that a construct can have:

• Some can be rendered to CloudFormation
• Some can accept Tags
• Some can have dependencies added to them, or retention policies changed.

Motivation

Because we have encapsulation, if we want to go the "classic OO" route, if we wanted to expose all the
functionality of underlying constructs, we'd have to repeat all features of every encapsulated construct
at every intermediate constructs.

For example, in the following example, even though Bucket and Pipeline are
the only Taggable objects (let's say exemplified by the present of a
setTags() method), SecretBucket and BestPipeline would need to both
implement this method and forward to the correct subobjects.

┌─────────────┐
│BestPipeline │
└─────────────┘
       │
       │   ┌─────────────┐
       ├──▶│SecretBucket │
       │   └─────────────┘
       │          │
       │          │    ┌────────────────┐
       │          └───▶│     Bucket     │
       │               └────────────────┘
       │
       │   ┌─────────────┐
       └──▶│  Pipeline   │
           └─────────────┘

Even though this is strictly correct in an OO sense, there is a certain burden
associated with doing this forwarding work that will make it harder for users
to write "correct" constructs (as there will be a lot of busywork involved with
doing it), and so the most probable end result will be that users writing
constructs won't consider all potential uses which will make it unnecessarily
hard (or even impossible) to reuse their constructs.

This will probably lead to user frustration and fragmentation of the construct
ecosystem that we're hoping to foster.

Benefits

The essense of a component system is to disentangle certain aspects of an
object's behavior from its identity (whereas typical OO modeling would tie
those together).

It allows us to express commands like:

• For all physics objects, do physics.
• For all renderable objects, draw them onto this surface.

In our case we have the added constraint that we want to select
a subtree to apply our operations to.

• For all taggable objects in this subtree, set these tags.
• For all renderable objects in this subtree, render to CloudFormation.
• For all objects that have a monitoring pack, collect all monitoring packs.

Current Situation

Our tree model currently has provisions for iterating over all children of
a subtree. However, we have no generalized model for applying an operation
to all nodes it makes sense to apply this operation to. Right now, we
do it in an ad-hoc fashion:

• For CloudFormation rendering, we recurse through the tree top-to-bottom,
  and if a construct happens to have a certain method, we'll call it.
• For tags rendering we go the opposite way, iterating up the tree from a leaf to find
  constructs that happen to have a certain public property and if it exists,
  assume it's a set of tags that also need to be applied to us.

Never mind that the mechanism we have for tags doesn't even allow specifying
tags on an arbitrary part of the tree: it HAS to be on a construct that is
already tag-aware, otherwise there is no construct.tags.setTag() attribute to
even be called.

Proposed Implementation

We already have most of the tools necessary to achieve this solution. The only
thing we need to do is an composite way of encoding aspects to a Construct,
and some mechanism to walk the tree to do something to every construct that
a certain aspect applies to. Let's call those "visitors" for now, but we could
call them something more memorable as well (aspectors, twiddlers, what have
you). We have the question of how to match up the visitor with the aspect,
given that things like instanceof are not reliable. A string indicating
a type should do, it seems simple enough.

The following mechanism would do it:

class Construct {
    public readonly aspects: IAspect[];
    public apply(visitor: IAspectVisitor);
}

interface IAspect {
    readonly type: string;
}

interface IAspectVisitor {
    readonly aspectType: string;
    visit(aspect: Aspect);
}

Hopefully it's clear enough how apply() might work to satisfy the goals
we want.

(Normally we'd use generics to make aspect visiting typesafe, but since we don't
have generics we're going to have to do typecasting in the visitor. An
acceptable compromise.)

Example 1: Tags

So if we want to apply tags to a subtree, we can now do this (assuming some
sort of TaggableAspect class):

class Tag implements IAspectVisitor {
    readonly aspectType = 'taggable';

    constructor(private readonly name: string, private readonly value: string) {
    }

    public visit(aspect: Aspect) {
        (aspect as TaggableAspect).setTag(this.name, this.value);
    }
}

How would using TaggableAspect look from the perspective of a taggable Resource?

class Bucket {
    public readonly aspects = new Array<IAspect>();
    private readondly taggable = new TaggableAspect();

    constructor(...) {
        this.aspects.push(this.taggable);
    }

    public toCloudFormation() {
        return {
            ...
            tags: this.taggable.render()
        };
    }
}

Example 2: Rendering CloudFormation (and other synthesis)

For example:

// Resource (for example, not saying this is what we would actually
// do because we can be more typesafe, but you get it)
class Resource {
    public readonly aspects = new Array<IAspect>();
    private properties: any = {};

    constructor(...) {
        this.aspects.push(new CloudFormationRendering(this.properties));
    }
}

And in Stack we'd go:

class Stack {
    public toCloudFormation() {
        // ...
        const collect = new CollectCloudFormation();
        this.apply(collect);
        return collect.template;
    }
}

Laziness

The apply() statement as modeled right now will be applied as soon as it's
called, which (though very simple, which is a plus) requires its users to be
aware of ordering, and makes it possible to make mistakes against ordering.

I'd prefer it if we keep the CDK constructs as declarative as possible, with
as little potential to make ordering mistakes as possible.

For example:

class MyConstruct {
    constructor(...) {
        super(parent, id);

        this.apply(new Tag('name', 'Stewart'));

        // Should "Child" have the tag applied? In a declarative world, yes.
        // In an ordered world, nya nya, you should have put the statements
        // in a different order.
        new Child(this, 'Child');
    }

    public addChild() {
        // What if children get added in a method and paying attention to
        // ordering in the constructor is not good enough anymore? Do
        // we now force the parent construct to do more work to retag
        // this child? 
        new Child(this, 'Child');
    }
}

// But what if it's the consumer?
const constr = new MyConstruct();
constr.apply(new Tag('color', 'yellow'));
constr.addChild();  // Did I mean for this child to be tagged?

A simple extension is make a reactive visitor system, where apply() recurses
over the children immediately, but constructs also remember all visitors that
are applied to them and will apply the visitors to any new children that get
added later. This pattern crops up a number of times already in the CDK code,
since in many places we want to be invariant against the order in which things
happen (ELBv2 example: first adding a Target to a TargetGroup and then adding
the TargetGroup to a Listener, vs first adding a TargetGroup to a listener and
then adding a Target to it -- in both cases do we want to associate open up the
appropriate SecurityGroup ports, and so an (unformalized) reactive system is in
place).

If desired, we could even make this behavior configurable:

construct.apply(new Tag('name', 'value'), {
    applyToFutureChildren: false    // Default would be true
});

[rix0rrr]

cc @RomainMuller @eladb @moofish32 for opinions

[eladb]

[+ @clareliguori @sam-goodwin]

A few things come to mind:

I love the game-system analogy. I think it's apt. You mention that the CDK is a bit different since it's organized as a hierarchy, but game worlds are also organized as a hierarchy! Anyway - totally relevant.

I am wondering how this interplays with what we called "Aspects" in #282. I think there's a connection but there are also a few use cases there that I am not sure this addresses? Should we merge those up?

My main feedback about the API design is that I think there's a use case to apply an aspect without explicit support from the target (i.e. I'd like to apply an aspect that traverses the tree and calculates the cost of stack). So maybe instead of IAspectVisitor#aspectType it should be IAspectVisitor#shouldApply(construct). This way, the Tag visitor would just check if the construct has a taggable property. In a sense it's more "strongly-typed".

I wonder if this removes the need to maintain a list of aspects at the Construct level. But maybe it's still useful as a general purpose mechanism... Not sure. If we leave it, can it be protected or even just encapsulated altogether by Construct (i.e. construct.addAspect(new TaggableAspect(this))).

visit should probably also accept the target Construct (I am sure that will become handy).

Laziness: I like to think about this as "attach" instead of "apply", and definitely vote for the declarative model (any order-dependent model will not hold water in my mind). The reactive solution sounds shaky... I think a phased approach would work better - perhaps we devise a phase model where aspects say in which phase they expect to be applied (currently we have one phase: "synthesis", but maybe we needed two). Maybe that's where this is connected to #282...

[rix0rrr]

The advantage of the reactive method is that it will be invariant against ordering even if the visitors were to add new children. Any non-reactive system wouldn't be

[moofish32]

First, from the tags aspect I think this makes a lot more sense. I've really struggled with tags without aspects. For some reason as I designed tags I thought we wanted to avoid putting more intelligence on Construct so I didn't go after this solution at all. However, looking through PR comments and notes I don't see it written down. So I definitely think the ECS approach will be cleaner.

I do think we are going to give up some things.

1. IDE's (VIM for the people with good taste) will not be able to autocomplete and inform users that resources support tags.
2. We are moving further from the known CFN schema. One thing that has frustrated some early adopters (and even myself a few times) is I know what the property name is in CFN, but I can't find it in CDK. Aspect concepts will definitely increase the cognitive load for many users that might be more Ops background and less dev. (I'm still in favor, but I think this is worth calling out).

I agree with @eladb, that the reactive model is shaky right now. For example, removing a tag is quite difficult with the reactive model we have today. Now I might also argue that use case should not happen very often, but I just don't have any data for it either way. The concept of phases does make sense to me, but I don't have any good feel for what we will have besides synthesis?

The other part of tags that seems to get ugly in code is what to do about initialization. Once we allow two entry points we have to resolve conflicts. For example, if a parent has a tag added after the L1 is initialized, which tag takes precedence? The reverse is also possible. L1s also support a few input formats, I haven't seen a situation where CDK alters the L1 types, but life would be easier for CDK if tag inputs were standardized. ASG will likely be a snow flake, but that's fine for just one special case.

I'm overall in favor of this design, but I definitely expect a few users that will disagree. There are definitely plenty of valuable use cases, including pre-deploy security checks that have been discussed in other threads and can be addressed here.

[eladb]

The advantage of the reactive method is that it will be invariant against ordering even if the visitors were to add new children. Any non-reactive system wouldn't be

I think the phased approach might be a good compromise. I realize it won’t solve the problem in the general case but I think it should suffice.

——-

I was also wondering about CloudFormation rendering- is this really an aspect? I am not sure!

[rix0rrr]

I was also wondering about CloudFormation rendering- is this really an aspect? I am not sure!

I think any behavior that we want to apply to a swath of constructs (but only the ones that support it) is an aspect. Or end goal was to be cloud agnostic, so we should be able to mix AWS and GCP constructs in one tree, for example. What is the fact that some constructs render to CFN and others don't, if not an aspect?

But more general than CFN vs GCP, some constructs "synthesize themselves" and others don't. An aspect (command) could totally be "synthesize yourself to this directory".

[rix0rrr]

If we want to do a definite aspect ordering, I feel it should be top-down so that aspects that apply in-place mutations (such as tags) applied lower in the tree override higher-level aspects without extra work.

[eladb]

I think any behavior that we want to apply to a swath of constructs (but only the ones that support it) is an aspect. Or end goal was to be cloud agnostic, so we should be able to mix AWS and GCP constructs in one tree, for example. What is the fact that some constructs render to CFN and others don't, if not an aspect?

But synthesizing an output is not "applying a behavior to a construct". It actually expects a result. In a sense, it's a synchronous operation and not just "attaching" a behavior to the tree. I feel those are things are not the same.

If we want to do a definite aspect ordering, I feel it should be top-down so that aspects that apply in-place mutations (such as tags) applied lower in the tree override higher-level aspects without extra work.

Isn't this a per-aspect behavior (and I even think that for tagging we should support both)?

[rix0rrr]

I guess if we don't do "constructs have aspect objects" but feature detecting by means of "constructs have public members that we test against", this all works happily and fine in JavaScript, but how will this translate to JSII languages?

• Can people write constructs with aspects in JSII?
• Can people write visitors in JSII?

For the former, can we feature-detect public methods in Java from JavaScript?

For the latter, can someone in Java feature-detect a public method in JavaScript?

I think the answer to say "both of these can only be written in JavaScript" is not good enough.

[rix0rrr]

Isn't this a per-aspect behavior (and I even think that for tagging we should support both)?

Yes, I suppose you're right. We do need to decide on a visiting order though, top/down or bottom/up. I was trying to think of reasons to prefer one over the other.

[rix0rrr]

But synthesizing [...] actually expects a result.

Sure. I guess I was thinking of the mechanism (tree walking and applying some code to every construct) moreso that the naming and timing. The generic walking is still a useful feature to have, to implement these.

Maybe the answer is that aspects are implemented using the tree walker?

[eladb]

Yes, tree walking is useful, but I am not sure that interesting as a shared utility. Given a node with children, implementing a simple DFS/BFS is quite trivial. Without jsii supporting lambdas, a generic tree walker would require too much ceremony in my mind, but I don't object to it.

Bottom line, I think we should leave synthesis out of this story for now besides aiding in defining the application phases.

[moofish32]

I was avoiding asking this question, but I do want to ask it now that the conversation has shifted. IConnectable is this an aspect? It meets a lot of the discussion above

1. Applies to many constructs
2. Is a mutation that is a not a sync call

I'll be honest I don't really like the idea of it as an aspect because I want clear control of Security Groups.

[moofish32]

I guess if we don't do "constructs have aspect objects" but feature detecting by means of "constructs have public members that we test against", this all works happily and fine in JavaScript, but how will this translate to JSII languages?

• Can people write constructs with aspects in JSII?
• Can people write visitors in JSII?

For the former, can we feature-detect public methods in Java from JavaScript?

For the latter, can someone in Java feature-detect a public method in JavaScript?

I think the answer to say "both of these can only be written in JavaScript" is not good enough.

I was surprised in Typescript that public static members of a child class are not available in the parent during super(parent) for the constructor. So that often means you can't do any prep work in cloudformation.Resource constructor based on public members in the child L1.

[moofish32]

ping - @rix0rrr @eladb - should we revisit this soon?

[sam-goodwin]

The reactive solution sounds shaky... I think a phased approach would work better - perhaps we devise a phase model where aspects say in which phase they expect to be applied (currently we have one phase: "synthesis", but maybe we needed two).

Are there any use-cases for aspects running during or after synthesis? It seems to me that aspects all run prior to synthesis, perhaps the term 'phase' is overloaded here.

I agree with @eladb, that the reactive model is shaky right now. For example, removing a tag is quite difficult with the reactive model we have today.

It's not clear to me what the objections to reactive application of aspects are. How is removing a tag complicated by a reactive model? What does it mean to be declarative in this context? If you can both add and remove a tag then order and scope matters, and I don't see how phases help us here. If we guarantee a predictable order of application for existing and future children with a reactive model, is that not the simplest behavior to reason about?

[rix0rrr]

I think we should move for implementing this, even though we haven't quite specced out all the use cases it could be used for.

Yes, tree walking is useful, but I am not sure that interesting as a shared utility. Given a node with children, implementing a simple DFS/BFS is quite trivial.

The aspect system I'm proposing is composed of 2 things:

• A tree walking standard
• A place on tree nodes to declaratively attach such tree walkers. In effect, a way to attach arbitrary behavior in a way that looks like data to arbitrary nodes in a meaningful way.

Right now, it can be used to simplify the implementation of Tags, and my spidey senses are saying we can find other uses for it in the future.

[moofish32]

I'm going to try to answer a few questions from above and propose a slight modification of this proposal with some specifics about tags.

Laziness
The apply() statement as modeled right now will be applied as soon as it's
called, which (though very simple, which is a plus) requires its users to be
aware of ordering, and makes it possible to make mistakes against ordering.

I'd prefer it if we keep the CDK constructs as declarative as possible, with
as little potential to make ordering mistakes as possible.

I think the shaky part that @sam-goodwin is asking me about is really captured here. If apply triggers as soon as it is called we are going to have to code for a lot of challenges and non-intuitive situations. For example, should the result of the following two cases be different?

parent.apply(AddTag('hello', 'value'))
child.apply(RemoveTag('hello'))

and

child.apply(RemoveTag('hello'))
parent.apply(AddTag('hello', 'value'))

I would like these to both result in the same situation that parent has tag hello and child does not. If we move apply to synthesis instead of when called I think we can resolve the above examples to the same answer. This also means we move tag resolution to walk the tree from root to leaf instead of the current leaf to root.

Precedence

What happens when tags disagree. The example above is a simple use case. In the first design I tried to provide complete set of controls to the developer for precedence and I think that created an overcomplicated solution. Three options:

1. Should we pick a precedence and not enable the user to toggle this?
2. Should we enable some control, but not full control (such as setting a precedence integer?)?
3. Should we enable full control via tag properties to specify the potential situations?

In my opinion full control is too complex and not necessary for the 80 % use case. I currently took more of a full control option and I think we should eliminate it. This means removing sticky and overwrite from the current tag-manager, propagate still makes sense and should default true.

The simplified rules for precedence then become:

1. Closest to me wins
2. Last one in wins

Closest to me is the construct has the highest priority and in a cascading fashion delegates that priority to it's nearest parent. If the construct has a tag set multiple times the last one in wins. I'm voting to not enable further user control of precedence, but I'm open to hearing use cases.

Removal

Removal in the current implementation has a special RemoveTagProps I think we should kill that and consolidate to just one interface with two aspects Tag and RemoveTag that accept the same props interface. Propagating a remove, will remove it from all children, not propagating a remove will allow the tag to propagate to children. I think that is clear enough.

Features being removed

• Ability to tag an object and allow your parent to override
• Ability to write a tag only if it does not exist locally already
• Discoverability in IDE related to specific objects that support tags

I don't think these issues are deal breakers most developers can either write unit tests or synth templates and inspect to understand what is going to happen.

Edit

Many of these ideas came from a conversation with @rix0rrr. So if you like them, they were his. If you don't, yell at me 😀

[moofish32]

Alright, back from vacation and I have a few more questions/decisions to talk about.

Aspect API

/**
 * Aspects that can be applied to Constructs.
 */
export interface IAspect {
  readonly type: string;
  visit(node: Construct): void;
}

The main change here is that I am handing the Construct to the Aspect on visit. I think this makes the most sense, but we didn't have that design above. The reason I need the Construct for tags is I need to know about the Construct in order to determine precedence. For example, if there are 4 aspects trying to apply a tag, which one is the closest to the Construct or which one was the last one applied is difficult to answer unless I have the Construct and the list of aspects (in order).

Alternatively I did consider making Aspect extend Construct, but fundamentally I didn't think that was a good idea and ergonomically it is a bit rough.

myConstruct.apply(new SetTag('MyKey', 'MyValue'));
# versus
myConstruct.apply(new SetTag(myParent, 'MyPathId', { key: 'MyKey', value: 'MyValue' }));

Propagate

Propagate false is an interesting scenario. If the tag is applied to an L2 then there are literally no tags that could possibly be applied to because the tags are on the children L1s. I think it makes sense that even when propagate is false if the Construct is not a Resource (L1) that we propagate the tag to any direct children resources. This logic is a bit ugly, but looks like

Propagate to Resources Only

if propagate
  then apply to all children
else if node is not a Resource
  then filter all children that are Resources
  apply to all children resources

This sounds ok, but my next question is should the tag propagate until it hits a resource or the leaf node? Obviously in our current implementation this should almost always have a Resource in the path. This would change the pseudo code to something like:
Propagate to at least one Resource

if propagate
  then apply to all children
else if node is not a Resource
  then apply to all children // regardless of resource or not

This situation has some interesting results regarding our current VPC and Subnet L2s.

├── VpcNetwork(L2)
│   ├── VPC (L1)
│   ├── IGW (L1)
│   ├── IGW Attachment (L1)
│   ├── Subnet (L2) [array]
│   │   ├── Subnet (L1)
│   │   ├── NAT (L1)
│   │   ├── Route Table (L1)
│   │   ├── Route Table Association (L1)
│   │   ├── Routes (L1)
│   │   ├── EIP for NAT (L1)

For Propagate to Resources Only the tag appears on VPC and IGW. For Propagate to at least one Resource the situation results in the same behavior as propagate: true.

Now we can say these constructs are unique, and we can override the visit function to customize this tag propagation on the L2 for subnet and vpc. However, it does bring up that the tree structure we choose can impact us. For example, should the L1 VPC actually be the parent for IGW and attachment? Should the L2 subnet have the L1 VPC as a parent? Should the EIP belong to the NAT? When I was coding this it wasn't even something I considered, but now I'm wondering if we shouldn't be aware. If we move these around now we create some very ugly upgrade paths due to logical IDs changing.

Which propagation strategy do you prefer from above? Or is there another option here? If we do nothing propagate: false on a non-resource just disappears.

Resource Public API change

When we flip tag propagation and resolution to happen at synthesis we have to open up the resource public API a bit, specifically properties need to be modified.

/**
 * Represents a CloudFormation resource.
 */
export class Resource extends Referenceable {
 // snip

  /**
   * AWS resource properties.
   *
   * This object is rendered via a call to "renderProperties(this.properties)".
   */
  public readonly properties: any;
// snip

I don't see any way around this if we are resolving during synthesis. I considered using overrides, but that seems like a horrible idea since overrides should not normally be needed. 👎

The other option is make renderProperties check aspects for tags. I didn't like this because now Construct and Resource will both know about Aspect. I felt that this was leaking a dependency out that I was unnecessary if I could only modify the properties.

Synthesis Time

I have two options to discuss here.

1. Have app invoke visit() cascading down the tree
2. Have app apply it's aspects to it's direct children (stacks). Let stacks cascade the visit() call right after locking in the toCloudFormation() call.

I prefer the second option because this means during testing you don't have to manually trigger visit() on your test stack. However, I could argue that 1 is a more pure solution.

I have a good portion of this coded and working, but I think these key decisions should be discussed. I plan to clean up the original PR tonight and push up some code to help this discussion.

Your feedback is greatly needed on the topics above.

[rix0rrr]

Welcome back! Same for me :).

The main change here is that I am handing the Construct to the Aspect on visit.

Agreed this is correct. Before we had IAspect as the "visited thing", but we can probably do without it and visit the constructs directly.

The reason I need the Construct for tags is I need to know about the Construct in order to determine precedence.

Not sure you need it for that reason. I'm still favoring order implied in the visiting mechanism, so the work that is done in visit() could be simple direct mutation. On the other hand, nothing prohibits doing aspect queries on the Construct in question, like Construct.getApplicableAspects() or something, which might imply some ordering. But given that visit() will be called for every applied aspect, and if every aspect is going to look at all other aspects, they need to code defensively against that and I'm not sure it would appreciably simplify the logic.

Alternatively I did consider making Aspect extend Construct, but fundamentally I didn't think that was a good idea

Agreed.

Also, I think I would favor SetTag simply being called Tag, so it would be:

construct.apply(new Tag('key', 'value'[, options]));
construct.apply(new RemoveTag('key'));

I'm optimizing the 99% case of applying tags and never removing any, over naming consistency between the classes.

Propagate false is an interesting scenario. If the tag is applied to an L2 then there are literally no tags that could possibly be applied to because the tags are on the children L1s.

Which makes me think this is actually not an useful feature to support. If propagate=false, you need to set the tag on a Resource directly to have any effect.

But in that case, you might as well have set propagate=true; it would have had the same effect since a Resource never has children.

Or is propagate=false intended more to support setting tags on VpcNetwork, leading to tags on AWS::EC2::VPC and not on any of the ancillary helper classes in it like Subnet? So in effect, is what we're trying to say:

"Set tags the PRIMARY RESOURCE of this compound construct, and not any of the others" ?

Seems like propagate is just a single mechanism to achieve that goal, and there might be other mechanism to explore to achieve the same ends. For example, we might consider selectors (a la jQuery/CSS).

When we flip tag propagation and resolution to happen at synthesis we have to open up the resource public API a bit, specifically properties need to be modified.

I'm not sure why this is true. The logic you have proposed in the PR (a (generated) Resource knows whether it's taggable or not and renders the tags as part of its toCloudFormation()) seems solid to me. I'm imagining something like the following:

// Taggable resource is identified by having a 'tags' property of type 'TagManager'.
// 'tags' is used in renderProperties.
//
// This is all generated by cfn2ts.
class TaggableGeneratedResource extends Resource {
    public readonly tags: TagManager;

    public renderProperties() {
        return {
            // ...,
            tags: tags.render(singleTagRenderingFunction)
        };
    }
}


// An aspect calsl the methods on 'tags'.
class Tag implements IAspect {
    constructor(private readonly key: string, private readonly value: string) {
    }

    public visit(construct: Construct) {
        if (TagManager.isTaggable(construct)) {
            construct.tags.set(this.key, this.value);
        }
    }
}


interface ITaggable {
    tags: TagManager;
}

// TagManager has a static function to do some duck typing 
class TagManager {
    // Anything that has a 'tags' property is probably taggable.
    // All hail Javascript
    public static isTaggable(x: any): x is Taggable {
        return x.tags !== undefined;
    }

    public set(key: string, value: string) {
        // ...
    }

    public remove(key: string) {
        // ...
    }

    public render(singleTagRenderCallback: (t: Tag) => any) {
        // ...
    }
}

Have app invoke visit() cascading down the tree

I prefer this since aspects are a concept that exceeds stack contents.

If we have the function:

app.applyAspects();

That is not too big of a hassle to call inside tests that test aspects (imo).

[rix0rrr]

I have not explored the idea of having selectors any more in my head yet, other than mentioning it above. It would be worthwhile to think about what that might look like.

It might be as simple as adding a decorator class:

class RestrictAspect implements IAspect {
    constructor(private readonly inner: IAspect, private readonly pathPattern: string) {
    }

    public visit(construct: Construct) {
        if (this.pathMatches(construct.path)) {
            this.inner.apply(construct);
        }
    }
}


// To be used as:
vpc.apply(new RestrictAspect(new Tag('key', 'value'), '*/Resource');

This is a rough sketch, to make it really ergonomic we probably need more information from the framework, such as where the tag was defined (so we can do relative resource paths) or (gasp) construct class names so we can select on those in addition to selecting on ids.

EDIT:

Could also be that apply() returns an object we can use to configure more about the tag application:

vpc.apply(new Tag('key', 'value')).restrictTo('*/Resource');

Nicer to read, though not as generic as the decorator.

[moofish32]

@rix0rrr I had some code started and I thought the concept was quicker to finish to help our discussion then try to doc-code and compare. I created #1451 just for discussion, if we like the approach I'll roll that back into the other branch in the original PR.