handle 'Invalid Swagger Document' and refactor some validation into Swagger Editor constructor #2263

torresxb1 · 2021-12-14T10:36:12Z

Issue #, if available:

Description of changes:
Mostly refactoring to make validation a bit less messy.
Also handles Invalid Swagger Document ValueError.

Description of how you validated changes:

Checklist:

Add/update tests using:
- Correct values
- Bad/wrong values (None, empty, wrong type, length, etc.)
- Intrinsic Functions
make pr passes
Update documentation
Verify transformed template deploys and application functions as expected

Examples?

Please reach out in the comments, if you want to add an example. Examples will be
added to sam init through https://github.com/awslabs/aws-sam-cli-app-templates/

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

…wagger Editor constructor

… into handle_invalid_swagger_document

torresxb1 · 2021-12-14T19:23:07Z

samtranslator/model/api/api_generator.py

@@ -237,6 +237,8 @@ def __init__(
        self.template_conditions = template_conditions
        self.mode = mode

+        self.swagger_editor = SwaggerEditor(self.definition_body) if self.definition_body else None


A lot of the validation would happen in the SwaggerEditor constructor after these changes, so instantiating it in the ApiGenerator constructor will make the validation happen early.
Also, this file used to instantiate multiple SwaggerEditors because it kept modifying self.definition_body and then would use the new value in each new instance of SwaggerEditor. Now, there's only one SwaggerEditor used and only at the end we use all of the changes made to update self.definition_body once.

torresxb1 · 2021-12-14T19:23:58Z

samtranslator/model/api/api_generator.py

@@ -280,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.definition_body)
+            self.definition_body = self._openapi_postprocess(self.swagger_editor.swagger)


This is now the only time we update self.definition_body with the result of all of the changes we made with the SwaggerEditor

torresxb1 · 2021-12-14T19:25:25Z

samtranslator/model/api/api_generator.py

-        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.",
-            )
-


removing all of these kind of validations, since they're now done when SwaggerEditor is instantiated

torresxb1 · 2021-12-14T19:27:31Z

samtranslator/swagger/swagger.py

-        # https://swagger.io/specification/#path-item-object
-        # According to swagger spec,
-        # each path item object must be a dict (even it is empty).
-        # We can do an early path validation on path item objects,
-        # so we don't need to validate wherever we use them.
-        for path in self.iter_on_path():
-            SwaggerEditor.validate_path_item_is_dict(self.get_path(path), path)


moved this into validate_definition_body

… into handle_invalid_swagger_document

codecov-commenter · 2022-01-04T02:33:18Z

Codecov Report

Merging #2263 (dda9f61) into develop (e7a1496) will increase coverage by 0.84%.
The diff coverage is 96.95%.

@@             Coverage Diff             @@
##           develop    #2263      +/-   ##
===========================================
+ Coverage    93.58%   94.42%   +0.84%     
===========================================
  Files           90       97       +7     
  Lines         6124     7073     +949     
  Branches      1260     1430     +170     
===========================================
+ Hits          5731     6679     +948     
+ Misses         183      179       -4     
- Partials       210      215       +5

Impacted Files	Coverage Δ
samtranslator/model/api/http_api_generator.py	`91.21% <75.00%> (+0.02%)`	⬆️
samtranslator/model/api/api_generator.py	`94.01% <90.62%> (-0.35%)`	⬇️
samtranslator/model/apigateway.py	`96.98% <96.00%> (-0.18%)`	⬇️
samtranslator/__init__.py	`100.00% <100.00%> (ø)`
samtranslator/feature_toggle/dialup.py	`100.00% <100.00%> (ø)`
samtranslator/feature_toggle/feature_toggle.py	`100.00% <100.00%> (+12.16%)`	⬆️
samtranslator/intrinsics/actions.py	`98.79% <100.00%> (+0.07%)`	⬆️
samtranslator/metrics/method_decorator.py	`100.00% <100.00%> (ø)`
samtranslator/metrics/metrics.py	`100.00% <100.00%> (ø)`
samtranslator/model/__init__.py	`97.64% <100.00%> (ø)`
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5db070...dda9f61. Read the comment docs.

hawflau · 2022-01-17T18:55:24Z

samtranslator/swagger/swagger.py

+        Checks if definition_body is a valid Swagger document
+
+        :param dict definition_body: Data to be validated
+        :return: True, if definition_body is a valid Swagger document


(minor): please also add docstring for the exception raised

oops, the docstring shouldn't say it returns True either. Thanks for catching it!

hoffa · 2022-01-17T19:40:28Z

samtranslator/swagger/swagger.py


+        self.paths = self._doc.get("paths")


Out of curiosity, why was _doc["paths"] changed to _doc.get("paths")?

I think at some point I had the self.validate_definition_body(doc) line after self.paths = self._doc["paths"]. So if paths wasn't there, it would throw a runtime error (vs. with .get it returns None). However, shouldn't make a difference now that validation happens before (self.validate_definition_body(doc) line before getting paths)

…n into Swagger Editor constructor (aws#2263)" This reverts commit 59df2db.

…n into Swagger Editor constructor (#2263)" (#2346) This reverts commit 59df2db.

torresxb1 added 4 commits December 13, 2021 16:44

handle 'Invalid Swagger Document' and refactor some validation into S…

13ebde7

…wagger Editor constructor

missed removing a comment

5b8cc25

Merge branch 'develop' of github.com:aws/serverless-application-model…

09b5a52

… into handle_invalid_swagger_document

move path item validation into method

9a0112f

github-actions bot added the pr/internal label Dec 14, 2021

torresxb1 commented Dec 14, 2021

View reviewed changes

torresxb1 added 5 commits January 3, 2022 12:27

Merge branch 'develop' of github.com:aws/serverless-application-model…

30015da

… into handle_invalid_swagger_document

update new function to use swagger editor instance

4d56f72

small change, changing test to use swagger instead of openapi

8fceaa0

Merge branch 'develop' of github.com:aws/serverless-application-model…

520f032

… into handle_invalid_swagger_document

update deployment hashes in some updated tests

fbd6636

hawflau requested changes Jan 17, 2022

View reviewed changes

torresxb1 requested a review from hawflau January 17, 2022 19:38

hoffa reviewed Jan 17, 2022

View reviewed changes

add docstring :raises

dda9f61

torresxb1 force-pushed the handle_invalid_swagger_document branch from ae44a68 to dda9f61 Compare January 17, 2022 19:43

moelasmar approved these changes Jan 17, 2022

View reviewed changes

hawflau approved these changes Jan 18, 2022

View reviewed changes

torresxb1 merged commit 59df2db into aws:develop Jan 24, 2022

torresxb1 deleted the handle_invalid_swagger_document branch March 14, 2022 20:20

torresxb1 added a commit to torresxb1/serverless-application-model that referenced this pull request Mar 14, 2022

Revert "handle 'Invalid Swagger Document' and refactor some validatio…

5fa9f3f

…n into Swagger Editor constructor (aws#2263)" This reverts commit 59df2db.

torresxb1 mentioned this pull request Mar 14, 2022

Revert "handle 'Invalid Swagger Document' and refactor some validatio… #2346

Merged

8 tasks

torresxb1 added a commit that referenced this pull request Mar 14, 2022

Revert "handle 'Invalid Swagger Document' and refactor some validatio…

efef989

…n into Swagger Editor constructor (#2263)" (#2346) This reverts commit 59df2db.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle 'Invalid Swagger Document' and refactor some validation into Swagger Editor constructor #2263

handle 'Invalid Swagger Document' and refactor some validation into Swagger Editor constructor #2263

torresxb1 commented Dec 14, 2021 •

edited

Loading

torresxb1 Dec 14, 2021

torresxb1 Dec 14, 2021

torresxb1 Dec 14, 2021

torresxb1 Dec 14, 2021

codecov-commenter commented Jan 4, 2022 •

edited

Loading

hawflau Jan 17, 2022

torresxb1 Jan 17, 2022

torresxb1 Jan 17, 2022

hoffa Jan 17, 2022 •

edited

Loading

torresxb1 Jan 17, 2022 •

edited

Loading

handle 'Invalid Swagger Document' and refactor some validation into Swagger Editor constructor #2263

handle 'Invalid Swagger Document' and refactor some validation into Swagger Editor constructor #2263

Conversation

torresxb1 commented Dec 14, 2021 • edited Loading

torresxb1 Dec 14, 2021

Choose a reason for hiding this comment

torresxb1 Dec 14, 2021

Choose a reason for hiding this comment

torresxb1 Dec 14, 2021

Choose a reason for hiding this comment

torresxb1 Dec 14, 2021

Choose a reason for hiding this comment

codecov-commenter commented Jan 4, 2022 • edited Loading

Codecov Report

hawflau Jan 17, 2022

Choose a reason for hiding this comment

torresxb1 Jan 17, 2022

Choose a reason for hiding this comment

torresxb1 Jan 17, 2022

Choose a reason for hiding this comment

hoffa Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

torresxb1 Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

torresxb1 commented Dec 14, 2021 •

edited

Loading

codecov-commenter commented Jan 4, 2022 •

edited

Loading

hoffa Jan 17, 2022 •

edited

Loading

torresxb1 Jan 17, 2022 •

edited

Loading