Revert "handle 'Invalid Swagger Document' and refactor some validation into Swagger Editor constructor (#2263)" (#2346)

This reverts commit 59df2db.

Author: torresxb1

samtranslator/model/api/api_generator.py

@@ -236,8 +236,6 @@ def __init__(
         self.template_conditions = template_conditions
         self.mode = mode
 
-        self.swagger_editor = SwaggerEditor(self.definition_body) if self.definition_body else None
-
     def _construct_rest_api(self):
         """Constructs and returns the ApiGateway RestApi.
 
@@ -284,7 +282,7 @@ def _construct_rest_api(self):
             rest_api.BodyS3Location = self._construct_body_s3_dict()
         elif self.definition_body:
             # # Post Process OpenApi Auth Settings
-            self.definition_body = self._openapi_postprocess(self.swagger_editor.swagger)
+            self.definition_body = self._openapi_postprocess(self.definition_body)
             rest_api.Body = self.definition_body
 
         if self.name:
@@ -310,7 +308,9 @@ def _add_endpoint_extension(self):
             raise InvalidResourceException(
                 self.logical_id, "DisableExecuteApiEndpoint works only within 'DefinitionBody' property."
             )
-        self.swagger_editor.add_disable_execute_api_endpoint_extension(self.disable_execute_api_endpoint)
+        editor = SwaggerEditor(self.definition_body)
+        editor.add_disable_execute_api_endpoint_extension(self.disable_execute_api_endpoint)
+        self.definition_body = editor.swagger
 
     def _construct_body_s3_dict(self):
         """Constructs the RestApi's `BodyS3Location property`_, from the SAM Api's DefinitionUri property.
@@ -626,6 +626,13 @@ def _add_cors(self):
         else:
             raise InvalidResourceException(self.logical_id, INVALID_ERROR)
 
+        if not SwaggerEditor.is_valid(self.definition_body):
+            raise InvalidResourceException(
+                self.logical_id,
+                "Unable to add Cors configuration because "
+                "'DefinitionBody' does not contain a valid Swagger definition.",
+            )
+
         if properties.AllowCredentials is True and properties.AllowOrigin == _CORS_WILDCARD:
             raise InvalidResourceException(
                 self.logical_id,
@@ -634,9 +641,10 @@ def _add_cors(self):
                 "'AllowOrigin' is \"'*'\" or not set",
             )
 
-        for path in self.swagger_editor.iter_on_path():
+        editor = SwaggerEditor(self.definition_body)
+        for path in editor.iter_on_path():
             try:
-                self.swagger_editor.add_cors(
+                editor.add_cors(
                     path,
                     properties.AllowOrigin,
                     properties.AllowHeaders,
@@ -647,6 +655,9 @@ def _add_cors(self):
             except InvalidTemplateException as ex:
                 raise InvalidResourceException(self.logical_id, ex.message)
 
+        # Assign the Swagger back to template
+        self.definition_body = editor.swagger
+
     def _add_binary_media_types(self):
         """
         Add binary media types to Swagger
@@ -659,7 +670,11 @@ def _add_binary_media_types(self):
         if self.binary_media and not self.definition_body:
             return
 
-        self.swagger_editor.add_binary_media_types(self.binary_media)
+        editor = SwaggerEditor(self.definition_body)
+        editor.add_binary_media_types(self.binary_media)
+
+        # Assign the Swagger back to template
+        self.definition_body = editor.swagger
 
     def _add_auth(self):
         """
@@ -678,31 +693,40 @@ def _add_auth(self):
         if not all(key in AuthProperties._fields for key in self.auth.keys()):
             raise InvalidResourceException(self.logical_id, "Invalid value for 'Auth' property")
 
+        if not SwaggerEditor.is_valid(self.definition_body):
+            raise InvalidResourceException(
+                self.logical_id,
+                "Unable to add Auth configuration because "
+                "'DefinitionBody' does not contain a valid Swagger definition.",
+            )
+        swagger_editor = SwaggerEditor(self.definition_body)
         auth_properties = AuthProperties(**self.auth)
         authorizers = self._get_authorizers(auth_properties.Authorizers, auth_properties.DefaultAuthorizer)
 
         if authorizers:
-            self.swagger_editor.add_authorizers_security_definitions(authorizers)
+            swagger_editor.add_authorizers_security_definitions(authorizers)
             self._set_default_authorizer(
-                self.swagger_editor,
+                swagger_editor,
                 authorizers,
                 auth_properties.DefaultAuthorizer,
                 auth_properties.AddDefaultAuthorizerToCorsPreflight,
                 auth_properties.Authorizers,
             )
 
         if auth_properties.ApiKeyRequired:
-            self.swagger_editor.add_apikey_security_definition()
-            self._set_default_apikey_required(self.swagger_editor)
+            swagger_editor.add_apikey_security_definition()
+            self._set_default_apikey_required(swagger_editor)
 
         if auth_properties.ResourcePolicy:
             SwaggerEditor.validate_is_dict(
                 auth_properties.ResourcePolicy, "ResourcePolicy must be a map (ResourcePolicyStatement)."
             )
-            for path in self.swagger_editor.iter_on_path():
-                self.swagger_editor.add_resource_policy(auth_properties.ResourcePolicy, path, self.stage_name)
+            for path in swagger_editor.iter_on_path():
+                swagger_editor.add_resource_policy(auth_properties.ResourcePolicy, path, self.stage_name)
             if auth_properties.ResourcePolicy.get("CustomStatements"):
-                self.swagger_editor.add_custom_statements(auth_properties.ResourcePolicy.get("CustomStatements"))
+                swagger_editor.add_custom_statements(auth_properties.ResourcePolicy.get("CustomStatements"))
+
+        self.definition_body = self._openapi_postprocess(swagger_editor.swagger)
 
     def _construct_usage_plan(self, rest_api_stage=None):
         """Constructs and returns the ApiGateway UsagePlan, ApiGateway UsagePlanKey, ApiGateway ApiKey for Auth.
@@ -905,6 +929,15 @@ def _add_gateway_responses(self):
                         ),
                     )
 
+        if not SwaggerEditor.is_valid(self.definition_body):
+            raise InvalidResourceException(
+                self.logical_id,
+                "Unable to add Auth configuration because "
+                "'DefinitionBody' does not contain a valid Swagger definition.",
+            )
+
+        swagger_editor = SwaggerEditor(self.definition_body)
+
         # The dicts below will eventually become part of swagger/openapi definition, thus requires using Py27Dict()
         gateway_responses = Py27Dict()
         for response_type, response in self.gateway_responses.items():
@@ -916,7 +949,10 @@ def _add_gateway_responses(self):
             )
 
         if gateway_responses:
-            self.swagger_editor.add_gateway_responses(gateway_responses)
+            swagger_editor.add_gateway_responses(gateway_responses)
+
+        # Assign the Swagger back to template
+        self.definition_body = swagger_editor.swagger
 
     def _add_models(self):
         """
@@ -932,10 +968,22 @@ def _add_models(self):
                 self.logical_id, "Models works only with inline Swagger specified in " "'DefinitionBody' property."
             )
 
+        if not SwaggerEditor.is_valid(self.definition_body):
+            raise InvalidResourceException(
+                self.logical_id,
+                "Unable to add Models definitions because "
+                "'DefinitionBody' does not contain a valid Swagger definition.",
+            )
+
         if not all(isinstance(model, dict) for model in self.models.values()):
             raise InvalidResourceException(self.logical_id, "Invalid value for 'Models' property")
 
-        self.swagger_editor.add_models(self.models)
+        swagger_editor = SwaggerEditor(self.definition_body)
+        swagger_editor.add_models(self.models)
+
+        # Assign the Swagger back to template
+
+        self.definition_body = self._openapi_postprocess(swagger_editor.swagger)
 
     def _openapi_postprocess(self, definition_body):
         """

samtranslator/swagger/swagger.py

@@ -45,13 +45,19 @@ def __init__(self, doc):
         modifications on this copy.
 
         :param dict doc: Swagger document as a dictionary
-        :raises InvalidDocumentException if doc is invalid
+        :raises ValueError: If the input Swagger document does not meet the basic Swagger requirements.
         """
 
+        if not SwaggerEditor.is_valid(doc):
+            raise ValueError("Invalid Swagger document")
+
         self._doc = copy.deepcopy(doc)
-        self.validate_definition_body(doc)
+        self.paths = self._doc["paths"]
+        self.security_definitions = self._doc.get("securityDefinitions", Py27Dict())
+        self.gateway_responses = self._doc.get(self._X_APIGW_GATEWAY_RESPONSES, Py27Dict())
+        self.resource_policy = self._doc.get(self._X_APIGW_POLICY, Py27Dict())
+        self.definitions = self._doc.get("definitions", Py27Dict())
 
-        self.paths = self._doc.get("paths")
         # https://swagger.io/specification/#path-item-object
         # According to swagger spec,
         # each path item object must be a dict (even it is empty).
@@ -61,11 +67,6 @@ def __init__(self, doc):
             for path_item in self.get_conditional_contents(self.paths.get(path)):
                 SwaggerEditor.validate_path_item_is_dict(path_item, path)
 
-        self.security_definitions = self._doc.get("securityDefinitions", Py27Dict())
-        self.gateway_responses = self._doc.get(self._X_APIGW_GATEWAY_RESPONSES, Py27Dict())
-        self.resource_policy = self._doc.get(self._X_APIGW_POLICY, Py27Dict())
-        self.definitions = self._doc.get("definitions", Py27Dict())
-
     def get_conditional_contents(self, item):
         """
         Returns the contents of the given item.
@@ -165,6 +166,7 @@ def add_path(self, path, method=None):
 
         :param string path: Path name
         :param string method: HTTP method
+        :raises ValueError: If the value of `path` in Swagger is not a dictionary
         """
         method = self._normalize_method_name(method)
 
@@ -1284,32 +1286,6 @@ def is_valid(data):
                 )
         return False
 
-    def validate_definition_body(self, definition_body):
-        """
-        Checks if definition_body is a valid Swagger document
-
-        :param dict definition_body: Data to be validated
-        :raises InvalidDocumentException if definition_body is invalid
-        """
-
-        SwaggerEditor.validate_is_dict(definition_body, "DefinitionBody must be a dictionary.")
-        SwaggerEditor.validate_is_dict(
-            definition_body.get("paths"), "The 'paths' property of DefinitionBody must be present and be a dictionary."
-        )
-
-        has_swagger = definition_body.get("swagger")
-        has_openapi3 = definition_body.get("openapi") and SwaggerEditor.safe_compare_regex_with_string(
-            SwaggerEditor.get_openapi_version_3_regex(), definition_body["openapi"]
-        )
-        if not (has_swagger) and not (has_openapi3):
-            raise InvalidDocumentException(
-                [
-                    InvalidTemplateException(
-                        "DefinitionBody must have either: (1) a 'swagger' property or (2) an 'openapi' property with version 3.x or 3.x.x"
-                    )
-                ]
-            )
-
     @staticmethod
     def validate_is_dict(obj, exception_message):
         """

tests/model/api/test_api_generator.py

@@ -18,7 +18,7 @@ def test_construct_usage_plan_with_invalid_usage_plan_type(self, invalid_usage_p
             Mock(),
             Mock(),
             Mock(),
-            {"paths": {}, "openapi": "3.0.1"},
+            Mock(),
             Mock(),
             Mock(),
             Mock(),
@@ -39,7 +39,7 @@ def test_construct_usage_plan_with_invalid_usage_plan_fields(self, AuthPropertie
             Mock(),
             Mock(),
             Mock(),
-            {"paths": {}, "openapi": "3.0.1"},
+            Mock(),
             Mock(),
             Mock(),
             Mock(),

tests/swagger/test_swagger.py

@@ -18,7 +18,7 @@ class TestSwaggerEditor_init(TestCase):
     def test_must_raise_on_invalid_swagger(self):
 
         invalid_swagger = {"paths": {}}  # Missing "Swagger" keyword
-        with self.assertRaises(InvalidDocumentException):
+        with self.assertRaises(ValueError):
             SwaggerEditor(invalid_swagger)
 
     def test_must_succeed_on_valid_swagger(self):
@@ -32,13 +32,13 @@ def test_must_succeed_on_valid_swagger(self):
     def test_must_fail_on_invalid_openapi_version(self):
         invalid_swagger = {"openapi": "2.3.0", "paths": {"/foo": {}, "/bar": {}}}
 
-        with self.assertRaises(InvalidDocumentException):
+        with self.assertRaises(ValueError):
             SwaggerEditor(invalid_swagger)
 
     def test_must_fail_on_invalid_openapi_version_2(self):
         invalid_swagger = {"openapi": "3.1.1.1", "paths": {"/foo": {}, "/bar": {}}}
 
-        with self.assertRaises(InvalidDocumentException):
+        with self.assertRaises(ValueError):
             SwaggerEditor(invalid_swagger)
 
     def test_must_succeed_on_valid_openapi3(self):
@@ -53,7 +53,7 @@ def test_must_succeed_on_valid_openapi3(self):
     def test_must_fail_with_bad_values_for_path(self, invalid_path_item):
         invalid_swagger = {"openapi": "3.1.1.1", "paths": {"/foo": {}, "/bad": invalid_path_item}}
 
-        with self.assertRaises(InvalidDocumentException):
+        with self.assertRaises(ValueError):
             SwaggerEditor(invalid_swagger)
 
 

tests/translator/input/api_with_resource_refs.yaml

@@ -6,8 +6,8 @@ Resources:
     Properties:
       StageName: foo
       DefinitionBody:
-        paths: {}
-        openapi: "3.0.1"
+        "this": "is"
+        "a": "swagger"
 
   MyFunction:
     Type: "AWS::Serverless::Function"

tests/translator/input/error_api_definition_body_invalid_openapi_version.yaml

@@ -121,6 +121,14 @@ Resources:
       Auth:
         MyBad: Foo
 
+  AuthWithInvalidDefinitionBodyApi:
+    Type: AWS::Serverless::Api
+    Properties:
+      StageName: Prod
+      DefinitionBody: { invalid: true }
+      Auth:
+        DefaultAuthorizer: Foo
+
   AuthWithMissingDefaultAuthorizerApi:
     Type: AWS::Serverless::Api
     Properties: