Skip to content
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.

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

(stepfunctions): the service does not support JSON paths for MaxConcurrency on a Map state but CDK can generate one #20835

Open
wvauclain opened this issue Jun 22, 2022 · 3 comments
Labels
@aws-cdk/aws-stepfunctions Related to AWS StepFunctions bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@wvauclain
Copy link

wvauclain commented Jun 22, 2022

Describe the bug

Allowing JSON paths for MaxConcurrency was added in response to #20152, but JSON paths are not actually supported for the MaxConcurrency key in a Map state by Step Functions.

Expected Behavior

I would expect synth to fail in the reproduction example.

Current Behavior

Synth succeeds, but deploying fails with the following error:

11:31:04 AM | CREATE_FAILED        | AWS::StepFunctions::StateMachine | StateMachine2E01A3A5
Resource handler returned message: "Invalid State Machine Definition: 'SCHEMA_VALIDATION_FAILED: Expected value of type Integer at /States/Map State/MaxConcurrency'"

Reproduction Steps

import { Duration, Stack, StackProps } from 'aws-cdk-lib';
import * as stepfunctions from 'aws-cdk-lib/aws-stepfunctions';
import { Construct } from 'constructs';

export class SfReproStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const map = new stepfunctions.Map(this, 'Map State', {
      maxConcurrency: stepfunctions.JsonPath.numberAt('$.maxConcurrency'),
      itemsPath: stepfunctions.JsonPath.stringAt('$.inputForMap'),
    });
    map.iterator(new stepfunctions.Pass(this, 'Pass State'));

    const machine = new stepfunctions.StateMachine(this, "StateMachine", {
      definition: map,
      stateMachineName: 'ReproStateMachine',
      timeout: Duration.minutes(5),
    });
  }
}

Possible Solution

I think #20279 should be reverted until Step Functions supports this.

Additional Information/Context

No response

CDK CLI Version

2.28.1 (build d035432)

Framework Version

No response

Node.js Version

v18.4.0

OS

macOS 12.3.1

Language

Typescript

Language Version

No response

Other information

No response

@wvauclain wvauclain added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 22, 2022
@github-actions github-actions bot added the @aws-cdk/aws-stepfunctions Related to AWS StepFunctions label Jun 22, 2022
@wvauclain wvauclain changed the title (stepfunctions): the service does not support MaxConcurrency on a Map state but CDK can generate one (stepfunctions): the service does not support JSON paths for MaxConcurrency on a Map state but CDK can generate one Jun 22, 2022
@kaizencc
Copy link
Contributor

Hi @wvauclain! all #20279 was not invalidate on tokens, so it should stay. but i think you are right, max concurrency does not support json paths so that is bad. The fix here is that we should check to see if we've been given a json path and throw a more descriptive error in that case. Thanks for bringing this to our attention!

@connorgiles
Copy link

connorgiles commented Dec 15, 2023

Also running into this issue. It appears it might need to be provided as MaxConcurrencyPath instead of MaxConcurrency based on these docs.

Is there any reason to not want to expose the additional ASL property?

mergify bot pushed a commit that referenced this issue Mar 2, 2024
### Issue # (if applicable)

Relates to #20835 

### Reason for this change

`MaxConcurrency` does not support `JsonPath`. This change adds `MaxConcurrencyPath` so that CDK users can specify a `JsonPath` for their `MaxConcurrency`

_Note_ : This does not invalidate JsonPaths for `MaxConcurrency`, as I'm unsure how to do so without reverting #20279 . Open to suggestions

### Description of changes

Added a new `maxConcurrencyPath` field that accepts a `JsonPath` value. Decided to go with another explicit field as it is similar to what is done for `ErrorPath` and `CausePath`, in addition to most other Path fields

### Description of how you validated changes

Added unit tests

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@abdelnn
Copy link
Contributor

abdelnn commented Mar 15, 2024

Update: I've added support for MaxConcurrencyPath in #29330 , however that change does not include invalidating JsonPath values for MaxConcurrency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-stepfunctions Related to AWS StepFunctions bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

No branches or pull requests

4 participants