Bugfix/eventbridge/transformer issue with nested key #11998

maxhoheiser · 2024-12-06T12:30:08Z

Motivation

InputTransformer map values from the event to keys defined in InputPathMap and these can then be used in InputTemplate to replace placeholders.
These placeholders can also be in a continuous string "string/" this was failing in localstack as raised here #11126

Changes

add support for string nested placeholders in json input templates
add support for string nested placeholders in string input templates

github-actions · 2024-12-06T13:13:25Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 1h 8m 41s ⏱️ - 42m 32s
2 364 tests - 1 540 2 269 ✅ - 1 328 95 💤 - 212 0 ❌ ±0
2 366 runs - 1 540 2 269 ✅ - 1 328 97 💤 - 212 0 ❌ ±0

Results for commit bf6335f. ± Comparison against base commit 5399278.

This pull request removes 1555 and adds 15 tests. Note that renamed tests count towards both.

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]
…

tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement["<listmulti> multiple list items"]
tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement["<listsingle> single list item <listmulti> multiple list items <systemstring> system account id <payload> payload <userId> user id"]
tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement["<listsingle> single list item"]
tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement["Payload of <payload> with path users-service/users/<userId> and <userId>"]
tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement[{"id" : "<userId>"}]
tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement[{"id" : <userId>}]
tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement[{"method": "PUT", "nested": {"level1": {"level2": {"level3": "users-service/users/<userId>"} } }, "bod": "<userId>"}]
tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement[{"method": "PUT", "path": "users-service/users/<userId>", "bod": <payload>}]
tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement[{"method": "PUT", "path": "users-service/users/<userId>", "bod": [<userId>, "hardcoded"]}]
tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement[{"method": "PUT", "path": "users-service/users/<userId>", "id": <userId>, "body": <payload>}]
…

♻️ This comment has been updated with latest results.

bentsku

Nice fix!

I believe we might still have a few corner cases to cover, we could discover them with some parametrized unit tests for the is_nested_in_string function, and maybe even the replace_template_placeholders one.

I think we're not properly replacing dict values inside JSON templates string fields, and I'm not 100% certain of the is_nested_in_string check if the matched value is between quotes with nothing else, and it'd be great to validate those.

This documentation part is pretty useful to try to understand edge cases: https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-transform-target-input.html#eb-transform-input-issues

I think we should have all the cases in this table: https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-transform-target-input.html#eb-transform-target-input-template as part of unit tests for replace_template_placeholders, as they are pretty well defined. Some of those could maybe be tackled in a follow up, but it'd be good to know how far we support.

Thanks!

bentsku · 2024-12-10T11:09:07Z

localstack-core/localstack/services/events/utils.py

@@ -248,3 +248,32 @@ def get_trace_header_encoded_region_account(
            return json.dumps({"original_id": original_id, "original_account": source_account_id})
        else:
            return json.dumps({"original_account": source_account_id})
+
+
+def is_nested_in_string(template, match) -> bool:


I would like to get unit tests for this function, as it is quite complex and could break in unexpected ways, and just to validate its limits. Do you think that'd be possible to add to the PR?

added unit test for the function test_is_nested_in_string

Awesome, thanks! I'll wait for the pipeline to be green and review the changes today. Thank you!

bentsku · 2024-12-10T11:09:52Z

localstack-core/localstack/services/events/target.py

+        if is_json_template:
+            if is_nested_in_string(template, match):
+                return value  # Return raw string for nested placeholders
+            else:
+                return to_json_str(value)
        if isinstance(value, datetime.datetime):
            return event_time_to_time_string(value)
+        if isinstance(value, dict):
+            return dict_to_simple_string(value)


Looking at the AWS documentation, it seems that if you set a dict value inside a string in a json_template, it will also strip the quotes out of it, like it would do in a regular string templates. It would be good to validate this in AWS.

I think the dict_to_simple_string should also be used in the is_nested_in_string condition if the value is a string, because now you'll return a dict out of the replace function.

If I understand you correctly this scenario should be covered by your proposed test cast '{"method": "PUT", "path": "users-service/users/<payload>", "bod": "<payload>"}, since payload is a dictionary and it is set in the middle part in the string "users-service/users/<payload>"

This will interestingly fail for AWS since it will not be correctly transformed since it would result in a string that is not a json string but the system expects a json string

I added a test for this failure also

bentsku · 2024-12-10T11:17:07Z

tests/aws/services/events/test_events_inputs.py

+        [
+            '{"method": "PUT", "path": "users-service/users/<userId>", "bod": <payload>}',
+            '"Payload of <payload> with path users-service/users/<userId>"',
+        ],


I would add a few cases here, just to test the limit, and some are given by the docs:

'{"method": "PUT", "path": "users-service/users/<payload>", "bod": "<payload>"}

'{"method": "PUT", "path": "users-service/users/<userId>", "bod": "<userId>"}',

'{"method": "PUT", "path": "users-service/users/<userId>", "bod": [<userId>, "hardcoded"]}',

There's also this big one with multiline text in the documentation that raises questions for me:
https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-transform-target-input.html#eb-transform-input-issues

{ "detail": { "severity": ["HIGH"], "status": ["ACTIVE"] }, "detail-type": ["Inspector2 Finding"], "source": ["inspector2"] }

Input Path

{ "account": "$.detail.awsAccountId", "ami": "$.detail.resources[0].details.awsEc2Instance.imageId", "arn": "$.detail.findingArn", "description": "$.detail.description", "instance": "$.detail.resources[0].id", "platform": "$.detail.resources[0].details.awsEc2Instance.platform", "region": "$.detail.resources[0].region", "severity": "$.detail.severity", "time": "$.time", "title": "$.detail.title", "type": "$.detail.type" }

Input template

"<severity> severity finding <title>" "Description: <description>" "ARN: \"<arn>\"" "Type: <type>" "AWS Account: <account>" "Region: <region>" "EC2 Instance: <instance>" "Platform: <platform>" "AMI: <ami>"

added something similar to as a test case also with nested lists and target replacement values

bentsku · 2024-12-10T11:19:07Z

localstack-core/localstack/services/events/utils.py

+    ):
+        return False
+
+    return left_quote != -1 and template[left_quote + 1 : right_quote].strip() != match.group(0)


not sure I fully grasp this condition, does it mean that if we have "<placeholder>" we want to return False and add quotes to the value again? This might be worth to test it

bentsku · 2024-12-10T12:09:23Z

localstack-core/localstack/services/events/utils.py

@@ -273,3 +273,7 @@ def is_nested_in_string(template, match) -> bool:
        return False

    return left_quote != -1 and template[left_quote + 1 : right_quote].strip() != match.group(0)
+
+
+def dict_to_simple_string(d):


Ah, I missed this one too: I think this does not cover all use case if there are nested values here.
It seems what AWS does is stripped all quotes from the JSON string, it might be easier that way.

Because what if you have a nested dict values here? I don't think that works, same if v is a list with strings inside.

I added a test case for nested dicts in string transformer '{"method": "PUT", "nested": {"level1": {"level2": {"level3": "users-service/users/<userId>"} } }, "bod": "<userId>"}',

bentsku

Wow, quite a lot of coverage now! Thank you for addressing the comments, I know it was a lot 😅 I think this is really great, thanks for fixing this issue! This was quite the rabbit hole...
I only have a small nit regarding the log statement dumping JSON.

I've done a run for -ext, and it was all green, so we should be on the safe side! LGTM! And congrats for this quite big addition! 😄

localstack-core/localstack/services/events/target.py

maxhoheiser added aws:events Amazon EventBridge semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Dec 6, 2024

maxhoheiser self-assigned this Dec 6, 2024

maxhoheiser requested a review from joe4dev as a code owner December 6, 2024 12:30

bentsku self-requested a review December 9, 2024 16:16

bentsku requested changes Dec 10, 2024

View reviewed changes

bentsku reviewed Dec 10, 2024

View reviewed changes

maxhoheiser force-pushed the bugfix/eventbridge/transformer-issue-with-nested-key branch from 7b32f39 to 1ca23e1 Compare December 13, 2024 12:27

maxhoheiser force-pushed the bugfix/eventbridge/transformer-issue-with-nested-key branch from 0b5aec2 to 29cfd7f Compare December 20, 2024 13:20

maxhoheiser added 16 commits December 27, 2024 11:25

feat: add validated test

8dfc4aa

feat: fix nested replacement template strings

0e193e7

feat: add test nested replacement keys in string template

8571690

feat: handle nested replacement keys with string template

ea2a5af

feat: add multiple positive and negative test cases

607448e

feat: extend test cases

f29f3b7

feat: add test cases

1c3aca7

feat: fix all test cases

4f466b2

feat: add test case

3df353a

refactor: docstring of is nested str

257d031

feat: add unit test for is nested in str

32fdc3d

fix: unit test for test_is_nested_in_string

324dcd7

feat: add test case single quoted json

6514054

feat: match is nested in quote on single quoted

89a2606

fix: input template single quoted

19a3d78

refactor: cleanup

f96f363

maxhoheiser force-pushed the bugfix/eventbridge/transformer-issue-with-nested-key branch from a9edd1a to f96f363 Compare December 27, 2024 10:25

fix: dont send non events

bf6335f

bentsku approved these changes Dec 30, 2024

View reviewed changes

localstack-core/localstack/services/events/target.py Show resolved Hide resolved

maxhoheiser merged commit 3e1471a into master Jan 2, 2025
31 checks passed

maxhoheiser deleted the bugfix/eventbridge/transformer-issue-with-nested-key branch January 2, 2025 12:51

maxhoheiser mentioned this pull request Jan 3, 2025

bug: eventbridge input transformer with string replacement in json #11126

Closed

1 task

alexrashed mentioned this pull request Jan 7, 2025

Update CODEOWNERS #12105

Merged

kkarimi mentioned this pull request Jan 22, 2025

bug: EventBridge messages fail because Unable to find EventBridge connection #12155

Closed

1 task

Uh oh!

Bugfix/eventbridge/transformer issue with nested key #11998

Bugfix/eventbridge/transformer issue with nested key #11998

Uh oh!

Conversation

maxhoheiser commented Dec 6, 2024

Motivation

Changes

Uh oh!

github-actions bot commented Dec 6, 2024 • 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 • 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!

bentsku Dec 16, 2024 • 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!

maxhoheiser Dec 13, 2024 • 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!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 6, 2024 •

edited

Loading

bentsku left a comment •

edited

Loading

bentsku Dec 16, 2024 •

edited

Loading

maxhoheiser Dec 13, 2024 •

edited

Loading