8000 [SFN] No Output Reductions for Successful Catch Blocks by MEPalma · Pull Request #11552 · localstack/localstack · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[SFN] No Output Reductions for Successful Catch Blocks #11552

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 3 commits into from
Oct 7, 2024
Merged

[SFN] No Output Reductions for Successful Catch Blocks #11552

merged 3 commits into from
Oct 7, 2024

Conversation

MEPalma
Copy link
Contributor
@MEPalma MEPalma commented Sep 20, 2024

Motivation

When an error in a state is successfully handled by a catch block, the error object containing the error message and cause string is directly passed as the state's output. This means that if the user defines output reduction logic, such as ResultPath and OutputPath, these are not applied. However, the current implementation of the SFN v2 interpreter mistakenly applies these reductions, which can lead to JsonPath errors and disrupt the state machine's logic, as seen in issue #11494.
These changes type the output handling for successful catch blocks so these can be detected and bypass output reductions.

Changes

  • Typed CatcherDecl outputs to be CatcherOutput objects
  • Add bypass logic in the state's evaluation of the output
  • Added relevant snapshot tests which test the use of OutputPath and ResultPath as output reductions

@MEPalma MEPalma added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Sep 20, 2024
@MEPalma MEPalma added this to the 3.8 milestone Sep 20, 2024
@MEPalma MEPalma self-assigned this Sep 20, 2024
@github-actions GitHub Actions
Copy link
github-actions bot commented Sep 20, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0    2 suites  ±0   29m 31s ⏱️ - 1h 5m 16s
1 048 tests  - 2 393  890 ✅  - 2 153  158 💤  - 240  0 ❌ ±0 
1 050 runs   - 2 393  890 ✅  - 2 153  160 💤  - 240  0 ❌ ±0 

Results for commit f5ece05. ± Comparison against base commit 5271fc0.

This pull request removes 2403 and adds 10 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_basic.TestAPIGateway ‑ test_create_rest_api_with_custom_id[localstack_path_based_url]
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_invoke_endpoint_cors_headers[http://allowed-False-UrlType.LS_PATH_BASED]
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_invoke_endpoint_cors_headers[http://allowed-True-UrlType.LS_PATH_BASED]
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_invoke_endpoint_cors_headers[http://denied-False-UrlType.LS_PATH_BASED]
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_invoke_endpoint_cors_headers[http://denied-True-UrlType.LS_PATH_BASED]
tests.aws.services.apigateway.test_apigateway_common.TestApigatewayRouting ‑ test_api_not_existing
tests.aws.services.events.test_events_targets.TestEventsTargetApiGateway ‑ test_put_events_with_target_api_gateway
tests.aws.services.stepfunctions.v2.error_handling.test_task_service_lambda.TestTaskServiceLambda ‑ test_raise_exception_catch_output_path[$.Payload]
tests.aws.services.stepfunctions.v2.error_handling.test_task_service_lambda.TestTaskServiceLambda ‑ test_raise_exception_catch_output_path[$.no.such.path]
tests.aws.services.stepfunctions.v2.error_handling.test_task_service_lambda.TestTaskServiceLambda ‑ test_raise_exception_catch_output_path[None]
This pull request removes 246 skipped tests and adds 6 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.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_create_rest_api_with_custom_id[localstack_path_based_url]
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_invoke_endpoint_cors_headers[http://allowed-False-UrlType.LS_PATH_BASED]
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_invoke_endpoint_cors_headers[http://allowed-True-UrlType.LS_PATH_BASED]
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_invoke_endpoint_cors_headers[http://denied-False-UrlType.LS_PATH_BASED]
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_invoke_endpoint_cors_headers[http://denied-True-UrlType.LS_PATH_BASED]
tests.aws.services.events.test_events_targets.TestEventsTargetApiGateway ‑ test_put_events_with_target_api_gateway

♻️ This comment has been updated with latest results.

gregfurman
gregfurman approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor
@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

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

LGTM! Just a question on simplifying the statemachine and whether we are including all necessary test-cases for these changes.

localstack-core/localstack/services/stepfunctions/asl/component/state/state.py
Comment on lines +182 to +183
if isinstance(output, CatcherOutput):
env.inp = copy.deepcopy(output)
Copy link
Contributor

Choose a reason for hiding this comment

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

Kudos on this clean solution, wow!

Copy link

Choose a reason for hiding this comment

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

Thank you! I've been struggling for hours to understand what was going on here as I believe Step Functions Catch on custom errors is effectively broken due to this :)

@MEPalma MEPalma modified the milestones: 3.8, 4.0 Oct 1, 2024
derek-cbtw
derek-cbtw reviewed Oct 7, 2024
View reviewed changes
localstack-core/localstack/services/stepfunctions/asl/component/state/state.py Outdated
self.comment = None
self.input_path = InputPath(InputPath.DEFAULT_PATH)
self.output_path = OutputPath(OutputPath.DEFAULT_PATH)
self.output_path = OutputPath(OutputPath.DEFAULT_PATH)
Copy link

Choose a reason for hiding this comment

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

Double assignment

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.output_path = OutputPath(OutputPath.DEFAULT_PATH)

@MEPalma MEPalma added semver: patch Non-breaking changes which can be included in patch releases semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases and removed semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases semver: patch Non-breaking changes which can be included in patch releases labels Oct 7, 2024
@MEPalma MEPalma merged commit 8de5342 into master Oct 7, 2024
50 of 53 checks passed
@MEPalma MEPalma deleted the MEP-sfn-no_state_output_reductions_on_catch branch October 7, 2024 12:47
macnev2013 pushed a commit that referenced this pull request Oct 22, 2024
@MEPalma @macnev2013
Step Functions: No Output Reductions for Successful Catch Blocks (#11552
41c4922 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregfurman gregfurman gregfurman approved these changes

@derek-cbtw derek-cbtw derek-cbtw left review comments

@joe4dev joe4dev Awaiting requested review from joe4dev joe4dev is a code owner

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

Assignees

@MEPalma MEPalma

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@MEPalma @gregfurman @derek-cbtw
0