From 86f2714613f06aaf2bcee27da2f66066c8e863d0 Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Tue, 19 Oct 2021 03:01:47 -0700 Subject: [PATCH] fix(cfn-diff): correctly handle Date strings in diff (#16591) Turns out, `parseFloat()` in JavaScript is even crazier than we thought, and returns nonsense like `2021` for a string containing a Date like `'2021-10-25'`. For that reason, add an explicit check that the string parsed looks like a number before calling `parseFloat()`. Fixes #16444 ---- *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-template.ts | 8 ++- .../cloudformation-diff/lib/diff/util.ts | 18 ++----- .../test/diff-template.test.ts | 51 +++++++------------ 3 files changed, 27 insertions(+), 50 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts index b3f282802b675..6e06e56f90af1 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts @@ -99,13 +99,17 @@ function calculateTemplateDiff(currentTemplate: { [key: string]: any }, newTempl for (const key of unionOf(Object.keys(currentTemplate), Object.keys(newTemplate)).sort()) { const oldValue = currentTemplate[key]; const newValue = newTemplate[key]; - if (deepEqual(oldValue, newValue)) { continue; } + if (deepEqual(oldValue, newValue)) { + continue; + } const handler: DiffHandler = DIFF_HANDLERS[key] || ((_diff, oldV, newV) => unknown[key] = impl.diffUnknown(oldV, newV)); handler(differences, oldValue, newValue); } - if (Object.keys(unknown).length > 0) { differences.unknown = new types.DifferenceCollection(unknown); } + if (Object.keys(unknown).length > 0) { + differences.unknown = new types.DifferenceCollection(unknown); + } return new types.TemplateDiff(differences); } diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts index 59c8606be0a35..1cbd4b1a111d7 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts @@ -138,20 +138,10 @@ export function unionOf(lv: string[] | Set, rv: string[] | Set): * A parseFloat implementation that does the right thing for * strings like '0.0.0' * (for which JavaScript's parseFloat() returns 0). + * We return NaN for all of these strings that do not represent numbers, + * and so comparing them fails, + * and doesn't short-circuit the diff logic. */ function safeParseFloat(str: string): number { - const ret = parseFloat(str); - const nonNumericRegex = /\d*\.\d+\./; - if (ret === 0) { - // if the str is exactly '0', that's OK; - // but parseFloat() also returns 0 for things like '0.0'; - // in this case, return NaN, so we'll fall back to string comparison - return str === '0' ? ret : NaN; - } else if (nonNumericRegex.test(str)) { - // if the str contains non-numeric characters, - // return NaN, so we'll fall back to string comparison - return NaN; - } else { - return ret; - } + return Number(str); } 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 9241d0e8e28eb..d43ec99808d31 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts @@ -582,70 +582,57 @@ test('when a property changes including equivalent DependsOn', () => { expect(differences.resources.differenceCount).toBe(1); }); -test('when a property with a number-like format changes', () => { - const bucketName = 'ShineyBucketName'; - const tagChanges = { - '0.31.1-prod': '0.31.2-prod', - '8.0.5.5.4-identifier': '8.0.5.5.5-identifier', - '1.1.1.1': '1.1.2.2', - '1.2.3': '1.2.4', - '2.2.2.2': '2.2.3.2', - '3.3.3.3': '3.4.3.3', - }; - const oldTags = Object.keys(tagChanges); - const newTags = Object.values(tagChanges); +test.each([ + ['0.31.1-prod', '0.31.2-prod'], + ['8.0.5.5.4-identifier', '8.0.5.5.5-identifier'], + ['1.1.1.1', '1.1.1.2'], + ['1.2.3', '1.2.4'], + ['2.2.2.2', '2.2.3.2'], + ['3.3.3.3', '3.4.3.3'], + ['2021-10-23T06:07:08.000Z', '2021-10-23T09:10:11.123Z'], +])("reports a change when a string property with a number-like format changes from '%s' to '%s'", (oldValue, newValue) => { + // GIVEN const currentTemplate = { Resources: { - QueueResource: { - Type: 'AWS::SQS::Queue', - }, BucketResource: { Type: 'AWS::S3::Bucket', Properties: { - BucketName: bucketName, - Tags: oldTags, + Tags: [oldValue], }, }, }, }; const newTemplate = { Resources: { - QueueResource: { - Type: 'AWS::SQS::Queue', - }, BucketResource: { Type: 'AWS::S3::Bucket', Properties: { - BucketName: bucketName, - Tags: newTags, + Tags: [newValue], }, }, }, }; - + // WHEN const differences = diffTemplate(currentTemplate, newTemplate); + + // THEN expect(differences.differenceCount).toBe(1); expect(differences.resources.differenceCount).toBe(1); const difference = differences.resources.changes.BucketResource; expect(difference).not.toBeUndefined(); expect(difference?.oldResourceType).toEqual('AWS::S3::Bucket'); expect(difference?.propertyUpdates).toEqual({ - Tags: { oldValue: oldTags, newValue: newTags, changeImpact: ResourceImpact.WILL_UPDATE, isDifferent: true }, + Tags: { oldValue: [oldValue], newValue: [newValue], changeImpact: ResourceImpact.WILL_UPDATE, isDifferent: true }, }); }); test('when a property with a number-like format doesn\'t change', () => { - const bucketName = 'ShineyBucketName'; const tags = ['0.31.1-prod', '8.0.5.5.4-identifier', '1.1.1.1', '1.2.3']; const currentTemplate = { Resources: { - QueueResource: { - Type: 'AWS::SQS::Queue', - }, BucketResource: { Type: 'AWS::S3::Bucket', Properties: { - BucketName: bucketName, Tags: tags, }, }, @@ -653,13 +640,9 @@ test('when a property with a number-like format doesn\'t change', () => { }; const newTemplate = { Resources: { - QueueResource: { - Type: 'AWS::SQS::Queue', - }, BucketResource: { Type: 'AWS::S3::Bucket', Properties: { - BucketName: bucketName, Tags: tags, }, }, @@ -671,4 +654,4 @@ test('when a property with a number-like format doesn\'t change', () => { expect(differences.resources.differenceCount).toBe(0); const difference = differences.resources.changes.BucketResource; expect(difference).toBeUndefined(); -}); \ No newline at end of file +});