Fix: SAM crashes method_definition for path is invalid #1802
Conversation
| """ | ||
|
|
||
| if parameters is None or not isinstance(parameters, dict): | ||
| raise TypeError("parameters must be a valid dictionary") |
There was a problem hiding this comment.
Is TypeError causes 500 errors?
There was a problem hiding this comment.
If we raise TypeError instead of InvalidDocumentException, will it cause 500 server error on lambda execution? Is that the reason we changed the type of the exception here?
There was a problem hiding this comment.
ohh i see. yes TypeError causes 500 server error.
| for method_definition in self.get_method_contents(self.get_path(path)[normalized_method_name]): | ||
|
|
||
| # If no integration given, then we don't need to process this definition (could be AWS::NoValue) | ||
| if not isinstance(method_definition, dict): |
There was a problem hiding this comment.
Can we start refactoring these branches? The patterns are essentially identical, but takes 8 LoC for a single check.
There was a problem hiding this comment.
irrelevant: we should have mypy to end this manual type check nightmare!
There was a problem hiding this comment.
if we define the possible types of method_definition, and mypy will scream if you don't have this check.
There was a problem hiding this comment.
This is the challenge right now and itself could be a lot of work (of course worth doing) which is to assess and note every parameter's data type. If we can do that then it would solve all current and future SAM crashes as well.
There was a problem hiding this comment.
@prenx4x grep InvalidDocumentException . -RC2 gives me a bunch of very similar checks: if value doesn't match type, throw the exception. It's noisy and repetitive, would be good to start refactoring these so we can simplify common patterns and reduce cognitive load, otherwise it won't be maintainable.
There was a problem hiding this comment.
And more generally, this sort of "lazy" checking doesn't seem to be ideal; maybe we can validate up-front what needs to be validated, and use EAFP elsewhere. This would reduce complexity and improve maintainability.
There was a problem hiding this comment.
I see. One thing to note here is that the errors could be similar, but the message (or cause) is very different. This message is seen by the user and it should be clear enough to point out the issue in their template. We we find same errors with same cause at multiple places we can refactor it, but if causes are different then they should be separate exceptions.
There was a problem hiding this comment.
Right, and that's fine; messages can be custom. But we should still be continuously improving our codebase.
This doesn't mean refactoring everything at once, but if we recognize common patterns that can benefit from refactoring in new code, we should go ahead and do it, and ensure that in the long term our quality keeps increasing.
* Fix: SAM crashes method_definition for path is invalid * Fix: SAM crashes whenmappings is null * Removed print statement in test_translator * Fix: Swagger security not a dict Co-authored-by: Mufaddal Makati <[email protected]>
* Support DLQ, RetryPolicy properties for EventBridgeRule,Schedule event sources (#1842) * Add DeadLetterConfig,RetryPolicy properties for EventBridgeRule,Schedule event sources * Minor fix,rename function argument * Update test class name * Combine dlq extraction/generation into the utility class * Remove unused import * fix: propagate condition to sqs queue policy for sqssubscription (#1798) * fix: propagate condition to sqs queue policy for sqssubscription * Update unit test for function_event_conditions * Update black commands in Makefile to check only .py files * Update test with one more SNS event source with sqsSubscription set * Revert "Update black commands in Makefile to check only .py files" This reverts commit 115ff09. * chore: Remove biased language from pylintrc (#1847) * fix: Support new CodeDeploy ManagedPolicy (#1858) * fix: Support new CodeDeploy MangedPolicy in regions without AWSCodeDeployRoleForLambda CodeDeploy is migrating from AWSCodeDeployRoleForLambda to AWSCodeDeployRoleForLambdaLimited. Some partitions do not support AWSCodeDeployRoleForLambda and therefore we need to use the newer one in those partitions. We cannot widely update to AWSCodeDeployRoleForLambdaLimited since this can cause customer's stacks to fail unexpectedly. * Forgot to commit unit tests * Handle PR feedback Co-authored-by: Jacob Fuss <[email protected]> * fix: Update Slack invite link (#1877) Co-authored-by: Jacob Fuss <[email protected]> * feature: Support for custom checkpointing (#1883) Co-authored-by: Vinayak <[email protected]> * Fix: Description in AWS::Serverless::HttpApi (#1884) * Fix: Description in AWS::Serverless::HttpApi * Update _set to _add * Update AWS::S3::Bucket properties (#1885) * Update AWS::S3::Bucket properties * Fix type checking validators for AWS::S3::Bucket * Update to use any_type() in favor of supporing ref * Fix: Replaced invalid AMQ managed policy by providing policy statements (#1891) * Fix for invalid MQ event source managed policy * Fix for invalid managed policy for MQ, included support for new MQ event source property, updated test cases * Black reformatting * Test case changes * Changed policy name * Modified test cases with new policy name * chore: bump version 1.34.0 (#1892) * Fix: SAM crashes method_definition for path is invalid (#1802) * Fix: SAM crashes method_definition for path is invalid * Fix: SAM crashes whenmappings is null * Removed print statement in test_translator * Fix: Swagger security not a dict Co-authored-by: Mufaddal Makati <[email protected]> Co-authored-by: ejafarli <[email protected]> Co-authored-by: _sam <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: vinayaksood <[email protected]> Co-authored-by: Vinayak <[email protected]> Co-authored-by: Qingchuan Ma <[email protected]> Co-authored-by: Mufaddal Makati <[email protected]> Co-authored-by: Mufaddal Makati <[email protected]>
* Support DLQ, RetryPolicy properties for EventBridgeRule,Schedule event sources (#1842) * Add DeadLetterConfig,RetryPolicy properties for EventBridgeRule,Schedule event sources * Minor fix,rename function argument * Update test class name * Combine dlq extraction/generation into the utility class * Remove unused import * fix: propagate condition to sqs queue policy for sqssubscription (#1798) * fix: propagate condition to sqs queue policy for sqssubscription * Update unit test for function_event_conditions * Update black commands in Makefile to check only .py files * Update test with one more SNS event source with sqsSubscription set * Revert "Update black commands in Makefile to check only .py files" This reverts commit 115ff09. * chore: Remove biased language from pylintrc (#1847) * fix: Support new CodeDeploy ManagedPolicy (#1858) * fix: Support new CodeDeploy MangedPolicy in regions without AWSCodeDeployRoleForLambda CodeDeploy is migrating from AWSCodeDeployRoleForLambda to AWSCodeDeployRoleForLambdaLimited. Some partitions do not support AWSCodeDeployRoleForLambda and therefore we need to use the newer one in those partitions. We cannot widely update to AWSCodeDeployRoleForLambdaLimited since this can cause customer's stacks to fail unexpectedly. * Forgot to commit unit tests * Handle PR feedback Co-authored-by: Jacob Fuss <[email protected]> * fix: Update Slack invite link (#1877) Co-authored-by: Jacob Fuss <[email protected]> * feature: Support for custom checkpointing (#1883) Co-authored-by: Vinayak <[email protected]> * Fix: Description in AWS::Serverless::HttpApi (#1884) * Fix: Description in AWS::Serverless::HttpApi * Update _set to _add * Update AWS::S3::Bucket properties (#1885) * Update AWS::S3::Bucket properties * Fix type checking validators for AWS::S3::Bucket * Update to use any_type() in favor of supporing ref * Fix: Replaced invalid AMQ managed policy by providing policy statements (#1891) * Fix for invalid MQ event source managed policy * Fix for invalid managed policy for MQ, included support for new MQ event source property, updated test cases * Black reformatting * Test case changes * Changed policy name * Modified test cases with new policy name * chore: bump version 1.34.0 (#1892) * Fix: SAM crashes method_definition for path is invalid (#1802) * Fix: SAM crashes method_definition for path is invalid * Fix: SAM crashes whenmappings is null * Removed print statement in test_translator * Fix: Swagger security not a dict Co-authored-by: Mufaddal Makati <[email protected]> Co-authored-by: ejafarli <[email protected]> Co-authored-by: _sam <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: vinayaksood <[email protected]> Co-authored-by: Vinayak <[email protected]> Co-authored-by: Qingchuan Ma <[email protected]> Co-authored-by: Mufaddal Makati <[email protected]> Co-authored-by: Mufaddal Makati <[email protected]> Co-authored-by: ejafarli <[email protected]> Co-authored-by: _sam <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: vinayaksood <[email protected]> Co-authored-by: Vinayak <[email protected]> Co-authored-by: Qingchuan Ma <[email protected]> Co-authored-by: Mufaddal Makati <[email protected]> Co-authored-by: Mufaddal Makati <[email protected]>
4 participants
Issue #, if available:
Description of changes:
Fix: SAM crashes method_definition for path is invalid
Fix: SAM crashes when mappings is null
Fix: Swagger security not a dict
Description of how you validated changes:
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.