Skip to content

Step Functions: Extended Support for Escape Sequences in String Literals #12222

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.

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

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

MEPalma
Copy link
Contributor

@MEPalma MEPalma commented Feb 3, 2025

Motivation

The SFN v2 interpreter currently does not consistently handle escape sequences in string literals. This can lead to issues when users declare strings with escape sequences, potentially causing failures in string comparison logic.

Previous changes in #12072 introduced a strategy to correctly interpret string literals containing escape sequences, but this fix was limited to JSONata expression literals. However, escape sequence handling issues persist in other areas, such as baseline string literals, intrinsic functions, and JSONPath strings.

During this investigation, all string literal types were analyzed and tested. However, after extensive research into AWS’s behavior, we found that only strict string literals could be reliably aligned with AWS’s handling of escape sequences. Additionally, issues related to parsing string literals as JSONata expressions in Output and Assign blocks were identified and resolved.

That said, achieving full parity with AWS’s handling of escape sequences in JSONPath and intrinsic functions proved impractical. Specifically, we could not limit LocalStack’s escape sequence support to match AWS’s restrictions without introducing significant limitations. As a result, LocalStack continues to allow escape sequences in more scenarios than AWS.

It's worth noting that with the recent shift towards JSONata expressions, state machines are expected to standardize around just two types of string literals: String Literals and JSONata Expressions. Given this transition, precisely mirroring AWS’s handling of escape sequences in other string types is not an immediate priority.

* Please refer to the details included in the backlog item associated to these changes for more insights.

Changes

  • adjust preprocessing logic of base string literals to correctly handle escape sequences; these changes include occurrances of similar logics in preprocessing payload template string values and bindings
  • adjust the parsing of JSONata string literals in Assign and Output template
  • add several positive and negative snapshot tests regarding the use of escape sequences in various string literal types

@MEPalma MEPalma added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Feb 3, 2025
@MEPalma MEPalma added this to the 4.2 milestone Feb 3, 2025
@MEPalma MEPalma self-assigned this Feb 3, 2025
Copy link

github-actions bot commented Feb 3, 2025

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   34m 32s ⏱️ - 1h 46m 2s
1 425 tests  - 2 650  1 352 ✅  - 2 408  73 💤  - 242  0 ❌ ±0 
1 427 runs   - 2 650  1 352 ✅  - 2 408  75 💤  - 242  0 ❌ ±0 

Results for commit acb958e. ± Comparison against base commit 84b2f5f.

This pull request removes 2656 and adds 6 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.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_escape_sequence_parsing[ESCAPE_SEQUENCES_JSONATA_COMPARISON_ASSIGN]
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_escape_sequence_parsing[ESCAPE_SEQUENCES_JSONATA_COMPARISON_OUTPUT]
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_escape_sequence_parsing[ESCAPE_SEQUENCES_JSONPATH]
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_escape_sequence_parsing[ESCAPE_SEQUENCES_STRING_LITERALS]
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_illegal_escapes[ESCAPE_SEQUENCES_ILLEGAL_INTRINSIC_FUNCTION]
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_illegal_escapes[ESCAPE_SEQUENCES_ILLEGAL_INTRINSIC_FUNCTION_2]
This pull request removes 244 skipped tests and adds 2 skipped tests. Note that renamed tests count towards both.
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input4-FAILED]
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_deployed_infra_state
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_populate_data
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_user_clicks_are_stored
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_api_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_invalid_desiredstate
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_double_create_with_client_token
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_lifecycle
…
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_illegal_escapes[ESCAPE_SEQUENCES_ILLEGAL_INTRINSIC_FUNCTION]
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_illegal_escapes[ESCAPE_SEQUENCES_ILLEGAL_INTRINSIC_FUNCTION_2]

Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

Thorough test coverage 👏👏 and sensible decision to defer intrinsic function literal exceptions to a point where we want to tackle better static validation support 💭 .

@markers.aws.validated
@pytest.mark.skip(
reason=(
"Lack of generalisable approach to escape sequences support "
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for this clarification 👍

@@ -1183,8 +1183,8 @@ def visitPayload_value_null(self, ctx: ASLParser.Payload_value_nullContext) -> P
return PayloadValueNull()

def visitPayload_value_str(self, ctx: ASLParser.Payload_value_strContext) -> PayloadValueStr:
str_val = self._inner_string_of(parser_rule_context=ctx.string_literal())
return PayloadValueStr(val=str_val)
string_literal: StringLiteral = self.visitString_literal(ctx=ctx.string_literal())
Copy link
Member

Choose a reason for hiding this comment

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

literal string abstraction sounds clearer than "innver_string" 👍

"IntrinsicEscape": {
"Type": "Pass",
"Parameters": {
"parsed.$": "States.StringToJson('{\"text\":\"An \\\"escaped double quote\\\" here\"}')"
Copy link
Member

Choose a reason for hiding this comment

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

I love the descriptive message clarifying the test intention 💯 ✨

@MEPalma MEPalma merged commit 91fa86d into master Feb 4, 2025
36 checks passed
@MEPalma MEPalma deleted the MEP-SFN-escape_sequences branch February 4, 2025 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants