fix(stepfunctions): resolve JSONata $merge regression with dynamic args#13717
Conversation
shubhiscoding
requested review from
dominikschubert,
gregfurman,
joe4dev and
tiurin
as code owners
|
Hey guys the pr is ready for review, would really like for someone to take a look at it please |
tiurin
left a comment
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
added
aws:stepfunctions
tiurin
left a comment
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.
|
Updated the pr to Narrow the capturing group to skip bare Also I ran all tests in |
tiurin
left a comment
tiurin
left a comment
There was a problem hiding this comment.
@shubhiscoding thanks for addressing the changes! Currently, CI run fails because of linting error. Please run make format to fix those errors.
|
done |
tiurin
left a comment
tiurin
left a comment
There was a problem hiding this comment.
Nice work @shubhiscoding, thank you for the contribution and following up on change requests!
tiurin
added
notes: needed
2 participants
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