Apigw ng refactor headers to werkzeug headers #11227

cloutierMat · 2024-07-18T02:08:45Z

Motivation

As discussed with @bentsku, to simplify the usage of headers through the invocation, we will only keep the Werzeug headers as part of the context. We were mostly using the raw_headers through the handlers anyway, so it makes sense to only keep them in the context.

This is also in preparation to a refactor that will move some of the duplicate code in the integration handlers to the integration request handler.

Changes

InvocationContext gets rid of raw_headers and multi_value_headers. The value of header is what used to be known as raw_headers
IntegrationRequest type for headers is now a Werkzeug Headers
Various fixes through the handlers to fully support the new types

TODO

Currently targeting apigw next gen add circle ci pipeline #11226 to allow for ci-pipeline with Next Gen enabled. Rebase and target master once merge

cloutierMat · 2024-07-18T03:22:26Z

tests/unit/services/apigateway/test_handler_request.py

            {
                "host": host_header,
                "test-header": "value1",
                "test-header-multi": ["value2", "value3"],
-                "content-length": len(body),


It is now removed and no longer accessible in the invocation requests. This is on parity with aws.

bentsku

LGTM! Nice refactor! Just minor comments, nothing blocking, minor optimizations I'd say 😄

This is great, this is gonna help simplify the code along the way, the multiple headers made it quite complicated... this is great! Great job, I hope this will help for the remapping then!

bentsku · 2024-07-18T10:14:49Z

localstack-core/localstack/services/apigateway/next_gen/execute_api/handlers/method_request.py

@@ -120,7 +120,7 @@ def _get_missing_required_parameters(method: Method, request: InvocationRequest)
            param_type, param_value = request_parameter.removeprefix("method.request.").split(".")
            match param_type:
                case "header":
-                    is_missing = param_value not in request.get("headers", [])
+                    is_missing = param_value not in dict(request.get("headers"))


nit: should we call this once outside the loop, something like case_sensitive_headers in order call this multiple times?

Big oversitght! Nice catch!

localstack-core/localstack/services/apigateway/next_gen/execute_api/header_utils.py

bentsku · 2024-07-18T10:17:52Z

localstack-core/localstack/services/apigateway/next_gen/execute_api/integrations/aws.py

+        multi_value_headers = defaultdict(list)
+        for key, value in headers:
+            multi_value_headers[key].append(value)
+


Maybe we could simplify it like you've done in apigateway/next_gen/execute_api/integrations/http.py:

Suggested change

multi_value_headers = defaultdict(list)

for key, value in headers:

multi_value_headers[key].append(value)

return {k: headers.getlist(k) for k in headers.keys()}

Actually, we might want to update both to this implementation.

headers = Headers({"foo":"bar","FOO":"BAR"}) dict_comp_headers ={k: headers.getlist(k) for k in headers.keys()} # {'foo': ['bar', 'BAR'], 'FOO': ['bar', 'BAR']} multi_value_headers = defaultdict(list) for key, value in headers: multi_value_headers[key].append(value) # defaultdict(<class 'list'>, {'foo': ['bar'], 'FOO': ['BAR']})

Right, that does look better 🤔

I am creating a util that we can reuse for the parameter mapping as well, that will simplify work with casing and multi value everywhere, I believe.

bentsku · 2024-07-18T14:45:59Z

...ck-core/localstack/services/apigateway/next_gen/execute_api/handlers/integration_response.py

@@ -203,7 +203,7 @@ def get_response_template(
            return ""

        # The invocation request header is used to find the right response templated
-        accepts = request["raw_headers"].getlist("accept")
+        accepts = request["headers"].getlist("accept")


it seems for the render_request_template_mapping (btw, it seems the name was copy pasted from the integration request) we didn't call dict(context.invocation_request.get("headers", {}) for the MappingTemplateParams on line 233

It seems like we might not need to, since the VTL template is calling .get(), it appears we are returning the proper values.

We can probably update the render_request_template_mapping as well, and safe a bit on efficiency by not converting the headers to dict for the templates.

github-actions · 2024-07-18T15:51:47Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 24m 31s ⏱️ - 1h 9m 31s
682 tests - 2 527 636 ✅ - 2 186 46 💤 - 341 0 ❌ ±0
684 runs - 2 527 636 ✅ - 2 186 48 💤 - 341 0 ❌ ±0

Results for commit 76aa3ea. ± Comparison against base commit 91c4dff.

This pull request removes 2527 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 · 2024-07-18T16:38:11Z

localstack-core/localstack/services/apigateway/next_gen/execute_api/header_utils.py

+def build_multi_value_headers(headers: Headers) -> dict[str, list[str]]:
+    multi_value_headers = defaultdict(list)
+    for key, value in headers:
+        multi_value_headers[key].append(value)
+
+    return multi_value_headers


@bentsku This util was added as a way to always get the proper multi value headers with none of the duplication created by Headers.getlist()

bentsku

LGTM! Thanks for addressing the comments and looking more into the multi values headers, that's a great find! 🚀

bentsku · 2024-07-18T16:55:43Z

...ack-core/localstack/services/apigateway/next_gen/execute_api/handlers/integration_request.py

@@ -128,7 +130,7 @@ def render_request_template_mapping(
                    params=MappingTemplateParams(
                        path=request.get("path_parameters"),
                        querystring=request.get("query_string_parameters", {}),
-                        header=request.get("headers", {}),
+                        header=request.get("headers"),


as we remove the default value, and we know the headers are always set, should we use request["headers"]?

Right, I will fix in the follow up PR for the remapped headers.

bentsku · 2024-07-18T16:56:51Z

localstack-core/localstack/services/apigateway/next_gen/execute_api/handlers/method_request.py

@@ -113,14 +113,16 @@ def _get_missing_required_parameters(method: Method, request: InvocationRequest)
        if not (request_parameters := method.get("requestParameters")):
            return missing_params

+        case_sensitive_headers = list(request.get("headers").keys())


nit: we could even use set() here as we use as a lookup, but really not important 😄

I know they say set() is meant to be faster, but every time I tried to timeit, it turned out to be slightly slower. Maybe because I am working with smaller sets? It might have a bigger impact on greater sets? 🤔

Just gave it a try with a list of parameters of 7, and 8 headers, and it's about 15% faster. I guess it can depend heavily 😄

bentsku · 2024-07-18T16:59:11Z

localstack-core/localstack/services/apigateway/next_gen/execute_api/handlers/parse.py

+        invocation_headers = Headers()
+        for key, value in headers:
+            if should_drop_header_from_invocation(key):
+                LOG.debug("Dropping header from invocation request: '%s'", key)


this might be quite verbose, we could also build a list and log it maybe, we'll see once we try it more!

cloutierMat self-assigned this Jul 18, 2024

cloutierMat added the semver: patch Non-breaking changes which can be included in patch releases label Jul 18, 2024

cloutierMat changed the base branch from master to apigw-ng-add-circle-ci-pipeline July 18, 2024 02:11

cloutierMat commented Jul 18, 2024

View reviewed changes

cloutierMat mentioned this pull request Jul 18, 2024

make next-gen APIGW provider default #11035

Merged

2 tasks

cloutierMat marked this pull request as ready for review July 18, 2024 03:38

cloutierMat requested a review from bentsku as a code owner July 18, 2024 03:38

cloutierMat added this to the 3.6 milestone Jul 18, 2024

bentsku approved these changes Jul 18, 2024

View reviewed changes

bentsku reviewed Jul 18, 2024

View reviewed changes

bentsku mentioned this pull request Jul 18, 2024

APIGW NG Refactor IntegrationRequest for Proxy Integration #11234

Merged

Base automatically changed from apigw-ng-add-circle-ci-pipeline to master July 18, 2024 15:30

cloutierMat requested review from alexrashed, dfangl and dominikschubert as code owners July 18, 2024 15:30

cloutierMat added 4 commits July 18, 2024 09:34

refactor invocation headers

251f642

update tests and reference

89b1de7

fix tests

7dfdb62

fix headers update in aws integration

732be5a

cloutierMat force-pushed the apigw-ng-refactor-headers-to-werkzeug-headers branch from c5c9ad2 to 732be5a Compare July 18, 2024 15:34

cloutierMat removed request for dominikschubert, alexrashed and dfangl July 18, 2024 16:00

pr comment + added build_multi_value_headers util

6f1f883

cloutierMat commented Jul 18, 2024

View reviewed changes

fix list comprehension

76aa3ea

cloutierMat requested a review from bentsku July 18, 2024 16:52

bentsku approved these changes Jul 18, 2024

View reviewed changes

cloutierMat merged commit debe841 into master Jul 18, 2024
33 checks passed

cloutierMat deleted the apigw-ng-refactor-headers-to-werkzeug-headers branch July 18, 2024 17:12

cloutierMat mentioned this pull request Jul 19, 2024

Apigw ng implement header remapping #11237

Merged

Uh oh!

Apigw ng refactor headers to werkzeug headers #11227

Apigw ng refactor headers to werkzeug headers #11227

Uh oh!

Conversation

cloutierMat commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cloutierMat commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading