incorporated review requests, improved testing

packages/@aws-cdk/cloudformation-include/test/invalid-templates.test.ts

@@ -104,25 +104,13 @@ describe('CDK Include', () => {
   test("throws a validation exception when Fn::Sub in string form uses a key that isn't in the template", () => {
     expect(() => {
       includeTestTemplate(stack, 'fn-sub-key-not-in-template-string.json');
-    }).toThrow(/Element used in Ref expression with logical ID: 'AFakeResource' in Fn::Sub not found/);
+    }).toThrow(/Element referenced in Fn::Sub expression with logical ID: 'AFakeResource' was not found in the template/);
   });
 
-  test("throws a validation exception when Fn::Sub in map form uses a key that isn't in the template", () => {
+  test('throws a validation exception when Fn::Sub has an empty ${} reference', () => {
     expect(() => {
-      includeTestTemplate(stack, 'fn-sub-key-not-in-template.json');
-    }).toThrow(/Element used in Ref expression with logical ID: 'AFakeResource' not found/);
-  });
-
-  test("throws a validation exception when Fn::Sub in map form uses a key that isn't in the template and isn't in the map", () => {
-    expect(() => {
-      includeTestTemplate(stack, 'fn-sub-key-not-in-map.json');
-    }).toThrow(/Element used in Ref expression with logical ID: 'AFakeResource' in Fn::Sub not found/);
-  });
-
-  test("throws a validation exception when Fn::Sub uses a resource attribute that doesn't exist", () => {
-    expect(() => {
-      includeTestTemplate(stack, 'fn-sub-bad-get-att.json');
-    }).toThrow(/Resource referenced in Fn::Sub expression with logical ID: 'FakeBucket' not found/);
+      includeTestTemplate(stack, 'fn-sub-${}-only.json');
+    }).toThrow(/Element referenced in Fn::Sub expression with logical ID: '' was not found in the template/);
   });
 });
 

packages/@aws-cdk/cloudformation-include/test/test-templates/fn-sub-brace-edges.json

@@ -0,0 +1,55 @@
+{
+  "Resources": {
+    "Bucket": {
+      "Type": "Custom::ManyStrings",
+      "Properties": {
+        "SymbolsOnly": {
+          "DollarSign": {
+            "Fn::Sub": "$"
+          },
+          "OpeningBrace": {
+            "Fn::Sub": "{"
+          },
+          "ClosingBrace": {
+            "Fn::Sub": "}"
+          },
+          "DollarOpeningBrace": {
+            "Fn::Sub": "${"
+          },
+          "DollarClosingBrace": {
+            "Fn::Sub": "$}"
+          },
+          "OpeningBraceDollar": {
+            "Fn::Sub": "{$"
+          },
+          "ClosingBraceDollar": {
+            "Fn::Sub": "}$"
+          }
+        },
+        "SymbolsAndResources": {
+          "DollarSign": {
+            "Fn::Sub": "DoesNotExist$DoesNotExist"
+          },
+          "OpeningBrace": {
+            "Fn::Sub": "DoesNotExist{DoesNotExist"
+          },
+          "ClosingBrace": {
+            "Fn::Sub": "DoesNotExist}DoesNotExist"
+          },
+          "DollarOpeningBrace": {
+            "Fn::Sub": "DoesNotExist${DoesNotExist"
+          },
+          "DollarClosingBrace": {
+            "Fn::Sub": "DoesNotExist$}DoesNotExist"
+          },
+          "OpeningBraceDollar": {
+            "Fn::Sub": "DoesNotExist{$DoesNotExist"
+          },
+          "ClosingBraceDollar": {
+            "Fn::Sub": "DoesNotExist}$DoesNotExist"
+          }
+        }
+      }
+    }
+  }
+}

packages/@aws-cdk/cloudformation-include/test/test-templates/fn-sub-valid-escaping.json → packages/@aws-cdk/cloudformation-include/test/test-templates/fn-sub-escaping.json

@@ -3,7 +3,7 @@
     "Bucket": {
       "Type": "AWS::S3::Bucket",
       "Properties": {
-        "BucketName": { "Fn::Sub": "some-bucket${!AWS::AccountId}7896${ ! AWS::Region}${!Immediate}234" }
+        "BucketName": { "Fn::Sub": "some-bucket${!AWS::AccountId}7896${ ! DoesNotExist}${!Immediate}234" }
       }
     }
   }

packages/@aws-cdk/cloudformation-include/test/test-templates/fn-sub-map-dotted-attributes.json

@@ -4,13 +4,7 @@
       "Type": "AWS::S3::Bucket",
       "Properties": {
         "BucketName": { 
-          "Fn::Sub": [ 
-            "${Region}${ELB}-${ELB.SourceSecurityGroup.GroupName}-${Name}", 
-            { 
-              "Name": { "Ref" : "ELB" }, 
-              "Region": { "Fn::Base64": "AWS::Region" }
-            } 
-          ]
+          "Fn::Sub": "${ELB.SourceSecurityGroup.GroupName}"
         }
       }
     },
@@ -20,8 +14,7 @@
         "AvailabilityZones": [
           "us-east-1a"
         ],
-        "Listeners": [
-        {
+        "Listeners": [{
           "LoadBalancerPort": "80",
           "InstancePort": "80",
           "Protocol": "HTTP"

packages/@aws-cdk/cloudformation-include/test/test-templates/fn-sub-override.json

@@ -0,0 +1,16 @@
+{
+  "Resources": {
+    "Bucket": {
+      "Type": "AWS::S3::Bucket",
+      "Properties": {
+        "BucketName": "bucket"
+      }
+    },
+    "AnotherBucket": {
+      "Type": "AWS::S3::Bucket",
+      "Properties": {
+        "BucketName": { "Fn::Sub": "${Bucket}-${!Bucket}-${Bucket.DomainName}" }
+      }
+    }
+  }
+}

packages/@aws-cdk/cloudformation-include/test/test-templates/fn-sub-shadow-attribute.json

@@ -14,10 +14,7 @@
       }
     },
     "AnotherBucket": {
-      "Type": "AWS::S3::Bucket",
-      "Properties": {
-        "BucketName": "another-bucket"
-      }
+      "Type": "AWS::S3::Bucket"
     }
   }
 }

packages/@aws-cdk/cloudformation-include/test/test-templates/fn-sub-shadow.json

@@ -14,10 +14,7 @@
       }
     },
     "AnotherBucket": {
-      "Type": "AWS::S3::Bucket",
-      "Properties": {
-        "BucketName": "another-bucket"
-      }
+      "Type": "AWS::S3::Bucket"
     }
   }
 }

packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/fn-sub-bad-get-att.json → packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/fn-sub-${}-only.json

@@ -1,9 +1,11 @@
 {
   "Resources": {
-    "Resource": {
+    "Bucket": {
       "Type": "AWS::S3::Bucket",
       "Properties": {
-        "BucketName": { "Fn::Sub": "${FakeBucket.BucketName}" }
+        "BucketName": { 
+          "Fn::Sub": "${}"
+        }
       }
     }
   }

packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/fn-sub-key-not-in-template-string.json

@@ -1,9 +1,9 @@
 {
   "Resources": {
-    "AnotherBucket": {
+    "Bucket": {
       "Type": "AWS::S3::Bucket",
       "Properties": {
-        "AccessControl": { "Fn::Sub": "unusued-bucket-fn-sub-${AFakeResource}" }
+        "AccessControl": { "Fn::Sub": "${AFakeResource}" }
       }
     }
   }

packages/@aws-cdk/cloudformation-include/test/test-templates/yaml/invalid/short-form-import-sub.yaml

@@ -0,0 +1,7 @@
+Resources:
+  Bucket:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName:
+        !ImportValue
+          !Sub ${AWS::Region}

packages/@aws-cdk/cloudformation-include/test/test-templates/yaml/long-form-vpc.yaml

@@ -43,9 +43,10 @@ Resources:
               AnotherParameter:
                 Fn::Base64: AnotherValue
       AccessControl:
-        Fn::Sub:
-          - "${Region}-foo-${!Immediate}-foo-${Vpc}-${Vpc.Id}-${Name}"
-          - Name:
-              Ref: Vpc
-            Region:
-              Fn::Base64: AWS::Region
+        Fn::ImportValue: 
+          Fn::Sub:
+            - "${Region}-foo-${!Immediate}-foo-${Vpc}-${Vpc.Id}-${Name}"
+            - Name:
+                Ref: Vpc
+              Region:
+                Fn::Base64: AWS::Region

packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts

@@ -193,18 +193,18 @@ describe('CDK Include', () => {
   });
 
   test('can ingest a template with Fn::Sub in string form with escaped and unescaped references and output it unchanged', () => {
-    includeTestTemplate(stack, 'fn-sub-valid-string.json');
+    includeTestTemplate(stack, 'fn-sub-string.json');
 
     expect(stack).toMatchTemplate(
-      loadTestFileToJsObject('fn-sub-valid-string.json'),
+      loadTestFileToJsObject('fn-sub-string.json'),
     );
   });
 
-  test('can ingest a template with Fn::Sub in string form with escaped references and output it unchanged', () => {
-    includeTestTemplate(stack, 'fn-sub-valid-escaping.json');
+  test('can parse the string argument Fn::Sub with escaped references that contain whitespace', () => {
+    includeTestTemplate(stack, 'fn-sub-escaping.json');
 
     expect(stack).toMatchTemplate(
-      loadTestFileToJsObject('fn-sub-valid-escaping.json'),
+      loadTestFileToJsObject('fn-sub-escaping.json'),
     );
   });
 
@@ -216,23 +216,23 @@ describe('CDK Include', () => {
     );
   });
 
-  test('can ingest a template with Fn::Sub with a key that is a logicalID and output it unchanged', () => {
+  test('can ingest a template with Fn::Sub shadowing a logical ID from the template and output it unchanged', () => {
     includeTestTemplate(stack, 'fn-sub-shadow.json');
 
     expect(stack).toMatchTemplate(
       loadTestFileToJsObject('fn-sub-shadow.json'),
     );
   });
 
-  test('can ingest a template with Fn::Sub with a key that is a logicalID with an attribue and output it unchanged', () => {
+  test('can ingest a template with Fn::Sub attribute expression shadowing a logical ID from the template, and output it unchanged', () => {
     includeTestTemplate(stack, 'fn-sub-shadow-attribute.json');
 
     expect(stack).toMatchTemplate(
       loadTestFileToJsObject('fn-sub-shadow-attribute.json'),
     );
   });
 
-  test('can modify resources used in Fn::Sub references and see the changes in the template', () => {
+  test('can modify resources used in Fn::Sub in map form references and see the changes in the template', () => {
     const cfnTemplate = includeTestTemplate(stack, 'fn-sub-shadow.json');
 
     cfnTemplate.getResource('AnotherBucket').overrideLogicalId('NewBucket');
@@ -249,6 +249,26 @@ describe('CDK Include', () => {
     });
   });
 
+  test('can modify resources used in Fn::Sub in string form and see the changes in the template', () => {
+    const cfnTemplate = includeTestTemplate(stack, 'fn-sub-override.json');
+
+    cfnTemplate.getResource('Bucket').overrideLogicalId('NewBucket');
+
+    expect(stack).toHaveResourceLike('AWS::S3::Bucket', {
+      "BucketName": {
+        "Fn::Sub": "${NewBucket}-${!Bucket}-${NewBucket.DomainName}",
+      },
+    });
+  });
+
+  test('can ingest a template with Fn::Sub with brace edge cases and output it unchanged', () => {
+    includeTestTemplate(stack, 'fn-sub-brace-edges.json');
+
+    expect(stack).toMatchTemplate(
+      loadTestFileToJsObject('fn-sub-brace-edges.json'),
+    );
+  });
+
   test('can ingest a template with a Ref expression for an array value, and output it unchanged', () => {
     includeTestTemplate(stack, 'ref-array-property.json');
 

packages/@aws-cdk/cloudformation-include/test/yaml-templates.test.ts

@@ -324,21 +324,27 @@ describe('CDK Include', () => {
     );
   });
 
-  test('can ingest a yaml tempalte with Fn::Sub in string form and output it unchanged', () => {
-    includeTestTemplate(stack, 'sub-string.yaml');
+  test('can ingest a YAML tempalte with Fn::Sub in string form and output it unchanged', () => {
+    includeTestTemplate(stack, 'short-form-fnsub-string.yaml');
 
     expect(stack).toMatchTemplate(
-      loadTestFileToJsObject('sub-string.yaml'),
+      loadTestFileToJsObject('short-form-fnsub-string.yaml'),
     );
   });
 
-  test('can ingest a yaml tempalte with Fn::Sub in map form and output it unchanged', () => {
+  test('can ingest a YAML tmeplate with Fn::Sub in map form and output it unchanged', () => {
     includeTestTemplate(stack, 'short-form-sub-map.yaml');
 
     expect(stack).toMatchTemplate(
       loadTestFileToJsObject('short-form-sub-map.yaml'),
     );
   });
+
+  test('the parser throws an error on a YAML tmeplate with short form import value that uses short form sub', () => {
+    expect(() => {
+      includeTestTemplate(stack, 'invalid/short-form-import-sub.yaml');
+    }).toThrow(/A node can have at most one tag/);
+  });
 });
 
 function includeTestTemplate(scope: core.Construct, testTemplate: string): inc.CfnInclude {