fix(toolkit-lib): new mappingSource option to replace multiple mutual exclusive options in the refactor API (#559)

Introduce a new type, `MappingSource`, that allows the user to tell how
they want the toolkit to get the mappings: computing it, using an
explicitly provided one, or using the reverse of an explicitly provided
one.

This replaces three previous separate properties: `exclude`, `mappings`
and `revert` with a single `mappingSource`.

---
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license

---------

Signed-off-by: github-actions <[email protected]>
Co-authored-by: github-actions <[email protected]>

Author: otaviomacedo

packages/@aws-cdk/cloudformation-diff/lib/mappings.ts

@@ -18,13 +18,13 @@ export function formatTypedMappings(stream: NodeJS.WritableStream, mappings: Typ
   const rows = mappings.map((m) => [m.type, m.sourcePath, m.destinationPath]);
 
   const formatter = new Formatter(stream, {});
+  formatter.print(`${env}:`);
   if (mappings.length > 0) {
-    formatter.print(`${env}:`);
     formatter.print(chalk.green(formatTable(header.concat(rows), undefined)));
-    formatter.print(' ');
   } else {
     formatter.print('Nothing to refactor.');
   }
+  formatter.print(' ');
 }
 
 export function formatAmbiguitySectionHeader(stream: NodeJS.WritableStream) {

packages/@aws-cdk/toolkit-lib/lib/actions/refactor/index.ts

@@ -1,4 +1,66 @@
 import type { StackSelector } from '../../api/cloud-assembly';
+import type { ExcludeList } from '../../api/refactoring';
+import { InMemoryExcludeList, NeverExclude } from '../../api/refactoring';
+
+type MappingType = 'auto' | 'explicit';
+
+/**
+ * The source of the resource mappings to be used for refactoring.
+ */
+export class MappingSource {
+  /**
+   * The mapping will be automatically generated based on a comparison of
+   * the deployed stacks and the local stacks.
+   *
+   * @param exclude A list of resource locations to exclude from the mapping.
+   */
+  public static auto(exclude: string[] = []): MappingSource {
+    const excludeList = new InMemoryExcludeList(exclude);
+    return new MappingSource('auto', [], excludeList);
+  }
+
+  /**
+   * An explicitly provided list of mappings, which will be used for refactoring.
+   */
+  public static explicit(groups: MappingGroup[]): MappingSource {
+    return new MappingSource('explicit', groups, new NeverExclude());
+  }
+
+  /**
+   * An explicitly provided list of mappings, which will be used for refactoring,
+   * but in reverse, that is, the source locations will become the destination
+   * locations and vice versa.
+   */
+  public static reverse(groups: MappingGroup[]): MappingSource {
+    const reverseGroups = groups.map((group) => ({
+      ...group,
+      resources: Object.fromEntries(Object.entries(group.resources).map(([src, dst]) => [dst, src])),
+    }));
+
+    return MappingSource.explicit(reverseGroups);
+  }
+
+  /**
+   * @internal
+   */
+  public readonly source: MappingType;
+
+  /**
+   * @internal
+   */
+  public readonly groups: MappingGroup[];
+
+  /**
+   * @internal
+   */
+  public readonly exclude: ExcludeList;
+
+  private constructor(source: MappingType, groups: MappingGroup[], exclude: ExcludeList) {
+    this.source = source;
+    this.groups = groups;
+    this.exclude = exclude;
+  }
+}
 
 export interface RefactorOptions {
   /**
@@ -16,30 +78,9 @@ export interface RefactorOptions {
   stacks?: StackSelector;
 
   /**
-   * A list of resources that will not be part of the refactor.
-   * Elements of this list must be the _destination_ locations
-   * that should be excluded, i.e., the location to which a
-   * resource would be moved if the refactor were to happen.
-   *
-   * The format of the locations in the file can be either:
-   *
-   * - Stack name and logical ID (e.g. `Stack1.MyQueue`)
-   * - A construct path (e.g. `Stack1/Foo/Bar/Resource`).
-   */
-  exclude?: string[];
-
-  /**
-   * An explicit mapping to be used by the toolkit (as opposed to letting the
-   * toolkit itself compute the mapping).
-   */
-  mappings?: MappingGroup[];
-
-  /**
-   * Modifies the behavior of the 'mappings' option by swapping source and
-   * destination locations. This is useful when you want to undo a refactor
-   * that was previously applied.
+   * How the toolkit should obtain the mappings
    */
-  revert?: boolean;
+  mappingSource?: MappingSource;
 }
 
 export interface MappingGroup {

packages/@aws-cdk/toolkit-lib/lib/api/refactoring/exclude.ts

@@ -5,12 +5,23 @@ import type { ResourceLocation } from './cloudformation';
 
 export interface ExcludeList {
   isExcluded(location: ResourceLocation): boolean;
+
+  union(other: ExcludeList): ExcludeList;
+}
+
+abstract class AbstractExcludeList implements ExcludeList {
+  abstract isExcluded(location: ResourceLocation): boolean;
+
+  union(other: ExcludeList): ExcludeList {
+    return new UnionExcludeList([this, other]);
+  }
 }
 
-export class ManifestExcludeList implements ExcludeList {
+export class ManifestExcludeList extends AbstractExcludeList {
   private readonly excludedLocations: CfnResourceLocation[];
 
   constructor(manifest: AssemblyManifest) {
+    super();
     this.excludedLocations = this.getExcludedLocations(manifest);
   }
 
@@ -48,11 +59,12 @@ export class ManifestExcludeList implements ExcludeList {
   }
 }
 
-export class InMemoryExcludeList implements ExcludeList {
+export class InMemoryExcludeList extends AbstractExcludeList {
   private readonly excludedLocations: CfnResourceLocation[];
   private readonly excludedPaths: string[];
 
   constructor(items: string[]) {
+    super();
     this.excludedLocations = [];
     this.excludedPaths = [];
 
@@ -85,28 +97,24 @@ export class InMemoryExcludeList implements ExcludeList {
   }
 }
 
-export class UnionExcludeList implements ExcludeList {
+export class UnionExcludeList extends AbstractExcludeList {
   constructor(private readonly excludeLists: ExcludeList[]) {
+    super();
   }
 
   isExcluded(location: ResourceLocation): boolean {
     return this.excludeLists.some((excludeList) => excludeList.isExcluded(location));
   }
 }
 
-export class NeverExclude implements ExcludeList {
+export class NeverExclude extends AbstractExcludeList {
   isExcluded(_location: ResourceLocation): boolean {
     return false;
   }
 }
 
-export class AlwaysExclude implements ExcludeList {
+export class AlwaysExclude extends AbstractExcludeList {
   isExcluded(_location: ResourceLocation): boolean {
     return true;
   }
 }
-
-export function fromManifestAndExclusionList(manifest: AssemblyManifest, exclude?: string[]): ExcludeList {
-  return new UnionExcludeList([new ManifestExcludeList(manifest), new InMemoryExcludeList(exclude ?? [])]);
-}
-

packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts

@@ -29,6 +29,7 @@ import { appendObject, prepareDiff } from '../actions/diff/private';
 import type { DriftOptions, DriftResult } from '../actions/drift';
 import { type ListOptions } from '../actions/list';
 import type { MappingGroup, RefactorOptions } from '../actions/refactor';
+import { MappingSource } from '../actions/refactor';
 import { type RollbackOptions } from '../actions/rollback';
 import { type SynthOptions } from '../actions/synth';
 import type { WatchOptions } from '../actions/watch';
@@ -59,10 +60,11 @@ import { CloudWatchLogEventMonitor, findCloudWatchLogGroups } from '../api/logs-
 import { Mode, PluginHost } from '../api/plugin';
 import {
   formatAmbiguitySectionHeader,
-  formatAmbiguousMappings, formatMappingsHeader,
+  formatAmbiguousMappings,
+  formatMappingsHeader,
   formatTypedMappings,
-  fromManifestAndExclusionList,
   getDeployedStacks,
+  ManifestExcludeList,
   usePrescribedMappings,
 } from '../api/refactoring';
 import type { CloudFormationStack, ResourceMapping } from '../api/refactoring/cloudformation';
@@ -1075,21 +1077,14 @@ export class Toolkit extends CloudAssemblySourceBuilder {
   }
 
   private async _refactor(assembly: StackAssembly, ioHelper: IoHelper, options: RefactorOptions = {}): Promise<void> {
-    if (options.mappings && options.exclude) {
-      throw new ToolkitError("Cannot use both 'exclude' and 'mappings'.");
-    }
-
-    if (options.revert && !options.mappings) {
-      throw new ToolkitError("The 'revert' options can only be used with the 'mappings' option.");
-    }
-
     if (!options.dryRun) {
       throw new ToolkitError('Refactor is not available yet. Too see the proposed changes, use the --dry-run flag.');
     }
 
     const sdkProvider = await this.sdkProvider('refactor');
     const stacks = await assembly.selectStacksV2(ALL_STACKS);
-    const exclude = fromManifestAndExclusionList(assembly.cloudAssembly.manifest, options.exclude);
+    const mappingSource = options.mappingSource ?? MappingSource.auto();
+    const exclude = mappingSource.exclude.union(new ManifestExcludeList(assembly.cloudAssembly.manifest));
     const filteredStacks = await assembly.selectStacksV2(options.stacks ?? ALL_STACKS);
 
     const refactoringContexts: RefactoringContext[] = [];
@@ -1099,7 +1094,7 @@ export class Toolkit extends CloudAssemblySourceBuilder {
         deployedStacks,
         localStacks,
         filteredStacks: filteredStacks.stackArtifacts,
-        mappings: await getUserProvidedMappings(),
+        mappings: await getUserProvidedMappings(environment),
       }));
     }
 
@@ -1133,7 +1128,7 @@ export class Toolkit extends CloudAssemblySourceBuilder {
       const environments: Map<string, cxapi.Environment> = new Map();
 
       for (const stack of stacks.stackArtifacts) {
-        const environment = stack.environment;
+        const environment = await sdkProvider.resolveEnvironment(stack.environment);
         const key = hashObject(environment);
         environments.set(key, environment);
         if (stackGroups.has(key)) {
@@ -1156,21 +1151,14 @@ export class Toolkit extends CloudAssemblySourceBuilder {
       return result;
     }
 
-    async function getUserProvidedMappings(): Promise<ResourceMapping[] | undefined> {
-      if (options.revert) {
-        return usePrescribedMappings(revert(options.mappings ?? []), sdkProvider);
-      }
-      if (options.mappings != null) {
-        return usePrescribedMappings(options.mappings ?? [], sdkProvider);
-      }
-      return undefined;
-    }
+    async function getUserProvidedMappings(environment: cxapi.Environment): Promise<ResourceMapping[] | undefined> {
+      return mappingSource.source == 'explicit'
+        ? usePrescribedMappings(mappingSource.groups.filter(matchesEnvironment), sdkProvider)
+        : undefined;
 
-    function revert(mappings: MappingGroup[]): MappingGroup[] {
-      return mappings.map(group => ({
-        ...group,
-        resources: Object.fromEntries(Object.entries(group.resources).map(([src, dst]) => ([dst, src]))),
-      }));
+      function matchesEnvironment(g: MappingGroup): boolean {
+        return g.account === environment.account && g.region === environment.region;
+      }
     }
   }
 

packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts

@@ -1,5 +1,5 @@
 import { GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation';
-import { StackSelectionStrategy, Toolkit } from '../../lib';
+import { MappingSource, StackSelectionStrategy, Toolkit } from '../../lib';
 import { SdkProvider } from '../../lib/api/aws-auth/private';
 import { builderFixture, TestIoHost } from '../_helpers';
 import { mockCloudFormationClient, MockSdk } from '../_helpers/mock-sdk';
@@ -350,15 +350,15 @@ test('uses the explicit mapping when provided, instead of computing it on-the-fl
   const cx = await builderFixture(toolkit, 'stack-with-bucket');
   await toolkit.refactor(cx, {
     dryRun: true,
-    mappings: [
+    mappingSource: MappingSource.explicit([
       {
         account: '123456789012',
         region: 'us-east-1',
         resources: {
           'Stack1.OldLogicalID': 'Stack1.NewLogicalID',
         },
       },
-    ],
+    ]),
   });
 
   // THEN
@@ -418,16 +418,14 @@ test('uses the reverse of an explicit mapping when provided', async () => {
   const cx = await builderFixture(toolkit, 'stack-with-bucket');
   await toolkit.refactor(cx, {
     dryRun: true,
-    // ... this is the mapping we used...
-    mappings: [{
+    // ... this is the mapping we used, and now we want to revert it
+    mappingSource: MappingSource.reverse([{
       account: '123456789012',
       region: 'us-east-1',
       resources: {
         'Stack1.OldLogicalID': 'Stack1.NewLogicalID',
       },
-    }],
-    // ...and now we want to revert it
-    revert: true,
+    }]),
   });
 
   // THEN
@@ -450,35 +448,6 @@ test('uses the reverse of an explicit mapping when provided', async () => {
   );
 });
 
-test('exclude and mappings are mutually exclusive', async () => {
-  // WHEN
-  const cx = await builderFixture(toolkit, 'stack-with-bucket');
-  await expect(
-    toolkit.refactor(cx, {
-      dryRun: true,
-      exclude: ['Stack1/OldLogicalID'],
-      mappings: [{
-        account: '123456789012',
-        region: 'us-east-1',
-        resources: {
-          'Stack1.OldLogicalID': 'Stack1.NewLogicalID',
-        },
-      }],
-    }),
-  ).rejects.toThrow(/Cannot use both 'exclude' and 'mappings'/);
-});
-
-test('revert can only be used with mappings', async () => {
-  // WHEN
-  const cx = await builderFixture(toolkit, 'stack-with-bucket');
-  await expect(
-    toolkit.refactor(cx, {
-      dryRun: true,
-      revert: true,
-    }),
-  ).rejects.toThrow(/The 'revert' options can only be used with the 'mappings' option/);
-});
-
 test('computes one set of mappings per environment', async () => {
   // GIVEN
   mockCloudFormationClient

packages/aws-cdk/lib/cli/cdk-toolkit.ts

@@ -2,8 +2,8 @@ import * as path from 'path';
 import { format } from 'util';
 import { RequireApproval } from '@aws-cdk/cloud-assembly-schema';
 import * as cxapi from '@aws-cdk/cx-api';
-import { StackSelectionStrategy, ToolkitError, PermissionChangeType, Toolkit } from '@aws-cdk/toolkit-lib';
 import type { DeploymentMethod, ToolkitAction, ToolkitOptions } from '@aws-cdk/toolkit-lib';
+import { StackSelectionStrategy, ToolkitError, PermissionChangeType, Toolkit, MappingSource } from '@aws-cdk/toolkit-lib';
 import * as chalk from 'chalk';
 import * as chokidar from 'chokidar';
 import * as fs from 'fs-extra';
@@ -1231,9 +1231,7 @@ export class CdkToolkit {
           patterns: options.selector.patterns,
           strategy: options.selector.patterns.length > 0 ? StackSelectionStrategy.PATTERN_MATCH : StackSelectionStrategy.ALL_STACKS,
         },
-        exclude: await readExcludeFile(options.excludeFile),
-        mappings: await readMappingFile(options.mappingFile),
-        revert: options.revert,
+        mappingSource: await mappingSource(),
       });
     } catch (e) {
       error((e as Error).message);
@@ -1266,6 +1264,16 @@ export class CdkToolkit {
       }
       return undefined;
     }
+
+    async function mappingSource(): Promise<MappingSource> {
+      if (options.mappingFile != null) {
+        return MappingSource.explicit(await readMappingFile(options.mappingFile));
+      }
+      if (options.revert) {
+        return MappingSource.reverse(await readMappingFile(options.mappingFile));
+      }
+      return MappingSource.auto((await readExcludeFile(options.excludeFile)) ?? []);
+    }
   }
 
   private async selectStacksForList(patterns: string[]) {