Fix: Top SAM crashes - #1671
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1671 +/- ##
===========================================
- Coverage 94.10% 93.85% -0.25%
===========================================
Files 86 86
Lines 5408 5434 +26
Branches 1080 1091 +11
===========================================
+ Hits 5089 5100 +11
- Misses 148 155 +7
- Partials 171 179 +8
Continue to review full report at Codecov.
|
jfuss
left a comment
jfuss
left a comment
There was a problem hiding this comment.
Overall this is a good start. The major piece of feedback is, if we are going to call/say a template is invalid we should make sure the details are enough for the customer to see that and update/investigate the template accordingly without too much effort. The more descriptive we can be the better experience the customer will have.
There was a problem hiding this comment.
Why is this an error? I can understand raising this for when parameters is not a dict but why if parameters is None? We create this in a couple places (one being here). This Resolver gets created with the Parameters and Mappings, which means this error message is not correct. In the current state, the customer could send an invalid Mapping and we would report that the Parameters section is invalid.
Two things we need to do here:
- Understand why
parameters is Noneis important and document this for the future. - Update this error message to be more accurate. We should make sure information we return can be used by the customer to debug/fix the problem themselves with only that information.
There was a problem hiding this comment.
If this was raise to the customer, they would have no idea what this means as it is an implementation detail. We should understand deeper if this error is meant as a guard in the code or is something customers will run into. I don't think InvalidTemplateException is the right thing we want to throw here, as that will suggests customers template was wrong. Maybe a custom Exception and still fail with that Error is a better approach, as this looks like a guard in the code when SAM is used as a library not run in the service.
There was a problem hiding this comment.
We should add what is valid to help the customer understand what they need to change it too.
There was a problem hiding this comment.
Is it that it wasn't defined properly or that we couldn't resolve something or something else?
There was a problem hiding this comment.
Instead of adding an explicit check for api_auth here, you should update line 1112 to be api_auth = api.get("Auth", {}). This will default api_auth to be {}. Typically, this is preferred when dealing with dicts in python.
There was a problem hiding this comment.
Make sure to provide some details about where this is in the template. We generally do this through adding the logical id, so the customer at least knows what resource they need to dig into. This goes for all the exceptions, not just this one.
Codecov Report
@@ Coverage Diff @@
## develop #1671 +/- ##
===========================================
- Coverage 94.15% 93.80% -0.35%
===========================================
Files 86 87 +1
Lines 5439 5618 +179
Branches 1087 1130 +43
===========================================
+ Hits 5121 5270 +149
- Misses 147 163 +16
- Partials 171 185 +14
Continue to review full report at Codecov.
|
jfuss
left a comment
jfuss
left a comment
There was a problem hiding this comment.
Can we add additional test cases in test_transform_invalid_document to verify we have fixed the issue.
| if not default_authorizer: | ||
| return | ||
|
|
||
| if not isinstance(default_authorizer, Hashable): |
There was a problem hiding this comment.
I think this should be isinstance(default_authorizer, str) because this if will catch other cases that are hashable but we only expect a string.
| """ | ||
|
|
||
| if parameters is None or not isinstance(parameters, dict): | ||
| if parameters is None: |
There was a problem hiding this comment.
Should we be throwing TypeError for no parameters? This will crash SAM. We should validate through a test or if parameters can be None here, lets default parameters to {}.
| raise TypeError("parameters must be a valid dictionary") | ||
|
|
||
| if not isinstance(parameters, dict): | ||
| raise InvalidTemplateException("'Mappings' is not a valid dictionary") |
| def resources_to_link(self, resources): | ||
| if isinstance(self.Bucket, dict) and "Ref" in self.Bucket: | ||
| bucket_id = self.Bucket["Ref"] | ||
| if not isinstance(bucket_id, collections.Hashable): |
There was a problem hiding this comment.
Should this check for hashable or string?
| ), | ||
| ) | ||
|
|
||
| if not isinstance(method_authorizer, collections.Hashable): |
| model=method_model, method=self.Method, path=self.Path | ||
| ), | ||
| ) | ||
| if not isinstance(method_model, collections.Hashable): |
|
We can close this PR as I am breaking it in favor of #1757 and others to come. |
4 participants
Issue #, if available:
Description of changes:
Fixed some common SAM crashes
Description of how you validated changes:
Changes validated using unit tests
Checklist:
make prpassesExamples?
Please reach out in the comments, if you want to add an example. Examples will be
added to
sam initthrough 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.