Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Allow Implicit HTTP APIs to write when value of method is None #2413

Merged
merged 3 commits into from
Jun 16, 2022

Conversation

jfuss
Copy link
Contributor

@jfuss jfuss commented Jun 7, 2022

SAM would crash if a the customer was using Implicit APIs that had a method
whose value was None/Null. SAM assumed the value would be a dictonary and therefore
would crash with a NoneType error when adding a key/value pair.

Issue #, if available:
N/A

Description of changes:
SAM would crash if a the customer was using Implicit APIs that had a method
whose value was None/Null. SAM assumed the value would be a dictonary and therefore
would crash with a NoneType error when adding a key/value pair.

Description of how you validated changes:
Ran the example in the tests before the change, which produced a crash. The tests passed with the patch applied.

Checklist:

  • Add/update unit tests using:
  • Add/update integration tests
  • make pr passes
  • Update documentation
  • Verify transformed template deploys and application functions as expected
  • Do these changes include any template validations?
    • Did the newly validated properties support intrinsics prior to adding the validations? (If unsure, please review Intrinsic Functions before proceeding).
      • Does the pull request ensure that intrinsics remain functional with the new validations?

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.

SAM would crash if a the customer was using Implicit APIs that had a method
whose value was None/Null. SAM assumed the value would be a dictonary and therefore
would crash with a NoneType error when adding a key/value pair.
},
}

self.editor = OpenApiEditor(self.original_openapi)

def test_must_override_null_path(self):
path = "/newpath"
Copy link
Contributor

Choose a reason for hiding this comment

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

could you explain what you are trying to test here. my understanding is you are testing that the /nullmethod path in the original_openapi defined in the setUp method will be override. But I am not able to understand why you used /newpath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I swear I updated this.. It should be /nullmethod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did change it locally but somehow got a conflict and didn't update. Not sure how this happened but will be updated in the next diff.

@@ -0,0 +1,89 @@
{
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 this file got changed? I am not able to understand how it is related to this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I got tired out our long list of file names in our tests and instead make it auto created (like we do for error files). With that, the function_with_alias_and_code_shas256 test cases were never run (where not in the list). Therefore the outputs where not passing (and we were missing two .json files too). So instead of splitting this, I just combined them. I can split them out of this commit and into it's own if you thing that is best. Was being a bit lazy honestly and the PR was small enough that it felt ok.

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2022

Codecov Report

Merging #2413 (c1850b6) into develop (e7a1496) will increase coverage by 0.86%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2413      +/-   ##
===========================================
+ Coverage    93.58%   94.44%   +0.86%     
===========================================
  Files           90       98       +8     
  Lines         6124     7261    +1137     
  Branches      1260     1507     +247     
===========================================
+ Hits          5731     6858    +1127     
- Misses         183      195      +12     
+ Partials       210      208       -2     
Impacted Files Coverage Δ
samtranslator/region_configuration.py 77.77% <0.00%> (-22.23%) ⬇️
samtranslator/model/codedeploy.py 90.90% <0.00%> (-9.10%) ⬇️
samtranslator/validator/validator.py 91.80% <0.00%> (-3.85%) ⬇️
samtranslator/model/exceptions.py 97.67% <0.00%> (-2.33%) ⬇️
samtranslator/open_api/open_api.py 90.16% <0.00%> (-1.81%) ⬇️
samtranslator/model/s3_utils/uri_parser.py 68.42% <0.00%> (-0.81%) ⬇️
samtranslator/yaml_helper.py 89.47% <0.00%> (-0.53%) ⬇️
samtranslator/translator/logical_id_generator.py 90.62% <0.00%> (-0.29%) ⬇️
samtranslator/model/api/api_generator.py 94.08% <0.00%> (-0.28%) ⬇️
samtranslator/model/apigateway.py 96.98% <0.00%> (-0.18%) ⬇️
... and 46 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 5046f68...c1850b6. Read the comment docs.

@jfuss jfuss merged commit 30f206a into aws:develop Jun 16, 2022
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