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.33k stars 3.76k forks source link

aws_stepfunctions_tasks: DynamoAttributeValue.fromStringSet requires an array #25693

Open dbartholomae opened 1 year ago

dbartholomae commented 1 year ago

Describe the bug

When creating a JsonPath value, it returns a string even for arrays, but DynamoAttributeValue.fromStringSet needs a string array.

    const releaseSlot = new DynamoUpdateItem(this, "ReleaseSlot", {
      key: {
        [partitionKey]: DynamoAttributeValue.fromString(fixedKey),
      },

      updateExpression: `DELETE ${workflowsKey} :val`,
      expressionAttributeValues: {
        ":val": DynamoAttributeValue.fromStringSet(
          JsonPath.array(
            JsonPath.stringAt("$$.Execution.Id")
          ) as unknown as string[]
        ),
      },
      table: slots,
    });

Expected Behavior

This should work without the as unknown as string[] cast

Current Behavior

as unknown as string[] cast is required

Reproduction Steps

The code above without the cast will cause the TypeScript error.

Possible Solution

Accept a string or string[]. This would be non-breaking.

Additional Information/Context

No response

CDK CLI Version

2.80.0

Framework Version

No response

Node.js Version

16.14.2

OS

Mac

Language

Typescript

Language Version

5.0.4

Other information

No response

peterwoodworth commented 1 year ago

Since JsonPath.array returns a string, you can specify the JsonPath.array to be in an array like so:

    new tasks.DynamoUpdateItem(this, 'UpdateItem', {
      key: {
        MessageId: tasks.DynamoAttributeValue.fromString('message-007')
      },
      table: table,
      expressionAttributeValues: {
        ':val': tasks.DynamoAttributeValue.fromStringSet([sfn.JsonPath.array(sfn.JsonPath.stringAt('$.messageIds'))]),
        ':rand': tasks.DynamoAttributeValue.fromNumber(20),
      },
      updateExpression: 'SET TotalCount = :val + :rand',
    });

Accept a string or string[]. This would be non-breaking.

I think that with our current implementation, accepting just a string would never work. It looks to me like we will create the stringset definition with whatever is given since we're only expecting a string array. Additionally, I'm pretty sure we can't have union types because of the other languages https://github.com/aws/aws-cdk/blob/cdafcc52ad4aea3ef7f1446da7521fb504cb33b9/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/dynamodb/shared-types.ts#L165-L167

dbartholomae commented 1 year ago

My current implementation works on runtime and just fails on compile time. Unfortunately, your suggested solution doesn't: https://github.com/aws/aws-cdk/blob/cdafcc52ad4aea3ef7f1446da7521fb504cb33b9/packages/aws-cdk-lib/aws-stepfunctions/lib/private/json-path.ts#L190

JsonPath can't be used inside an array :(

You are right that unions won't work with jsii, though. Maybe an additional method?

peterwoodworth commented 1 year ago

You're right, I didn't actually attach the task when synthesizing before.

Maybe an additional method?

I think it would probably be best if we can create a new method and deprecate the old one, I'm not too sure exactly what this would look like however

dbartholomae commented 1 year ago

How about JsonPath.arrayString? I'm not sure if we can deprecate the old one - if it is used at all, then the new one won't actually work in the other usecase which most likely expects an array.

dbartholomae commented 11 months ago

This was discussed in the first CDK office hours with @mrgrain

Should this be a p1 as type assignments won't work in other languages?