Skip to content

Apigw ng request template mapping #11138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Jul 4, 2024
Merged

Apigw ng request template mapping #11138

merged 10 commits into from
Jul 4, 2024

Conversation

cloutierMat
Copy link
Contributor

@cloutierMat cloutierMat commented Jul 3, 2024 •
edited
Loading

Motivation

Adding VTL Template Mapping to our Integration request handler and port ApiGatewayVtlTemplate class to NG provider.

The request template allows to create a request body based on the provided VTL template. You can also use it to #set requests override the path parameters, headers and query strings. See: https://docs.aws.amazon.com/apigateway/latest/developerguide/models-mappings.html

When no templates are found based on content-type, an UnsupportedMediaTypeError can be raised depending on the set passthrough behaviour.

Changes

  • Implements ApiGatewayVtlTemplate, which is ported and simplified from legacy templates.py.
  • adds logic for and exception for passthrough behavior
  • augments request template with the request overrides

TODO

What's left to do:

  • Implement unit tests for ApiGatewayVtlTemplate. Port from legacy to prevent regressions as we move forward
  • Implement unit tests for IntegrationRequestHandler. There might be more tests needed as parameters mapping was not tested yet

@cloutierMat cloutierMat added aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases labels Jul 3, 2024
@cloutierMat cloutierMat added this to the 3.6 milestone Jul 3, 2024
@cloutierMat cloutierMat self-assigned this Jul 3, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 3, 2024 •
edited
Loading

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   23m 34s ⏱️ - 1h 16m 11s
643 tests  - 2 513  598 ✅  - 2 159  45 💤  - 350  0 ❌ ±0 
645 runs   - 2 513  598 ✅  - 2 159  47 💤  - 350  0 ❌ ±0 

Results for commit e9675ef. ± Comparison against base commit 6ea7da3.

This pull request removes 2513 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

@cloutierMat cloutierMat marked this pull request as ready for review July 3, 2024 23:33
@cloutierMat cloutierMat requested a review from bentsku as a code owner July 3, 2024 23:33
@cloutierMat
Copy link
Contributor Author

@bentsku We might need more tests in the handler to make sure that both step work good together.

I didn't want to test all scenarios for now as I feel most of the edge cases should be tested either in the unit tests for ParametersMapper or ApiGatewayVtlTemplate or in AWS validated tests later on, and focus the handler testa on asserting a correct order of operations.

Happy to collaborate and align our testing efforts! 🤘

bentsku
bentsku reviewed Jul 3, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

Wow! Impressive set of change! Agreed about the handler, to me it makes lots of sense to do this way. 👍

Just have a comment about the decoding/re-encoding of the body, I believe we could avoid the little dance by always returning bytes from render_request_template_mapping.

Great work once again, nice set of tests, this is really taking shape now! I'll hurry to complete the IntegrationResponse now so that we can continue from there with the VTL templating too.

Also a small comment about the InvocationRequest in the tests, I wonder if we should just use raw Request object and parse them directly to simplify our work. 🤔

@cloutierMat cloutierMat mentioned this pull request Jul 4, 2024
Merged
2 tasks
cloutierMat
cloutierMat commented Jul 4, 2024
View reviewed changes
...ack-core/localstack/services/apigateway/next_gen/execute_api/handlers/integration_request.py Outdated
Comment on lines 82 to 88
# AWS doesn't send a body for these methods. Mock needs the body to get the status code
# TODO this might need to be done in the integrations?
if (
integration_method in [HTTPMethod.GET, HTTPMethod.HEAD, HTTPMethod.OPTIONS]
and not integration_type == IntegrationType.MOCK
):
body = b""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bentsku I added this section after validating it's behavior against aws.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I answered here too 😄 #11138 (comment)

@cloutierMat cloutierMat requested a review from bentsku July 4, 2024 02:21
bentsku
bentsku reviewed Jul 4, 2024
View reviewed changes
tests/unit/services/apigateway/test_handler_integration_request.py
multi_value_query_string_parameters={"qs": ["ignored-value", "test-qs-value"]},
multi_value_headers=header_dict,
body=b"",
request = InvocationRequestParser().create_invocation_request(
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, very neat 👌

bentsku
bentsku approved these changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments! 🚀 very nice changes, I like the parsing of the request in the test, makes it a bit simpler to not juggle with the headers kind, we might be able to simplify at some point... maybe the headers field of the context is only used in AWS_PROXY, so we might be able to remove it.

Really nice PR, really neat and well organized! And a lot of added tests! 🚀

@cloutierMat cloutierMat merged commit aab3720 into master Jul 4, 2024
32 checks passed
@cloutierMat cloutierMat deleted the apigw-ng-request-template-mapping branch July 4, 2024 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

Assignees

@cloutierMat cloutierMat

Labels
aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
3.6
Development

Successfully merging this pull request may close these issues.
2 participants
@cloutierMat @bentsku