Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion samtranslator/intrinsics/resolver.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Help resolve intrinsic functions

from samtranslator.intrinsics.actions import Action, SubAction, RefAction, GetAttAction
from samtranslator.model.exceptions import InvalidTemplateException, InvalidDocumentException

# All intrinsics are supported by default
DEFAULT_SUPPORTED_INTRINSICS = {action.intrinsic_name: action() for action in [RefAction, SubAction, GetAttAction]}
Expand All @@ -17,7 +18,9 @@ def __init__(self, parameters, supported_intrinsics=DEFAULT_SUPPORTED_INTRINSICS
"""

if parameters is None or not isinstance(parameters, dict):
raise TypeError("parameters must be a valid dictionary")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is TypeError causes 500 errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, what?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohh i see. yes TypeError causes 500 server error.

raise InvalidDocumentException(
[InvalidTemplateException("'Mappings' or 'Parameters' is either null or not a valid dictionary.")]
)

if not isinstance(supported_intrinsics, dict) or not all(
[isinstance(value, Action) for value in supported_intrinsics.values()]
Expand Down
16 changes: 16 additions & 0 deletions samtranslator/swagger/swagger.py
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,14 @@ def set_path_default_authorizer(
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):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we start refactoring these branches? The patterns are essentially identical, but takes 8 LoC for a single check.

Copy link
Contributor

Choose a reason for hiding this comment

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

irrelevant: we should have mypy to end this manual type check nightmare!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hoffa I dont understand. What (or how) do you mean refactor?
@aahung These errors are coming from user template. How will mypy know what is valid and what is not?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we define the possible types of method_definition, and mypy will scream if you don't have this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

raise InvalidDocumentException(
[
InvalidTemplateException(
"{} for path {} is not a valid dictionary.".format(method_definition, path)
)
]
)
if not self.method_definition_has_integration(method_definition):
continue
existing_security = method_definition.get("security", [])
Expand All @@ -548,6 +556,14 @@ def set_path_default_authorizer(
# (e.g. sigv4 (AWS_IAM), api_key (API Key/Usage Plans), NONE (marker for ignoring default))
# We want to ensure only a single Authorizer security entry exists while keeping everything else
for security in existing_security:
if not isinstance(security, dict):
raise InvalidDocumentException(
[
InvalidTemplateException(
"{} in Security for path {} is not a valid dictionary.".format(security, path)
)
]
)
if authorizer_names.isdisjoint(security.keys()):
existing_non_authorizer_security.append(security)
else:
Expand Down
5 changes: 3 additions & 2 deletions tests/intrinsics/test_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from mock import Mock, patch
from samtranslator.intrinsics.resolver import IntrinsicsResolver
from samtranslator.intrinsics.actions import Action
from samtranslator.model.exceptions import InvalidDocumentException


class TestParameterReferenceResolution(TestCase):
Expand Down Expand Up @@ -101,11 +102,11 @@ def test_skip_invalid_values_for_sub(self):
self.assertEqual(output, expected)

def test_throw_on_empty_parameters(self):
with self.assertRaises(TypeError):
with self.assertRaises(InvalidDocumentException):
IntrinsicsResolver(None).resolve_parameter_refs({})

def test_throw_on_non_dict_parameters(self):
with self.assertRaises(TypeError):
with self.assertRaises(InvalidDocumentException):
IntrinsicsResolver([1, 2, 3]).resolve_parameter_refs({})

def test_short_circuit_on_empty_parameters(self):
Expand Down
66 changes: 66 additions & 0 deletions tests/translator/input/error_invalid_method_definition.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
Globals:
Api:
Name: "some api"
CacheClusterEnabled: True
CacheClusterSize: "1.6"
Auth:
DefaultAuthorizer: MyCognitoAuth
Authorizers:
MyCognitoAuth:
UserPoolArn: !GetAtt MyUserPool.Arn
Variables:
SomeVar: Value

Resources:
ImplicitApiFunction:
Type: AWS::Serverless::Function
Properties:
CodeUri: s3://sam-demo-bucket/member_portal.zip
Handler: index.gethtml
Runtime: nodejs12.x
Events:
GetHtml:
Type: Api
Properties:
Path: /
Method: get

ExplicitApi:
Type: AWS::Serverless::Api
Properties:
StageName: SomeStage
DefinitionBody:
swagger: 2.0
info:
version: '1.0'
title: !Ref AWS::StackName
paths:
"/":
parameters:
- name: domain
in: path
description: Application domain
type: string
required: true
tags:
- InvalidMethodDefinition
get:
x-amazon-apigateway-integration:
httpMethod: POST
type: aws_proxy
uri: !Sub arn:aws:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${ImplicitApiFunction.Arn}/invocations
responses: {}

MyUserPool:
Type: AWS::Cognito::UserPool
Properties:
UserPoolName: UserPoolName
Policies:
PasswordPolicy:
MinimumLength: 8
UsernameAttributes:
- email
Schema:
- AttributeDataType: String
Name: email
Required: false
23 changes: 23 additions & 0 deletions tests/translator/input/error_mappings_is_null.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
Mappings:

Parameters:
Stage:
Type: String
Default: 'beta'
Deployment:
Type: String
Default: 'AllAtOnce'
Custom:
Type: String
Default: 'CustomDeployment'

Resources:
MinimalFunction:
Type: 'AWS::Serverless::Function'
Properties:
CodeUri: s3://sam-demo-bucket/hello.zip
Handler: hello.handler
Runtime: python2.7
AutoPublishAlias: live
DeploymentPreference:
Type: TestDeploymentConfiguration
70 changes: 70 additions & 0 deletions tests/translator/input/error_swagger_security_not_dict.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
transformId: AWS::Serverless-2016-10-31
AWSTemplateFormatVersion: '2010-09-09'
Resources:
AuthFunction:
Type: AWS::Serverless::Function
AccessingPartyAPI:
Type: AWS::Serverless::Api
Properties:
EndpointConfiguration: REGIONAL
StageName: demo
Auth:
DefaultAuthorizer: CustomAuthorizer
Authorizers:
CustomAuthorizer:
FunctionPayloadType: TOKEN
FunctionArn:
Fn::GetAtt:
- AuthFunction
- Arn
AddDefaultAuthorizerToCorsPreflight: false
DefinitionBody:
paths:
"/path":
put:
responses:
'201':
content:
application/json:
schema:
"$ref": "abcd"
x-amazon-apigateway-integration:
contentHandling: CONVERT_TO_TEXT
responses:
default:
statusCode: '200'
uri:
Fn::Sub: foobar
httpMethod: POST
passthroughBehavior: when_no_match
type: aws_proxy
requestBody:
content:
application/json:
schema:
required:
- readoutId
- status
type: object
security:
CustomAuthorizer: []

openapi: 3.0.3
components:
securitySchemes:
CustomAuthorizer:
in: header
type: apiKey
name: Authorization

AccessingPartyAPIFunction:
Type: AWS::Serverless::Function
Properties:
Events:
PutReservation:
Type: Api
Properties:
Path: "/path"
RestApiId:
Ref: AccessingPartyAPI
Method: put
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"errorMessage":"Invalid Serverless Application Specification document. Number of errors found: 1. Structure of the SAM template is invalid. ['InvalidMethodDefinition'] for path / is not a valid dictionary."}
1 change: 1 addition & 0 deletions tests/translator/output/error_mappings_is_null.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. Structure of the SAM template is invalid. 'Mappings' or 'Parameters' is either null or not a valid dictionary."}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. Structure of the SAM template is invalid. CustomAuthorizer in Security for path /path is not a valid dictionary."
}
3 changes: 3 additions & 0 deletions tests/translator/test_translator.py
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,9 @@ def _generate_new_deployment_hash(self, logical_id, dict_to_hash, rest_api_to_sw
"error_httpapi_mtls_configuration_invalid_type",
"error_resource_policy_not_dict",
"error_implicit_http_api_auth_any_method",
"error_invalid_method_definition",
"error_mappings_is_null",
"error_swagger_security_not_dict",
],
)
@patch("boto3.session.Session.region_name", "ap-southeast-1")
Expand Down