From a1fb0bebd7e08b257bf963320ee733509ba37141 Mon Sep 17 00:00:00 2001 From: Jordan De Mello Date: Thu, 28 Mar 2024 15:58:29 -0400 Subject: [PATCH 1/3] fix(stepfunctions): the catch field in CustomState is not rendered --- .../test/integ.custom-state.ts | 32 ++-- .../lib/states/custom-state.ts | 7 +- .../test/custom-state.test.ts | 144 +++++++++++++++++- 3 files changed, 171 insertions(+), 12 deletions(-) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.ts index a3bd05c8de318..0b7a8e01a445b 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.ts +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.ts @@ -23,11 +23,6 @@ const stateJson = { }, }, ResultPath: null, - Retry: [{ - ErrorEquals: [sfn.Errors.PERMISSIONS], - IntervalSeconds: 20, - MaxAttempts: 2, - }], }; const failure = new sfn.Fail(stack, 'failed', { @@ -39,10 +34,6 @@ const custom = new sfn.CustomState(stack, 'my custom task', { stateJson, }); -const customWithInlineRetry = new sfn.CustomState(stack, 'my custom task with inline Retriers', { - stateJson, -}); - custom.addCatch(failure); custom.addRetry({ errors: [sfn.Errors.TIMEOUT], @@ -50,7 +41,28 @@ custom.addRetry({ maxAttempts: 5, }); -const chain = sfn.Chain.start(custom).next(customWithInlineRetry).next(finalStatus); +const customWithInlineRetry = new sfn.CustomState(stack, 'my custom task with inline Retriers', { + stateJson: { + ...stateJson, + Retry: [{ + ErrorEquals: [sfn.Errors.PERMISSIONS], + IntervalSeconds: 20, + MaxAttempts: 2, + }] + } +}); + +const customWithInlineCatch = new sfn.CustomState(stack, 'my custom task with inline Catchers', { + stateJson: { + ...stateJson, + Catch: [{ + ErrorEquals: [sfn.Errors.PERMISSIONS], + Next: 'failed' + }] + } +}); + +const chain = sfn.Chain.start(custom).next(customWithInlineRetry).next(customWithInlineCatch).next(finalStatus); const sm = new sfn.StateMachine(stack, 'StateMachine', { definition: chain, diff --git a/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts b/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts index 63744f75e9433..c27aaec92bffc 100644 --- a/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts +++ b/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts @@ -74,11 +74,16 @@ export class CustomState extends State implements IChainable, INextable { ...this.renderRetryCatch(), }; - // merge the Retry field defined in the stateJson into the state + // Retriers and Catchers can be specified directly in the stateJson or indirectly to the construct with addRetry() and addCatch(). + // renderRetryCatch() only renders the indirectly supplied Retriers and Catchers, so we need to manually merge in those directly in the stateJson if (Array.isArray(this.stateJson.Retry)) { state.Retry = Array.isArray(state.Retry) ? [...state.Retry, ...this.stateJson.Retry] : [...this.stateJson.Retry]; } + if (Array.isArray(this.stateJson.Catch)) { + state.Catch = Array.isArray(state.Catch) ? [...state.Catch, ...this.stateJson.Catch] : [...this.stateJson.Catch]; + } + return state; } } diff --git a/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts b/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts index 610e25b425ad9..cfd490288dc70 100644 --- a/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts +++ b/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts @@ -1,8 +1,9 @@ import { render } from './private/render-util'; import * as cdk from '../../core'; import * as sfn from '../lib'; +import { Errors } from '../lib/types'; -describe('Custom State', () => { +describe.only('Custom State', () => { let stack: cdk.Stack; let stateJson: any; @@ -309,4 +310,145 @@ describe('Custom State', () => { }, ); }); + + test('expect retry to merge when specifying strategy inline and through construct', () => { + // GIVEN + const custom = new sfn.CustomState(stack, 'Custom', { + stateJson: { + ...stateJson, + Retry: [{ + ErrorEquals: ['States.TaskFailed'], + }], + }, + }).addRetry({ errors: [Errors.TIMEOUT] }); + const chain = sfn.Chain.start(custom); + + // THEN + expect(render(stack, chain)).toStrictEqual( + { + StartAt: 'Custom', + States: { + Custom: { + Type: 'Task', + Resource: 'arn:aws:states:::dynamodb:putItem', + Parameters: { + TableName: 'MyTable', + Item: { + id: { + S: 'MyEntry', + }, + }, + }, + ResultPath: null, + Retry: [ + { + ErrorEquals: ['States.Timeout'], + }, + { + ErrorEquals: ['States.TaskFailed'], + }, + ], + End: true, + }, + }, + }, + ); + }); + + test('expect catch to not fail when specifying strategy inline', () => { + // GIVEN + const custom = new sfn.CustomState(stack, 'Custom', { + stateJson: { + ...stateJson, + Catch: [{ + ErrorEquals: ['States.TaskFailed'], + Next: 'Failed', + }], + }, + }); + const chain = sfn.Chain.start(custom); + + // THEN + expect(render(stack, chain)).toStrictEqual( + { + StartAt: 'Custom', + States: { + Custom: { + Type: 'Task', + Resource: 'arn:aws:states:::dynamodb:putItem', + Parameters: { + TableName: 'MyTable', + Item: { + id: { + S: 'MyEntry', + }, + }, + }, + ResultPath: null, + Catch: [{ + ErrorEquals: ['States.TaskFailed'], + Next: 'Failed', + }], + End: true, + }, + }, + }, + ); + }); + + test('expect catch to merge when specifying strategy inline and through construct', () => { + // GIVEN + const failure = new sfn.Fail(stack, 'Failed', { + error: 'DidNotWork', + cause: 'We got stuck', + }); + + const custom = new sfn.CustomState(stack, 'Custom', { + stateJson: { + ...stateJson, + Catch: [{ + ErrorEquals: ['States.TaskFailed'], + Next: 'Failed', + }], + }, + }).addCatch(failure, { errors: [Errors.TIMEOUT] }); + const chain = sfn.Chain.start(custom); + + // THEN + expect(render(stack, chain)).toStrictEqual( + { + StartAt: 'Custom', + States: { + Custom: { + Type: 'Task', + Resource: 'arn:aws:states:::dynamodb:putItem', + Parameters: { + TableName: 'MyTable', + Item: { + id: { + S: 'MyEntry', + }, + }, + }, + ResultPath: null, + Catch: [ + { + ErrorEquals: ['States.Timeout'], + Next: 'Failed', + }, { + ErrorEquals: ['States.TaskFailed'], + Next: 'Failed', + }, + ], + End: true, + }, + Failed: { + Type: 'Fail', + Error: 'DidNotWork', + Cause: 'We got stuck', + }, + }, + }, + ); + }); }); \ No newline at end of file From 75a074598ed52a7ea5fe481a6de2c5965055dc9e Mon Sep 17 00:00:00 2001 From: Jordan De Mello Date: Fri, 29 Mar 2024 13:34:52 -0400 Subject: [PATCH 2/3] Fix linting issues --- .../aws-stepfunctions-custom-state-integ.template.json | 2 +- .../test/aws-stepfunctions/test/integ.custom-state.ts | 10 +++++----- .../aws-stepfunctions/test/custom-state.test.ts | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.js.snapshot/aws-stepfunctions-custom-state-integ.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.js.snapshot/aws-stepfunctions-custom-state-integ.template.json index e350c49d15925..22f258c125c05 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.js.snapshot/aws-stepfunctions-custom-state-integ.template.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.js.snapshot/aws-stepfunctions-custom-state-integ.template.json @@ -20,7 +20,7 @@ "StateMachine2E01A3A5": { "Type": "AWS::StepFunctions::StateMachine", "Properties": { - "DefinitionString": "{\"StartAt\":\"my custom task\",\"States\":{\"my custom task\":{\"Next\":\"my custom task with inline Retriers\",\"Type\":\"Task\",\"Resource\":\"arn:aws:states:::dynamodb:putItem\",\"Parameters\":{\"TableName\":\"my-cool-table\",\"Item\":{\"id\":{\"S\":\"my-entry\"}}},\"ResultPath\":null,\"Retry\":[{\"ErrorEquals\":[\"States.Timeout\"],\"IntervalSeconds\":10,\"MaxAttempts\":5},{\"ErrorEquals\":[\"States.Permissions\"],\"IntervalSeconds\":20,\"MaxAttempts\":2}],\"Catch\":[{\"ErrorEquals\":[\"States.ALL\"],\"Next\":\"failed\"}]},\"my custom task with inline Retriers\":{\"Next\":\"final step\",\"Type\":\"Task\",\"Resource\":\"arn:aws:states:::dynamodb:putItem\",\"Parameters\":{\"TableName\":\"my-cool-table\",\"Item\":{\"id\":{\"S\":\"my-entry\"}}},\"ResultPath\":null,\"Retry\":[{\"ErrorEquals\":[\"States.Permissions\"],\"IntervalSeconds\":20,\"MaxAttempts\":2}]},\"final step\":{\"Type\":\"Pass\",\"End\":true},\"failed\":{\"Type\":\"Fail\",\"Error\":\"DidNotWork\",\"Cause\":\"We got stuck\"}},\"TimeoutSeconds\":30}", + "DefinitionString": "{\"StartAt\":\"my custom task\",\"States\":{\"my custom task\":{\"Next\":\"my custom task with inline Retriers\",\"Type\":\"Task\",\"Resource\":\"arn:aws:states:::dynamodb:putItem\",\"Parameters\":{\"TableName\":\"my-cool-table\",\"Item\":{\"id\":{\"S\":\"my-entry\"}}},\"ResultPath\":null,\"Retry\":[{\"ErrorEquals\":[\"States.Timeout\"],\"IntervalSeconds\":10,\"MaxAttempts\":5}],\"Catch\":[{\"ErrorEquals\":[\"States.ALL\"],\"Next\":\"failed\"}]},\"my custom task with inline Retriers\":{\"Next\":\"my custom task with inline Catchers\",\"Type\":\"Task\",\"Resource\":\"arn:aws:states:::dynamodb:putItem\",\"Parameters\":{\"TableName\":\"my-cool-table\",\"Item\":{\"id\":{\"S\":\"my-entry\"}}},\"ResultPath\":null,\"Retry\":[{\"ErrorEquals\":[\"States.Permissions\"],\"IntervalSeconds\":20,\"MaxAttempts\":2}]},\"my custom task with inline Catchers\":{\"Next\":\"final step\",\"Type\":\"Task\",\"Resource\":\"arn:aws:states:::dynamodb:putItem\",\"Parameters\":{\"TableName\":\"my-cool-table\",\"Item\":{\"id\":{\"S\":\"my-entry\"}}},\"ResultPath\":null,\"Catch\":[{\"ErrorEquals\":[\"States.Permissions\"],\"Next\":\"failed\"}]},\"final step\":{\"Type\":\"Pass\",\"End\":true},\"failed\":{\"Type\":\"Fail\",\"Error\":\"DidNotWork\",\"Cause\":\"We got stuck\"}},\"TimeoutSeconds\":30}", "RoleArn": { "Fn::GetAtt": [ "StateMachineRoleB840431D", diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.ts index 0b7a8e01a445b..544eccc344621 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.ts +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.custom-state.ts @@ -48,8 +48,8 @@ const customWithInlineRetry = new sfn.CustomState(stack, 'my custom task with in ErrorEquals: [sfn.Errors.PERMISSIONS], IntervalSeconds: 20, MaxAttempts: 2, - }] - } + }], + }, }); const customWithInlineCatch = new sfn.CustomState(stack, 'my custom task with inline Catchers', { @@ -57,9 +57,9 @@ const customWithInlineCatch = new sfn.CustomState(stack, 'my custom task with in ...stateJson, Catch: [{ ErrorEquals: [sfn.Errors.PERMISSIONS], - Next: 'failed' - }] - } + Next: 'failed', + }], + }, }); const chain = sfn.Chain.start(custom).next(customWithInlineRetry).next(customWithInlineCatch).next(finalStatus); diff --git a/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts b/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts index cfd490288dc70..1040ceb24fcf2 100644 --- a/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts +++ b/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts @@ -3,7 +3,7 @@ import * as cdk from '../../core'; import * as sfn from '../lib'; import { Errors } from '../lib/types'; -describe.only('Custom State', () => { +describe('Custom State', () => { let stack: cdk.Stack; let stateJson: any; From c8edf8ab152f921ecf4c40c74bf07ecc25b33c33 Mon Sep 17 00:00:00 2001 From: Jordan De Mello Date: Fri, 5 Apr 2024 20:29:07 -0400 Subject: [PATCH 3/3] Add warning on multiple retry and catch sources --- .../lib/states/custom-state.ts | 49 +++++++++++++ .../test/custom-state.test.ts | 71 +++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts b/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts index c27aaec92bffc..0b0bff69565e3 100644 --- a/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts +++ b/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts @@ -1,6 +1,7 @@ import { Construct } from 'constructs'; import { State } from './state'; import { Chain } from '..'; +import { Annotations } from '../../../core/'; import { CatchProps, IChainable, INextable, RetryProps } from '../types'; /** @@ -74,6 +75,14 @@ export class CustomState extends State implements IChainable, INextable { ...this.renderRetryCatch(), }; + if (this.hasMultipleRetrySources(state)) { + this.addMultipleRetrySourcesWarning(); + } + + if (this.hasMultipleCatchSources(state)) { + this.addMultipleCatchSourcesWarning(); + } + // Retriers and Catchers can be specified directly in the stateJson or indirectly to the construct with addRetry() and addCatch(). // renderRetryCatch() only renders the indirectly supplied Retriers and Catchers, so we need to manually merge in those directly in the stateJson if (Array.isArray(this.stateJson.Retry)) { @@ -86,4 +95,44 @@ export class CustomState extends State implements IChainable, INextable { return state; } + + private hasMultipleRetrySources(state: any): boolean { + if (!Array.isArray(state.Retry)) { + return false; + } + + if (!Array.isArray(this.stateJson.Retry)) { + return false; + } + + return state.Retry.length > 0 && this.stateJson.Retry.length > 0; + } + + private hasMultipleCatchSources(state: any): boolean { + if (!Array.isArray(state.Catch)) { + return false; + } + + if (!Array.isArray(this.stateJson.Catch)) { + return false; + } + + return state.Catch.length > 0 && this.stateJson.Catch.length > 0; + } + + private addMultipleRetrySourcesWarning(): void { + Annotations.of(this).addWarningV2('@aws-cdk/aws-stepfunctions:multipleRetrySources', [ + 'CustomState constructs can configure state retries using the stateJson property or by using the addRetry() function.', + 'When retries are configured using both of these, the state definition\'s Retry field is generated ', + 'by first rendering retries from addRetry(), then rendering retries from the stateJson.', + ].join('\n')); + } + + private addMultipleCatchSourcesWarning(): void { + Annotations.of(this).addWarningV2('@aws-cdk/aws-stepfunctions:multipleCatchSources', [ + 'CustomState constructs can configure state catchers using the stateJson property or by using the addCatch() function.', + 'When catchers are configured using both of these, the state definition\'s Catch field is generated ', + 'by first rendering catchers from addCatch(), then rendering catchers from the stateJson.', + ].join('\n')); + } } diff --git a/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts b/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts index 1040ceb24fcf2..f332c25924f71 100644 --- a/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts +++ b/packages/aws-cdk-lib/aws-stepfunctions/test/custom-state.test.ts @@ -1,4 +1,5 @@ import { render } from './private/render-util'; +import { Annotations, Match } from '../../assertions'; import * as cdk from '../../core'; import * as sfn from '../lib'; import { Errors } from '../lib/types'; @@ -451,4 +452,74 @@ describe('Custom State', () => { }, ); }); + + test('expect warning message to be emitted when retries specified both in stateJson and through addRetry()', () => { + const customState = new sfn.CustomState(stack, 'my custom task', { + stateJson: { + Type: 'Task', + Resource: 'arn:aws:states:::dynamodb:putItem', + Parameters: { + TableName: 'my-cool-table', + Item: { + id: { + S: 'my-entry', + }, + }, + }, + Retry: [{ + ErrorEquals: ['States.TaskFailed'], + }], + }, + }); + + customState.addRetry({ + errors: [sfn.Errors.TIMEOUT], + interval: cdk.Duration.seconds(10), + maxAttempts: 5, + }); + + new sfn.StateMachine(stack, 'StateMachine', { + definition: sfn.Chain.start(customState), + timeout: cdk.Duration.seconds(30), + }); + + Annotations.fromStack(stack).hasWarning('/Default/my custom task', Match.stringLikeRegexp('CustomState constructs can configure state retries')); + }); + + test('expect warning message to be emitted when catchers specified both in stateJson and through addCatch()', () => { + const customState = new sfn.CustomState(stack, 'my custom task', { + stateJson: { + Type: 'Task', + Resource: 'arn:aws:states:::dynamodb:putItem', + Parameters: { + TableName: 'my-cool-table', + Item: { + id: { + S: 'my-entry', + }, + }, + }, + Catch: [ + { + ErrorEquals: ['States.Timeout'], + Next: 'Failed', + }, + ], + }, + }); + + const failure = new sfn.Fail(stack, 'Failed', { + error: 'DidNotWork', + cause: 'We got stuck', + }); + + customState.addCatch(failure, { errors: [Errors.TIMEOUT] }); + + new sfn.StateMachine(stack, 'StateMachine', { + definition: sfn.Chain.start(customState), + timeout: cdk.Duration.seconds(30), + }); + + Annotations.fromStack(stack).hasWarning('/Default/my custom task', Match.stringLikeRegexp('CustomState constructs can configure state catchers')); + }); }); \ No newline at end of file