Fix apigw input path formatting #12379

cloutierMat · 2025-03-12T18:56:41Z

Motivation

When using VTL template for apigateway, AWS generally returns a java object string representation {foo=bar}. But when accessing a list through $input.path('$'), it returns a valid json string, even if there are dict inside. A nested list inside a dict will cause the encapsulated object to be json encoded as well {list=[{"foo":"bar"}]}.

This pr implements java representation for result of $input.path and proper list formatting. Follow ups will be required to implement the Java formatting over more results in the VTL template.

Changes

Introduces JavaDictFormatter to return Java like object
Implements InputPathDictFormatter and InputPathListFormatter to manage string formatting of children of $input.path
Add tests

github-actions · 2025-03-12T20:19:23Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 1h 51m 33s ⏱️ -26s
4 289 tests +164 3 969 ✅ +164 320 💤 ±0 0 ❌ ±0
4 291 runs +164 3 969 ✅ +164 322 💤 ±0 0 ❌ ±0

Results for commit 00e191b. ± Comparison against base commit 257af99.

This pull request removes 1 and adds 165 tests. Note that renamed tests count towards both.

tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_map_state_item_selector

tests.aws.services.apigateway.test_apigateway_api.TestApiGatewayApiRestApi ‑ test_create_rest_api_with_binary_media_types
tests.aws.services.apigateway.test_apigateway_common.TestApiGatewayCommon ‑ test_input_path_template_formatting
tests.aws.services.iam.test_iam.TestIAMServiceRoles ‑ test_service_role_already_exists
tests.aws.services.iam.test_iam.TestIAMServiceRoles ‑ test_service_role_deletion
tests.aws.services.iam.test_iam.TestIAMServiceRoles ‑ test_service_role_lifecycle[accountdiscovery.ssm.amazonaws.com]
tests.aws.services.iam.test_iam.TestIAMServiceRoles ‑ test_service_role_lifecycle[acm.amazonaws.com]
tests.aws.services.iam.test_iam.TestIAMServiceRoles ‑ test_service_role_lifecycle[appmesh.amazonaws.com]
tests.aws.services.iam.test_iam.TestIAMServiceRoles ‑ test_service_role_lifecycle[autoscaling-plans.amazonaws.com]
tests.aws.services.iam.test_iam.TestIAMServiceRoles ‑ test_service_role_lifecycle[autoscaling.amazonaws.com]
tests.aws.services.iam.test_iam.TestIAMServiceRoles ‑ test_service_role_lifecycle[backup.amazonaws.com]
…

♻️ This comment has been updated with latest results.

bentsku

Wow, impressive PR! Thanks a lot for this deep dive into VTL!

I have a few comments, mostly related to naming, it made things a bit hard to follow with the Formatter naming that still contains the actual object inside. I see where it comes from, as it's mostly a wrapper to provide a clean str implementation, but it makes thing a bit more complex to understand. But I'm not hard set on changing it either 😄

I would like to know the implication for the $input.json util function as well, as it relies on the $input.path implementation, just to be sure we haven't changed something there too, if that's possible? I would think both are very similar and behaves the same.

Also would want to make sure the change to fixture doesn't break any weird tests upstream 😄

In any case, all the logic is there, and this looks really great, you really went the extra mile here! 🚀 thank you 🙏

bentsku · 2025-03-13T15:43:57Z

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

+    return value
+
+
+def get_input_path_formatter(value: any) -> any:


Suggested change

def get_input_path_formatter(value: any) -> any:

def get_input_path_formatter(value: any) -> Any:

bentsku · 2025-03-13T15:47:52Z

localstack-core/localstack/testing/pytest/fixtures.py

@@ -2175,6 +2175,7 @@ def _echo(request: Request) -> Response:
            "headers": dict(request.headers),
            "url": request.url,
            "method": request.method,
+            "json": request.json if request.is_json else None,


question: can't this change break other uses of the fixture maybe upstream? Not sure where it is used 😕 might be worth to trigger a run

Good idea, it didn't to seem impact anything. And since this is the behavior implemented in httpbin, there shouldn't be an issue with it unless there are some weird if not is_aws_cloud(). But I triggered a run upstream 13842940185.

bentsku · 2025-03-13T15:49:19Z

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

+
+
+class JavaDictFormatter(dict):
+    # TODO apply this class more generally through the template mappings


Nice! Can't wait to have more parity here, and also introduce the "hidden" dict sometimes used! We're getting real close now 👀

bentsku · 2025-03-13T15:50:39Z

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

+    if isinstance(value, dict):
+        return JavaDictFormatter(value)
+    if isinstance(value, list):
+        return [get_java_formatter(item) for item in value]


nice recursion 😄

bentsku · 2025-03-13T15:56:48Z

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

+    return value
+
+
+class JavaDictFormatter(dict):


I'm a bit on the fence about the *Formatter naming, as it still effectively contains/is the primitive value passed, just extended to support VTL, especially the __str__ internal method.
Same with the Java naming, maybe we could scope it down only to VTL instead? Something like VTLMap or VTLDictExtended?

Thanks for the suggestion, I struggled with the naming here. I think I like the VTLMap better.

bentsku · 2025-03-13T15:57:42Z

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

+        return dict_to_string(self)
+
+
+class InputPathListFormatter(list):


same comment for the naming, I think it could be something related to JSON, list and VTL? Naming is hard 😅
Because the documentation mentions returning a JSON object/string, so I think it'd make sense to make it part of the name instead of directly linking it as a result of path? it will depends on how the $input.json also behaves 🤔

the behavior of $input.json hasn't changed and is behaving as it should. But I realise that we are recursively casting every object in input just to ignore it all and json dumping it anyway. I will separate those a bit more to not uselessly cast everything.

Reguarding the naming, VTLJsonMap and VTLJsonList should improve things a bit?

bentsku · 2025-03-13T16:01:04Z

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

@@ -142,7 +197,7 @@ def path(self, path):
        if not self.value:
            return {}
        value = self.value if isinstance(self.value, dict) else json.loads(self.value)
-        return extract_jsonpath(value, path)
+        return get_input_path_formatter(extract_jsonpath(value, path))

    def json(self, path):


as we are calling self.path in the json method, I wonder how those 2 behave together, especially the dumping of the weird map value. I think this might change existing behavior because we would return a Python dictionary before, and now will return a weird VTL dictionary instead?

json.dumps does return the same as before as it does not call str() on dict or list

str(input.path("$"))='{a=1, b=[{"c":5}]}' str(input.json("$"))='{"a": 1, "b": [{"c": 5}]}'

bentsku · 2025-03-13T16:06:08Z

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

@@ -50,6 +53,58 @@ class MappingTemplateVariables(TypedDict, total=False):
    stageVariables: dict[str, str]


+def get_java_formatter(value):


coming back to the naming, it could be something like get_vtl_object or something related to "type" or maybe even call it cast_to_vtl_object as it's what we are doing, type casting? I can't really find any good word to replace "formatter". https://velocity.apache.org/engine/devel/developer-guide.html#how-velocity-works

edited: switched back to snake case because I don't know why I put that way, got confused with all the classes before 😅

bentsku

LGTM! Thanks for addressing the comments so quickly! And for adding the doc strings 😄 this is a great PR and addition, thanks a lot for really pushing this 💯🥇

cloutierMat added 2 commits March 12, 2025 12:54

Implement java formatting

93343f5

added tests

9cb1524

cloutierMat self-assigned this Mar 12, 2025

cloutierMat added aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases labels Mar 12, 2025

fix formatter factory

2cf4d0f

fix tests

93014c9

cloutierMat marked this pull request as ready for review March 12, 2025 21:49

cloutierMat requested review from bentsku and dominikschubert as code owners March 12, 2025 21:49

bentsku reviewed Mar 13, 2025

View reviewed changes

cloutierMat added 3 commits March 13, 2025 14:16

renamed class and pr comments

b8b9e50

slight refactor to improve $input.json

2d6b6fd

fixed upstream test

00e191b

bentsku approved these changes Mar 14, 2025

View reviewed changes

cloutierMat merged commit 339c073 into master Mar 14, 2025
31 checks passed

cloutierMat deleted the fix-apigw-input-path-formatting branch March 14, 2025 00:37

		return value


		def get_input_path_formatter(value: any) -> any:



		class JavaDictFormatter(dict):
		# TODO apply this class more generally through the template mappings

		return dict_to_string(self)


		class InputPathListFormatter(list):

		@@ -50,6 +53,58 @@ class MappingTemplateVariables(TypedDict, total=False):
		stageVariables: dict[str, str]


		def get_java_formatter(value):

Uh oh!

Fix apigw input path formatting #12379

Fix apigw input path formatting #12379

Uh oh!

Conversation

cloutierMat commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

github-actions bot commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

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!

cloutierMat Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

cloutierMat Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bentsku Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cloutierMat commented Mar 12, 2025 •

edited

Loading

github-actions bot commented Mar 12, 2025 •

edited

Loading

cloutierMat Mar 13, 2025 •

edited

Loading

cloutierMat Mar 13, 2025 •

edited

Loading

bentsku Mar 13, 2025 •

edited

Loading