fix(stepfunctions): resolve JSONata $merge regression with dynamic args#13717
fix(stepfunctions): resolve JSONata $merge regression with dynamic args#13717shubhiscoding wants to merge 2 commits intolocalstack:mainfrom
Conversation
|
Hey guys the pr is ready for review, would really like for someone to take a look at it please |
tiurin
left a comment
There was a problem hiding this comment.
Hi @shubhiscoding, thank you for looking into the issue and providing a PR! We will review it shortly. Non-capturing branch was added recently, let us review the reason for that change first.
Immediate feedback on tests: the preferred way of testing the changes is with AWS parity tests. While unit tests make sense in some cases, adding a validated snapshot test to tests/aws/services/stepfunctions/v2/evaluate_jsonata that fails before the change and passes after it would guarantee the parity with AWS.
|
Hii @tiurin Thanks for the feedback! I've added an AWS-validated snapshot test ( Kept the unit tests as well since they directly test the regex extraction and are fast to run, but happy to remove them if you'd prefer only the snapshot test. Let me know if any other changes are needed! |
tiurin
left a comment
There was a problem hiding this comment.
@shubhiscoding Thanks for addressing comment about parity tests, nice work!
I've approved CI pipeline runs for this PR. On the first run all jobs failed because a significant change has just been introduced. Please rebase this branch onto the latest main to make CI execute checks correctly.
Meanwhile, I've run step functions test suite locally on this branch and unfortunately the fix introduces another regression. See comment below for the details.
| # allowing escapes | ||
| r"(?:\"(?:\\.|[^\"\\])*\"|\'(?:\\.|[^\'\\])*\')" | ||
| r"|" | ||
| # 3) Non-capturing branch for bracket expressions [...] |
There was a problem hiding this comment.
This has been added while working on adding new capabilities to TestState API. Removing it makes test_map_state_failure_count test fail:
Specifically, the one that uses
map_task_state state machine template.
While the change introduced regression, simply removing it will introduce another one. Let's try to find a solution that makes all existing tests pass.
Fixes #13579
Motivation
Since v4.12, the JSONata
$merge()function returns{}when merging objectsfrom dynamic input references like
$states.input.part1. Static values work fine.Changes
Removed the bracket expression branch from the variable reference regex in
jsonata.py. This branch was consuming variable references inside JSONataarray literals
[...], preventing them from being bound to actual values.The bracket branch was redundant — its purpose was to prevent false $ captures inside bracket notation like $states.input["field-name"], but this case is already handled by branch 2 (string literal skipper), which consumes the "field-name" before the variable branch can see it. All other Step Functions JSONata operations (string literals, regex literals, bracket field access, filter expressions) continue to work correctly without it.
Added unit tests for
extract_jsonata_variable_referencescovering arrayliterals, bracket field access, string/regex literals, and edge cases.
Tests
pytest tests/unit/services/stepfunctions/test_jsonata_variable_references.pypytest tests/aws/services/stepfunctions/v2/evaluate_jsonata/$merge([$states.input.part1, $states.input.part2])now returns merged resultRelated