fix(cfn-include): handle numbers expressed as strings in templates #9525

comcalvi · 2020-08-07T19:39:04Z

Closes #9524

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

mergify · 2020-08-07T19:39:35Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

skinny85

This is a little to permissive right now.

Also, @rix0rrr already said he'd veto this change. Please add him as a reviewer as well.

packages/@aws-cdk/core/lib/runtime.ts

…strings-as-numbers

…ormation does

rix0rrr

As I said to Adam before, I am not okay with this, for the simple reason that we have type contracts to uphold. Both inside TypeScript as well as across our jsii language barrier.

What you are saying is that you are cool with the following:

interface CorsRule {
  // ...
  readonly maxAge: number;
}

const rule: CorsRule = readRuleFromTemplate("some-user-controlled-template.json");


console.log(rule.maxAge + 1);  // <-- Type error: cannot add a string and a number

// What do you mean maxAge is a "string"? It says "number" in the type right there!

I appreciate you are trying to roundtrip the quotation here. I have 2 responses to that:

Do your bookkeeping another way. Make a subclass that keeps track of whether properties have been modified, and just outputs the original unparsed string if not. Or (nasty nasty suggestion) tack a magic property onto that number you parsed to tell it to render out with quotes again.
Why do we even want so desperately to roundtrip those quotes? If it's only for the benefit of cdk diff, then shouldn't we be updating cdk diff to consider 10 and "10" the same value? That might even be useful in more cases than just the one you are trying to solve here.

Personally I'm leaning towards the second solution. Anything but destroying the value of our type system guarantees for --honestly-- little benefit.

rix0rrr · 2020-08-10T09:34:49Z

packages/@aws-cdk/core/lib/runtime.ts

@@ -218,8 +218,11 @@ export function validateString(x: any): ValidationResult {

 export function validateNumber(x: any): ValidationResult {
  if (canInspect(x) && typeof x !== 'number') {
-    return new ValidationResult(`${JSON.stringify(x)} should be a number`);
+    if (typeof x === 'string' && isNaN(parseInt(x))) {


Why must it be an int? Is "3.141592654" not allowed? Why does "10" get preferential treatment?

…strings-as-numbers

…hey parse too, are considered the same. (eg 10 and '10')

...-cdk/cloudformation-include/test/test-templates/invalid/invalid-string-passed-to-number.json

packages/@aws-cdk/core/lib/cfn-parse.ts

packages/@aws-cdk/cloudformation-include/test/test-templates/parsing-as-numbers.json

rix0rrr

Thanks Calvin! I like this much better.

Got some niggles about some comments: I'd like to see some clarification on code that's deviates from what you'd expect, so in the future I don't have to go and do code archeology when I pass by there again.

In this PR:

deepEqual: has a looser definition of "equality" now than you would expect from the name -- why is that?
getNumber: doesn't actually always return a number -- why is that?

These kinds of comments make it easier for me to decide whether this kind of unexpectedness was intentional or an oversight/bug.

Other than that, ready to ship it!

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

rix0rrr · 2020-08-11T08:13:10Z

packages/@aws-cdk/core/lib/cfn-parse.ts

+      }
+    }
+
+    // otherwise, return the input


...and let the validator handle the fact that it's not the type we want.

That's a pretty important detail to understanding the behavior of the parser here. At first glance this code would be wrong, but it becomes correct in combination with some other code somewhere else.

rix0rrr

👍

mergify · 2020-08-17T17:39:55Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-08-17T18:12:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: cc0e857
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-08-17T18:12:38Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…ws#9525) Closes aws#9524 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

allowed strings to be accepted if they can be parsed as integers

e31d300

mergify bot added the contribution/core This is a PR that came from AWS. label Aug 7, 2020

comcalvi changed the title ~~fix(cfn-include): allow cfn-include to handle numbers expressed as strings in json templates~~ fix(cfn-include): handle numbers expressed as strings in json templates Aug 7, 2020

skinny85 suggested changes Aug 7, 2020

View reviewed changes

packages/@aws-cdk/core/lib/runtime.ts Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/aws/aws-cdk into parsing-…

b4c57a5

…strings-as-numbers

SomayaB assigned skinny85 Aug 8, 2020

skinny85 requested a review from rix0rrr August 8, 2020 00:30

now cfn-include can parse strings that are numeric values, as clouddf…

88079d1

…ormation does

comcalvi requested a review from skinny85 August 8, 2020 01:04

rix0rrr requested changes Aug 10, 2020

View reviewed changes

comcalvi added 6 commits August 10, 2020 10:55

Merge branch 'master' of https://github.com/aws/aws-cdk into parsing-…

cbab4d1

…strings-as-numbers

strings that can be parsed to numbers, when diff'd with the numbers t…

b2af7b0

…hey parse too, are considered the same. (eg 10 and '10')

ensured that the diff equality works in both directions

2f6d9cd

all numeric types are now handled, not just integers

1b40af2

fixed blank line issues

24a11fe

made the style of deepEqual() consistent

3182c5c

comcalvi requested a review from rix0rrr August 10, 2020 20:36

comcalvi added 2 commits August 10, 2020 16:40

minor cosmetic change

fa789a4

consistent use of parseFloat() over parseInt()

ff5f8cd

skinny85 suggested changes Aug 10, 2020

View reviewed changes

incorporated review comments

1ce9daf

comcalvi requested a review from skinny85 August 10, 2020 21:20

rix0rrr reviewed Aug 11, 2020

View reviewed changes

comcalvi added 3 commits August 12, 2020 13:52

tmp

ab5ff73

updated comments around unexpected behavior

1b13a29

updated comments

f407ca9

comcalvi requested a review from rix0rrr August 12, 2020 18:31

rix0rrr approved these changes Aug 17, 2020

View reviewed changes

comcalvi removed the request for review from skinny85 August 17, 2020 17:31

comcalvi requested a review from skinny85 August 17, 2020 17:31

skinny85 changed the title ~~fix(cfn-include): handle numbers expressed as strings in json templates~~ fix(cfn-include): handle numbers expressed as strings in templates Aug 17, 2020

skinny85 approved these changes Aug 17, 2020

View reviewed changes

Merge branch 'master' into parsing-strings-as-numbers

cc0e857

mergify bot merged commit e9a4102 into aws:master Aug 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cfn-include): handle numbers expressed as strings in templates #9525

fix(cfn-include): handle numbers expressed as strings in templates #9525

comcalvi commented Aug 7, 2020 •

edited by skinny85

Loading

mergify bot commented Aug 7, 2020

skinny85 left a comment

rix0rrr left a comment •

edited by skinny85

Loading

rix0rrr Aug 10, 2020

rix0rrr left a comment •

edited

Loading

rix0rrr Aug 11, 2020

rix0rrr left a comment

mergify bot commented Aug 17, 2020

aws-cdk-automation commented Aug 17, 2020

mergify bot commented Aug 17, 2020

fix(cfn-include): handle numbers expressed as strings in templates #9525

fix(cfn-include): handle numbers expressed as strings in templates #9525

Conversation

comcalvi commented Aug 7, 2020 • edited by skinny85 Loading

mergify bot commented Aug 7, 2020

skinny85 left a comment

Choose a reason for hiding this comment

rix0rrr left a comment • edited by skinny85 Loading

Choose a reason for hiding this comment

rix0rrr Aug 10, 2020

Choose a reason for hiding this comment

rix0rrr left a comment • edited Loading

Choose a reason for hiding this comment

rix0rrr Aug 11, 2020

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 17, 2020

aws-cdk-automation commented Aug 17, 2020

AWS CodeBuild CI Report

mergify bot commented Aug 17, 2020

comcalvi commented Aug 7, 2020 •

edited by skinny85

Loading

rix0rrr left a comment •

edited by skinny85

Loading

rix0rrr left a comment •

edited

Loading