aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.55k stars 3.87k forks source link

stepfunctions: Default Choice state not created when using afterwards with includeOtherwise #20685

Open andriusvelykis opened 2 years ago

andriusvelykis commented 2 years ago

Describe the bug

It is not possible to use nested Choice states that only define single .when() cases each and utilise .afterwards({ includeOtherwise: true }) for the Default flow, because the nested Default branch is not created.

The state machine would approximate the following algorithm:

if (firstFlag) {
  // Do Something
  if (secondFlag) {
    // Failure
  }
}
// All Successful

Expected Behavior

The inner Choice should have a Default transition to All Successful:

{
  "StartAt": "First Choice",
  "States": {
    "First Choice": {
      "Type": "Choice",
      "Choices": [
        {
          "Variable": "$.firstFlag",
          "BooleanEquals": false,
          "Next": "Do Something"
        }
      ],
      "Default": "All Successful"
    },
    "All Successful": {
      "Type": "Succeed"
    },
    "Do Something": {
      "Type": "Pass",
      "Next": "Second Choice"
    },
    "Second Choice": {
      "Type": "Choice",
      "Choices": [
        {
          "Variable": "$.secondFlag",
          "BooleanEquals": false,
          "Next": "Failure"
        }
      ],
      "Default": "All Successful"
    },
    "Failure": {
      "Type": "Fail"
    }
  }
}

expected-inner-default

Current Behavior

The inner Choice does not have a Default transition created:

{
  "StartAt": "First Choice",
  "States": {
    "First Choice": {
      "Type": "Choice",
      "Choices": [
        {
          "Variable": "$.firstFlag",
          "BooleanEquals": false,
          "Next": "Do Something"
        }
      ],
      "Default": "All Successful"
    },
    "All Successful": {
      "Type": "Succeed"
    },
    "Do Something": {
      "Type": "Pass",
      "Next": "Second Choice"
    },
    "Second Choice": {
      "Type": "Choice",
      "Choices": [
        {
          "Variable": "$.secondFlag",
          "BooleanEquals": false,
          "Next": "Failure"
        }
      ]
    },
    "Failure": {
      "Type": "Fail"
    }
  }
}

no-inner-default

Reproduction Steps

Create the step machine using the following CDK:

new sfn.Choice(stack, "First Choice")
  .when(
    sfn.Condition.booleanEquals("$.firstFlag", false),
    new sfn.Pass(stack, "Do Something").next(
      new sfn.Choice(stack, "Second Choice")
        .when(sfn.Condition.booleanEquals("$.secondFlag", false), new sfn.Fail(stack, "Failure"))
        .afterwards({ includeOtherwise: true })
    )
  )
  .afterwards({ includeOtherwise: true })
  .next(new sfn.Succeed(stack, "All Successful"))

Execute the step machine using the following input to trigger the missing Default branch:

{
  "firstFlag": false,
  "secondFlag": true
}

Possible Solution

No response

Additional Information/Context

This could be worked around by providing a dummy Pass state after .afterwards() definition:

new sfn.Choice(stack, "First Choice")
  .when(
    sfn.Condition.booleanEquals("$.firstFlag", false),
    new sfn.Pass(stack, "Do Something").next(
      new sfn.Choice(stack, "Second Choice")
        .when(sfn.Condition.booleanEquals("$.secondFlag", false), new sfn.Fail(stack, "Failure"))
        .afterwards({ includeOtherwise: true })
        .next(new sfn.Pass(stack, "Unnecessary Do-Nothing Pass for Default"))
    )
  )
  .afterwards({ includeOtherwise: true })
  .next(new sfn.Succeed(stack, "All Successful"))

workaround-default

CDK CLI Version

1.159.0

Framework Version

No response

Node.js Version

16.13.2

OS

MacOS

Language

Typescript

Language Version

4.5.5

Other information

Might be related to issue aws/aws-cdk#16210 and discussion aws/aws-cdk#19718.

peterwoodworth commented 2 years ago

Isn't this what Choice.otherwise() is supposed to accomplish?

github-actions[bot] commented 2 years ago

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

andriusvelykis commented 2 years ago

@peterwoodworth Choice.otherwise() requires to explicitly indicate the next state - whereas using .afterwards({ includeOtherwise: true }) allows creating a state machine fragment that afterwards can be chained to something else without knowing at the time of definition. This is important to create reusable state machine fragments that can be chained to complex workflows.

If you see the outer Choice statement in my example, it does not have an otherwise() definition, and the All Successful step is added using .afterwards({ includeOtherwise: true }).next(/* All Successful */). It works at the top level - the Default (i.e. otherwise) branch is created correctly. But when Choice is nested, the same pattern does not work - that's why I consider it a bug.

tocology commented 10 months ago

Is there any update on this? I tested out with the newer version of cdk and have the similar pattern but still doesn't work for the nested choice.

abdelnn commented 6 months ago

I think this is working as intended. The reason this behaviour is seen is simply because the next Choice state does not contain a .next state, so the includeOtherwise: true does not know what it's default is. Reading through the comments in the implementation:

https://github.com/aws/aws-cdk/blob/919d16ff611ee01495ae2cb4c646c4e27378b3e3/packages/aws-cdk-lib/aws-stepfunctions/lib/states/choice.ts#L122-L130

A default is only added when a .next is added to your chain after specifying .afterwards({ includeOtherwise: true })

I don't think this is a nested state issue, as the reason the top level choice works is because it specifies the next state after its .afterwards({ includeOtherwise: true })

The default is set is a next as can be seen here as well:

https://github.com/aws/aws-cdk/blob/919d16ff611ee01495ae2cb4c646c4e27378b3e3/packages/aws-cdk-lib/aws-stepfunctions/lib/states/choice.ts#L133-L144

khushail commented 3 months ago

Hi @andriusvelykis , as explained by @abdelnn here - https://github.com/aws/aws-cdk/issues/20685#issuecomment-1986673919, this explanation seems helpful and descriptive. Could you please verify if this issue is helpful?

github-actions[bot] commented 3 months ago

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

kamzn commented 2 months ago

I also came across this issue organically and assumed that .afterwards({includeOtherwise: true}) would gather up the nested choices with no defaults.

I did fix it using a dummy pass step, though in my case instead of putting an .afterwards({includeOtherwise: true}).next( new sfn.Pass(stack, "Unnecessary Do-Nothing Pass for Default") on the inside choice I think it's slightly cleaner to explicitly define the .otherwise() on the inside choice, which also works:

new sfn.Choice(stack, "First Choice")
  .when(
    sfn.Condition.booleanEquals("$.firstFlag", false),
    new sfn.Pass(stack, "Do Something").next(
      new sfn.Choice(stack, "Second Choice")
        .when(sfn.Condition.booleanEquals("$.secondFlag", false), new sfn.Fail(stack, "Failure"))
        .otherwise(new sfn.Pass(stack, "Unnecessary Do-Nothing Pass for Default"))
    )
  )
  .afterwards({ includeOtherwise: true })
  .next(new sfn.Succeed(stack, "All Successful"))