Skip to content

fix StringSplit regex escape #11995

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 1 commit into from
Dec 6, 2024
Merged

fix StringSplit regex escape #11995

merged 1 commit into from
Dec 6, 2024

Conversation

cloutierMat
Copy link
Contributor

@cloutierMat cloutierMat commented Dec 5, 2024 •
edited
Loading

Motivation

The StringSplit function allows to split on any of the character provided by the second argument. The current code was manually escaping each character, appending \\ to it. While allowing for the most common special characters delimiter, the unfortunate side effect is that digits and letters would be incorrectly escaped.

Changes

  • update the StringSplit function to use re.escape instead for a safe escaping experience
  • perform the join directly on the list comprehension for a small performance upgrade

@cloutierMat cloutierMat added aws:stepfunctions AWS Step Functions semver: patch Non-breaking changes which can be included in patch releases labels Dec 5, 2024
@cloutierMat cloutierMat added this to the 4.1 milestone Dec 5, 2024
@cloutierMat cloutierMat self-assigned this Dec 5, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2024

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   35m 39s ⏱️ - 1h 14m 0s
1 308 tests  - 2 522  1 243 ✅  - 2 269  65 💤  - 253  0 ❌ ±0 
1 310 runs   - 2 522  1 243 ✅  - 2 269  67 💤  - 253  0 ❌ ±0 

Results for commit 1880d02. ± Comparison against base commit 6df4dbc.

This pull request removes 2522 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]
…

@cloutierMat cloutierMat marked this pull request as ready for review December 5, 2024 21:04
MEPalma
MEPalma approved these changes Dec 6, 2024
View reviewed changes
Copy link
Contributor

@MEPalma MEPalma left a comment

Choose a reason for hiding this comment

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

re.escape is definately a better approach to this, thanks!

@cloutierMat cloutierMat merged commit 523b896 into master Dec 6, 2024
40 checks passed
@cloutierMat cloutierMat deleted the sfn-fix-StringSplit-escape branch December 6, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MEPalma MEPalma MEPalma approved these changes

@joe4dev joe4dev Awaiting requested review from joe4dev

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

@gregfurman gregfurman Awaiting requested review from gregfurman

Assignees

@cloutierMat cloutierMat

Labels
aws:stepfunctions AWS Step Functions semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.1
Development

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