fix: Fix two places that could cause internal errors (#2930)

samtranslator/model/__init__.py

@@ -9,7 +9,11 @@
 from pydantic.error_wrappers import ValidationError
 
 from samtranslator.intrinsics.resolver import IntrinsicsResolver
-from samtranslator.model.exceptions import ExpectedType, InvalidResourceException, InvalidResourcePropertyTypeException
+from samtranslator.model.exceptions import (
+    ExpectedType,
+    InvalidResourceException,
+    InvalidResourcePropertyTypeException,
+)
 from samtranslator.model.tags.resource_tagging import get_tag_list
 from samtranslator.model.types import IS_DICT, IS_STR, Validator, any_type, is_type
 from samtranslator.plugins import LifeCycleEvents
@@ -121,7 +125,7 @@ class Resource(ABC):
 
     def __init__(
         self,
-        logical_id: str,
+        logical_id: Optional[Any],
         relative_id: Optional[str] = None,
         depends_on: Optional[List[str]] = None,
         attributes: Optional[Dict[str, Any]] = None,
@@ -134,8 +138,7 @@ def __init__(
         :param depends_on Value of DependsOn resource attribute
         :param attributes Dictionary of resource attributes and their values
         """
-        self._validate_logical_id(logical_id)  # type: ignore[no-untyped-call]
-        self.logical_id = logical_id
+        self.logical_id = self._validate_logical_id(logical_id)
         self.relative_id = relative_id
         self.depends_on = depends_on
 
@@ -214,8 +217,8 @@ def from_dict(cls, logical_id: str, resource_dict: Dict[str, Any], relative_id:
         resource.validate_properties()
         return resource
 
-    @classmethod
-    def _validate_logical_id(cls, logical_id):  # type: ignore[no-untyped-def]
+    @staticmethod
+    def _validate_logical_id(logical_id: Optional[Any]) -> str:
         """Validates that the provided logical id is an alphanumeric string.
 
         :param str logical_id: the logical id to validate
@@ -224,9 +227,12 @@ def _validate_logical_id(cls, logical_id):  # type: ignore[no-untyped-def]
         :raises TypeError: if the logical id is invalid
         """
         pattern = re.compile(r"^[A-Za-z0-9]+$")
-        if logical_id is not None and pattern.match(logical_id):
-            return True
-        raise InvalidResourceException(logical_id, "Logical ids must be alphanumeric.")
+        if isinstance(logical_id, str) and pattern.match(logical_id):
+            return logical_id
+        # TODO: Doing validation in this class is kind of off,
+        # we need to surface this validation to where the template is loaded
+        # or the logical IDs are generated.
+        raise InvalidResourceException(str(logical_id), "Logical ids must be alphanumeric.")
 
     @classmethod
     def _validate_resource_dict(cls, logical_id, resource_dict):  # type: ignore[no-untyped-def]

samtranslator/model/eventsources/push.py

@@ -634,47 +634,49 @@ class Api(PushEventSource):
     RequestModel: Optional[Dict[str, Any]]
     RequestParameters: Optional[List[Any]]
 
-    def resources_to_link(self, resources):  # type: ignore[no-untyped-def]
+    def resources_to_link(self, resources: Dict[str, Any]) -> Dict[str, Any]:
         """
         If this API Event Source refers to an explicit API resource, resolve the reference and grab
         necessary data from the explicit API
         """
+        return self.resources_to_link_for_rest_api(resources, self.relative_id, self.RestApiId)
 
+    @staticmethod
+    def resources_to_link_for_rest_api(
+        resources: Dict[str, Any], relative_id: str, raw_rest_api_id: Optional[Any]
+    ) -> Dict[str, Any]:
         # If RestApiId is a resource in the same template, then we try find the StageName by following the reference
         # Otherwise we default to a wildcard. This stage name is solely used to construct the permission to
         # allow this stage to invoke the Lambda function. If we are unable to resolve the stage name, we will
         # simply permit all stages to invoke this Lambda function
         # This hack is necessary because customers could use !ImportValue, !Ref or other intrinsic functions which
         # can be sometimes impossible to resolve (ie. when it has cross-stack references)
-        permitted_stage = "*"
         stage_suffix = "AllStages"
-        explicit_api = None
-        rest_api_id = self.get_rest_api_id_string(self.RestApiId)
+        explicit_api_resource_properties = None
+        rest_api_id = Api.get_rest_api_id_string(raw_rest_api_id)
         if isinstance(rest_api_id, str):
-            if (
-                rest_api_id in resources
-                and "Properties" in resources[rest_api_id]
-                and "StageName" in resources[rest_api_id]["Properties"]
-            ):
-                explicit_api = resources[rest_api_id]["Properties"]
-                permitted_stage = explicit_api["StageName"]
-
-                # Stage could be a intrinsic, in which case leave the suffix to default value
-                if isinstance(permitted_stage, str):
-                    if not permitted_stage:
-                        raise InvalidResourceException(rest_api_id, "StageName cannot be empty.")
-                    stage_suffix = permitted_stage
-                else:
-                    stage_suffix = "Stage"  # type: ignore[unreachable]
-
+            rest_api_resource = sam_expect(
+                resources.get(rest_api_id), relative_id, "RestApiId", is_sam_event=True
+            ).to_be_a_map("RestApiId property of Api event must reference a valid resource in the same template.")
+
+            explicit_api_resource_properties = sam_expect(
+                rest_api_resource.get("Properties", {}), rest_api_id, "Properties", is_resource_attribute=True
+            ).to_be_a_map()
+            permitted_stage = explicit_api_resource_properties.get("StageName")
+
+            # Stage could be an intrinsic, in which case leave the suffix to default value
+            if isinstance(permitted_stage, str):
+                if not permitted_stage:
+                    raise InvalidResourceException(rest_api_id, "StageName cannot be empty.")
+                stage_suffix = permitted_stage
             else:
-                # RestApiId is a string, not an intrinsic, but we did not find a valid API resource for this ID
-                raise InvalidEventException(
-                    self.relative_id,
-                    "RestApiId property of Api event must reference a valid resource in the same template.",
-                )
+                stage_suffix = "Stage"
 
-        return {"explicit_api": explicit_api, "api_id": rest_api_id, "explicit_api_stage": {"suffix": stage_suffix}}
+        return {
+            "explicit_api": explicit_api_resource_properties,
+            "api_id": rest_api_id,
+            "explicit_api_stage": {"suffix": stage_suffix},
+        }
 
     @cw_timer(prefix=FUNCTION_EVETSOURCE_METRIC_PREFIX)
     def to_cloudformation(self, **kwargs):  # type: ignore[no-untyped-def]

samtranslator/model/stepfunctions/events.py

@@ -299,42 +299,12 @@ class Api(EventSource):
     Auth: Optional[Dict[str, Any]]
     UnescapeMappingTemplate: Optional[bool]
 
-    def resources_to_link(self, resources):  # type: ignore[no-untyped-def]
+    def resources_to_link(self, resources: Dict[str, Any]) -> Dict[str, Any]:
         """
         If this API Event Source refers to an explicit API resource, resolve the reference and grab
         necessary data from the explicit API
         """
-
-        # If RestApiId is a resource in the same template, then we try find the StageName by following the reference
-        # Otherwise we default to a wildcard. This stage name is solely used to construct the permission to
-        # allow this stage to invoke the State Machine. If we are unable to resolve the stage name, we will
-        # simply permit all stages to invoke this State Machine
-        # This hack is necessary because customers could use !ImportValue, !Ref or other intrinsic functions which
-        # can be sometimes impossible to resolve (ie. when it has cross-stack references)
-        permitted_stage = "*"
-        stage_suffix = "AllStages"
-        explicit_api = None
-        rest_api_id = PushApi.get_rest_api_id_string(self.RestApiId)
-        if isinstance(rest_api_id, str):
-            if (
-                rest_api_id in resources
-                and "Properties" in resources[rest_api_id]
-                and "StageName" in resources[rest_api_id]["Properties"]
-            ):
-                explicit_api = resources[rest_api_id]["Properties"]
-                permitted_stage = explicit_api["StageName"]
-
-                # Stage could be a intrinsic, in which case leave the suffix to default value
-                stage_suffix = permitted_stage if isinstance(permitted_stage, str) else "Stage"
-
-            else:
-                # RestApiId is a string, not an intrinsic, but we did not find a valid API resource for this ID
-                raise InvalidEventException(
-                    self.relative_id,
-                    "RestApiId property of Api event must reference a valid resource in the same template.",
-                )
-
-        return {"explicit_api": explicit_api, "api_id": rest_api_id, "explicit_api_stage": {"suffix": stage_suffix}}
+        return PushApi.resources_to_link_for_rest_api(resources, self.relative_id, self.RestApiId)
 
     @cw_timer(prefix=SFN_EVETSOURCE_METRIC_PREFIX)
     def to_cloudformation(self, resource, **kwargs):  # type: ignore[no-untyped-def]

samtranslator/plugins/api/implicit_api_plugin.py

@@ -207,7 +207,7 @@ def _add_api_to_swagger(self, event_id, event_properties, template):  # type: ig
             and template.get(api_id).type != self.SERVERLESS_API_RESOURCE_TYPE
         )
 
-        # RestApiId is not pointing to a valid  API resource
+        # RestApiId is not pointing to a valid API resource
         if isinstance(api_id, dict) or is_referencing_http_from_api_event:
             raise InvalidEventException(
                 event_id,

tests/model/stepfunctions/test_api_event.py

@@ -75,6 +75,7 @@ def test_resources_to_link_with_explicit_api(self):
 
     def test_resources_to_link_with_undefined_explicit_api(self):
         resources = {}
+        self.api_event_source.relative_id = "event_id"
         self.api_event_source.RestApiId = {"Ref": "MyExplicitApi"}
         with self.assertRaises(InvalidEventException):
             self.api_event_source.resources_to_link(resources)

tests/translator/input/error_api_event_ref_invalid_resource.yaml

@@ -0,0 +1,18 @@
+AWSTemplateFormatVersion: '2010-09-09'
+
+Resources:
+  MyFunction:
+    Type: AWS::Serverless::Function
+    Properties:
+      Runtime: python3.9
+      Events:
+        Event1:
+          Type: Api
+          Properties:
+            Path: /channel/limit-validate
+            RestApiId: RestApi
+            Method: OPTIONS
+
+  RestApi:
+    Type: AWS::Serverless::Api
+    Properties: 123

tests/translator/input/error_api_event_ref_nothing.yaml

@@ -0,0 +1,14 @@
+AWSTemplateFormatVersion: '2010-09-09'
+
+Resources:
+  MyFunction:
+    Type: AWS::Serverless::Function
+    Properties:
+      Runtime: python3.9
+      Events:
+        Event1:
+          Type: Api
+          Properties:
+            Path: /channel/limit-validate
+            RestApiId: RestApi
+            Method: OPTIONS

tests/translator/input/error_invalid_logical_id.yaml

@@ -5,3 +5,9 @@ Resources:
       Runtime: python2.7
       Handler: hello.handler
       CodeUri: s3://bucket/code.zip
+  0:
+    Type: AWS::Serverless::Function
+    Properties:
+      Runtime: python2.7
+      Handler: hello.handler
+      CodeUri: s3://bucket/code.zip

tests/translator/output/error_api_event_ref_invalid_resource.json

@@ -0,0 +1,3 @@
+{
+  "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. Resource with id [RestApi] is invalid. Attribute 'Properties' should be a map."
+}

tests/translator/output/error_api_event_ref_nothing.json

@@ -0,0 +1,3 @@
+{
+  "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. Resource with id [MyFunction] is invalid. Event with id [Event1] is invalid. RestApiId must be a valid reference to an 'AWS::Serverless::Api' resource in same template."
+}

tests/translator/output/error_invalid_logical_id.json

@@ -1,8 +1,3 @@
 {
-  "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. Resource with id [Bad-Function-Name-@#$%^&] is invalid. Logical ids must be alphanumeric.",
-  "errors": [
-    {
-      "errorMessage": "Resource with id [Bad-Function-Name-@#$%^&] is invalid. Logical ids must be alphanumeric."
-    }
-  ]
+  "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 2. Resource with id [0] is invalid. Logical ids must be alphanumeric. Resource with id [Bad-Function-Name-@#$%^&] is invalid. Logical ids must be alphanumeric."
 }