refactor: encapsulate logicalId logic in an interface

In an attempt to keep things DRY, encapsulate the logic around overriding a construct's logicalId in one place and add tests.

src/constructs/core/migrating.test.ts

@@ -0,0 +1,83 @@
+import "@aws-cdk/assert/jest";
+import { SynthUtils } from "@aws-cdk/assert";
+import { Bucket } from "@aws-cdk/aws-s3";
+import type { SynthedStack } from "../../../test/utils";
+import { simpleGuStackForTesting } from "../../../test/utils";
+import { GuMigratingResource } from "./migrating";
+
+describe("GuMigratingResource", () => {
+  /*
+  NOTE: In reality, we'd never directly call `GuMigratingResource.setLogicalId` as the GuConstruct will do that.
+  We're calling it here to test the function in isolation.
+   */
+
+  // eslint-disable-next-line @typescript-eslint/no-empty-function -- we are testing `console.warn` being called, we don't need to see the message
+  const warn = jest.spyOn(console, "warn").mockImplementation(() => {});
+
+  // eslint-disable-next-line @typescript-eslint/no-empty-function -- we are testing `console.error` being called, we don't need to see the message
+  const error = jest.spyOn(console, "error").mockImplementation(() => {});
+
+  afterEach(() => {
+    warn.mockReset();
+    error.mockReset();
+  });
+
+  test("creating a new resource in a new stack", () => {
+    const stack = simpleGuStackForTesting({ migratedFromCloudFormation: false });
+    const construct = new Bucket(stack, "MyBucket");
+
+    GuMigratingResource.setLogicalId(construct, stack, {});
+
+    expect(warn).toHaveBeenCalledTimes(0);
+    expect(error).toHaveBeenCalledTimes(0);
+
+    const json = SynthUtils.toCloudFormation(stack) as SynthedStack;
+    const resourceKeys = Object.keys(json.Resources);
+
+    expect(resourceKeys).toHaveLength(1);
+    expect(resourceKeys[0]).toMatch(/^MyBucket[A-Z0-9]+$/);
+  });
+
+  test("Keeping a resource's logicalId when migrating a stack", () => {
+    const stack = simpleGuStackForTesting({ migratedFromCloudFormation: true });
+    const construct = new Bucket(stack, "MyBucket");
+
+    GuMigratingResource.setLogicalId(construct, stack, { existingLogicalId: "my-pre-existing-bucket" });
+
+    expect(warn).toHaveBeenCalledTimes(0);
+    expect(error).toHaveBeenCalledTimes(0);
+
+    const json = SynthUtils.toCloudFormation(stack) as SynthedStack;
+    expect(Object.keys(json.Resources)).toContain("my-pre-existing-bucket");
+  });
+
+  test("Migrating a stack and falling back to the construct's ID when existingLogicalId is not set", () => {
+    const stack = simpleGuStackForTesting({ migratedFromCloudFormation: true });
+    const construct = new Bucket(stack, "MyBucket");
+
+    GuMigratingResource.setLogicalId(construct, stack, {});
+
+    expect(warn).toHaveBeenCalledTimes(1);
+    expect(warn).toHaveBeenCalledWith(
+      "The migratedFromCloudFormation prop on the GuStack is true, however a logical id has not been specified for MyBucket meaning a new resource will be created with a new ID. Is this correct?"
+    );
+
+    const json = SynthUtils.toCloudFormation(stack) as SynthedStack;
+    expect(Object.keys(json.Resources)).toContain("MyBucket");
+  });
+
+  test("Specifying a resource's existingLogicalId in a new stack", () => {
+    const stack = simpleGuStackForTesting({ migratedFromCloudFormation: false });
+    const construct = new Bucket(stack, "MyBucket");
+
+    GuMigratingResource.setLogicalId(construct, stack, { existingLogicalId: "my-pre-existing-bucket" });
+
+    expect(error).toHaveBeenCalledTimes(1);
+    expect(error).toHaveBeenCalledWith(
+      "The migratedFromCloudFormation prop on the GuStack is false and existingLogicalId has been set on the construct MyBucket. This is not recommended as it's preferred to keep the auto-generated ID for new resources"
+    );
+
+    const json = SynthUtils.toCloudFormation(stack) as SynthedStack;
+    expect(Object.keys(json.Resources)).toContain("my-pre-existing-bucket");
+  });
+});

src/constructs/core/migrating.ts

@@ -0,0 +1,41 @@
+import type { CfnElement, IConstruct } from "@aws-cdk/core";
+import type { GuStack } from "./stack";
+
+export interface GuMigratingResource {
+  existingLogicalId?: string;
+}
+
+export const GuMigratingResource = {
+  setLogicalId<T extends IConstruct>(
+    construct: T,
+    { migratedFromCloudFormation }: GuStack,
+    { existingLogicalId }: GuMigratingResource
+  ): void {
+    const overrideLogicalId = (logicalId: string) => {
+      const defaultChild = construct.node.defaultChild as CfnElement;
+      defaultChild.overrideLogicalId(logicalId);
+    };
+
+    const id = construct.node.id;
+
+    if (!migratedFromCloudFormation && existingLogicalId) {
+      console.error(
+        `The migratedFromCloudFormation prop on the GuStack is false and existingLogicalId has been set on the construct ${id}. This is not recommended as it's preferred to keep the auto-generated ID for new resources`
+      );
+    }
+
+    if (migratedFromCloudFormation && !existingLogicalId) {
+      console.warn(
+        `The migratedFromCloudFormation prop on the GuStack is true, however a logical id has not been specified for ${id} meaning a new resource will be created with a new ID. Is this correct?`
+      );
+    }
+
+    if (existingLogicalId) {
+      overrideLogicalId(existingLogicalId);
+    }
+
+    if (migratedFromCloudFormation && !existingLogicalId) {
+      overrideLogicalId(id);
+    }
+  },
+};