feat(refactor): resources with the same physical ID are considered equivalent (#360)

Incorporate the physical ID into the computation of resource digests.
This gives the user more flexibility: if a resource with physical ID set
is moved and updated at the same time, the algorithm can still detect
that it is the same resource.

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

Author: otaviomacedo

packages/@aws-cdk/tmp-toolkit-helpers/src/api/refactoring/digest.ts

@@ -1,17 +1,24 @@
 import * as crypto from 'node:crypto';
+import { loadResourceModel } from '@aws-cdk/cloudformation-diff/lib/diff/util';
 import type { CloudFormationTemplate } from './cloudformation';
 
 /**
  * Computes the digest for each resource in the template.
  *
  * Conceptually, the digest is computed as:
  *
- *     digest(resource) = hash(type + properties + dependencies.map(d))
+ *     d(resource) = hash(type + physicalId)                       , if physicalId is defined
+ *                 = hash(type + properties + dependencies.map(d)) , otherwise
  *
- * where `hash` is a cryptographic hash function. In other words, the digest of a
- * resource is computed from its type, its own properties (that is, excluding
- * properties that refer to other resources), and the digests of each of its
- * dependencies.
+ * where `hash` is a cryptographic hash function. In other words, if a resource has
+ * a physical ID, we use the physical ID plus its type to uniquely identify
+ * that resource. In this case, the digest can be computed from these two fields
+ * alone. A corollary is that such resources can be renamed and have their
+ * properties updated at the same time, and still be considered equivalent.
+ *
+ * Otherwise, the digest is computed from its type, its own properties (that is,
+ * excluding properties that refer to other resources), and the digests of each of
+ * its dependencies.
  *
  * The digest of a resource, defined recursively this way, remains stable even if
  * one or more of its dependencies gets renamed. Since the resources in a
@@ -82,9 +89,28 @@ export function computeResourceDigests(template: CloudFormationTemplate): Record
   const result: Record<string, string> = {};
   for (const id of order) {
     const resource = resources[id];
-    const depDigests = Array.from(graph[id]).map((d) => result[d]);
-    const propsWithoutRefs = hashObject(stripReferences(stripConstructPath(resource)));
-    const toHash = resource.Type + propsWithoutRefs + depDigests.join('');
+    const resourceProperties = resource.Properties ?? {};
+    const model = loadResourceModel(resource.Type);
+    const identifier = intersection(Object.keys(resourceProperties), model?.primaryIdentifier ?? []);
+    let toHash: string;
+
+    if (identifier.length === model?.primaryIdentifier?.length) {
+      // The resource has a physical ID defined, so we can
+      // use the ID and the type as the identity of the resource.
+      toHash =
+        resource.Type +
+        identifier
+          .sort()
+          .map((attr) => JSON.stringify(resourceProperties[attr]))
+          .join('');
+    } else {
+      // The resource does not have a physical ID defined, so we need to
+      // compute the digest based on its properties and dependencies.
+      const depDigests = Array.from(graph[id]).map((d) => result[d]);
+      const propertiesHash = hashObject(stripReferences(stripConstructPath(resource)));
+      toHash = resource.Type + propertiesHash + depDigests.join('');
+    }
+
     result[id] = crypto.createHash('sha256').update(toHash).digest('hex');
   }
 
@@ -151,3 +177,7 @@ function stripConstructPath(resource: any): any {
   delete copy.Metadata['aws:cdk:path'];
   return copy;
 }
+
+function intersection<T>(a: T[], b: T[]): T[] {
+  return a.filter((value) => b.includes(value));
+}

packages/aws-cdk/test/api/refactoring/refactoring.test.ts

@@ -1,3 +1,8 @@
+const mockLoadResourceModel = jest.fn();
+jest.mock('@aws-cdk/cloudformation-diff/lib/diff/util', () => ({
+  loadResourceModel: mockLoadResourceModel,
+}));
+
 import {
   GetTemplateCommand,
   ListStacksCommand,
@@ -293,6 +298,238 @@ describe('computeResourceDigests', () => {
     const result = computeResourceDigests(template);
     expect(result['Q1']).toBe(result['Q2']);
   });
+
+  test('uses physical ID if present', () => {
+    mockLoadResourceModel.mockReturnValue({
+      primaryIdentifier: ['FooName']
+    });
+
+    const template = {
+      Resources: {
+        Foo1: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            ShouldBeIgnored: true,
+          },
+        },
+        Foo2: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            ShouldAlsoBeIgnored: true,
+          },
+        },
+      },
+    };
+
+    const result = computeResourceDigests(template);
+    expect(result['Foo1']).toBe(result['Foo2']);
+  });
+
+  test('uses physical ID if present - with dependencies', () => {
+    mockLoadResourceModel.mockReturnValue({
+      primaryIdentifier: ['FooName']
+    });
+
+    const template = {
+      Resources: {
+        Foo1: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            ShouldBeIgnored: true,
+          },
+        },
+        Foo2: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            ShouldAlsoBeIgnored: true,
+          },
+        },
+        Bucket1: {
+          Type: 'AWS::S3::Bucket',
+          Properties: { Dep: { Ref: 'Foo1' } },
+        },
+        Bucket2: {
+          Type: 'AWS::S3::Bucket',
+          Properties: { Dep: { Ref: 'Foo2' } },
+        },
+      },
+    };
+
+    const result = computeResourceDigests(template);
+    expect(result['Bucket1']).toBe(result['Bucket2']);
+  });
+
+  test('different physical IDs lead to different digests', () => {
+    mockLoadResourceModel.mockReturnValue({
+      primaryIdentifier: ['FooName']
+    });
+
+    const template = {
+      Resources: {
+        Foo1: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            ShouldBeIgnored: true,
+          },
+        },
+        Foo2: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'YYYYYYYYY',
+            ShouldAlsoBeIgnored: true,
+          },
+        },
+      },
+    };
+
+    const result = computeResourceDigests(template);
+    expect(result['Foo1']).not.toBe(result['Foo2']);
+  });
+
+  test('primaryIdentifier is a composite field - different values', () => {
+    mockLoadResourceModel.mockReturnValue({
+      primaryIdentifier: ['FooName', 'BarName']
+    });
+
+    const template = {
+      Resources: {
+        Foo1: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            BarName: 'YYYYYYYYY',
+            ShouldBeIgnored: true,
+          },
+        },
+        Foo2: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            BarName: 'ZZZZZZZZZ',
+            ShouldAlsoBeIgnored: true,
+          },
+        },
+      },
+    };
+
+    const result = computeResourceDigests(template);
+    expect(result['Foo1']).not.toBe(result['Foo2']);
+  });
+
+  test('primaryIdentifier is a composite field - same value', () => {
+    mockLoadResourceModel.mockReturnValue({
+      primaryIdentifier: ['FooName', 'BarName']
+    });
+
+    const template = {
+      Resources: {
+        Foo1: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            BarName: 'YYYYYYYYY',
+            ShouldBeIgnored: true,
+          },
+        },
+        Foo2: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            BarName: 'YYYYYYYYY',
+            ShouldAlsoBeIgnored: true,
+          },
+        },
+      },
+    };
+
+    const result = computeResourceDigests(template);
+    expect(result['Foo1']).toBe(result['Foo2']);
+  });
+
+  test('primaryIdentifier is a composite field but not all of them are set in the resource', () => {
+    mockLoadResourceModel.mockReturnValue({
+      primaryIdentifier: ['FooName', 'BarName']
+    });
+
+    const template = {
+      Resources: {
+        Foo1: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            ShouldBeIgnored: true,
+          },
+        },
+        Foo2: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            FooName: 'XXXXXXXXX',
+            ShouldAlsoBeIgnored: true,
+          },
+        },
+      },
+    };
+
+    const result = computeResourceDigests(template);
+    expect(result['Foo1']).not.toBe(result['Foo2']);
+  });
+
+  test('resource properties does not contain primaryIdentifier - different values', () => {
+    mockLoadResourceModel.mockReturnValue({
+      primaryIdentifier: ['FooName']
+    });
+
+    const template = {
+      Resources: {
+        Foo1: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            ShouldNotBeIgnored: true,
+          },
+        },
+        Foo2: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            ShouldNotBeIgnoredEither: true,
+          },
+        },
+      },
+    };
+
+    const result = computeResourceDigests(template);
+    expect(result['Foo1']).not.toBe(result['Foo2']);
+  });
+
+  test('resource properties does not contain primaryIdentifier - same value', () => {
+    mockLoadResourceModel.mockReturnValue({
+      primaryIdentifier: ['FooName']
+    });
+
+    const template = {
+      Resources: {
+        Foo1: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            SomeProp: true,
+          },
+        },
+        Foo2: {
+          Type: 'AWS::S3::Foo',
+          Properties: {
+            SomeProp: true,
+          },
+        },
+      },
+    };
+
+    const result = computeResourceDigests(template);
+    expect(result['Foo1']).toBe(result['Foo2']);
+  });
 });
 
 describe('typed mappings', () => {