Skip to content

Conversation

torresxb1
Copy link
Contributor

@torresxb1 torresxb1 commented Nov 9, 2021

Issue #, if available:

Description of changes:
_openapi_postprocess expects the value at a path to be a dict. The following template would cause an unhandled error:

Resources:
  ApiWithInvalidPath:
    Type: AWS::Serverless::Api
    Properties:
      StageName: Prod
      OpenApiVersion: 3.0.1
      DefinitionBody:
        openapi: 3.0.1
        info:
          title: test invalid paths Api
        paths:
          /foo: null

This PR checks that it is a dict and throws an error if it's not. Also did some refactoring while I updated this part of the code.

Description of how you validated changes:
Added end-to-end tests

Checklist:

  • Add/update tests using:
  • 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.

@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2021

Codecov Report

Merging #2216 (5eb4f19) into develop (e7a1496) will increase coverage by 0.86%.
The diff coverage is 98.40%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2216      +/-   ##
===========================================
+ Coverage    93.58%   94.45%   +0.86%     
===========================================
  Files           90       95       +5     
  Lines         6124     6560     +436     
  Branches      1260     1324      +64     
===========================================
+ Hits          5731     6196     +465     
+ Misses         183      169      -14     
+ Partials       210      195      -15     
Impacted Files Coverage Δ
samtranslator/model/lambda_.py 93.10% <ø> (ø)
samtranslator/plugins/globals/globals.py 99.05% <ø> (ø)
samtranslator/translator/logical_id_generator.py 100.00% <ø> (+9.09%) ⬆️
samtranslator/region_configuration.py 77.77% <63.63%> (-22.23%) ⬇️
samtranslator/translator/translator.py 97.26% <90.00%> (+0.07%) ⬆️
samtranslator/swagger/swagger.py 94.00% <92.68%> (+0.63%) ⬆️
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.75% <100.00%> (+0.03%) ⬆️
... 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 dd9d814...5eb4f19. Read the comment docs.

@torresxb1 torresxb1 changed the title fix: handle non-dict DefinitionBody path item and options responses value fix: handle non-dict DefinitionBody path item in _openapi_postprocess Nov 9, 2021
Comment on lines +1227 to +1251
@staticmethod
def validate_is_dict(obj, exception_message):
"""
Throws exception if obj is not a dict
:param obj: object being validated
:param exception_message: message to include in exception if obj is not a dict
"""

if not isinstance(obj, dict):
raise InvalidDocumentException([InvalidTemplateException(exception_message)])

@staticmethod
def validate_path_item_is_dict(path_item, path):
"""
Throws exception if path_item is not a dict
:param path_item: path_item (value at the path) being validated
:param path: path name
"""

SwaggerEditor.validate_is_dict(
path_item, "Value of '{}' path must be a dictionary according to Swagger spec.".format(path)
)

Copy link
Contributor Author

@torresxb1 torresxb1 Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure where's the best place to add this, but thought this was good for now since some validation is done in this file. Also am I supposed to also add this to open_api.py?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you add a new method, and did not used validate_is_dict. The new method is just a wrapper for the existing one.

Copy link
Contributor Author

@torresxb1 torresxb1 Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because this same action (validating the path item) would be done in swagger.py and api_generator.py. I mainly just didn't want to repeat the error message and make sure that it was consistent in every place if it's changed in the future. Not sure if there's a better way of doing it, where I just generate the error message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@torresxb1 torresxb1 marked this pull request as ready for review November 10, 2021 00:00
@moelasmar moelasmar self-requested a review November 15, 2021 19:16
@torresxb1 torresxb1 merged commit c8e4da9 into aws:develop Nov 15, 2021
@torresxb1 torresxb1 deleted the handle_null_fields_in_definitionbody_options branch November 15, 2021 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants