Step Functions, Support for Output Blocks in Choice Rules, Improvments to JSONata Choice Defaults

[MEPalma]

Motivation

The SFN v2 interpreter currently lacks support for Output block declarations within choice rules, as noted in #12048. Additionally, the current implementation of the choice state does not handle Output and Assign blocks as intended, since it always evaluates the default Assign and Output blocks before returning.
A further issue lies in the use of a member variable to track the next state name, which has been identified as an antipattern and addressed in recent changes #12068.

Changes

• Adds support for Output blocks in choice rules.
• Refines the evaluation logic for default Assign and Output blocks.
• Resolves the evaluation component antipattern.
• Snapshot tests for Output blocks in choice rules
• Snapshot tests for default Ouput and Assign evaluation logic

[github-actions]

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   32m 18s ⏱️ - 1h 20m 37s
1 418 tests  - 2 595  1 346 ✅  - 2 350  72 💤  - 245  0 ❌ ±0 
1 420 runs   - 2 595  1 346 ✅  - 2 350  74 💤  - 245  0 ❌ ±0 

Results for commit 340ee29. ± Comparison against base commit 81d9fb0.

This pull request removes 2600 and adds 5 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.apigateway.test_apigateway_import.TestApiGatewayImportRestApi ‑ test_import_with_integer_http_status_code
tests.aws.services.stepfunctions.v2.assign.test_assign_base.TestAssignBase ‑ test_assign_in_choice[CONDITION_FALSE]
tests.aws.services.stepfunctions.v2.assign.test_assign_base.TestAssignBase ‑ test_assign_in_choice[CONDITION_TRUE]
tests.aws.services.stepfunctions.v2.outputdecl.test_output.TestArgumentsBase ‑ test_output_in_choice[CONDITION_FALSE]
tests.aws.services.stepfunctions.v2.outputdecl.test_output.TestArgumentsBase ‑ test_output_in_choice[CONDITION_TRUE]

This pull request skips 1 test.

tests.aws.services.stepfunctions.v2.evaluate_jsonata.test_base_evaluate_expressions.TestBaseEvaluateJsonata ‑ test_base_task_from_input[TIMEOUT_SECONDS]

♻️ This comment has been updated with latest results.

[tiurin]

question: Did you need to override base _eval_body method because of this legacy handling? If yes, is there any existing test that covers this?

[MEPalma]

I refactored the reusable parts of states' evaluation into: loading the state input, evaluating the state logic, and evaluation the state output. whilst the logical glue of these three steps is being reused in the _eval_body of CommonStateField. This works well with the two known exception for evalutatint the state output: catch blocks in execution states and choice states.
We do have many snpahost tests for legacy (or JSONPath) choice states in aws.services.stepfunctions.v2.scenarios

[tiurin]

Great refactoring! 🎉
Now all special cases are handled within their respective states and common method is clean and readable! 👏

[tiurin]

LGTM!
I'd appreciate a bit more information about the differences from base _eval_body method. It is not obvious why it needs to be overridden. Override that mostly repeats base method with slight differences adds additional maintenance effort.

As a suggestion to follow up later, maybe we could analyze if super()._eval_body(env=env) can be called from override instead of copying most parts of it?

[tiurin]

question: in the base method this block is wrapped in a try-catch block that handles NoSuchJsonPathError. Why is it not needed here?

[tiurin]

Also, there is another generic check in base method that is not present here:

        if not isinstance(env.program_state(), ProgramRunning):
            return

Is there anything specific about Choice state that doesn't require the check?