aws-cdkのPRを読む（#38502）

cdkPR

chore(aspects): handle special case of ancestry - #38502

aspectは、スタックのTree構造に対して全リソースを対象に操作の実行順序を管理している。
今回のPRでは、Aspectの先祖を判定するテスト用のロジックで不具合があり、該当不具合の修正を行なっている。

修正内容

該当の関数はConstructを対象にaがbの先祖ならtrueを返すという実装になっている。
判定にnodeのpathを利用しているが、（コメントで説明がある通り）同じnodeに属しているか、bのnodeのpathがaのnodeのpathで始まる場合（aがbの先祖である）ときにtrueを返却している。

この時、ConstructのRootは先祖を持たないため、node.pathには空文字が設定されているため、a（root）はbの先祖だが、判定がfalseとなるケースを修正している。

修正内容として、node.pathが空文字の時（aがconstruct treeのroot）であるとき、絶対に先祖となるため必ずtrueを返却するguardを追加した修正となっている。

そもそもAspectとは何か

aws-cdkではスタックを実装するとスタックを親としてスタック内で実装されたリソースが子として紐付き、Tree構造で表現される。cdkでは最初にスタックのTree（Construct Tree）を作り、その後synthesisする。

synthesis.ts - invokeAspects

Aspectsはsynthesis（コードをCloudFormationテンプレートに）するフェーズの前に呼びされる。

Aspectの実装はAspectを登録する仕組みと実行順序のルールを定義している。 Aspectの中で実際に何を検査するのかは各機能ごとの実装に任されている。 たとえば、tag-aspect.tsでは、IAspectをTagBaseはresourceに対するタグの操作を行う実装となっている。 つまり、core側で実装さているaspectはAspectを登録してスキャンする仕組みを持っており、機能別（tag-base）の実装が"どう検査するか"（タグの追加・削除）を実装するかが定義されている。

Appendix: 関数の中に関数を定義

synthesis.ts の invokeAspects の実装を見ると、以下のように関数の中に関数を実装しており、ネストした構造となっていた。

typescript
function invokeAspects(root: IConstruct) {
  const invokedByPath: { [nodePath: string]: AspectApplication[] } = { };
  let nestedAspectWarning = false;
  recurse(root, []);
 
  function recurse(construct: IConstruct, inheritedAspects: AspectApplication[]) {
    ...
  }
}

分けた方が以下のメリットがあるように感じる

私の感覚では、あまり記載しない実装方法であったので、AIに聞いてみた

以下AIの回答：

md
この質問は非常にいいです。普通なら別関数にした方が見やすいように見えます。
 
ただし、ここでの recurse は「この関数が持つ状態に強く依存する」ので、内部関数として閉じ込めているのが自然です。
 
ここで recurse が閉じ込められている理由は、次の 3 つです。
 
1. invokedByPath を共有したい
このメモリは「この node に対して、どの aspect がすでに実行されたか」を管理するためのものです。
 
recurse が内部にあると、この状態を外から渡さずに持てます。
 
2. nestedAspectWarning を持ちたい
Aspect の中で新しい aspect を追加したら warnings を出す、というルールがここに入っています。
 
この state も外に出すとややこしくなります。
 
3. この再帰が一つのアルゴリズムとしてまとまっている
親から inherited aspect を受け取り
本ノードで local aspect を合成
priority でソート
子ノードへ再帰
これは一つの DFS のアルゴリズムです。
このアルゴリズムの内部状態を外部に露出しない方が、設計として安全です。
 
なので「普通に別関数にした方が見通しが良い」と思うのは自然ですが、
ここでは「この再帰は tree traversal の一部であり、その state を閉じ込めている」という意図があると考えるのが正確です。

つまり、再帰的な処理を必要としているが、他に使わないのであえて外に出す必要がない。
また再帰で使う状態を閉じ込めることができる。
これを外に出そうとすると関数のIFに再帰の状態を定義（外部に露出）しないといけないので、内部の状態を呼び出し元の閉じたスコープ管理できない（どこで初期化されるのかわからない）。

とのこと。なるほどね。