From 3f2ad862cac9859d28ce7fc4ef730f7bae5223cc Mon Sep 17 00:00:00 2001 From: Marty Sweet Date: Thu, 6 Dec 2018 12:03:05 +0000 Subject: [PATCH 1/5] Merge lambda response headers and multiValueHeaders --- samcli/local/apigw/local_apigw_service.py | 32 ++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/samcli/local/apigw/local_apigw_service.py b/samcli/local/apigw/local_apigw_service.py index 6096085231..bf07dc8c54 100644 --- a/samcli/local/apigw/local_apigw_service.py +++ b/samcli/local/apigw/local_apigw_service.py @@ -152,7 +152,7 @@ def _request_handler(self, **kwargs): route.binary_types, request) except (KeyError, TypeError, ValueError): - LOG.error("Function returned an invalid response (must include one of: body, headers or " + LOG.error("Function returned an invalid response (must include one of: body, headers, multiValueHeaders or " "statusCode in the response object). Response received: %s", lambda_response) return ServiceErrorResponses.lambda_failure_response() @@ -193,8 +193,31 @@ def _parse_lambda_output(lambda_output, binary_types, flask_request): if not isinstance(json_output, dict): raise TypeError("Lambda returned %{s} instead of dict", type(json_output)) + # TODO: Implement full tests + # Merge multiValueHeaders headers with headers + # Convert into CSV for Flask compatibility + + # TODO: Move to separate testable function + # Merge multiValueHeaders into h, this will overwrite existing items as per documentation + h = {**(json_output.get("headers") or {}), **(json_output.get("multiValueHeaders") or {})} + + # Now we have single headers and arrays, convert arrays to CSV + for i, s in enumerate(h): + if isinstance(h[s], list): + h[s] = ", ".join(h[s]) + + # TODO: Cleanup and put in PR/comments for merge reasoning + # "headers": {"headerName": "headerValue", ...}, + # "multiValueHeaders": {"headerName": ["headerValue", "headerValue2", ...], ...}, + #The headers and multiValueHeaders keys can be unspecified if no extra response headers are to be returned. + #The headers key can only contain single-value headers. + #The multiValueHeaders key can contain multi-value headers as well as single-value headers. + # You can use the multiValueHeaders key to specify all of your extra headers, including any single-value ones. + # ASSUMING CORRECTNESS If you specify values for both headers and multiValueHeaders, API Gateway merges them into a single list. + # If the same key-value pair is specified in both, only the values from multiValueHeaders will appear in the merged list. + status_code = json_output.get("statusCode") or 200 - headers = CaseInsensitiveDict(json_output.get("headers") or {}) + headers = CaseInsensitiveDict(h) body = json_output.get("body") or "no data" is_base_64_encoded = json_output.get("isBase64Encoded") or False @@ -234,7 +257,10 @@ def _should_base64_decode_body(binary_types, flask_request, lamba_response_heade True if the body from the request should be converted to binary, otherwise false """ - best_match_mimetype = flask_request.accept_mimetypes.best_match([lamba_response_headers["Content-Type"]]) + + # Get the first part of the content-type header, to allow for extras such as text/html; charset=utf-8 + content_type = lamba_response_headers['Content-Type'].split(";", 1)[0] + best_match_mimetype = flask_request.accept_mimetypes.best_match([content_type]) is_best_match_in_binary_types = best_match_mimetype in binary_types or '*/*' in binary_types return best_match_mimetype and is_best_match_in_binary_types and is_base_64_encoded From 2eb4bb557e2a49f6dcf8f885f0eaa0db0f238e52 Mon Sep 17 00:00:00 2001 From: Marty Sweet Date: Fri, 7 Dec 2018 16:39:01 +0000 Subject: [PATCH 2/5] Add unit tests --- samcli/local/apigw/local_apigw_service.py | 59 ++++++++++-------- .../local/apigw/test_local_apigw_service.py | 60 ++++++++++++++++--- 2 files changed, 86 insertions(+), 33 deletions(-) diff --git a/samcli/local/apigw/local_apigw_service.py b/samcli/local/apigw/local_apigw_service.py index bf07dc8c54..8f7dad7fbb 100644 --- a/samcli/local/apigw/local_apigw_service.py +++ b/samcli/local/apigw/local_apigw_service.py @@ -193,31 +193,9 @@ def _parse_lambda_output(lambda_output, binary_types, flask_request): if not isinstance(json_output, dict): raise TypeError("Lambda returned %{s} instead of dict", type(json_output)) - # TODO: Implement full tests - # Merge multiValueHeaders headers with headers - # Convert into CSV for Flask compatibility - - # TODO: Move to separate testable function - # Merge multiValueHeaders into h, this will overwrite existing items as per documentation - h = {**(json_output.get("headers") or {}), **(json_output.get("multiValueHeaders") or {})} - - # Now we have single headers and arrays, convert arrays to CSV - for i, s in enumerate(h): - if isinstance(h[s], list): - h[s] = ", ".join(h[s]) - - # TODO: Cleanup and put in PR/comments for merge reasoning - # "headers": {"headerName": "headerValue", ...}, - # "multiValueHeaders": {"headerName": ["headerValue", "headerValue2", ...], ...}, - #The headers and multiValueHeaders keys can be unspecified if no extra response headers are to be returned. - #The headers key can only contain single-value headers. - #The multiValueHeaders key can contain multi-value headers as well as single-value headers. - # You can use the multiValueHeaders key to specify all of your extra headers, including any single-value ones. - # ASSUMING CORRECTNESS If you specify values for both headers and multiValueHeaders, API Gateway merges them into a single list. - # If the same key-value pair is specified in both, only the values from multiValueHeaders will appear in the merged list. - + headers = LocalApigwService._merge_response_headers(json_output.get("headers"), + json_output.get("multiValueHeaders")) status_code = json_output.get("statusCode") or 200 - headers = CaseInsensitiveDict(h) body = json_output.get("body") or "no data" is_base_64_encoded = json_output.get("isBase64Encoded") or False @@ -265,6 +243,39 @@ def _should_base64_decode_body(binary_types, flask_request, lamba_response_heade return best_match_mimetype and is_best_match_in_binary_types and is_base_64_encoded + @staticmethod + def _merge_response_headers(headers, multi_headers): + """ + # Merge multiValueHeaders headers with headers + # Convert into CSV for Flask compatibility + # If you specify values for both headers and multiValueHeaders, API Gateway merges them into a single list. + # If the same key-value pair is specified in both, only the values from multiValueHeaders will + # appear in the merged list. + + Parameters + ---------- + headers (dict) + Headers map from the lambda_response_headers + multi_headers (dict) + multiValueHeaders map from the lambda_response_headers + + Returns + ------- + Merged list in accordance to the AWS documentation within a CaseInsensitiveDict + + """ + + # Merge dictionary + h = (headers or {}).copy() + h.update(multi_headers or {}) + + # Now we have single headers map with Strings and Lists, convert Lists to CSV Strings for Flask + for _, s in enumerate(h): + if isinstance(h[s], list): + h[s] = ", ".join(h[s]) + + return CaseInsensitiveDict(h) + @staticmethod def _construct_event(flask_request, port, binary_types): """ diff --git a/tests/unit/local/apigw/test_local_apigw_service.py b/tests/unit/local/apigw/test_local_apigw_service.py index 234554deca..8145015f68 100644 --- a/tests/unit/local/apigw/test_local_apigw_service.py +++ b/tests/unit/local/apigw/test_local_apigw_service.py @@ -10,7 +10,6 @@ class TestApiGatewayService(TestCase): - def setUp(self): self.function_name = Mock() self.api_gateway_route = Route(['GET'], self.function_name, '/') @@ -51,7 +50,6 @@ def test_request_must_invoke_lambda(self): @patch('samcli.local.apigw.local_apigw_service.LambdaOutputParser') def test_request_handler_returns_process_stdout_when_making_response(self, lambda_output_parser_mock): - make_response_mock = Mock() self.service.service_response = make_response_mock @@ -150,7 +148,6 @@ def test_initalize_with_values(self): @patch('samcli.local.apigw.local_apigw_service.ServiceErrorResponses') def test_request_handles_error_when_invoke_cant_find_function(self, service_error_responses_patch): - not_found_response_mock = Mock() self.service._construct_event = Mock() self.service._get_current_route = Mock() @@ -212,7 +209,6 @@ def test_request_handler_errors_when_unable_to_read_binary_data(self, service_er @patch('samcli.local.apigw.local_apigw_service.request') def test_get_current_route(self, request_patch): - request_mock = Mock() request_mock.endpoint = "path" request_mock.method = "method" @@ -249,7 +245,6 @@ def test_get_current_route_keyerror(self, request_patch): class TestApiGatewayModel(TestCase): - def setUp(self): self.function_name = "name" self.api_gateway = Route(['POST'], self.function_name, '/') @@ -260,8 +255,48 @@ def test_class_initialization(self): self.assertEquals(self.api_gateway.path, '/') -class TestServiceParsingLambdaOutput(TestCase): +class TestLambdaHeaderDictionaryMerge(TestCase): + def test_none_dictionaries_produce_result(self): + headers = None + multi_value_headers = None + + result = LocalApigwService._merge_response_headers(headers, multi_value_headers) + + self.assertEquals(result, {}) + + def test_headers_are_merged(self): + headers = {"h1": "value1", "h2": "value2"} + multi_value_headers = {"h3": ["value3"]} + + result = LocalApigwService._merge_response_headers(headers, multi_value_headers) + + self.assertIn("h1", result) + self.assertIn("h2", result) + self.assertIn("h3", result) + self.assertEquals(result["h1"], "value1") + self.assertEquals(result["h2"], "value2") + self.assertEquals(result["h3"], "value3") + + def test_multivalue_headers_are_turned_into_csv(self): + headers = None + multi_value_headers = {"h1": ["a", "b", "c"]} + + result = LocalApigwService._merge_response_headers(headers, multi_value_headers) + + self.assertIn("h1", result) + self.assertEquals(result["h1"], "a, b, c") + + def test_multivalue_headers_override_headers_dict(self): + headers = {"h1": "ValueA"} + multi_value_headers = {"h1": ["ValueB"]} + result = LocalApigwService._merge_response_headers(headers, multi_value_headers) + + self.assertIn("h1", result) + self.assertEquals(result["h1"], "ValueB") + + +class TestServiceParsingLambdaOutput(TestCase): def test_default_content_type_header_added_with_no_headers(self): lambda_output = '{"statusCode": 200, "body": "{\\"message\\":\\"Hello from Lambda\\"}", ' \ '"isBase64Encoded": false}' @@ -289,6 +324,15 @@ def test_custom_content_type_header_is_not_modified(self): self.assertIn("Content-Type", headers) self.assertEquals(headers["Content-Type"], "text/xml") + def test_custom_content_type_multivalue_header_is_not_modified(self): + lambda_output = '{"statusCode": 200, "multiValueHeaders":{"Content-Type": ["text/xml"]}, "body": "{}", ' \ + '"isBase64Encoded": false}' + + (_, headers, _) = LocalApigwService._parse_lambda_output(lambda_output, binary_types=[], flask_request=Mock()) + + self.assertIn("Content-Type", headers) + self.assertEquals(headers["Content-Type"], "text/xml") + def test_extra_values_ignored(self): lambda_output = '{"statusCode": 200, "headers": {}, "body": "{\\"message\\":\\"Hello from Lambda\\"}", ' \ '"isBase64Encoded": false, "another_key": "some value"}' @@ -343,7 +387,7 @@ def test_status_code_not_int(self): def test_status_code_negative_int(self): lambda_output = '{"statusCode": -1, "headers": {}, "body": "{\\"message\\":\\"Hello from Lambda\\"}", ' \ - '"isBase64Encoded": false}' + '"isBase64Encoded": false}' with self.assertRaises(TypeError): LocalApigwService._parse_lambda_output(lambda_output, @@ -378,7 +422,6 @@ def test_properties_are_null(self): class TestService_construct_event(TestCase): - def setUp(self): self.request_mock = Mock() self.request_mock.endpoint = "endpoint" @@ -462,7 +505,6 @@ def test_query_string_params_with_param_value_being_non_empty_list(self): class TestService_should_base64_encode(TestCase): - @parameterized.expand([ param("Mimeyype is in binary types", ['image/gif'], 'image/gif'), param("Mimetype defined and binary types has */*", ['*/*'], 'image/gif'), From 3beffddcbe31dc9c7aa313548f79e0eaec02d7bb Mon Sep 17 00:00:00 2001 From: Marty Sweet Date: Fri, 7 Dec 2018 16:57:55 +0000 Subject: [PATCH 3/5] Fix unintentional whitespace changes --- tests/unit/local/apigw/test_local_apigw_service.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/unit/local/apigw/test_local_apigw_service.py b/tests/unit/local/apigw/test_local_apigw_service.py index 8145015f68..ee55cdd63a 100644 --- a/tests/unit/local/apigw/test_local_apigw_service.py +++ b/tests/unit/local/apigw/test_local_apigw_service.py @@ -10,6 +10,7 @@ class TestApiGatewayService(TestCase): + def setUp(self): self.function_name = Mock() self.api_gateway_route = Route(['GET'], self.function_name, '/') @@ -50,6 +51,7 @@ def test_request_must_invoke_lambda(self): @patch('samcli.local.apigw.local_apigw_service.LambdaOutputParser') def test_request_handler_returns_process_stdout_when_making_response(self, lambda_output_parser_mock): + make_response_mock = Mock() self.service.service_response = make_response_mock @@ -148,6 +150,7 @@ def test_initalize_with_values(self): @patch('samcli.local.apigw.local_apigw_service.ServiceErrorResponses') def test_request_handles_error_when_invoke_cant_find_function(self, service_error_responses_patch): + not_found_response_mock = Mock() self.service._construct_event = Mock() self.service._get_current_route = Mock() @@ -209,6 +212,7 @@ def test_request_handler_errors_when_unable_to_read_binary_data(self, service_er @patch('samcli.local.apigw.local_apigw_service.request') def test_get_current_route(self, request_patch): + request_mock = Mock() request_mock.endpoint = "path" request_mock.method = "method" @@ -245,6 +249,7 @@ def test_get_current_route_keyerror(self, request_patch): class TestApiGatewayModel(TestCase): + def setUp(self): self.function_name = "name" self.api_gateway = Route(['POST'], self.function_name, '/') @@ -297,6 +302,7 @@ def test_multivalue_headers_override_headers_dict(self): class TestServiceParsingLambdaOutput(TestCase): + def test_default_content_type_header_added_with_no_headers(self): lambda_output = '{"statusCode": 200, "body": "{\\"message\\":\\"Hello from Lambda\\"}", ' \ '"isBase64Encoded": false}' @@ -422,6 +428,7 @@ def test_properties_are_null(self): class TestService_construct_event(TestCase): + def setUp(self): self.request_mock = Mock() self.request_mock.endpoint = "endpoint" @@ -505,6 +512,7 @@ def test_query_string_params_with_param_value_being_non_empty_list(self): class TestService_should_base64_encode(TestCase): + @parameterized.expand([ param("Mimeyype is in binary types", ['image/gif'], 'image/gif'), param("Mimetype defined and binary types has */*", ['*/*'], 'image/gif'), From 676c308eb1903316639cc2148a3e15e54bd0fb34 Mon Sep 17 00:00:00 2001 From: Marty Sweet Date: Fri, 7 Dec 2018 16:59:08 +0000 Subject: [PATCH 4/5] Fix unintentional whitespace tab change --- tests/unit/local/apigw/test_local_apigw_service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/local/apigw/test_local_apigw_service.py b/tests/unit/local/apigw/test_local_apigw_service.py index ee55cdd63a..4bb5a2665f 100644 --- a/tests/unit/local/apigw/test_local_apigw_service.py +++ b/tests/unit/local/apigw/test_local_apigw_service.py @@ -393,7 +393,7 @@ def test_status_code_not_int(self): def test_status_code_negative_int(self): lambda_output = '{"statusCode": -1, "headers": {}, "body": "{\\"message\\":\\"Hello from Lambda\\"}", ' \ - '"isBase64Encoded": false}' + '"isBase64Encoded": false}' with self.assertRaises(TypeError): LocalApigwService._parse_lambda_output(lambda_output, From 35fed69aff2fa9f0296b915d01a288e5a88d2ab0 Mon Sep 17 00:00:00 2001 From: Marty Sweet Date: Fri, 25 Jan 2019 16:38:28 +0000 Subject: [PATCH 5/5] provide default headers, simplify merge, adjust tests --- samcli/local/apigw/local_apigw_service.py | 18 ++++++++---------- .../local/apigw/test_local_apigw_service.py | 8 ++++---- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/samcli/local/apigw/local_apigw_service.py b/samcli/local/apigw/local_apigw_service.py index 8f7dad7fbb..581e6e4507 100644 --- a/samcli/local/apigw/local_apigw_service.py +++ b/samcli/local/apigw/local_apigw_service.py @@ -193,8 +193,8 @@ def _parse_lambda_output(lambda_output, binary_types, flask_request): if not isinstance(json_output, dict): raise TypeError("Lambda returned %{s} instead of dict", type(json_output)) - headers = LocalApigwService._merge_response_headers(json_output.get("headers"), - json_output.get("multiValueHeaders")) + headers = LocalApigwService._merge_response_headers(json_output.get("headers") or {}, + json_output.get("multiValueHeaders") or {}) status_code = json_output.get("statusCode") or 200 body = json_output.get("body") or "no data" is_base_64_encoded = json_output.get("isBase64Encoded") or False @@ -265,16 +265,14 @@ def _merge_response_headers(headers, multi_headers): """ - # Merge dictionary - h = (headers or {}).copy() - h.update(multi_headers or {}) + processed_headers = CaseInsensitiveDict(headers) - # Now we have single headers map with Strings and Lists, convert Lists to CSV Strings for Flask - for _, s in enumerate(h): - if isinstance(h[s], list): - h[s] = ", ".join(h[s]) + # Convert multi_header Lists to CSV Strings for Flask + # This gives multiValueHeaders precedence over the API Gateway headers property + for _, s in enumerate(multi_headers): + processed_headers[s] = ", ".join(multi_headers[s]) - return CaseInsensitiveDict(h) + return processed_headers @staticmethod def _construct_event(flask_request, port, binary_types): diff --git a/tests/unit/local/apigw/test_local_apigw_service.py b/tests/unit/local/apigw/test_local_apigw_service.py index 4bb5a2665f..54771be238 100644 --- a/tests/unit/local/apigw/test_local_apigw_service.py +++ b/tests/unit/local/apigw/test_local_apigw_service.py @@ -261,9 +261,9 @@ def test_class_initialization(self): class TestLambdaHeaderDictionaryMerge(TestCase): - def test_none_dictionaries_produce_result(self): - headers = None - multi_value_headers = None + def test_empty_dictionaries_produce_empty_result(self): + headers = {} + multi_value_headers = {} result = LocalApigwService._merge_response_headers(headers, multi_value_headers) @@ -283,7 +283,7 @@ def test_headers_are_merged(self): self.assertEquals(result["h3"], "value3") def test_multivalue_headers_are_turned_into_csv(self): - headers = None + headers = {} multi_value_headers = {"h1": ["a", "b", "c"]} result = LocalApigwService._merge_response_headers(headers, multi_value_headers)