Refactor API GW tests, assert correct JSON content-type for DDB integration #7843
Conversation
67f9dce to
504c7da
Compare
| @@ -449,12 +449,14 @@ def invoke(self, invocation_context: ApiInvocationContext): | |||
|
|
|||
| # apply response templates | |||
| response_content = json.dumps(remove_attributes(response, ["ResponseMetadata"])) | |||
| response_obj = requests_response(content=response_content) | |||
| invocation_context.response = response_obj = requests_response(content=response_content) | |||
There was a problem hiding this comment.
Note: In one of the next refactoring iterations, we should unify the response handling for API GW integrations.
Most of the BackendIntegration implementations are returning a Response from invoke(..) (even though it is not defined in the interface), and some of the implementations also update the invocation_context.response property on the context.
We should think about how we can unify this (either having the response only on the context, or only returning it from invoke), to have a clear interface and make the request processing less ambiguous. 👍
There was a problem hiding this comment.
Agreed, if the integrations are self-contained as they're leaning to im not sure we will have the invocation_context.response anymore because all the work is done, meaning the response from the integration and the response templating is done. Food for thought, I guess
9d4b209 to
c0fe894
Compare
| deploy_cfn_template( | ||
| template=TEST_TEMPLATE_1, | ||
| parameters={"ApiName": api_name, "IntegrationUri": int_uri}, | ||
| parameters={"ApiName": api_name, "IntegrationUri": f"{echo_http_server}/post"}, |
There was a problem hiding this comment.
I like that much better that we're using the http_server fixture for our tests, it will make the tests much less flaky. However, we won't be able to test them anymore against AWS this way, or it would need a bit more setup. Should we leave some comment on how to AWS validate the test, maybe replacing manually the int_uri to the httpbin one?
There was a problem hiding this comment.
Ah, that's a great catch! Not sure how I could have missed that. 😕 You're totally right, we should add a switch that allows both local and remote testing. Let me see - one option would be to make the echo server return the same response as httpbin, or the other option would be to encapsulate the switch in the fixture itself. Will do another iteration to fix that 👍
There was a problem hiding this comment.
Done - added a echo_http_server_post fixture that can handle POST requests for both, local testing and AWS parity tests.
Comment on lines
-457
to
+458
invocation_context.response = response
# TODO: set response header based on response templates
headers = {HEADER_CONTENT_TYPE: APPLICATION_JSON}
response = requests_response(response, headers=headers)
There was a problem hiding this comment.
So, if I understood well, for now, we're only returning JSON, so we're setting the headers. LGTM 👍
But just a question, we now set the response before being rendered in the template to the invocation_context, whereas before we would set it after, is that wanted?
There was a problem hiding this comment.
I don't think we need it after, once we move all the templating inside the integrations, that's why we put the response inside the invocation_context, but we should agree on this, whatever it is.
There was a problem hiding this comment.
Great points. In my view, this is exactly a symptom of the current duality of response handling. In future iterations, we'll need the response details at some point of processing the integration requests (e.g., response mapping templates may contain variables like "$context.responseLength") - agree with Cesar what we should agree on one approach and unify.
Slightly updated the PR and removed the context.response assignment to address this point for now 👍
| invocation_context.response = response | ||
| # TODO: set response header based on response templates | ||
| headers = {HEADER_CONTENT_TYPE: APPLICATION_JSON} | ||
| response = requests_response(response, headers=headers) |
There was a problem hiding this comment.
So, if I understood well, for now, we're only returning JSON, so we're setting the headers. LGTM 👍
But just a question, we now set the response before being rendered in the template to the invocation_context, whereas before we would set it after, is that wanted?
There was a problem hiding this comment.
I don't think we need it after, once we move all the templating inside the integrations, that's why we put the response inside the invocation_context, but we should agree on this, whatever it is.
There was a problem hiding this comment.
Great points. In my view, this is exactly a symptom of the current duality of response handling. In future iterations, we'll need the response details at some point of processing the integration requests (e.g., response mapping templates may contain variables like "$context.responseLength") - agree with Cesar what we should agree on one approach and unify.
Slightly updated the PR and removed the context.response assignment to address this point for now 👍
|
Might still need to update the pro tests here: localstack/.github/workflows/pro-integration.yml Lines 207 to 222 in 31c2825 |
There was a problem hiding this comment.
LGTM, the separation is helpful and also good thinking around the responses - another PoV on it is apply the response templates on the integration response but leave the HTTP layer outside of the integrations, but would require some sort of data structure that contains the response dictionary, the content type, etc, so we can build an HTTP response after.
| @@ -449,12 +449,14 @@ def invoke(self, invocation_context: ApiInvocationContext): | |||
|
|
|||
| # apply response templates | |||
| response_content = json.dumps(remove_attributes(response, ["ResponseMetadata"])) | |||
| response_obj = requests_response(content=response_content) | |||
| invocation_context.response = response_obj = requests_response(content=response_content) | |||
There was a problem hiding this comment.
Agreed, if the integrations are self-contained as they're leaning to im not sure we will have the invocation_context.response anymore because all the work is done, meaning the response from the integration and the response templating is done. Food for thought, I guess
| invocation_context.response = response | ||
| # TODO: set response header based on response templates | ||
| headers = {HEADER_CONTENT_TYPE: APPLICATION_JSON} | ||
| response = requests_response(response, headers=headers) |
There was a problem hiding this comment.
I don't think we need it after, once we move all the templating inside the integrations, that's why we put the response inside the invocation_context, but we should agree on this, whatever it is.
|
Changes Unknown when pulling ca9f093 on apigw-dynamodb-responses into ** on master**. |
Refactor API Gateway tests, assert correct JSON content-type for DynamoDB integration.
Summary of changes:
tests/integration/apigatewaymoduleconftest.pyinto that same fileresponsein the invocation context (to avoidAttributeError)application/json)test_rest_api_to_dynamodb_integrationsnapshot test to also capture the response headers (to verify that the content-type matches)echo_http_serverfixture instead of makinghttpbin.orgrequests (should fix some flaky tests with 504 timeouts) - some TODOs still remaining in the codebaseSide note: As also mentioned in the PR comment below, in a future iteration we should unify the logic/interface of integration response handling. 👍 /cc @calvernaz @bentsku