From 49cdb47f0487b6dc96ca1a732df83e8a72e93e61 Mon Sep 17 00:00:00 2001 From: comcalvi <66279577+comcalvi@users.noreply.github.com> Date: Wed, 19 Aug 2020 03:47:14 -0400 Subject: [PATCH] fix(cloudformation-diff): DependsOn singleton arrays aren't equal to string values (#9814) ---- Closes #9813 This allows `DependsOn: "A"` and `DependsOn: ["A"]` and `DependsOn: ["A", "B"]` and `DependsOn: ["B", "A"]` to be equivalent. ---- It also fixes a bug that would cause `BucketName: { 'Fn::Select': [0, ['name2', 'name1']] }` and `BucketName: { 'Fn::Select': [0, ['name1', 'name2']] }` to be equal. *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../cloudformation-diff/lib/diff/util.ts | 42 ++++++- .../test/diff-template.test.ts | 113 ++++++++++++++++++ 2 files changed, 154 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts index c8fcbf694c17e..02858f802647c 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts @@ -17,7 +17,9 @@ export function deepEqual(lvalue: any, rvalue: any): boolean { if (lvalue === rvalue) { return true; } // allows a numeric 10 and a literal "10" to be equivalent; // this is consistent with CloudFormation. - if (parseFloat(lvalue) === parseFloat(rvalue)) { return true; } + if (((typeof lvalue === 'string') || (typeof rvalue === 'string')) && (parseFloat(lvalue) === parseFloat(rvalue))) { + return true; + } if (typeof lvalue !== typeof rvalue) { return false; } if (Array.isArray(lvalue) !== Array.isArray(rvalue)) { return false; } if (Array.isArray(lvalue) /* && Array.isArray(rvalue) */) { @@ -36,6 +38,7 @@ export function deepEqual(lvalue: any, rvalue: any): boolean { if (keys.length !== Object.keys(rvalue).length) { return false; } for (const key of keys) { if (!rvalue.hasOwnProperty(key)) { return false; } + if (key === 'DependsOn') { return dependsOnEqual(lvalue[key], rvalue[key]); } if (!deepEqual(lvalue[key], rvalue[key])) { return false; } } return true; @@ -45,6 +48,43 @@ export function deepEqual(lvalue: any, rvalue: any): boolean { return false; } +/** + * Compares two arguments to DependsOn for equality. + * + * @param lvalue the left operand of the equality comparison. + * @param rvalue the right operand of the equality comparison. + * + * @returns +true+ if both +lvalue+ and +rvalue+ are equivalent to each other. + */ +function dependsOnEqual(lvalue: any, rvalue: any): boolean { + // allows ['Value'] and 'Value' to be equal + if (Array.isArray(lvalue) !== Array.isArray(rvalue)) { + const array = Array.isArray(lvalue) ? lvalue : rvalue; + const nonArray = Array.isArray(lvalue) ? rvalue : lvalue; + + if (array.length === 1 && deepEqual(array[0], nonArray)) { + return true; + } + return false; + } + + // allows arrays passed to DependsOn to be equivalent irrespective of element order + if (Array.isArray(lvalue) && Array.isArray(rvalue)) { + if (lvalue.length !== rvalue.length) { return false; } + for (let i = 0 ; i < lvalue.length ; i++) { + for (let j = 0 ; j < lvalue.length ; j++) { + if ((!deepEqual(lvalue[i], rvalue[j])) && (j === lvalue.length - 1)) { + return false; + } + break; + } + } + return true; + } + + return false; +} + /** * Produce the differences between two maps, as a map, using a specified diff function. * diff --git a/packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts b/packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts index e351162863cb3..9aff935252717 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts @@ -374,3 +374,116 @@ test('adding and removing quotes from a numeric property causes no changes', () differences = diffTemplate(newTemplate, currentTemplate); expect(differences.resources.differenceCount).toBe(0); }); + +test('single element arrays are equivalent to the single element in DependsOn expressions', () => { + // GIVEN + const currentTemplate = { + Resources: { + BucketResource: { + Type: 'AWS::S3::Bucket', + DependsOn: ['SomeResource'], + }, + }, + }; + + // WHEN + const newTemplate = { + Resources: { + BucketResource: { + Type: 'AWS::S3::Bucket', + DependsOn: 'SomeResource', + }, + }, + }; + + let differences = diffTemplate(currentTemplate, newTemplate); + expect(differences.resources.differenceCount).toBe(0); + + differences = diffTemplate(newTemplate, currentTemplate); + expect(differences.resources.differenceCount).toBe(0); +}); + + +test('array equivalence is independent of element order in DependsOn expressions', () => { + // GIVEN + const currentTemplate = { + Resources: { + BucketResource: { + Type: 'AWS::S3::Bucket', + DependsOn: ['SomeResource', 'AnotherResource'], + }, + }, + }; + + // WHEN + const newTemplate = { + Resources: { + BucketResource: { + Type: 'AWS::S3::Bucket', + DependsOn: ['AnotherResource', 'SomeResource'], + }, + }, + }; + + let differences = diffTemplate(currentTemplate, newTemplate); + expect(differences.resources.differenceCount).toBe(0); + + differences = diffTemplate(newTemplate, currentTemplate); + expect(differences.resources.differenceCount).toBe(0); +}); + +test('arrays of different length are considered unequal in DependsOn expressions', () => { + // GIVEN + const currentTemplate = { + Resources: { + BucketResource: { + Type: 'AWS::S3::Bucket', + DependsOn: ['SomeResource', 'AnotherResource', 'LastResource'], + }, + }, + }; + + // WHEN + const newTemplate = { + Resources: { + BucketResource: { + Type: 'AWS::S3::Bucket', + DependsOn: ['AnotherResource', 'SomeResource'], + }, + }, + }; + + let differences = diffTemplate(currentTemplate, newTemplate); + expect(differences.resources.differenceCount).toBe(1); + + differences = diffTemplate(newTemplate, currentTemplate); + expect(differences.resources.differenceCount).toBe(1); +}); + +test('arrays that differ only in element order are considered unequal outside of DependsOn expressions', () => { + // GIVEN + const currentTemplate = { + Resources: { + BucketResource: { + Type: 'AWS::S3::Bucket', + BucketName: { 'Fn::Select': [0, ['name1', 'name2']] }, + }, + }, + }; + + // WHEN + const newTemplate = { + Resources: { + BucketResource: { + Type: 'AWS::S3::Bucket', + BucketName: { 'Fn::Select': [0, ['name2', 'name1']] }, + }, + }, + }; + + let differences = diffTemplate(currentTemplate, newTemplate); + expect(differences.resources.differenceCount).toBe(1); + + differences = diffTemplate(newTemplate, currentTemplate); + expect(differences.resources.differenceCount).toBe(1); +});