Update resolution validation to only return an error if there are no …

…valid values (#3390) * Only raise resolution errors if all fail to validate * Update test results for new resolution errors
aws-cloudformation · Jun 24, 2024 · 25bfb1a · 25bfb1a
1 parent 3d9be79
commit 25bfb1a
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 17 deletions.
diff --git a/src/cfnlint/jsonschema/_resolvers_cfn.py b/src/cfnlint/jsonschema/_resolvers_cfn.py
@@ -11,7 +11,7 @@
 
 import regex as re
 
-from cfnlint.helpers import AVAILABILITY_ZONES
+from cfnlint.helpers import AVAILABILITY_ZONES, REGEX_SUB_PARAMETERS
 from cfnlint.jsonschema import ValidationError, Validator
 from cfnlint.jsonschema._typing import ResolutionResult
 from cfnlint.jsonschema._utils import equal
@@ -313,7 +313,21 @@ def sub(validator: Validator, instance: Any) -> ResolutionResult:
         return
 
     # its a string
-    yield from _sub_string(validator, instance)
+    sub_parameters = REGEX_SUB_PARAMETERS.findall(instance)
+    parameters = {}
+    for parameter in sub_parameters:
+        if "." in parameter:
+            parameters[parameter] = {"Fn::GetAtt": parameter}
+        else:
+            parameters[parameter] = {"Ref": parameter}
+    for resolved_parameters in _sub_parameter_expansion(validator, parameters):
+        resolved_validator = validator.evolve(
+            context=validator.context.evolve(
+                ref_values=resolved_parameters,
+            )
+        )
+        yield from _sub_string(resolved_validator, instance)
+        # yield from _sub_string(validator, instance)
 
 
 def if_(validator: Validator, instance: Any) -> ResolutionResult:

diff --git a/src/cfnlint/rules/functions/_BaseFn.py b/src/cfnlint/rules/functions/_BaseFn.py
@@ -62,20 +62,34 @@ def resolve(
     ) -> ValidationResult:
         key, _ = self.key_value(instance)
 
+        return_err: ValidationError | None = None
         for value, v, resolve_err in validator.resolve_value(instance):
             if resolve_err:
                 yield resolve_err
                 continue
-            for err in self.fix_errors(
-                v.descend(
-                    instance=value,
-                    schema=s,
-                    path=key,
+            errs = list(
+                self.fix_errors(
+                    v.descend(
+                        instance=value,
+                        schema=s,
+                        path=key,
+                    )
                 )
-            ):
-                err.message = err.message.replace(f"{value!r}", f"{instance!r}")
-                err.message = f"{err.message} when {self.fn.name!r} is resolved"
-                yield err
+            )
+
+            if not errs:
+                return
+
+            return_err = errs[0]
+
+        if return_err:
+            return_err.message = return_err.message.replace(
+                f"{value!r}", f"{instance!r}"
+            )
+            return_err.message = (
+                f"{return_err.message} when {self.fn.name!r} is resolved"
+            )
+            yield return_err
 
     def _resolve_ref(self, validator, schema) -> Any:
 

diff --git a/test/unit/module/cfn_yaml/test_yaml.py b/test/unit/module/cfn_yaml/test_yaml.py
@@ -27,7 +27,7 @@ def setUp(self):
             },
             "generic_bad": {
                 "filename": "test/fixtures/templates/bad/generic.yaml",
-                "failures": 33,
+                "failures": 28,
             },
         }
 

diff --git a/test/unit/module/test_rules_collections.py b/test/unit/module/test_rules_collections.py
@@ -69,7 +69,7 @@ def test_fail_run(self):
         filename = "test/fixtures/templates/bad/generic.yaml"
         template = cfnlint.decode.cfn_yaml.load(filename)
         cfn = Template(filename, template, ["us-east-1"])
-        expected_err_count = 36
+        expected_err_count = 31
         matches = []
         matches.extend(self.rules.run(filename, cfn))
         assert (

diff --git a/test/unit/rules/functions/test_sub.py b/test/unit/rules/functions/test_sub.py
@@ -26,6 +26,15 @@ def cfn():
     return Template(
         "",
         {
+            "Parameters": {
+                "MyParameter": {
+                    "Type": "String",
+                    "AllowedValues": [
+                        "one",
+                        "two",
+                    ],
+                }
+            },
             "Resources": {
                 "MyResource": {"Type": "AWS::S3::Bucket"},
                 "MySimpleAd": {"Type": "AWS::DirectoryService::SimpleAD"},
@@ -96,8 +105,8 @@ def context(cfn):
                 ValidationError(
                     (
                         "'bar' is not one of ["
-                        "'MyResource', 'MySimpleAd', 'MyPolicy', 'AWS::AccountId', "
-                        "'AWS::NoValue', 'AWS::NotificationARNs', "
+                        "'MyParameter', 'MyResource', 'MySimpleAd', 'MyPolicy', "
+                        "'AWS::AccountId', 'AWS::NoValue', 'AWS::NotificationARNs', "
                         "'AWS::Partition', 'AWS::Region', "
                         "'AWS::StackId', 'AWS::StackName', 'AWS::URLSuffix']"
                     ),
@@ -115,8 +124,8 @@ def context(cfn):
                 ValidationError(
                     (
                         "'foo' is not one of ["
-                        "'MyResource', 'MySimpleAd', 'MyPolicy', 'AWS::AccountId', "
-                        "'AWS::NoValue', 'AWS::NotificationARNs', "
+                        "'MyParameter', 'MyResource', 'MySimpleAd', 'MyPolicy', "
+                        "'AWS::AccountId', 'AWS::NoValue', 'AWS::NotificationARNs', "
                         "'AWS::Partition', 'AWS::Region', "
                         "'AWS::StackId', 'AWS::StackName', 'AWS::URLSuffix']"
                     ),
@@ -264,6 +273,25 @@ def context(cfn):
                 ),
             ],
         ),
+        (
+            "One valid resolution",
+            {"Fn::Sub": "${MyParameter}"},
+            {"type": "string", "const": "two"},
+            [],
+        ),
+        (
+            "No valid resolution",
+            {"Fn::Sub": "${MyParameter}"},
+            {"type": "string", "const": "three"},
+            [
+                ValidationError(
+                    ("'three' was expected when 'Fn::Sub' is resolved"),
+                    path=deque(["Fn::Sub"]),
+                    schema_path=deque(["const"]),
+                    validator="fn_sub",
+                ),
+            ],
+        ),
     ],
 )
 def test_validate(name, instance, schema, expected, rule, context, cfn):